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

[Sanjay Chitroda]

On 17 May 2026 8:57:38 am IST, Sanjay Chitroda <sanjayembeddedse@xxxxxxxxx> wrote:
>
>
>On 16 May 2026 8:47:20 pm IST, Stepan Ionichev <sozdayvek@xxxxxxxxx> 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;