Message ID | 20220207081709.27288-1-u.kleine-koenig@pengutronix.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | mfd: stmfx: Improve error message triggered by regulator fault in .remove() | expand |
On Mon, 07 Feb 2022, Uwe Kleine-König wrote: > Returning a non-zero value in an i2c remove callback results in the i2c > core emitting a very generic error message ("remove failed (-ESOMETHING), > will be ignored") and as the message indicates not further error handling > is done. > > Instead emit a more specific error message and then return zero in > .remove(). > > The long-term goal is to make the i2c remove prototype return void, making > all implementations return 0 is preparatory work for this change. > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > drivers/mfd/stmfx.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/drivers/mfd/stmfx.c b/drivers/mfd/stmfx.c > index e095a3930142..16631c675f2f 100644 > --- a/drivers/mfd/stmfx.c > +++ b/drivers/mfd/stmfx.c > @@ -392,17 +392,21 @@ static int stmfx_chip_init(struct i2c_client *client) > return ret; > } > > -static int stmfx_chip_exit(struct i2c_client *client) > +static void stmfx_chip_exit(struct i2c_client *client) > { > struct stmfx *stmfx = i2c_get_clientdata(client); > > regmap_write(stmfx->map, STMFX_REG_IRQ_SRC_EN, 0); > regmap_write(stmfx->map, STMFX_REG_SYS_CTRL, 0); > > - if (stmfx->vdd) > - return regulator_disable(stmfx->vdd); > + if (stmfx->vdd) { > + int ret = regulator_disable(stmfx->vdd); > > - return 0; > + if (ret) Nit: Premise of the patch is fine, but please can you use the standard function call, check the return value format please. Something about this is triggering my OCD! :) int ret; ret = regulator_disable(stmfx->vdd); if (ret) do_thing(); > + dev_err(&client->dev, > + "Failed to disable vdd regulator: %pe\n", > + ERR_PTR(ret)); > + } > } > > static int stmfx_probe(struct i2c_client *client, > @@ -466,7 +470,9 @@ static int stmfx_remove(struct i2c_client *client) > { > stmfx_irq_exit(client); > > - return stmfx_chip_exit(client); > + stmfx_chip_exit(client); > + > + return 0; > } > > #ifdef CONFIG_PM_SLEEP > > base-commit: dcb85f85fa6f142aae1fe86f399d4503d49f2b60
On Mon, Feb 14, 2022 at 01:46:37PM +0000, Lee Jones wrote: > On Mon, 07 Feb 2022, Uwe Kleine-König wrote: > > > Returning a non-zero value in an i2c remove callback results in the i2c > > core emitting a very generic error message ("remove failed (-ESOMETHING), > > will be ignored") and as the message indicates not further error handling > > is done. > > > > Instead emit a more specific error message and then return zero in > > .remove(). > > > > The long-term goal is to make the i2c remove prototype return void, making > > all implementations return 0 is preparatory work for this change. > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > drivers/mfd/stmfx.c | 16 +++++++++++----- > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/mfd/stmfx.c b/drivers/mfd/stmfx.c > > index e095a3930142..16631c675f2f 100644 > > --- a/drivers/mfd/stmfx.c > > +++ b/drivers/mfd/stmfx.c > > @@ -392,17 +392,21 @@ static int stmfx_chip_init(struct i2c_client *client) > > return ret; > > } > > > > -static int stmfx_chip_exit(struct i2c_client *client) > > +static void stmfx_chip_exit(struct i2c_client *client) > > { > > struct stmfx *stmfx = i2c_get_clientdata(client); > > > > regmap_write(stmfx->map, STMFX_REG_IRQ_SRC_EN, 0); > > regmap_write(stmfx->map, STMFX_REG_SYS_CTRL, 0); > > > > - if (stmfx->vdd) > > - return regulator_disable(stmfx->vdd); > > + if (stmfx->vdd) { > > + int ret = regulator_disable(stmfx->vdd); > > > > - return 0; > > + if (ret) > > Nit: Premise of the patch is fine, but please can you use the standard > function call, check the return value format please. Something about > this is triggering my OCD! :) > > int ret; > > ret = regulator_disable(stmfx->vdd); > if (ret) > do_thing(); Not sure I understand you correctly. Do you want just: regmap_write(stmfx->map, STMFX_REG_SYS_CTRL, 0); if (stmfx->vdd) { - int ret = regulator_disable(stmfx->vdd); + int ret; + + ret = regulator_disable(stmfx->vdd); if (ret) ... squashed into the patch? Best regards Uwe
On Mon, 14 Feb 2022, Uwe Kleine-König wrote: > On Mon, Feb 14, 2022 at 01:46:37PM +0000, Lee Jones wrote: > > On Mon, 07 Feb 2022, Uwe Kleine-König wrote: > > > > > Returning a non-zero value in an i2c remove callback results in the i2c > > > core emitting a very generic error message ("remove failed (-ESOMETHING), > > > will be ignored") and as the message indicates not further error handling > > > is done. > > > > > > Instead emit a more specific error message and then return zero in > > > .remove(). > > > > > > The long-term goal is to make the i2c remove prototype return void, making > > > all implementations return 0 is preparatory work for this change. > > > > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > drivers/mfd/stmfx.c | 16 +++++++++++----- > > > 1 file changed, 11 insertions(+), 5 deletions(-) > > > > > > diff --git a/drivers/mfd/stmfx.c b/drivers/mfd/stmfx.c > > > index e095a3930142..16631c675f2f 100644 > > > --- a/drivers/mfd/stmfx.c > > > +++ b/drivers/mfd/stmfx.c > > > @@ -392,17 +392,21 @@ static int stmfx_chip_init(struct i2c_client *client) > > > return ret; > > > } > > > > > > -static int stmfx_chip_exit(struct i2c_client *client) > > > +static void stmfx_chip_exit(struct i2c_client *client) > > > { > > > struct stmfx *stmfx = i2c_get_clientdata(client); > > > > > > regmap_write(stmfx->map, STMFX_REG_IRQ_SRC_EN, 0); > > > regmap_write(stmfx->map, STMFX_REG_SYS_CTRL, 0); > > > > > > - if (stmfx->vdd) > > > - return regulator_disable(stmfx->vdd); > > > + if (stmfx->vdd) { > > > + int ret = regulator_disable(stmfx->vdd); > > > > > > - return 0; > > > + if (ret) > > > > Nit: Premise of the patch is fine, but please can you use the standard > > function call, check the return value format please. Something about > > this is triggering my OCD! :) > > > > int ret; > > > > ret = regulator_disable(stmfx->vdd); > > if (ret) > > do_thing(); > > Not sure I understand you correctly. Do you want just: > > regmap_write(stmfx->map, STMFX_REG_SYS_CTRL, 0); > > if (stmfx->vdd) { > - int ret = regulator_disable(stmfx->vdd); > + int ret; > + > + ret = regulator_disable(stmfx->vdd); > if (ret) > ... > > squashed into the patch? Effectively, yes please. The diff would look like: > > > - if (stmfx->vdd) > > > - return regulator_disable(stmfx->vdd); > > > + if (stmfx->vdd) { > > > + int ret; > > > + > > > + ret = regulator_disable(stmfx->vdd); > > > - > > > - return 0; > > > + if (ret) Thanks.
diff --git a/drivers/mfd/stmfx.c b/drivers/mfd/stmfx.c index e095a3930142..16631c675f2f 100644 --- a/drivers/mfd/stmfx.c +++ b/drivers/mfd/stmfx.c @@ -392,17 +392,21 @@ static int stmfx_chip_init(struct i2c_client *client) return ret; } -static int stmfx_chip_exit(struct i2c_client *client) +static void stmfx_chip_exit(struct i2c_client *client) { struct stmfx *stmfx = i2c_get_clientdata(client); regmap_write(stmfx->map, STMFX_REG_IRQ_SRC_EN, 0); regmap_write(stmfx->map, STMFX_REG_SYS_CTRL, 0); - if (stmfx->vdd) - return regulator_disable(stmfx->vdd); + if (stmfx->vdd) { + int ret = regulator_disable(stmfx->vdd); - return 0; + if (ret) + dev_err(&client->dev, + "Failed to disable vdd regulator: %pe\n", + ERR_PTR(ret)); + } } static int stmfx_probe(struct i2c_client *client, @@ -466,7 +470,9 @@ static int stmfx_remove(struct i2c_client *client) { stmfx_irq_exit(client); - return stmfx_chip_exit(client); + stmfx_chip_exit(client); + + return 0; } #ifdef CONFIG_PM_SLEEP
Returning a non-zero value in an i2c remove callback results in the i2c core emitting a very generic error message ("remove failed (-ESOMETHING), will be ignored") and as the message indicates not further error handling is done. Instead emit a more specific error message and then return zero in .remove(). The long-term goal is to make the i2c remove prototype return void, making all implementations return 0 is preparatory work for this change. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- drivers/mfd/stmfx.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) base-commit: dcb85f85fa6f142aae1fe86f399d4503d49f2b60