[PATCH v2 0/4] ti-ads7950: fix gpio handling and facelift

[PATCH v2 0/4] ti-ads7950: fix gpio handling and facelift

[Dmitry Torokhov]

The original reason for this series is to make sure ti_ads7950_get()
returns result in range [0, 1] or a negative error code to comply with
gpiolib requirement.

During review David noticed the fact that the function also clobbers
gpio state. Fixing that lead to adding cleanups using guard() and devm.

v2:

- style fixes in #1 (per David)
- fix to not clobber state (#2, new)
- #3 and #4 (new)


Dmitry Torokhov (4):
  iio: adc: ti-ads7950: normalize return value of gpio_get
  iio: adc: ti-ads7950: do not clobber gpio state in ti_ads7950_get()
  iio: adc: ti-ads7950: switch to using guard() notation
  iio: adc: ti-ads7950: complete conversion to using managed resources

 drivers/iio/adc/ti-ads7950.c | 206 +++++++++++++++--------------------
 1 file changed, 90 insertions(+), 116 deletions(-)

Thanks.

-- 
Dmitry

[PATCH v2 1/4] iio: adc: ti-ads7950: normalize return value of gpio_get

[Dmitry Torokhov]

The GPIO get callback is expected to return 0 or 1 (or a negative error
code). Ensure that the value returned by ti_ads7950_get() for output
pins is normalized to the [0, 1] range.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/iio/adc/ti-ads7950.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index bbe1ce577789..b8cc39fc39fb 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -433,7 +433,7 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
 
 	/* If set as output, return the output */
 	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
-		ret = st->cmd_settings_bitmask & BIT(offset);
+		ret = (st->cmd_settings_bitmask & BIT(offset)) ? 1 : 0;
 		goto out;
 	}
 
-- 
2.53.0.335.g19a08e0c02-goog

Re: [PATCH v2 1/4] iio: adc: ti-ads7950: normalize return value of gpio_get

[Andy Shevchenko]

On Wed, Feb 18, 2026 at 06:29:25PM -0800, Dmitry Torokhov wrote:
> The GPIO get callback is expected to return 0 or 1 (or a negative error
> code). Ensure that the value returned by ti_ads7950_get() for output
> pins is normalized to the [0, 1] range.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/4] iio: adc: ti-ads7950: normalize return value of gpio_get

[Bartosz Golaszewski]

On Thu, 19 Feb 2026 03:29:25 +0100, Dmitry Torokhov
<dmitry.torokhov@gmail.com> said:
> The GPIO get callback is expected to return 0 or 1 (or a negative error
> code). Ensure that the value returned by ti_ads7950_get() for output
> pins is normalized to the [0, 1] range.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/iio/adc/ti-ads7950.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index bbe1ce577789..b8cc39fc39fb 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -433,7 +433,7 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
>
>  	/* If set as output, return the output */
>  	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
> -		ret = st->cmd_settings_bitmask & BIT(offset);
> +		ret = (st->cmd_settings_bitmask & BIT(offset)) ? 1 : 0;
>  		goto out;
>  	}
>
> --
> 2.53.0.335.g19a08e0c02-goog
>
>

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>

Re: [PATCH v2 1/4] iio: adc: ti-ads7950: normalize return value of gpio_get

[Linus Walleij]

On Thu, Feb 19, 2026 at 3:29 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:

> The GPIO get callback is expected to return 0 or 1 (or a negative error
> code). Ensure that the value returned by ti_ads7950_get() for output
> pins is normalized to the [0, 1] range.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Reviewed-by: Linus Walleij <linusw@kernel.org>

Yours,
Linus Walleij

Re: [PATCH v2 1/4] iio: adc: ti-ads7950: normalize return value of gpio_get

[Jonathan Cameron]

On Wed, 18 Feb 2026 18:29:25 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> The GPIO get callback is expected to return 0 or 1 (or a negative error
> code). Ensure that the value returned by ti_ads7950_get() for output
> pins is normalized to the [0, 1] range.
Fixes?  I'm not quite sure with something that says 'expected'

Jonathan

> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/iio/adc/ti-ads7950.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index bbe1ce577789..b8cc39fc39fb 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -433,7 +433,7 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
>  
>  	/* If set as output, return the output */
>  	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
> -		ret = st->cmd_settings_bitmask & BIT(offset);
> +		ret = (st->cmd_settings_bitmask & BIT(offset)) ? 1 : 0;
>  		goto out;
>  	}
>  

Re: [PATCH v2 1/4] iio: adc: ti-ads7950: normalize return value of gpio_get

[Dmitry Torokhov]

On Sun, Feb 22, 2026 at 02:03:15PM +0000, Jonathan Cameron wrote:
> On Wed, 18 Feb 2026 18:29:25 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > The GPIO get callback is expected to return 0 or 1 (or a negative error
> > code). Ensure that the value returned by ti_ads7950_get() for output
> > pins is normalized to the [0, 1] range.
> Fixes?  I'm not quite sure with something that says 'expected'

Will add a tag.

Thanks.

-- 
Dmitry

[PATCH v2 2/4] iio: adc: ti-ads7950: do not clobber gpio state in ti_ads7950_get()

[Dmitry Torokhov]

GPIO state was inadvertently overwritten by the result of sip_sync,
reuniting in ti_ads7950_get() only returning 0 as gpio state (or error).

Fix this by introducing a separate variable to hold the state.

Reported-by: David Lechner <dlechner@baylibre.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/iio/adc/ti-ads7950.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index b8cc39fc39fb..2a7d4a1d9fa9 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -427,13 +427,14 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
 static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct ti_ads7950_state *st = gpiochip_get_data(chip);
-	int ret;
+	int ret = 0;
+	bool state;
 
 	mutex_lock(&st->slock);
 
 	/* If set as output, return the output */
 	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
-		ret = (st->cmd_settings_bitmask & BIT(offset)) ? 1 : 0;
+		state = st->cmd_settings_bitmask & BIT(offset);
 		goto out;
 	}
 
@@ -444,7 +445,7 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
 	if (ret)
 		goto out;
 
-	ret = ((st->single_rx >> 12) & BIT(offset)) ? 1 : 0;
+	state = (st->single_rx >> 12) & BIT(offset);
 
 	/* Revert back to original settings */
 	st->cmd_settings_bitmask &= ~TI_ADS7950_CR_GPIO_DATA;
@@ -456,7 +457,7 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
 out:
 	mutex_unlock(&st->slock);
 
-	return ret;
+	return ret ?: state;
 }
 
 static int ti_ads7950_get_direction(struct gpio_chip *chip,
-- 
2.53.0.335.g19a08e0c02-goog

Re: [PATCH v2 2/4] iio: adc: ti-ads7950: do not clobber gpio state in ti_ads7950_get()

[Andy Shevchenko]

On Wed, Feb 18, 2026 at 06:29:26PM -0800, Dmitry Torokhov wrote:
> GPIO state was inadvertently overwritten by the result of sip_sync,

sip?

> reuniting in ti_ads7950_get() only returning 0 as gpio state (or error).

GPIO

> Fix this by introducing a separate variable to hold the state.

...

> -	int ret;
> +	int ret = 0;

This kind of assignment is harder to maintain, because it leaves a room for
subtle mistakes (when it's reused by a newly injected code, quite similar,
actually, to what this change tries to address). Please, decouple definition
and assignment.

> +	bool state;

Not sure about this (yes, I know it was suggested). I would leave it still
the same type as one that is returned by the function or a compatible one
that is the same as st->* (if the used ones are all of the same type).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/4] iio: adc: ti-ads7950: do not clobber gpio state in ti_ads7950_get()

[David Lechner]

On 2/18/26 8:29 PM, Dmitry Torokhov wrote:
> GPIO state was inadvertently overwritten by the result of sip_sync,
> reuniting in ti_ads7950_get() only returning 0 as gpio state (or error).
> 
> Fix this by introducing a separate variable to hold the state.
> 
> Reported-by: David Lechner <dlechner@baylibre.com>

This should have a Fixes: tag since it is fixing a real bug.

Also, fixes should come first in the series.

> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/iio/adc/ti-ads7950.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index b8cc39fc39fb..2a7d4a1d9fa9 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -427,13 +427,14 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
>  static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> -	int ret;
> +	int ret = 0;
> +	bool state;
>  
>  	mutex_lock(&st->slock);
>  
>  	/* If set as output, return the output */
>  	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
> -		ret = (st->cmd_settings_bitmask & BIT(offset)) ? 1 : 0;
> +		state = st->cmd_settings_bitmask & BIT(offset);

I agree it would be better to put...

		ret = 0;

here.

>  		goto out;
>  	}
>  
> @@ -444,7 +445,7 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
>  	if (ret)
>  		goto out;
>  
> -	ret = ((st->single_rx >> 12) & BIT(offset)) ? 1 : 0;
> +	state = (st->single_rx >> 12) & BIT(offset);
>  
>  	/* Revert back to original settings */
>  	st->cmd_settings_bitmask &= ~TI_ADS7950_CR_GPIO_DATA;
> @@ -456,7 +457,7 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
>  out:
>  	mutex_unlock(&st->slock);
>  
> -	return ret;
> +	return ret ?: state;>  }
>  
>  static int ti_ads7950_get_direction(struct gpio_chip *chip,

[PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[Dmitry Torokhov]

guard() notation allows early returns when encountering errors, making
control flow more obvious. Use it.

Also variables that now only hold error codes (or 0) are renamed to
"error" to make their purpose clearer.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/iio/adc/ti-ads7950.c | 105 ++++++++++++++++-------------------
 1 file changed, 48 insertions(+), 57 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 2a7d4a1d9fa9..d31397f37ec4 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -306,18 +306,17 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
 	struct iio_poll_func *pf = p;
 	struct iio_dev *indio_dev = pf->indio_dev;
 	struct ti_ads7950_state *st = iio_priv(indio_dev);
-	int ret;
+	int error;
 
-	mutex_lock(&st->slock);
-	ret = spi_sync(st->spi, &st->ring_msg);
-	if (ret < 0)
-		goto out;
+	scoped_guard(mutex, &st->slock) {
+		error = spi_sync(st->spi, &st->ring_msg);
+		if (error)
+			break;
 
-	iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
-					   iio_get_time_ns(indio_dev));
+		iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
+						   iio_get_time_ns(indio_dev));
+	}
 
-out:
-	mutex_unlock(&st->slock);
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
@@ -326,22 +325,19 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
 static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
 {
 	struct ti_ads7950_state *st = iio_priv(indio_dev);
-	int ret, cmd;
+	int error;
+	int cmd;
+
+	guard(mutex)(&st->slock);
 
-	mutex_lock(&st->slock);
 	cmd = TI_ADS7950_MAN_CMD(TI_ADS7950_CR_CHAN(ch));
 	st->single_tx = cmd;
 
-	ret = spi_sync(st->spi, &st->scan_single_msg);
-	if (ret)
-		goto out;
-
-	ret = st->single_rx;
-
-out:
-	mutex_unlock(&st->slock);
+	error = spi_sync(st->spi, &st->scan_single_msg);
+	if (error)
+		return error;
 
-	return ret;
+	return st->single_rx;
 }
 
 static int ti_ads7950_get_range(struct ti_ads7950_state *st)
@@ -407,9 +403,9 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
 			  int value)
 {
 	struct ti_ads7950_state *st = gpiochip_get_data(chip);
-	int ret;
+	int error;
 
-	mutex_lock(&st->slock);
+	guard(mutex)(&st->slock);
 
 	if (value)
 		st->cmd_settings_bitmask |= BIT(offset);
@@ -417,47 +413,44 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
 		st->cmd_settings_bitmask &= ~BIT(offset);
 
 	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
-	ret = spi_sync(st->spi, &st->scan_single_msg);
-
-	mutex_unlock(&st->slock);
+	error = spi_sync(st->spi, &st->scan_single_msg);
+	if (error)
+		return error;
 
-	return ret;
+	return 0;
 }
 
 static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct ti_ads7950_state *st = gpiochip_get_data(chip);
-	int ret = 0;
 	bool state;
+	int error;
 
-	mutex_lock(&st->slock);
+	guard(mutex)(&st->slock);
 
 	/* If set as output, return the output */
 	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
 		state = st->cmd_settings_bitmask & BIT(offset);
-		goto out;
+		return state;
 	}
 
 	/* GPIO data bit sets SDO bits 12-15 to GPIO input */
 	st->cmd_settings_bitmask |= TI_ADS7950_CR_GPIO_DATA;
 	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
-	ret = spi_sync(st->spi, &st->scan_single_msg);
-	if (ret)
-		goto out;
+	error = spi_sync(st->spi, &st->scan_single_msg);
+	if (error)
+		return error;
 
 	state = (st->single_rx >> 12) & BIT(offset);
 
 	/* Revert back to original settings */
 	st->cmd_settings_bitmask &= ~TI_ADS7950_CR_GPIO_DATA;
 	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
-	ret = spi_sync(st->spi, &st->scan_single_msg);
-	if (ret)
-		goto out;
-
-out:
-	mutex_unlock(&st->slock);
+	error = spi_sync(st->spi, &st->scan_single_msg);
+	if (error)
+		return error;
 
-	return ret ?: state;
+	return state;
 }
 
 static int ti_ads7950_get_direction(struct gpio_chip *chip,
@@ -473,9 +466,9 @@ static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
 				     int input)
 {
 	struct ti_ads7950_state *st = gpiochip_get_data(chip);
-	int ret = 0;
+	int error;
 
-	mutex_lock(&st->slock);
+	guard(mutex)(&st->slock);
 
 	/* Only change direction if needed */
 	if (input && (st->gpio_cmd_settings_bitmask & BIT(offset)))
@@ -483,15 +476,14 @@ static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
 	else if (!input && !(st->gpio_cmd_settings_bitmask & BIT(offset)))
 		st->gpio_cmd_settings_bitmask |= BIT(offset);
 	else
-		goto out;
+		return 0;
 
 	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
-	ret = spi_sync(st->spi, &st->scan_single_msg);
+	error = spi_sync(st->spi, &st->scan_single_msg);
+	if (error)
+		return error;
 
-out:
-	mutex_unlock(&st->slock);
-
-	return ret;
+	return 0;
 }
 
 static int ti_ads7950_direction_input(struct gpio_chip *chip,
@@ -514,27 +506,26 @@ static int ti_ads7950_direction_output(struct gpio_chip *chip,
 
 static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
 {
-	int ret = 0;
+	int error;
 
-	mutex_lock(&st->slock);
+	guard(mutex)(&st->slock);
 
 	/* Settings for Manual/Auto1/Auto2 commands */
 	/* Default to 5v ref */
 	st->cmd_settings_bitmask = TI_ADS7950_CR_RANGE_5V;
 	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
-	ret = spi_sync(st->spi, &st->scan_single_msg);
-	if (ret)
-		goto out;
+	error = spi_sync(st->spi, &st->scan_single_msg);
+	if (error)
+		return error;
 
 	/* Settings for GPIO command */
 	st->gpio_cmd_settings_bitmask = 0x0;
 	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
-	ret = spi_sync(st->spi, &st->scan_single_msg);
-
-out:
-	mutex_unlock(&st->slock);
+	error = spi_sync(st->spi, &st->scan_single_msg);
+	if (error)
+		return error;
 
-	return ret;
+	return 0;
 }
 
 static int ti_ads7950_probe(struct spi_device *spi)
-- 
2.53.0.335.g19a08e0c02-goog

Re: [PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[Andy Shevchenko]

On Wed, Feb 18, 2026 at 06:29:27PM -0800, Dmitry Torokhov wrote:
> guard() notation allows early returns when encountering errors, making

guard()()

// strictly speaking

> control flow more obvious. Use it.

I like the change, but...

> Also variables that now only hold error codes (or 0) are renamed to
> "error" to make their purpose clearer.

...this does not belong to the patch. If you wish to rename, better doing
it separately. This, in particular, adds undesired churn in the change making
it unclear.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[David Lechner]

On 2/19/26 1:51 AM, Andy Shevchenko wrote:
> On Wed, Feb 18, 2026 at 06:29:27PM -0800, Dmitry Torokhov wrote:
>> guard() notation allows early returns when encountering errors, making
> 
> guard()()
> 
> // strictly speaking
> 
>> control flow more obvious. Use it.
> 
> I like the change, but...
> 
>> Also variables that now only hold error codes (or 0) are renamed to
>> "error" to make their purpose clearer.

Normally I would not give my opinion on this, but since I wrote the driver
originally, I will say please don't rename. I prefer to always use "ret".

> 
> ...this does not belong to the patch. If you wish to rename, better doing
> it separately. This, in particular, adds undesired churn in the change making
> it unclear.
> 

Re: [PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[Dmitry Torokhov]

On Sat, Feb 21, 2026 at 11:20:42AM -0600, David Lechner wrote:
> On 2/19/26 1:51 AM, Andy Shevchenko wrote:
> > On Wed, Feb 18, 2026 at 06:29:27PM -0800, Dmitry Torokhov wrote:
> >> guard() notation allows early returns when encountering errors, making
> > 
> > guard()()
> > 
> > // strictly speaking
> > 
> >> control flow more obvious. Use it.
> > 
> > I like the change, but...
> > 
> >> Also variables that now only hold error codes (or 0) are renamed to
> >> "error" to make their purpose clearer.
> 
> Normally I would not give my opinion on this, but since I wrote the driver
> originally, I will say please don't rename. I prefer to always use "ret".

I hope I can convince you otherwise.

IMO "ret" or "retval" should be used when the returned value is intended
to be used during normal operation. For cases where we only expect to
have an error or 0 for success "error" or "err" is more appropriate.
This allows you to write

	error = do_action(...);
	if (error) {
		// handle error somehow, typically simply report.
	}

This also helps when reading the code as you know that there is usually
no reason to care about the specific value in this variable (maybe
except -EPROBE_DEFER).

I will push the conversion ret -> error to the very last patch so it can
easily be dropped if I am unsuccessful in swaying your opinion.

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[David Lechner]

On 2/22/26 3:31 PM, Dmitry Torokhov wrote:
> On Sat, Feb 21, 2026 at 11:20:42AM -0600, David Lechner wrote:
>> On 2/19/26 1:51 AM, Andy Shevchenko wrote:
>>> On Wed, Feb 18, 2026 at 06:29:27PM -0800, Dmitry Torokhov wrote:
>>>> guard() notation allows early returns when encountering errors, making
>>>
>>> guard()()
>>>
>>> // strictly speaking
>>>
>>>> control flow more obvious. Use it.
>>>
>>> I like the change, but...
>>>
>>>> Also variables that now only hold error codes (or 0) are renamed to
>>>> "error" to make their purpose clearer.
>>
>> Normally I would not give my opinion on this, but since I wrote the driver
>> originally, I will say please don't rename. I prefer to always use "ret".
> 
> I hope I can convince you otherwise.

I'm afraid not. "ret" is used > 35k times in IIO and err is used < 3k times
and error < 1k times. So I am really used to seeing only "ret" for errors and
"error" looks very unusual to my eyes because of this.

You mentioned valuing the common pattern in your other reply, so I hope
you can understand that this is what I value more here.

> 
> IMO "ret" or "retval" should be used when the returned value is intended
> to be used during normal operation. For cases where we only expect to
> have an error or 0 for success "error" or "err" is more appropriate.
> This allows you to write
> 
> 	error = do_action(...);
> 	if (error) {
> 		// handle error somehow, typically simply report.
> 	}
> 
> This also helps when reading the code as you know that there is usually
> no reason to care about the specific value in this variable (maybe
> except -EPROBE_DEFER).
> 
> I will push the conversion ret -> error to the very last patch so it can
> easily be dropped if I am unsuccessful in swaying your opinion.
> 
> Thanks.
> 

To give you fair warning, it will still be NAK from me.

Re: [PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[David Lechner]

On 2/18/26 8:29 PM, Dmitry Torokhov wrote:
> guard() notation allows early returns when encountering errors, making
> control flow more obvious. Use it.
> 
> Also variables that now only hold error codes (or 0) are renamed to
> "error" to make their purpose clearer.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/iio/adc/ti-ads7950.c | 105 ++++++++++++++++-------------------
>  1 file changed, 48 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 2a7d4a1d9fa9..d31397f37ec4 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -306,18 +306,17 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>  	struct iio_poll_func *pf = p;
>  	struct iio_dev *indio_dev = pf->indio_dev;
>  	struct ti_ads7950_state *st = iio_priv(indio_dev);
> -	int ret;
> +	int error;
>  
> -	mutex_lock(&st->slock);
> -	ret = spi_sync(st->spi, &st->ring_msg);
> -	if (ret < 0)
> -		goto out;
> +	scoped_guard(mutex, &st->slock) {
> +		error = spi_sync(st->spi, &st->ring_msg);
> +		if (error)
> +			break;

I'm not a fan of scoped_guard() because of the hidden for loop in it.
It hides the fact that the break; is breaking out of that for loop.

It would be more clear/obvious written as:

	do {
		guard(mutex)(&st->slock);

		ret = spi_sync(st->spi, &st->ring_msg);
		if (ret)
			break;

		iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
						   iio_get_time_ns(indio_dev));
	} while (0);

>  
> -	iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
> -					   iio_get_time_ns(indio_dev));
> +		iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
> +						   iio_get_time_ns(indio_dev));
> +	}
>  
> -out:
> -	mutex_unlock(&st->slock);
>  	iio_trigger_notify_done(indio_dev->trig);
>  
>  	return IRQ_HANDLED;
> @@ -326,22 +325,19 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>  static int ti_ads7950_scan_direct(struct iio_dev *indio_dev, unsigned int ch)
>  {
>  	struct ti_ads7950_state *st = iio_priv(indio_dev);
> -	int ret, cmd;
> +	int error;
> +	int cmd;
> +
> +	guard(mutex)(&st->slock);
>  
> -	mutex_lock(&st->slock);
>  	cmd = TI_ADS7950_MAN_CMD(TI_ADS7950_CR_CHAN(ch));
>  	st->single_tx = cmd;
>  
> -	ret = spi_sync(st->spi, &st->scan_single_msg);
> -	if (ret)
> -		goto out;
> -
> -	ret = st->single_rx;
> -
> -out:
> -	mutex_unlock(&st->slock);
> +	error = spi_sync(st->spi, &st->scan_single_msg);
> +	if (error)
> +		return error;
>  
> -	return ret;
> +	return st->single_rx;
>  }
>  
>  static int ti_ads7950_get_range(struct ti_ads7950_state *st)
> @@ -407,9 +403,9 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
>  			  int value)
>  {
>  	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> -	int ret;
> +	int error;
>  
> -	mutex_lock(&st->slock);
> +	guard(mutex)(&st->slock);
>  
>  	if (value)
>  		st->cmd_settings_bitmask |= BIT(offset);
> @@ -417,47 +413,44 @@ static int ti_ads7950_set(struct gpio_chip *chip, unsigned int offset,
>  		st->cmd_settings_bitmask &= ~BIT(offset);
>  
>  	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> -	ret = spi_sync(st->spi, &st->scan_single_msg);
> -
> -	mutex_unlock(&st->slock);
> +	error = spi_sync(st->spi, &st->scan_single_msg);
> +	if (error)
> +		return error;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
>  {
>  	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> -	int ret = 0;
>  	bool state;
> +	int error;
>  
> -	mutex_lock(&st->slock);
> +	guard(mutex)(&st->slock);
>  
>  	/* If set as output, return the output */
>  	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
>  		state = st->cmd_settings_bitmask & BIT(offset);
> -		goto out;
> +		return state;

This can return directly instead of using local variable.

>  	}
>  
>  	/* GPIO data bit sets SDO bits 12-15 to GPIO input */
>  	st->cmd_settings_bitmask |= TI_ADS7950_CR_GPIO_DATA;
>  	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> -	ret = spi_sync(st->spi, &st->scan_single_msg);
> -	if (ret)
> -		goto out;
> +	error = spi_sync(st->spi, &st->scan_single_msg);
> +	if (error)
> +		return error;
>  
>  	state = (st->single_rx >> 12) & BIT(offset);
>  
>  	/* Revert back to original settings */
>  	st->cmd_settings_bitmask &= ~TI_ADS7950_CR_GPIO_DATA;
>  	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> -	ret = spi_sync(st->spi, &st->scan_single_msg);
> -	if (ret)
> -		goto out;
> -
> -out:
> -	mutex_unlock(&st->slock);
> +	error = spi_sync(st->spi, &st->scan_single_msg);
> +	if (error)
> +		return error;
>  
> -	return ret ?: state;
> +	return state;
>  }
>  
>  static int ti_ads7950_get_direction(struct gpio_chip *chip,
> @@ -473,9 +466,9 @@ static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
>  				     int input)
>  {
>  	struct ti_ads7950_state *st = gpiochip_get_data(chip);
> -	int ret = 0;
> +	int error;
>  
> -	mutex_lock(&st->slock);
> +	guard(mutex)(&st->slock);
>  
>  	/* Only change direction if needed */
>  	if (input && (st->gpio_cmd_settings_bitmask & BIT(offset)))
> @@ -483,15 +476,14 @@ static int _ti_ads7950_set_direction(struct gpio_chip *chip, int offset,
>  	else if (!input && !(st->gpio_cmd_settings_bitmask & BIT(offset)))
>  		st->gpio_cmd_settings_bitmask |= BIT(offset);
>  	else
> -		goto out;
> +		return 0;
>  
>  	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
> -	ret = spi_sync(st->spi, &st->scan_single_msg);
> +	error = spi_sync(st->spi, &st->scan_single_msg);

Can just return directly here now.

> +	if (error)
> +		return error;
>  
> -out:
> -	mutex_unlock(&st->slock);
> -
> -	return ret;
> +	return 0;
>  }
>  
>  static int ti_ads7950_direction_input(struct gpio_chip *chip,
> @@ -514,27 +506,26 @@ static int ti_ads7950_direction_output(struct gpio_chip *chip,
>  
>  static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
>  {
> -	int ret = 0;
> +	int error;
>  
> -	mutex_lock(&st->slock);
> +	guard(mutex)(&st->slock);
>  
>  	/* Settings for Manual/Auto1/Auto2 commands */
>  	/* Default to 5v ref */
>  	st->cmd_settings_bitmask = TI_ADS7950_CR_RANGE_5V;
>  	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
> -	ret = spi_sync(st->spi, &st->scan_single_msg);
> -	if (ret)
> -		goto out;
> +	error = spi_sync(st->spi, &st->scan_single_msg);
> +	if (error)
> +		return error;
>  
>  	/* Settings for GPIO command */
>  	st->gpio_cmd_settings_bitmask = 0x0;
>  	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
> -	ret = spi_sync(st->spi, &st->scan_single_msg);
> -
> -out:
> -	mutex_unlock(&st->slock);
> +	error = spi_sync(st->spi, &st->scan_single_msg);

And can return directly here.

> +	if (error)
> +		return error;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int ti_ads7950_probe(struct spi_device *spi)

Re: [PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[Dmitry Torokhov]

On Sat, Feb 21, 2026 at 11:34:33AM -0600, David Lechner wrote:
> On 2/18/26 8:29 PM, Dmitry Torokhov wrote:
> > +	scoped_guard(mutex, &st->slock) {
> > +		error = spi_sync(st->spi, &st->ring_msg);
> > +		if (error)
> > +			break;
> 
> I'm not a fan of scoped_guard() because of the hidden for loop in it.
> It hides the fact that the break; is breaking out of that for loop.
> 
> It would be more clear/obvious written as:
> 
> 	do {
> 		guard(mutex)(&st->slock);
> 
> 		ret = spi_sync(st->spi, &st->ring_msg);
> 		if (ret)
> 			break;
> 
> 		iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
> 						   iio_get_time_ns(indio_dev));
> 	} while (0);

OK.

I could also make it

	scoped_guard(mutex, &st->slock) {
		ret = spi_sync(st->spi, &st->ring_msg);
		if (!ret)
			iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
							   iio_get_time_ns(indio_dev));
	}

to avoid using "break".

I think you will find that scoped_guard() will gain the foothold in the
kernel so having implementation that does not follow common pattern
might not be the best option.

> >  
> >  	/* If set as output, return the output */
> >  	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
> >  		state = st->cmd_settings_bitmask & BIT(offset);
> > -		goto out;
> > +		return state;
> 
> This can return directly instead of using local variable.

This will require the explicitly normalizing, which we avoided by
introducing "bool state" to begin with...

> >  
> >  	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
> > -	ret = spi_sync(st->spi, &st->scan_single_msg);
> > +	error = spi_sync(st->spi, &st->scan_single_msg);
> 
> Can just return directly here now.

I think there is benefit in explicitly calling out the error paths and
explicitly return 0 on success. It removes the doubt whether a function
can return positive value on success.

Thanks.

-- 
Dmitry

Re: [PATCH v2 3/4] iio: adc: ti-ads7950: switch to using guard() notation

[David Lechner]

On 2/22/26 3:37 PM, Dmitry Torokhov wrote:
> On Sat, Feb 21, 2026 at 11:34:33AM -0600, David Lechner wrote:
>> On 2/18/26 8:29 PM, Dmitry Torokhov wrote:
>>> +	scoped_guard(mutex, &st->slock) {
>>> +		error = spi_sync(st->spi, &st->ring_msg);
>>> +		if (error)
>>> +			break;
>>
>> I'm not a fan of scoped_guard() because of the hidden for loop in it.
>> It hides the fact that the break; is breaking out of that for loop.
>>
>> It would be more clear/obvious written as:
>>
>> 	do {
>> 		guard(mutex)(&st->slock);
>>
>> 		ret = spi_sync(st->spi, &st->ring_msg);
>> 		if (ret)
>> 			break;
>>
>> 		iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
>> 						   iio_get_time_ns(indio_dev));
>> 	} while (0);
> 
> OK.
> 
> I could also make it
> 
> 	scoped_guard(mutex, &st->slock) {
> 		ret = spi_sync(st->spi, &st->ring_msg);
> 		if (!ret)
> 			iio_push_to_buffers_with_timestamp(indio_dev, &st->rx_buf[2],
> 							   iio_get_time_ns(indio_dev));
> 	}

This is OK.

> 
> to avoid using "break".
> 
> I think you will find that scoped_guard() will gain the foothold in the
> kernel so having implementation that does not follow common pattern
> might not be the best option.

In general, I agree with this idea.

In this case, the common pattern unfortunately leads to common bugs.
Since we were discussing this, I audited the kernel and found and 
reported 3 unintentional mistakes with break/continue when scoped_guard()
was introduced. And I have caught more in reviews before they made it
into the kernel.

So I make an exception here to thinking that the common pattern is best.

> 
>>>  
>>>  	/* If set as output, return the output */
>>>  	if (st->gpio_cmd_settings_bitmask & BIT(offset)) {
>>>  		state = st->cmd_settings_bitmask & BIT(offset);
>>> -		goto out;
>>> +		return state;
>>
>> This can return directly instead of using local variable.
> 
> This will require the explicitly normalizing, which we avoided by
> introducing "bool state" to begin with...
> 
>>>  
>>>  	st->single_tx = TI_ADS7950_GPIO_CMD_SETTINGS(st);
>>> -	ret = spi_sync(st->spi, &st->scan_single_msg);
>>> +	error = spi_sync(st->spi, &st->scan_single_msg);
>>
>> Can just return directly here now.
> 
> I think there is benefit in explicitly calling out the error paths and
> explicitly return 0 on success. It removes the doubt whether a function
> can return positive value on success.

In the IIO subsystem, the direct return is preferred (maintainer and
other reviewers frequently request this).

> 
> Thanks.
> 

[PATCH v2 4/4] iio: adc: ti-ads7950: complete conversion to using managed resources

[Dmitry Torokhov]

All resources that the driver needs have managed API now. Switch to
using them to make code clearer and drop ti_ads7950_remove().

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/iio/adc/ti-ads7950.c | 98 +++++++++++++++---------------------
 1 file changed, 40 insertions(+), 58 deletions(-)

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index d31397f37ec4..1c53e000bdcc 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -528,19 +528,26 @@ static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
 	return 0;
 }
 
+static void ti_ads7950_power_off(void *data)
+{
+	struct ti_ads7950_state *st = data;
+
+	regulator_disable(st->reg);
+}
+
 static int ti_ads7950_probe(struct spi_device *spi)
 {
 	struct ti_ads7950_state *st;
 	struct iio_dev *indio_dev;
 	const struct ti_ads7950_chip_info *info;
-	int ret;
+	int error;
 
 	spi->bits_per_word = 16;
 	spi->mode |= SPI_CS_WORD;
-	ret = spi_setup(spi);
-	if (ret < 0) {
+	error = spi_setup(spi);
+	if (error) {
 		dev_err(&spi->dev, "Error in spi setup\n");
-		return ret;
+		return error;
 	}
 
 	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
@@ -598,36 +605,36 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	mutex_init(&st->slock);
 
 	st->reg = devm_regulator_get(&spi->dev, "vref");
-	if (IS_ERR(st->reg)) {
-		ret = dev_err_probe(&spi->dev, PTR_ERR(st->reg),
+	error = PTR_ERR_OR_ZERO(st->reg);
+	if (error)
+		return dev_err_probe(&spi->dev, error,
 				     "Failed to get regulator \"vref\"\n");
-		goto error_destroy_mutex;
-	}
 
-	ret = regulator_enable(st->reg);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to enable regulator \"vref\"\n");
-		goto error_destroy_mutex;
-	}
+	error = regulator_enable(st->reg);
+	if (error)
+		return dev_err_probe(&spi->dev, error,
+				     "Failed to enable regulator \"vref\"\n");
 
-	ret = iio_triggered_buffer_setup(indio_dev, NULL,
-					 &ti_ads7950_trigger_handler, NULL);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to setup triggered buffer\n");
-		goto error_disable_reg;
-	}
+	error = devm_add_action_or_reset(&spi->dev, ti_ads7950_power_off, st);
+	if (error)
+		return error;
 
-	ret = ti_ads7950_init_hw(st);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to init adc chip\n");
-		goto error_cleanup_ring;
-	}
+	error = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev, NULL,
+						&ti_ads7950_trigger_handler,
+						NULL);
+	if (error)
+		return dev_err_probe(&spi->dev, error,
+				     "Failed to setup triggered buffer\n");
 
-	ret = iio_device_register(indio_dev);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to register iio device\n");
-		goto error_cleanup_ring;
-	}
+	error = ti_ads7950_init_hw(st);
+	if (error)
+		return dev_err_probe(&spi->dev, error,
+				     "Failed to init adc chip\n");
+
+	error = devm_iio_device_register(&spi->dev, indio_dev);
+	if (error)
+		return dev_err_probe(&spi->dev, error,
+				     "Failed to register iio device\n");
 
 	/* Add GPIO chip */
 	st->chip.label = dev_name(&st->spi->dev);
@@ -642,36 +649,12 @@ static int ti_ads7950_probe(struct spi_device *spi)
 	st->chip.get = ti_ads7950_get;
 	st->chip.set = ti_ads7950_set;
 
-	ret = gpiochip_add_data(&st->chip, st);
-	if (ret) {
-		dev_err(&spi->dev, "Failed to init GPIOs\n");
-		goto error_iio_device;
-	}
+	error = devm_gpiochip_add_data(&spi->dev, &st->chip, st);
+	if (error)
+		return dev_err_probe(&spi->dev, error,
+				     "Failed to init GPIOs\n");
 
 	return 0;
-
-error_iio_device:
-	iio_device_unregister(indio_dev);
-error_cleanup_ring:
-	iio_triggered_buffer_cleanup(indio_dev);
-error_disable_reg:
-	regulator_disable(st->reg);
-error_destroy_mutex:
-	mutex_destroy(&st->slock);
-
-	return ret;
-}
-
-static void ti_ads7950_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct ti_ads7950_state *st = iio_priv(indio_dev);
-
-	gpiochip_remove(&st->chip);
-	iio_device_unregister(indio_dev);
-	iio_triggered_buffer_cleanup(indio_dev);
-	regulator_disable(st->reg);
-	mutex_destroy(&st->slock);
 }
 
 static const struct spi_device_id ti_ads7950_id[] = {
@@ -714,7 +697,6 @@ static struct spi_driver ti_ads7950_driver = {
 		.of_match_table = ads7950_of_table,
 	},
 	.probe		= ti_ads7950_probe,
-	.remove		= ti_ads7950_remove,
 	.id_table	= ti_ads7950_id,
 };
 module_spi_driver(ti_ads7950_driver);
-- 
2.53.0.335.g19a08e0c02-goog

Re: [PATCH v2 4/4] iio: adc: ti-ads7950: complete conversion to using managed resources

[Andy Shevchenko]

On Wed, Feb 18, 2026 at 06:29:28PM -0800, Dmitry Torokhov wrote:
> All resources that the driver needs have managed API now. Switch to
> using them to make code clearer and drop ti_ads7950_remove().

...

>  static int ti_ads7950_probe(struct spi_device *spi)
>  {
>  	struct ti_ads7950_state *st;
>  	struct iio_dev *indio_dev;
>  	const struct ti_ads7950_chip_info *info;
> -	int ret;
> +	int error;

Unrelated change.

...

>  	spi->bits_per_word = 16;
>  	spi->mode |= SPI_CS_WORD;
> -	ret = spi_setup(spi);
> -	if (ret < 0) {
> +	error = spi_setup(spi);
> +	if (error) {
>  		dev_err(&spi->dev, "Error in spi setup\n");
> -		return ret;
> +		return error;
>  	}

Ditto.

And since there is already dev_err_probe() in use, I would expect this
also be converted.

Would be also nice to use

	struct device *dev = &spi->dev;

to make less LoCs and/or make them shorter.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] iio: adc: ti-ads7950: complete conversion to using managed resources

[Dmitry Torokhov]

On Thu, Feb 19, 2026 at 09:59:35AM +0200, Andy Shevchenko wrote:
> On Wed, Feb 18, 2026 at 06:29:28PM -0800, Dmitry Torokhov wrote:
> > All resources that the driver needs have managed API now. Switch to
> > using them to make code clearer and drop ti_ads7950_remove().
> 
> ...
> 
> >  static int ti_ads7950_probe(struct spi_device *spi)
> >  {
> >  	struct ti_ads7950_state *st;
> >  	struct iio_dev *indio_dev;
> >  	const struct ti_ads7950_chip_info *info;
> > -	int ret;
> > +	int error;
> 
> Unrelated change.
> 
> ...
> 
> >  	spi->bits_per_word = 16;
> >  	spi->mode |= SPI_CS_WORD;
> > -	ret = spi_setup(spi);
> > -	if (ret < 0) {
> > +	error = spi_setup(spi);
> > +	if (error) {
> >  		dev_err(&spi->dev, "Error in spi setup\n");
> > -		return ret;
> > +		return error;
> >  	}
> 
> Ditto.
> 
> And since there is already dev_err_probe() in use, I would expect this
> also be converted.

But that would be unrelated change ;)

> 
> Would be also nice to use
> 
> 	struct device *dev = &spi->dev;
> 
> to make less LoCs and/or make them shorter.

I actually dislike introducing such temporaries in majority of the
cases. It makes it hard to follow what device we are dealing with and
does not make the code significantly shorter.

Thanks.

-- 
Dmitry

Re: [PATCH v2 4/4] iio: adc: ti-ads7950: complete conversion to using managed resources

[Andy Shevchenko]

On Fri, Feb 20, 2026 at 04:09:47PM -0800, Dmitry Torokhov wrote:
> On Thu, Feb 19, 2026 at 09:59:35AM +0200, Andy Shevchenko wrote:
> > On Wed, Feb 18, 2026 at 06:29:28PM -0800, Dmitry Torokhov wrote:

...

> > > +	if (error) {
> > >  		dev_err(&spi->dev, "Error in spi setup\n");
> > > -		return ret;
> > > +		return error;
> > >  	}
> > 
> > And since there is already dev_err_probe() in use, I would expect this
> > also be converted.
> 
> But that would be unrelated change ;)

I never told that this should be done here.

...

> > Would be also nice to use
> > 
> > 	struct device *dev = &spi->dev;
> > 
> > to make less LoCs and/or make them shorter.
> 
> I actually dislike introducing such temporaries in majority of the
> cases. It makes it hard to follow what device we are dealing with and
> does not make the code significantly shorter.

May be, but my experience is telling me different story.
So we have a disagreement here, I leave it to Jonathan
to mediate.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 4/4] iio: adc: ti-ads7950: complete conversion to using managed resources

[Jonathan Cameron]

On Sun, 22 Feb 2026 21:12:17 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Fri, Feb 20, 2026 at 04:09:47PM -0800, Dmitry Torokhov wrote:
> > On Thu, Feb 19, 2026 at 09:59:35AM +0200, Andy Shevchenko wrote:  
> > > On Wed, Feb 18, 2026 at 06:29:28PM -0800, Dmitry Torokhov wrote:  
> 
> ...
> 
> > > > +	if (error) {
> > > >  		dev_err(&spi->dev, "Error in spi setup\n");
> > > > -		return ret;
> > > > +		return error;
> > > >  	}  
> > > 
> > > And since there is already dev_err_probe() in use, I would expect this
> > > also be converted.  
> > 
> > But that would be unrelated change ;)  
> 
> I never told that this should be done here.
> 
> ...
> 
> > > Would be also nice to use
> > > 
> > > 	struct device *dev = &spi->dev;
> > > 
> > > to make less LoCs and/or make them shorter.  
> > 
> > I actually dislike introducing such temporaries in majority of the
> > cases. It makes it hard to follow what device we are dealing with and
> > does not make the code significantly shorter.  
> 
> May be, but my experience is telling me different story.
> So we have a disagreement here, I leave it to Jonathan
> to mediate.
> 
When there are multiple struct device isntances that we access
in the driver I fully agree with Dmitry that it is helpful to
fully enumerate them.  However, for IIO drivers we keep the
one buried in struct iio_dev fairly well hidden so the confusion
"opportunity" doesn't tend to occur.  As such I tend to come down just
on the "have a local variable" side of things.

Jonathan

Re: [PATCH v2 4/4] iio: adc: ti-ads7950: complete conversion to using managed resources

[David Lechner]

On 2/18/26 8:29 PM, Dmitry Torokhov wrote:
> All resources that the driver needs have managed API now. Switch to
> using them to make code clearer and drop ti_ads7950_remove().
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/iio/adc/ti-ads7950.c | 98 +++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 58 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index d31397f37ec4..1c53e000bdcc 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -528,19 +528,26 @@ static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
>  	return 0;
>  }
>  
> +static void ti_ads7950_power_off(void *data)
> +{
> +	struct ti_ads7950_state *st = data;
> +
> +	regulator_disable(st->reg);
> +}
> +

For the regulator part, we can simplify this even more.

The part where we call regulator_get_voltage(st->reg); in
ti_ads7950_get_range() doesn't actually need to be done there.
It was something that I just naively copied from another driver.
(This was my first IIO driver after all!)

Instead, we can use devm_regulator_get_enable_read_voltage()
in the probe function and just store the voltage in
struct ti_ads7950_state.

I would do this in a separate patch first, then the rest of of
the devm stuff after that.


>  static int ti_ads7950_probe(struct spi_device *spi)
>  {
>  	struct ti_ads7950_state *st;
>  	struct iio_dev *indio_dev;
>  	const struct ti_ads7950_chip_info *info;
> -	int ret;
> +	int error;

As in the other patches, please do not rename ret. It is adding noise
in the diff. (And I like ret better anyway.)

Re: [PATCH v2 4/4] iio: adc: ti-ads7950: complete conversion to using managed resources

[Jonathan Cameron]

On Wed, 18 Feb 2026 18:29:28 -0800
Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:

> All resources that the driver needs have managed API now. Switch to
> using them to make code clearer and drop ti_ads7950_remove().
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Hi Dmitry

One additional comment from me.

>  static int ti_ads7950_probe(struct spi_device *spi)
>  {

>  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> @@ -598,36 +605,36 @@ static int ti_ads7950_probe(struct spi_device *spi)
>  	mutex_init(&st->slock);
>  
>  	st->reg = devm_regulator_get(&spi->dev, "vref");
> -	if (IS_ERR(st->reg)) {
> -		ret = dev_err_probe(&spi->dev, PTR_ERR(st->reg),
> +	error = PTR_ERR_OR_ZERO(st->reg);

To me this reads worse than original IS_ERR() / PTR_ERR() pair.

> +	if (error)
> +		return dev_err_probe(&spi->dev, error,
>  				     "Failed to get regulator \"vref\"\n");
> -		goto error_destroy_mutex;
> -	}
>  
> -	ret = regulator_enable(st->reg);
> -	if (ret) {
> -		dev_err(&spi->dev, "Failed to enable regulator \"vref\"\n");
> -		goto error_destroy_mutex;
> -	}
> +	error = regulator_enable(st->reg);
> +	if (error)
> +		return dev_err_probe(&spi->dev, error,
> +				     "Failed to enable regulator \"vref\"\n");

Re: [PATCH v2 4/4] iio: adc: ti-ads7950: complete conversion to using managed resources

[Dmitry Torokhov]

On Sun, Feb 22, 2026 at 02:09:23PM +0000, Jonathan Cameron wrote:
> On Wed, 18 Feb 2026 18:29:28 -0800
> Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> 
> > All resources that the driver needs have managed API now. Switch to
> > using them to make code clearer and drop ti_ads7950_remove().
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> 
> Hi Dmitry
> 
> One additional comment from me.
> 
> >  static int ti_ads7950_probe(struct spi_device *spi)
> >  {
> 
> >  	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > @@ -598,36 +605,36 @@ static int ti_ads7950_probe(struct spi_device *spi)
> >  	mutex_init(&st->slock);
> >  
> >  	st->reg = devm_regulator_get(&spi->dev, "vref");
> > -	if (IS_ERR(st->reg)) {
> > -		ret = dev_err_probe(&spi->dev, PTR_ERR(st->reg),
> > +	error = PTR_ERR_OR_ZERO(st->reg);
> 
> To me this reads worse than original IS_ERR() / PTR_ERR() pair.

OK, I'll keep that in mind. It is no longer there anyways after
converting to devm_regulator_get_enable_read_voltage() that David
suggested.

Thanks.

-- 
Dmitry