| Message ID | 1607414643-25498-4-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | mfd: bd9571mwv: Add support for BD9574MWF | expand |
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@renesas.com> 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> 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@linux-m68k.org 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
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
Hi deee Ho Yoshihiro-san, Geert, All, On Wed, 2020-12-09 at 14:30 +0100, Geert Uytterhoeven wrote: > Hi Shimoda-san, > > CC Matti (BD9573/6 driver for R-Car platforms) Thank Geert! I wouldn't have noticed this :) > > (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@renesas.com> 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> > > Thanks for your patch! > Indeed! It's really nice to see other people working on upstreaming drivers for ROHM PMICs! Thanks for all the good work! Can you please add me to CC if you ever resend the series? I like to know what kind of support there is added in-tree for ROHM PMICs :) Best Regards Matti -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote: > 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 > > > > > > @@ -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. First of all - I don't want to insist changes here so my comment can be ignored. I would definitely like seeing the support for BD9574 in-tree! But as a 'nit': I don't know what are the difficulties you are referring to so it's hard for me to comment this. Without better understanding of board dts files - I think BD9574MWF should really have own compatible as I understood it is different from the BD9571MWV. Relying on product code probing sounds like something that may easily break when/if new variants are produced. ( I've seen new HW variants using the same ID information being produced in previous companies I've worked. Sure ROHM wouldn't do this but still... :] ). And producing boards where DTS does not allow describing the correct components sounds like asking for a nose-bleed to me... If probing of IC type fails AND there is devices with wrong PMIC information burned in DT - then fixing it can be a nightmare. So I would really try make DTS files such that they can be changed to match the actual board. (Perhaps introduce the compatible for BD9574MWF - make this driver to match both of the PMICs - leave the runtime probing here for now - and in parallel work with the DTS files so that eventually the probing can be removed(?) That was my 10 cents on this topic :] ) Best Regards Matti Vaittinen
Hi Matti, Shimoda-san, On Thu, Dec 10, 2020 at 8:33 AM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote: > > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:30 > > > PM > > <snip> > > > > --- a/drivers/mfd/bd9571mwv.c > > > > +++ b/drivers/mfd/bd9571mwv.c > > > > > > > > @@ -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. To clarify: I meant to document and add the compatible value, but treat both compatible values the same in the driver, and still do runtime probing. > First of all - I don't want to insist changes here so my comment can be > ignored. I would definitely like seeing the support for BD9574 in-tree! > > But as a 'nit': > I don't know what are the difficulties you are referring to so it's > hard for me to comment this. Without better understanding of board dts > files - I think BD9574MWF should really have own compatible as I > understood it is different from the BD9571MWV. Relying on product code > probing sounds like something that may easily break when/if new > variants are produced. ( I've seen new HW variants using the same > ID information being produced in previous companies I've worked. Sure Yes, this happens from time to time, unfortunately. > ROHM wouldn't do this but still... :] ). And producing boards where DTS > does not allow describing the correct components sounds like asking for > a nose-bleed to me... If probing of IC type fails AND there is devices > with wrong PMIC information burned in DT - then fixing it can be a > nightmare. So I would really try make DTS files such that they can be The issue we're facing is that older Ebisu-4D boards have BD9571, while newer boards have BD9574. The schematics say "BD9574MWF-M (tentative ver:BD9571TL1_E3)", so it looks like both parts are pin-compatible (ignoring missing pins for AVS, LDO1, LDO2, and LDO6 on BD9574). Hence arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts has a device node compatible with "rohm,bd9571mwv". If we have runtime probing, we can keep on using that for both variants. > changed to match the actual board. (Perhaps introduce the compatible > for BD9574MWF - make this driver to match both of the PMICs - leave the > runtime probing here for now - and in parallel work with the DTS files > so that eventually the probing can be removed(?) That was my 10 cents > on this topic :] ) Exactly. Thanks! Gr{oetje,eeting}s, Geert
Hi Geert-san, Matti, Thank you for your comments! > From: Geert Uytterhoeven, Sent: Thursday, December 10, 2020 5:20 PM > > Hi Matti, Shimoda-san, > > On Thu, Dec 10, 2020 at 8:33 AM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote: > > > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 10:30 > > > > PM > > > <snip> > > > > > --- a/drivers/mfd/bd9571mwv.c > > > > > +++ b/drivers/mfd/bd9571mwv.c > > > > > > > > > > @@ -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. > > To clarify: I meant to document and add the compatible value, but > treat both compatible values the same in the driver, and still do runtime > probing. Thank you! I understood it. <snip> > > ROHM wouldn't do this but still... :] ). And producing boards where DTS > > does not allow describing the correct components sounds like asking for > > a nose-bleed to me... If probing of IC type fails AND there is devices > > with wrong PMIC information burned in DT - then fixing it can be a > > nightmare. So I would really try make DTS files such that they can be > > The issue we're facing is that older Ebisu-4D boards have BD9571, while > newer boards have BD9574. The schematics say "BD9574MWF-M (tentative > ver:BD9571TL1_E3)", so it looks like both parts are pin-compatible > (ignoring missing pins for AVS, LDO1, LDO2, and LDO6 on BD9574). > Hence arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts has a device node > compatible with "rohm,bd9571mwv". If we have runtime probing, we can > keep on using that for both variants. Thank you very much for explaining this! It's very clear :) Best regards, Yoshihiro Shimoda
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", }, 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. 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? (After this I wonder if you need the struct bd9571mwv at all?) > +}; > + > +/* 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? > + > +static const struct regmap_access_table bd9574mwf_volatile_table = { > + .yes_ranges = bd9574mwf_volatile_yes_ranges, > + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_volatile_yes_ranges), > +}; > + > +static const struct regmap_config bd9574mwf_regmap_config = { > + .reg_bits = 8, > + .val_bits = 8, > + .cache_type = REGCACHE_RBTREE, > + .rd_table = &bd9574mwf_readable_table, > + .wr_table = &bd9574mwf_writable_table, > + .volatile_table = &bd9574mwf_volatile_table, > + .max_register = 0xff, > +}; > + > +static const struct regmap_irq bd9574mwf_irqs[] = { > + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD1, 0, > + BD9574MWF_INT_INTREQ_MD1_INT), > + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E1, 0, > + BD9574MWF_INT_INTREQ_MD2_E1_INT), > + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E2, 0, > + BD9574MWF_INT_INTREQ_MD2_E2_INT), > + REGMAP_IRQ_REG(BD9574MWF_IRQ_PROT_ERR, 0, > + BD9574MWF_INT_INTREQ_PROT_ERR_INT), > + REGMAP_IRQ_REG(BD9574MWF_IRQ_GP, 0, > + BD9574MWF_INT_INTREQ_GP_INT), > + REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_HOLD_OF, 0, > + BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT), > + REGMAP_IRQ_REG(BD9574MWF_IRQ_WDT_OF, 0, > + BD9574MWF_INT_INTREQ_WDT_OF_INT), > + REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_TRG, 0, > + BD9574MWF_INT_INTREQ_BKUP_TRG_INT), > +}; > + > +static struct regmap_irq_chip bd9574mwf_irq_chip = { > + .name = "bd9574mwf", > + .status_base = BD9574MWF_INT_INTREQ, > + .mask_base = BD9574MWF_INT_INTMASK, > + .ack_base = BD9574MWF_INT_INTREQ, > + .init_ack_masked = true, > + .num_regs = 1, > + .irqs = bd9574mwf_irqs, > + .num_irqs = ARRAY_SIZE(bd9574mwf_irqs), > +}; > + > +static const struct bd957x_data bd9574mwf_data = { > + .product_code_val = BD9574MWF_PRODUCT_CODE_VAL, > + .part_number = BD9574MWF_PART_NUMBER, > + .regmap_config = &bd9574mwf_regmap_config, > + .irq_chip = &bd9574mwf_irq_chip, > + .cells = bd9574mwf_cells, > + .num_cells = ARRAY_SIZE(bd9574mwf_cells), > +}; > + > static int bd9571mwv_identify(struct bd9571mwv *bd) > { > struct device *dev = bd->dev; > @@ -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", > 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 > @@ -99,6 +99,86 @@ enum bd9571mwv_irqs { > BD9571MWV_IRQ_BKUP_TRG, > }; > > +/* List of registers for BD9574MWF */ > +#define BD9574MWF_VENDOR_CODE 0x00 > +#define BD9574MWF_VENDOR_CODE_VAL 0xdb > +#define BD9574MWF_PRODUCT_CODE 0x01 > +#define BD9574MWF_PRODUCT_CODE_VAL 0x74 > +#define BD9574MWF_PRODUCT_REVISION 0x02 > + > +#define BD9574MWF_I2C_FUSA_MODE 0x10 > +#define BD9574MWF_I2C_MD2_E1_BIT_1 0x11 > +#define BD9574MWF_I2C_MD2_E1_BIT_2 0x12 > + > +#define BD9574MWF_BKUP_MODE_CNT 0x20 > +#define BD9574MWF_BKUP_MODE_STATUS 0x21 > +#define BD9574MWF_BKUP_RECOVERY_CNT 0x22 > +#define BD9574MWF_BKUP_CTRL_TIM_CNT 0x23 > +#define BD9574MWF_WAITBKUP_WDT_CNT 0x24 > +#define BD9574MWF_BKUP_HOLD_TIM_CNT1 0x26 > +#define BD9574MWF_QLLM_CNT 0x27 > +#define BD9574MWF_BKUP_HOLD_TIM_CNT2 0x28 > + > +#define BD9574MWF_DCDC_FREQ 0x48 > + > +#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 > + > +#define BD9574MWF_GPIO_DIR 0x60 > +#define BD9574MWF_GPIO_OUT 0x61 > +#define BD9574MWF_GPIO_IN 0x62 > +#define BD9574MWF_GPIO_DEB 0x63 > +#define BD9574MWF_GPIO_INT_SET 0x64 > +#define BD9574MWF_GPIO_INT 0x65 > +#define BD9574MWF_GPIO_INTMASK 0x66 > +#define BD9574MWF_GPIO_MUX 0x67 > + > +#define BD9574MWF_REG_KEEP(n) (0x70 + (n)) > + > +#define BD9574MWF_PMIC_INTERNAL_STATUS 0x80 > +#define BD9574MWF_PROT_ERROR_STATUS0 0x81 > +#define BD9574MWF_PROT_ERROR_STATUS1 0x82 > +#define BD9574MWF_PROT_ERROR_STATUS2 0x83 > +#define BD9574MWF_PROT_ERROR_STATUS3 0x84 > +#define BD9574MWF_PROT_ERROR_STATUS4 0x85 > +#define BD9574MWF_PROT_ERROR_STATUS5 0x86 > +#define BD9574MWF_SYS_ERROR_STATUS 0x87 > + > +#define BD9574MWF_INT_INTREQ 0x90 > +#define BD9574MWF_INT_INTREQ_MD1_INT BIT(0) > +#define BD9574MWF_INT_INTREQ_MD2_E1_INT BIT(1) > +#define BD9574MWF_INT_INTREQ_MD2_E2_INT BIT(2) > +#define BD9574MWF_INT_INTREQ_PROT_ERR_INT BIT(3) > +#define BD9574MWF_INT_INTREQ_GP_INT BIT(4) > +#define BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT BIT(5) > +#define BD9574MWF_INT_INTREQ_WDT_OF_INT BIT(6) > +#define BD9574MWF_INT_INTREQ_BKUP_TRG_INT BIT(7) > +#define BD9574MWF_INT_INTMASK 0x91 > + > +#define BD9574MWF_SSCG_CNT 0xA0 > +#define BD9574MWF_POFFB_MRB 0xA1 > +#define BD9574MWF_SMRB_WR_PROT 0xA2 > +#define BD9574MWF_SMRB_ASSERT 0xA3 > +#define BD9574MWF_SMRB_STATUS 0xA4 > + > +#define BD9574MWF_PART_NUMBER "BD9574MWF" > + > +/* Define the BD9574MWF IRQ numbers */ > +enum bd9574mwf_irqs { > + BD9574MWF_IRQ_MD1, > + BD9574MWF_IRQ_MD2_E1, > + BD9574MWF_IRQ_MD2_E2, > + BD9574MWF_IRQ_PROT_ERR, > + BD9574MWF_IRQ_GP, > + BD9574MWF_IRQ_BKUP_HOLD_OF, > + BD9574MWF_IRQ_WDT_OF, > + BD9574MWF_IRQ_BKUP_TRG, > +}; > + > /** > * struct bd957x_data - internal data for the bd957x driver > * Overall a good looking driver! Thanks a lot! Best Regards Matti -- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND ~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. "non cogito me" dixit Rene Descarte, deinde evanescavit (Thanks for the translation Simon)
On Thu, 2020-12-10 at 09:19 +0100, Geert Uytterhoeven wrote: > Hi Matti, Shimoda-san, > > On Thu, Dec 10, 2020 at 8:33 AM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > On Thu, 2020-12-10 at 04:44 +0000, Yoshihiro Shimoda wrote: > > > > From: Geert Uytterhoeven, Sent: Wednesday, December 9, 2020 > > > > 10:30 > > > > PM > > > <snip> > > > > > --- a/drivers/mfd/bd9571mwv.c > > > > > +++ b/drivers/mfd/bd9571mwv.c > > > > > > > > > > @@ -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. > > To clarify: I meant to document and add the compatible value, but > treat both compatible values the same in the driver, and still do > runtime > probing. > > > First of all - I don't want to insist changes here so my comment > > can be > > ignored. I would definitely like seeing the support for BD9574 in- > > tree! > > > > But as a 'nit': > > I don't know what are the difficulties you are referring to so it's > > hard for me to comment this. Without better understanding of board > > dts > > files - I think BD9574MWF should really have own compatible as I > > understood it is different from the BD9571MWV. Relying on product > > code > > probing sounds like something that may easily break when/if new > > variants are produced. ( I've seen new HW variants using the same > > ID information being produced in previous companies I've worked. > > Sure > > Yes, this happens from time to time, unfortunately. > > > ROHM wouldn't do this but still... :] ). And producing boards where > > DTS > > does not allow describing the correct components sounds like asking > > for > > a nose-bleed to me... If probing of IC type fails AND there is > > devices > > with wrong PMIC information burned in DT - then fixing it can be a > > nightmare. So I would really try make DTS files such that they can > > be > > The issue we're facing is that older Ebisu-4D boards have BD9571, > while > newer boards have BD9574. The schematics say "BD9574MWF-M (tentative > ver:BD9571TL1_E3)", so it looks like both parts are pin-compatible > (ignoring missing pins for AVS, LDO1, LDO2, and LDO6 on BD9574). > Hence arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts has a device > node > compatible with "rohm,bd9571mwv". If we have runtime probing, we can > keep on using that for both variants. Thank you for the explanation :) This is a nice learning experience for me! > > changed to match the actual board. (Perhaps introduce the > > compatible > > for BD9574MWF - make this driver to match both of the PMICs - leave > > the > > runtime probing here for now - and in parallel work with the DTS > > files > > so that eventually the probing can be removed(?) That was my 10 > > cents > > on this topic :] ) > > Exactly. Thanks! I am more than happy with this solution :) --Matti
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
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@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. 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
Hi Matti-san, > From: Vaittinen, Matti, Sent: Thursday, December 10, 2020 8:56 PM > > 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: <snip> > > > 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. I got it. ROHM_CHIP_TYPE_BD957[14] are used from both gpio and regulator drivers so that adding IDs into rohm-generic.h is reasonable. > > > 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. I understood it. > > > (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) Thank you for the detail. I understood it. > 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 :) I got it :) For now, I would like to focus adding BD9574MWF support at first. After that, I'll try to clean-up these drivers later. > > <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, ...) Thank you for the suggestion! I think it's a good idea. So, I'll move struct bd957x_data from the header to MFD driver. > As Rob put it a while ago - "Naming is hard". :) I think so :) Best regards, Yoshihiro Shimoda
Hi again, > From: Yoshihiro Shimoda, Sent: Friday, December 11, 2020 9:49 AM > <snip> > > 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 :) > > I got it :) For now, I would like to focus adding BD9574MWF support at first. > After that, I'll try to clean-up these drivers later. JFYI. I submitted v2 patch series with this clean up because this is needed to avoid "struct bd957x_data" in the include/linux/mfd/bd9571mwv.h. Best regards, Yoshihiro Shimoda
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", }, +}; + +/* 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), +}; + +static const struct regmap_access_table bd9574mwf_volatile_table = { + .yes_ranges = bd9574mwf_volatile_yes_ranges, + .n_yes_ranges = ARRAY_SIZE(bd9574mwf_volatile_yes_ranges), +}; + +static const struct regmap_config bd9574mwf_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .cache_type = REGCACHE_RBTREE, + .rd_table = &bd9574mwf_readable_table, + .wr_table = &bd9574mwf_writable_table, + .volatile_table = &bd9574mwf_volatile_table, + .max_register = 0xff, +}; + +static const struct regmap_irq bd9574mwf_irqs[] = { + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD1, 0, + BD9574MWF_INT_INTREQ_MD1_INT), + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E1, 0, + BD9574MWF_INT_INTREQ_MD2_E1_INT), + REGMAP_IRQ_REG(BD9574MWF_IRQ_MD2_E2, 0, + BD9574MWF_INT_INTREQ_MD2_E2_INT), + REGMAP_IRQ_REG(BD9574MWF_IRQ_PROT_ERR, 0, + BD9574MWF_INT_INTREQ_PROT_ERR_INT), + REGMAP_IRQ_REG(BD9574MWF_IRQ_GP, 0, + BD9574MWF_INT_INTREQ_GP_INT), + REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_HOLD_OF, 0, + BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT), + REGMAP_IRQ_REG(BD9574MWF_IRQ_WDT_OF, 0, + BD9574MWF_INT_INTREQ_WDT_OF_INT), + REGMAP_IRQ_REG(BD9574MWF_IRQ_BKUP_TRG, 0, + BD9574MWF_INT_INTREQ_BKUP_TRG_INT), +}; + +static struct regmap_irq_chip bd9574mwf_irq_chip = { + .name = "bd9574mwf", + .status_base = BD9574MWF_INT_INTREQ, + .mask_base = BD9574MWF_INT_INTMASK, + .ack_base = BD9574MWF_INT_INTREQ, + .init_ack_masked = true, + .num_regs = 1, + .irqs = bd9574mwf_irqs, + .num_irqs = ARRAY_SIZE(bd9574mwf_irqs), +}; + +static const struct bd957x_data bd9574mwf_data = { + .product_code_val = BD9574MWF_PRODUCT_CODE_VAL, + .part_number = BD9574MWF_PART_NUMBER, + .regmap_config = &bd9574mwf_regmap_config, + .irq_chip = &bd9574mwf_irq_chip, + .cells = bd9574mwf_cells, + .num_cells = ARRAY_SIZE(bd9574mwf_cells), +}; + static int bd9571mwv_identify(struct bd9571mwv *bd) { struct device *dev = bd->dev; @@ -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", 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 @@ -99,6 +99,86 @@ enum bd9571mwv_irqs { BD9571MWV_IRQ_BKUP_TRG, }; +/* List of registers for BD9574MWF */ +#define BD9574MWF_VENDOR_CODE 0x00 +#define BD9574MWF_VENDOR_CODE_VAL 0xdb +#define BD9574MWF_PRODUCT_CODE 0x01 +#define BD9574MWF_PRODUCT_CODE_VAL 0x74 +#define BD9574MWF_PRODUCT_REVISION 0x02 + +#define BD9574MWF_I2C_FUSA_MODE 0x10 +#define BD9574MWF_I2C_MD2_E1_BIT_1 0x11 +#define BD9574MWF_I2C_MD2_E1_BIT_2 0x12 + +#define BD9574MWF_BKUP_MODE_CNT 0x20 +#define BD9574MWF_BKUP_MODE_STATUS 0x21 +#define BD9574MWF_BKUP_RECOVERY_CNT 0x22 +#define BD9574MWF_BKUP_CTRL_TIM_CNT 0x23 +#define BD9574MWF_WAITBKUP_WDT_CNT 0x24 +#define BD9574MWF_BKUP_HOLD_TIM_CNT1 0x26 +#define BD9574MWF_QLLM_CNT 0x27 +#define BD9574MWF_BKUP_HOLD_TIM_CNT2 0x28 + +#define BD9574MWF_DCDC_FREQ 0x48 + +#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 + +#define BD9574MWF_GPIO_DIR 0x60 +#define BD9574MWF_GPIO_OUT 0x61 +#define BD9574MWF_GPIO_IN 0x62 +#define BD9574MWF_GPIO_DEB 0x63 +#define BD9574MWF_GPIO_INT_SET 0x64 +#define BD9574MWF_GPIO_INT 0x65 +#define BD9574MWF_GPIO_INTMASK 0x66 +#define BD9574MWF_GPIO_MUX 0x67 + +#define BD9574MWF_REG_KEEP(n) (0x70 + (n)) + +#define BD9574MWF_PMIC_INTERNAL_STATUS 0x80 +#define BD9574MWF_PROT_ERROR_STATUS0 0x81 +#define BD9574MWF_PROT_ERROR_STATUS1 0x82 +#define BD9574MWF_PROT_ERROR_STATUS2 0x83 +#define BD9574MWF_PROT_ERROR_STATUS3 0x84 +#define BD9574MWF_PROT_ERROR_STATUS4 0x85 +#define BD9574MWF_PROT_ERROR_STATUS5 0x86 +#define BD9574MWF_SYS_ERROR_STATUS 0x87 + +#define BD9574MWF_INT_INTREQ 0x90 +#define BD9574MWF_INT_INTREQ_MD1_INT BIT(0) +#define BD9574MWF_INT_INTREQ_MD2_E1_INT BIT(1) +#define BD9574MWF_INT_INTREQ_MD2_E2_INT BIT(2) +#define BD9574MWF_INT_INTREQ_PROT_ERR_INT BIT(3) +#define BD9574MWF_INT_INTREQ_GP_INT BIT(4) +#define BD9574MWF_INT_INTREQ_BKUP_HOLD_OF_INT BIT(5) +#define BD9574MWF_INT_INTREQ_WDT_OF_INT BIT(6) +#define BD9574MWF_INT_INTREQ_BKUP_TRG_INT BIT(7) +#define BD9574MWF_INT_INTMASK 0x91 + +#define BD9574MWF_SSCG_CNT 0xA0 +#define BD9574MWF_POFFB_MRB 0xA1 +#define BD9574MWF_SMRB_WR_PROT 0xA2 +#define BD9574MWF_SMRB_ASSERT 0xA3 +#define BD9574MWF_SMRB_STATUS 0xA4 + +#define BD9574MWF_PART_NUMBER "BD9574MWF" + +/* Define the BD9574MWF IRQ numbers */ +enum bd9574mwf_irqs { + BD9574MWF_IRQ_MD1, + BD9574MWF_IRQ_MD2_E1, + BD9574MWF_IRQ_MD2_E2, + BD9574MWF_IRQ_PROT_ERR, + BD9574MWF_IRQ_GP, + BD9574MWF_IRQ_BKUP_HOLD_OF, + BD9574MWF_IRQ_WDT_OF, + BD9574MWF_IRQ_BKUP_TRG, +}; + /** * struct bd957x_data - internal data for the bd957x driver *