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@xxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxxx>
> > > [shimoda: rebase and refactor]
> > > Signed-off-by: Yoshihiro Shimoda <
> > > yoshihiro.shimoda.uh@xxxxxxxxxxx>
> > > ---
> > > 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
