Message-ID: <20260123091141.7ce41a22@jic23-huawei>
Date: Fri, 23 Jan 2026 09:11:41 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Petr Hodina via B4 Relay <devnull+petr.hodina.protonmail.com@...nel.org>
Cc: petr.hodina@...tonmail.com, David Lechner <dlechner@...libre.com>, Nuno
 Sá <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Bjorn Andersson <andersson@...nel.org>,
 Konrad Dybcio <konradybcio@...nel.org>, David Heidelberg <david@...t.cz>,
 linux-iio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH 2/3] iio: light: add AMS TCS3400 RGB and RGB-IR color
 sensor driver

On Mon, 19 Jan 2026 18:19:07 +0100
Petr Hodina via B4 Relay <devnull+petr.hodina.protonmail.com@...nel.org> wrote:

> From: Petr Hodina <petr.hodina@...tonmail.com>
> 
> Add support for the AMS TCS3400 I2C color light-to-digital converter.
> The driver supports RGBC and RGB-IR modes, programmable integration
> time, optional interrupt-driven buffered capture, and regulator-based
> power control.
> 
> Signed-off-by: Petr Hodina <petr.hodina@...tonmail.com>
Hi Petr,

Seems you have a lot of reviews already.  I'll take a quick look
as well but may well overlap with others.

Thanks,

Jonathan

>  M:	Mudit Sharma <muditsharma.info@...il.com>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index ac1408d374c9..73419d80e3a7 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -580,6 +580,17 @@ config ST_UVIS25_SPI
>  	depends on ST_UVIS25
>  	select REGMAP_SPI
>  
> +config TCS3400
> +	tristate "AMS TCS3400 color light-to-digital converter"
> +	depends on I2C
> +	default n
Drop that as it's the default with nothing said.
> +	help
> +	  If you say yes here you get support for the AMS TCS3400.
> +	  This sensor can detect ambient light and color (RGB) values.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tcs3400.

> diff --git a/drivers/iio/light/tcs3400.c b/drivers/iio/light/tcs3400.c
> new file mode 100644
> index 000000000000..22c8c4e803cf
> --- /dev/null
> +++ b/drivers/iio/light/tcs3400.c
> @@ -0,0 +1,505 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * TCS3400 - AMS/TAOS color light sensor with RGBC and RGB-IR channels
> + *
> + * Copyright (c) 2025 Petr Hodina
> + *
This trailing blank line adds little other than more scrolling. Drop it.
> + */
> +
> +#include <linux/module.h>
Alphabetical order and sanity check that you are following the
include what you use philosophy for includes (see threads
on list currently about that).

> +#include <linux/i2c.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger.h>
See below for comments on this.
> +#include <linux/iio/trigger_consumer.h>
> +
> +#define TCS3400_DRV_NAME "tcs3400"
> +#define TCS3400_CMD_REG(reg) (0x80 | (reg))
> +#define TCS3400_CMD_SPECIAL 0xE0
> +#define TCS3400_CMD_ALS_INT_CLR 0xE6
> +#define TCS3400_CMD_ALL_INT_CLR 0xE7
> +#define TCS3400_ENABLE 0x00

I'd add _REG postfix to the register addresses. Makes it
easier to tell what is address and what is field..

> +#define TCS3400_ATIME 0x01
> +#define TCS3400_WTIME 0x03
> +#define TCS3400_PERSIST 0x0C
> +#define TCS3400_CONTROL 0x0F /* Gain */
> +#define TCS3400_STATUS 0x13
> +#define TCS3400_CDATAL 0x14 /* Clear low */
> +#define TCS3400_RDATAL 0x16
> +#define TCS3400_GDATAL 0x18
> +#define TCS3400_BDATAL 0x1A
> +#define TCS3400_ID 0x12
> +#define TCS3400_CHSEL 0xC0 /* Access IR channel: 0x00 RGBC, 0x80 RGB-IR */
> +#define TCS3400_EN_PON BIT(0)
Fine a way to clearly associate fields with register.
Normally repeat the prefix. So this is maybe
TCS3400_ENABLE_PON

Put the fields with each register address define and it is
worth looking at some of the indentation tweaks we often
do to make it easier to read.


> +#define TCS3400_EN_AEN BIT(1)
> +#define TCS3400_EN_AIEN BIT(4)
> +#define TCS3400_STATUS_AVALID BIT(0)
> +#define TCS3400_STATUS_AINT BIT(4)
> +#define TCS3400_GAIN_1X 0x00
> +#define TCS3400_GAIN_4X 0x01
> +#define TCS3400_GAIN_16X 0x02
> +#define TCS3400_GAIN_64X 0x03
> +#define TCS3400_MAX_ATIME 256
> +
> +#define TCS3400_IIO_CHANNEL(_index, _mod) { \
> +	.type = IIO_INTENSITY, \
> +	.modified = 1, \
> +	.channel2 = IIO_MOD_##_mod, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> +			      BIT(IIO_CHAN_INFO_SCALE), \
> +			      BIT(IIO_CHAN_INFO_SCALE) | \
> +			      BIT(IIO_CHAN_INFO_INT_TIME), \
> +	.indexed = 1, \
> +	.channel = _index, \
> +	.scan_index = _index, \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +		.endianness = IIO_LE, \

Looks wrong with a le16_to_cpu in the read path.
Arguably that shouldn't be in the buffered path, but given it's i2c and
the cost of even talking to the device is high I don't mind if that
simplifies your code.


> +	}, \
> +}
> +
> +struct tcs3400_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +	struct regulator *vdd_supply;
> +	u8 atime;
> +	u8 gain;
> +	u8 channel_mode; /* 0x00 or 0x80 */
> +	u16 clear_ir; /* clear when mode=0x00, IR when mode=0x80 */

Sounds like you should control your ir_mode based on what channel is requested
not a separate attribute that changes the meaning of the data.

> +	u16 red;
> +	u16 green;
> +	u16 blue;
> +};
> +
> +static const int tcs3400_gains[] = {1, 4, 16, 64};

 = { 1, 4, 16, 64 };

is the preferred formatting in IIO.

> +
> +static int tcs3400_power_on(struct tcs3400_data *data)
> +{
> +	int ret;
> +
> +	ret = regulator_enable(data->vdd_supply);
> +	if (ret)
> +		return ret;
> +
> +	msleep(20);
> +
> +	return 0;
> +}
> +
> +static void tcs3400_power_off(struct tcs3400_data *data)
> +{
> +	regulator_disable(data->vdd_supply);
After changes suggested below, this will not be needed.
> +}
> +
> +static int tcs3400_write_reg(struct tcs3400_data *data, u8 reg, u8 val)
> +{
> +	return i2c_smbus_write_byte_data(data->client, TCS3400_CMD_REG(reg), val);
> +}
> +
> +static int tcs3400_read_reg(struct tcs3400_data *data, u8 reg, u8 *val)
> +{
> +	int ret = i2c_smbus_read_byte_data(data->client, TCS3400_CMD_REG(reg));
> +
> +	if (ret < 0)
> +		return ret;
> +	*val = ret;
> +
> +	return 0;
> +}
> +
> +static int tcs3400_read_word(struct tcs3400_data *data, u8 reg, u16 *val)
> +{
> +
> +	__le16 buf;
> +	int ret = i2c_smbus_read_i2c_block_data(data->client,
> +						TCS3400_CMD_REG(reg), 2, (u8 *)&buf);

sizeof(buf) rather than 2.

> +	if (ret < 0)
> +		return ret;
> +	*val = le16_to_cpu(buf);
> +	return 0;
> +}
> +static int tcs3400_clear_interrupt(struct tcs3400_data *data)
> +{
> +
> +	return i2c_smbus_write_byte(data->client, TCS3400_CMD_ALS_INT_CLR);
> +}
> +
> +static int tcs3400_read_channels(struct tcs3400_data *data)
> +{
> +
> +	int ret, retries = 20;
> +	u8 status;
> +
> +	do {
> +		ret = tcs3400_read_reg(data, TCS3400_STATUS, &status);
> +		if (ret)
> +			return ret;
> +		if (status & TCS3400_STATUS_AVALID)
> +			break;
> +		usleep_range(5000, 6000);

Need a comment on why this sleep.  Mostly likely fsleep will be better choice.

> +	} while (--retries);
> +	if (!retries) {
> +		dev_warn(&data->client->dev, "Timeout waiting for valid data\n");
> +		return -ETIMEDOUT;
> +	}
> +	ret = tcs3400_read_word(data, TCS3400_CDATAL, &data->clear_ir);
> +	if (ret)
> +		return ret;
> +
> +	ret = tcs3400_read_word(data, TCS3400_RDATAL, &data->red);
> +	if (ret)
> +		return ret;
> +
> +	ret = tcs3400_read_word(data, TCS3400_GDATAL, &data->green);
> +	if (ret)
> +		return ret;
> +
> +	ret = tcs3400_read_word(data, TCS3400_BDATAL, &data->blue);

Datasheet is really vague on how much of the i2c protocol is supported.
Any idea if bulk reads are an option?

If not I'd suggest for read_raw at least you read only what is actually wanted.
I2C busses are pretty slow! Might even be worth doing that for buffered
reads.


> +	if (ret)
> +		return ret;
> +	return 0;
return tcs...

> +}
> +
> +static irqreturn_t tcs3400_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct tcs3400_data *data = iio_priv(indio_dev);
> +	u16 buf[4];
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = tcs3400_read_channels(data);
> +	if (!ret) {		
> +		buf[0] = data->clear_ir;
> +		buf[1] = data->red;
> +		buf[2] = data->green;
> +		buf[3] = data->blue;
> +		iio_push_to_buffers_with_timestamp(indio_dev, buf,
> +						   iio_get_time_ns(indio_dev));

Use iio_push_to_buffers_with_ts() and fix the error you'll see as a result.
This function is deprecated because it makes it too easy to write a bug
like you have done where the timestamp is off the end of the buffer.

> +	}
> +	mutex_unlock(&data->lock);
> +
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t tcs3400_irq_handler(int irq, void *priv)
> +{
> +	struct iio_dev *indio_dev = priv;
> +	struct tcs3400_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = tcs3400_read_channels(data);
> +	if (!ret)
> +		iio_trigger_poll_nested(indio_dev->trig);

I'm not sure what intent of this is... You seem to be poking a
trigger that wasn't yours. That's a hard no as it could
do absolutely anything.

Superficially it looks like the driver should be registering
a trigger of it's own and at least some of this logic
belongs on that side of the trigger / device driver
boundary.

> +
> +	tcs3400_clear_interrupt(data);
> +	mutex_unlock(&data->lock);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int tcs3400_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct tcs3400_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = tcs3400_read_channels(data);
> +	if (ret) {
> +		mutex_unlock(&data->lock);
Guard will help here as well. See below.

> +		return ret;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->channel2) {
> +		case IIO_MOD_LIGHT_CLEAR:
> +			*val = data->clear_ir;
> +			break;
> +		case IIO_MOD_LIGHT_RED:
> +			*val = data->red;
> +			break;
> +		case IIO_MOD_LIGHT_GREEN:
> +			*val = data->green;
> +			break;
> +		case IIO_MOD_LIGHT_BLUE:
> +			*val = data->blue;
> +			break;
> +		default:
> +			ret = -EINVAL;
> +			break;
> +		}
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		*val2 = (TCS3400_MAX_ATIME - data->atime) * 2780000; /* 2.78 ms per cycle */
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&data->lock);

Use a guard() and early returns so the code flow is simpler.

> +
> +	return ret;
> +}

> +
> +static const struct iio_chan_spec tcs3400_channels[] = {
> +	TCS3400_IIO_CHANNEL(0, LIGHT_CLEAR),
> +	TCS3400_IIO_CHANNEL(1, LIGHT_RED),
> +	TCS3400_IIO_CHANNEL(2, LIGHT_GREEN),
> +	TCS3400_IIO_CHANNEL(3, LIGHT_BLUE),
> +	IIO_CHAN_SOFT_TIMESTAMP(4),
> +};
> +
> +static ssize_t tcs3400_enable_show(struct device *dev,
> +				   struct device_attribute *attr,
> +				   char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct tcs3400_data *data = iio_priv(indio_dev);
> +	u8 enable;
> +	int ret;
> +
> +	ret = tcs3400_read_reg(data, TCS3400_ENABLE, &enable);
> +	if (ret)
> +		return ret;
> +	return sprintf(buf, "%d\n", !!(enable & (TCS3400_EN_PON | TCS3400_EN_AEN)));
> +}
> +
> +static ssize_t tcs3400_enable_store(struct device *dev,
> +				    struct device_attribute *attr,
> +				    const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct tcs3400_data *data = iio_priv(indio_dev);
> +	bool enable;
> +	int ret;
> +	u8 val;
> +
> +	ret = kstrtobool(buf, &enable);
> +	if (ret)
> +		return ret;
> +	mutex_lock(&data->lock);
> +	if (enable)
> +		val = TCS3400_EN_PON | TCS3400_EN_AEN;
> +	else
> +		val = 0;

Not clear why you set up val under the lock.  Keep lock
scope minimal where it doesn't hurt readability.


> +	ret = tcs3400_write_reg(data, TCS3400_ENABLE, val);
> +	mutex_unlock(&data->lock);
> +	if (ret)
> +		return ret;
> +
> +	if (enable)
> +		msleep(20);
> +	return len;
> +}
> +
> +static ssize_t tcs3400_ir_mode_show(struct device *dev,
> +				    struct device_attribute *attr,
> +				    char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct tcs3400_data *data = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", !!data->channel_mode);
> +}
> +
> +static ssize_t tcs3400_ir_mode_store(struct device *dev,
> +				     struct device_attribute *attr,
> +				     const char *buf, size_t len)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct tcs3400_data *data = iio_priv(indio_dev);
> +	bool enable;
> +	int ret;
> +
> +	ret = kstrtobool(buf, &enable);
> +	if (ret)
> +		return ret;
> +	mutex_lock(&data->lock);
> +	data->channel_mode = enable ? 0x80 : 0x00;

Looks like it's setting 1 bit. Probably want a define for that field's mask.
Is there anything else in that register?

> +	ret = tcs3400_write_reg(data, TCS3400_CHSEL, data->channel_mode);
> +	mutex_unlock(&data->lock);
> +	return ret ? ret : len;
> +}
> +
> +static IIO_DEVICE_ATTR(enable, 0644, tcs3400_enable_show, tcs3400_enable_store, 0);
> +static IIO_DEVICE_ATTR(ir_mode, 0644, tcs3400_ir_mode_show, tcs3400_ir_mode_store, 0);
> +
> +static struct attribute *tcs3400_attributes[] = {
> +	&iio_dev_attr_enable.dev_attr.attr,
Why?  Generally expect a light sensor to be enabled and disabled automatically 
based on other config. E.g. whether we have asked for data, enabled buffered capture
or turned events on.

> +	&iio_dev_attr_ir_mode.dev_attr.attr,

Looks like custom ABI.  This is hard to get through review, but starting point
is an ABI document in Documentation/ABI/testing/sysfs-bus-iio-*
so we can easily see what the interface is.

> +	NULL
> +};
> +
> +static const struct attribute_group tcs3400_attribute_group = {
> +	.attrs = tcs3400_attributes,
> +};
> +
> +static const struct iio_info tcs3400_info = {
> +	.read_raw = tcs3400_read_raw,
> +	.write_raw = tcs3400_write_raw,
> +	.attrs = &tcs3400_attribute_group,
> +};
> +
> +static int tcs3400_probe(struct i2c_client *client)
> +{
> +	struct tcs3400_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +	u8 id;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->client = client;
> +	mutex_init(&data->lock);

	ret = devm_mutex_init() in new code.

> +
> +	i2c_set_clientdata(client, indio_dev);

Check if you still need this after the other changes suggested below.

> +
> +	data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(data->vdd_supply))
> +		return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
> +				     "Unable to get VDD regulator\n");
> +
> +	ret = tcs3400_power_on(data);

Given you only turn it on at probe and off at remove
devm_regulator_get_enable() is something you should be using.  Then just
code the sleep inline here.

> +	if (ret)
> +		goto err_power_off;
> +
> +	ret = i2c_smbus_read_byte_data(client, TCS3400_CMD_REG(TCS3400_ID));
> +	if (ret < 0) {
> +		ret = -ENODEV;
> +		goto err_power_off;
> +		return ret;
> +	}
> +
> +	id = ret;
> +	if (id == 0x90)
> +		dev_info(&client->dev, "TCS3401/5 Chip ID: 0x%02x\n", id);
> +	if (id == 0x93)
> +		dev_info(&client->dev, "TCS3403/7 Chip ID: 0x%02x\n", id);
> +	else {
> +		dev_err(&client->dev, "Unknown chip ID: 0x%02x\n", id);
> +		ret = -ENODEV;

you set ret then do nothing with it.  However, you should only dev_info
on a mismatch anyway as failing probe on an ID miss match breaks the
dt concept of fallback compatibles.   If the ID is miss matched, follow what
the firmware told you it was.

> +	}
> +
> +	data->atime = 0xF6; /* ~27.8 ms integration */
> +	data->gain = TCS3400_GAIN_1X;
> +	data->channel_mode = 0x00;
> +
> +	tcs3400_write_reg(data, TCS3400_ATIME, data->atime);

These look likely to need error checks.

> +	tcs3400_write_reg(data, TCS3400_CONTROL, data->gain);
> +	tcs3400_write_reg(data, TCS3400_PERSIST, 0x00); /* interrupt every cycle */

This seems like you have a data ready interrupt. Look at how we register
triggers for those in other drivers.

> +	tcs3400_write_reg(data, TCS3400_CHSEL, data->channel_mode); /* RGBC mode */
> +
> +	tcs3400_write_reg(data, TCS3400_ENABLE,
> +			  TCS3400_EN_PON | TCS3400_EN_AEN | TCS3400_EN_AIEN);
> +
> +	indio_dev->name = TCS3400_DRV_NAME;

Prefer it hard coded string inline here.  There is no particular reason
it should match the driver name, particularly as you are adding multiple
part support. It should match what you think the part is.

> +	indio_dev->channels = tcs3400_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(tcs3400_channels);
> +	indio_dev->info = &tcs3400_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;

Look for whether other drivers do this (and more importantly why they don't).

> +
> +	ret = devm_iio_triggered_buffer_setup(&client->dev, indio_dev,
> +					      NULL,
> +					      tcs3400_trigger_handler,
> +					      NULL);
> +	if (ret)
> +		goto err_power_off;
> +	if (client->irq > 0) {
> +		ret = devm_request_threaded_irq(&client->dev, client->irq,
> +						NULL, tcs3400_irq_handler,
> +						IRQF_TRIGGER_FALLING | IRQF_ONESHOT,

Firmware is responsible for direction of irq in modern drivers.
We have some historical ones we can't risk 'fixing' this in. So should 
be just IRQF_ONESHOT.


> +						"tcs3400_irq", indio_dev);
> +		if (ret)
> +			goto err_power_off;
> +	}
> +
> +	ret = devm_iio_device_register(&client->dev, indio_dev);
> +	if (ret)
> +		goto err_power_off;
> +	return 0;
> +err_power_off:
> +	tcs3400_write_reg(data, TCS3400_ENABLE, 0);
> +	tcs3400_power_off(data);
> +	return ret;
> +}
> +
> +static void tcs3400_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct tcs3400_data *data = iio_priv(indio_dev);
> +
> +	tcs3400_write_reg(data, TCS3400_ENABLE, 0);
> +	tcs3400_power_off(data);

Look at how and when devm managed cleanup happens.  This is turning the
power off before removing the userspace interfaces, never
a recipe for success! :)

Hint, see how we use devm_add_action_or_reset for this sort of driver
specific cleanup -> that ensures correct sequencing and allows
you to pair turn off with turn on.


> +}

Powered by blists - more mailing lists