Re: [PATCH v2 5/5] iio: light: si1133: use guard(mutex)() macro

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 28 Apr 2026 15:18:37 +0200
Joshua Crofts via B4 Relay <devnull+joshua.crofts1.gmail.com@kernel.org> wrote:

> From: Joshua Crofts <joshua.crofts1@gmail.com>
> 
> Remove mutex_lock()/mutex_unlock() and goto instances and add
> guard(mutex)() macro to modernize driver and improve mutex handling.
> 
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
One comment that is more about the existing code than the changes here, but
as you are touching it nice to clean that bit up!

Nice series,

Jonathan

> ---
>  drivers/iio/light/si1133.c | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/iio/light/si1133.c b/drivers/iio/light/si1133.c
> index f2a79675c026c86bb56d85a65cd035d7540d0ef8..3b40defda383bcbe731dc0b413903e0267f03b01 100644
> --- a/drivers/iio/light/si1133.c
> +++ b/drivers/iio/light/si1133.c
> @@ -8,6 +8,7 @@
>  
>  #include <linux/array_size.h>
>  #include <linux/bitops.h>
> +#include <linux/cleanup.h>
>  #include <linux/completion.h>
>  #include <linux/delay.h>
>  #include <linux/dev_printk.h>
> @@ -392,7 +393,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
>  	int err;
>  	int expected_seq;
>  
> -	mutex_lock(&data->mutex);
> +	guard(mutex)(&data->mutex);
>  
>  	expected_seq = (data->rsp_seq + 1) & SI1133_MAX_CMD_CTR;
>  
> @@ -403,19 +404,17 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
>  	if (err) {
>  		dev_warn(dev, "Failed to write command 0x%02x, ret=%d\n", cmd,
>  			 err);
> -		goto out;
> +		return err;
>  	}
>  
>  	if (cmd == SI1133_CMD_FORCE) {
>  		/* wait for irq */
>  		if (!wait_for_completion_timeout(&data->completion,
> -			msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS))) {
> -			err = -ETIMEDOUT;
> -			goto out;
> -		}
> +			msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS)))
> +			return -ETIMEDOUT;
This is some unfortunate indentation from readability point of view.
I'd reindent and not worry about the long line - it's very long but still under 100 so
tools won't complain and I think it is worth the pain here!

>  		if (!wait_for_completion_timeout(&data->completion,
						 msecs_to_jiffies(SI1133_COMPLETION_TIMEOUT_MS)))
			return -ETIMEDOUT;

or perhaps better yet, use a local variable for that timeout.


>  		err = regmap_read(data->regmap, SI1133_REG_RESPONSE0, &resp);
>  		if (err)
> -			goto out;
> +			return err;
>  	} else {
>  		err = regmap_read_poll_timeout(data->regmap,
>  					       SI1133_REG_RESPONSE0, resp,
> @@ -428,7 +427,7 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
>  			dev_warn(dev,
>  				 "Failed to read command 0x%02x, ret=%d\n",
>  				 cmd, err);
> -			goto out;
> +			return err;
>  		}
>  	}
>  
> @@ -439,9 +438,6 @@ static int si1133_command(struct si1133_data *data, u8 cmd)
>  		data->rsp_seq = expected_seq;
>  	}
>  
> -out:
> -	mutex_unlock(&data->mutex);
> -
>  	return err;
>  }
>  
>