| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aW5-vJJf7RNLo-Z4@smile.fi.intel.com>
Date: Mon, 19 Jan 2026 20:58:04 +0200
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: petr.hodina@...tonmail.com
Cc: Jonathan Cameron <jic23@...nel.org>,
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, Jan 19, 2026 at 06:19:07PM +0100, Petr Hodina via B4 Relay wrote:
> 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.
...
> + tristate "AMS TCS3400 color light-to-digital converter"
> + depends on I2C
> + default n
This is already default 'default', remove.
> + 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.
...
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * TCS3400 - AMS/TAOS color light sensor with RGBC and RGB-IR channels
> + *
> + * Copyright (c) 2025 Petr Hodina
> + *
Stray blank line.
> + */
...
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/delay.h>
> +#include <linux/pm_runtime.h>
Keep this ordered alphabetically and followed IWYU principle. Many are missing.
+ Blank line.
> +#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>
> +#include <linux/iio/trigger_consumer.h>
Also keep this group ordered.
...
> + +struct tcs3400_data {
Have you run `pahole`?
> + 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 */
> + u16 red;
> + u16 green;
> + u16 blue;
> +};
...
> +static const int tcs3400_gains[] = {1, 4, 16, 64};
Put inner spaces into {}.
...
> +static int tcs3400_power_on(struct tcs3400_data *data)
> +{
> + int ret;
> +
> + ret = regulator_enable(data->vdd_supply);
> + if (ret)
> + return ret;
> + msleep(20);
So-o long sleeps must be commented. Preferably with a reference
to the respective datasheet section / table / etc.
> + return 0;
> +}
...
> +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);
> +}
Why not a regmap?
...
> +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;
This is unmaintainable way of putting things. Better is to split value
definition and assignment.
int ret;
ret = ...
if (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);
> + if (ret < 0)
> + return ret;
> + *val = le16_to_cpu(buf);
> + return 0;
A lot of missing blank lines.
> +}
> +static int tcs3400_clear_interrupt(struct tcs3400_data *data)
> +{
> +
And here redundant blank line.
> + 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);
> + } while (--retries);
> + if (!retries) {
> + dev_warn(&data->client->dev, "Timeout waiting for valid data\n");
> + return -ETIMEDOUT;
> + }
This is reinvention of something from iopoll.h.
> + 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);
> + if (ret)
> + return ret;
> + return 0;
> +}
> +
> +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);
Use scoped_guard() from cleanup.h...
> + ret = tcs3400_read_channels(data);
> + if (!ret) {
...and usual pattern instead.
> + 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));
> + }
> + mutex_unlock(&data->lock);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
I stopped here, please, read other drivers that came into IIO subsystem lately
(last couple of Linux kernel release cycles) for the style that we want to see
in the drivers here.
...
> + 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),
Use
struct device *dev = &client->dev;
at the top of the function and reduce the verbosity in all the probe code.
> + "Unable to get VDD regulator\n");
...
> +err_power_off:
> + tcs3400_write_reg(data, TCS3400_ENABLE, 0);
> + tcs3400_power_off(data);
> + return ret;
Wrong. This messes up with releasing ordering. How did you test this?
...
> +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);
Properly wrapped into devm this entire function will gone.
> +}
...
> +static const struct of_device_id tcs3400_of_match[] = {
> + { .compatible = "ams,tcs3400" },
> + { }
> +};
> +
Drop this blank line.
> +MODULE_DEVICE_TABLE(of, tcs3400_of_match);
> +
> +static const struct i2c_device_id tcs3400_id[] = {
> + { "tcs3400", 0 },
Drop ', 0' part
> + { }
> +};
> +
Drop blank line.
> +MODULE_DEVICE_TABLE(i2c, tcs3400_id);
> +
> +static struct i2c_driver tcs3400_driver = {
> + .driver = {
> + .name = TCS3400_DRV_NAME,
> + .of_match_table = tcs3400_of_match,
> + },
> + .probe = tcs3400_probe,
> + .remove = tcs3400_remove,
> + .id_table = tcs3400_id,
> +};
> +
Misplaced blank line...
> +module_i2c_driver(tcs3400_driver);
...should be here.
> +MODULE_AUTHOR("Petr Hodina <petr.hodina@...tonmail.com>");
> +MODULE_DESCRIPTION("AMS TCS3400 RGB/IR color sensor IIO driver");
> +MODULE_LICENSE("GPL");
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists