[v3] iio: light: ltr390: Add device powerdown functionality via devm api

[PATCH v3] iio: light: ltr390: Add device powerdown functionality via devm api

Posted by Akshay Jindal 1 month, 3 weeks ago

Use devm_add_action_or_reset() to do cleanup when the device is removed.
Set client data with i2c_set_clientdata().

Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
---

Changes Since v2:
-----------------
- Replace i2c_client with ltr390_data as action parameter of devm_add_action_or_reset().
- Place i2c_set_clientdata above data population line.
- Modify changelog accordingly.

Changes Since v1:
-----------------
- Switch from remove callback to devm_* API for powerdown.
- Modify changelog and summary accordingly.

Testing details:
----------------
-> Tested on Raspberrypi 4B. Following tests were performed.

1. Sensor and interrupts should be disabled after module unload.
-> Before unload
akshayaj@raspberrypi:~ $ echo 1 | sudo tee /sys/bus/iio/devices/iio\:device0/events/in_illuminance_thresh_either_en
1
akshayaj@raspberrypi:~ $ cat /sys/bus/iio/devices/iio\:device0/events/in_illuminance_thresh_either_en
1
akshayaj@raspberrypi:~ $ i2cget -f -y 1 0x53 0x19
0x14
akshayaj@raspberrypi:~ $ i2cget -f -y 1 0x53 0x0
0x02

-> After unload
akshayaj@raspberrypi:~ $ sudo rmmod ltr390
akshayaj@raspberrypi:~ $ i2cget -f -y 1 0x53 0x0
0x00
akshayaj@raspberrypi:~ $ i2cget -f -y 1 0x53 0x19
0x10

 drivers/iio/light/ltr390.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 7733830dca67..07b8dd27dd9a 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -680,6 +680,22 @@ static irqreturn_t ltr390_interrupt_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static void ltr390_powerdown(void *priv)
+{
+	struct ltr390_data *data = priv;
+
+	guard(mutex)(&data->lock);
+
+	/* Ensure that power off and interrupts are disabled */
+	if (regmap_clear_bits(data->regmap, LTR390_INT_CFG,
+				LTR390_LS_INT_EN) < 0)
+		dev_err(&data->client->dev, "failed to disable interrupts\n");
+
+	if (regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
+			LTR390_SENSOR_ENABLE) < 0)
+		dev_err(&data->client->dev, "failed to disable sensor\n");
+}
+
 static int ltr390_probe(struct i2c_client *client)
 {
 	struct ltr390_data *data;
@@ -692,8 +708,9 @@ static int ltr390_probe(struct i2c_client *client)
 	if (!indio_dev)
 		return -ENOMEM;
 
-	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
 
+	data = iio_priv(indio_dev);
 	data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
 	if (IS_ERR(data->regmap))
 		return dev_err_probe(dev, PTR_ERR(data->regmap),
@@ -733,6 +750,10 @@ static int ltr390_probe(struct i2c_client *client)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to enable the sensor\n");
 
+	ret = devm_add_action_or_reset(dev, ltr390_powerdown, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add action or reset\n");
+
 	if (client->irq) {
 		ret = devm_request_threaded_irq(dev, client->irq,
 						NULL, ltr390_interrupt_handler,
-- 
2.43.0

Re: [PATCH v3] iio: light: ltr390: Add device powerdown functionality via devm api

Posted by Andy Shevchenko 1 month, 3 weeks ago

On Wed, Aug 13, 2025 at 07:00:14PM +0530, Akshay Jindal wrote:
> Use devm_add_action_or_reset() to do cleanup when the device is removed.

> Set client data with i2c_set_clientdata().

This is not used anymore, correct?

...

> -	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
>  
> +	data = iio_priv(indio_dev);
>  	data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
>  	if (IS_ERR(data->regmap))
>  		return dev_err_probe(dev, PTR_ERR(data->regmap),

So this hunk needs to be removed from the patch.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: light: ltr390: Add device powerdown functionality via devm api

Posted by Akshay Jindal 1 month, 3 weeks ago

On Wed, Aug 13, 2025 at 7:20 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Aug 13, 2025 at 07:00:14PM +0530, Akshay Jindal wrote:
> > Use devm_add_action_or_reset() to do cleanup when the device is removed.
>
> > Set client data with i2c_set_clientdata().
>
> This is not used anymore, correct?
>
> ...
>
> > -     data = iio_priv(indio_dev);
> > +     i2c_set_clientdata(client, indio_dev);
> >
> > +     data = iio_priv(indio_dev);
> >       data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
> >       if (IS_ERR(data->regmap))
> >               return dev_err_probe(dev, PTR_ERR(data->regmap),
>
> So this hunk needs to be removed from the patch.
I thought so, but removing i2c_set_clientdata would mean that
dev->driver_data will NOT contain a pointer to indio_dev.
Irrespective of usage, ideally dev->driver_data should contain legit value.
Hence I kept it.
If you feel otherwise, I can remove it, but I feel this should be kept.

Thanks,
Akshay

Re: [PATCH v3] iio: light: ltr390: Add device powerdown functionality via devm api

Posted by Andy Shevchenko 1 month, 3 weeks ago

On Wed, Aug 13, 2025 at 07:27:09PM +0530, Akshay Jindal wrote:
> On Wed, Aug 13, 2025 at 7:20 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Aug 13, 2025 at 07:00:14PM +0530, Akshay Jindal wrote:


...

> > > Set client data with i2c_set_clientdata().
> >
> > This is not used anymore, correct?

...

> > > -     data = iio_priv(indio_dev);
> > > +     i2c_set_clientdata(client, indio_dev);
> > >
> > > +     data = iio_priv(indio_dev);
> > >       data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
> > >       if (IS_ERR(data->regmap))
> > >               return dev_err_probe(dev, PTR_ERR(data->regmap),
> >
> > So this hunk needs to be removed from the patch.
> I thought so, but removing i2c_set_clientdata would mean that
> dev->driver_data will NOT contain a pointer to indio_dev.
> Irrespective of usage, ideally dev->driver_data should contain legit value.

NULL is legit value for driver_data. The rule of thumb is that we don't assign
something that we know will never be used. Also think about debugging issues,
with the "legit" value in some pointer it may lead to not noticing a real
problem or to a problem that never exists if driver_data left untouched.

> Hence I kept it.
> If you feel otherwise, I can remove it, but I feel this should be kept.

I feel definitely otherwise.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] iio: light: ltr390: Add device powerdown functionality via devm api

Posted by Akshay Jindal 1 month, 3 weeks ago

On Thu, Aug 14, 2025 at 3:13 AM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Aug 13, 2025 at 07:27:09PM +0530, Akshay Jindal wrote:
> > I thought so, but removing i2c_set_clientdata would mean that
> > dev->driver_data will NOT contain a pointer to indio_dev.
> > Irrespective of usage, ideally dev->driver_data should contain legit value.
>
> NULL is legit value for driver_data. The rule of thumb is that we don't assign
> something that we know will never be used. Also think about debugging issues,
> with the "legit" value in some pointer it may lead to not noticing a real
> problem or to a problem that never exists if driver_data left untouched.
>
> > Hence I kept it.
> > If you feel otherwise, I can remove it, but I feel this should be kept.
>
> I feel definitely otherwise.
Removed. Floated a v4.

Thanks,
Akshay