RE: [PATCH 3/3] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>]

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

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

> (After this I wonder if you need the
> struct bd9571mwv at all?)

I'm sorry, but I could not understand this.

> > +};
> > +
> > +/* Regmap for BD9574MWF */
> > +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
> > +	regmap_reg_range(BD9574MWF_VENDOR_CODE,
> > BD9574MWF_PRODUCT_REVISION),
> > +	regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> > +	regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INTMASK),
> > +	regmap_reg_range(BD9574MWF_GPIO_MUX, BD9574MWF_GPIO_MUX),
> > +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTMASK),
> > +};
> > +
> > +static const struct regmap_access_table bd9574mwf_readable_table = {
> > +	.yes_ranges	= bd9574mwf_readable_yes_ranges,
> > +	.n_yes_ranges	= ARRAY_SIZE(bd9574mwf_readable_yes_ranges),
> > +};
> > +
> > +static const struct regmap_range bd9574mwf_writable_yes_ranges[] = {
> > +	regmap_reg_range(BD9574MWF_GPIO_DIR, BD9574MWF_GPIO_OUT),
> > +	regmap_reg_range(BD9574MWF_GPIO_INT_SET,
> > BD9574MWF_GPIO_INTMASK),
> > +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTMASK),
> > +};
> > +
> > +static const struct regmap_access_table bd9574mwf_writable_table = {
> > +	.yes_ranges	= bd9574mwf_writable_yes_ranges,
> > +	.n_yes_ranges	= ARRAY_SIZE(bd9574mwf_writable_yes_ranges),
> > +};
> > +
> > +static const struct regmap_range bd9574mwf_volatile_yes_ranges[] = {
> > +	regmap_reg_range(BD9574MWF_GPIO_IN, BD9574MWF_GPIO_IN),
> > +	regmap_reg_range(BD9574MWF_GPIO_INT, BD9574MWF_GPIO_INT),
> > +	regmap_reg_range(BD9574MWF_INT_INTREQ, BD9574MWF_INT_INTREQ),
> > +};
> 
> Are you using the other interrupts/statuses or VDCORE MoniVDAC? Should
> they be volatile too?

At least, since BD9571MWV_DVFS_MONIVDAC is used on bd9571mfv-regulator.c,
I should add it into the volatile.

<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?

Best regards,
Yoshihiro Shimoda