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

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 <[email protected]>
> ---

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