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