[PATCH 0/8] iio: Improve error reporting for problems during .remove for various i2c drivers

[PATCH 0/8] iio: Improve error reporting for problems during .remove for various i2c drivers

[Uwe Kleine-König]

Hello,

the goal of this series is to improve the error messages emitted by
remove callbacks by several i2c drivers. The status quo is that these
might return an error code silently to the i2c core on remove. The
effect is a very unhelpful message:

	$bus $device: remove failed (EIO), will be ignored

. Note there is no further error handling and the device will be
removed, all devm cleanups are called.

With the patches from this series applied the emitted message better
tells what actually failed and the remove callback returns zero
unconditionally.

The motivation for this change is to make i2c remove callbacks return
void. The ability to return a value is misleading as driver authors
might expect some kind of error handling from the i2c core.

Best regards
Uwe

Uwe Kleine-König (8):
  iio:adc:ina2xx: Improve error reporting for problems during .remove()
  iio:adc:ti-ads1015: Improve error reporting for problems during
    .remove()
  iio:chemical:atlas: Improve error reporting for problems during
    .remove()
  iio:chemical:ccs811: Improve error reporting for problems during
    .remove()
  iio:light:pa12203001: Improve error reporting for problems during
    .remove()
  iio:light:us5182d: Improve error reporting for problems during
    .remove()
  iio:light:vcnl4000: Improve error reporting for problems during
    .remove()
  iio:light:vcnl4035: Improve error reporting for problems during
    .remove()

 drivers/iio/adc/ina2xx-adc.c        | 10 ++++++++--
 drivers/iio/adc/ti-ads1015.c        |  8 +++++++-
 drivers/iio/chemical/atlas-sensor.c |  8 +++++++-
 drivers/iio/chemical/ccs811.c       | 10 ++++++++--
 drivers/iio/light/pa12203001.c      |  8 +++++++-
 drivers/iio/light/us5182d.c         |  8 +++++++-
 drivers/iio/light/vcnl4000.c        |  8 +++++++-
 drivers/iio/light/vcnl4035.c        | 10 ++++++++--
 8 files changed, 59 insertions(+), 11 deletions(-)


base-commit: 3123109284176b1532874591f7c81f3837bbdc17
-- 
2.35.1

[PATCH 1/8] iio:adc:ina2xx: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in a generic
error message being emitted by the i2c core, but otherwise it doesn't
make a difference. The device goes away anyhow and the devm cleanups are
called.

So instead of triggering the generic i2c error message, emit a more
helpful message if a problem occurs and return 0 to suppress the generic
message.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/adc/ina2xx-adc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/adc/ina2xx-adc.c b/drivers/iio/adc/ina2xx-adc.c
index 8d902a32a0fd..2eafe98cff3e 100644
--- a/drivers/iio/adc/ina2xx-adc.c
+++ b/drivers/iio/adc/ina2xx-adc.c
@@ -1039,12 +1039,18 @@ static int ina2xx_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct ina2xx_chip_info *chip = iio_priv(indio_dev);
+	int ret;
 
 	iio_device_unregister(indio_dev);
 
 	/* Powerdown */
-	return regmap_update_bits(chip->regmap, INA2XX_CONFIG,
-				  INA2XX_MODE_MASK, 0);
+	ret = regmap_update_bits(chip->regmap, INA2XX_CONFIG,
+				 INA2XX_MODE_MASK, 0);
+	if (ret)
+		dev_warn(&client->dev, "Failed to power down device (%pe)\n",
+			 ERR_PTR(ret));
+
+	return 0;
 }
 
 static const struct i2c_device_id ina2xx_id[] = {
-- 
2.35.1

[PATCH 2/8] iio:adc:ti-ads1015: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in a generic
error message being emitted by the i2c core, but otherwise it doesn't
make a difference. The device goes away anyhow and the devm cleanups are
called.

So instead of triggering the generic i2c error message, emit a more
helpful message if a problem occurs and return 0 to suppress the generic
message.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/adc/ti-ads1015.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-ads1015.c b/drivers/iio/adc/ti-ads1015.c
index 068efbce1710..f8021d99db64 100644
--- a/drivers/iio/adc/ti-ads1015.c
+++ b/drivers/iio/adc/ti-ads1015.c
@@ -1061,6 +1061,7 @@ static int ads1015_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct ads1015_data *data = iio_priv(indio_dev);
+	int ret;
 
 	iio_device_unregister(indio_dev);
 
@@ -1068,7 +1069,12 @@ static int ads1015_remove(struct i2c_client *client)
 	pm_runtime_set_suspended(&client->dev);
 
 	/* power down single shot mode */
-	return ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
+	ret = ads1015_set_conv_mode(data, ADS1015_SINGLESHOT);
+	if (ret)
+		dev_warn(&client->dev, "Failed to power down (%pe)\n",
+			 ERR_PTR(ret));
+
+	return 0;
 }
 
 #ifdef CONFIG_PM
-- 
2.35.1

[PATCH 3/8] iio:chemical:atlas: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in a generic
error message being emitted by the i2c core, but otherwise it doesn't
make a difference. The device goes away anyhow and the devm cleanups are
called.

So instead of triggering the generic i2c error message, emit a more
helpful message if a problem occurs and return 0 to suppress the generic
message.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/chemical/atlas-sensor.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/chemical/atlas-sensor.c b/drivers/iio/chemical/atlas-sensor.c
index 56dea9734c8d..8378c00fa2ff 100644
--- a/drivers/iio/chemical/atlas-sensor.c
+++ b/drivers/iio/chemical/atlas-sensor.c
@@ -726,6 +726,7 @@ static int atlas_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct atlas_data *data = iio_priv(indio_dev);
+	int ret;
 
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
@@ -734,7 +735,12 @@ static int atlas_remove(struct i2c_client *client)
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 
-	return atlas_set_powermode(data, 0);
+	ret = atlas_set_powermode(data, 0);
+	if (ret)
+		dev_err(&client->dev, "Failed to power down device (%pe)\n",
+			ERR_PTR(ret));
+
+	return 0;
 }
 
 static int atlas_runtime_suspend(struct device *dev)
-- 
2.35.1

[PATCH 4/8] iio:chemical:ccs811: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in a generic
error message being emitted by the i2c core, but otherwise it doesn't
make a difference. The device goes away anyhow and the devm cleanups are
called.

So instead of triggering the generic i2c error message, emit a more
helpful message if a problem occurs and return 0 to suppress the generic
message.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/chemical/ccs811.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/chemical/ccs811.c b/drivers/iio/chemical/ccs811.c
index 847194fa1e46..c38093e24fca 100644
--- a/drivers/iio/chemical/ccs811.c
+++ b/drivers/iio/chemical/ccs811.c
@@ -536,14 +536,20 @@ static int ccs811_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct ccs811_data *data = iio_priv(indio_dev);
+	int ret;
 
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
 	if (data->drdy_trig)
 		iio_trigger_unregister(data->drdy_trig);
 
-	return i2c_smbus_write_byte_data(client, CCS811_MEAS_MODE,
-					 CCS811_MODE_IDLE);
+	ret = i2c_smbus_write_byte_data(client, CCS811_MEAS_MODE,
+					CCS811_MODE_IDLE);
+	if (ret)
+		dev_warn(&client->dev, "Failed to power down device (%pe)\n",
+			 ERR_PTR(ret));
+
+	return 0;
 }
 
 static const struct i2c_device_id ccs811_id[] = {
-- 
2.35.1

[PATCH 5/8] iio:light:pa12203001: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in a generic
error message being emitted by the i2c core, but otherwise it doesn't
make a difference. The device goes away anyhow and the devm cleanups are
called.

So instead of triggering the generic i2c error message, emit a more
helpful message if a problem occurs and return 0 to suppress the generic
message.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/light/pa12203001.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/pa12203001.c b/drivers/iio/light/pa12203001.c
index 528fa5dd2b13..772874e707ae 100644
--- a/drivers/iio/light/pa12203001.c
+++ b/drivers/iio/light/pa12203001.c
@@ -397,13 +397,19 @@ static int pa12203001_probe(struct i2c_client *client,
 static int pa12203001_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	int ret;
 
 	iio_device_unregister(indio_dev);
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 
-	return pa12203001_power_chip(indio_dev, PA12203001_CHIP_DISABLE);
+	ret = pa12203001_power_chip(indio_dev, PA12203001_CHIP_DISABLE);
+	if (ret)
+		dev_warn(&client->dev, "Failed to power down (%pe)\n",
+			 ERR_PTR(ret));
+
+	return 0;
 }
 
 #if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM)
-- 
2.35.1

[PATCH 6/8] iio:light:us5182d: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in a generic
error message being emitted by the i2c core, but otherwise it doesn't
make a difference. The device goes away anyhow and the devm cleanups are
called.

So instead of triggering the generic i2c error message, emit a more
helpful message if a problem occurs and return 0 to suppress the generic
message.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/light/us5182d.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/us5182d.c b/drivers/iio/light/us5182d.c
index 1492aaf8d84c..cbd9978540fa 100644
--- a/drivers/iio/light/us5182d.c
+++ b/drivers/iio/light/us5182d.c
@@ -907,13 +907,19 @@ static int us5182d_probe(struct i2c_client *client,
 static int us5182d_remove(struct i2c_client *client)
 {
 	struct us5182d_data *data = iio_priv(i2c_get_clientdata(client));
+	int ret;
 
 	iio_device_unregister(i2c_get_clientdata(client));
 
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
 
-	return us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
+	ret = us5182d_shutdown_en(data, US5182D_CFG0_SHUTDOWN_EN);
+	if (ret)
+		dev_warn(&client->dev, "Failed to shut down (%pe)\n",
+			 ERR_PTR(ret));
+
+	return 0;
 }
 
 #if defined(CONFIG_PM_SLEEP) || defined(CONFIG_PM)
-- 
2.35.1

[PATCH 7/8] iio:light:vcnl4000: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in a generic
error message being emitted by the i2c core, but otherwise it doesn't
make a difference. The device goes away anyhow and the devm cleanups are
called.

So instead of triggering the generic i2c error message, emit a more
helpful message if a problem occurs and return 0 to suppress the generic
message.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/light/vcnl4000.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index e02e92bc2928..947a41b86173 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -1115,13 +1115,19 @@ static int vcnl4000_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct vcnl4000_data *data = iio_priv(indio_dev);
+	int ret;
 
 	pm_runtime_dont_use_autosuspend(&client->dev);
 	pm_runtime_disable(&client->dev);
 	iio_device_unregister(indio_dev);
 	pm_runtime_set_suspended(&client->dev);
 
-	return data->chip_spec->set_power_state(data, false);
+	ret = data->chip_spec->set_power_state(data, false);
+	if (ret)
+		dev_warn(&client->dev, "Failed to power down (%pe)\n",
+			 ERR_PTR(ret));
+
+	return 0;
 }
 
 static int __maybe_unused vcnl4000_runtime_suspend(struct device *dev)
-- 
2.35.1

[PATCH 8/8] iio:light:vcnl4035: Improve error reporting for problems during .remove()

[Uwe Kleine-König]

Returning an error value in an i2c remove callback results in a generic
error message being emitted by the i2c core, but otherwise it doesn't
make a difference. The device goes away anyhow and the devm cleanups are
called.

So instead of triggering the generic i2c error message, emit a more
helpful message if a problem occurs and return 0 to suppress the generic
message.

This patch is a preparation for making i2c remove callbacks return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/iio/light/vcnl4035.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/vcnl4035.c b/drivers/iio/light/vcnl4035.c
index da2bf622a67b..2aaec6bef64c 100644
--- a/drivers/iio/light/vcnl4035.c
+++ b/drivers/iio/light/vcnl4035.c
@@ -604,14 +604,20 @@ static int vcnl4035_probe(struct i2c_client *client,
 static int vcnl4035_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	int ret;
 
 	pm_runtime_dont_use_autosuspend(&client->dev);
 	pm_runtime_disable(&client->dev);
 	iio_device_unregister(indio_dev);
 	pm_runtime_set_suspended(&client->dev);
 
-	return vcnl4035_set_als_power_state(iio_priv(indio_dev),
-					VCNL4035_MODE_ALS_DISABLE);
+	ret = vcnl4035_set_als_power_state(iio_priv(indio_dev),
+					   VCNL4035_MODE_ALS_DISABLE);
+	if (ret)
+		dev_warn(&client->dev, "Failed to put device into standby (%pe)\n",
+			 ERR_PTR(ret));
+
+	return 0;
 }
 
 static int __maybe_unused vcnl4035_runtime_suspend(struct device *dev)
-- 
2.35.1

Re: [PATCH 0/8] iio: Improve error reporting for problems during .remove for various i2c drivers

[Jonathan Cameron]

On Sun, 15 May 2022 17:59:21 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> Hello,
> 
> the goal of this series is to improve the error messages emitted by
> remove callbacks by several i2c drivers. The status quo is that these
> might return an error code silently to the i2c core on remove. The
> effect is a very unhelpful message:
> 
> 	$bus $device: remove failed (EIO), will be ignored
> 
> . Note there is no further error handling and the device will be
> removed, all devm cleanups are called.
> 
> With the patches from this series applied the emitted message better
> tells what actually failed and the remove callback returns zero
> unconditionally.
> 
> The motivation for this change is to make i2c remove callbacks return
> void. The ability to return a value is misleading as driver authors
> might expect some kind of error handling from the i2c core.

All look good to me, but I'll leave them on list for a little while
to let others take a look.  Give me a poke if I seem to have lost them
(it's been known to happen :( )

Thanks,

Jonathan

> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (8):
>   iio:adc:ina2xx: Improve error reporting for problems during .remove()
>   iio:adc:ti-ads1015: Improve error reporting for problems during
>     .remove()
>   iio:chemical:atlas: Improve error reporting for problems during
>     .remove()
>   iio:chemical:ccs811: Improve error reporting for problems during
>     .remove()
>   iio:light:pa12203001: Improve error reporting for problems during
>     .remove()
>   iio:light:us5182d: Improve error reporting for problems during
>     .remove()
>   iio:light:vcnl4000: Improve error reporting for problems during
>     .remove()
>   iio:light:vcnl4035: Improve error reporting for problems during
>     .remove()
> 
>  drivers/iio/adc/ina2xx-adc.c        | 10 ++++++++--
>  drivers/iio/adc/ti-ads1015.c        |  8 +++++++-
>  drivers/iio/chemical/atlas-sensor.c |  8 +++++++-
>  drivers/iio/chemical/ccs811.c       | 10 ++++++++--
>  drivers/iio/light/pa12203001.c      |  8 +++++++-
>  drivers/iio/light/us5182d.c         |  8 +++++++-
>  drivers/iio/light/vcnl4000.c        |  8 +++++++-
>  drivers/iio/light/vcnl4035.c        | 10 ++++++++--
>  8 files changed, 59 insertions(+), 11 deletions(-)
> 
> 
> base-commit: 3123109284176b1532874591f7c81f3837bbdc17

Re: [PATCH 0/8] iio: Improve error reporting for problems during .remove for various i2c drivers

[Jonathan Cameron]

On Sun, 15 May 2022 18:05:22 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Sun, 15 May 2022 17:59:21 +0200
> Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:
> 
> > Hello,
> > 
> > the goal of this series is to improve the error messages emitted by
> > remove callbacks by several i2c drivers. The status quo is that these
> > might return an error code silently to the i2c core on remove. The
> > effect is a very unhelpful message:
> > 
> > 	$bus $device: remove failed (EIO), will be ignored
> > 
> > . Note there is no further error handling and the device will be
> > removed, all devm cleanups are called.
> > 
> > With the patches from this series applied the emitted message better
> > tells what actually failed and the remove callback returns zero
> > unconditionally.
> > 
> > The motivation for this change is to make i2c remove callbacks return
> > void. The ability to return a value is misleading as driver authors
> > might expect some kind of error handling from the i2c core.  
> 
> All look good to me, but I'll leave them on list for a little while
> to let others take a look.  Give me a poke if I seem to have lost them
> (it's been known to happen :( )

Applied to the togreg branch of iio.git and pushed out for now as testing.

Very unlikely these will make this cycle unless there is a delay for
some reason.  Hence queued up for 5.20 and I may not push out for next to
pick up until merge window has mostly gone by.

> 
> Thanks,
> 
> Jonathan
> 
> > 
> > Best regards
> > Uwe
> > 
> > Uwe Kleine-König (8):
> >   iio:adc:ina2xx: Improve error reporting for problems during .remove()
> >   iio:adc:ti-ads1015: Improve error reporting for problems during
> >     .remove()
> >   iio:chemical:atlas: Improve error reporting for problems during
> >     .remove()
> >   iio:chemical:ccs811: Improve error reporting for problems during
> >     .remove()
> >   iio:light:pa12203001: Improve error reporting for problems during
> >     .remove()
> >   iio:light:us5182d: Improve error reporting for problems during
> >     .remove()
> >   iio:light:vcnl4000: Improve error reporting for problems during
> >     .remove()
> >   iio:light:vcnl4035: Improve error reporting for problems during
> >     .remove()
> > 
> >  drivers/iio/adc/ina2xx-adc.c        | 10 ++++++++--
> >  drivers/iio/adc/ti-ads1015.c        |  8 +++++++-
> >  drivers/iio/chemical/atlas-sensor.c |  8 +++++++-
> >  drivers/iio/chemical/ccs811.c       | 10 ++++++++--
> >  drivers/iio/light/pa12203001.c      |  8 +++++++-
> >  drivers/iio/light/us5182d.c         |  8 +++++++-
> >  drivers/iio/light/vcnl4000.c        |  8 +++++++-
> >  drivers/iio/light/vcnl4035.c        | 10 ++++++++--
> >  8 files changed, 59 insertions(+), 11 deletions(-)
> > 
> > 
> > base-commit: 3123109284176b1532874591f7c81f3837bbdc17  
>