Hi Shimoda-san, CC Matti (BD9573/6 driver for R-Car platforms) (I don't have the BD9574 datasheet, so I have to base my review on https://www.rohm.com/r-car-pmic) On Tue, Dec 8, 2020 at 9:06 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> 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> Thanks for your patch! > --- 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. > 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? > +}; > + > +/* 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? > +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_*? > + 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_*? > + 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. > 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). > +#define BD9574MWF_PART_NUMBER "BD9574MWF" BD9574MWF_PART_NAME? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
