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@xxxxxxxxx>
> > > > > ---
> > > > > 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 [李琼斯]
