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

[Jonathan Cameron]

On Wed, 13 May 2026 22:29:55 +0200
Aldo Conte <aldocontelk@xxxxxxxxx> wrote:

> 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 <andy@xxxxxxxxxx>
> > Signed-off-by: Aldo Conte <aldocontelk@xxxxxxxxx>
> > ---
> > 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?

Separate patch.  However, don't rush. This version wants to sit on list
for a least a few more days!  For something as complex at this set
I'd suggest at least a week between first few versions.  Maybe
accelerate if you have a bunch of tags and there is just a little thing
needed.

Jonathan

> 
> Thanks,
> Aldo
> 
> ...
> 
>