Re: [PATCH v3 6/7] iio: light: opt3001: move driver to guard(mutex)() use

[Joshua Crofts]

On Thu, 21 May 2026 at 11:55, Joshua Crofts via B4 Relay
<devnull+joshua.crofts1.gmail.com@xxxxxxxxxx> wrote:
>
> From: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
>
> Move driver to use guard(mutex)() macro, to facilitate automatic
> locking/unlocking of resources. This modernizes the driver and
> improves code style.
>
> While at it, remove unnecessary gotos and return variables.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>
> Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> ---
>  drivers/iio/light/opt3001.c | 140 +++++++++++++++++++-------------------------
>  1 file changed, 60 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index d5262488dac889066a53d3234762e0557f9e3cbc..ac1c7ba5679867f6b814894680e317615f2e373b 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -10,6 +10,7 @@
>
>  #include <linux/array_size.h>
>  #include <linux/bits.h>
> +#include <linux/cleanup.h>
>  #include <linux/delay.h>
>  #include <linux/dev_printk.h>
>  #include <linux/errno.h>
> @@ -477,7 +478,6 @@ static int opt3001_read_raw(struct iio_dev *iio,
>                             int *val, int *val2, long mask)
>  {
>         struct opt3001 *opt = iio_priv(iio);
> -       int ret;
>
>         if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
>                 return -EBUSY;
> @@ -485,23 +485,17 @@ static int opt3001_read_raw(struct iio_dev *iio,
>         if (chan->type != opt->chip_info->chan_type)
>                 return -EINVAL;
>
> -       mutex_lock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
>         switch (mask) {
>         case IIO_CHAN_INFO_RAW:
>         case IIO_CHAN_INFO_PROCESSED:
> -               ret = opt3001_get_processed(opt, val, val2);
> -               break;
> +               return opt3001_get_processed(opt, val, val2);
>         case IIO_CHAN_INFO_INT_TIME:
> -               ret = opt3001_get_int_time(opt, val, val2);
> -               break;
> +               return opt3001_get_int_time(opt, val, val2);
>         default:
> -               ret = -EINVAL;
> +               return -EINVAL;
>         }
> -
> -       mutex_unlock(&opt->lock);
> -
> -       return ret;
>  }
>
>  static int opt3001_write_raw(struct iio_dev *iio,
> @@ -509,7 +503,6 @@ static int opt3001_write_raw(struct iio_dev *iio,
>                              int val, int val2, long mask)
>  {
>         struct opt3001 *opt = iio_priv(iio);
> -       int ret;
>
>         if (opt->mode == OPT3001_CONFIGURATION_M_CONTINUOUS)
>                 return -EBUSY;
> @@ -523,11 +516,9 @@ static int opt3001_write_raw(struct iio_dev *iio,
>         if (val != 0)
>                 return -EINVAL;
>
> -       mutex_lock(&opt->lock);
> -       ret = opt3001_set_int_time(opt, val2);
> -       mutex_unlock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
> -       return ret;
> +       return opt3001_set_int_time(opt, val2);
>  }
>
>  static int opt3001_read_event_value(struct iio_dev *iio,
> @@ -538,28 +529,23 @@ static int opt3001_read_event_value(struct iio_dev *iio,
>                                     int *val, int *val2)
>  {
>         struct opt3001 *opt = iio_priv(iio);
> -       int ret = IIO_VAL_INT_PLUS_MICRO;
>
> -       mutex_lock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
>         switch (dir) {
>         case IIO_EV_DIR_RISING:
>                 opt3001_to_iio_ret(opt, opt->high_thresh_exp,
>                                    opt->high_thresh_mantissa,
>                                    val, val2);
> -               break;
> +               return IIO_VAL_INT_PLUS_MICRO;
>         case IIO_EV_DIR_FALLING:
>                 opt3001_to_iio_ret(opt, opt->low_thresh_exp,
>                                    opt->low_thresh_mantissa,
>                                    val, val2);
> -               break;
> +               return IIO_VAL_INT_PLUS_MICRO;
>         default:
> -               ret = -EINVAL;
> +               return -EINVAL;
>         }
> -
> -       mutex_unlock(&opt->lock);
> -
> -       return ret;
>  }
>
>  static int opt3001_write_event_value(struct iio_dev *iio,
> @@ -586,12 +572,12 @@ static int opt3001_write_event_value(struct iio_dev *iio,
>         if (val < 0)
>                 return -EINVAL;
>
> -       mutex_lock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
>         ret = opt3001_find_scale(opt, val, val2, &exponent);
>         if (ret < 0) {
>                 dev_err(dev, "can't find scale for %d.%06u\n", val, val2);
> -               goto err;
> +               return ret;
>         }
>
>         whole = opt->chip_info->factor_whole;
> @@ -614,20 +600,16 @@ static int opt3001_write_event_value(struct iio_dev *iio,
>                 opt->low_thresh_exp = exponent;
>                 break;
>         default:
> -               ret = -EINVAL;
> -               goto err;
> +               return -EINVAL;
>         }
>
>         ret = i2c_smbus_write_word_swapped(client, reg, value);
>         if (ret < 0) {
>                 dev_err(dev, "failed to write register %02x\n", reg);
> -               goto err;
> +               return ret;
>         }
>
> -err:
> -       mutex_unlock(&opt->lock);
> -
> -       return ret;
> +       return 0;
>  }
>
>  static int opt3001_read_event_config(struct iio_dev *iio,
> @@ -659,7 +641,7 @@ static int opt3001_write_event_config(struct iio_dev *iio,
>         if (!state && opt->mode == OPT3001_CONFIGURATION_M_SHUTDOWN)
>                 return 0;
>
> -       mutex_lock(&opt->lock);
> +       guard(mutex)(&opt->lock);
>
>         mode = state ? OPT3001_CONFIGURATION_M_CONTINUOUS
>                 : OPT3001_CONFIGURATION_M_SHUTDOWN;
> @@ -668,7 +650,7 @@ static int opt3001_write_event_config(struct iio_dev *iio,
>         if (ret < 0) {
>                 dev_err(dev, "failed to read register %02x\n",
>                         OPT3001_CONFIGURATION);
> -               goto err;
> +               return ret;
>         }
>
>         reg = ret;
> @@ -678,13 +660,10 @@ static int opt3001_write_event_config(struct iio_dev *iio,
>         if (ret < 0) {
>                 dev_err(dev, "failed to write register %02x\n",
>                         OPT3001_CONFIGURATION);
> -               goto err;
> +               return ret;
>         }
>
> -err:
> -       mutex_unlock(&opt->lock);
> -
> -       return ret;
> +       return 0;
>  }
>
>  static const struct iio_info opt3001_info = {
> @@ -726,6 +705,31 @@ static int opt3001_read_id(struct opt3001 *opt)
>         return 0;
>  }
>
> +static void opt3001_power_off(void *data)
> +{
> +       struct opt3001 *opt = data;
> +       struct i2c_client *client = opt->client;
> +       struct device *dev = opt->dev;
> +       u16 reg_val;
> +       int ret;
> +
> +       ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
> +       if (ret < 0) {
> +               dev_err(dev, "failed to read register %02x\n",
> +                       OPT3001_CONFIGURATION);
> +               return;
> +       }
> +
> +       reg_val = ret;
> +       opt3001_set_mode(opt, &reg_val, OPT3001_CONFIGURATION_M_SHUTDOWN);
> +
> +       ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION,
> +                                          reg_val);
> +       if (ret < 0)
> +               dev_err(dev, "failed to write to register %02x\n",
> +                       OPT3001_CONFIGURATION);
> +}
> +
>  static int opt3001_configure(struct opt3001 *opt)
>  {
>         struct i2c_client *client = opt->client;
> @@ -764,6 +768,11 @@ static int opt3001_configure(struct opt3001 *opt)
>                 return dev_err_probe(dev, ret, "failed to write register %02x\n",
>                                      OPT3001_CONFIGURATION);
>
> +       ret = devm_add_action_or_reset(dev, opt3001_power_off, opt);
> +       if (ret)
> +               return dev_err_probe(dev, ret,
> +                                    "failed to register power off function\n");
> +
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_LOW_LIMIT);
>         if (ret < 0)
>                 return dev_err_probe(dev, ret, "failed to read register %02x\n",
> @@ -857,7 +866,10 @@ static int opt3001_probe(struct i2c_client *client)
>         opt->client = client;
>         opt->chip_info = i2c_get_match_data(client);
>
> -       mutex_init(&opt->lock);
> +       ret = devm_mutex_init(dev, &opt->lock);
> +       if (ret)
> +               return ret;
> +
>         init_waitqueue_head(&opt->result_ready_queue);
>         i2c_set_clientdata(client, iio);
>
> @@ -879,49 +891,18 @@ static int opt3001_probe(struct i2c_client *client)
>
>         /* Make use of INT pin only if valid IRQ no. is given */
>         if (irq > 0) {
> -               ret = request_threaded_irq(irq, NULL, opt3001_irq,
> -                                          IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> -                                          "opt3001", iio);
> +               ret = devm_request_threaded_irq(dev, irq, NULL, opt3001_irq,
> +                                               IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +                                               "opt3001", iio);

Nope nope nope, disregard this patch, obviously I've merged this incorrectly
so that's another reason for a v4 (perhaps some patches could get picked up
though).

-- 
Kind regards

CJD