Re: [PATCH v2 2/5] iio: light: tcs3472: convert remaining locking to guard(mutex)

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 13 May 2026 00:32:12 +0200
Aldo Conte <aldocontelk@gmail.com> wrote:

> Convert the locking in tcs3472_read_event(), tcs3472_write_event(),
> tcs3472_read_event_config(), tcs3472_write_event_config(),
> tcs3472_powerdown() and tcs3472_resume() to use guard(mutex)
> instead of explicit mutex_lock()/mutex_unlock() pairs.
I'd skip the list.  If anyone cares which functions are changed then
they can read the code!

Convert several functions to use guard(mutex)()

> 
> This avoids manual unlock calls on each return path, drops the goto
> in tcs3472_write_event(), and removes 'ret' variables only needed to
> return after the unlock.
> 
> No functional change.
> 
> Suggested-by: Andy Shevchenko <andy@kernel.org>
> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
A couple of places where the use of guard() enables some nice cleanup.
Given it's the guard() that let's us do it I consider it part of the same
change.

Also one stray change in here that has nothing to do with the rest.

> ---
> v2:
> (Suggested by Andy)
> - Moved earlier in the series to avoid introducing manual locking in the devm
>   patch that would then be converted here.
> - Dropped "found by code inspection" (this is a cleanup, not a bug)
> - Test details moved to cover letter
> 
>  drivers/iio/light/tcs3472.c | 59 +++++++++++++------------------------
>  1 file changed, 21 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 9b0f64cd9c50..4fd4fd74d0d6 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c

>  
>  static int tcs3472_read_event_config(struct iio_dev *indio_dev,
> @@ -310,13 +300,10 @@ static int tcs3472_read_event_config(struct iio_dev *indio_dev,
>  	enum iio_event_direction dir)
>  {
>  	struct tcs3472_data *data = iio_priv(indio_dev);
> -	int ret;
>  
> -	mutex_lock(&data->lock);
> -	ret = !!(data->enable & TCS3472_ENABLE_AIEN);
> -	mutex_unlock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
> -	return ret;
> +	return !!(data->enable & TCS3472_ENABLE_AIEN);

Hmm. Kind of unrelated, but given the code is changing can we make this a FIELD_GET()
Linus got quite grumpy a few years ago about readability of !!.
Alternative would ? 1 : 0;

>  }
>  
>  static int tcs3472_write_event_config(struct iio_dev *indio_dev,
> @@ -327,7 +314,7 @@ static int tcs3472_write_event_config(struct iio_dev *indio_dev,
>  	int ret = 0;
>  	u8 enable_old;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	enable_old = data->enable;
>  
> @@ -342,7 +329,6 @@ static int tcs3472_write_event_config(struct iio_dev *indio_dev,
>  		if (ret)
>  			data->enable = enable_old;
		if (ret) {
			data->enable = enable_old;
			return ret;
		}
>  	}
> -	mutex_unlock(&data->lock);
>  
>  	return ret;

	return 0;

This one is a case of reducing how far we have to look to see what happens
in each code path. Fine in this patch as it's the guard() enabling it.

>  }
> @@ -379,6 +365,7 @@ static irqreturn_t tcs3472_trigger_handler(int irq, void *p)
>  	} scan = { };
>  
>  	int ret = tcs3472_req_data(data);
> +
Unrelated change so doesn't belong in a patch doing guard() stuff.

>  	if (ret < 0)
>  		goto done;
>  
> @@ -544,15 +531,13 @@ static int tcs3472_powerdown(struct tcs3472_data *data)
>  	int ret;
>  	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  		data->enable & ~enable_mask);
>  	if (!ret)

As below.

>  		data->enable &= ~enable_mask;
>  
> -	mutex_unlock(&data->lock);
> -
>  	return ret;
>  }
>  
> @@ -581,15 +566,13 @@ static int tcs3472_resume(struct device *dev)
>  	int ret;
>  	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
>  
> -	mutex_lock(&data->lock);
> +	guard(mutex)(&data->lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
>  		data->enable | enable_mask);
>  	if (!ret)
>  		data->enable |= enable_mask;
	
	if (ret)
		return ret;

	data->enable |= enable_mask;

	return 0;

One of the nicest things about cleanup.h magic is avoids us needing
to do handling of good paths 'out of line'.  Thus we can switch to
more ideomatic code that is easier to read. That can be done as
part of this change as it's the guard() stuff that made it possible.

Jonathan

>  
> -	mutex_unlock(&data->lock);
> -
>  	return ret;
>  }
>