[PATCH next 0/4] IIO: sca3000: devm resource management

[PATCH next 0/4] IIO: sca3000: devm resource management

[Harshit Mogalapalli]

Hi,

This is an attempt to update sca3000 accelerometer driver to make use
of devm_ based helpers where needed. I have split it into 4 patches.

Patch 1 does some refactoring and simplification.
Patch 2 switches request_threaded_irq() over to the devm helper.
Patch 3 converts iio_device_register() to its managed counterpart.
Patch 4 replaces the remove() callback with devm_add_action_or_reset()
        while preserving the existing iio_device_unregister() ->
        disable interrupts sequence. I would welcome an extra look at
	this patch.

The series builds cleanly and I have performed static analysis with
smatch checker and haven't tested on actual hardware.

Thanks for your time.

Regards,
Harshit

Harshit Mogalapalli (4):
  iio: sca3000: cache SPI device ID in probe
  iio: sca3000: switch IRQ handling to devm helpers
  iio: sca3000: manage device registration with devm helper
  iio: sca3000: stop interrupts via devm_add_action_or_reset()

 drivers/iio/accel/sca3000.c | 92 +++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 50 deletions(-)

-- 
2.50.1

[PATCH next 1/4] iio: sca3000: cache SPI device ID in probe

[Harshit Mogalapalli]

Store the spi_device_id at probe entry and reuse it for the name and
chip info instead of calling spi_get_device_id() repeatedly.

Reuse the local dev pointer for resource managed helpers. While at it,
reshuffle variable list in a reverse Christmas tree order.

No functional change intended.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 drivers/iio/accel/sca3000.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 9ef4d6e27466..afe6ef61a53b 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -1439,11 +1439,13 @@ static const struct iio_info sca3000_info = {
 
 static int sca3000_probe(struct spi_device *spi)
 {
-	int ret;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	struct device *dev = &spi->dev;
 	struct sca3000_state *st;
 	struct iio_dev *indio_dev;
+	int ret;
 
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -1451,10 +1453,9 @@ static int sca3000_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, indio_dev);
 	st->us = spi;
 	mutex_init(&st->lock);
-	st->info = &sca3000_spi_chip_info_tbl[spi_get_device_id(spi)
-					      ->driver_data];
+	st->info = &sca3000_spi_chip_info_tbl[id->driver_data];
 
-	indio_dev->name = spi_get_device_id(spi)->name;
+	indio_dev->name = id->name;
 	indio_dev->info = &sca3000_info;
 	if (st->info->temp_output) {
 		indio_dev->channels = sca3000_channels_with_temp;
@@ -1466,7 +1467,7 @@ static int sca3000_probe(struct spi_device *spi)
 	}
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
+	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
 					  &sca3000_ring_setup_ops);
 	if (ret)
 		return ret;
-- 
2.50.1

Re: [PATCH next 1/4] iio: sca3000: cache SPI device ID in probe

[Jonathan Cameron]

On Fri, 30 Jan 2026 13:43:08 -0800
Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote:

> Store the spi_device_id at probe entry and reuse it for the name and
> chip info instead of calling spi_get_device_id() repeatedly.
> 
> Reuse the local dev pointer for resource managed helpers. While at it,
> reshuffle variable list in a reverse Christmas tree order.
> 
> No functional change intended.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Hi Harshit,

A few comments inline but they are all about extra things to do rather
than really being comments on this patch which is fine as a starting point.

> ---
>  drivers/iio/accel/sca3000.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index 9ef4d6e27466..afe6ef61a53b 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -1439,11 +1439,13 @@ static const struct iio_info sca3000_info = {
>  
>  static int sca3000_probe(struct spi_device *spi)
>  {
> -	int ret;
> +	const struct spi_device_id *id = spi_get_device_id(spi);

This is probably ok as an intermediate step, but ultimately we
shouldn't be using the spi_device_id at all.

id->driver_data should be a pointer to a named (not in array)
sca3000_chip_info structure and so should the data in the of_match_id
table etc.

Then we use spi_get_device_match_data() which prefers the of_device_id
table entry (which would needed adding).


> +	struct device *dev = &spi->dev;
>  	struct sca3000_state *st;
>  	struct iio_dev *indio_dev;
> +	int ret;
>  
> -	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
>  	if (!indio_dev)
>  		return -ENOMEM;
>  
> @@ -1451,10 +1453,9 @@ static int sca3000_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, indio_dev);
>  	st->us = spi;
>  	mutex_init(&st->lock);
> -	st->info = &sca3000_spi_chip_info_tbl[spi_get_device_id(spi)
> -					      ->driver_data];  
> +	st->info = &sca3000_spi_chip_info_tbl[id->driver_data];
>  
> -	indio_dev->name = spi_get_device_id(spi)->name;
> +	indio_dev->name = id->name;

This also needs an update to make it get the name from within the
chip_info structure.  Whilst this works today it can be fragile
if we have dt compatibles that don't match exactly with the spi_device_id
table entries.

>  	indio_dev->info = &sca3000_info;
>  	if (st->info->temp_output) {
>  		indio_dev->channels = sca3000_channels_with_temp;
> @@ -1466,7 +1467,7 @@ static int sca3000_probe(struct spi_device *spi)
>  	}
>  	indio_dev->modes = INDIO_DIRECT_MODE;
>  
> -	ret = devm_iio_kfifo_buffer_setup(&spi->dev, indio_dev,
> +	ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
>  					  &sca3000_ring_setup_ops);
>  	if (ret)
>  		return ret;

Re: [PATCH next 1/4] iio: sca3000: cache SPI device ID in probe

[Harshit Mogalapalli]

Hi Jonathan,


> Hi Harshit,
> 
> A few comments inline but they are all about extra things to do rather
> than really being comments on this patch which is fine as a starting point.
> 
....
>>   static int sca3000_probe(struct spi_device *spi)
>>   {
>> -	int ret;
>> +	const struct spi_device_id *id = spi_get_device_id(spi);
> 
> This is probably ok as an intermediate step, but ultimately we
> shouldn't be using the spi_device_id at all.
> 
> id->driver_data should be a pointer to a named (not in array)
> sca3000_chip_info structure and so should the data in the of_match_id
> table etc.
> 
> Then we use spi_get_device_match_data() which prefers the of_device_id
> table entry (which would needed adding).
> 
> 
...
>> -	st->info = &sca3000_spi_chip_info_tbl[spi_get_device_id(spi)
>> -					      ->driver_data];
>> +	st->info = &sca3000_spi_chip_info_tbl[id->driver_data];
>>   
>> -	indio_dev->name = spi_get_device_id(spi)->name;
>> +	indio_dev->name = id->name;
> 
> This also needs an update to make it get the name from within the
> chip_info structure.  Whilst this works today it can be fragile
> if we have dt compatibles that don't match exactly with the spi_device_id
> table entries.
> 

Thanks a lot for the review, will try to make this change on top of 
these refactors.

Regards,
Harshit

[PATCH next 2/4] iio: sca3000: switch IRQ handling to devm helpers

[Harshit Mogalapalli]

Convert the threaded IRQ registration to devm_request_threaded_irq() so
that the probe and remove paths can drop manual freeing of irqs.

No functionality change.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 drivers/iio/accel/sca3000.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index afe6ef61a53b..0210c716cf38 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -1473,8 +1473,7 @@ static int sca3000_probe(struct spi_device *spi)
 		return ret;
 
 	if (spi->irq) {
-		ret = request_threaded_irq(spi->irq,
-					   NULL,
+		ret = devm_request_threaded_irq(dev, spi->irq, NULL,
 					   &sca3000_event_handler,
 					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 					   "sca3000",
@@ -1484,23 +1483,17 @@ static int sca3000_probe(struct spi_device *spi)
 	}
 	ret = sca3000_clean_setup(st);
 	if (ret)
-		goto error_free_irq;
+		return ret;
 
 	ret = sca3000_print_rev(indio_dev);
 	if (ret)
-		goto error_free_irq;
+		return ret;
 
 	ret = iio_device_register(indio_dev);
 	if (ret)
-		goto error_free_irq;
+		return ret;
 
 	return 0;
-
-error_free_irq:
-	if (spi->irq)
-		free_irq(spi->irq, indio_dev);
-
-	return ret;
 }
 
 static int sca3000_stop_all_interrupts(struct sca3000_state *st)
@@ -1530,8 +1523,6 @@ static void sca3000_remove(struct spi_device *spi)
 
 	/* Must ensure no interrupts can be generated after this! */
 	sca3000_stop_all_interrupts(st);
-	if (spi->irq)
-		free_irq(spi->irq, indio_dev);
 }
 
 static const struct spi_device_id sca3000_id[] = {
-- 
2.50.1

Re: [PATCH next 2/4] iio: sca3000: switch IRQ handling to devm helpers

[Jonathan Cameron]

On Fri, 30 Jan 2026 13:43:09 -0800
Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote:

> Convert the threaded IRQ registration to devm_request_threaded_irq() so
> that the probe and remove paths can drop manual freeing of irqs.
> 
> No functionality change.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
Hi Harshit,

Minor formatting update needed. See inline.

Thanks,

Jonathan

> ---
>  drivers/iio/accel/sca3000.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index afe6ef61a53b..0210c716cf38 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -1473,8 +1473,7 @@ static int sca3000_probe(struct spi_device *spi)
>  		return ret;
>  
>  	if (spi->irq) {
> -		ret = request_threaded_irq(spi->irq,
> -					   NULL,
> +		ret = devm_request_threaded_irq(dev, spi->irq, NULL,
>  					   &sca3000_event_handler,
Update the indent to keep these after the (
That is:
		ret = devm_request_threaded_irq(dev, spi->irq, NULL,
 						&sca3000_event_handler,
						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
						"sca3000",
etc



>  					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  					   "sca3000",
> @@ -1484,23 +1483,17 @@ static int sca3000_probe(struct spi_device *spi)
>  	}
>  	ret = sca3000_clean_setup(st);
>  	if (ret)
> -		goto error_free_irq;
> +		return ret;
>  
>  	ret = sca3000_print_rev(indio_dev);
>  	if (ret)
> -		goto error_free_irq;
> +		return ret;
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
> -		goto error_free_irq;
> +		return ret;
>  
>  	return 0;
> -
> -error_free_irq:
> -	if (spi->irq)
> -		free_irq(spi->irq, indio_dev);
> -
> -	return ret;
>  }
>  
>  static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> @@ -1530,8 +1523,6 @@ static void sca3000_remove(struct spi_device *spi)
>  
>  	/* Must ensure no interrupts can be generated after this! */
>  	sca3000_stop_all_interrupts(st);
> -	if (spi->irq)
> -		free_irq(spi->irq, indio_dev);
>  }
>  
>  static const struct spi_device_id sca3000_id[] = {

Re: [PATCH next 2/4] iio: sca3000: switch IRQ handling to devm helpers

[Harshit Mogalapalli]

Hi Jonathan,


On 31/01/26 22:00, Jonathan Cameron wrote:
>>   
>>   	if (spi->irq) {
>> -		ret = request_threaded_irq(spi->irq,
>> -					   NULL,
>> +		ret = devm_request_threaded_irq(dev, spi->irq, NULL,
>>   					   &sca3000_event_handler,
> Update the indent to keep these after the (
> That is:
> 		ret = devm_request_threaded_irq(dev, spi->irq, NULL,
>   						&sca3000_event_handler,
> 						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> 						"sca3000",
> etc
> 

Oh, right, I should have indented it properly, sorry about that. I 
thought checkpatch was catching these. Will fix this in a next version. 
Thanks for spotting this.

Regards,
Harshit

Re: [PATCH next 2/4] iio: sca3000: switch IRQ handling to devm helpers

[David Lechner]

On 1/30/26 3:43 PM, Harshit Mogalapalli wrote:
> Convert the threaded IRQ registration to devm_request_threaded_irq() so
> that the probe and remove paths can drop manual freeing of irqs.
> 
> No functionality change.
> 


...

> @@ -1484,23 +1483,17 @@ static int sca3000_probe(struct spi_device *spi)
>  	}
>  	ret = sca3000_clean_setup(st);
>  	if (ret)
> -		goto error_free_irq;
> +		return ret;
>  
>  	ret = sca3000_print_rev(indio_dev);
>  	if (ret)
> -		goto error_free_irq;
> +		return ret;
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
> -		goto error_free_irq;
> +		return ret;
>  
>  	return 0;
This can `return iio_device_register();` directly now.

Re: [PATCH next 2/4] iio: sca3000: switch IRQ handling to devm helpers

[Harshit Mogalapalli]

Hi David,



On 01/02/26 00:51, David Lechner wrote:
> On 1/30/26 3:43 PM, Harshit Mogalapalli wrote:
>> Convert the threaded IRQ registration to devm_request_threaded_irq() so
>> that the probe and remove paths can drop manual freeing of irqs.
>>
>> No functionality change.
>>
> 
> 
> ...
> 
>> @@ -1484,23 +1483,17 @@ static int sca3000_probe(struct spi_device *spi)
>>   	}
>>   	ret = sca3000_clean_setup(st);
>>   	if (ret)
>> -		goto error_free_irq;
>> +		return ret;
>>   
>>   	ret = sca3000_print_rev(indio_dev);
>>   	if (ret)
>> -		goto error_free_irq;
>> +		return ret;
>>   
>>   	ret = iio_device_register(indio_dev);
>>   	if (ret)
>> -		goto error_free_irq;
>> +		return ret;
>>   
>>   	return 0;
> This can `return iio_device_register();` directly now.

Thanks for the review and comments! I agree, it could have been simplified.

I am working on a v2 now. Will address it, in v2 basically the 
iio_device_register() --> devm_iio_device_register() conversion is done 
in last patch so we don't have this problem, so it kind of gets 
auto-resolved in v2.

Thanks,
Harshit

[PATCH next 3/4] iio: sca3000: manage device registration with devm helper

[Harshit Mogalapalli]

Convert the iio registration to use devm_* helpers so the probe no
longer needs a separate cleanup path and remove callback can also drop
the unregister.

No functional change intended.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 drivers/iio/accel/sca3000.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index 0210c716cf38..bf1957c7bc77 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -1489,11 +1489,7 @@ static int sca3000_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	ret = iio_device_register(indio_dev);
-	if (ret)
-		return ret;
-
-	return 0;
+	return devm_iio_device_register(dev, indio_dev);
 }
 
 static int sca3000_stop_all_interrupts(struct sca3000_state *st)
@@ -1519,8 +1515,6 @@ static void sca3000_remove(struct spi_device *spi)
 	struct iio_dev *indio_dev = spi_get_drvdata(spi);
 	struct sca3000_state *st = iio_priv(indio_dev);
 
-	iio_device_unregister(indio_dev);
-
 	/* Must ensure no interrupts can be generated after this! */
 	sca3000_stop_all_interrupts(st);
 }
-- 
2.50.1

Re: [PATCH next 3/4] iio: sca3000: manage device registration with devm helper

[Jonathan Cameron]

On Fri, 30 Jan 2026 13:43:10 -0800
Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote:

> Convert the iio registration to use devm_* helpers so the probe no
> longer needs a separate cleanup path and remove callback can also drop
> the unregister.
> 
> No functional change intended.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>

This results in the order of two bits of tear down being reversed.
It might be your intent but if so you need a clear description of
why it is fine to do this and why it is not a functional change.

Jonathan

> ---
>  drivers/iio/accel/sca3000.c | 8 +-------
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index 0210c716cf38..bf1957c7bc77 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -1489,11 +1489,7 @@ static int sca3000_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	ret = iio_device_register(indio_dev);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return devm_iio_device_register(dev, indio_dev);
>  }
>  
>  static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> @@ -1519,8 +1515,6 @@ static void sca3000_remove(struct spi_device *spi)
>  	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>  	struct sca3000_state *st = iio_priv(indio_dev);
>  
> -	iio_device_unregister(indio_dev);
> -
>  	/* Must ensure no interrupts can be generated after this! */
>  	sca3000_stop_all_interrupts(st);

This changes the ordering.  Now we stop interrupts before removing the
userspace interfaces.  You probably need to reorder this and the next
patch so there is no intermediate broken point.

>  }

[PATCH next 4/4] iio: sca3000: stop interrupts via devm_add_action_or_reset()

[Harshit Mogalapalli]

sca3000_stop_all_interrupts() is moved above the probe routine so the
new function sca3000_disable_interrupts() used in probe can directly
call it without additional declaration.

Used devm_add_action_or_reset() for shutting doing the interrupts. After
this there is no need to have a remove call back, so got rid of the
remove callback.

No functional change intended.

Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
---
 drivers/iio/accel/sca3000.c | 58 ++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 26 deletions(-)

diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
index bf1957c7bc77..7f7d29688a81 100644
--- a/drivers/iio/accel/sca3000.c
+++ b/drivers/iio/accel/sca3000.c
@@ -1437,6 +1437,34 @@ static const struct iio_info sca3000_info = {
 	.write_event_config = &sca3000_write_event_config,
 };
 
+static int sca3000_stop_all_interrupts(struct sca3000_state *st)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+	if (ret)
+		goto error_ret;
+	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
+				(st->rx[0] &
+				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
+				   SCA3000_REG_INT_MASK_RING_HALF |
+				   SCA3000_REG_INT_MASK_ALL_INTS)));
+error_ret:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static void sca3000_disable_interrupts(void *data)
+{
+	struct iio_dev *indio_dev = data;
+	struct sca3000_state *st = iio_priv(indio_dev);
+
+	/* Must ensure no interrupts can be generated after this! */
+	sca3000_stop_all_interrupts(st);
+}
+
+
 static int sca3000_probe(struct spi_device *spi)
 {
 	const struct spi_device_id *id = spi_get_device_id(spi);
@@ -1481,6 +1509,7 @@ static int sca3000_probe(struct spi_device *spi)
 		if (ret)
 			return ret;
 	}
+
 	ret = sca3000_clean_setup(st);
 	if (ret)
 		return ret;
@@ -1489,34 +1518,12 @@ static int sca3000_probe(struct spi_device *spi)
 	if (ret)
 		return ret;
 
-	return devm_iio_device_register(dev, indio_dev);
-}
-
-static int sca3000_stop_all_interrupts(struct sca3000_state *st)
-{
-	int ret;
-
-	mutex_lock(&st->lock);
-	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)
-		goto error_ret;
-	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
-				(st->rx[0] &
-				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
-				   SCA3000_REG_INT_MASK_RING_HALF |
-				   SCA3000_REG_INT_MASK_ALL_INTS)));
-error_ret:
-	mutex_unlock(&st->lock);
-	return ret;
-}
+		return ret;
 
-static void sca3000_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct sca3000_state *st = iio_priv(indio_dev);
+	return devm_add_action_or_reset(dev, sca3000_disable_interrupts, indio_dev);
 
-	/* Must ensure no interrupts can be generated after this! */
-	sca3000_stop_all_interrupts(st);
 }
 
 static const struct spi_device_id sca3000_id[] = {
@@ -1533,7 +1540,6 @@ static struct spi_driver sca3000_driver = {
 		.name = "sca3000",
 	},
 	.probe = sca3000_probe,
-	.remove = sca3000_remove,
 	.id_table = sca3000_id,
 };
 module_spi_driver(sca3000_driver);
-- 
2.50.1

Re: [PATCH next 4/4] iio: sca3000: stop interrupts via devm_add_action_or_reset()

[Jonathan Cameron]

On Fri, 30 Jan 2026 13:43:11 -0800
Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote:

> sca3000_stop_all_interrupts() is moved above the probe routine so the
> new function sca3000_disable_interrupts() used in probe can directly
> call it without additional declaration.
> 
> Used devm_add_action_or_reset() for shutting doing the interrupts. After
> this there is no need to have a remove call back, so got rid of the
> remove callback.
> 
> No functional change intended.
> 
> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>

Will need an update if you indeed didn't intend to change order in previous
patch.  devm_iio_device_register() is last in vast majority of probe
functions because it opens us up to userspace interaction. We almost
always want to cut that off on remove before doing anything else.

> ---
>  drivers/iio/accel/sca3000.c | 58 ++++++++++++++++++++-----------------
>  1 file changed, 32 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
> index bf1957c7bc77..7f7d29688a81 100644
> --- a/drivers/iio/accel/sca3000.c
> +++ b/drivers/iio/accel/sca3000.c
> @@ -1437,6 +1437,34 @@ static const struct iio_info sca3000_info = {
>  	.write_event_config = &sca3000_write_event_config,
>  };
>  
> +static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);

Use guard(mutex)(&st->lock); to simplify this. Remember to include cleanup.h
A future patch could then make use of guard() more widely in this driver.

> +	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> +	if (ret)
> +		goto error_ret;

Blank line here.  Keeps each group of action / error check clearly delineated
if we have a blank line between them. Note this is a case of do as I say
not as I did nearly 17 years back when I wrote this (younger me did many
things wrong ;)

With guard() above, you can just do
	if (ret)
		return ret;

here.
> +	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> +				(st->rx[0] &
> +				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> +				   SCA3000_REG_INT_MASK_RING_HALF |
> +				   SCA3000_REG_INT_MASK_ALL_INTS)));

With guard() above, this becomes
	return sca3000_write_reg();


> +error_ret:
> +	mutex_unlock(&st->lock);
> +	return ret;
> +}
> +
> +static void sca3000_disable_interrupts(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +	struct sca3000_state *st = iio_priv(indio_dev);
> +
> +	/* Must ensure no interrupts can be generated after this! */
> +	sca3000_stop_all_interrupts(st);
> +}
> +
> +
>  static int sca3000_probe(struct spi_device *spi)
>  {
>  	const struct spi_device_id *id = spi_get_device_id(spi);
> @@ -1481,6 +1509,7 @@ static int sca3000_probe(struct spi_device *spi)
>  		if (ret)
>  			return ret;
>  	}
> +

Unrelated change.  Make sure to check for these in patches before
you send them out.  It adds noise and typically means you'll end
up doing another version even if everything else is good.

>  	ret = sca3000_clean_setup(st);
>  	if (ret)
>  		return ret;
> @@ -1489,34 +1518,12 @@ static int sca3000_probe(struct spi_device *spi)
>  	if (ret)
>  		return ret;
>  
> -	return devm_iio_device_register(dev, indio_dev);
> -}
> -
> -static int sca3000_stop_all_interrupts(struct sca3000_state *st)
> -{
> -	int ret;
> -
> -	mutex_lock(&st->lock);
> -	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
> +	ret = devm_iio_device_register(dev, indio_dev);
>  	if (ret)
> -		goto error_ret;
> -	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
> -				(st->rx[0] &
> -				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
> -				   SCA3000_REG_INT_MASK_RING_HALF |
> -				   SCA3000_REG_INT_MASK_ALL_INTS)));
> -error_ret:
> -	mutex_unlock(&st->lock);
> -	return ret;
> -}
> +		return ret;
>  
> -static void sca3000_remove(struct spi_device *spi)
> -{
> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> -	struct sca3000_state *st = iio_priv(indio_dev);
> +	return devm_add_action_or_reset(dev, sca3000_disable_interrupts, indio_dev);
As mentioned above. This looks unlikely to be a good idea as a reorder of code.

I'd expect interfaces to go away and then interrupts to be stopped. In general
that should always be safe unless we have some racey bug somewhere in the IIO
core or the driver is doing something unusual.

Thanks,

Jonathan

>  
> -	/* Must ensure no interrupts can be generated after this! */
> -	sca3000_stop_all_interrupts(st);
>  }
>  
>  static const struct spi_device_id sca3000_id[] = {
> @@ -1533,7 +1540,6 @@ static struct spi_driver sca3000_driver = {
>  		.name = "sca3000",
>  	},
>  	.probe = sca3000_probe,
> -	.remove = sca3000_remove,
>  	.id_table = sca3000_id,
>  };
>  module_spi_driver(sca3000_driver);

Re: [PATCH next 4/4] iio: sca3000: stop interrupts via devm_add_action_or_reset()

[Harshit Mogalapalli]

Hi Jonathan,


On 31/01/26 22:09, Jonathan Cameron wrote:
> On Fri, 30 Jan 2026 13:43:11 -0800
> Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com> wrote:
> 
>> sca3000_stop_all_interrupts() is moved above the probe routine so the
>> new function sca3000_disable_interrupts() used in probe can directly
>> call it without additional declaration.
>>
>> Used devm_add_action_or_reset() for shutting doing the interrupts. After
>> this there is no need to have a remove call back, so got rid of the
>> remove callback.
>>
>> No functional change intended.
>>
>> Suggested-by: Andy Shevchenko <andriy.shevchenko@intel.com>
>> Signed-off-by: Harshit Mogalapalli <harshit.m.mogalapalli@oracle.com>
> 
> Will need an update if you indeed didn't intend to change order in previous
> patch.  

Yes, I didn't intend that change in Patch 3.

> devm_iio_device_register() is last in vast majority of probe
> functions because it opens us up to userspace interaction. We almost
> always want to cut that off on remove before doing anything else.
> 

Sure, thanks a lot!

I have checked in other drivers while thinking about it, but my code 
base changed due to incorrect ordering in PATCH 3 :(

Maybe the best idea is to do squash PATCH3 and PATCH 4 ?

>> ---
>>   drivers/iio/accel/sca3000.c | 58 ++++++++++++++++++++-----------------
>>   1 file changed, 32 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/iio/accel/sca3000.c b/drivers/iio/accel/sca3000.c
>> index bf1957c7bc77..7f7d29688a81 100644
>> --- a/drivers/iio/accel/sca3000.c
>> +++ b/drivers/iio/accel/sca3000.c
>> @@ -1437,6 +1437,34 @@ static const struct iio_info sca3000_info = {
>>   	.write_event_config = &sca3000_write_event_config,
>>   };
>>   
>> +static int sca3000_stop_all_interrupts(struct sca3000_state *st)
>> +{
>> +	int ret;
>> +
>> +	mutex_lock(&st->lock);
> 
> Use guard(mutex)(&st->lock); to simplify this. Remember to include cleanup.h
> A future patch could then make use of guard() more widely in this driver.
> 
>> +	ret = sca3000_read_data_short(st, SCA3000_REG_INT_MASK_ADDR, 1);
>> +	if (ret)
>> +		goto error_ret;
> 
> Blank line here.  Keeps each group of action / error check clearly delineated
> if we have a blank line between them. Note this is a case of do as I say
> not as I did nearly 17 years back when I wrote this (younger me did many
> things wrong ;)
> 
> With guard() above, you can just do
> 	if (ret)
> 		return ret;
> 
> here.
>> +	ret = sca3000_write_reg(st, SCA3000_REG_INT_MASK_ADDR,
>> +				(st->rx[0] &
>> +				 ~(SCA3000_REG_INT_MASK_RING_THREE_QUARTER |
>> +				   SCA3000_REG_INT_MASK_RING_HALF |
>> +				   SCA3000_REG_INT_MASK_ALL_INTS)));
> 
> With guard() above, this becomes
> 	return sca3000_write_reg();
> 
> 
>> +error_ret:
>> +	mutex_unlock(&st->lock);
>> +	return ret;
>> +}
>> +


Sure will simplify this function with scoped guards. Thanks for the 
suggestion.

>> +static void sca3000_disable_interrupts(void *data)
>> +{
>> +	struct iio_dev *indio_dev = data;
>> +	struct sca3000_state *st = iio_priv(indio_dev);
>> +
>> +	/* Must ensure no interrupts can be generated after this! */
>> +	sca3000_stop_all_interrupts(st);
>> +}
>> +
>> +
>>   static int sca3000_probe(struct spi_device *spi)
>>   {
>>   	const struct spi_device_id *id = spi_get_device_id(spi);
>> @@ -1481,6 +1509,7 @@ static int sca3000_probe(struct spi_device *spi)
>>   		if (ret)
>>   			return ret;
>>   	}
>> +
> 
> Unrelated change.  Make sure to check for these in patches before
> you send them out.  It adds noise and typically means you'll end
> up doing another version even if everything else is good.
> 

sure, I was actually aware of this new line addition while sending, 
wasn't sure if I had to do it in this or do it in a separate patch. But 
I understood that now to not mix unrelated changes.


>>   	ret = sca3000_clean_setup(st);
>>   	if (ret)
>>   		return ret;
...
>> +		return ret;
>>   
>> -static void sca3000_remove(struct spi_device *spi)
>> -{
>> -	struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> -	struct sca3000_state *st = iio_priv(indio_dev);
>> +	return devm_add_action_or_reset(dev, sca3000_disable_interrupts, indio_dev);
> As mentioned above. This looks unlikely to be a good idea as a reorder of code.
> 
> I'd expect interfaces to go away and then interrupts to be stopped. In general
> that should always be safe unless we have some racey bug somewhere in the IIO
> core or the driver is doing something unusual.
> 

Sure, so iio_device_unregister() is the first thing to happen. Will do a 
v2 trying to address all comments.


Thanks a lot for detailed review and guidance with this! I really
appreciate it.

> Thanks,
> 
> Jonathan
> 

Regards,
Harshit