| Message ID | 1608718963-21818-1-git-send-email-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| Headers | show |
| Series | treewide: bd9571mwv: Add support for BD9574MWF | expand |
Hi Lee, > From: Lee Jones, Sent: Thursday, December 24, 2020 12:39 AM > On Wed, 23 Dec 2020, Yoshihiro Shimoda wrote: > > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > > Since the driver supports BD9571MWV PMIC only, this patch makes > > the functions and data structure become more generic so that > > it can support other PMIC variants as well. Also remove printing > > part name which Lee Jones suggested. > > > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > Co-developed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > --- > > drivers/mfd/bd9571mwv.c | 89 +++++++++++++++++++++++++------------------ > > include/linux/mfd/bd9571mwv.h | 18 +-------- > > 2 files changed, 54 insertions(+), 53 deletions(-) > > Couple of small points. > > Remainder looks good. Thank you for your review! > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c > > index 49e968e..c905ab4 100644 > > --- a/drivers/mfd/bd9571mwv.c > > +++ b/drivers/mfd/bd9571mwv.c > > @@ -3,6 +3,7 @@ > > * ROHM BD9571MWV-M MFD driver > > * > > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> > > + * Copyright (C) 2020 Renesas Electronics Corporation > > * > > * Based on the TPS65086 driver > > */ > > @@ -14,6 +15,14 @@ > > > > #include <linux/mfd/bd9571mwv.h> > > > > +/* Driver data to distinguish bd957x variants */ > > +struct bd957x_ddata { > > + const struct regmap_config *regmap_config; > > + const struct regmap_irq_chip *irq_chip; > > > + const struct mfd_cell *cells; > > + int num_cells; > > Are you using these post-probe? > > If not, they're not ddata. I'm not using all these members post-probe. So, I'll remove ddata. <snip> > > static int bd9571mwv_probe(struct i2c_client *client, > > - const struct i2c_device_id *ids) > > + const struct i2c_device_id *ids) > > { > > - struct bd9571mwv *bd; > > - int ret; > > - > > - bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL); > > - if (!bd) > > - return -ENOMEM; > > - > > - i2c_set_clientdata(client, bd); > > - bd->dev = &client->dev; > > - bd->irq = client->irq; > > + const struct bd957x_ddata *ddata; > > + struct device *dev = &client->dev; > > + struct regmap *regmap; > > + struct regmap_irq_chip_data *irq_data; > > + int ret, irq = client->irq; > > + > > + /* Read the PMIC product code */ > > + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE); > > + if (ret < 0) { > > + dev_err(dev, "Failed to read product code\n"); > > + return ret; > > + } > > Nit: '\n' here. I got it. I'll add a blank line here. > > + switch (ret) { > > + case BD9571MWV_PRODUCT_CODE_BD9571MWV: > > + ddata = &bd9571mwv_ddata; > > Simply declare 'const struct mfd_cell *cells' locally in probe and > assign it here instead. I got it. I'll also add "const struct regmap_config *regmap_config;" and "const struct regmap_irq_chip *irq_chip;" locally in probe. Best regards, Yoshihiro Shimoda
On Thu, 24 Dec 2020, Yoshihiro Shimoda wrote: > Hi Lee, > > > From: Lee Jones, Sent: Thursday, December 24, 2020 12:39 AM > > On Wed, 23 Dec 2020, Yoshihiro Shimoda wrote: > > > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > > > > Since the driver supports BD9571MWV PMIC only, this patch makes > > > the functions and data structure become more generic so that > > > it can support other PMIC variants as well. Also remove printing > > > part name which Lee Jones suggested. > > > > > > Signed-off-by: Khiem Nguyen <khiem.nguyen.xt@renesas.com> > > > Co-developed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > > --- > > > drivers/mfd/bd9571mwv.c | 89 +++++++++++++++++++++++++------------------ > > > include/linux/mfd/bd9571mwv.h | 18 +-------- > > > 2 files changed, 54 insertions(+), 53 deletions(-) > > > > Couple of small points. > > > > Remainder looks good. > > Thank you for your review! > > > > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c > > > index 49e968e..c905ab4 100644 > > > --- a/drivers/mfd/bd9571mwv.c > > > +++ b/drivers/mfd/bd9571mwv.c > > > @@ -3,6 +3,7 @@ > > > * ROHM BD9571MWV-M MFD driver > > > * > > > * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com> > > > + * Copyright (C) 2020 Renesas Electronics Corporation > > > * > > > * Based on the TPS65086 driver > > > */ > > > @@ -14,6 +15,14 @@ > > > > > > #include <linux/mfd/bd9571mwv.h> > > > > > > +/* Driver data to distinguish bd957x variants */ > > > +struct bd957x_ddata { > > > + const struct regmap_config *regmap_config; > > > + const struct regmap_irq_chip *irq_chip; > > > > > + const struct mfd_cell *cells; > > > + int num_cells; > > > > Are you using these post-probe? > > > > If not, they're not ddata. > > I'm not using all these members post-probe. > So, I'll remove ddata. > > <snip> > > > static int bd9571mwv_probe(struct i2c_client *client, > > > - const struct i2c_device_id *ids) > > > + const struct i2c_device_id *ids) > > > { > > > - struct bd9571mwv *bd; > > > - int ret; > > > - > > > - bd = devm_kzalloc(&client->dev, sizeof(*bd), GFP_KERNEL); > > > - if (!bd) > > > - return -ENOMEM; > > > - > > > - i2c_set_clientdata(client, bd); > > > - bd->dev = &client->dev; > > > - bd->irq = client->irq; > > > + const struct bd957x_ddata *ddata; > > > + struct device *dev = &client->dev; > > > + struct regmap *regmap; > > > + struct regmap_irq_chip_data *irq_data; > > > + int ret, irq = client->irq; > > > + > > > + /* Read the PMIC product code */ > > > + ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE); > > > + if (ret < 0) { > > > + dev_err(dev, "Failed to read product code\n"); > > > + return ret; > > > + } > > > > Nit: '\n' here. > > I got it. I'll add a blank line here. > > > > + switch (ret) { > > > + case BD9571MWV_PRODUCT_CODE_BD9571MWV: > > > + ddata = &bd9571mwv_ddata; > > > > Simply declare 'const struct mfd_cell *cells' locally in probe and > > assign it here instead. > > I got it. I'll also add "const struct regmap_config *regmap_config;" > and "const struct regmap_irq_chip *irq_chip;" locally in probe. If you only use them there, then yes, that's correct. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Hi Lee, > From: Lee Jones, Sent: Thursday, December 24, 2020 4:34 PM > > Hi Lee, > > > > > From: Lee Jones, Sent: Thursday, December 24, 2020 12:39 AM > > > On Wed, 23 Dec 2020, Yoshihiro Shimoda wrote: > > > > From: Khiem Nguyen <khiem.nguyen.xt@renesas.com> <snip> > > > > + switch (ret) { > > > > + case BD9571MWV_PRODUCT_CODE_BD9571MWV: > > > > + ddata = &bd9571mwv_ddata; > > > > > > Simply declare 'const struct mfd_cell *cells' locally in probe and > > > assign it here instead. > > > > I got it. I'll also add "const struct regmap_config *regmap_config;" > > and "const struct regmap_irq_chip *irq_chip;" locally in probe. > > If you only use them there, then yes, that's correct. Thank you for the reply. Yes, I only use them there. So, I have submitted v7 patches which have such implementation. Best regards, Yoshihiro Shimoda
On Wed, Dec 23, 2020 at 11:22 AM Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > Add support for BD9574MWF which is similar chip with BD9571MWV. > Note that BD9574MWF has additional features "RECOV_GPOUT", > "FREQSEL" and "RTC_IN", but supports GPIO function only. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> This looks like it compile-time depends on the other patches right? Yours, Linus Walleij
Hi Linus, > From: Linus Walleij, Sent: Monday, December 28, 2020 6:16 AM > > > Add support for BD9574MWF which is similar chip with BD9571MWV. > > Note that BD9574MWF has additional features "RECOV_GPOUT", > > "FREQSEL" and "RTC_IN", but supports GPIO function only. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com> > > Acked-by: Linus Walleij <linus.walleij@linaro.org> Thank you for your Acked-by! I'll add your Acked-by in the next patch version as v9. > This looks like it compile-time depends on the other patches right? You're correct. Best regards, Yoshihiro Shimoda