[PATCH] ASoC: cs42l51: improve error handling in cs42l51_remove()

[PATCH] ASoC: cs42l51: improve error handling in cs42l51_remove()

[Uwe Kleine-König]

When disabling a regulator fails while the device goes away, it doesn't
make sense to reenable the already disabled regulators, so use
regulator_bulk_force_disable() instead of regulator_bulk_disable(). In
the error case returning an error code is also hardly sensible because
the only effect is that the i2c core emits a generic error message.
Replace this by a more specific message and make sure the i2c remove
callback returns 0 instead.

While at it also change cs42l51_remove() to return void as it doesn't
yield any meaningful return value any more.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

a bit unsure if regulator_bulk_force_disable() is indeed the right
function here, its documentation specifies a different usecase.

My motivation for this change was to make it obvious in
cs42l51_i2c_remove() that there is no error to handle to eventually make
i2c remove callbacks return void, too.

Best regards
Uwe

 sound/soc/codecs/cs42l51-i2c.c |  4 +++-
 sound/soc/codecs/cs42l51.c     | 11 ++++++++---
 sound/soc/codecs/cs42l51.h     |  2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/sound/soc/codecs/cs42l51-i2c.c b/sound/soc/codecs/cs42l51-i2c.c
index 70260e0a8f09..3cb21a2ba29f 100644
--- a/sound/soc/codecs/cs42l51-i2c.c
+++ b/sound/soc/codecs/cs42l51-i2c.c
@@ -31,7 +31,9 @@ static int cs42l51_i2c_probe(struct i2c_client *i2c,
 
 static int cs42l51_i2c_remove(struct i2c_client *i2c)
 {
-	return cs42l51_remove(&i2c->dev);
+	cs42l51_remove(&i2c->dev);
+
+	return 0;
 }
 
 static const struct dev_pm_ops cs42l51_pm_ops = {
diff --git a/sound/soc/codecs/cs42l51.c b/sound/soc/codecs/cs42l51.c
index c61b17dc2af8..f3667693e2ea 100644
--- a/sound/soc/codecs/cs42l51.c
+++ b/sound/soc/codecs/cs42l51.c
@@ -793,14 +793,19 @@ int cs42l51_probe(struct device *dev, struct regmap *regmap)
 }
 EXPORT_SYMBOL_GPL(cs42l51_probe);
 
-int cs42l51_remove(struct device *dev)
+void cs42l51_remove(struct device *dev)
 {
 	struct cs42l51_private *cs42l51 = dev_get_drvdata(dev);
+	int ret;
 
 	gpiod_set_value_cansleep(cs42l51->reset_gpio, 1);
 
-	return regulator_bulk_disable(ARRAY_SIZE(cs42l51->supplies),
-				      cs42l51->supplies);
+	ret = regulator_bulk_force_disable(ARRAY_SIZE(cs42l51->supplies),
+					   cs42l51->supplies);
+	if (ret)
+		dev_warn(dev, "Failed to disable all regulators (%pe)\n",
+			 ERR_PTR(ret));
+
 }
 EXPORT_SYMBOL_GPL(cs42l51_remove);
 
diff --git a/sound/soc/codecs/cs42l51.h b/sound/soc/codecs/cs42l51.h
index 9d06cf7f8876..a79343e8a54e 100644
--- a/sound/soc/codecs/cs42l51.h
+++ b/sound/soc/codecs/cs42l51.h
@@ -13,7 +13,7 @@ struct device;
 
 extern const struct regmap_config cs42l51_regmap;
 int cs42l51_probe(struct device *dev, struct regmap *regmap);
-int cs42l51_remove(struct device *dev);
+void cs42l51_remove(struct device *dev);
 int __maybe_unused cs42l51_suspend(struct device *dev);
 int __maybe_unused cs42l51_resume(struct device *dev);
 extern const struct of_device_id cs42l51_of_match[];
-- 
2.30.2

Re: [PATCH] ASoC: cs42l51: improve error handling in cs42l51_remove()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

On Thu, Oct 21, 2021 at 12:36:27PM +0200, Uwe Kleine-König wrote:

> a bit unsure if regulator_bulk_force_disable() is indeed the right
> function here, its documentation specifies a different usecase.

> My motivation for this change was to make it obvious in
> cs42l51_i2c_remove() that there is no error to handle to eventually make
> i2c remove callbacks return void, too.

It would be better to just ignore the errors on disable.  Realistically
you'd have to really be trying to trigger an error here and it's most
likely that the system is in enough trouble if one is triggered that
it's just not worrying about.  I'm not sure how likely it is that anyone
would ever remove one of these devices in production though.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cs42l51: improve error handling in cs42l51_remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

On Thu, Oct 21, 2021 at 07:38:08PM +0100, Mark Brown wrote:
> On Thu, Oct 21, 2021 at 12:36:27PM +0200, Uwe Kleine-König wrote:
> 
> > a bit unsure if regulator_bulk_force_disable() is indeed the right
> > function here, its documentation specifies a different usecase.
> 
> > My motivation for this change was to make it obvious in
> > cs42l51_i2c_remove() that there is no error to handle to eventually make
> > i2c remove callbacks return void, too.
> 
> It would be better to just ignore the errors on disable. 

Yeah, and part of this is if regulator #1 fails to disable still disable
regulators #2 and #3. (I don't know how many there are.) That was why I
picked regulator_bulk_force_disable() which has this behaviour.

> Realistically you'd have to really be trying to trigger an error here
> and it's most likely that the system is in enough trouble if one is
> triggered that it's just not worrying about.  I'm not sure how likely
> it is that anyone would ever remove one of these devices in production
> though.

So compared to my patch you'd just drop the warning?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cs42l51: improve error handling in cs42l51_remove()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

On Thu, Oct 21, 2021 at 10:58:39PM +0200, Uwe Kleine-König wrote:
> On Thu, Oct 21, 2021 at 07:38:08PM +0100, Mark Brown wrote:

> > Realistically you'd have to really be trying to trigger an error here
> > and it's most likely that the system is in enough trouble if one is
> > triggered that it's just not worrying about.  I'm not sure how likely
> > it is that anyone would ever remove one of these devices in production
> > though.

> So compared to my patch you'd just drop the warning?!

The warning is fine so long as there's no action on it but use regular
regulator_bulk_disable() since as you youself said force disable is not
appropriate here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cs42l51: improve error handling in cs42l51_remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1368 bytes --]

On Thu, Oct 21, 2021 at 10:31:56PM +0100, Mark Brown wrote:
> On Thu, Oct 21, 2021 at 10:58:39PM +0200, Uwe Kleine-König wrote:
> > On Thu, Oct 21, 2021 at 07:38:08PM +0100, Mark Brown wrote:
> 
> > > Realistically you'd have to really be trying to trigger an error here
> > > and it's most likely that the system is in enough trouble if one is
> > > triggered that it's just not worrying about.  I'm not sure how likely
> > > it is that anyone would ever remove one of these devices in production
> > > though.
> 
> > So compared to my patch you'd just drop the warning?!
> 
> The warning is fine so long as there's no action on it but use regular
> regulator_bulk_disable() since as you youself said force disable is not
> appropriate here.

It's just the documentation of regulator_bulk_force_disable() that
irritates me. It's behaviour is exactly fine. If a user of several
regulators goes away, it should try to disable all regulators and if one
fails to disable it's better to the others instead of keeping all
enabled.

But I didn't feel strong enough to continue to argue, my focus is a
different one. Will send a v2 with keeping regulator_bulk_disable().

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] ASoC: cs42l51: improve error handling in cs42l51_remove()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 945 bytes --]

On Fri, Oct 22, 2021 at 09:12:17AM +0200, Uwe Kleine-König wrote:
> On Thu, Oct 21, 2021 at 10:31:56PM +0100, Mark Brown wrote:

> > The warning is fine so long as there's no action on it but use regular
> > regulator_bulk_disable() since as you youself said force disable is not
> > appropriate here.

> It's just the documentation of regulator_bulk_force_disable() that
> irritates me. It's behaviour is exactly fine. If a user of several
> regulators goes away, it should try to disable all regulators and if one
> fails to disable it's better to the others instead of keeping all
> enabled.

Well, you definitely don't want force disable since it ignores
refcounting and might impact something else sharing the regulator.
You'd want to add a separate function that just ignored errors, though
bear in mind that a lot of datasheets recommend against having the
device partially powered, warning that it may damage the part.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]