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

["Vaittinen, Matti" <Matti.Vaittinen@xxxxxxxxxxxxxxxxx>]

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@xxxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxx>

>  
>  	/* 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;
>  	}
>