[PATCH v2 00/10] treewide: bd9571mwv: Add support for BD9574MWF

[PATCH v2 00/10] treewide: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

Add BD9574MWF support into bd9571mwv gpio, mfd and regulator drivers.
Latest Ebisu-4D boards has this chip instead of BD9571MWV so that
we need this patch series.

Changes from v1:
 - Document BD9574MWF on the dt-binding.
 - Add ROHM_CHIP_TYPE_BD957[14] into rohm-generic.h.
 - To simplify gpio and regulator drivers, using regmap instead of
   using struct bd9571mwv.
 - Remove BD9574MWF definitions to make gpio and regulator driver
   simple to support for BD9574MWF.
 - Add BD9574MWF support for gpio and regulator drivers.
 - Add missing regmap ranges for BD9574MWF.
 - Rename "part_number" with "part_name".
 https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=398059

Khiem Nguyen (2):
  mfd: bd9571mwv: Make the driver more generic
  mfd: bd9571mwv: Add support for BD9574MWF

Yoshihiro Shimoda (8):
  dt-bindings: mfd: bd9571mwv: Document BD9574MWF
  mfd: rohm-generic: Add BD9571 and BD9574
  regulator: bd9571mwv: rid of using struct bd9571mwv
  regulator: bd9571mwv: Add BD9574MWF support
  gpio: bd9571mwv: Use the SPDX license identifier
  gpio: bd9571mwv: rid of using struct bd9571mwv
  gpio: bd9571mwv: Add BD9574MWF support
  mfd: bd9571mwv: Use the SPDX license identifier

 .../devicetree/bindings/mfd/bd9571mwv.txt          |   4 +-
 drivers/gpio/gpio-bd9571mwv.c                      |  35 ++---
 drivers/mfd/bd9571mwv.c                            | 167 ++++++++++++++++++---
 drivers/regulator/bd9571mwv-regulator.c            |  59 +++++---
 include/linux/mfd/bd9571mwv.h                      |  46 +++---
 include/linux/mfd/rohm-generic.h                   |   2 +
 6 files changed, 215 insertions(+), 98 deletions(-)

-- 
2.7.4

[PATCH v2 01/10] dt-bindings: mfd: bd9571mwv: Document BD9574MWF

[Yoshihiro Shimoda]

Document other similar specification chip BD9574MWF.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 Documentation/devicetree/bindings/mfd/bd9571mwv.txt | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/bd9571mwv.txt b/Documentation/devicetree/bindings/mfd/bd9571mwv.txt
index 8c46786..1d6413e 100644
--- a/Documentation/devicetree/bindings/mfd/bd9571mwv.txt
+++ b/Documentation/devicetree/bindings/mfd/bd9571mwv.txt
@@ -1,7 +1,7 @@
-* ROHM BD9571MWV Power Management Integrated Circuit (PMIC) bindings
+* ROHM BD9571MWV/BD9574MWF Power Management Integrated Circuit (PMIC) bindings
 
 Required properties:
- - compatible		: Should be "rohm,bd9571mwv".
+ - compatible		: Should be "rohm,bd9571mwv" or "rohm,bd9574mwf".
  - reg			: I2C slave address.
  - interrupts		: The interrupt line the device is connected to.
  - interrupt-controller	: Marks the device node as an interrupt controller.
-- 
2.7.4

[PATCH v2 02/10] mfd: rohm-generic: Add BD9571 and BD9574

[Yoshihiro Shimoda]

Add chip IDs for BD9571MWV and BD9574MWF.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 include/linux/mfd/rohm-generic.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/mfd/rohm-generic.h b/include/linux/mfd/rohm-generic.h
index 4283b5b..affacf8 100644
--- a/include/linux/mfd/rohm-generic.h
+++ b/include/linux/mfd/rohm-generic.h
@@ -12,6 +12,8 @@ enum rohm_chip_type {
 	ROHM_CHIP_TYPE_BD71847,
 	ROHM_CHIP_TYPE_BD70528,
 	ROHM_CHIP_TYPE_BD71828,
+	ROHM_CHIP_TYPE_BD9571,
+	ROHM_CHIP_TYPE_BD9574,
 	ROHM_CHIP_TYPE_AMOUNT
 };
 
-- 
2.7.4

Re: [PATCH v2 02/10] mfd: rohm-generic: Add BD9571 and BD9574

[Vaittinen, Matti]

On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> Add chip IDs for BD9571MWV and BD9574MWF.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  include/linux/mfd/rohm-generic.h | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/mfd/rohm-generic.h
> b/include/linux/mfd/rohm-generic.h
> index 4283b5b..affacf8 100644
> --- a/include/linux/mfd/rohm-generic.h
> +++ b/include/linux/mfd/rohm-generic.h
> @@ -12,6 +12,8 @@ enum rohm_chip_type {
>  	ROHM_CHIP_TYPE_BD71847,
>  	ROHM_CHIP_TYPE_BD70528,
>  	ROHM_CHIP_TYPE_BD71828,
> +	ROHM_CHIP_TYPE_BD9571,
> +	ROHM_CHIP_TYPE_BD9574,
>  	ROHM_CHIP_TYPE_AMOUNT
>  };
>  


Just a note to Lee & Others:
This will probably conflict with the BD9573/BD9576 patch series (which
introduces those chip IDs here). Conflict should be trivial though.

With that note:
Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>


--Matti

[PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv

[Yoshihiro Shimoda]

To simplify this driver, use dev_get_regmap() and
rid of using struct bd9571mwv.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/regulator/bd9571mwv-regulator.c | 49 +++++++++++++++++----------------
 1 file changed, 26 insertions(+), 23 deletions(-)

diff --git a/drivers/regulator/bd9571mwv-regulator.c b/drivers/regulator/bd9571mwv-regulator.c
index e690c2c..02120b0 100644
--- a/drivers/regulator/bd9571mwv-regulator.c
+++ b/drivers/regulator/bd9571mwv-regulator.c
@@ -17,7 +17,7 @@
 #include <linux/mfd/bd9571mwv.h>
 
 struct bd9571mwv_reg {
-	struct bd9571mwv *bd;
+	struct regmap *regmap;
 
 	/* DDR Backup Power */
 	u8 bkup_mode_cnt_keepon;	/* from "rohm,ddr-backup-power" */
@@ -137,26 +137,30 @@ static const struct regulator_desc regulators[] = {
 };
 
 #ifdef CONFIG_PM_SLEEP
-static int bd9571mwv_bkup_mode_read(struct bd9571mwv *bd, unsigned int *mode)
+static int bd9571mwv_bkup_mode_read(struct device * dev,
+				    struct bd9571mwv_reg *bdreg,
+				    unsigned int *mode)
 {
 	int ret;
 
-	ret = regmap_read(bd->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
+	ret = regmap_read(bdreg->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
 	if (ret) {
-		dev_err(bd->dev, "failed to read backup mode (%d)\n", ret);
+		dev_err(dev, "failed to read backup mode (%d)\n", ret);
 		return ret;
 	}
 
 	return 0;
 }
 
-static int bd9571mwv_bkup_mode_write(struct bd9571mwv *bd, unsigned int mode)
+static int bd9571mwv_bkup_mode_write(struct device *dev,
+				     struct bd9571mwv_reg *bdreg,
+				     unsigned int mode)
 {
 	int ret;
 
-	ret = regmap_write(bd->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
+	ret = regmap_write(bdreg->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
 	if (ret) {
-		dev_err(bd->dev, "failed to configure backup mode 0x%x (%d)\n",
+		dev_err(dev, "failed to configure backup mode 0x%x (%d)\n",
 			mode, ret);
 		return ret;
 	}
@@ -194,7 +198,7 @@ static ssize_t backup_mode_store(struct device *dev,
 	 * Configure DDR Backup Mode, to change the role of the accessory power
 	 * switch from a power switch to a wake-up switch, or vice versa
 	 */
-	ret = bd9571mwv_bkup_mode_read(bdreg->bd, &mode);
+	ret = bd9571mwv_bkup_mode_read(dev, bdreg, &mode);
 	if (ret)
 		return ret;
 
@@ -202,7 +206,7 @@ static ssize_t backup_mode_store(struct device *dev,
 	if (bdreg->bkup_mode_enabled)
 		mode |= bdreg->bkup_mode_cnt_keepon;
 
-	ret = bd9571mwv_bkup_mode_write(bdreg->bd, mode);
+	ret = bd9571mwv_bkup_mode_write(dev, bdreg, mode);
 	if (ret)
 		return ret;
 
@@ -221,7 +225,7 @@ static int bd9571mwv_suspend(struct device *dev)
 		return 0;
 
 	/* Save DDR Backup Mode */
-	ret = bd9571mwv_bkup_mode_read(bdreg->bd, &mode);
+	ret = bd9571mwv_bkup_mode_read(dev, bdreg, &mode);
 	if (ret)
 		return ret;
 
@@ -235,7 +239,7 @@ static int bd9571mwv_suspend(struct device *dev)
 	mode |= bdreg->bkup_mode_cnt_keepon;
 
 	if (mode != bdreg->bkup_mode_cnt_saved)
-		return bd9571mwv_bkup_mode_write(bdreg->bd, mode);
+		return bd9571mwv_bkup_mode_write(dev, bdreg, mode);
 
 	return 0;
 }
@@ -248,7 +252,7 @@ static int bd9571mwv_resume(struct device *dev)
 		return 0;
 
 	/* Restore DDR Backup Mode */
-	return bd9571mwv_bkup_mode_write(bdreg->bd, bdreg->bkup_mode_cnt_saved);
+	return bd9571mwv_bkup_mode_write(dev, bdreg, bdreg->bkup_mode_cnt_saved);
 }
 
 static const struct dev_pm_ops bd9571mwv_pm  = {
@@ -268,7 +272,6 @@ static int bd9571mwv_regulator_remove(struct platform_device *pdev)
 
 static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 {
-	struct bd9571mwv *bd = dev_get_drvdata(pdev->dev.parent);
 	struct regulator_config config = { };
 	struct bd9571mwv_reg *bdreg;
 	struct regulator_dev *rdev;
@@ -279,40 +282,40 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 	if (!bdreg)
 		return -ENOMEM;
 
-	bdreg->bd = bd;
+	bdreg->regmap = dev_get_regmap(pdev->dev.parent, NULL);
 
 	platform_set_drvdata(pdev, bdreg);
 
 	config.dev = &pdev->dev;
-	config.dev->of_node = bd->dev->of_node;
-	config.driver_data = bd;
-	config.regmap = bd->regmap;
+	config.dev->of_node = pdev->dev.parent->of_node;
+	config.driver_data = bdreg;
+	config.regmap = bdreg->regmap;
 
 	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
 		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
 					       &config);
 		if (IS_ERR(rdev)) {
-			dev_err(bd->dev, "failed to register %s regulator\n",
+			dev_err(&pdev->dev, "failed to register %s regulator\n",
 				pdev->name);
 			return PTR_ERR(rdev);
 		}
 	}
 
 	val = 0;
-	of_property_read_u32(bd->dev->of_node, "rohm,ddr-backup-power", &val);
+	of_property_read_u32(config.dev->of_node, "rohm,ddr-backup-power", &val);
 	if (val & ~BD9571MWV_BKUP_MODE_CNT_KEEPON_MASK) {
-		dev_err(bd->dev, "invalid %s mode %u\n",
+		dev_err(&pdev->dev, "invalid %s mode %u\n",
 			"rohm,ddr-backup-power", val);
 		return -EINVAL;
 	}
 	bdreg->bkup_mode_cnt_keepon = val;
 
-	bdreg->rstbmode_level = of_property_read_bool(bd->dev->of_node,
+	bdreg->rstbmode_level = of_property_read_bool(config.dev->of_node,
 						      "rohm,rstbmode-level");
-	bdreg->rstbmode_pulse = of_property_read_bool(bd->dev->of_node,
+	bdreg->rstbmode_pulse = of_property_read_bool(config.dev->of_node,
 						      "rohm,rstbmode-pulse");
 	if (bdreg->rstbmode_level && bdreg->rstbmode_pulse) {
-		dev_err(bd->dev, "only one rohm,rstbmode-* may be specified");
+		dev_err(&pdev->dev, "only one rohm,rstbmode-* may be specified");
 		return -EINVAL;
 	}
 
-- 
2.7.4

Re: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv

[Vaittinen, Matti]

On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> To simplify this driver, use dev_get_regmap() and
> rid of using struct bd9571mwv.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/regulator/bd9571mwv-regulator.c | 49 +++++++++++++++++----
> ------------
>  1 file changed, 26 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/regulator/bd9571mwv-regulator.c
> b/drivers/regulator/bd9571mwv-regulator.c
> index e690c2c..02120b0 100644
> --- a/drivers/regulator/bd9571mwv-regulator.c
> +++ b/drivers/regulator/bd9571mwv-regulator.c
> @@ -17,7 +17,7 @@
>  #include <linux/mfd/bd9571mwv.h>
>  
>  struct bd9571mwv_reg {
> -	struct bd9571mwv *bd;
> +	struct regmap *regmap;

As a 'nit':
I might consider adding the dev pointer here to avoid extra argument
with all the bkup_mode functions below. (just pass this struct and
mode). But that's only my preference - feel free to ignore this comment
if patch is Ok to Mark, Marek & Others :)

Overall, looks good to me :)
Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

>  
>  	/* DDR Backup Power */
>  	u8 bkup_mode_cnt_keepon;	/* from "rohm,ddr-backup-power" */
> @@ -137,26 +137,30 @@ static const struct regulator_desc regulators[]
> = {
>  };
>  
>  #ifdef CONFIG_PM_SLEEP
> -static int bd9571mwv_bkup_mode_read(struct bd9571mwv *bd, unsigned
> int *mode)
> +static int bd9571mwv_bkup_mode_read(struct device * dev,
> +				    struct bd9571mwv_reg *bdreg,
> +				    unsigned int *mode)
>  {
>  	int ret;
>  
> -	ret = regmap_read(bd->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
> +	ret = regmap_read(bdreg->regmap, BD9571MWV_BKUP_MODE_CNT,
> mode);
>  	if (ret) {
> -		dev_err(bd->dev, "failed to read backup mode (%d)\n",
> ret);
> +		dev_err(dev, "failed to read backup mode (%d)\n", ret);
>  		return ret;
>  	}
>  
>  	return 0;
>  }
>  
> -static int bd9571mwv_bkup_mode_write(struct bd9571mwv *bd, unsigned
> int mode)
> +static int bd9571mwv_bkup_mode_write(struct device *dev,
> +				     struct bd9571mwv_reg *bdreg,
> +				     unsigned int mode)
>  {
>  	int ret;
>  
> -	ret = regmap_write(bd->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
> +	ret = regmap_write(bdreg->regmap, BD9571MWV_BKUP_MODE_CNT,
> mode);
>  	if (ret) {
> -		dev_err(bd->dev, "failed to configure backup mode 0x%x
> (%d)\n",
> +		dev_err(dev, "failed to configure backup mode 0x%x
> (%d)\n",
>  			mode, ret);
>  		return ret;
>  	}
> @@ -194,7 +198,7 @@ static ssize_t backup_mode_store(struct device
> *dev,
>  	 * Configure DDR Backup Mode, to change the role of the
> accessory power
>  	 * switch from a power switch to a wake-up switch, or vice
> versa
>  	 */
> -	ret = bd9571mwv_bkup_mode_read(bdreg->bd, &mode);
> +	ret = bd9571mwv_bkup_mode_read(dev, bdreg, &mode);
>  	if (ret)
>  		return ret;
>  
> @@ -202,7 +206,7 @@ static ssize_t backup_mode_store(struct device
> *dev,
>  	if (bdreg->bkup_mode_enabled)
>  		mode |= bdreg->bkup_mode_cnt_keepon;
>  
> -	ret = bd9571mwv_bkup_mode_write(bdreg->bd, mode);
> +	ret = bd9571mwv_bkup_mode_write(dev, bdreg, mode);
>  	if (ret)
>  		return ret;
>  
> @@ -221,7 +225,7 @@ static int bd9571mwv_suspend(struct device *dev)
>  		return 0;
>  
>  	/* Save DDR Backup Mode */
> -	ret = bd9571mwv_bkup_mode_read(bdreg->bd, &mode);
> +	ret = bd9571mwv_bkup_mode_read(dev, bdreg, &mode);
>  	if (ret)
>  		return ret;
>  
> @@ -235,7 +239,7 @@ static int bd9571mwv_suspend(struct device *dev)
>  	mode |= bdreg->bkup_mode_cnt_keepon;
>  
>  	if (mode != bdreg->bkup_mode_cnt_saved)
> -		return bd9571mwv_bkup_mode_write(bdreg->bd, mode);
> +		return bd9571mwv_bkup_mode_write(dev, bdreg, mode);
>  
>  	return 0;
>  }
> @@ -248,7 +252,7 @@ static int bd9571mwv_resume(struct device *dev)
>  		return 0;
>  
>  	/* Restore DDR Backup Mode */
> -	return bd9571mwv_bkup_mode_write(bdreg->bd, bdreg-
> >bkup_mode_cnt_saved);
> +	return bd9571mwv_bkup_mode_write(dev, bdreg, bdreg-
> >bkup_mode_cnt_saved);
>  }
>  
>  static const struct dev_pm_ops bd9571mwv_pm  = {
> @@ -268,7 +272,6 @@ static int bd9571mwv_regulator_remove(struct
> platform_device *pdev)
>  
>  static int bd9571mwv_regulator_probe(struct platform_device *pdev)
>  {
> -	struct bd9571mwv *bd = dev_get_drvdata(pdev->dev.parent);
>  	struct regulator_config config = { };
>  	struct bd9571mwv_reg *bdreg;
>  	struct regulator_dev *rdev;
> @@ -279,40 +282,40 @@ static int bd9571mwv_regulator_probe(struct
> platform_device *pdev)
>  	if (!bdreg)
>  		return -ENOMEM;
>  
> -	bdreg->bd = bd;
> +	bdreg->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>  
>  	platform_set_drvdata(pdev, bdreg);
>  
>  	config.dev = &pdev->dev;
> -	config.dev->of_node = bd->dev->of_node;
> -	config.driver_data = bd;
> -	config.regmap = bd->regmap;
> +	config.dev->of_node = pdev->dev.parent->of_node;
> +	config.driver_data = bdreg;
> +	config.regmap = bdreg->regmap;
>  
>  	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
>  		rdev = devm_regulator_register(&pdev->dev,
> &regulators[i],
>  					       &config);
>  		if (IS_ERR(rdev)) {
> -			dev_err(bd->dev, "failed to register %s
> regulator\n",
> +			dev_err(&pdev->dev, "failed to register %s
> regulator\n",
>  				pdev->name);
>  			return PTR_ERR(rdev);
>  		}
>  	}
>  
>  	val = 0;
> -	of_property_read_u32(bd->dev->of_node, "rohm,ddr-backup-power", 
> &val);
> +	of_property_read_u32(config.dev->of_node, "rohm,ddr-backup-
> power", &val);
>  	if (val & ~BD9571MWV_BKUP_MODE_CNT_KEEPON_MASK) {
> -		dev_err(bd->dev, "invalid %s mode %u\n",
> +		dev_err(&pdev->dev, "invalid %s mode %u\n",
>  			"rohm,ddr-backup-power", val);
>  		return -EINVAL;
>  	}
>  	bdreg->bkup_mode_cnt_keepon = val;
>  
> -	bdreg->rstbmode_level = of_property_read_bool(bd->dev->of_node,
> +	bdreg->rstbmode_level = of_property_read_bool(config.dev-
> >of_node,
>  						      "rohm,rstbmode-
> level");
> -	bdreg->rstbmode_pulse = of_property_read_bool(bd->dev->of_node,
> +	bdreg->rstbmode_pulse = of_property_read_bool(config.dev-
> >of_node,
>  						      "rohm,rstbmode-
> pulse");
>  	if (bdreg->rstbmode_level && bdreg->rstbmode_pulse) {
> -		dev_err(bd->dev, "only one rohm,rstbmode-* may be
> specified");
> +		dev_err(&pdev->dev, "only one rohm,rstbmode-* may be
> specified");
>  		return -EINVAL;
>  	}
>  

Re: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv

[Geert Uytterhoeven]

Hi Matti,

On Fri, Dec 11, 2020 at 3:03 PM Vaittinen, Matti
<Matti.Vaittinen@fi.rohmeurope.com> wrote:
> On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > To simplify this driver, use dev_get_regmap() and
> > rid of using struct bd9571mwv.
> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/regulator/bd9571mwv-regulator.c | 49 +++++++++++++++++----
> > ------------
> >  1 file changed, 26 insertions(+), 23 deletions(-)
> >
> > diff --git a/drivers/regulator/bd9571mwv-regulator.c
> > b/drivers/regulator/bd9571mwv-regulator.c
> > index e690c2c..02120b0 100644
> > --- a/drivers/regulator/bd9571mwv-regulator.c
> > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > @@ -17,7 +17,7 @@
> >  #include <linux/mfd/bd9571mwv.h>
> >
> >  struct bd9571mwv_reg {
> > -     struct bd9571mwv *bd;
> > +     struct regmap *regmap;
>
> As a 'nit':
> I might consider adding the dev pointer here to avoid extra argument
> with all the bkup_mode functions below. (just pass this struct and
> mode). But that's only my preference - feel free to ignore this comment
> if patch is Ok to Mark, Marek & Others :)

Struct regmap already contains a struct device pointer, but that's internal
to regmap.

Perhaps adding a regmap_device() helper to retrieve the device pointer
might be worthwhile?
Or a regmap_err() helper to print error messages?

>
> Overall, looks good to me :)
> Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
>
> >
> >       /* DDR Backup Power */
> >       u8 bkup_mode_cnt_keepon;        /* from "rohm,ddr-backup-power" */
> > @@ -137,26 +137,30 @@ static const struct regulator_desc regulators[]
> > = {
> >  };
> >
> >  #ifdef CONFIG_PM_SLEEP
> > -static int bd9571mwv_bkup_mode_read(struct bd9571mwv *bd, unsigned
> > int *mode)
> > +static int bd9571mwv_bkup_mode_read(struct device * dev,
> > +                                 struct bd9571mwv_reg *bdreg,
> > +                                 unsigned int *mode)
> >  {
> >       int ret;
> >
> > -     ret = regmap_read(bd->regmap, BD9571MWV_BKUP_MODE_CNT, mode);
> > +     ret = regmap_read(bdreg->regmap, BD9571MWV_BKUP_MODE_CNT,
> > mode);
> >       if (ret) {
> > -             dev_err(bd->dev, "failed to read backup mode (%d)\n",
> > ret);
> > +             dev_err(dev, "failed to read backup mode (%d)\n", ret);
> >               return ret;
> >       }

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

Re: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv

[Geert Uytterhoeven]

On Tue, Dec 15, 2020 at 5:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Dec 11, 2020 at 3:03 PM Vaittinen, Matti
> <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > > To simplify this driver, use dev_get_regmap() and
> > > rid of using struct bd9571mwv.
> > >
> > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > >  drivers/regulator/bd9571mwv-regulator.c | 49 +++++++++++++++++----
> > > ------------
> > >  1 file changed, 26 insertions(+), 23 deletions(-)
> > >
> > > diff --git a/drivers/regulator/bd9571mwv-regulator.c
> > > b/drivers/regulator/bd9571mwv-regulator.c
> > > index e690c2c..02120b0 100644
> > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > @@ -17,7 +17,7 @@
> > >  #include <linux/mfd/bd9571mwv.h>
> > >
> > >  struct bd9571mwv_reg {
> > > -     struct bd9571mwv *bd;
> > > +     struct regmap *regmap;
> >
> > As a 'nit':
> > I might consider adding the dev pointer here to avoid extra argument
> > with all the bkup_mode functions below. (just pass this struct and
> > mode). But that's only my preference - feel free to ignore this comment
> > if patch is Ok to Mark, Marek & Others :)
>
> Struct regmap already contains a struct device pointer, but that's internal
> to regmap.
>
> Perhaps adding a regmap_device() helper to retrieve the device pointer
> might be worthwhile?

-EEXISTS ;-)

struct device *regmap_get_device(struct regmap *map)

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

RE: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv

[Yoshihiro Shimoda]

Hi Geert-san, Matti-san,

> From: Geert Uytterhoeven, Sent: Wednesday, December 16, 2020 1:13 AM
> On Tue, Dec 15, 2020 at 5:02 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Fri, Dec 11, 2020 at 3:03 PM Vaittinen, Matti
> > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
<snip>
> > > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > > @@ -17,7 +17,7 @@
> > > >  #include <linux/mfd/bd9571mwv.h>
> > > >
> > > >  struct bd9571mwv_reg {
> > > > -     struct bd9571mwv *bd;
> > > > +     struct regmap *regmap;
> > >
> > > As a 'nit':
> > > I might consider adding the dev pointer here to avoid extra argument
> > > with all the bkup_mode functions below. (just pass this struct and
> > > mode). But that's only my preference - feel free to ignore this comment
> > > if patch is Ok to Mark, Marek & Others :)
> >
> > Struct regmap already contains a struct device pointer, but that's internal
> > to regmap.
> >
> > Perhaps adding a regmap_device() helper to retrieve the device pointer
> > might be worthwhile?
> 
> -EEXISTS ;-)
> 
> struct device *regmap_get_device(struct regmap *map)

Thank you for finding this. I'll fix this patch.

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv

[Vaittinen, Matti]

On Wed, 2020-12-16 at 02:13 +0000, Yoshihiro Shimoda wrote:
> Hi Geert-san, Matti-san,
> 
> > From: Geert Uytterhoeven, Sent: Wednesday, December 16, 2020 1:13
> > AM
> > On Tue, Dec 15, 2020 at 5:02 PM Geert Uytterhoeven <
> > geert@linux-m68k.org> wrote:
> > > On Fri, Dec 11, 2020 at 3:03 PM Vaittinen, Matti
> > > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> <snip>
> > > > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > > > @@ -17,7 +17,7 @@
> > > > >  #include <linux/mfd/bd9571mwv.h>
> > > > > 
> > > > >  struct bd9571mwv_reg {
> > > > > -     struct bd9571mwv *bd;
> > > > > +     struct regmap *regmap;
> > > > 
> > > > As a 'nit':
> > > > I might consider adding the dev pointer here to avoid extra
> > > > argument
> > > > with all the bkup_mode functions below. (just pass this struct
> > > > and
> > > > mode). But that's only my preference - feel free to ignore this
> > > > comment
> > > > if patch is Ok to Mark, Marek & Others :)
> > > 
> > > Struct regmap already contains a struct device pointer, but
> > > that's internal
> > > to regmap.
> > > 
> > > Perhaps adding a regmap_device() helper to retrieve the device
> > > pointer
> > > might be worthwhile?
> > 
> > -EEXISTS ;-)
> > 
> > struct device *regmap_get_device(struct regmap *map)
> 
> Thank you for finding this. I'll fix this patch.

Just a small reminder that this device is probably the MFD device, not
the device created for regulator driver. (Regmap is created for MFD).
For prints this only means we're issuing prints as if MFD device
generated them, right? I'm not sure it is the best approach - but I'll
leave this to Mark & others to judge :)

> 
> Best regards,
> Yoshihiro Shimoda
> 

RE: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv

[Yoshihiro Shimoda]

Hi Matti-san,

> From: Vaittinen, Matti, Sent: Wednesday, December 16, 2020 3:00 PM
> On Wed, 2020-12-16 at 02:13 +0000, Yoshihiro Shimoda wrote:
> > Hi Geert-san, Matti-san,
> >
> > > From: Geert Uytterhoeven, Sent: Wednesday, December 16, 2020 1:13
> > > AM
> > > On Tue, Dec 15, 2020 at 5:02 PM Geert Uytterhoeven <
> > > geert@linux-m68k.org> wrote:
> > > > On Fri, Dec 11, 2020 at 3:03 PM Vaittinen, Matti
> > > > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > > > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > <snip>
> > > > > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > > > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > > > > @@ -17,7 +17,7 @@
> > > > > >  #include <linux/mfd/bd9571mwv.h>
> > > > > >
> > > > > >  struct bd9571mwv_reg {
> > > > > > -     struct bd9571mwv *bd;
> > > > > > +     struct regmap *regmap;
> > > > >
> > > > > As a 'nit':
> > > > > I might consider adding the dev pointer here to avoid extra
> > > > > argument
> > > > > with all the bkup_mode functions below. (just pass this struct
> > > > > and
> > > > > mode). But that's only my preference - feel free to ignore this
> > > > > comment
> > > > > if patch is Ok to Mark, Marek & Others :)
> > > >
> > > > Struct regmap already contains a struct device pointer, but
> > > > that's internal
> > > > to regmap.
> > > >
> > > > Perhaps adding a regmap_device() helper to retrieve the device
> > > > pointer
> > > > might be worthwhile?
> > >
> > > -EEXISTS ;-)
> > >
> > > struct device *regmap_get_device(struct regmap *map)
> >
> > Thank you for finding this. I'll fix this patch.
> 
> Just a small reminder that this device is probably the MFD device, not
> the device created for regulator driver. (Regmap is created for MFD).
> For prints this only means we're issuing prints as if MFD device
> generated them, right? I'm not sure it is the best approach - but I'll
> leave this to Mark & others to judge :)

Thank you for the comment. You're correct. regmap_get_device() is
the MFD device. Also, original code had used the MFD device as
"dev_err(bd->dev, ...)". So, printk behavior is the same as before :)

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 03/10] regulator: bd9571mwv: rid of using struct bd9571mwv

[Vaittinen, Matti]

On Wed, 2020-12-16 at 06:29 +0000, Yoshihiro Shimoda wrote:
> Hi Matti-san,
> 
> > From: Vaittinen, Matti, Sent: Wednesday, December 16, 2020 3:00 PM
> > On Wed, 2020-12-16 at 02:13 +0000, Yoshihiro Shimoda wrote:
> > > Hi Geert-san, Matti-san,
> > > 
> > > > From: Geert Uytterhoeven, Sent: Wednesday, December 16, 2020
> > > > 1:13
> > > > AM
> > > > On Tue, Dec 15, 2020 at 5:02 PM Geert Uytterhoeven <
> > > > geert@linux-m68k.org> wrote:
> > > > > On Fri, Dec 11, 2020 at 3:03 PM Vaittinen, Matti
> > > > > <Matti.Vaittinen@fi.rohmeurope.com> wrote:
> > > > > > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > > <snip>
> > > > > > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > > > > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > > > > > @@ -17,7 +17,7 @@
> > > > > > >  #include <linux/mfd/bd9571mwv.h>
> > > > > > > 
> > > > > > >  struct bd9571mwv_reg {
> > > > > > > -     struct bd9571mwv *bd;
> > > > > > > +     struct regmap *regmap;
> > > > > > 
> > > > > > As a 'nit':
> > > > > > I might consider adding the dev pointer here to avoid extra
> > > > > > argument
> > > > > > with all the bkup_mode functions below. (just pass this
> > > > > > struct
> > > > > > and
> > > > > > mode). But that's only my preference - feel free to ignore
> > > > > > this
> > > > > > comment
> > > > > > if patch is Ok to Mark, Marek & Others :)
> > > > > 
> > > > > Struct regmap already contains a struct device pointer, but
> > > > > that's internal
> > > > > to regmap.
> > > > > 
> > > > > Perhaps adding a regmap_device() helper to retrieve the
> > > > > device
> > > > > pointer
> > > > > might be worthwhile?
> > > > 
> > > > -EEXISTS ;-)
> > > > 
> > > > struct device *regmap_get_device(struct regmap *map)
> > > 
> > > Thank you for finding this. I'll fix this patch.
> > 
> > Just a small reminder that this device is probably the MFD device,
> > not
> > the device created for regulator driver. (Regmap is created for
> > MFD).
> > For prints this only means we're issuing prints as if MFD device
> > generated them, right? I'm not sure it is the best approach - but
> > I'll
> > leave this to Mark & others to judge :)
> 
> Thank you for the comment. You're correct. regmap_get_device() is
> the MFD device. Also, original code had used the MFD device as
> "dev_err(bd->dev, ...)". So, printk behavior is the same as before :)

Right. I must admit didn't catch that. I actually think using the
&pdev->dev for prints issued by the regulator driver would be more
correct but I'm not complaining if using MFD device is Ok to Mark &
others :) I do appreciate your work with this, thanks!

> 
> Best regards,
> Yoshihiro Shimoda
> 

[PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support

[Yoshihiro Shimoda]

Add support for BD9574MWF which is silimar chip with BD9571MWV.
Note that BD9574MWF doesn't support AVS and VID.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/regulator/bd9571mwv-regulator.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/bd9571mwv-regulator.c b/drivers/regulator/bd9571mwv-regulator.c
index 02120b0..041339b 100644
--- a/drivers/regulator/bd9571mwv-regulator.c
+++ b/drivers/regulator/bd9571mwv-regulator.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 /*
- * ROHM BD9571MWV-M regulator driver
+ * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  *
@@ -9,6 +9,7 @@
  * NOTE: VD09 is missing
  */
 
+#include <linux/mfd/rohm-generic.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 	struct regulator_dev *rdev;
 	unsigned int val;
 	int i;
+	enum rohm_chip_type chip = platform_get_device_id(pdev)->driver_data;
 
 	bdreg = devm_kzalloc(&pdev->dev, sizeof(*bdreg), GFP_KERNEL);
 	if (!bdreg)
@@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 	config.regmap = bdreg->regmap;
 
 	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
+		/* BD9574MWF supports DVFS only */
+		if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id != DVFS)
+			continue;
 		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
 					       &config);
 		if (IS_ERR(rdev)) {
@@ -339,7 +344,8 @@ static int bd9571mwv_regulator_probe(struct platform_device *pdev)
 }
 
 static const struct platform_device_id bd9571mwv_regulator_id_table[] = {
-	{ "bd9571mwv-regulator", },
+	{ "bd9571mwv-regulator", ROHM_CHIP_TYPE_BD9571 },
+	{ "bd9574mwf-regulator", ROHM_CHIP_TYPE_BD9574 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, bd9571mwv_regulator_id_table);
-- 
2.7.4

Re: [PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support

[Vaittinen, Matti]

Hello again Shimada-san,

On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> Add support for BD9574MWF which is silimar chip with BD9571MWV.
> Note that BD9574MWF doesn't support AVS and VID.

I'd like to understand what is VID?

> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> ---
>  drivers/regulator/bd9571mwv-regulator.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/bd9571mwv-regulator.c
> b/drivers/regulator/bd9571mwv-regulator.c
> index 02120b0..041339b 100644
> --- a/drivers/regulator/bd9571mwv-regulator.c
> +++ b/drivers/regulator/bd9571mwv-regulator.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
> - * ROHM BD9571MWV-M regulator driver
> + * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
>   *
>   * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
>   *
> @@ -9,6 +9,7 @@
>   * NOTE: VD09 is missing
>   */
>  
> +#include <linux/mfd/rohm-generic.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct
> platform_device *pdev)
>  	struct regulator_dev *rdev;
>  	unsigned int val;
>  	int i;
> +	enum rohm_chip_type chip = platform_get_device_id(pdev)-
> >driver_data;
>  
>  	bdreg = devm_kzalloc(&pdev->dev, sizeof(*bdreg), GFP_KERNEL);
>  	if (!bdreg)
> @@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct
> platform_device *pdev)
>  	config.regmap = bdreg->regmap;
>  
>  	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> +		/* BD9574MWF supports DVFS only */
> +		if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id
> != DVFS)
> +			continue;

Does this mean that reading VD09 voltage is not supported by driver? (I
assumed VD09 initial voltage can be set from eeprom(?) and read by
driver - I may be wrong though). Perhaps it is worth mentioning in the
commit message as a driver restriction?

And just asking out of the curiosity - are the other voltage rails
listed in data-sheet (VD18, DDR0, VD33, VD09 and LDO1,...,LDO4) set-up
from DT as fixed-regulators? Any reason why they are not set-up here?

>  		rdev = devm_regulator_register(&pdev->dev,
> &regulators[i],
>  					       &config);
>  		if (IS_ERR(rdev)) {
> @@ -339,7 +344,8 @@ static int bd9571mwv_regulator_probe(struct
> platform_device *pdev)
>  }
>  
>  static const struct platform_device_id
> bd9571mwv_regulator_id_table[] = {
> -	{ "bd9571mwv-regulator", },
> +	{ "bd9571mwv-regulator", ROHM_CHIP_TYPE_BD9571 },
> +	{ "bd9574mwf-regulator", ROHM_CHIP_TYPE_BD9574 },
>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(platform, bd9571mwv_regulator_id_table);

RE: [PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support

[Yoshihiro Shimoda]

Hello Matti-san,

> From: Vaittinen, Matti, Sent: Friday, December 11, 2020 9:34 PM
> 
> Hello again Shimada-san,
> 
> On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > Add support for BD9574MWF which is silimar chip with BD9571MWV.
> > Note that BD9574MWF doesn't support AVS and VID.
> 
> I'd like to understand what is VID?

It seems reading some voltages from registers.
For example, BD9571 has "VD18_VID" register which
is prohibit to write. But, BD9574 doesn't have this
register. Also, the driver names "vid_ops",
so I described "VID" here. Perhaps, we should revise
the description to clear. (Please look "Updated description" in this email.)

> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> > ---
> >  drivers/regulator/bd9571mwv-regulator.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/regulator/bd9571mwv-regulator.c
> > b/drivers/regulator/bd9571mwv-regulator.c
> > index 02120b0..041339b 100644
> > --- a/drivers/regulator/bd9571mwv-regulator.c
> > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > @@ -1,6 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0
> >  /*
> > - * ROHM BD9571MWV-M regulator driver
> > + * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
> >   *
> >   * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
> >   *
> > @@ -9,6 +9,7 @@
> >   * NOTE: VD09 is missing
> >   */
> >
> > +#include <linux/mfd/rohm-generic.h>
> >  #include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > @@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct
> > platform_device *pdev)
> >  	struct regulator_dev *rdev;
> >  	unsigned int val;
> >  	int i;
> > +	enum rohm_chip_type chip = platform_get_device_id(pdev)-
> > >driver_data;
> >
> >  	bdreg = devm_kzalloc(&pdev->dev, sizeof(*bdreg), GFP_KERNEL);
> >  	if (!bdreg)
> > @@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct
> > platform_device *pdev)
> >  	config.regmap = bdreg->regmap;
> >
> >  	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> > +		/* BD9574MWF supports DVFS only */
> > +		if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id
> > != DVFS)
> > +			continue;
> 
> Does this mean that reading VD09 voltage is not supported by driver?

Yes. Also, reading VD{18,25,33} voltage are not supported.

> (I
> assumed VD09 initial voltage can be set from eeprom(?) and read by
> driver - I may be wrong though). Perhaps it is worth mentioning in the
> commit message as a driver restriction?

Yes, these voltage can be set from eeprom and read by driver.
So, I updated the description like below.

-- Updated description --
Add support for BD9574MWF which is similar chip with BD9571MWV.
Note that since BD9574MWF doesn't have avs_ops and vid_ops
related registers, this driver avoids to use these registers
if BD9574MWF is used.
------------------------

> And just asking out of the curiosity - are the other voltage rails
> listed in data-sheet (VD18, DDR0, VD33, VD09 and LDO1,...,LDO4) set-up
> from DT as fixed-regulators? Any reason why they are not set-up here?

TBH, I don't know why. Perhaps, the driver cannot read DDR0, LDO[1-4] values?

Best regards,
Yoshihiro Shimoda

Re: [PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support

[Vaittinen, Matti]

Hello Shimoda-san,

On Mon, 2020-12-14 at 04:57 +0000, Yoshihiro Shimoda wrote:
> Hello Matti-san,
> 
> > From: Vaittinen, Matti, Sent: Friday, December 11, 2020 9:34 PM
> > 
> > Hello again Shimada-san,
> > 
> > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > > Add support for BD9574MWF which is silimar chip with BD9571MWV.
> > > Note that BD9574MWF doesn't support AVS and VID.
> > 
> > I'd like to understand what is VID?
> 
> It seems reading some voltages from registers.
> For example, BD9571 has "VD18_VID" register which
> is prohibit to write. But, BD9574 doesn't have this
> register. Also, the driver names "vid_ops",
> so I described "VID" here. Perhaps, we should revise
> the description to clear. (Please look "Updated description" in this
> email.)

Thank you for detailed explanation. So as far as I understood - VID is
a register which displays the current output voltage, right? If I am
not mistaken, this is different from register where (initial) voltage
is set?

> 
> > > Signed-off-by: Yoshihiro Shimoda <
> > > yoshihiro.shimoda.uh@renesas.com>
> > > ---
> > >  drivers/regulator/bd9571mwv-regulator.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/regulator/bd9571mwv-regulator.c
> > > b/drivers/regulator/bd9571mwv-regulator.c
> > > index 02120b0..041339b 100644
> > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > @@ -1,6 +1,6 @@
> > >  // SPDX-License-Identifier: GPL-2.0
> > >  /*
> > > - * ROHM BD9571MWV-M regulator driver
> > > + * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
> > >   *
> > >   * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com
> > > >
> > >   *
> > > @@ -9,6 +9,7 @@
> > >   * NOTE: VD09 is missing
> > >   */
> > > 
> > > +#include <linux/mfd/rohm-generic.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > > @@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct
> > > platform_device *pdev)
> > >  	struct regulator_dev *rdev;
> > >  	unsigned int val;
> > >  	int i;
> > > +	enum rohm_chip_type chip = platform_get_device_id(pdev)-
> > > > driver_data;
> > > 
> > >  	bdreg = devm_kzalloc(&pdev->dev, sizeof(*bdreg), GFP_KERNEL);
> > >  	if (!bdreg)
> > > @@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct
> > > platform_device *pdev)
> > >  	config.regmap = bdreg->regmap;
> > > 
> > >  	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> > > +		/* BD9574MWF supports DVFS only */
> > > +		if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id
> > > != DVFS)
> > > +			continue;
> > 
> > Does this mean that reading VD09 voltage is not supported by
> > driver?
> 
> Yes. Also, reading VD{18,25,33} voltage are not supported.

I think that would be excellent comment here. Maybe something like: "We
don't support voltage rails VD{09,18,25,33} by this driver on BD9574."

> 
> > (I
> > assumed VD09 initial voltage can be set from eeprom(?) and read by
> > driver - I may be wrong though). Perhaps it is worth mentioning in
> > the
> > commit message as a driver restriction?
> 
> Yes, these voltage can be set from eeprom and read by driver.
> So, I updated the description like below.
> 
> -- Updated description --
> Add support for BD9574MWF which is similar chip with BD9571MWV.
> Note that since BD9574MWF doesn't have avs_ops and vid_ops
> related registers, this driver avoids to use these registers
> if BD9574MWF is used.
> ------------------------

Thank you :) What I was after is that I would like to leave a note
about 'what could be improved' or about what is the 'software
limitation' here so that if anyone later needs the other voltage rails
he would have a hint about what could be done.

Do you think mentioning that "the VD09 voltage could be read from PMIC
but that is not supported by this commit" in commit message would be
Ok?

> 
> > And just asking out of the curiosity - are the other voltage rails
> > listed in data-sheet (VD18, DDR0, VD33, VD09 and LDO1,...,LDO4)
> > set-up
> > from DT as fixed-regulators? Any reason why they are not set-up
> > here?
> 
> TBH, I don't know why. Perhaps, the driver cannot read DDR0, LDO[1-4] 
> values?

I also think that all voltages can't be read. I was just thinking that
it might make sense to always create the fixed regulators from PMIC
driver - because if PMIC is used - then these voltage rails do exist.
(This was just a question so that I could learn - not so much of a
review comment.)

If you re-spin the series for other fixups - then I would appreciate
some comment about omitting the rest of the voltage outputs.

Other than that - for what it is worth:

Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>


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)

RE: [PATCH v2 04/10] regulator: bd9571mwv: Add BD9574MWF support

[Yoshihiro Shimoda]

Hello Matti-san,

> From: Vaittinen, Matti, Sent: Monday, December 14, 2020 4:13 PM
> 
> Hello Shimoda-san,
> 
> On Mon, 2020-12-14 at 04:57 +0000, Yoshihiro Shimoda wrote:
> > Hello Matti-san,
> >
> > > From: Vaittinen, Matti, Sent: Friday, December 11, 2020 9:34 PM
> > >
> > > Hello again Shimada-san,
> > >
> > > On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > > > Add support for BD9574MWF which is silimar chip with BD9571MWV.
> > > > Note that BD9574MWF doesn't support AVS and VID.
> > >
> > > I'd like to understand what is VID?
> >
> > It seems reading some voltages from registers.
> > For example, BD9571 has "VD18_VID" register which
> > is prohibit to write. But, BD9574 doesn't have this
> > register. Also, the driver names "vid_ops",
> > so I described "VID" here. Perhaps, we should revise
> > the description to clear. (Please look "Updated description" in this
> > email.)
> 
> Thank you for detailed explanation. So as far as I understood - VID is
> a register which displays the current output voltage, right?

Yes.

> If I am
> not mistaken, this is different from register where (initial) voltage
> is set?

Yes. I checked on my environment (H3 Salvator-XS).

> > > > Signed-off-by: Yoshihiro Shimoda <
> > > > yoshihiro.shimoda.uh@renesas.com>
> > > > ---
> > > >  drivers/regulator/bd9571mwv-regulator.c | 10 ++++++++--
> > > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/regulator/bd9571mwv-regulator.c
> > > > b/drivers/regulator/bd9571mwv-regulator.c
> > > > index 02120b0..041339b 100644
> > > > --- a/drivers/regulator/bd9571mwv-regulator.c
> > > > +++ b/drivers/regulator/bd9571mwv-regulator.c
> > > > @@ -1,6 +1,6 @@
> > > >  // SPDX-License-Identifier: GPL-2.0
> > > >  /*
> > > > - * ROHM BD9571MWV-M regulator driver
> > > > + * ROHM BD9571MWV-M and BD9574MWF-M regulator driver
> > > >   *
> > > >   * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com
> > > > >
> > > >   *
> > > > @@ -9,6 +9,7 @@
> > > >   * NOTE: VD09 is missing
> > > >   */
> > > >
> > > > +#include <linux/mfd/rohm-generic.h>
> > > >  #include <linux/module.h>
> > > >  #include <linux/of.h>
> > > >  #include <linux/platform_device.h>
> > > > @@ -277,6 +278,7 @@ static int bd9571mwv_regulator_probe(struct
> > > > platform_device *pdev)
> > > >  	struct regulator_dev *rdev;
> > > >  	unsigned int val;
> > > >  	int i;
> > > > +	enum rohm_chip_type chip = platform_get_device_id(pdev)-
> > > > > driver_data;
> > > >
> > > >  	bdreg = devm_kzalloc(&pdev->dev, sizeof(*bdreg), GFP_KERNEL);
> > > >  	if (!bdreg)
> > > > @@ -292,6 +294,9 @@ static int bd9571mwv_regulator_probe(struct
> > > > platform_device *pdev)
> > > >  	config.regmap = bdreg->regmap;
> > > >
> > > >  	for (i = 0; i < ARRAY_SIZE(regulators); i++) {
> > > > +		/* BD9574MWF supports DVFS only */
> > > > +		if (chip == ROHM_CHIP_TYPE_BD9574 && regulators[i].id
> > > > != DVFS)
> > > > +			continue;
> > >
> > > Does this mean that reading VD09 voltage is not supported by
> > > driver?
> >
> > Yes. Also, reading VD{18,25,33} voltage are not supported.
> 
> I think that would be excellent comment here. Maybe something like: "We
> don't support voltage rails VD{09,18,25,33} by this driver on BD9574."

Thank you for the suggestion! I'll use this comment.

> > > (I
> > > assumed VD09 initial voltage can be set from eeprom(?) and read by
> > > driver - I may be wrong though). Perhaps it is worth mentioning in
> > > the
> > > commit message as a driver restriction?
> >
> > Yes, these voltage can be set from eeprom and read by driver.
> > So, I updated the description like below.
> >
> > -- Updated description --
> > Add support for BD9574MWF which is similar chip with BD9571MWV.
> > Note that since BD9574MWF doesn't have avs_ops and vid_ops
> > related registers, this driver avoids to use these registers
> > if BD9574MWF is used.
> > ------------------------
> 
> Thank you :) What I was after is that I would like to leave a note
> about 'what could be improved' or about what is the 'software
> limitation' here so that if anyone later needs the other voltage rails
> he would have a hint about what could be done.
> 
> Do you think mentioning that "the VD09 voltage could be read from PMIC
> but that is not supported by this commit" in commit message would be
> Ok?

I think OK because VD09 could be read from "BD9574MWF_VD09_VINIT"
register, but that is not supported but this commit.

> > > And just asking out of the curiosity - are the other voltage rails
> > > listed in data-sheet (VD18, DDR0, VD33, VD09 and LDO1,...,LDO4)
> > > set-up
> > > from DT as fixed-regulators? Any reason why they are not set-up
> > > here?
> >
> > TBH, I don't know why. Perhaps, the driver cannot read DDR0, LDO[1-4]
> > values?
> 
> I also think that all voltages can't be read. I was just thinking that
> it might make sense to always create the fixed regulators from PMIC
> driver - because if PMIC is used - then these voltage rails do exist.
> (This was just a question so that I could learn - not so much of a
> review comment.)
> 
> If you re-spin the series for other fixups - then I would appreciate
> some comment about omitting the rest of the voltage outputs.
> 
> Other than that - for what it is worth:
> 
> Reviewed-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

Thank you very much for your review!

Best regards,
Yoshihiro Shimoda

[PATCH v2 05/10] gpio: bd9571mwv: Use the SPDX license identifier

[Yoshihiro Shimoda]

Use the SPDX license identifier instead of a local description.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/gpio/gpio-bd9571mwv.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-bd9571mwv.c b/drivers/gpio/gpio-bd9571mwv.c
index c0abc9c..abb622c 100644
--- a/drivers/gpio/gpio-bd9571mwv.c
+++ b/drivers/gpio/gpio-bd9571mwv.c
@@ -1,17 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * ROHM BD9571MWV-M GPIO driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether expressed or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License version 2 for more details.
- *
  * Based on the TPS65086 driver
  *
  * NOTE: Interrupts are not supported yet.
-- 
2.7.4

Re: [PATCH v2 05/10] gpio: bd9571mwv: Use the SPDX license identifier

[Geert Uytterhoeven]

On Fri, Dec 11, 2020 at 2:47 PM Yoshihiro Shimoda
<yoshihiro.shimoda.uh@renesas.com> wrote:
> Use the SPDX license identifier instead of a local description.
>
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[PATCH v2 06/10] gpio: bd9571mwv: rid of using struct bd9571mwv

[Yoshihiro Shimoda]

To simplify this driver, use dev_get_regmap() and
rid of using struct bd9571mwv.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/gpio/gpio-bd9571mwv.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-bd9571mwv.c b/drivers/gpio/gpio-bd9571mwv.c
index abb622c..0e5395f 100644
--- a/drivers/gpio/gpio-bd9571mwv.c
+++ b/drivers/gpio/gpio-bd9571mwv.c
@@ -16,8 +16,8 @@
 #include <linux/mfd/bd9571mwv.h>
 
 struct bd9571mwv_gpio {
+	struct regmap *regmap;
 	struct gpio_chip chip;
-	struct bd9571mwv *bd;
 };
 
 static int bd9571mwv_gpio_get_direction(struct gpio_chip *chip,
@@ -26,7 +26,7 @@ static int bd9571mwv_gpio_get_direction(struct gpio_chip *chip,
 	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
 	int ret, val;
 
-	ret = regmap_read(gpio->bd->regmap, BD9571MWV_GPIO_DIR, &val);
+	ret = regmap_read(gpio->regmap, BD9571MWV_GPIO_DIR, &val);
 	if (ret < 0)
 		return ret;
 	if (val & BIT(offset))
@@ -40,8 +40,7 @@ static int bd9571mwv_gpio_direction_input(struct gpio_chip *chip,
 {
 	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
 
-	regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_DIR,
-			   BIT(offset), 0);
+	regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_DIR, BIT(offset), 0);
 
 	return 0;
 }
@@ -52,9 +51,9 @@ static int bd9571mwv_gpio_direction_output(struct gpio_chip *chip,
 	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
 
 	/* Set the initial value */
-	regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_OUT,
+	regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_OUT,
 			   BIT(offset), value ? BIT(offset) : 0);
-	regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_DIR,
+	regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_DIR,
 			   BIT(offset), BIT(offset));
 
 	return 0;
@@ -65,7 +64,7 @@ static int bd9571mwv_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
 	int ret, val;
 
-	ret = regmap_read(gpio->bd->regmap, BD9571MWV_GPIO_IN, &val);
+	ret = regmap_read(gpio->regmap, BD9571MWV_GPIO_IN, &val);
 	if (ret < 0)
 		return ret;
 
@@ -77,7 +76,7 @@ static void bd9571mwv_gpio_set(struct gpio_chip *chip, unsigned int offset,
 {
 	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
 
-	regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_OUT,
+	regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_OUT,
 			   BIT(offset), value ? BIT(offset) : 0);
 }
 
@@ -105,9 +104,9 @@ static int bd9571mwv_gpio_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, gpio);
 
-	gpio->bd = dev_get_drvdata(pdev->dev.parent);
+	gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
 	gpio->chip = template_chip;
-	gpio->chip.parent = gpio->bd->dev;
+	gpio->chip.parent = pdev->dev.parent;
 
 	ret = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
 	if (ret < 0) {
-- 
2.7.4

Re: [PATCH v2 06/10] gpio: bd9571mwv: rid of using struct bd9571mwv

[Vaittinen, Matti]

On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> To simplify this driver, use dev_get_regmap() and
> rid of using struct bd9571mwv.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

FWIW:
Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

> ---
>  drivers/gpio/gpio-bd9571mwv.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-bd9571mwv.c b/drivers/gpio/gpio-
> bd9571mwv.c
> index abb622c..0e5395f 100644
> --- a/drivers/gpio/gpio-bd9571mwv.c
> +++ b/drivers/gpio/gpio-bd9571mwv.c
> @@ -16,8 +16,8 @@
>  #include <linux/mfd/bd9571mwv.h>
>  
>  struct bd9571mwv_gpio {
> +	struct regmap *regmap;
>  	struct gpio_chip chip;
> -	struct bd9571mwv *bd;
>  };
>  
>  static int bd9571mwv_gpio_get_direction(struct gpio_chip *chip,
> @@ -26,7 +26,7 @@ static int bd9571mwv_gpio_get_direction(struct
> gpio_chip *chip,
>  	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
>  	int ret, val;
>  
> -	ret = regmap_read(gpio->bd->regmap, BD9571MWV_GPIO_DIR, &val);
> +	ret = regmap_read(gpio->regmap, BD9571MWV_GPIO_DIR, &val);
>  	if (ret < 0)
>  		return ret;
>  	if (val & BIT(offset))
> @@ -40,8 +40,7 @@ static int bd9571mwv_gpio_direction_input(struct
> gpio_chip *chip,
>  {
>  	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
>  
> -	regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_DIR,
> -			   BIT(offset), 0);
> +	regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_DIR,
> BIT(offset), 0);
>  
>  	return 0;
>  }
> @@ -52,9 +51,9 @@ static int bd9571mwv_gpio_direction_output(struct
> gpio_chip *chip,
>  	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
>  
>  	/* Set the initial value */
> -	regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_OUT,
> +	regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_OUT,
>  			   BIT(offset), value ? BIT(offset) : 0);
> -	regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_DIR,
> +	regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_DIR,
>  			   BIT(offset), BIT(offset));
>  
>  	return 0;
> @@ -65,7 +64,7 @@ static int bd9571mwv_gpio_get(struct gpio_chip
> *chip, unsigned int offset)
>  	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
>  	int ret, val;
>  
> -	ret = regmap_read(gpio->bd->regmap, BD9571MWV_GPIO_IN, &val);
> +	ret = regmap_read(gpio->regmap, BD9571MWV_GPIO_IN, &val);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -77,7 +76,7 @@ static void bd9571mwv_gpio_set(struct gpio_chip
> *chip, unsigned int offset,
>  {
>  	struct bd9571mwv_gpio *gpio = gpiochip_get_data(chip);
>  
> -	regmap_update_bits(gpio->bd->regmap, BD9571MWV_GPIO_OUT,
> +	regmap_update_bits(gpio->regmap, BD9571MWV_GPIO_OUT,
>  			   BIT(offset), value ? BIT(offset) : 0);
>  }
>  
> @@ -105,9 +104,9 @@ static int bd9571mwv_gpio_probe(struct
> platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, gpio);
>  
> -	gpio->bd = dev_get_drvdata(pdev->dev.parent);
> +	gpio->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>  	gpio->chip = template_chip;
> -	gpio->chip.parent = gpio->bd->dev;
> +	gpio->chip.parent = pdev->dev.parent;
>  
>  	ret = devm_gpiochip_add_data(&pdev->dev, &gpio->chip, gpio);
>  	if (ret < 0) {

[PATCH v2 07/10] gpio: bd9571mwv: Add BD9574MWF support

[Yoshihiro Shimoda]

Add support for BD9574MWF which is silimar chip with BD9571MWV.
Note that BD9574MWF has an additional feature, but doesn't
support it for now.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/gpio/gpio-bd9571mwv.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-bd9571mwv.c b/drivers/gpio/gpio-bd9571mwv.c
index 0e5395f..df6102b 100644
--- a/drivers/gpio/gpio-bd9571mwv.c
+++ b/drivers/gpio/gpio-bd9571mwv.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * ROHM BD9571MWV-M GPIO driver
+ * ROHM BD9571MWV-M and BD9574MWF-M GPIO driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  *
@@ -10,6 +10,7 @@
  */
 
 #include <linux/gpio/driver.h>
+#include <linux/mfd/rohm-generic.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
@@ -118,7 +119,8 @@ static int bd9571mwv_gpio_probe(struct platform_device *pdev)
 }
 
 static const struct platform_device_id bd9571mwv_gpio_id_table[] = {
-	{ "bd9571mwv-gpio", },
+	{ "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
+	{ "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(platform, bd9571mwv_gpio_id_table);
-- 
2.7.4

Re: [PATCH v2 07/10] gpio: bd9571mwv: Add BD9574MWF support

[Vaittinen, Matti]

On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> Add support for BD9574MWF which is silimar chip with BD9571MWV.
> Note that BD9574MWF has an additional feature, but doesn't
> support it for now.

nit:
Perhaps mention which feature? And I think you mean the driver does not
support it yet?

> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

FWIW:
Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

> ---
>  drivers/gpio/gpio-bd9571mwv.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-bd9571mwv.c b/drivers/gpio/gpio-
> bd9571mwv.c
> index 0e5395f..df6102b 100644
> --- a/drivers/gpio/gpio-bd9571mwv.c
> +++ b/drivers/gpio/gpio-bd9571mwv.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /*
> - * ROHM BD9571MWV-M GPIO driver
> + * ROHM BD9571MWV-M and BD9574MWF-M GPIO driver
>   *
>   * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
>   *
> @@ -10,6 +10,7 @@
>   */
>  
>  #include <linux/gpio/driver.h>
> +#include <linux/mfd/rohm-generic.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  
> @@ -118,7 +119,8 @@ static int bd9571mwv_gpio_probe(struct
> platform_device *pdev)
>  }
>  
>  static const struct platform_device_id bd9571mwv_gpio_id_table[] = {
> -	{ "bd9571mwv-gpio", },
> +	{ "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
> +	{ "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },

I guess these CHIP_TYPES are used by subsequent patches?

I guess this means the existing functionality in both chips is same,
right? (GPIO register addresses etc? - I don't have BD9571 data-sheet
so I can't check)

>  	{ /* sentinel */ }
>  };
>  MODULE_DEVICE_TABLE(platform, bd9571mwv_gpio_id_table);

RE: [PATCH v2 07/10] gpio: bd9571mwv: Add BD9574MWF support

[Yoshihiro Shimoda]

Hi Matti-san,

Thank you for your review!

> From: Vaittinen, Matti, Sent: Friday, December 11, 2020 9:55 PM
> 
> On Fri, 2020-12-11 at 20:27 +0900, Yoshihiro Shimoda wrote:
> > Add support for BD9574MWF which is silimar chip with BD9571MWV.
> > Note that BD9574MWF has an additional feature, but doesn't
> > support it for now.
> 
> nit:
> Perhaps mention which feature?

BD9574MWF GPIO[01] have 4 functions like below.
 1) GPIO, 2) "RECOV_GPOUT", 3) "FREQSEL", 4) "RTC_IN"

It seems "pinctrl" features though and I don't know
these features in detail for now.

> And I think you mean the driver does not support it yet?

You're correct. Now this driver only support the 1) GPIO.

> >
> > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
> 
> FWIW:
> Reviewed-By: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>

Thank you for your review!

> > ---
> >  drivers/gpio/gpio-bd9571mwv.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpio/gpio-bd9571mwv.c b/drivers/gpio/gpio-
> > bd9571mwv.c
> > index 0e5395f..df6102b 100644
> > --- a/drivers/gpio/gpio-bd9571mwv.c
> > +++ b/drivers/gpio/gpio-bd9571mwv.c
> > @@ -1,6 +1,6 @@
> >  // SPDX-License-Identifier: GPL-2.0-only
> >  /*
> > - * ROHM BD9571MWV-M GPIO driver
> > + * ROHM BD9571MWV-M and BD9574MWF-M GPIO driver
> >   *
> >   * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
> >   *
> > @@ -10,6 +10,7 @@
> >   */
> >
> >  #include <linux/gpio/driver.h>
> > +#include <linux/mfd/rohm-generic.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> >
> > @@ -118,7 +119,8 @@ static int bd9571mwv_gpio_probe(struct
> > platform_device *pdev)
> >  }
> >
> >  static const struct platform_device_id bd9571mwv_gpio_id_table[] = {
> > -	{ "bd9571mwv-gpio", },
> > +	{ "bd9571mwv-gpio", ROHM_CHIP_TYPE_BD9571 },
> > +	{ "bd9574mwf-gpio", ROHM_CHIP_TYPE_BD9574 },
> 
> I guess these CHIP_TYPES are used by subsequent patches?
> 
> I guess this means the existing functionality in both chips is same,
> right? (GPIO register addresses etc? - I don't have BD9571 data-sheet
> so I can't check)

Yes, the existing functionality in both chips is same.
GPIO register addresses and bits are the same.
Note that BD9574MWF has one more register, but the driver
doesn't use it for now.

Best regards,
Yoshihiro Shimoda

[PATCH v2 08/10] mfd: bd9571mwv: Use the SPDX license identifier

[Yoshihiro Shimoda]

Use the SPDX license identifier instead of a local description.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mfd/bd9571mwv.c       | 10 +---------
 include/linux/mfd/bd9571mwv.h | 10 +---------
 2 files changed, 2 insertions(+), 18 deletions(-)

diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
index fab3cdc..80c6ef0 100644
--- a/drivers/mfd/bd9571mwv.c
+++ b/drivers/mfd/bd9571mwv.c
@@ -1,17 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * ROHM BD9571MWV-M MFD driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether expressed or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License version 2 for more details.
- *
  * Based on the TPS65086 driver
  */
 
diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
index eb05569..bcc7092 100644
--- a/include/linux/mfd/bd9571mwv.h
+++ b/include/linux/mfd/bd9571mwv.h
@@ -1,17 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
 /*
  * ROHM BD9571MWV-M driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- *
- * This program is distributed "as is" WITHOUT ANY WARRANTY of any
- * kind, whether expressed or implied; without even the implied warranty
- * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License version 2 for more details.
- *
  * Based on the TPS65086 driver
  */
 
-- 
2.7.4

RE: [PATCH v2 08/10] mfd: bd9571mwv: Use the SPDX license identifier

[Yoshihiro Shimoda]

> From: Yoshihiro Shimoda, Sent: Friday, December 11, 2020 8:28 PM
> 
> Use the SPDX license identifier instead of a local description.
> 
> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

I forgot to add Geert-san's Reviewed-by here. So, I'll submit v3 patch series later.

https://patchwork.kernel.org/project/linux-renesas-soc/patch/1607414643-25498-2-git-send-email-yoshihiro.shimoda.uh@renesas.com/#23832735

Best regards,
Yoshihiro Shimoda

[PATCH v2 09/10] mfd: bd9571mwv: Make the driver more generic

[Yoshihiro Shimoda]

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.

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       | 71 +++++++++++++++++++++++++++++++++++--------
 include/linux/mfd/bd9571mwv.h | 18 ++---------
 2 files changed, 62 insertions(+), 27 deletions(-)

diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
index 80c6ef0..adb9e3d 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,34 @@
 
 #include <linux/mfd/bd9571mwv.h>
 
+/**
+ * struct bd957x_data - internal data for the bd957x driver
+ *
+ * Internal data to distinguish bd957x variants
+ */
+struct bd957x_data {
+	char *part_name;
+	const struct regmap_config *regmap_config;
+	const struct regmap_irq_chip *irq_chip;
+	const struct mfd_cell *cells;
+	int num_cells;
+};
+
+/**
+ * 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;
+	const struct bd957x_data *data;
+
+	/* IRQ Data */
+	int irq;
+	struct regmap_irq_chip_data *irq_data;
+};
+
 static const struct mfd_cell bd9571mwv_cells[] = {
 	{ .name = "bd9571mwv-regulator", },
 	{ .name = "bd9571mwv-gpio", },
@@ -102,6 +131,14 @@ static struct regmap_irq_chip bd9571mwv_irq_chip = {
 	.num_irqs	= ARRAY_SIZE(bd9571mwv_irqs),
 };
 
+static const struct bd957x_data bd9571mwv_data = {
+	.part_name = BD9571MWV_PART_NAME,
+	.regmap_config = &bd9571mwv_regmap_config,
+	.irq_chip = &bd9571mwv_irq_chip,
+	.cells = bd9571mwv_cells,
+	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
+};
+
 static int bd9571mwv_identify(struct bd9571mwv *bd)
 {
 	struct device *dev = bd->dev;
@@ -127,13 +164,6 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
 			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);
 	if (ret) {
 		dev_err(dev, "Failed to read revision register (ret=%i)\n",
@@ -141,7 +171,8 @@ static int bd9571mwv_identify(struct bd9571mwv *bd)
 		return ret;
 	}
 
-	dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
+	dev_info(dev, "Device: %s rev. %d\n", bd->data->part_name,
+		 value & 0xff);
 
 	return 0;
 }
@@ -160,7 +191,23 @@ static int bd9571mwv_probe(struct i2c_client *client,
 	bd->dev = &client->dev;
 	bd->irq = client->irq;
 
-	bd->regmap = devm_regmap_init_i2c(client, &bd9571mwv_regmap_config);
+	/* Read the PMIC product code */
+	ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed reading at 0x%02x\n",
+			BD9571MWV_PRODUCT_CODE);
+		return ret;
+	}
+	switch (ret) {
+	case BD9571MWV_PRODUCT_CODE_VAL:
+		bd->data = &bd9571mwv_data;
+		break;
+	default:
+		dev_err(bd->dev, "Unsupported device 0x%x\n", ret);
+		return -ENOENT;
+	}
+
+	bd->regmap = devm_regmap_init_i2c(client, bd->data->regmap_config);
 	if (IS_ERR(bd->regmap)) {
 		dev_err(bd->dev, "Failed to initialize register map\n");
 		return PTR_ERR(bd->regmap);
@@ -171,14 +218,14 @@ static int bd9571mwv_probe(struct i2c_client *client,
 		return ret;
 
 	ret = regmap_add_irq_chip(bd->regmap, bd->irq, IRQF_ONESHOT, 0,
-				  &bd9571mwv_irq_chip, &bd->irq_data);
+				  bd->data->irq_chip, &bd->irq_data);
 	if (ret) {
 		dev_err(bd->dev, "Failed to register IRQ chip\n");
 		return ret;
 	}
 
-	ret = mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, bd9571mwv_cells,
-			      ARRAY_SIZE(bd9571mwv_cells), NULL, 0,
+	ret = mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, bd->data->cells,
+			      bd->data->num_cells, NULL, 0,
 			      regmap_irq_get_domain(bd->irq_data));
 	if (ret) {
 		regmap_del_irq_chip(bd->irq, bd->irq_data);
diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
index bcc7092..5ab976a 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
  */
@@ -83,6 +84,8 @@
 
 #define BD9571MWV_ACCESS_KEY			0xff
 
+#define BD9571MWV_PART_NAME			"BD9571MWV"
+
 /* Define the BD9571MWV IRQ numbers */
 enum bd9571mwv_irqs {
 	BD9571MWV_IRQ_MD1,
@@ -94,19 +97,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 */
-- 
2.7.4

Re: [PATCH v2 09/10] mfd: bd9571mwv: Make the driver more generic

[Vaittinen, Matti]

Hi Yoshihiro-san,


On Fri, 2020-12-11 at 20:27 +0900, 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.
> 
> 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       | 71
> +++++++++++++++++++++++++++++++++++--------
>  include/linux/mfd/bd9571mwv.h | 18 ++---------
>  2 files changed, 62 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> index 80c6ef0..adb9e3d 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,34 @@
>  
>  #include <linux/mfd/bd9571mwv.h>
>  
> +/**
> + * struct bd957x_data - internal data for the bd957x driver
> + *
> + * Internal data to distinguish bd957x variants
> + */
> +struct bd957x_data {
> +	char *part_name;
> +	const struct regmap_config *regmap_config;
> +	const struct regmap_irq_chip *irq_chip;
> +	const struct mfd_cell *cells;
> +	int num_cells;
> +};
> +
> +/**
> + * 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;
> +	const struct bd957x_data *data;
> +
> +	/* IRQ Data */
> +	int irq;
> +	struct regmap_irq_chip_data *irq_data;
> +};
> +

I still don't see why you actually need this structure?

>  static const struct mfd_cell bd9571mwv_cells[] = {
>  	{ .name = "bd9571mwv-regulator", },
>  	{ .name = "bd9571mwv-gpio", },
> @@ -102,6 +131,14 @@ static struct regmap_irq_chip bd9571mwv_irq_chip
> = {
>  	.num_irqs	= ARRAY_SIZE(bd9571mwv_irqs),
>  };
>  
> +static const struct bd957x_data bd9571mwv_data = {
> +	.part_name = BD9571MWV_PART_NAME,
> +	.regmap_config = &bd9571mwv_regmap_config,
> +	.irq_chip = &bd9571mwv_irq_chip,
> +	.cells = bd9571mwv_cells,
> +	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
> +};
> +
>  static int bd9571mwv_identify(struct bd9571mwv *bd)
>  {
>  	struct device *dev = bd->dev;
> @@ -127,13 +164,6 @@ static int bd9571mwv_identify(struct bd9571mwv
> *bd)
>  			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);
>  	if (ret) {
>  		dev_err(dev, "Failed to read revision register
> (ret=%i)\n",
> @@ -141,7 +171,8 @@ static int bd9571mwv_identify(struct bd9571mwv
> *bd)
>  		return ret;
>  	}
>  
> -	dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> +	dev_info(dev, "Device: %s rev. %d\n", bd->data->part_name,
> +		 value & 0xff);
>  
>  	return 0;
>  }
> @@ -160,7 +191,23 @@ static int bd9571mwv_probe(struct i2c_client
> *client,
>  	bd->dev = &client->dev;
>  	bd->irq = client->irq;
>  
> -	bd->regmap = devm_regmap_init_i2c(client,
> &bd9571mwv_regmap_config);
> +	/* Read the PMIC product code */
> +	ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed reading at 0x%02x\n",
> +			BD9571MWV_PRODUCT_CODE);
> +		return ret;
> +	}
> +	switch (ret) {
> +	case BD9571MWV_PRODUCT_CODE_VAL:
> +		bd->data = &bd9571mwv_data;
> +		break;
> +	default:
> +		dev_err(bd->dev, "Unsupported device 0x%x\n", ret);
> +		return -ENOENT;
> +	}
> +
> +	bd->regmap = devm_regmap_init_i2c(client, bd->data-
> >regmap_config);
>  	if (IS_ERR(bd->regmap)) {
>  		dev_err(bd->dev, "Failed to initialize register
> map\n");
>  		return PTR_ERR(bd->regmap);
> @@ -171,14 +218,14 @@ static int bd9571mwv_probe(struct i2c_client
> *client,
>  		return ret;
>  
>  	ret = regmap_add_irq_chip(bd->regmap, bd->irq, IRQF_ONESHOT, 0,
> -				  &bd9571mwv_irq_chip, &bd->irq_data);
> +				  bd->data->irq_chip, &bd->irq_data);

I think you already did the big task when you cleaned up the sub-
drivers from using the struct bd9571mwv. Thumbs up for that!

So, as I said in comment to previous version - I don't see this struct
bd9571mwv being really used anywhere else but as an argument to IC
identification function and argument for the remove. I think that by
switching regmap_add_irq_chip to devm_regmap_add_irq_chip you could get
rid of the remove, error cleanup path and the i2c_clientdata. And if
you revised the arguments for identification function you could
probably further clean the struct definitions.

But as I previously said - this is only a 'nit' from me. I appreciate
your work with these drivers!

>  	if (ret) {
>  		dev_err(bd->dev, "Failed to register IRQ chip\n");
>  		return ret;
>  	}
>  
> -	ret = mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO,
> bd9571mwv_cells,
> -			      ARRAY_SIZE(bd9571mwv_cells), NULL, 0,
> +	ret = mfd_add_devices(bd->dev, PLATFORM_DEVID_AUTO, bd->data-
> >cells,
> +			      bd->data->num_cells, NULL, 0,
>  			      regmap_irq_get_domain(bd->irq_data));
>  	if (ret) {
>  		regmap_del_irq_chip(bd->irq, bd->irq_data);
> diff --git a/include/linux/mfd/bd9571mwv.h
> b/include/linux/mfd/bd9571mwv.h
> index bcc7092..5ab976a 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
>   */
> @@ -83,6 +84,8 @@
>  
>  #define BD9571MWV_ACCESS_KEY			0xff
>  
> +#define BD9571MWV_PART_NAME			"BD9571MWV"
> +
>  /* Define the BD9571MWV IRQ numbers */
>  enum bd9571mwv_irqs {
>  	BD9571MWV_IRQ_MD1,
> @@ -94,19 +97,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 */


Best Regards
    Matti Vaittinen

RE: [PATCH v2 09/10] mfd: bd9571mwv: Make the driver more generic

[Yoshihiro Shimoda]

Hi Matti-san,

Thank you for your review!

> From: Vaittinen, Matti, Sent: Friday, December 11, 2020 10:29 PM
> 
> Hi Yoshihiro-san,
> 
> On Fri, 2020-12-11 at 20:27 +0900, 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.
> >
> > 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       | 71
> > +++++++++++++++++++++++++++++++++++--------
> >  include/linux/mfd/bd9571mwv.h | 18 ++---------
> >  2 files changed, 62 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
> > index 80c6ef0..adb9e3d 100644
> > --- a/drivers/mfd/bd9571mwv.c
> > +++ b/drivers/mfd/bd9571mwv.c
<snip>
> > +struct bd9571mwv {
> > +	struct device *dev;
> > +	struct regmap *regmap;
> > +	const struct bd957x_data *data;
> > +
> > +	/* IRQ Data */
> > +	int irq;
> > +	struct regmap_irq_chip_data *irq_data;
> > +};
> > +
> 
> I still don't see why you actually need this structure?

I'm sorry. I completely forgot that you said we can remove this
structure in the previous patch's review...

> >  static const struct mfd_cell bd9571mwv_cells[] = {
> >  	{ .name = "bd9571mwv-regulator", },
> >  	{ .name = "bd9571mwv-gpio", },
> > @@ -102,6 +131,14 @@ static struct regmap_irq_chip bd9571mwv_irq_chip
> > = {
> >  	.num_irqs	= ARRAY_SIZE(bd9571mwv_irqs),
> >  };
> >
> > +static const struct bd957x_data bd9571mwv_data = {
> > +	.part_name = BD9571MWV_PART_NAME,
> > +	.regmap_config = &bd9571mwv_regmap_config,
> > +	.irq_chip = &bd9571mwv_irq_chip,
> > +	.cells = bd9571mwv_cells,
> > +	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
> > +};
> > +
> >  static int bd9571mwv_identify(struct bd9571mwv *bd)
> >  {
> >  	struct device *dev = bd->dev;
> > @@ -127,13 +164,6 @@ static int bd9571mwv_identify(struct bd9571mwv
> > *bd)
> >  			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);
> >  	if (ret) {
> >  		dev_err(dev, "Failed to read revision register
> > (ret=%i)\n",
> > @@ -141,7 +171,8 @@ static int bd9571mwv_identify(struct bd9571mwv
> > *bd)
> >  		return ret;
> >  	}
> >
> > -	dev_info(dev, "Device: BD9571MWV rev. %d\n", value & 0xff);
> > +	dev_info(dev, "Device: %s rev. %d\n", bd->data->part_name,
> > +		 value & 0xff);
> >
> >  	return 0;
> >  }
> > @@ -160,7 +191,23 @@ static int bd9571mwv_probe(struct i2c_client
> > *client,
> >  	bd->dev = &client->dev;
> >  	bd->irq = client->irq;
> >
> > -	bd->regmap = devm_regmap_init_i2c(client,
> > &bd9571mwv_regmap_config);
> > +	/* Read the PMIC product code */
> > +	ret = i2c_smbus_read_byte_data(client, BD9571MWV_PRODUCT_CODE);
> > +	if (ret < 0) {
> > +		dev_err(&client->dev, "failed reading at 0x%02x\n",
> > +			BD9571MWV_PRODUCT_CODE);
> > +		return ret;
> > +	}
> > +	switch (ret) {
> > +	case BD9571MWV_PRODUCT_CODE_VAL:
> > +		bd->data = &bd9571mwv_data;
> > +		break;
> > +	default:
> > +		dev_err(bd->dev, "Unsupported device 0x%x\n", ret);
> > +		return -ENOENT;
> > +	}
> > +
> > +	bd->regmap = devm_regmap_init_i2c(client, bd->data-
> > >regmap_config);
> >  	if (IS_ERR(bd->regmap)) {
> >  		dev_err(bd->dev, "Failed to initialize register
> > map\n");
> >  		return PTR_ERR(bd->regmap);
> > @@ -171,14 +218,14 @@ static int bd9571mwv_probe(struct i2c_client
> > *client,
> >  		return ret;
> >
> >  	ret = regmap_add_irq_chip(bd->regmap, bd->irq, IRQF_ONESHOT, 0,
> > -				  &bd9571mwv_irq_chip, &bd->irq_data);
> > +				  bd->data->irq_chip, &bd->irq_data);
> 
> I think you already did the big task when you cleaned up the sub-
> drivers from using the struct bd9571mwv. Thumbs up for that!
> 
> So, as I said in comment to previous version - I don't see this struct
> bd9571mwv being really used anywhere else but as an argument to IC
> identification function and argument for the remove. I think that by
> switching regmap_add_irq_chip to devm_regmap_add_irq_chip you could get
> rid of the remove, error cleanup path and the i2c_clientdata. And if
> you revised the arguments for identification function you could
> probably further clean the struct definitions.

Thank you for the detailed comments. I agreed we can simplify the code
if we use devm_regmap_add_irq_chip. Also, I found a bug in the current
code which we should call devm_mfd_add_devices() instead of
mfd_add_devices(). So, I'll make a fixed patch too.

> But as I previously said - this is only a 'nit' from me. I appreciate
> your work with these drivers!

Thank you very much for your review!

Best regards,
Yoshihiro Shimoda

[PATCH v2 10/10] mfd: bd9571mwv: Add support for BD9574MWF

[Yoshihiro Shimoda]

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       | 86 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mfd/bd9571mwv.h | 18 +++++++--
 2 files changed, 99 insertions(+), 5 deletions(-)

diff --git a/drivers/mfd/bd9571mwv.c b/drivers/mfd/bd9571mwv.c
index adb9e3d..14a17c7 100644
--- a/drivers/mfd/bd9571mwv.c
+++ b/drivers/mfd/bd9571mwv.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /*
- * ROHM BD9571MWV-M MFD driver
+ * ROHM BD9571MWV-M and BD9574MVF-M MFD driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  * Copyright (C) 2020 Renesas Electronics Corporation
@@ -11,6 +11,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/mfd/core.h>
+#include <linux/mfd/rohm-generic.h>
 #include <linux/module.h>
 
 #include <linux/mfd/bd9571mwv.h>
@@ -43,6 +44,7 @@ struct bd9571mwv {
 	struct regmap_irq_chip_data *irq_data;
 };
 
+/* For BD9571MWV */
 static const struct mfd_cell bd9571mwv_cells[] = {
 	{ .name = "bd9571mwv-regulator", },
 	{ .name = "bd9571mwv-gpio", },
@@ -139,6 +141,81 @@ static const struct bd957x_data bd9571mwv_data = {
 	.num_cells = ARRAY_SIZE(bd9571mwv_cells),
 };
 
+/* For BD9574MWF */
+static const struct mfd_cell bd9574mwf_cells[] = {
+	{ .name = "bd9574mwf-regulator", },
+	{ .name = "bd9574mwf-gpio", },
+};
+
+static const struct regmap_range bd9574mwf_readable_yes_ranges[] = {
+	regmap_reg_range(BD9571MWV_VENDOR_CODE, BD9571MWV_PRODUCT_REVISION),
+	regmap_reg_range(BD9571MWV_BKUP_MODE_CNT, BD9571MWV_BKUP_MODE_CNT),
+	regmap_reg_range(BD9571MWV_DVFS_VINIT, BD9571MWV_DVFS_SETVMAX),
+	regmap_reg_range(BD9571MWV_DVFS_SETVID, BD9571MWV_DVFS_MONIVDAC),
+	regmap_reg_range(BD9571MWV_GPIO_IN, BD9571MWV_GPIO_IN),
+	regmap_reg_range(BD9571MWV_GPIO_INT, BD9571MWV_GPIO_INTMASK),
+	regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_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(BD9571MWV_BKUP_MODE_CNT, BD9571MWV_BKUP_MODE_CNT),
+	regmap_reg_range(BD9571MWV_DVFS_SETVID, BD9571MWV_DVFS_SETVID),
+	regmap_reg_range(BD9571MWV_GPIO_DIR, BD9571MWV_GPIO_OUT),
+	regmap_reg_range(BD9571MWV_GPIO_INT_SET, BD9571MWV_GPIO_INTMASK),
+	regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_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(BD9571MWV_DVFS_MONIVDAC, BD9571MWV_DVFS_MONIVDAC),
+	regmap_reg_range(BD9571MWV_GPIO_IN, BD9571MWV_GPIO_IN),
+	regmap_reg_range(BD9571MWV_GPIO_INT, BD9571MWV_GPIO_INT),
+	regmap_reg_range(BD9571MWV_INT_INTREQ, BD9571MWV_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 struct regmap_irq_chip bd9574mwf_irq_chip = {
+	.name		= "bd9574mwf",
+	.status_base	= BD9571MWV_INT_INTREQ,
+	.mask_base	= BD9571MWV_INT_INTMASK,
+	.ack_base	= BD9571MWV_INT_INTREQ,
+	.init_ack_masked = true,
+	.num_regs	= 1,
+	.irqs		= bd9571mwv_irqs,
+	.num_irqs	= ARRAY_SIZE(bd9571mwv_irqs),
+};
+
+static const struct bd957x_data bd9574mwf_data = {
+	.part_name = BD9574MWF_PART_NAME,
+	.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;
@@ -202,6 +279,9 @@ static int bd9571mwv_probe(struct i2c_client *client,
 	case BD9571MWV_PRODUCT_CODE_VAL:
 		bd->data = &bd9571mwv_data;
 		break;
+	case BD9574MWF_PRODUCT_CODE_VAL:
+		bd->data = &bd9574mwf_data;
+		break;
 	default:
 		dev_err(bd->dev, "Unsupported device 0x%x\n", ret);
 		return -ENOENT;
@@ -246,12 +326,14 @@ static int bd9571mwv_remove(struct i2c_client *client)
 
 static const struct of_device_id bd9571mwv_of_match_table[] = {
 	{ .compatible = "rohm,bd9571mwv", },
+	{ .compatible = "rohm,bd9574mwf", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, bd9571mwv_of_match_table);
 
 static const struct i2c_device_id bd9571mwv_id_table[] = {
-	{ "bd9571mwv", 0 },
+	{ "bd9571mwv", ROHM_CHIP_TYPE_BD9571 },
+	{ "bd9574mwf", ROHM_CHIP_TYPE_BD9574 },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(i2c, bd9571mwv_id_table);
diff --git a/include/linux/mfd/bd9571mwv.h b/include/linux/mfd/bd9571mwv.h
index 5ab976a..0fc7789 100644
--- a/include/linux/mfd/bd9571mwv.h
+++ b/include/linux/mfd/bd9571mwv.h
@@ -1,6 +1,6 @@
 /* SPDX-License-Identifier: GPL-2.0-only */
 /*
- * ROHM BD9571MWV-M driver
+ * ROHM BD9571MWV-M and BD9574MWF-M driver
  *
  * Copyright (C) 2017 Marek Vasut <marek.vasut+renesas@gmail.com>
  * Copyright (C) 2020 Renesas Electronics Corporation
@@ -14,11 +14,12 @@
 #include <linux/device.h>
 #include <linux/regmap.h>
 
-/* List of registers for BD9571MWV */
+/* List of registers for BD9571MWV and BD9574MWF */
 #define BD9571MWV_VENDOR_CODE			0x00
 #define BD9571MWV_VENDOR_CODE_VAL		0xdb
 #define BD9571MWV_PRODUCT_CODE			0x01
 #define BD9571MWV_PRODUCT_CODE_VAL		0x60
+#define BD9574MWF_PRODUCT_CODE_VAL		0x74
 #define BD9571MWV_PRODUCT_REVISION		0x02
 
 #define BD9571MWV_I2C_FUSA_MODE			0x10
@@ -48,6 +49,7 @@
 #define BD9571MWV_VD33_VID			0x44
 
 #define BD9571MWV_DVFS_VINIT			0x50
+#define BD9574MWF_VD09_VINIT			0x51
 #define BD9571MWV_DVFS_SETVMAX			0x52
 #define BD9571MWV_DVFS_BOOSTVID			0x53
 #define BD9571MWV_DVFS_SETVID			0x54
@@ -61,6 +63,7 @@
 #define BD9571MWV_GPIO_INT_SET			0x64
 #define BD9571MWV_GPIO_INT			0x65
 #define BD9571MWV_GPIO_INTMASK			0x66
+#define BD9574MWF_GPIO_MUX			0x67
 
 #define BD9571MWV_REG_KEEP(n)			(0x70 + (n))
 
@@ -70,6 +73,8 @@
 #define BD9571MWV_PROT_ERROR_STATUS2		0x83
 #define BD9571MWV_PROT_ERROR_STATUS3		0x84
 #define BD9571MWV_PROT_ERROR_STATUS4		0x85
+#define BD9574MWF_PROT_ERROR_STATUS5		0x86
+#define BD9574MWF_SYSTEM_ERROR_STATUS		0x87
 
 #define BD9571MWV_INT_INTREQ			0x90
 #define BD9571MWV_INT_INTREQ_MD1_INT		BIT(0)
@@ -82,9 +87,16 @@
 #define BD9571MWV_INT_INTREQ_BKUP_TRG_INT	BIT(7)
 #define BD9571MWV_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 BD9571MWV_ACCESS_KEY			0xff
 
 #define BD9571MWV_PART_NAME			"BD9571MWV"
+#define BD9574MWF_PART_NAME			"BD9574MWF"
 
 /* Define the BD9571MWV IRQ numbers */
 enum bd9571mwv_irqs {
@@ -93,7 +105,7 @@ enum bd9571mwv_irqs {
 	BD9571MWV_IRQ_MD2_E2,
 	BD9571MWV_IRQ_PROT_ERR,
 	BD9571MWV_IRQ_GP,
-	BD9571MWV_IRQ_128H_OF,
+	BD9571MWV_IRQ_128H_OF,	/* BKUP_HOLD on BD9574MWF */
 	BD9571MWV_IRQ_WDT_OF,
 	BD9571MWV_IRQ_BKUP_TRG,
 };
-- 
2.7.4