1. Patchew
  2. linux
  3. View series

[PATCH] iio: resolver: ad2s1210: split trigger handler body into a helper

Stepan Ionichev posted 1 patch 1 week, 1 day ago 
drivers/iio/resolver/ad2s1210.c | 33 ++++++++++++++++++++-------------
1 file changed, 20 insertions(+), 13 deletions(-)
[PATCH] iio: resolver: ad2s1210: split trigger handler body into a helper
Posted by Stepan Ionichev 1 week, 1 day ago 
ad2s1210_trigger_handler() takes st->lock via guard(mutex), then runs
the per-channel read/event/push sequence, and finally calls
iio_trigger_notify_done() before returning IRQ_HANDLED. Because the
guard releases at scope exit, iio_trigger_notify_done() runs with
st->lock still held. The previous "goto error_ret" structure also
hides that the locked region extends all the way to notify_done.

The device does not register any triggers itself, so this is not a
deadlock source today, but holding a device mutex across the trigger
notification path is fragile if a trigger ever gets attached
elsewhere.

Lift the body of the handler into a helper, ad2s1210_collect_and_push(),
that owns the guard and returns 0 / negative errno via direct returns
instead of "goto error_ret". The irq handler then becomes a short
wrapper that calls the helper, calls iio_trigger_notify_done() with no
state lock held, and returns IRQ_HANDLED.

No change in behaviour for the read paths: every error path still
skips the event/buffer push, and iio_trigger_notify_done() is still
called on every IRQ regardless of result. The fault-register read no
longer leaks a negative errno into irqreturn_t either, since the
caller never propagates the helper's return value.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Link: https://lore.kernel.org/lkml/20260516122838.163a77d3@jic23-huawei/
Signed-off-by: Stepan Ionichev <sozdayvek@gmail.com>
---
 drivers/iio/resolver/ad2s1210.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/iio/resolver/ad2s1210.c b/drivers/iio/resolver/ad2s1210.c
index 1be19fe8a..20b0b2b5c 100644
--- a/drivers/iio/resolver/ad2s1210.c
+++ b/drivers/iio/resolver/ad2s1210.c
@@ -1276,10 +1276,8 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
 	return regmap_write(st->regmap, reg, writeval);
 }
 
-static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
+static int ad2s1210_collect_and_push(struct iio_dev *indio_dev, s64 timestamp)
 {
-	struct iio_poll_func *pf = p;
-	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ad2s1210_state *st = iio_priv(indio_dev);
 	size_t chan = 0;
 	int ret;
@@ -1295,15 +1293,15 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
 					       AD2S1210_REG_POSITION_MSB,
 					       &st->sample.raw, 2);
 			if (ret < 0)
-				goto error_ret;
+				return ret;
 		} else {
 			ret = ad2s1210_set_mode(st, MOD_POS);
 			if (ret < 0)
-				goto error_ret;
+				return ret;
 
 			ret = spi_read(st->sdev, &st->sample, 3);
 			if (ret < 0)
-				goto error_ret;
+				return ret;
 		}
 
 		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
@@ -1315,15 +1313,15 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
 					       AD2S1210_REG_VELOCITY_MSB,
 					       &st->sample.raw, 2);
 			if (ret < 0)
-				goto error_ret;
+				return ret;
 		} else {
 			ret = ad2s1210_set_mode(st, MOD_VEL);
 			if (ret < 0)
-				goto error_ret;
+				return ret;
 
 			ret = spi_read(st->sdev, &st->sample, 3);
 			if (ret < 0)
-				goto error_ret;
+				return ret;
 		}
 
 		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
@@ -1334,16 +1332,25 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
 
 		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
 		if (ret < 0)
-			goto error_ret;
+			return ret;
 
 		st->sample.fault = reg_val;
 	}
 
-	ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
+	ad2s1210_push_events(indio_dev, st->sample.fault, timestamp);
 	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
-				    pf->timestamp);
+				    timestamp);
+
+	return 0;
+}
+
+static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+
+	ad2s1210_collect_and_push(indio_dev, pf->timestamp);
 
-error_ret:
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
-- 
2.43.0
Re: [PATCH] iio: resolver: ad2s1210: split trigger handler body into a helper
Posted by Sanjay Chitroda 1 week, 1 day ago 

On 16 May 2026 8:47:20 pm IST, Stepan Ionichev <sozdayvek@gmail.com> wrote:
>ad2s1210_trigger_handler() takes st->lock via guard(mutex), then runs

>--- a/drivers/iio/resolver/ad2s1210.c
>+++ b/drivers/iio/resolver/ad2s1210.c
>@@ -1276,10 +1276,8 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
> 	return regmap_write(st->regmap, reg, writeval);
> }
> 
>-static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
>+static int ad2s1210_collect_and_push(struct iio_dev *indio_dev, s64 timestamp)
Hi Stepan,

Is the return value of this function checked by any caller?

If not, should this helper return void instead?

> {
>-	struct iio_poll_func *pf = p;
>-	struct iio_dev *indio_dev = pf->indio_dev;
> 	struct ad2s1210_state *st = iio_priv(indio_dev);
> 	size_t chan = 0;
> 	int ret;
>@@ -1295,15 +1293,15 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
> 					       AD2S1210_REG_POSITION_MSB,
> 					       &st->sample.raw, 2);
> 			if (ret < 0)
>-				goto error_ret;
>+				return ret;
> 		} else {
> 			ret = ad2s1210_set_mode(st, MOD_POS);
> 			if (ret < 0)
>-				goto error_ret;
>+				return ret;
> 
> 			ret = spi_read(st->sdev, &st->sample, 3);
> 			if (ret < 0)
>-				goto error_ret;
>+				return ret;
> 		}
> 
> 		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
>@@ -1315,15 +1313,15 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
> 					       AD2S1210_REG_VELOCITY_MSB,
> 					       &st->sample.raw, 2);
> 			if (ret < 0)
>-				goto error_ret;
>+				return ret;
> 		} else {
> 			ret = ad2s1210_set_mode(st, MOD_VEL);
> 			if (ret < 0)
>-				goto error_ret;
>+				return ret;
> 
> 			ret = spi_read(st->sdev, &st->sample, 3);
> 			if (ret < 0)
>-				goto error_ret;
>+				return ret;
> 		}
> 
> 		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
>@@ -1334,16 +1332,25 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
> 
> 		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
> 		if (ret < 0)
>-			goto error_ret;
>+			return ret;
> 

I cannot find this goto instance in mainline.

Is this patch based on top of iio/testing or another branch?

For patch dependencies like this, is it preferred to mention the base commit in the changelog for single patches as well or in the commit prefix?


> 		st->sample.fault = reg_val;
> 	}
> 
>-	ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
>+	ad2s1210_push_events(indio_dev, st->sample.fault, timestamp);
> 	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
>-				    pf->timestamp);
>+				    timestamp);
>+
>+	return 0;
>+}
>+
>+static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
>+{
>+	struct iio_poll_func *pf = p;
>+	struct iio_dev *indio_dev = pf->indio_dev;
>+
>+	ad2s1210_collect_and_push(indio_dev, pf->timestamp);
> 
The return value does not appear to be used by the caller.

Should this helper return void instead, or should the caller handle the returned error code?

The helper function changes look good otherwise.

Thanks, Sanjay

>-error_ret:
> 	iio_trigger_notify_done(indio_dev->trig);
> 
> 	return IRQ_HANDLED;
Re: [PATCH] iio: resolver: ad2s1210: split trigger handler body into a helper
Posted by Sanjay Chitroda 1 week, 1 day ago 

On 17 May 2026 8:57:38 am IST, Sanjay Chitroda <sanjayembeddedse@gmail.com> wrote:
>
>
>On 16 May 2026 8:47:20 pm IST, Stepan Ionichev <sozdayvek@gmail.com> wrote:
>>ad2s1210_trigger_handler() takes st->lock via guard(mutex), then runs
>
>>--- a/drivers/iio/resolver/ad2s1210.c
>>+++ b/drivers/iio/resolver/ad2s1210.c
>>@@ -1276,10 +1276,8 @@ static int ad2s1210_debugfs_reg_access(struct iio_dev *indio_dev,
>> 	return regmap_write(st->regmap, reg, writeval);
>> }
>> 
>>-static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
>>+static int ad2s1210_collect_and_push(struct iio_dev *indio_dev, s64 timestamp)
>Hi Stepan,
>
>Is the return value of this function checked by any caller?
>
>If not, should this helper return void instead?
>
>> {
>>-	struct iio_poll_func *pf = p;
>>-	struct iio_dev *indio_dev = pf->indio_dev;
>> 	struct ad2s1210_state *st = iio_priv(indio_dev);
>> 	size_t chan = 0;
>> 	int ret;
>>@@ -1295,15 +1293,15 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
>> 					       AD2S1210_REG_POSITION_MSB,
>> 					       &st->sample.raw, 2);
>> 			if (ret < 0)
>>-				goto error_ret;
>>+				return ret;
>> 		} else {
>> 			ret = ad2s1210_set_mode(st, MOD_POS);
>> 			if (ret < 0)
>>-				goto error_ret;
>>+				return ret;
>> 
>> 			ret = spi_read(st->sdev, &st->sample, 3);
>> 			if (ret < 0)
>>-				goto error_ret;
>>+				return ret;
>> 		}
>> 
>> 		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
>>@@ -1315,15 +1313,15 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
>> 					       AD2S1210_REG_VELOCITY_MSB,
>> 					       &st->sample.raw, 2);
>> 			if (ret < 0)
>>-				goto error_ret;
>>+				return ret;
>> 		} else {
>> 			ret = ad2s1210_set_mode(st, MOD_VEL);
>> 			if (ret < 0)
>>-				goto error_ret;
>>+				return ret;
>> 
>> 			ret = spi_read(st->sdev, &st->sample, 3);
>> 			if (ret < 0)
>>-				goto error_ret;
>>+				return ret;
>> 		}
>> 
>> 		memcpy(&st->scan.chan[chan++], &st->sample.raw, 2);
>>@@ -1334,16 +1332,25 @@ static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
>> 
>> 		ret = regmap_read(st->regmap, AD2S1210_REG_FAULT, &reg_val);
>> 		if (ret < 0)
>>-			goto error_ret;
>>+			return ret;
>> 
>
>I cannot find this goto instance in mainline.
>
>Is this patch based on top of iio/testing or another branch?
>
>For patch dependencies like this, is it preferred to mention the base commit in the changelog for single patches as well or in the commit prefix?
>
Looks like David has submitted change answers my concern.

Thanks, Sanjay
>
>> 		st->sample.fault = reg_val;
>> 	}
>> 
>>-	ad2s1210_push_events(indio_dev, st->sample.fault, pf->timestamp);
>>+	ad2s1210_push_events(indio_dev, st->sample.fault, timestamp);
>> 	iio_push_to_buffers_with_ts(indio_dev, &st->scan, sizeof(st->scan),
>>-				    pf->timestamp);
>>+				    timestamp);
>>+
>>+	return 0;
>>+}
>>+
>>+static irqreturn_t ad2s1210_trigger_handler(int irq, void *p)
>>+{
>>+	struct iio_poll_func *pf = p;
>>+	struct iio_dev *indio_dev = pf->indio_dev;
>>+
>>+	ad2s1210_collect_and_push(indio_dev, pf->timestamp);
>> 
>The return value does not appear to be used by the caller.
>
>Should this helper return void instead, or should the caller handle the returned error code?
>
>The helper function changes look good otherwise.
>
>Thanks, Sanjay
>
>>-error_ret:
>> 	iio_trigger_notify_done(indio_dev->trig);
>> 
>> 	return IRQ_HANDLED;
Re: [PATCH] iio: resolver: ad2s1210: split trigger handler body into a helper
Posted by Stepan Ionichev 1 week, 1 day ago 
On Sat, May 16, 2026 at 20:17:21, Stepan Ionichev wrote:
> [PATCH] iio: resolver: ad2s1210: split trigger handler body into a helper

Please drop this -- I missed that David posted essentially the same
refactor eight minutes earlier:

  https://lore.kernel.org/linux-iio/20260516-iio-resolver-refactor-trigger-handler-v1-1-25b11ba0155e@baylibre.com/

His version is cleaner (void helper, since the irq handler never
looks at the return value).

Reviewing his patch separately.

Stepan

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.