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

Andy Shevchenko
2026-05-06T10:00:44Z

On Wed, May 06, 2026 at 11:43:09AM +0200, Aldo Conte wrote:

> Found by code inspection.

It's unusual to start a commit message with this...
Move it to the end of the commit message (before tag block).

> 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.

> Compiled with `make drivers/iio/light/tcs3472.o W=1`.

Unneeded detail, move to the comments / cover letter.

> Tested on Raspberry Pi 3B with TCS3472 (Adafruit breakout):
> Verified that RGBC channel reads return valid data and that ENABLE
> register is cleared to 0x00 on device removal.

Ditto.

> Signed-off-by: Aldo Conte <aldocontelk@gmail.com>
> ---

FYI: Here is the location for the comments / changelog / et cetera.

> drivers/iio/light/tcs3472.c | 85 ++++++++++++++++---------------------

...

> +static int tcs3472_powerdown(struct tcs3472_data *data)
> +{
> + int ret;
> + u8 enable_mask = TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON;
> +
> + mutex_lock(&data->lock);
> +
> + ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE,
> + data->enable & ~enable_mask);
> + if (!ret)
> + data->enable &= ~enable_mask;

This patter is discouraged.

> + mutex_unlock(&data->lock);
> +
> + return ret;
> +}

Make the guard()() patch to be first in the series. With that being done and
taking the above this becomes as simple as

static int tcs3472_powerdown(struct tcs3472_data *data)
{
u8 value = data->enable & ~(TCS3472_ENABLE_AEN | TCS3472_ENABLE_PON);
int ret;

guard(mutex)(&data->lock);

ret = i2c_smbus_write_byte_data(data->client, TCS3472_ENABLE, value);
if (ret)
return ret;

data->enable = value;
return 0;
}

...

> + ret = devm_add_action_or_reset(&client->dev,

With temporary

struct device *dev = &cliend->dev;

at the top of the function this and others become easier to read.

> + tcs3472_powerdown_action, data);
> + if (ret)
> + return ret;

--
With Best Regards,
Andy Shevchenko