Re: [PATCH 05/14] mfd: rohm-bd96801: Add chip info

[Lee Jones <lee@kernel.org>]

On Fri, 21 Mar 2025, Matti Vaittinen wrote:

> On 20/03/2025 18:52, Lee Jones wrote:
> > On Thu, 13 Mar 2025, Matti Vaittinen wrote:
> > 
> > > Prepare for adding support for BD96802 which is very similar to BD96801.
> > > Separate chip specific data into own structure which can be picked to be
> > > used by the device-tree.
> > > 
> > > Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
> > > ---
> > >   drivers/mfd/rohm-bd96801.c | 83 ++++++++++++++++++++++++++------------
> > >   1 file changed, 57 insertions(+), 26 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/rohm-bd96801.c b/drivers/mfd/rohm-bd96801.c
> > > index 60ec8db790a7..1232f571e4b1 100644
> > > --- a/drivers/mfd/rohm-bd96801.c
> > > +++ b/drivers/mfd/rohm-bd96801.c
> > > @@ -40,7 +40,21 @@
> > >   #include <linux/mfd/rohm-bd96801.h>
> > >   #include <linux/mfd/rohm-generic.h>
> > > -static const struct resource regulator_errb_irqs[] = {
> > > +struct bd968xx_chip_data {
> > > +	const struct resource *errb_irqs;
> > > +	const struct resource *intb_irqs;
> > > +	int num_errb_irqs;
> > > +	int num_intb_irqs;
> > > +	const struct regmap_irq_chip *errb_irq_chip;
> > > +	const struct regmap_irq_chip *intb_irq_chip;
> > > +	const struct regmap_config *regmap_config;
> > > +	struct mfd_cell *cells;
> > 
> > We're not passing MFD data through OF to be fed back through MFD APIs.
> > 
> > It's generally considered better to device_get_match_data() on an enum,
> > then populate MFD cells using that as a differentiator.
> 
> Or, at least someone has done this at the beginning and it got copied all
> over the place, right? ;) Sometimes we just need to challenge the status quo
> to develop ;)

This is not one of those times.

I've never allowed mixing platform init strategies (arch-plat, OF, ACPI, MFD).

> I can go back to enum + switch - case in probe, and pick the correct data
> there. Done that before as well. It's just that during my journey to some
> other subsystems, I've realized people can often just skip the enum and
> switch - case, making things a tad simpler :)
> 
> Well, not a big deal to me. I suppose it has some value to keep things
> consistent inside a subsystem - and I'm not offering to drop the switch
> cases from all of the drivers :p
> 
> TL; DR - Ok.
> 
> >    git grep compatible -- drivers/mfd | grep data
> > 
> > > +	int num_cells;
> > > +	int unlock_reg;
> > > +	int unlock_val;
> > > +};
> > > +
> > > +static const struct resource bd96801_reg_errb_irqs[] = {
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_OTP_ERR_STAT, "bd96801-otp-err"),
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_DBIST_ERR_STAT, "bd96801-dbist-err"),
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_EEP_ERR_STAT, "bd96801-eep-err"),
> > > @@ -98,7 +112,7 @@ static const struct resource regulator_errb_irqs[] = {
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_LDO7_SHDN_ERR_STAT, "bd96801-ldo7-shdn-err"),
> > >   };
> > > -static const struct resource regulator_intb_irqs[] = {
> > > +static const struct resource bd96801_reg_intb_irqs[] = {
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_TW_STAT, "bd96801-core-thermal"),
> > >   	DEFINE_RES_IRQ_NAMED(BD96801_BUCK1_OCPH_STAT, "bd96801-buck1-overcurr-h"),
> > > @@ -345,18 +359,37 @@ static const struct regmap_config bd96801_regmap_config = {
> > >   	.cache_type = REGCACHE_MAPLE,
> > >   };
> > > +static const struct bd968xx_chip_data bd96801_chip_data = {
> > 
> > Just call it 'struct bd968xx' then below instead of cd, use ddata.
> > 
> >    git grep "cc =" -- drivers/mfd
> > 
> > VS
> > 
> >    git grep "ddata =" -- drivers/mfd
> > 
> > Conforrrrrrmmm ...    =;-)
> 
> I've lived through the depression of the early 90's in Finland. Learned how
> to avoid wasting things - especially letters. Wouldn't guess when reading my
> review replies, right?
> 
> ...Ok.
> 
> Thanks for the review :) Much appreciated.

NP

-- 
Lee Jones [李琼斯]