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

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

Hi Geert-san,

Thank you for your review!

> From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:30 PM
<snip> 
> > --- 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 */
> 
> Note that bd9571mwv_cells[] above also applies to BD9571MWV.

Yes, so, I'll move this comment.

> >  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", },
> 
> No regulator cell?

Oops, we should add regulator for bd9574mwf to use backup_mode and DVFS.
However, since BD9574MWF doesn't have AVS, we should add
" bd9574mwf-regulator", not "bd9571mwv-regulator".
# Your indicated web site said BD9574MWF supports AVS feature.
# But, "Application Circuit" doesn't have any AVS pins.
# Of course, the datasheet doesn't mention about AVS :)

> > +};
> > +
> > +/* Regmap for BD9574MWF */
> 
> Note that bd9574mwf_cells[] above also applies to BD9574MWF.
> Perhaps the comments should be changed slightly, and moved up,
> to serve as a separator between chip variants?

I think so. I'll fix it.

> > +static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
> > +       regmap_reg_range(BD9574MWF_VENDOR_CODE, BD9574MWF_PRODUCT_REVISION),
> 
> Missing BD9574MWF_BKUP_MODE_CNT and BD9574MWF_DVFS_*?

Yes, I'll add these.

> > +       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[] = {
> 
> Missing BD9574MWF_BKUP_MODE_CNT and BD9574MWF_DVFS_*?

Yes, I'll add these.

> > +       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),
> > +};
> 
> > @@ -182,6 +272,8 @@ static int bd9571mwv_probe(struct i2c_client *client,
> >         product_code = (unsigned int)ret;
> >         if (product_code == BD9571MWV_PRODUCT_CODE_VAL)
> >                 bd->data = &bd9571mwv_data;
> > +       else if (product_code == BD9574MWF_PRODUCT_CODE_VAL)
> > +               bd->data = &bd9574mwf_data;
> >
> >         if (!bd->data) {
> >                 dev_err(bd->dev, "No found supported device %d\n",
> 
> While BD9571MWV and BD9574MWF can be distinguished at runtime,
> I think it would still be a good idea to document a "rohm,bd9574mwf"
> compatible value in the DT bindings, and let the driver match on that.

In this driver point of view, we can use such the DT bindings,
however, in the board point of view, it's difficult to describe
which chip is installed on r8a77990-ebisu.dts. So, I'd like to
keep this runtime detection.

> > diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
> > index 0126b52..e9e219b 100644
> > --- a/include/linux/mfd/bd9571mwv.h
> > +++ b/include/linux/mfd/bd9571mwv.h
> 
> > +#define BD9574MWF_VDCORE_VINIT                 0x50
> > +#define BD9574MWF_VD09_VINIT                   0x51
> > +#define BD9574MWF_VDCORE_SETVMAX               0x52
> > +#define BD9574MWF_VDCORE_SETVID                        0x54
> > +#define BD9574MWF_VDCORE_MONIVDAC              0x55
> > +#define BD9574MWF_VDCORE_PGD_CNT               0x56
> 
> Some of the above are the same as the corresponding BD9571MWV
> registers, so using the same define may simplify regulator support
> (cfr. BD9571MWV_DVFS_SETVID and BD9571MWV_DVFS_MONIVDAC).

Indeed. I'll fix it.

> > +#define BD9574MWF_PART_NUMBER                  "BD9574MWF"
> 
> BD9574MWF_PART_NAME?

Yes, I'll rename it.

Best regards,
Yoshihiro Shimoda