RE: [PATCH v6 11/12] mfd: bd9571mwv: Make the driver more generic
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 > > > > + 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
Re: [PATCH v6 11/12] mfd: bd9571mwv: Make the driver more generic
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 > > > > > > 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 > > > Co-developed-by: Yoshihiro Shimoda > > > Signed-off-by: Yoshihiro Shimoda > > > Reviewed-by: Matti Vaittinen > > > --- > > > 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 > > > + * Copyright (C) 2020 Renesas Electronics Corporation > > > * > > > * Based on the TPS65086 driver > > > */ > > > @@ -14,6 +15,14 @@ > > > > > > #include > > > > > > +/* 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. > > > > > 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
RE: [PATCH v6 11/12] mfd: bd9571mwv: Make the driver more generic
Hi Lee, > From: Lee Jones, Sent: Thursday, December 24, 2020 12:39 AM > On Wed, 23 Dec 2020, Yoshihiro Shimoda wrote: > > From: Khiem Nguyen > > > > 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 > > Co-developed-by: Yoshihiro Shimoda > > Signed-off-by: Yoshihiro Shimoda > > Reviewed-by: Matti Vaittinen > > --- > > 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 > > + * Copyright (C) 2020 Renesas Electronics Corporation > > * > > * Based on the TPS65086 driver > > */ > > @@ -14,6 +15,14 @@ > > > > #include > > > > +/* 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. > > 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
Re: [PATCH v6 11/12] mfd: bd9571mwv: Make the driver more generic
On Wed, 23 Dec 2020, Yoshihiro Shimoda wrote: > From: Khiem Nguyen > > 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 > Co-developed-by: Yoshihiro Shimoda > Signed-off-by: Yoshihiro Shimoda > Reviewed-by: Matti Vaittinen > --- > 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. > 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 > + * Copyright (C) 2020 Renesas Electronics Corporation > * > * Based on the TPS65086 driver > */ > @@ -14,6 +15,14 @@ > > #include > > +/* 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. > +}; [...] > 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. > + 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. > + break; > + default: > + dev_err(dev, "Unsupported device 0x%x\n", ret); > + return -ENODEV; > + } > > - bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config); > - if (IS_ERR(bd->regmap)) { > - dev_err(bd->dev, "Failed to initialize register map\n"); > - return PTR_ERR(bd->regmap); > + regmap = devm_regmap_init_i2c(client, ddata->regmap_config); > + if (IS_ERR(regmap)) { > + dev_err(dev, "Failed to initialize register map\n"); > + return PTR_ERR(regmap); > } -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog