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

[Sanjay Chitroda <sanjayembeddedse@gmail.com>]

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;