[v4] ti-ads7950: fix gpio handling and facelift

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

Posted by Dmitry Torokhov 3 days, 10 hours ago

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

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

diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
index 028acd42741f..6e9ea9cc33bf 100644
--- a/drivers/iio/adc/ti-ads7950.c
+++ b/drivers/iio/adc/ti-ads7950.c
@@ -299,18 +299,19 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
 	struct ti_ads7950_state *st = iio_priv(indio_dev);
 	int ret;
 
-	mutex_lock(&st->slock);
-	ret = spi_sync(st->spi, &st->ring_msg);
-	if (ret < 0)
-		goto out;
-
-	iio_push_to_buffers_with_ts_unaligned(indio_dev, &st->rx_buf[2],
-					      sizeof(*st->rx_buf) *
-					      TI_ADS7950_MAX_CHAN,
-					      iio_get_time_ns(indio_dev));
-
-out:
-	mutex_unlock(&st->slock);
+	do {
+		guard(mutex)(&st->slock);
+
+		ret = spi_sync(st->spi, &st->ring_msg);
+		if (ret)
+			break;
+
+		iio_push_to_buffers_with_ts_unaligned(indio_dev, &st->rx_buf[2],
+						      sizeof(*st->rx_buf) *
+						      TI_ADS7950_MAX_CHAN,
+						      iio_get_time_ns(indio_dev));
+	} while (0);
+
 	iio_trigger_notify_done(indio_dev->trig);
 
 	return IRQ_HANDLED;
@@ -321,20 +322,16 @@ 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;
 
-	mutex_lock(&st->slock);
+	guard(mutex)(&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);
+		return ret;
 
-	return ret;
+	return st->single_rx;
 }
 
 static int ti_ads7950_get_range(struct ti_ads7950_state *st)
@@ -400,9 +397,8 @@ 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;
 
-	mutex_lock(&st->slock);
+	guard(mutex)(&st->slock);
 
 	if (value)
 		st->cmd_settings_bitmask |= BIT(offset);
@@ -410,11 +406,8 @@ 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);
 
-	return ret;
+	return spi_sync(st->spi, &st->scan_single_msg);
 }
 
 static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
@@ -423,13 +416,12 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
 	bool state;
 	int ret;
 
-	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);
-		ret = 0;
-		goto out;
+		return state;
 	}
 
 	/* GPIO data bit sets SDO bits 12-15 to GPIO input */
@@ -437,7 +429,7 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
 	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
 	ret = spi_sync(st->spi, &st->scan_single_msg);
 	if (ret)
-		goto out;
+		return ret;
 
 	state = (st->single_rx >> 12) & BIT(offset);
 
@@ -446,12 +438,9 @@ static int ti_ads7950_get(struct gpio_chip *chip, unsigned int offset)
 	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);
+		return ret;
 
-	return ret ?: state;
+	return state;
 }
 
 static int ti_ads7950_get_direction(struct gpio_chip *chip,
@@ -467,9 +456,8 @@ 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;
 
-	mutex_lock(&st->slock);
+	guard(mutex)(&st->slock);
 
 	/* Only change direction if needed */
 	if (input && (st->gpio_cmd_settings_bitmask & BIT(offset)))
@@ -477,15 +465,11 @@ 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);
-
-out:
-	mutex_unlock(&st->slock);
 
-	return ret;
+	return spi_sync(st->spi, &st->scan_single_msg);
 }
 
 static int ti_ads7950_direction_input(struct gpio_chip *chip,
@@ -508,9 +492,9 @@ 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 ret;
 
-	mutex_lock(&st->slock);
+	guard(mutex)(&st->slock);
 
 	/* Settings for Manual/Auto1/Auto2 commands */
 	/* Default to 5v ref */
@@ -518,17 +502,12 @@ static int ti_ads7950_init_hw(struct ti_ads7950_state *st)
 	st->single_tx = TI_ADS7950_MAN_CMD_SETTINGS(st);
 	ret = spi_sync(st->spi, &st->scan_single_msg);
 	if (ret)
-		goto out;
+		return ret;
 
 	/* 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);
-
-	return ret;
+	return spi_sync(st->spi, &st->scan_single_msg);
 }
 
 static int ti_ads7950_probe(struct spi_device *spi)

-- 
2.53.0.1018.g2bb0e51243-goog

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

Posted by Bartosz Golaszewski 3 days ago

On Mon, 30 Mar 2026 00:47:06 +0200, Dmitry Torokhov
<dmitry.torokhov@gmail.com> said:
> guard() notation allows early returns when encountering errors, making
> control flow more obvious. Use it.
>
> Reviewed-by: David Lechner <dlechner@baylibre.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/iio/adc/ti-ads7950.c | 83 +++++++++++++++++---------------------------
>  1 file changed, 31 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
> index 028acd42741f..6e9ea9cc33bf 100644
> --- a/drivers/iio/adc/ti-ads7950.c
> +++ b/drivers/iio/adc/ti-ads7950.c
> @@ -299,18 +299,19 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>  	struct ti_ads7950_state *st = iio_priv(indio_dev);
>  	int ret;
>
> -	mutex_lock(&st->slock);
> -	ret = spi_sync(st->spi, &st->ring_msg);
> -	if (ret < 0)
> -		goto out;
> -
> -	iio_push_to_buffers_with_ts_unaligned(indio_dev, &st->rx_buf[2],
> -					      sizeof(*st->rx_buf) *
> -					      TI_ADS7950_MAX_CHAN,
> -					      iio_get_time_ns(indio_dev));
> -
> -out:
> -	mutex_unlock(&st->slock);
> +	do {
> +		guard(mutex)(&st->slock);

Am I missing something? Why isn't it just a:

	scoped_guard(mutex, &st->slock) {
		...
	}

?

Bart

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

Posted by Dmitry Torokhov 3 days ago

On March 30, 2026 2:15:42 AM PDT, Bartosz Golaszewski <brgl@kernel.org> wrote:
>On Mon, 30 Mar 2026 00:47:06 +0200, Dmitry Torokhov
><dmitry.torokhov@gmail.com> said:
>> guard() notation allows early returns when encountering errors, making
>> control flow more obvious. Use it.
>>
>> Reviewed-by: David Lechner <dlechner@baylibre.com>
>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> ---
>>  drivers/iio/adc/ti-ads7950.c | 83 +++++++++++++++++---------------------------
>>  1 file changed, 31 insertions(+), 52 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>> index 028acd42741f..6e9ea9cc33bf 100644
>> --- a/drivers/iio/adc/ti-ads7950.c
>> +++ b/drivers/iio/adc/ti-ads7950.c
>> @@ -299,18 +299,19 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>>  	struct ti_ads7950_state *st = iio_priv(indio_dev);
>>  	int ret;
>>
>> -	mutex_lock(&st->slock);
>> -	ret = spi_sync(st->spi, &st->ring_msg);
>> -	if (ret < 0)
>> -		goto out;
>> -
>> -	iio_push_to_buffers_with_ts_unaligned(indio_dev, &st->rx_buf[2],
>> -					      sizeof(*st->rx_buf) *
>> -					      TI_ADS7950_MAX_CHAN,
>> -					      iio_get_time_ns(indio_dev));
>> -
>> -out:
>> -	mutex_unlock(&st->slock);
>> +	do {
>> +		guard(mutex)(&st->slock);
>
>Am I missing something? Why isn't it just a:
>
>	scoped_guard(mutex, &st->slock) {
>		...
>	}

Maintainer's preference. It was a scoped guard in the first iteration. 

Thanks. 

-- 
Dmitry

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

Posted by Bartosz Golaszewski 2 days, 23 hours ago

On Mon, 30 Mar 2026 11:18:14 +0200, Dmitry Torokhov
<dmitry.torokhov@gmail.com> said:
> On March 30, 2026 2:15:42 AM PDT, Bartosz Golaszewski <brgl@kernel.org> wrote:
>>On Mon, 30 Mar 2026 00:47:06 +0200, Dmitry Torokhov
>><dmitry.torokhov@gmail.com> said:
>>> guard() notation allows early returns when encountering errors, making
>>> control flow more obvious. Use it.
>>>
>>> Reviewed-by: David Lechner <dlechner@baylibre.com>
>>> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>>> ---
>>>  drivers/iio/adc/ti-ads7950.c | 83 +++++++++++++++++---------------------------
>>>  1 file changed, 31 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/drivers/iio/adc/ti-ads7950.c b/drivers/iio/adc/ti-ads7950.c
>>> index 028acd42741f..6e9ea9cc33bf 100644
>>> --- a/drivers/iio/adc/ti-ads7950.c
>>> +++ b/drivers/iio/adc/ti-ads7950.c
>>> @@ -299,18 +299,19 @@ static irqreturn_t ti_ads7950_trigger_handler(int irq, void *p)
>>>  	struct ti_ads7950_state *st = iio_priv(indio_dev);
>>>  	int ret;
>>>
>>> -	mutex_lock(&st->slock);
>>> -	ret = spi_sync(st->spi, &st->ring_msg);
>>> -	if (ret < 0)
>>> -		goto out;
>>> -
>>> -	iio_push_to_buffers_with_ts_unaligned(indio_dev, &st->rx_buf[2],
>>> -					      sizeof(*st->rx_buf) *
>>> -					      TI_ADS7950_MAX_CHAN,
>>> -					      iio_get_time_ns(indio_dev));
>>> -
>>> -out:
>>> -	mutex_unlock(&st->slock);
>>> +	do {
>>> +		guard(mutex)(&st->slock);
>>
>>Am I missing something? Why isn't it just a:
>>
>>	scoped_guard(mutex, &st->slock) {
>>		...
>>	}
>
> Maintainer's preference. It was a scoped guard in the first iteration.
>

Fair enough, though I don't really understand that. It looks less readable this
way IMO.

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