[PATCH 1/2] iio: temperature: tmp006: convert probe to device-managed

[PATCH 1/2] iio: temperature: tmp006: convert probe to device-managed

[Alexandru Ardelean]

This change converts the driver to register via devm_iio_device_register().
For the tmp006_powerdown() hook, this uses a devm_add_action() hook to put
the device in powerdown mode when it's unregistered.

With these changes, the remove hook can be removed.

The i2c_set_clientdata() call is staying around as the private data is used
in the PM routines.

Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
---
 drivers/iio/temperature/tmp006.c | 32 +++++++++++++++-----------------
 1 file changed, 15 insertions(+), 17 deletions(-)

diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
index 54976c7dad92..db1ac6029c27 100644
--- a/drivers/iio/temperature/tmp006.c
+++ b/drivers/iio/temperature/tmp006.c
@@ -193,6 +193,17 @@ static bool tmp006_check_identification(struct i2c_client *client)
 	return mid == TMP006_MANUFACTURER_MAGIC && did == TMP006_DEVICE_MAGIC;
 }
 
+static int tmp006_powerdown(struct tmp006_data *data)
+{
+	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
+		data->config & ~TMP006_CONFIG_MOD_MASK);
+}
+
+static void tmp006_powerdown_cleanup(void *data)
+{
+	tmp006_powerdown(data);
+}
+
 static int tmp006_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -228,23 +239,11 @@ static int tmp006_probe(struct i2c_client *client,
 		return ret;
 	data->config = ret;
 
-	return iio_device_register(indio_dev);
-}
-
-static int tmp006_powerdown(struct tmp006_data *data)
-{
-	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
-		data->config & ~TMP006_CONFIG_MOD_MASK);
-}
-
-static int tmp006_remove(struct i2c_client *client)
-{
-	struct iio_dev *indio_dev = i2c_get_clientdata(client);
-
-	iio_device_unregister(indio_dev);
-	tmp006_powerdown(iio_priv(indio_dev));
+	ret = devm_add_action(&client->dev, tmp006_powerdown_cleanup, data);
+	if (ret < 0)
+		return ret;
 
-	return 0;
+	return devm_iio_device_register(&client->dev, indio_dev);
 }
 
 #ifdef CONFIG_PM_SLEEP
@@ -277,7 +276,6 @@ static struct i2c_driver tmp006_driver = {
 		.pm	= &tmp006_pm_ops,
 	},
 	.probe = tmp006_probe,
-	.remove = tmp006_remove,
 	.id_table = tmp006_id,
 };
 module_i2c_driver(tmp006_driver);
-- 
2.31.1

[PATCH 2/2] iio: temperature: tmp006: make sure the chip is powered up in probe

[Alexandru Ardelean]

When the device is probed, there's no guarantee that the device is not in
power-down mode. This can happen if the driver is unregistered and
re-probed.

To make sure this doesn't happen, the value of the TMP006_CONFIG register
(which is read in the probe function and stored in the device's private
data) is being checked to see if the MOD bits have the correct value.

This is a fix for a somewhat-rare corner case. As it stands, this doesn't
look like a high priority to go into the Fixes route.

Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
---
 drivers/iio/temperature/tmp006.c | 33 +++++++++++++++++++++-----------
 1 file changed, 22 insertions(+), 11 deletions(-)

diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
index db1ac6029c27..e4943a0bc9aa 100644
--- a/drivers/iio/temperature/tmp006.c
+++ b/drivers/iio/temperature/tmp006.c
@@ -193,15 +193,23 @@ static bool tmp006_check_identification(struct i2c_client *client)
 	return mid == TMP006_MANUFACTURER_MAGIC && did == TMP006_DEVICE_MAGIC;
 }
 
-static int tmp006_powerdown(struct tmp006_data *data)
+static int tmp006_power(struct device *dev, bool up)
 {
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct tmp006_data *data = iio_priv(indio_dev);
+
+	if (up)
+		data->config |= TMP006_CONFIG_MOD_MASK;
+	else
+		data->config &= ~TMP006_CONFIG_MOD_MASK;
+
 	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
-		data->config & ~TMP006_CONFIG_MOD_MASK);
+		data->config);
 }
 
-static void tmp006_powerdown_cleanup(void *data)
+static void tmp006_powerdown_cleanup(void *dev)
 {
-	tmp006_powerdown(data);
+	tmp006_power(dev, false);
 }
 
 static int tmp006_probe(struct i2c_client *client,
@@ -239,7 +247,14 @@ static int tmp006_probe(struct i2c_client *client,
 		return ret;
 	data->config = ret;
 
-	ret = devm_add_action(&client->dev, tmp006_powerdown_cleanup, data);
+	if ((ret & TMP006_CONFIG_MOD_MASK) != TMP006_CONFIG_MOD_MASK) {
+		ret = tmp006_power(&client->dev, true);
+		if (ret < 0)
+			return ret;
+	}
+
+	ret = devm_add_action_or_reset(&client->dev, tmp006_powerdown_cleanup,
+				       &client->dev);
 	if (ret < 0)
 		return ret;
 
@@ -249,16 +264,12 @@ static int tmp006_probe(struct i2c_client *client,
 #ifdef CONFIG_PM_SLEEP
 static int tmp006_suspend(struct device *dev)
 {
-	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
-	return tmp006_powerdown(iio_priv(indio_dev));
+	return tmp006_power(dev, false);
 }
 
 static int tmp006_resume(struct device *dev)
 {
-	struct tmp006_data *data = iio_priv(i2c_get_clientdata(
-		to_i2c_client(dev)));
-	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
-		data->config | TMP006_CONFIG_MOD_MASK);
+	return tmp006_power(dev, true);
 }
 #endif
 
-- 
2.31.1

Re: [PATCH 2/2] iio: temperature: tmp006: make sure the chip is powered up in probe

[Jonathan Cameron]

On Thu, 24 Jun 2021 11:19:24 +0300
Alexandru Ardelean <aardelean@deviqon.com> wrote:

> When the device is probed, there's no guarantee that the device is not in
> power-down mode. This can happen if the driver is unregistered and
> re-probed.
> 
> To make sure this doesn't happen, the value of the TMP006_CONFIG register
> (which is read in the probe function and stored in the device's private
> data) is being checked to see if the MOD bits have the correct value.
> 
> This is a fix for a somewhat-rare corner case. As it stands, this doesn't
> look like a high priority to go into the Fixes route.
> 
> Signed-off-by: Alexandru Ardelean <aardelean@deviqon.com>
This one 'looks' right and as it's been on the list a while I'll apply it.
Would be great if anyone can test it though!

Series applied.

Thanks,

Jonathan

> ---
>  drivers/iio/temperature/tmp006.c | 33 +++++++++++++++++++++-----------
>  1 file changed, 22 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/temperature/tmp006.c b/drivers/iio/temperature/tmp006.c
> index db1ac6029c27..e4943a0bc9aa 100644
> --- a/drivers/iio/temperature/tmp006.c
> +++ b/drivers/iio/temperature/tmp006.c
> @@ -193,15 +193,23 @@ static bool tmp006_check_identification(struct i2c_client *client)
>  	return mid == TMP006_MANUFACTURER_MAGIC && did == TMP006_DEVICE_MAGIC;
>  }
>  
> -static int tmp006_powerdown(struct tmp006_data *data)
> +static int tmp006_power(struct device *dev, bool up)
>  {
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct tmp006_data *data = iio_priv(indio_dev);
> +
> +	if (up)
> +		data->config |= TMP006_CONFIG_MOD_MASK;
> +	else
> +		data->config &= ~TMP006_CONFIG_MOD_MASK;
> +
>  	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
> -		data->config & ~TMP006_CONFIG_MOD_MASK);
> +		data->config);
>  }
>  
> -static void tmp006_powerdown_cleanup(void *data)
> +static void tmp006_powerdown_cleanup(void *dev)
>  {
> -	tmp006_powerdown(data);
> +	tmp006_power(dev, false);
>  }
>  
>  static int tmp006_probe(struct i2c_client *client,
> @@ -239,7 +247,14 @@ static int tmp006_probe(struct i2c_client *client,
>  		return ret;
>  	data->config = ret;
>  
> -	ret = devm_add_action(&client->dev, tmp006_powerdown_cleanup, data);
> +	if ((ret & TMP006_CONFIG_MOD_MASK) != TMP006_CONFIG_MOD_MASK) {
> +		ret = tmp006_power(&client->dev, true);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(&client->dev, tmp006_powerdown_cleanup,
> +				       &client->dev);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -249,16 +264,12 @@ static int tmp006_probe(struct i2c_client *client,
>  #ifdef CONFIG_PM_SLEEP
>  static int tmp006_suspend(struct device *dev)
>  {
> -	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> -	return tmp006_powerdown(iio_priv(indio_dev));
> +	return tmp006_power(dev, false);
>  }
>  
>  static int tmp006_resume(struct device *dev)
>  {
> -	struct tmp006_data *data = iio_priv(i2c_get_clientdata(
> -		to_i2c_client(dev)));
> -	return i2c_smbus_write_word_swapped(data->client, TMP006_CONFIG,
> -		data->config | TMP006_CONFIG_MOD_MASK);
> +	return tmp006_power(dev, true);
>  }
>  #endif
>