Re: [RFC PATCH 3/6] mfd: support ROHM BD96801 PMIC core

[Matti Vaittinen]

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).

> > +	  IC for industrial and automotive use. The BD96801 can be used as a
> > +	  master PMIC in a chained PMIC solutions with suitable companion PMICs
..

> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// Copyright (C) 2022 ROHM Semiconductors
>
> No updates for 2 years?

The year should be updated - thanks. But, now that you asked...  Almost 
no updates. The patches have rotten in my outbox, waiting for the 
permisson to be sent out... But yeah, I've sure added some changes 
before sending the series - I'll update the copyright :)

> > +
> > +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. 
Intentionally writing code like

err = -EINVAL;
if (err == ...)

just makes me feel a bit sick.

> > +	}
> > +
> > +	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...

So, same old story, I can change this if it is important enough for 
others, but personally I rather work with the short lines.

..

> > diff --git a/include/linux/mfd/rohm-bd96801.h b/include/linux/mfd/rohm-bd96801.h
> > new file mode 100644
> > index 000000000000..47b07171dcb2
> > --- /dev/null
> > +++ b/include/linux/mfd/rohm-bd96801.h
> > @@ -0,0 +1,212 @@
> > +/* SPDX-License-Identifier: GPL-2.0-or-later */
> > +/* Copyright (C) 2020 ROHM Semiconductors */
> > +

..

> > +/* IRQ register area */
> > +#define BD96801_REG_INT_MAIN		0x51
> > +
> > +/*
> > + * The BD96801 has two physical IRQ lines, INTB and ERRB.
> > + * For now we just handle the INTB.

Note to self, this comment is no longer true.

Thanks for the review!

Yours,
	-- Matti


--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland

~~ When things go utterly wrong vim users can always type :help! ~~