[PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Akshay Jindal]

Implement the .remove() callback in the ltr390 driver to ensure proper
cleanup when the device is removed.

Set client data with i2c_set_clientdata() to ensure indio_dev is accessible
in .remove(). Replace devm_iio_device_register() with iio_device_register()
and explicitly unregister the device to align with the updated removal path
and follow common patterns used in other IIO drivers.

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, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
index 7733830dca67..af1b04c8524e 100644
--- a/drivers/iio/light/ltr390.c
+++ b/drivers/iio/light/ltr390.c
@@ -693,7 +693,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),
@@ -744,7 +744,26 @@ static int ltr390_probe(struct i2c_client *client)
 					     "request irq (%d) failed\n", client->irq);
 	}
 
-	return devm_iio_device_register(dev, indio_dev);
+	return iio_device_register(indio_dev);
+}
+
+static void ltr390_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	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(&client->dev, "failed to disable interrupts\n");
+
+	if (regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
+			LTR390_SENSOR_ENABLE) < 0)
+		dev_err(&client->dev, "failed to disable sensor\n");
+
+	iio_device_unregister(indio_dev);
 }
 
 static int ltr390_suspend(struct device *dev)
@@ -786,6 +805,7 @@ static struct i2c_driver ltr390_driver = {
 		.pm = pm_sleep_ptr(&ltr390_pm_ops),
 	},
 	.probe = ltr390_probe,
+	.remove = ltr390_remove,
 	.id_table = ltr390_id,
 };
 module_i2c_driver(ltr390_driver);
-- 
2.43.0

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Andy Shevchenko]

On Mon, Aug 4, 2025 at 9:25 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
> Implement the .remove() callback in the ltr390 driver to ensure proper
> cleanup when the device is removed.
>
> Set client data with i2c_set_clientdata() to ensure indio_dev is accessible
> in .remove(). Replace devm_iio_device_register() with iio_device_register()
> and explicitly unregister the device to align with the updated removal path
> and follow common patterns used in other IIO drivers.

Doesn't sound right to me. HAve you investigated PM runtime paths?
Looking at what the code you added there it sounds to me like a part
of PM runtime ->suspend() callback.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Akshay Jindal]

On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> Doesn't sound right to me. HAve you investigated PM runtime paths?
Yes I did investigate and found that PM runtime->suspend() callback
co-exists with remove callback.

> Looking at what the code you added there it sounds to me like a part
> of PM runtime ->suspend() callback.
Yes, part of functionality will always be common, because both the
callback implementations put
the device into powered down or low power state which is what has been done here
Both _suspend() and remove are called at different times in the lifecycle of the
driver, but with respect to register setting, they put the device into
power down state.
Additionally .remove() can have code for:
1. disable runtime power management (if enabled while device registration).
2. device cleanup (disabling interrupt and cleaning up other configs done).
2. unregister the device.

For eg: another light sensor bh1750
static void bh1750_remove(struct i2c_client *client)
{
    iio_device_unregister(indio_dev);
    mutex_lock(&data->lock);
    i2c_smbus_write_byte(client, BH1750_POWER_DOWN);
    mutex_unlock(&data->lock);
}

static int bh1750_suspend(struct device *dev)
{
    mutex_lock(&data->lock);
    ret = i2c_smbus_write_byte(data->client, BH1750_POWER_DOWN);
    mutex_unlock(&data->lock);
    return ret;
}

In drivers/iio/light, you can find similar examples in pa12203001,
rpr0521, apds9960,
vcnl4000, isl29028, vcnl4035. You can find many more examples in
sensors other than light sensors.

Thanks,
Akshay

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Andy Shevchenko]

On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
> On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > Doesn't sound right to me. HAve you investigated PM runtime paths?
> Yes I did investigate and found that PM runtime->suspend() callback
> co-exists with remove callback.
>
> > Looking at what the code you added there it sounds to me like a part
> > of PM runtime ->suspend() callback.
> Yes, part of functionality will always be common, because both the
> callback implementations put
> the device into powered down or low power state which is what has been done here
> Both _suspend() and remove are called at different times in the lifecycle of the
> driver, but with respect to register setting, they put the device into
> power down state.

Are you sure about the remove stage and how it interacts with runtime
PM? Please, check how the device driver core manages PM on the remove
stage.

> Additionally .remove() can have code for:
> 1. disable runtime power management (if enabled while device registration).

If the device core enables it for you, it will disable it
symmetrically. I don't see the issue here, it should be done
automatically. If you do that explicitly, use the respective
devm_pm_runtime_*() call.

> 2. device cleanup (disabling interrupt and cleaning up other configs done).

Wrap them into devm if required.

> 2. unregister the device.

Already done in the original code which your patch reverts, why?

> For eg: another light sensor bh1750
> static void bh1750_remove(struct i2c_client *client)
> {
>     iio_device_unregister(indio_dev);
>     mutex_lock(&data->lock);
>     i2c_smbus_write_byte(client, BH1750_POWER_DOWN);
>     mutex_unlock(&data->lock);
> }
>
> static int bh1750_suspend(struct device *dev)
> {
>     mutex_lock(&data->lock);
>     ret = i2c_smbus_write_byte(data->client, BH1750_POWER_DOWN);
>     mutex_unlock(&data->lock);
>     return ret;
> }

Correct and where do you see the problem here? Perhaps the problem is
in the cleanup aordering and some other bugs vs. devm calls?

> In drivers/iio/light, you can find similar examples in pa12203001,
> rpr0521, apds9960,
> vcnl4000, isl29028, vcnl4035. You can find many more examples in
> sensors other than light sensors.

Good, should all they need to be fixed?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Jonathan Cameron]

On Tue, 5 Aug 2025 14:47:32 +0200
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> >
> > On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:  
> > >
> > > Doesn't sound right to me. HAve you investigated PM runtime paths?  
> > Yes I did investigate and found that PM runtime->suspend() callback
> > co-exists with remove callback.
> >  
> > > Looking at what the code you added there it sounds to me like a part
> > > of PM runtime ->suspend() callback.  
> > Yes, part of functionality will always be common, because both the
> > callback implementations put
> > the device into powered down or low power state which is what has been done here
> > Both _suspend() and remove are called at different times in the lifecycle of the
> > driver, but with respect to register setting, they put the device into
> > power down state.  
> 
> Are you sure about the remove stage and how it interacts with runtime
> PM? Please, check how the device driver core manages PM on the remove
> stage.
> 
> > Additionally .remove() can have code for:
> > 1. disable runtime power management (if enabled while device registration).  
> 
> If the device core enables it for you, it will disable it
> symmetrically. I don't see the issue here, it should be done
> automatically. If you do that explicitly, use the respective
> devm_pm_runtime_*() call.
> 
> > 2. device cleanup (disabling interrupt and cleaning up other configs done).  
> 
> Wrap them into devm if required.
> 
> > 2. unregister the device.  
> 
> Already done in the original code which your patch reverts, why?
> 
> > For eg: another light sensor bh1750
> > static void bh1750_remove(struct i2c_client *client)
> > {
> >     iio_device_unregister(indio_dev);
> >     mutex_lock(&data->lock);
> >     i2c_smbus_write_byte(client, BH1750_POWER_DOWN);
> >     mutex_unlock(&data->lock);
> > }
> >
> > static int bh1750_suspend(struct device *dev)
> > {
> >     mutex_lock(&data->lock);
> >     ret = i2c_smbus_write_byte(data->client, BH1750_POWER_DOWN);
> >     mutex_unlock(&data->lock);
> >     return ret;
> > }  
> 
> Correct and where do you see the problem here? Perhaps the problem is
> in the cleanup aordering and some other bugs vs. devm calls?
> 
> > In drivers/iio/light, you can find similar examples in pa12203001,
> > rpr0521, apds9960,
> > vcnl4000, isl29028, vcnl4035. You can find many more examples in
> > sensors other than light sensors.  
> 
> Good, should all they need to be fixed?

The complex corners that occur with devm + runtime pm are around
things that we must not run if we are already in runtime suspend.
Typically disabling power supplies (as we can underflow counters
and getting warning prints).  Seeing as this driver is not
doing that it should be simple to use a devm_add_action_or_reset()

Key thing to consider is that runtime pm may not be built.
So the flow should work with those calls doing nothing.  That means that
if you turn the device on in probe we should make sure to explicitly turn
it off in the remove flow. That's where devm_add_action_or_reset()
comes in handy.

ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
Is the paired operation with the second disable you add in remove.
Wrap that in a devm callback.

More complex is the interrupt enable as that doesn't pair with
anything in particular in probe. I'm curious though, do we need
to disable it given the next operation turns off the sensor and on
probe we reset the sensor.

Is just clearing the enable bit enough? 

Jonathan



> 

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Andy Shevchenko]

On Wed, Aug 06, 2025 at 04:18:01PM +0100, Jonathan Cameron wrote:
> On Tue, 5 Aug 2025 14:47:32 +0200
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > > On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:  

...

> > > > Doesn't sound right to me. HAve you investigated PM runtime paths?  
> > > Yes I did investigate and found that PM runtime->suspend() callback
> > > co-exists with remove callback.
> > >  
> > > > Looking at what the code you added there it sounds to me like a part
> > > > of PM runtime ->suspend() callback.  
> > > Yes, part of functionality will always be common, because both the
> > > callback implementations put
> > > the device into powered down or low power state which is what has been done here
> > > Both _suspend() and remove are called at different times in the lifecycle of the
> > > driver, but with respect to register setting, they put the device into
> > > power down state.  
> > 
> > Are you sure about the remove stage and how it interacts with runtime
> > PM? Please, check how the device driver core manages PM on the remove
> > stage.
> > 
> > > Additionally .remove() can have code for:
> > > 1. disable runtime power management (if enabled while device registration).  
> > 
> > If the device core enables it for you, it will disable it
> > symmetrically. I don't see the issue here, it should be done
> > automatically. If you do that explicitly, use the respective
> > devm_pm_runtime_*() call.
> > 
> > > 2. device cleanup (disabling interrupt and cleaning up other configs done).  
> > 
> > Wrap them into devm if required.
> > 
> > > 2. unregister the device.  
> > 
> > Already done in the original code which your patch reverts, why?
> > 
> > > For eg: another light sensor bh1750
> > > static void bh1750_remove(struct i2c_client *client)
> > > {
> > >     iio_device_unregister(indio_dev);
> > >     mutex_lock(&data->lock);
> > >     i2c_smbus_write_byte(client, BH1750_POWER_DOWN);
> > >     mutex_unlock(&data->lock);
> > > }
> > >
> > > static int bh1750_suspend(struct device *dev)
> > > {
> > >     mutex_lock(&data->lock);
> > >     ret = i2c_smbus_write_byte(data->client, BH1750_POWER_DOWN);
> > >     mutex_unlock(&data->lock);
> > >     return ret;
> > > }  
> > 
> > Correct and where do you see the problem here? Perhaps the problem is
> > in the cleanup aordering and some other bugs vs. devm calls?
> > 
> > > In drivers/iio/light, you can find similar examples in pa12203001,
> > > rpr0521, apds9960,
> > > vcnl4000, isl29028, vcnl4035. You can find many more examples in
> > > sensors other than light sensors.  
> > 
> > Good, should all they need to be fixed?
> 
> The complex corners that occur with devm + runtime pm are around
> things that we must not run if we are already in runtime suspend.
> Typically disabling power supplies (as we can underflow counters
> and getting warning prints).  Seeing as this driver is not
> doing that it should be simple to use a devm_add_action_or_reset()
> 
> Key thing to consider is that runtime pm may not be built.

This will mean that user does not want to handle PM at all at runtime, so why
should it be our problem? If device is off, it's not the problem of the driver
to do the power cycle at run time (yes, this might not apply to the system
suspend and resume cases, which has to be implemented as well).

> So the flow should work with those calls doing nothing.  That means that
> if you turn the device on in probe we should make sure to explicitly turn
> it off in the remove flow. That's where devm_add_action_or_reset()
> comes in handy.

I don't think we should do that explicitly in remove. As I pointed out above,
this the case that driver should not override.  Otherwise there is no point in
having the common runtime PM. User deliberately makes it not compiled, so they
should prepare to leave with it.

> ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> Is the paired operation with the second disable you add in remove.
> Wrap that in a devm callback.
> 
> More complex is the interrupt enable as that doesn't pair with
> anything in particular in probe. I'm curious though, do we need
> to disable it given the next operation turns off the sensor and on
> probe we reset the sensor.
> 
> Is just clearing the enable bit enough? 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Jonathan Cameron]

On Wed, 6 Aug 2025 23:02:44 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Wed, Aug 06, 2025 at 04:18:01PM +0100, Jonathan Cameron wrote:
> > On Tue, 5 Aug 2025 14:47:32 +0200
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:  
> > > > On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> > > > <andy.shevchenko@gmail.com> wrote:    
> 
> ...
> 
> > > > > Doesn't sound right to me. HAve you investigated PM runtime paths?    
> > > > Yes I did investigate and found that PM runtime->suspend() callback
> > > > co-exists with remove callback.
> > > >    
> > > > > Looking at what the code you added there it sounds to me like a part
> > > > > of PM runtime ->suspend() callback.    
> > > > Yes, part of functionality will always be common, because both the
> > > > callback implementations put
> > > > the device into powered down or low power state which is what has been done here
> > > > Both _suspend() and remove are called at different times in the lifecycle of the
> > > > driver, but with respect to register setting, they put the device into
> > > > power down state.    
> > > 
> > > Are you sure about the remove stage and how it interacts with runtime
> > > PM? Please, check how the device driver core manages PM on the remove
> > > stage.
> > >   
> > > > Additionally .remove() can have code for:
> > > > 1. disable runtime power management (if enabled while device registration).    
> > > 
> > > If the device core enables it for you, it will disable it
> > > symmetrically. I don't see the issue here, it should be done
> > > automatically. If you do that explicitly, use the respective
> > > devm_pm_runtime_*() call.
> > >   
> > > > 2. device cleanup (disabling interrupt and cleaning up other configs done).    
> > > 
> > > Wrap them into devm if required.
> > >   
> > > > 2. unregister the device.    
> > > 
> > > Already done in the original code which your patch reverts, why?
> > >   
> > > > For eg: another light sensor bh1750
> > > > static void bh1750_remove(struct i2c_client *client)
> > > > {
> > > >     iio_device_unregister(indio_dev);
> > > >     mutex_lock(&data->lock);
> > > >     i2c_smbus_write_byte(client, BH1750_POWER_DOWN);
> > > >     mutex_unlock(&data->lock);
> > > > }
> > > >
> > > > static int bh1750_suspend(struct device *dev)
> > > > {
> > > >     mutex_lock(&data->lock);
> > > >     ret = i2c_smbus_write_byte(data->client, BH1750_POWER_DOWN);
> > > >     mutex_unlock(&data->lock);
> > > >     return ret;
> > > > }    
> > > 
> > > Correct and where do you see the problem here? Perhaps the problem is
> > > in the cleanup aordering and some other bugs vs. devm calls?
> > >   
> > > > In drivers/iio/light, you can find similar examples in pa12203001,
> > > > rpr0521, apds9960,
> > > > vcnl4000, isl29028, vcnl4035. You can find many more examples in
> > > > sensors other than light sensors.    
> > > 
> > > Good, should all they need to be fixed?  
> > 
> > The complex corners that occur with devm + runtime pm are around
> > things that we must not run if we are already in runtime suspend.
> > Typically disabling power supplies (as we can underflow counters
> > and getting warning prints).  Seeing as this driver is not
> > doing that it should be simple to use a devm_add_action_or_reset()
> > 
> > Key thing to consider is that runtime pm may not be built.  
> 
> This will mean that user does not want to handle PM at all at runtime, so why
> should it be our problem? If device is off, it's not the problem of the driver
> to do the power cycle at run time (yes, this might not apply to the system
> suspend and resume cases, which has to be implemented as well).
> 
> > So the flow should work with those calls doing nothing.  That means that
> > if you turn the device on in probe we should make sure to explicitly turn
> > it off in the remove flow. That's where devm_add_action_or_reset()
> > comes in handy.  
> 
> I don't think we should do that explicitly in remove. As I pointed out above,
> this the case that driver should not override.  Otherwise there is no point in
> having the common runtime PM. User deliberately makes it not compiled, so they
> should prepare to leave with it.

Hmm. I don't agree. We turned it on so on error or remove I think we
should turn it off again.  We do that in many drivers that never made use of
any of the standard PM stuff because they only touch enable and disable in
probe and remove.  If nothing else I don't like the lack of balance between
probe and remove if we don't do it.

Jonathan

> 
> > ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> > Is the paired operation with the second disable you add in remove.
> > Wrap that in a devm callback.
> > 
> > More complex is the interrupt enable as that doesn't pair with
> > anything in particular in probe. I'm curious though, do we need
> > to disable it given the next operation turns off the sensor and on
> > probe we reset the sensor.
> > 
> > Is just clearing the enable bit enough?   
> 

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Akshay Jindal]

On Tue, Aug 5, 2025 at 6:18 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> Are you sure about the remove stage and how it interacts with runtime
> PM? Please, check how the device driver core manages PM on the remove
> stage.
This driver does not even have support for runtime power management.
Pardon me, but I am not able to see how runtime PM came into picture.
Am I missing something here?

Code walkthrough says the remove call flows from
driver_unregister------__device_release_driver--------
dev->bus->remove ---- i2c_device_remove-----.remove() callback.

Request you to correct me here.

Thanks,
Akshay

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Andy Shevchenko]

On Fri, Aug 8, 2025 at 4:24 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
>
> On Tue, Aug 5, 2025 at 6:18 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > Are you sure about the remove stage and how it interacts with runtime
> > PM? Please, check how the device driver core manages PM on the remove
> > stage.
> This driver does not even have support for runtime power management.

Exactly. And my point is to enable it instead of playing tricks on ->remove().

> Pardon me, but I am not able to see how runtime PM came into picture.
> Am I missing something here?
>
> Code walkthrough says the remove call flows from
> driver_unregister------__device_release_driver--------
> dev->bus->remove ---- i2c_device_remove-----.remove() callback.
>
> Request you to correct me here.

Right, and now find the PM runtime calls there. Put them in the
picture and you will see my point.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Akshay Jindal]

On Fri, Aug 8, 2025 at 8:04 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Aug 8, 2025 at 4:24 PM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> >
> > On Tue, Aug 5, 2025 at 6:18 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > Are you sure about the remove stage and how it interacts with runtime
> > > PM? Please, check how the device driver core manages PM on the remove
> > > stage.
> > This driver does not even have support for runtime power management.
>
> Exactly. And my point is to enable it instead of playing tricks on ->remove().
Got it!!

>
> > Pardon me, but I am not able to see how runtime PM came into picture.
> > Am I missing something here?
> >
> > Code walkthrough says the remove call flows from
> > driver_unregister------__device_release_driver--------
> > dev->bus->remove ---- i2c_device_remove-----.remove() callback.
> >
> > Request you to correct me here.
>
> Right, and now find the PM runtime calls there. Put them in the
> picture and you will see my point.
>
Thanks Andy, will do that.

Regards,
Akshay

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Andy Shevchenko]

On Thu, Aug 07, 2025 at 02:04:01PM +0100, Jonathan Cameron wrote:
> On Wed, 6 Aug 2025 23:02:44 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Wed, Aug 06, 2025 at 04:18:01PM +0100, Jonathan Cameron wrote:
> > > On Tue, 5 Aug 2025 14:47:32 +0200
> > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:  
> > > > > On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> > > > > <andy.shevchenko@gmail.com> wrote:    

...

> > > > > > Doesn't sound right to me. HAve you investigated PM runtime paths?    
> > > > > Yes I did investigate and found that PM runtime->suspend() callback
> > > > > co-exists with remove callback.
> > > > >    
> > > > > > Looking at what the code you added there it sounds to me like a part
> > > > > > of PM runtime ->suspend() callback.    
> > > > > Yes, part of functionality will always be common, because both the
> > > > > callback implementations put
> > > > > the device into powered down or low power state which is what has been done here
> > > > > Both _suspend() and remove are called at different times in the lifecycle of the
> > > > > driver, but with respect to register setting, they put the device into
> > > > > power down state.    
> > > > 
> > > > Are you sure about the remove stage and how it interacts with runtime
> > > > PM? Please, check how the device driver core manages PM on the remove
> > > > stage.
> > > >   
> > > > > Additionally .remove() can have code for:
> > > > > 1. disable runtime power management (if enabled while device registration).    
> > > > 
> > > > If the device core enables it for you, it will disable it
> > > > symmetrically. I don't see the issue here, it should be done
> > > > automatically. If you do that explicitly, use the respective
> > > > devm_pm_runtime_*() call.
> > > >   
> > > > > 2. device cleanup (disabling interrupt and cleaning up other configs done).    
> > > > 
> > > > Wrap them into devm if required.
> > > >   
> > > > > 2. unregister the device.    
> > > > 
> > > > Already done in the original code which your patch reverts, why?
> > > >   
> > > > > For eg: another light sensor bh1750
> > > > > static void bh1750_remove(struct i2c_client *client)
> > > > > {
> > > > >     iio_device_unregister(indio_dev);
> > > > >     mutex_lock(&data->lock);
> > > > >     i2c_smbus_write_byte(client, BH1750_POWER_DOWN);
> > > > >     mutex_unlock(&data->lock);
> > > > > }
> > > > >
> > > > > static int bh1750_suspend(struct device *dev)
> > > > > {
> > > > >     mutex_lock(&data->lock);
> > > > >     ret = i2c_smbus_write_byte(data->client, BH1750_POWER_DOWN);
> > > > >     mutex_unlock(&data->lock);
> > > > >     return ret;
> > > > > }    
> > > > 
> > > > Correct and where do you see the problem here? Perhaps the problem is
> > > > in the cleanup aordering and some other bugs vs. devm calls?
> > > >   
> > > > > In drivers/iio/light, you can find similar examples in pa12203001,
> > > > > rpr0521, apds9960,
> > > > > vcnl4000, isl29028, vcnl4035. You can find many more examples in
> > > > > sensors other than light sensors.    
> > > > 
> > > > Good, should all they need to be fixed?  
> > > 
> > > The complex corners that occur with devm + runtime pm are around
> > > things that we must not run if we are already in runtime suspend.
> > > Typically disabling power supplies (as we can underflow counters
> > > and getting warning prints).  Seeing as this driver is not
> > > doing that it should be simple to use a devm_add_action_or_reset()
> > > 
> > > Key thing to consider is that runtime pm may not be built.  
> > 
> > This will mean that user does not want to handle PM at all at runtime, so why
> > should it be our problem? If device is off, it's not the problem of the driver
> > to do the power cycle at run time (yes, this might not apply to the system
> > suspend and resume cases, which has to be implemented as well).
> > 
> > > So the flow should work with those calls doing nothing.  That means that
> > > if you turn the device on in probe we should make sure to explicitly turn
> > > it off in the remove flow. That's where devm_add_action_or_reset()
> > > comes in handy.  
> > 
> > I don't think we should do that explicitly in remove. As I pointed out above,
> > this the case that driver should not override.  Otherwise there is no point in
> > having the common runtime PM. User deliberately makes it not compiled, so they
> > should prepare to leave with it.
> 
> Hmm. I don't agree. We turned it on so on error or remove I think we
> should turn it off again.  We do that in many drivers that never made use of
> any of the standard PM stuff because they only touch enable and disable in
> probe and remove.  If nothing else I don't like the lack of balance between
> probe and remove if we don't do it.

We can do it, but this sounds to me like a step back. Implementing proper PM
runtime callbacks is a step forward.

Doing the former in the existing drivers is not an argument to me because all
of them avoiding use of the PM APIs. Note, with PM APIs it may also involve
devlink and other benefits that device driver core gives us.

> > > ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> > > Is the paired operation with the second disable you add in remove.
> > > Wrap that in a devm callback.
> > > 
> > > More complex is the interrupt enable as that doesn't pair with
> > > anything in particular in probe. I'm curious though, do we need
> > > to disable it given the next operation turns off the sensor and on
> > > probe we reset the sensor.
> > > 
> > > Is just clearing the enable bit enough?   

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Jonathan Cameron]

On Sat, 9 Aug 2025 12:53:40 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Thu, Aug 07, 2025 at 02:04:01PM +0100, Jonathan Cameron wrote:
> > On Wed, 6 Aug 2025 23:02:44 +0300
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> > > On Wed, Aug 06, 2025 at 04:18:01PM +0100, Jonathan Cameron wrote:  
> > > > On Tue, 5 Aug 2025 14:47:32 +0200
> > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:    
> > > > > On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:    
> > > > > > On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> > > > > > <andy.shevchenko@gmail.com> wrote:      
> 
> ...
> 
> > > > > > > Doesn't sound right to me. HAve you investigated PM runtime paths?      
> > > > > > Yes I did investigate and found that PM runtime->suspend() callback
> > > > > > co-exists with remove callback.
> > > > > >      
> > > > > > > Looking at what the code you added there it sounds to me like a part
> > > > > > > of PM runtime ->suspend() callback.      
> > > > > > Yes, part of functionality will always be common, because both the
> > > > > > callback implementations put
> > > > > > the device into powered down or low power state which is what has been done here
> > > > > > Both _suspend() and remove are called at different times in the lifecycle of the
> > > > > > driver, but with respect to register setting, they put the device into
> > > > > > power down state.      
> > > > > 
> > > > > Are you sure about the remove stage and how it interacts with runtime
> > > > > PM? Please, check how the device driver core manages PM on the remove
> > > > > stage.
> > > > >     
> > > > > > Additionally .remove() can have code for:
> > > > > > 1. disable runtime power management (if enabled while device registration).      
> > > > > 
> > > > > If the device core enables it for you, it will disable it
> > > > > symmetrically. I don't see the issue here, it should be done
> > > > > automatically. If you do that explicitly, use the respective
> > > > > devm_pm_runtime_*() call.
> > > > >     
> > > > > > 2. device cleanup (disabling interrupt and cleaning up other configs done).      
> > > > > 
> > > > > Wrap them into devm if required.
> > > > >     
> > > > > > 2. unregister the device.      
> > > > > 
> > > > > Already done in the original code which your patch reverts, why?
> > > > >     
> > > > > > For eg: another light sensor bh1750
> > > > > > static void bh1750_remove(struct i2c_client *client)
> > > > > > {
> > > > > >     iio_device_unregister(indio_dev);
> > > > > >     mutex_lock(&data->lock);
> > > > > >     i2c_smbus_write_byte(client, BH1750_POWER_DOWN);
> > > > > >     mutex_unlock(&data->lock);
> > > > > > }
> > > > > >
> > > > > > static int bh1750_suspend(struct device *dev)
> > > > > > {
> > > > > >     mutex_lock(&data->lock);
> > > > > >     ret = i2c_smbus_write_byte(data->client, BH1750_POWER_DOWN);
> > > > > >     mutex_unlock(&data->lock);
> > > > > >     return ret;
> > > > > > }      
> > > > > 
> > > > > Correct and where do you see the problem here? Perhaps the problem is
> > > > > in the cleanup aordering and some other bugs vs. devm calls?
> > > > >     
> > > > > > In drivers/iio/light, you can find similar examples in pa12203001,
> > > > > > rpr0521, apds9960,
> > > > > > vcnl4000, isl29028, vcnl4035. You can find many more examples in
> > > > > > sensors other than light sensors.      
> > > > > 
> > > > > Good, should all they need to be fixed?    
> > > > 
> > > > The complex corners that occur with devm + runtime pm are around
> > > > things that we must not run if we are already in runtime suspend.
> > > > Typically disabling power supplies (as we can underflow counters
> > > > and getting warning prints).  Seeing as this driver is not
> > > > doing that it should be simple to use a devm_add_action_or_reset()
> > > > 
> > > > Key thing to consider is that runtime pm may not be built.    
> > > 
> > > This will mean that user does not want to handle PM at all at runtime, so why
> > > should it be our problem? If device is off, it's not the problem of the driver
> > > to do the power cycle at run time (yes, this might not apply to the system
> > > suspend and resume cases, which has to be implemented as well).
> > >   
> > > > So the flow should work with those calls doing nothing.  That means that
> > > > if you turn the device on in probe we should make sure to explicitly turn
> > > > it off in the remove flow. That's where devm_add_action_or_reset()
> > > > comes in handy.    
> > > 
> > > I don't think we should do that explicitly in remove. As I pointed out above,
> > > this the case that driver should not override.  Otherwise there is no point in
> > > having the common runtime PM. User deliberately makes it not compiled, so they
> > > should prepare to leave with it.  
> > 
> > Hmm. I don't agree. We turned it on so on error or remove I think we
> > should turn it off again.  We do that in many drivers that never made use of
> > any of the standard PM stuff because they only touch enable and disable in
> > probe and remove.  If nothing else I don't like the lack of balance between
> > probe and remove if we don't do it.  
> 
> We can do it, but this sounds to me like a step back. Implementing proper PM
> runtime callbacks is a step forward.
I entirely agree that runtime PM is good to have and it does a lot more
than just turning the power on and off once per probe / remove cycle.

But runtime_pm is currently optional (system wide) and that's not something
I think we are in a position to change.  We should support runtime pm in
as many drivers as possible but not rely on it for 'correct functionality'.
To me turning a device off at remove that we turned on at probe is something
we should do. Now if it's already off, then sure don't turn it off again if
it has side effects to do so (which is the heart of the underflow warning issue
with regulators)

> 
> Doing the former in the existing drivers is not an argument to me because all
> of them avoiding use of the PM APIs. Note, with PM APIs it may also involve
> devlink and other benefits that device driver core gives us.

We have lots of drivers that do both minimal management at probe and remove
as necessary to function, and if it is enabled, full runtime pm.

Though recently it has shown up that there are some underflow issues if we
aren't careful wrt to regulators being handled by devm.

> 
> > > > ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> > > > Is the paired operation with the second disable you add in remove.
> > > > Wrap that in a devm callback.
> > > > 
> > > > More complex is the interrupt enable as that doesn't pair with
> > > > anything in particular in probe. I'm curious though, do we need
> > > > to disable it given the next operation turns off the sensor and on
> > > > probe we reset the sensor.
> > > > 
> > > > Is just clearing the enable bit enough?     
> 

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Andy Shevchenko]

On Sat, Aug 9, 2025 at 9:57 PM Jonathan Cameron <jic23@kernel.org> wrote:
> On Sat, 9 Aug 2025 12:53:40 +0300
> Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > On Thu, Aug 07, 2025 at 02:04:01PM +0100, Jonathan Cameron wrote:
> > > On Wed, 6 Aug 2025 23:02:44 +0300
> > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > > On Wed, Aug 06, 2025 at 04:18:01PM +0100, Jonathan Cameron wrote:
> > > > > On Tue, 5 Aug 2025 14:47:32 +0200
> > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > > > > > > On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> > > > > > > <andy.shevchenko@gmail.com> wrote:

...

> > > > > > > > Doesn't sound right to me. HAve you investigated PM runtime paths?
> > > > > > > Yes I did investigate and found that PM runtime->suspend() callback
> > > > > > > co-exists with remove callback.
> > > > > > >
> > > > > > > > Looking at what the code you added there it sounds to me like a part
> > > > > > > > of PM runtime ->suspend() callback.
> > > > > > > Yes, part of functionality will always be common, because both the
> > > > > > > callback implementations put
> > > > > > > the device into powered down or low power state which is what has been done here
> > > > > > > Both _suspend() and remove are called at different times in the lifecycle of the
> > > > > > > driver, but with respect to register setting, they put the device into
> > > > > > > power down state.
> > > > > >
> > > > > > Are you sure about the remove stage and how it interacts with runtime
> > > > > > PM? Please, check how the device driver core manages PM on the remove
> > > > > > stage.
> > > > > >
> > > > > > > Additionally .remove() can have code for:
> > > > > > > 1. disable runtime power management (if enabled while device registration).
> > > > > >
> > > > > > If the device core enables it for you, it will disable it
> > > > > > symmetrically. I don't see the issue here, it should be done
> > > > > > automatically. If you do that explicitly, use the respective
> > > > > > devm_pm_runtime_*() call.
> > > > > >
> > > > > > > 2. device cleanup (disabling interrupt and cleaning up other configs done).
> > > > > >
> > > > > > Wrap them into devm if required.
> > > > > >
> > > > > > > 2. unregister the device.
> > > > > >
> > > > > > Already done in the original code which your patch reverts, why?
> > > > > >
> > > > > > > For eg: another light sensor bh1750
> > > > > > > static void bh1750_remove(struct i2c_client *client)
> > > > > > > {
> > > > > > >     iio_device_unregister(indio_dev);
> > > > > > >     mutex_lock(&data->lock);
> > > > > > >     i2c_smbus_write_byte(client, BH1750_POWER_DOWN);
> > > > > > >     mutex_unlock(&data->lock);
> > > > > > > }
> > > > > > >
> > > > > > > static int bh1750_suspend(struct device *dev)
> > > > > > > {
> > > > > > >     mutex_lock(&data->lock);
> > > > > > >     ret = i2c_smbus_write_byte(data->client, BH1750_POWER_DOWN);
> > > > > > >     mutex_unlock(&data->lock);
> > > > > > >     return ret;
> > > > > > > }
> > > > > >
> > > > > > Correct and where do you see the problem here? Perhaps the problem is
> > > > > > in the cleanup aordering and some other bugs vs. devm calls?
> > > > > >
> > > > > > > In drivers/iio/light, you can find similar examples in pa12203001,
> > > > > > > rpr0521, apds9960,
> > > > > > > vcnl4000, isl29028, vcnl4035. You can find many more examples in
> > > > > > > sensors other than light sensors.
> > > > > >
> > > > > > Good, should all they need to be fixed?
> > > > >
> > > > > The complex corners that occur with devm + runtime pm are around
> > > > > things that we must not run if we are already in runtime suspend.
> > > > > Typically disabling power supplies (as we can underflow counters
> > > > > and getting warning prints).  Seeing as this driver is not
> > > > > doing that it should be simple to use a devm_add_action_or_reset()
> > > > >
> > > > > Key thing to consider is that runtime pm may not be built.
> > > >
> > > > This will mean that user does not want to handle PM at all at runtime, so why
> > > > should it be our problem? If device is off, it's not the problem of the driver
> > > > to do the power cycle at run time (yes, this might not apply to the system
> > > > suspend and resume cases, which has to be implemented as well).
> > > >
> > > > > So the flow should work with those calls doing nothing.  That means that
> > > > > if you turn the device on in probe we should make sure to explicitly turn
> > > > > it off in the remove flow. That's where devm_add_action_or_reset()
> > > > > comes in handy.
> > > >
> > > > I don't think we should do that explicitly in remove. As I pointed out above,
> > > > this the case that driver should not override.  Otherwise there is no point in
> > > > having the common runtime PM. User deliberately makes it not compiled, so they
> > > > should prepare to leave with it.
> > >
> > > Hmm. I don't agree. We turned it on so on error or remove I think we
> > > should turn it off again.  We do that in many drivers that never made use of
> > > any of the standard PM stuff because they only touch enable and disable in
> > > probe and remove.  If nothing else I don't like the lack of balance between
> > > probe and remove if we don't do it.
> >
> > We can do it, but this sounds to me like a step back. Implementing proper PM
> > runtime callbacks is a step forward.
> I entirely agree that runtime PM is good to have and it does a lot more
> than just turning the power on and off once per probe / remove cycle.

> But runtime_pm is currently optional (system wide) and that's not something
> I think we are in a position to change.

True.

>  We should support runtime pm in
> as many drivers as possible but not rely on it for 'correct functionality'.

Why not? If the driver doesn't care about the PM, then why bother at
all. The PM runtime is for that.

> To me turning a device off at remove that

> we turned

Key words! And that can be done in different ways. One of which is the
PM runtime. The custom on and off is basically an open coded part or
the runtime PM functionality.

> on at probe is something
> we should do.

Yes, of course. I do not object to this flow.

> Now if it's already off, then sure don't turn it off again if
> it has side effects to do so (which is the heart of the underflow warning issue
> with regulators)

Sure.

> > Doing the former in the existing drivers is not an argument to me because all
> > of them avoiding use of the PM APIs. Note, with PM APIs it may also involve
> > devlink and other benefits that device driver core gives us.
>
> We have lots of drivers that do both minimal management at probe and remove
> as necessary to function, and if it is enabled, full runtime pm.
>
> Though recently it has shown up that there are some underflow issues if we
> aren't careful wrt to regulators being handled by devm.
>
> > > > > ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SENSOR_ENABLE);
> > > > > Is the paired operation with the second disable you add in remove.
> > > > > Wrap that in a devm callback.
> > > > >
> > > > > More complex is the interrupt enable as that doesn't pair with
> > > > > anything in particular in probe. I'm curious though, do we need
> > > > > to disable it given the next operation turns off the sensor and on
> > > > > probe we reset the sensor.
> > > > >
> > > > > Is just clearing the enable bit enough?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Akshay Jindal]

On Sun, Aug 10, 2025 at 2:04 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Sat, Aug 9, 2025 at 9:57 PM Jonathan Cameron <jic23@kernel.org> wrote:
> > On Sat, 9 Aug 2025 12:53:40 +0300
> > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > On Thu, Aug 07, 2025 at 02:04:01PM +0100, Jonathan Cameron wrote:
> > > > On Wed, 6 Aug 2025 23:02:44 +0300
> > > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:
> > > > > On Wed, Aug 06, 2025 at 04:18:01PM +0100, Jonathan Cameron wrote:
> > > > > > On Tue, 5 Aug 2025 14:47:32 +0200
> > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > > > > > > On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:
> > > > > > > > On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> > > > > > > > <andy.shevchenko@gmail.com> wrote:
>
> ...
>
> > > We can do it, but this sounds to me like a step back. Implementing proper PM
> > > runtime callbacks is a step forward.
> > I entirely agree that runtime PM is good to have and it does a lot more
> > than just turning the power on and off once per probe / remove cycle.

Initially, while working on a patch for this driver(sysfs for data
freshness), while testing
I needed to suspend the sensor but could not because the driver only supports
system suspend and resume. At that time, I had made up my mind that I
have to add
runtime suspend support for this driver because before Andy's
comments, I used to consider
runtime PM support as a way to give control to users to do on-demand
suspension and
resuming sensor operations. But now I learnt that it is so much more.

So Irrespective of the acceptance of this patch, my next patch was
going to be runtime PM support.

Will it be acceptable, that this driver like many other drivers have
support for both remove callback
and runtime PM?

Thanks,
Akshay.

Re: [PATCH] iio: light: ltr390: Add remove callback with needed support in device registration

[Jonathan Cameron]

On Mon, 11 Aug 2025 02:18:34 +0530
Akshay Jindal <akshayaj.lkd@gmail.com> wrote:

> On Sun, Aug 10, 2025 at 2:04 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Sat, Aug 9, 2025 at 9:57 PM Jonathan Cameron <jic23@kernel.org> wrote:  
> > > On Sat, 9 Aug 2025 12:53:40 +0300
> > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> > > > On Thu, Aug 07, 2025 at 02:04:01PM +0100, Jonathan Cameron wrote:  
> > > > > On Wed, 6 Aug 2025 23:02:44 +0300
> > > > > Andy Shevchenko <andriy.shevchenko@intel.com> wrote:  
> > > > > > On Wed, Aug 06, 2025 at 04:18:01PM +0100, Jonathan Cameron wrote:  
> > > > > > > On Tue, 5 Aug 2025 14:47:32 +0200
> > > > > > > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:  
> > > > > > > > On Tue, Aug 5, 2025 at 6:05 AM Akshay Jindal <akshayaj.lkd@gmail.com> wrote:  
> > > > > > > > > On Tue, Aug 5, 2025 at 2:36 AM Andy Shevchenko
> > > > > > > > > <andy.shevchenko@gmail.com> wrote:  
> >
> > ...
> >  
> > > > We can do it, but this sounds to me like a step back. Implementing proper PM
> > > > runtime callbacks is a step forward.  
> > > I entirely agree that runtime PM is good to have and it does a lot more
> > > than just turning the power on and off once per probe / remove cycle.  
> 
> Initially, while working on a patch for this driver(sysfs for data
> freshness), while testing
> I needed to suspend the sensor but could not because the driver only supports
> system suspend and resume. At that time, I had made up my mind that I
> have to add
> runtime suspend support for this driver because before Andy's
> comments, I used to consider
> runtime PM support as a way to give control to users to do on-demand
> suspension and
> resuming sensor operations. But now I learnt that it is so much more.
> 
> So Irrespective of the acceptance of this patch, my next patch was
> going to be runtime PM support.
> 
> Will it be acceptable, that this driver like many other drivers have
> support for both remove callback
> and runtime PM?
Sounds good. Though you probably don't need an explicit remove callback,
just one more devm_add_action_or_reset().

That stuff just allows you to register the 'undo' immediately after the
'do' in probe which makes for easy to read code in many cases.

Jonathan

> 
> Thanks,
> Akshay.
>