linux-kernel - Re: [PATCH v2] iio: light: ltr390: Add device powerdown functionality via devm api

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHp75VfPNDzvxu8znHn45iS9Db_=E7A9nf2xmtVnY8cAi1HnfA@mail.gmail.com>
Date: Wed, 13 Aug 2025 12:13:02 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Akshay Jindal <akshayaj.lkd@...il.com>
Cc: anshulusr@...il.com, jic23@...nel.org, dlechner@...libre.com, 
	nuno.sa@...log.com, andy@...nel.org, shuah@...nel.org, 
	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] iio: light: ltr390: Add device powerdown functionality
 via devm api

On Tue, Aug 12, 2025 at 7:07 PM Akshay Jindal <akshayaj.lkd@...il.com> wrote:
>
> Use devm_add_action_or_reset to do cleanup when the device is removed.

devm_add_action_or_reset()

> Set client data with i2c_set_clientdata() to ensure indio_dev is accessible
> in powerdown function.

...

> +static void ltr390_powerdown(void *client)
> +{
> +       struct i2c_client *i2c_clnt = client;
> +       struct iio_dev *indio_dev = i2c_get_clientdata(i2c_clnt);

Why? I mean why can't you simply provide a data pointer directly as a
parameter to this function?

> +       struct ltr390_data *data = iio_priv(indio_dev);
> +
> +       guard(mutex)(&data->lock);
> +
> +       /* Ensure that power off and interrupts are disabled */
> +       if (regmap_clear_bits(data->regmap, LTR390_INT_CFG,
> +                               LTR390_LS_INT_EN) < 0)
> +               dev_err(&i2c_clnt->dev, "failed to disable interrupts\n");
> +
> +       if (regmap_clear_bits(data->regmap, LTR390_MAIN_CTRL,
> +                       LTR390_SENSOR_ENABLE) < 0)
> +               dev_err(&i2c_clnt->dev, "failed to disable sensor\n");
> +}

...

> @@ -693,7 +711,7 @@ static int ltr390_probe(struct i2c_client *client)
>                 return -ENOMEM;
>
>         data = iio_priv(indio_dev);

> -

Stray change.

> +       i2c_set_clientdata(client, indio_dev);

I would suggest moving this to be before data = ... line above. And
add a blank line after it. BUT, read above first.

>         data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
>         if (IS_ERR(data->regmap))
>                 return dev_err_probe(dev, PTR_ERR(data->regmap),

-- 
With Best Regards,
Andy Shevchenko