Re: [PATCH v2 3/5] iio: light: tcs3472: use devm for resource management

On 5/13/26 00:32, Aldo Conte wrote:
> Convert the driver to use device-managed resource allocation:
> - Add tcs3472_powerdown_action() and register it with
>    devm_add_action_or_reset() to ensure the device is powered down on
>    cleanup. Before this patch, the chip remained powered if probe
>    failed after enabling it.
> - Replace iio_triggered_buffer_setup() with
>    devm_iio_triggered_buffer_setup().
> - Replace request_threaded_irq() with devm_request_threaded_irq().
> - Replace iio_device_register() with devm_iio_device_register().
> - Remove tcs3472_remove() as all cleanup is now handled by devm.
> 
> Rewrite the read-modify-write pattern in tcs3472_powerdown() and
> tcs3472_resume() so the new register value is computed first, written
> to the chip, and committed to data->enable only on success.
> 
> Use a local 'dev = &client->dev' in tcs3472_probe() to keep the devm
> calls compact.
> 
> Suggested-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Aldo Conte <[email protected]>
> ---
> v2:
> (Suggested by Andy)
> - Rewrote read-modify-write in tcs3472_powerdown() and tcs3472_resume()
> - Use local 'struct device *dev = &client->dev' in probe.
> - Dropped "Compiled with W=1" and test details from the commit message.
> 
>   drivers/iio/light/tcs3472.c | 107 +++++++++++++++++-------------------
>   1 file changed, 51 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/iio/light/tcs3472.c b/drivers/iio/light/tcs3472.c
> index 4fd4fd74d0d6..7a6dc8360326 100644
> --- a/drivers/iio/light/tcs3472.c
> +++ b/drivers/iio/light/tcs3472.c
> @@ -427,13 +427,38 @@ static const struct iio_info tcs3472_info = {
>   	.attrs = &tcs3472_attribute_group,
>   };
>   
> +static int tcs3472_powerdown(struct tcs3472_data *data)
> +{
> +	u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> +	u8 value;
> +	int ret;
> +
> +	guard(mutex)(&data->lock);
> +
> +	value = data->enable & ~enable_mask;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
> +	if (ret)
> +		return ret;
> +
> +	data->enable = value;
> +
> +	return 0;
> +}
> +
> +static void tcs3472_powerdown_action(void *data)
> +{
> +	tcs3472_powerdown(data);
> +}
> +
>   static int tcs3472_probe(struct i2c_client *client)
>   {
> +	struct device *dev = &client->dev;
>   	struct tcs3472_data *data;
>   	struct iio_dev *indio_dev;
>   	int ret;
>   
> -	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>   	if (indio_dev == NULL)
>   		return -ENOMEM;
> 

Hi Jonathan,

In v3 I have a small style cleanup: replacing 'if (indio_dev == NULL)'
with 'if (!indio_dev)' in tcs3472_probe(). This was suggested by
Joshua Crofts in his v2 review.

What do you prefer it is a separate patch
or folded into the devm conversion patch (which already modifies the
context next to that line).

Either way works for me. Which do you prefer?

Thanks,
Aldo

...