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

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

[Akshay Jindal]

Use devm_add_action_or_reset to do cleanup when the device is removed.
Set client data with i2c_set_clientdata() to ensure indio_dev is accessible
in powerdown function.

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

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 | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 7733830dca67..f9c8e7fc57fd 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -680,6 +680,24 @@ static irqreturn_t ltr390_interrupt_handler(int irq, void *private)
 	return IRQ_HANDLED;
 }
 
+static void ltr390_powerdown(void *client)
+{
+	struct i2c_client *i2c_clnt = client;
+	struct iio_dev *indio_dev = i2c_get_clientdata(i2c_clnt);
+	struct ltr390_data *data = iio_priv(indio_dev);
+
+	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(&i2c_clnt->dev, "failed to disable interrupts\n");
+
+	if (regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
+			LTR390_SENSOR_ENABLE) < 0)
+		dev_err(&i2c_clnt->dev, "failed to disable sensor\n");
+}
+
 static int ltr390_probe(struct i2c_client *client)
 {
 	struct ltr390_data *data;
@@ -693,7 +711,7 @@ static int ltr390_probe(struct i2c_client *client)
 		return -ENOMEM;
 
 	data = iio_priv(indio_dev);
-
+	i2c_set_clientdata(client, 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 +751,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, client);
+	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 v2] iio: light: ltr390: Add device powerdown functionality via devm api

[Akshay Jindal]

On Tue, Aug 12, 2025 at 10:37 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
> Use devm_add_action_or_reset to do cleanup when the device is removed.
> Set client data with i2c_set_clientdata() to ensure indio_dev is accessible
> in powerdown function.
>
> Signed-off-by: Akshay Jindal <akshayaj.lkd@gmail.com>
> ---
Just realized I did not include a 'Changes since v1' section.

Changes since v1:
-------------------------
- Switched from remove callback to devm_* API for powerdown.
- Modified the changelog accordingly and summary accordingly.

Inorder to avoid redundancy, I am replying on the patch mail.
I am happy to RESEND the patch with updated Changes since v1 section.

Thanks,
Akshay

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

[Andy Shevchenko]

On Tue, Aug 12, 2025 at 7:07 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
> Use devm_add_action_or_reset to do cleanup when the device is removed.

devm_add_action_or_reset()

> Set client data with i2c_set_clientdata() to ensure indio_dev is accessible
> in powerdown function.

...

> +static void ltr390_powerdown(void *client)
> +{
> +       struct i2c_client *i2c_clnt = client;
> +       struct iio_dev *indio_dev = i2c_get_clientdata(i2c_clnt);

Why? I mean why can't you simply provide a data pointer directly as a
parameter to this function?

> +       struct ltr390_data *data = iio_priv(indio_dev);
> +
> +       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(&i2c_clnt->dev, "failed to disable interrupts\n");
> +
> +       if (regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
> +                       LTR390_SENSOR_ENABLE) < 0)
> +               dev_err(&i2c_clnt->dev, "failed to disable sensor\n");
> +}

...

> @@ -693,7 +711,7 @@ static int ltr390_probe(struct i2c_client *client)
>                 return -ENOMEM;
>
>         data = iio_priv(indio_dev);

> -

Stray change.

> +       i2c_set_clientdata(client, indio_dev);

I would suggest moving this to be before data = ... line above. And
add a blank line after it. BUT, read above first.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Akshay Jindal]

Sent a v3 for review. Acknowledged your comments inline.

On Wed, Aug 13, 2025 at 3:43 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Aug 12, 2025 at 7:07 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> >
> > Use devm_add_action_or_reset to do cleanup when the device is removed.
>
> devm_add_action_or_reset()
>
done

> > +static void ltr390_powerdown(void *client)
> > +{
> > +       struct i2c_client *i2c_clnt = client;
> > +       struct iio_dev *indio_dev = i2c_get_clientdata(i2c_clnt);
>
> Why? I mean why can't you simply provide a data pointer directly as a
> parameter to this function?
>
yeah that is much better. Done.

>
> > @@ -693,7 +711,7 @@ static int ltr390_probe(struct i2c_client *client)
> >                 return -ENOMEM;
> >
> >         data = iio_priv(indio_dev);
> Stray change.
>
> > +       i2c_set_clientdata(client, indio_dev);
>
> I would suggest moving this to be before data = ... line above. And
> add a blank line after it. BUT, read above first.
>
Done.