From: Lee Jones <lee@kernel.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Conor Dooley <conor+dt@kernel.org>,
Liam Girdwood <lgirdwood@gmail.com>,
Mark Brown <broonie@kernel.org>,
Wim Van Sebroeck <wim@linux-watchdog.org>,
Guenter Roeck <linux@roeck-us.net>,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-watchdog@vger.kernel.org
Subject: Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core
Date: Wed, 17 Apr 2024 13:24:54 +0100 [thread overview]
Message-ID: <20240417122454.GY2399047@google.com> (raw)
In-Reply-To: <700b63a1-ce91-4d91-9db7-43c195ba7a6f@gmail.com>
On Fri, 12 Apr 2024, Matti Vaittinen wrote:
> On 4/12/24 10:23, Lee Jones wrote:
> > On Fri, 12 Apr 2024, Matti Vaittinen wrote:
> >
> > > Hi deee Ho Lee!
> > >
> > > Thanks a ton for taking a look at this :) I already sent the V2 yesterday,
> > > briefly before receiving your comments. I think all of the comments are
> > > relevant for the V2 as well, I will fix them for the V3 when I get to that.
> > > If you find the time to take a look at V2, then the major things are
> > > addition of a watchdog IRQ + a work-around for the debugFS name collision
> > > for IRQ domains.
> > >
> > > On 4/11/24 17:38, Lee Jones wrote:
> > > > On Tue, 02 Apr 2024, Matti Vaittinen wrote:
> > > >
> > > > > The ROHM BD96801 PMIC is highly customizable automotive grade PMIC
> > > > > which integrates regulator and watchdog funtionalities.
> > > > >
> > > > > Provide IRQ and register accesses for regulator/watchdog drivers.
> > > > >
> > > > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > > > ---
> > > > > drivers/mfd/Kconfig | 13 +
> > > > > drivers/mfd/Makefile | 1 +
> > > > > drivers/mfd/rohm-bd96801.c | 454 +++++++++++++++++++++++++++++++
> > > > > include/linux/mfd/rohm-bd96801.h | 212 +++++++++++++++
> > > > > include/linux/mfd/rohm-generic.h | 1 +
> > > > > 5 files changed, 681 insertions(+)
> > > > > create mode 100644 drivers/mfd/rohm-bd96801.c
> > > > > create mode 100644 include/linux/mfd/rohm-bd96801.h
> > > > >
> > > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > > > > index 4b023ee229cf..947045eb3a8e 100644
> > > > > --- a/drivers/mfd/Kconfig
> > > > > +++ b/drivers/mfd/Kconfig
> > > > > @@ -2089,6 +2089,19 @@ config MFD_ROHM_BD957XMUF
> > > > > BD9573MUF Power Management ICs. BD9576 and BD9573 are primarily
> > > > > designed to be used to power R-Car series processors.
> > > > > +config MFD_ROHM_BD96801
> > > > > + tristate "ROHM BD96801 Power Management IC"
> > > > > + depends on I2C=y
> > > > > + depends on OF
> > > > > + select REGMAP_I2C
> > > > > + select REGMAP_IRQ
> > > > > + select MFD_CORE
> > > > > + help
> > > > > + Select this option to get support for the ROHM BD96801 Power
> > > > > + Management IC. The ROHM BD96801 is a highly scalable power management
> > > >
> > > > Power Management
> > >
> > > Out of the curiosity, why is the "Power Management IC" written with
> > > capitals, when speaking of a class of devices instead of a model? (I am 100%
> > > fine with the change, just curious).
> >
> > It's no different to how its expressed in the tristate section above.
> >
> > Power Management IC or PMIC.
> >
> > "provides power management capabilities" describes its function?
> >
> > "is a scalable Power Management IC", describes the device?
> >
> > But actually, it just looks odd when both are used in the same section.
> >
> > /me likes uniformity and consistency.
>
> It's okay, thanks for the explanation :)
>
> > > > > + IC for industrial and automotive use. The BD96801 can be used as a
> > > > > + master PMIC in a chained PMIC solutions with suitable companion PMICs
> > > ...
> > >
> > > > > +static int bd96801_i2c_probe(struct i2c_client *i2c)
> > > > > +{
> > > > > + int i, ret, intb_irq, errb_irq, num_regu_irqs, num_intb, num_errb = 0;
> > > > > + struct regmap_irq_chip_data *intb_irq_data, *errb_irq_data;
> > > > > + struct irq_domain *intb_domain, *errb_domain;
> > > > > + const struct fwnode_handle *fwnode;
> > > > > + struct resource *regulator_res;
> > > > > + struct regmap *regmap;
> > > > > +
> > > > > + fwnode = dev_fwnode(&i2c->dev);
> > > > > + if (!fwnode) {
> > > > > + dev_err(&i2c->dev, "no fwnode\n");
> > > > > + return -EINVAL;
> > > >
> > > > Why not dev_err_probe() here for uniformity?
> > >
> > > I can change it to dev_err_probe() if it's strongly preferred. It just feels
> > > silly to use dev_err_probe() when the return value is hardcoded.
> >
> > Not at all:
> >
> > git grep dev_err_probe | grep "\-[A-Z]"
>
> Yes, I know people do use the dev_err_probe() with hardcoded errors but it
> does not make me feel any better about it :)
<look into my swirling eyes> Uniformity within the function!
> > > Intentionally writing code like
> > >
> > > err = -EINVAL;
> > > if (err == ...)
> > >
> > > just makes me feel a bit sick.
> >
> > Why would you want to do that?
>
> This is what the dev_err_probe() with a hardcoded err does, right?
>
> int dev_err_probe(const struct device *dev, int err, const char *fmt, ...)
> {
> ...
> if (err != -EPROBE_DEFER) {
> dev_err(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> } else {
> device_set_deferred_probe_reason(dev, &vaf);
> dev_dbg(dev, "error %pe: %pV", ERR_PTR(err), &vaf);
> }
> ...
> }
Attempt to purge this info from you brain!
> > > > > + }
> > > > > +
> > > > > + intb_irq = fwnode_irq_get_byname(fwnode, "intb");
> > > > > + if (intb_irq < 0)
> > > > > + return dev_err_probe(&i2c->dev, intb_irq,
> > > > > + "No INTB IRQ configured\n");
> > > >
> > > > This function would look nicer if you expanded to 100-chars.
> > >
> > > The reason why I still prefer the good old 80-chars for files I work with,
> > > is that I am often having 3 terminal windows parallel on my laptop screen.
> > > (Or, when I have my wide mofnitor connected it is 3 editor windows +
> > > minicom). I need to keep the terminals small enough. Besides... I hate to
> > > admit this, but the time is finally taking it's toll. My eyes aren't quite
> > > the same they were 2 years ago...
> >
> > Upgrade your 14" CRT monitor to something more modern. :)
>
> But those things were built to last! And throwing away perfectly working
> stuff... :)
Can't argue with that! Maybe put 2 side-by-side or 4 in a matrix!
--
Lee Jones [李琼斯]
next prev parent reply other threads:[~2024-04-17 12:25 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-02 13:07 [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC Matti Vaittinen
2024-04-02 13:07 ` [RFC PATCH 1/6] dt-bindings: ROHM BD96801 PMIC regulators Matti Vaittinen
2024-04-02 13:08 ` [RFC PATCH 2/6] dt-bindings: mfd: bd96801 PMIC core Matti Vaittinen
2024-04-02 13:08 ` [RFC PATCH 3/6] mfd: support ROHM BD96801 " Matti Vaittinen
2024-04-11 14:38 ` Lee Jones
2024-04-12 5:40 ` Matti Vaittinen
2024-04-12 5:50 ` Matti Vaittinen
2024-04-12 7:23 ` Lee Jones
2024-04-12 8:58 ` Matti Vaittinen
2024-04-17 12:24 ` Lee Jones [this message]
2024-04-02 13:11 ` [RFC PATCH 5/6] watchdog: ROHM BD96801 PMIC WDG driver Matti Vaittinen
2024-04-02 16:15 ` Krzysztof Kozlowski
2024-04-02 17:11 ` Guenter Roeck
2024-04-03 6:34 ` Matti Vaittinen
2024-04-03 12:41 ` Guenter Roeck
2024-04-03 12:47 ` Matti Vaittinen
2024-04-03 13:26 ` Guenter Roeck
2024-04-02 13:12 ` [RFC PATCH 6/6] MAINTAINERS: Add ROHM BD96801 'scalable PMIC' entries Matti Vaittinen
2024-04-04 7:26 ` [RFC PATCH 0/6] Support ROHM BD96801 scalable PMIC Matti Vaittinen
2024-04-04 12:09 ` Mark Brown
2024-04-04 13:15 ` Matti Vaittinen
2024-04-05 9:19 ` Matti Vaittinen
2024-04-05 21:27 ` Mark Brown
2024-04-22 10:52 ` Matti Vaittinen
2024-05-09 5:08 ` Mark Brown
2024-05-09 7:03 ` Matti Vaittinen
2024-05-09 15:38 ` Mark Brown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240417122454.GY2399047@google.com \
--to=lee@kernel.org \
--cc=broonie@kernel.org \
--cc=conor+dt@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=lgirdwood@gmail.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=matti.vaittinen@fi.rohmeurope.com \
--cc=mazziesaccount@gmail.com \
--cc=robh@kernel.org \
--cc=wim@linux-watchdog.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).