Re: [PATCH v3 5/7] iio: light: opt3001: prefer dev_err_probe()

[Maxwell Doose]

Hi Joshua,

On Thu, May 21, 2026 at 5:13 AM Joshua Crofts via B4 Relay
<devnull+joshua.crofts1.gmail.com@xxxxxxxxxx> wrote:
>
> From: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
>
> Switch driver to use dev_err_probe() to unify error messages generated
> in *_probe() and probe path functions.
>
> Signed-off-by: Joshua Crofts <joshua.crofts1@xxxxxxxxx>
> ---
>  drivers/iio/light/opt3001.c | 56 ++++++++++++++++++---------------------------
>  1 file changed, 22 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/iio/light/opt3001.c b/drivers/iio/light/opt3001.c
> index 22a73c1afa74988dabf9db10c3b55bd07529fb20..d5262488dac889066a53d3234762e0557f9e3cbc 100644
> --- a/drivers/iio/light/opt3001.c
> +++ b/drivers/iio/light/opt3001.c
> @@ -706,21 +706,17 @@ static int opt3001_read_id(struct opt3001 *opt)
>         int ret;
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_MANUFACTURER_ID);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_MANUFACTURER_ID);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_MANUFACTURER_ID);
>
>         manufacturer[0] = ret >> 8;
>         manufacturer[1] = ret & 0xff;
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_DEVICE_ID);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_DEVICE_ID);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_DEVICE_ID);
>
>         device_id = ret;
>
> @@ -738,11 +734,9 @@ static int opt3001_configure(struct opt3001 *opt)
>         u16 reg;
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_CONFIGURATION);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_CONFIGURATION);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_CONFIGURATION);
>
>         reg = ret;
>
> @@ -766,28 +760,22 @@ static int opt3001_configure(struct opt3001 *opt)
>         reg &= ~OPT3001_CONFIGURATION_FC_MASK;
>
>         ret = i2c_smbus_write_word_swapped(client, OPT3001_CONFIGURATION, reg);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to write register %02x\n",
> -                       OPT3001_CONFIGURATION);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to write register %02x\n",
> +                                    OPT3001_CONFIGURATION);
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_LOW_LIMIT);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_LOW_LIMIT);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_LOW_LIMIT);
>
>         opt->low_thresh_mantissa = OPT3001_REG_MANTISSA(ret);
>         opt->low_thresh_exp = OPT3001_REG_EXPONENT(ret);
>
>         ret = i2c_smbus_read_word_swapped(client, OPT3001_HIGH_LIMIT);
> -       if (ret < 0) {
> -               dev_err(dev, "failed to read register %02x\n",
> -                       OPT3001_HIGH_LIMIT);
> -               return ret;
> -       }
> +       if (ret < 0)
> +               return dev_err_probe(dev, ret, "failed to read register %02x\n",
> +                                    OPT3001_HIGH_LIMIT);
>
>         opt->high_thresh_mantissa = OPT3001_REG_MANTISSA(ret);
>         opt->high_thresh_exp = OPT3001_REG_EXPONENT(ret);

Ah, but all of the above don't seem like _probe() functions. I'm not
an expert on dev_err_probe() but I'm pretty sure it's only for use (or
at least, designed for use) in _probe() functions. Only function where
I know it's 100% fine in is the one below.

> @@ -894,10 +882,10 @@ static int opt3001_probe(struct i2c_client *client)
>                 ret = request_threaded_irq(irq, NULL, opt3001_irq,
>                                            IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>                                            "opt3001", iio);
> -               if (ret) {
> -                       dev_err(dev, "failed to request IRQ #%d\n", irq);
> -                       return ret;
> -               }
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                            "failed to request IRQ #%d\n",
> +                                            irq);
>                 opt->use_irq = true;
>         } else {
>                 dev_dbg(dev, "enabling interrupt-less operation\n");
>
> --
> 2.47.3
>

Regardless, I'd wait for Jonathan or Andy to give their comments on
this before you make any decisions regarding which ones to switch over
(I also haven't looked at the v1 or v2 so I don't know whether or not
they recommended you put dev_err_probe() in the non-_probe()
functions).

best regards,
max