* [PATCH 1/3] regulator: bd718x7: Drop unnecessary info print
2022-11-23 11:59 [PATCH 0/3] regulator: ROHM BDxxxxx minor print improvements Matti Vaittinen
@ 2022-11-23 12:00 ` Matti Vaittinen
2022-11-23 12:00 ` [PATCH 2/3] regulator: bd71815: bd71828: bd9576: Use dev_err_probe() Matti Vaittinen
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Matti Vaittinen @ 2022-11-23 12:00 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Matti Vaittinen, Liam Girdwood, Mark Brown, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1376 bytes --]
A device-tree node name was printed when bd718x7 driver's feedback-loop
feature was developed. This print was accidentally left in driver. The
info-level print just pollutes the log.
Drop unnecessary pr_info()
Fixes: d2ad981151b3 ("regulator: bd718x7: Support external connection to scale voltages")
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
drivers/regulator/bd718x7-regulator.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index 00efb18a836c..d161b0026f33 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -1576,8 +1576,6 @@ static int setup_feedback_loop(struct device *dev, struct device_node *np,
if (!of_node_name_eq(np, desc->of_match))
continue;
- pr_info("Looking at node '%s'\n", desc->of_match);
-
/* The feedback loop connection does not make sense for LDOs */
if (desc->id >= BD718XX_LDO1)
return -EINVAL;
--
2.38.1
--
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 to Simon Glass for the translation =]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/3] regulator: bd71815: bd71828: bd9576: Use dev_err_probe()
2022-11-23 11:59 [PATCH 0/3] regulator: ROHM BDxxxxx minor print improvements Matti Vaittinen
2022-11-23 12:00 ` [PATCH 1/3] regulator: bd718x7: Drop unnecessary info print Matti Vaittinen
@ 2022-11-23 12:00 ` Matti Vaittinen
2022-11-23 12:00 ` [PATCH 3/3] regulator: bd718x7: " Matti Vaittinen
2022-11-23 17:23 ` [PATCH 0/3] regulator: ROHM BDxxxxx minor print improvements Mark Brown
3 siblings, 0 replies; 5+ messages in thread
From: Matti Vaittinen @ 2022-11-23 12:00 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Matti Vaittinen, Liam Girdwood, Mark Brown, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5837 bytes --]
The dev_err_probe() has (at least) following benefits over dev_err()
when printing an error print for a failed function call at a device
driver probe:
- Omit error level print if error is 'EPRBE_DEFER'
- Standardized print format for returned error
- return the error value allowing shortening calls like:
if (ret) {
dev_err(...);
return ret;
}
to
if (ret)
return dev_err_probe(...);
Convert the ROHM BD71828, ROHM BD71815 and ROHM BD9576 regulator drivers
to use the dev_err_probe() when returned error is not hard-coded constant.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
drivers/regulator/bd71815-regulator.c | 10 ++---
drivers/regulator/bd71828-regulator.c | 21 +++++------
drivers/regulator/bd9576-regulator.c | 54 +++++++++++++--------------
3 files changed, 38 insertions(+), 47 deletions(-)
diff --git a/drivers/regulator/bd71815-regulator.c b/drivers/regulator/bd71815-regulator.c
index c2b8b8be7824..8b55046eded8 100644
--- a/drivers/regulator/bd71815-regulator.c
+++ b/drivers/regulator/bd71815-regulator.c
@@ -602,12 +602,10 @@ static int bd7181x_probe(struct platform_device *pdev)
config.ena_gpiod = NULL;
rdev = devm_regulator_register(&pdev->dev, desc, &config);
- if (IS_ERR(rdev)) {
- dev_err(&pdev->dev,
- "failed to register %s regulator\n",
- desc->name);
- return PTR_ERR(rdev);
- }
+ if (IS_ERR(rdev))
+ return dev_err_probe(&pdev->dev, PTR_ERR(rdev),
+ "failed to register %s regulator\n",
+ desc->name);
}
return 0;
}
diff --git a/drivers/regulator/bd71828-regulator.c b/drivers/regulator/bd71828-regulator.c
index a4f09a5a30ca..ad728f4f2241 100644
--- a/drivers/regulator/bd71828-regulator.c
+++ b/drivers/regulator/bd71828-regulator.c
@@ -750,23 +750,20 @@ static int bd71828_probe(struct platform_device *pdev)
rd = &bd71828_rdata[i];
rdev = devm_regulator_register(&pdev->dev,
&rd->desc, &config);
- if (IS_ERR(rdev)) {
- dev_err(&pdev->dev,
- "failed to register %s regulator\n",
- rd->desc.name);
- return PTR_ERR(rdev);
- }
+ if (IS_ERR(rdev))
+ return dev_err_probe(&pdev->dev, PTR_ERR(rdev),
+ "failed to register %s regulator\n",
+ rd->desc.name);
+
for (j = 0; j < rd->reg_init_amnt; j++) {
ret = regmap_update_bits(config.regmap,
rd->reg_inits[j].reg,
rd->reg_inits[j].mask,
rd->reg_inits[j].val);
- if (ret) {
- dev_err(&pdev->dev,
- "regulator %s init failed\n",
- rd->desc.name);
- return ret;
- }
+ if (ret)
+ return dev_err_probe(&pdev->dev, ret,
+ "regulator %s init failed\n",
+ rd->desc.name);
}
}
return 0;
diff --git a/drivers/regulator/bd9576-regulator.c b/drivers/regulator/bd9576-regulator.c
index 393c8693b327..02c70768652b 100644
--- a/drivers/regulator/bd9576-regulator.c
+++ b/drivers/regulator/bd9576-regulator.c
@@ -953,30 +953,28 @@ static int bd957x_probe(struct platform_device *pdev)
dev_fwnode(pdev->dev.parent),
"rohm,vout1-en", GPIOD_OUT_LOW,
"vout1-en");
- if (!IS_ERR(en)) {
- /* VOUT1_OPS gpio ctrl */
- /*
- * Regulator core prioritizes the ena_gpio over
- * enable/disable/is_enabled callbacks so no need to
- * clear them. We can still use same ops
- */
+
+ /* VOUT1_OPS gpio ctrl */
+ /*
+ * Regulator core prioritizes the ena_gpio over
+ * enable/disable/is_enabled callbacks so no need to clear them
+ * even if GPIO is used. So, we can still use same ops.
+ *
+ * In theory it is possible someone wants to set vout1-en LOW
+ * during OTP loading and set VOUT1 to be controlled by GPIO -
+ * but control the GPIO from some where else than this driver.
+ * For that to work we should unset the is_enabled callback
+ * here.
+ *
+ * I believe such case where rohm,vout1-en-low is set and
+ * vout1-en-gpios is not is likely to be a misconfiguration.
+ * So let's just err out for now.
+ */
+ if (!IS_ERR(en))
config.ena_gpiod = en;
- } else {
- /*
- * In theory it is possible someone wants to set
- * vout1-en LOW during OTP loading and set VOUT1 to be
- * controlled by GPIO - but control the GPIO from some
- * where else than this driver. For that to work we
- * should unset the is_enabled callback here.
- *
- * I believe such case where rohm,vout1-en-low is set
- * and vout1-en-gpios is not is likely to be a
- * misconfiguration. So let's just err out for now.
- */
- dev_err(&pdev->dev,
- "Failed to get VOUT1 control GPIO\n");
- return PTR_ERR(en);
- }
+ else
+ return dev_err_probe(&pdev->dev, PTR_ERR(en),
+ "Failed to get VOUT1 control GPIO\n");
}
/*
@@ -1037,12 +1035,10 @@ static int bd957x_probe(struct platform_device *pdev)
r->rdev = devm_regulator_register(&pdev->dev, desc,
&config);
- if (IS_ERR(r->rdev)) {
- dev_err(&pdev->dev,
- "failed to register %s regulator\n",
- desc->name);
- return PTR_ERR(r->rdev);
- }
+ if (IS_ERR(r->rdev))
+ return dev_err_probe(&pdev->dev, PTR_ERR(r->rdev),
+ "failed to register %s regulator\n",
+ desc->name);
/*
* Clear the VOUT1 GPIO setting - rest of the regulators do not
* support GPIO control
--
2.38.1
--
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 to Simon Glass for the translation =]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] regulator: bd718x7: Use dev_err_probe()
2022-11-23 11:59 [PATCH 0/3] regulator: ROHM BDxxxxx minor print improvements Matti Vaittinen
2022-11-23 12:00 ` [PATCH 1/3] regulator: bd718x7: Drop unnecessary info print Matti Vaittinen
2022-11-23 12:00 ` [PATCH 2/3] regulator: bd71815: bd71828: bd9576: Use dev_err_probe() Matti Vaittinen
@ 2022-11-23 12:00 ` Matti Vaittinen
2022-11-23 17:23 ` [PATCH 0/3] regulator: ROHM BDxxxxx minor print improvements Mark Brown
3 siblings, 0 replies; 5+ messages in thread
From: Matti Vaittinen @ 2022-11-23 12:00 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen
Cc: Matti Vaittinen, Liam Girdwood, Mark Brown, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5196 bytes --]
The dev_err_probe() has (at least) following benefits over dev_err()
when printing an error print for a failed function call at a device
driver probe:
- Omit error level print if error is 'EPRBE_DEFER'
- Standardized print format for returned error
- return the error value allowing shortening calls like:
if (ret) {
dev_err(...);
return ret;
}
to
if (ret)
return dev_err_probe(...);
Convert the ROHM BD718x7 regulator driver to use the dev_err_probe() when
returned error is not hard-coded constant.
NOTE:
This commit also changes the error handling path to return immediately
from a spot where the error is spotted instead of using a single point
of exit.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
For many years I have preferred a single point of exit in a function
when managing it does not require any big tricks. In my experience a
single point of exit makes resource-leaking and lock releasing much less
error prone. Eg, consider mistakes like:
...
mutex_lock(&mtx)
...
if (err)
return err;
...
mutex_unlock(&mtx);
return 0;
Vs.
...
mutex_lock(&mtx)
...
if (err)
goto err_out;
...
err_out:
mutex_unlock(&mtx);
return err;
I still think a single point of exit is often a good idea. However, I am
slowly adapting to thought that the single point of exit does not really
play a big role in bd718x7 regulator probe and using dev_err_probe()
allows us to avoid the extra {} after condition... So, maybe it indeed
is a time for me to ditch the goto here. Please, let me know if you
think othervice :)
---
drivers/regulator/bd718x7-regulator.c | 51 ++++++++++-----------------
1 file changed, 19 insertions(+), 32 deletions(-)
diff --git a/drivers/regulator/bd718x7-regulator.c b/drivers/regulator/bd718x7-regulator.c
index d161b0026f33..894fab0d53d0 100644
--- a/drivers/regulator/bd718x7-regulator.c
+++ b/drivers/regulator/bd718x7-regulator.c
@@ -1706,20 +1706,17 @@ static int bd718xx_probe(struct platform_device *pdev)
break;
default:
dev_err(&pdev->dev, "Unsupported chip type\n");
- err = -EINVAL;
- goto err;
+ return -EINVAL;
}
/* Register LOCK release */
err = regmap_update_bits(regmap, BD718XX_REG_REGLOCK,
(REGLOCK_PWRSEQ | REGLOCK_VREG), 0);
- if (err) {
- dev_err(&pdev->dev, "Failed to unlock PMIC (%d)\n", err);
- goto err;
- } else {
- dev_dbg(&pdev->dev, "Unlocked lock register 0x%x\n",
- BD718XX_REG_REGLOCK);
- }
+ if (err)
+ return dev_err_probe(&pdev->dev, err, "Failed to unlock PMIC\n");
+
+ dev_dbg(&pdev->dev, "Unlocked lock register 0x%x\n",
+ BD718XX_REG_REGLOCK);
use_snvs = of_property_read_bool(pdev->dev.parent->of_node,
"rohm,reset-snvs-powered");
@@ -1736,13 +1733,11 @@ static int bd718xx_probe(struct platform_device *pdev)
BD718XX_WDOG_POWEROFF_MASK |
BD718XX_KEY_L_POWEROFF_MASK,
BD718XX_POWOFF_TO_RDY);
- if (err) {
- dev_err(&pdev->dev, "Failed to change reset target\n");
- goto err;
- } else {
- dev_dbg(&pdev->dev,
- "Changed all resets from SVNS to READY\n");
- }
+ if (err)
+ return dev_err_probe(&pdev->dev, err,
+ "Failed to change reset target\n");
+
+ dev_dbg(&pdev->dev, "Changed all resets from SVNS to READY\n");
}
config.dev = pdev->dev.parent;
@@ -1778,13 +1773,10 @@ static int bd718xx_probe(struct platform_device *pdev)
desc->ops = swops[i];
rdev = devm_regulator_register(&pdev->dev, desc, &config);
- if (IS_ERR(rdev)) {
- dev_err(&pdev->dev,
- "failed to register %s regulator\n",
- desc->name);
- err = PTR_ERR(rdev);
- goto err;
- }
+ if (IS_ERR(rdev))
+ return dev_err_probe(&pdev->dev, PTR_ERR(rdev),
+ "failed to register %s regulator\n",
+ desc->name);
/*
* Regulator register gets the regulator constraints and
@@ -1807,28 +1799,23 @@ static int bd718xx_probe(struct platform_device *pdev)
!rdev->constraints->boot_on)) {
err = regmap_update_bits(regmap, r->init.reg,
r->init.mask, r->init.val);
- if (err) {
- dev_err(&pdev->dev,
+ if (err)
+ return dev_err_probe(&pdev->dev, err,
"Failed to take control for (%s)\n",
desc->name);
- goto err;
- }
}
for (j = 0; j < r->additional_init_amnt; j++) {
err = regmap_update_bits(regmap,
r->additional_inits[j].reg,
r->additional_inits[j].mask,
r->additional_inits[j].val);
- if (err) {
- dev_err(&pdev->dev,
+ if (err)
+ return dev_err_probe(&pdev->dev, err,
"Buck (%s) initialization failed\n",
desc->name);
- goto err;
- }
}
}
-err:
return err;
}
--
2.38.1
--
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 to Simon Glass for the translation =]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 0/3] regulator: ROHM BDxxxxx minor print improvements
2022-11-23 11:59 [PATCH 0/3] regulator: ROHM BDxxxxx minor print improvements Matti Vaittinen
` (2 preceding siblings ...)
2022-11-23 12:00 ` [PATCH 3/3] regulator: bd718x7: " Matti Vaittinen
@ 2022-11-23 17:23 ` Mark Brown
3 siblings, 0 replies; 5+ messages in thread
From: Mark Brown @ 2022-11-23 17:23 UTC (permalink / raw)
To: Matti Vaittinen, Matti Vaittinen; +Cc: Liam Girdwood, linux-kernel
On Wed, 23 Nov 2022 13:59:48 +0200, Matti Vaittinen wrote:
> Minor (printing) improvements for ROHM regulator drivers.
>
> This series:
>
> - Drops an unnecessary info print from bd718x7.
> (Added a fixes tag for this but not really sure if worth
> adding to stable)
> - Convert the ROHM BDxxxxx PMIC regulator drivers to use dev_err_probe().
> - Change the probe logic for bd718x7 to favor the more usual devm-style
> where errors are returned immediately.
>
> [...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
Thanks!
[1/3] regulator: bd718x7: Drop unnecessary info print
commit: 44501eba9bb28946382b7a53099ce8098d1610f0
[2/3] regulator: bd71815: bd71828: bd9576: Use dev_err_probe()
commit: d4e93e8da012880882671a46ac6ae3aefcae8076
[3/3] regulator: bd718x7: Use dev_err_probe()
commit: 662a9bf883f42d09145cf083d93dadf0307f15ea
All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying
to this mail.
Thanks,
Mark
^ permalink raw reply [flat|nested] 5+ messages in thread