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.1On 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);
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
© 2016 - 2026 Red Hat, Inc.