| Message ID | 20211021103627.70975-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: cs42l51: improve error handling in cs42l51_remove() | expand |
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.
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
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.
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
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.
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[];
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(-)