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