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

From: Vaittinen, Matti
Date: Fri Dec 11 2020 - 07:02:11 EST


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