| Message ID | 1607686060-17448-4-git-send-email-yoshihiro.shimoda.uh@renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | treewide: bd9571mwv: Add support for BD9574MWF | expand |
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
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
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
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 >
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
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 >
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, ®ulators[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; }
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(-)