Date:   Thu, 10 Dec 2020 11:56:17 +0000
From:   "Vaittinen, Matti" <Matti.Vaittinen@...rohmeurope.com>
To:     "yoshihiro.shimoda.uh@...esas.com" <yoshihiro.shimoda.uh@...esas.com>
CC:     "marek.vasut+renesas@...il.com" <marek.vasut+renesas@...il.com>,
        "geert+renesas@...der.be" <geert+renesas@...der.be>,
        "khiem.nguyen.xt@...esas.com" <khiem.nguyen.xt@...esas.com>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "lee.jones@...aro.org" <lee.jones@...aro.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

Hi Yoshihiro san,

On Thu, 2020-12-10 at 10:58 +0000, Yoshihiro Shimoda wrote:
> Hi Matti,
> 
> > From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 5:28 PM
> > 
> > On Tue, 2020-12-08 at 17:04 +0900, Yoshihiro Shimoda wrote:
> > > From: Khiem Nguyen <khiem.nguyen.xt@...esas.com>
> > > 
> > > The new PMIC BD9574MWF inherits features from BD9571MWV.
> > > Add the support of new PMIC to existing bd9571mwv driver.
> > > 
> > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@...esas.com>
> > > [shimoda: rebase and refactor]
> > > Signed-off-by: Yoshihiro Shimoda <
> > > yoshihiro.shimoda.uh@...esas.com>
> > > ---
> > >  drivers/mfd/bd9571mwv.c       | 92
> > > +++++++++++++++++++++++++++++++++++++++++++
> > >  include/linux/mfd/bd9571mwv.h | 80
> > > +++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 172 insertions(+)
> > > 
> > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> > > index 57bdb6a..f8f0a87 100644
> > > --- a/drivers/mfd/bd9571mwv.c
> > > +++ b/drivers/mfd/bd9571mwv.c
> > > @@ -20,6 +20,7 @@ static const struct mfd_cell bd9571mwv_cells[]
> > > = {
> > >  	{ .name = "bd9571mwv-gpio", },
> > >  };
> > > 
> > > +/* Regmap for BD9571MWV */
> > >  static const struct regmap_range bd9571mwv_readable_yes_ranges[]
> > > = {
> > >  	regmap_reg_range(BD9571MWV_VENDOR_CODE,
> > > BD9571MWV_PRODUCT_REVISION),
> > >  	regmap_reg_range(BD9571MWV_BKUP_MODE_CNT,
> > > BD9571MWV_BKUP_MODE_CNT),
> > > @@ -112,6 +113,95 @@ static const struct bd957x_data
> > > bd9571mwv_data =
> > > {
> > >  	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
> > >  };
> > > 
> > > +static const struct mfd_cell bd9574mwf_cells[] = {
> > > +	{ .name = "bd9571mwv-gpio", },
> > 
> > Another 'nit' suggestion which you can ignore if it does not make
> > sense
> > :)
> > 
> > Are the GPIO blocks 100% identical? If not, then I would suggest
> > changing this to:
> > { .name = "bd9574mwf-gpio", },
> 
> The GPIO blocks are not 100% identical. BD9574MWF seems to have
> an additional feature which GPIOx pin are used for other mode by
> using gpio mux regisiter.
> 
> > and populating the platform_driver id_table for sub driver(s) using
> > something like:
> > 
> > static const struct platform_device_id bd957x_gpio_id[] = {
> >         { "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
> >         { "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
> >         { },
> > };
> > 
> > Then you can get the IC type using
> > platform_get_device_id(pdev)->driver_data.
> 
> I got it. So, perhaps, I should add these types into
> include/linux/mfd/rohm-generic.h.

That would make sense to me. I like the idea of collecting the IDs in
same place - but on the other hand, the define in id-table is not
really visible outside the sub-driver - so you can probably also define
the type in sub-device driver if you wish. I don't have strong opinion
on that.
> 
> > Next, I think the parent data from MFD is only used to get the
> > regmap
> > and dev in sub-devices, right? Maybe you could simplify this and
> > get
> > rid of the whole MFD parent data structure? I think you can use
> > 
> > pdev->dev.parent to get the parent device and
> > dev_get_regmap(pdev->dev.parent, NULL);
> > 
> > to get the regmap?
> 
> IIUC, these comments are related to gpio-bd9571mwv.c.
> # Also, bd9571mwv-regulator.c?
> If so, I didn't try this yet, but perhaps, we can modify such things.

Correct. My suggestion was related to how regmap and dev pointers are
obtained in sub-devices. It is related to MFD because I think you could
remove the MFD driver data usage.

> 
> > (After this I wonder if you need the
> > struct bd9571mwv at all?)
> 
> I'm sorry, but I could not understand this.

I believe the struct bd9571mwv is defined only to collect all the MFD
driver data in one struct so that it can be passed to sub-drivers using
the MFD device private data. But as far as I can tell, the sub-devices
only use regmap and dev pointers from this data. If this is the case,
then I think there is no need to define this struct or populate the MFD
driver data (unless I am missing something).

(And as a further clen-up, one could probably switch from:
regmap_add_irq_chip
to
devm_regmap_add_irq_chip
and get rid of the remove function)

but as I said - these are only 'nit' comments and I am not insisting on
changing these. Especially since some of the comments are more related
to changing the original bd9571mwv than adding support for this new IC.
I just think one might be able to make this a little bit simpler :)

> <snip>
> > >  /**
> > >   * struct bd957x_data - internal data for the bd957x driver
> > >   *
> > 
> > Overall a good looking driver! Thanks a lot!
> 
> Thank you very much for your review!
> 
> By the way, I realized Linux kernel supports bd9576-regulator.c
> and it has "bd957x", but it doesn't seem to be compatible with
> BD9571.
> So, I wonder if I should not use "bd957x" in the bd9571mwv driver to
> avoid
> confusion. But, what do you think?

Valid point. I think BD9573 and BD9573 are close to each-others but
largely different from BD9571 and BD9574... Good example why wildcards
are so hard. I have previously attempted to use the wildcards in ROHM
PMIC software naming - and usually failed. The numbering does not
really seem to reflect the functionality... So maybe i am not the best
person to comment on this XD

On the other hand, sometimes we want to highlight that some of the
functions/defines are used by all IC versions a driver supports - while
others are IC specific. For example, inside the driver which supports
BD71837 and BD71847 I used BD718XX as a prefix for defines that were
common for both ICs. IC specific defines I named with BD71837 and
BD71847. I think that was quite clear inside the driver (until BD71850
was made - which uses same defines as BD71847... :| )

So... My suggestion - you can probably use wildcards inside a driver
(and comment things when wildcards do not match anymore). But I would
not add wildcards to any globally visible defines (in definitions
included from headers, file names, ...)

As Rob put it a while ago - "Naming is hard". :)

Best Regards
    Matti Vaittinen

Powered by blists - more mailing lists