diff mbox series

[v6,11/12] mfd: bd9571mwv: Make the driver more generic

Message ID 1608718963-21818-12-git-send-email-yoshihiro.shimoda.uh@renesas.com (mailing list archive)
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series treewide: bd9571mwv: Add support for BD9574MWF | expand

Commit Message

Yoshihiro Shimoda Dec. 23, 2020, 10:22 a.m. UTC 
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(-)

Comments

Lee Jones Dec. 23, 2020, 3:39 p.m. UTC | #1 
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.

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

> +};

[...]

>  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);
>  	}
Yoshihiro Shimoda Dec. 24, 2020, 6:46 a.m. UTC | #2 
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
Lee Jones Dec. 24, 2020, 7:34 a.m. UTC | #3 
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.
Yoshihiro Shimoda Dec. 24, 2020, 8:23 a.m. UTC | #4 
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
diff mbox series

Patch

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;
+};
+
 static const struct mfd_cell bd9571mwv_cells[] = {
 	{ .name = "bd9571mwv-regulator", },
 	{ .name = "bd9571mwv-gpio", },
@@ -102,13 +111,19 @@  static struct regmap_irq_chip bd9571mwv_irq_chip = {
 	.num_irqs	= ARRAY_SIZE(bd9571mwv_irqs),
 };
 
-static int bd9571mwv_identify(struct bd9571mwv *bd)
+static const struct bd957x_ddata bd9571mwv_ddata = {
+	.regmap_config = &bd9571mwv_regmap_config,
+	.irq_chip = &bd9571mwv_irq_chip,
+	.cells = bd9571mwv_cells,
+	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
+};
+
+static int bd957x_identify(struct device *dev, struct regmap *regmap)
 {
-	struct device *dev = bd->dev;
 	unsigned int value;
 	int ret;
 
-	ret = regmap_read(bd->regmap, BD9571MWV_VENDOR_CODE, &value);
+	ret = regmap_read(regmap, BD9571MWV_VENDOR_CODE, &value);
 	if (ret) {
 		dev_err(dev, "Failed to read vendor code register (ret=%i)\n",
 			ret);
@@ -121,66 +136,66 @@  static int bd9571mwv_identify(struct bd9571mwv *bd)
 		return -EINVAL;
 	}
 
-	ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_CODE, &value);
+	ret = regmap_read(regmap, BD9571MWV_PRODUCT_CODE, &value);
 	if (ret) {
 		dev_err(dev, "Failed to read product code register (ret=%i)\n",
 			ret);
 		return ret;
 	}
-
-	if (value != BD9571MWV_PRODUCT_CODE_VAL) {
-		dev_err(dev, "Invalid product code ID %02x (expected %02x)\n",
-			value, BD9571MWV_PRODUCT_CODE_VAL);
-		return -EINVAL;
-	}
-
-	ret = regmap_read(bd->regmap, BD9571MWV_PRODUCT_REVISION, &value);
+	ret = regmap_read(regmap, BD9571MWV_PRODUCT_REVISION, &value);
 	if (ret) {
 		dev_err(dev, "Failed to read revision register (ret=%i)\n",
 			ret);
 		return ret;
 	}
 
-	dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
-
 	return 0;
 }
 
 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;
+	}
+	switch (ret) {
+	case BD9571MWV_PRODUCT_CODE_BD9571MWV:
+		ddata = &bd9571mwv_ddata;
+		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);
 	}
 
-	ret = bd9571mwv_identify(bd);
+	ret = bd957x_identify(dev, regmap);
 	if (ret)
 		return ret;
 
-	ret = devm_regmap_add_irq_chip(bd->dev, bd->regmap, bd->irq,
-				       IRQF_ONESHOT, 0, &bd9571mwv_irq_chip,
-				       &bd->irq_data);
+	ret = devm_regmap_add_irq_chip(dev, regmap, irq, IRQF_ONESHOT, 0,
+				       ddata->irq_chip, &irq_data);
 	if (ret) {
-		dev_err(bd->dev, "Failed to register IRQ chip\n");
+		dev_err(dev, "Failed to register IRQ chip\n");
 		return ret;
 	}
 
-	return devm_mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO,
-				    bd9571mwv_cells, ARRAY_SIZE(bd9571mwv_cells),
-				    NULL, 0, regmap_irq_get_domain(bd->irq_data));
+	return devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO, ddata->cells,
+				    ddata->num_cells, NULL, 0,
+				    regmap_irq_get_domain(irq_data));
 }
 
 static const struct of_device_id bd9571mwv_of_match_table[] = {
diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
index bcc7092..e1716ec 100644
--- a/include/linux/mfd/bd9571mwv.h
+++ b/include/linux/mfd/bd9571mwv.h
@@ -3,6 +3,7 @@ 
  * ROHM BD9571MWV-M driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
+ * Copyright (C) 2020 Renesas Electronics Corporation
  *
  * Based on the TPS65086 driver
  */
@@ -17,7 +18,7 @@ 
 #define BD9571MWV_VENDOR_CODE			0x00
 #define BD9571MWV_VENDOR_CODE_VAL		0xdb
 #define BD9571MWV_PRODUCT_CODE			0x01
-#define BD9571MWV_PRODUCT_CODE_VAL		0x60
+#define BD9571MWV_PRODUCT_CODE_BD9571MWV	0x60
 #define BD9571MWV_PRODUCT_REVISION		0x02
 
 #define BD9571MWV_I2C_FUSA_MODE			0x10
@@ -94,19 +95,4 @@  enum bd9571mwv_irqs {
 	BD9571MWV_IRQ_WDT_OF,
 	BD9571MWV_IRQ_BKUP_TRG,
 };
-
-/**
- * struct bd9571mwv - state holder for the bd9571mwv driver
- *
- * Device data may be used to access the BD9571MWV chip
- */
-struct bd9571mwv {
-	struct device *dev;
-	struct regmap *regmap;
-
-	/* IRQ Data */
-	int irq;
-	struct regmap_irq_chip_data *irq_data;
-};
-
 #endif /* __LINUX_MFD_BD9571MWV_H */