This light sensor can sense four channels of visible light
(red, green, blue, clear) and IR light. These features allow it to
be used for sensing the light source, or getting the environment color
temperature to adjust display gamma and backlight level.
It is used on Sony Xperia Yoshino family devices.
Tested on Sony Xperia XZ1 (poplar).
Markuss Broks (2):
dt-bindings: iio: tcs3490: Add bindings for AMS TCS3490 light sensor
iio: light: Add support for AMS TCS3490 Color Light-to-Digital
Converter
.../bindings/iio/light/ams,tcs3490.yaml | 52 ++++
MAINTAINERS | 7 +
drivers/iio/light/Kconfig | 12 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/tcs3490.c | 272 ++++++++++++++++++
5 files changed, 344 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml
create mode 100644 drivers/iio/light/tcs3490.c
--
2.39.0
Add device-tree bindings for the AMS TCS3490 Color ALS.
Signed-off-by: Markuss Broks <[email protected]>
---
.../bindings/iio/light/ams,tcs3490.yaml | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml
diff --git a/Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml b/Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml
new file mode 100644
index 000000000000..0938d5edd791
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/ams,tcs3490.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS TCS3490 Color Light-to-Digital Converter
+
+maintainers:
+ - Markuss Broks <[email protected]>
+
+description: |
+ This color and IR light sensor is typically used to adjust the backlight
+ intensity and correct the display color gamut. Additionally can be used
+ for light source type detection as it reports the IR content of the light.
+ It can sense four channels of visible light (red, green, blue, clear) and
+ IR light.
+
+properties:
+ compatible:
+ enum:
+ - ams,tcs3490
+
+ reg:
+ description:
+ I2C address of the device (typically 0x39).
+ maxItems: 1
+
+ vdd-supply: true
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ light@39 {
+ compatible = "ams,tcs3490";
+ reg = <0x39>;
+ vdd-supply = <&cam_vio_vreg>;
+ };
+ };
+...
--
2.39.0
Add a driver for AMS TCS3490 Color Light-to-Digital Converter. This
device provides color and IR (red, green, blue, clear and IR) light
sensing. The color sensing can be used for light source detection and
color temperature measurements.
Signed-off-by: Markuss Broks <[email protected]>
---
MAINTAINERS | 7 +
drivers/iio/light/Kconfig | 12 ++
drivers/iio/light/Makefile | 1 +
drivers/iio/light/tcs3490.c | 270 ++++++++++++++++++++++++++++++++++++
4 files changed, 290 insertions(+)
create mode 100644 drivers/iio/light/tcs3490.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 30e032abd196..3c47e132e4dd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1144,6 +1144,13 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/light/ams,as73211.yaml
F: drivers/iio/light/as73211.c
+AMS TCS3490 DRIVER
+M: Markuss Broks <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml
+F: drivers/iio/light/tcs3490.c
+
AMT (Automatic Multicast Tunneling)
M: Taehee Yoo <[email protected]>
L: [email protected]
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 0d4447df7200..582e3853e835 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -494,6 +494,18 @@ config TCS3472
This driver can also be built as a module. If so, the module
will be called tcs3472.
+config TCS3490
+ tristate "AMS TCS3490 color light-to-digital converter"
+ depends on I2C
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say Y here if you have an AMS TCS3490 color light-to digital converter
+ which provides RGB color and IR light sensing.
+
+ This driver can also be built as a module. If so, the module
+ will be called tcs3490.
+
config SENSORS_TSL2563
tristate "TAOS TSL2560, TSL2561, TSL2562 and TSL2563 ambient light sensors"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index d74d2b5ff14c..81663465d8c1 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_ST_UVIS25_I2C) += st_uvis25_i2c.o
obj-$(CONFIG_ST_UVIS25_SPI) += st_uvis25_spi.o
obj-$(CONFIG_TCS3414) += tcs3414.o
obj-$(CONFIG_TCS3472) += tcs3472.o
+obj-$(CONFIG_TCS3490) += tcs3490.o
obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
obj-$(CONFIG_TSL2583) += tsl2583.o
obj-$(CONFIG_TSL2591) += tsl2591.o
diff --git a/drivers/iio/light/tcs3490.c b/drivers/iio/light/tcs3490.c
new file mode 100644
index 000000000000..6fa2c220a6a1
--- /dev/null
+++ b/drivers/iio/light/tcs3490.c
@@ -0,0 +1,270 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * AMS TCS3490 Color Light-to-Digital Converter
+ *
+ * Copyright (c) 2023 Markuss Broks <[email protected]>
+ */
+
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+
+#define TCS3490_SUSPEND 0
+#define TCS3490_POWER_ON_INTERNAL BIT(0)
+#define TCS3490_ADC_ENABLE BIT(1)
+
+#define TCS3490_ENABLE 0x80
+#define TCS3490_GAIN_CTRL 0x8f
+#define TCS3490_REVID 0x91
+#define TCS3490_ID 0x92
+#define TCS3490_STATUS 0x93
+
+#define TCS3490_CLEAR_IR_BASE 0x94
+#define TCS3490_RED_BASE 0x96
+#define TCS3490_GREEN_BASE 0x98
+#define TCS3490_BLUE_BASE 0x9a
+
+#define TCS3490_CLEAR_IR_MODE 0xc0
+
+#define TCS3490_STATUS_RGBC_VALID BIT(0)
+#define TCS3490_STATUS_ALS_SAT BIT(7)
+
+#define TCS3490_MODE_CLEAR 0
+#define TCS3490_MODE_IR BIT(7)
+
+#define TCS3490_GAIN_MASK GENMASK(1, 0)
+
+#define TCS3490_LIGHT_CHANNEL(_color, _idx) { \
+ .type = IIO_INTENSITY, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = \
+ BIT(IIO_CHAN_INFO_CALIBSCALE), \
+ .address = _idx, \
+ .modified = 1, \
+ .channel2 = IIO_MOD_LIGHT_##_color, \
+} \
+
+struct tcs3490 {
+ struct i2c_client *client;
+ struct regmap *regmap;
+ struct regulator *vdd_supply;
+};
+
+const unsigned int tcs3490_gain_multiplier[] = {1, 4, 16, 64};
+
+static const struct regmap_config tcs3490_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 8,
+};
+
+static const struct iio_chan_spec tcs3490_channels[] = {
+ TCS3490_LIGHT_CHANNEL(RED, TCS3490_RED_BASE),
+ TCS3490_LIGHT_CHANNEL(GREEN, TCS3490_GREEN_BASE),
+ TCS3490_LIGHT_CHANNEL(BLUE, TCS3490_BLUE_BASE),
+ TCS3490_LIGHT_CHANNEL(CLEAR, TCS3490_CLEAR_IR_BASE),
+ TCS3490_LIGHT_CHANNEL(IR, TCS3490_CLEAR_IR_BASE),
+};
+
+static int tcs3490_get_gain(struct tcs3490 *data, int *val)
+{
+ int ret;
+ unsigned int gain;
+
+ ret = regmap_read(data->regmap, TCS3490_GAIN_CTRL, &gain);
+ if (ret)
+ return ret;
+
+ gain &= TCS3490_GAIN_MASK;
+
+ *val = tcs3490_gain_multiplier[gain];
+ return 0;
+}
+
+static int tcs3490_set_gain(struct tcs3490 *data, unsigned int gain)
+{
+ int ret, i;
+
+ for (i = 0; i < ARRAY_SIZE(tcs3490_gain_multiplier); i++) {
+ if (tcs3490_gain_multiplier[i] == gain)
+ break;
+ }
+
+ if (i == ARRAY_SIZE(tcs3490_gain_multiplier))
+ return -EINVAL;
+
+ ret = regmap_write(data->regmap, TCS3490_GAIN_CTRL, i);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int tcs3490_read_channel(struct tcs3490 *data,
+ const struct iio_chan_spec *chan,
+ int *val)
+{
+ unsigned int tries = 20;
+ unsigned int val_l, val_h, status;
+ int ret;
+
+ ret = regmap_write(data->regmap, TCS3490_ENABLE,
+ TCS3490_POWER_ON_INTERNAL | TCS3490_ADC_ENABLE);
+ if (ret)
+ return ret;
+
+ switch (chan->channel2) {
+ case IIO_MOD_LIGHT_RED:
+ case IIO_MOD_LIGHT_GREEN:
+ case IIO_MOD_LIGHT_BLUE:
+ break;
+ case IIO_MOD_LIGHT_CLEAR:
+ ret = regmap_write(data->regmap, TCS3490_CLEAR_IR_MODE, TCS3490_MODE_CLEAR);
+ break;
+ case IIO_MOD_LIGHT_IR:
+ ret = regmap_write(data->regmap, TCS3490_CLEAR_IR_MODE, TCS3490_MODE_IR);
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ do {
+ usleep_range(3000, 4000);
+
+ ret = regmap_read(data->regmap, TCS3490_STATUS, &status);
+ if (ret)
+ return ret;
+ if (status & TCS3490_STATUS_RGBC_VALID)
+ break;
+ } while (--tries);
+
+ if (!tries)
+ return -ETIMEDOUT;
+
+ ret = regmap_read(data->regmap, chan->address, &val_l);
+ if (ret)
+ return ret;
+
+ ret = regmap_read(data->regmap, chan->address + 1, &val_h);
+ if (ret)
+ return ret;
+
+ *val = (val_h << 8) | val_l;
+
+ ret = regmap_write(data->regmap, TCS3490_ENABLE, TCS3490_SUSPEND);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int tcs3490_read_raw(struct iio_dev *indio_dev,
+ const struct iio_chan_spec *chan,
+ int *val, int *val2, long mask)
+{
+ struct tcs3490 *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = tcs3490_read_channel(data, chan, val);
+ if (ret < 0)
+ return ret;
+
+ ret = IIO_VAL_INT;
+ break;
+ case IIO_CHAN_INFO_CALIBSCALE:
+ ret = tcs3490_get_gain(data, val);
+ ret = IIO_VAL_INT;
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ if (ret < 0)
+ return ret;
+ return IIO_VAL_INT;
+}
+
+static int tcs3490_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct tcs3490 *data = iio_priv(indio_dev);
+
+ if (mask == IIO_CHAN_INFO_CALIBSCALE)
+ return tcs3490_set_gain(data, val);
+
+ return -EINVAL;
+}
+
+static const struct iio_info tcs3490_info = {
+ .read_raw = tcs3490_read_raw,
+ .write_raw = tcs3490_write_raw,
+};
+
+static int tcs3490_probe(struct i2c_client *client)
+{
+ struct tcs3490 *data;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+
+ data->client = client;
+
+ 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 regulators\n");
+
+ data->regmap = devm_regmap_init_i2c(client, &tcs3490_regmap_config);
+ if (IS_ERR(data->regmap))
+ return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
+ "Failed to register the register map\n");
+
+ ret = regulator_enable(data->vdd_supply);
+ if (ret)
+ return dev_err_probe(&client->dev, ret,
+ "Unable to enable regulators\n");
+
+ indio_dev->name = "tcs3490";
+ indio_dev->info = &tcs3490_info;
+ indio_dev->channels = tcs3490_channels;
+ indio_dev->num_channels = ARRAY_SIZE(tcs3490_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = regmap_write(data->regmap, TCS3490_ENABLE, TCS3490_SUSPEND);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct of_device_id tcs3490_of_match[] = {
+ { .compatible = "ams,tcs3490", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, tcs3490_of_match);
+
+static struct i2c_driver tcs3490_driver = {
+ .driver = {
+ .name = "tcs3490",
+ .of_match_table = of_match_ptr(tcs3490_of_match),
+ },
+ .probe_new = tcs3490_probe,
+};
+
+module_i2c_driver(tcs3490_driver);
+
+MODULE_DESCRIPTION("AMS TCS3490 Color Light-to-Digital Converter driver");
+MODULE_AUTHOR("Markuss Broks <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.39.0
On Tue, Jan 24, 2023 at 01:10:25AM +0200, Markuss Broks wrote:
> Add a driver for AMS TCS3490 Color Light-to-Digital Converter. This
> device provides color and IR (red, green, blue, clear and IR) light
> sensing. The color sensing can be used for light source detection and
> color temperature measurements.
...
> +AMS TCS3490 DRIVER
> +M: Markuss Broks <[email protected]>
> +L: [email protected]
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml
Shouldn't actually be added with the schema patch?
> +F: drivers/iio/light/tcs3490.c
I dunno what's the rules but it feels a bit inconsistent in case the schema
will leave while driver got, for example, rewritten (as brand new) and reverted
(as old one). In such (quite unlikely) circumstances we may end up with the
dangling file.
Rob, Krzysztof, Jonathan, what is yours take from this?
...
> +config TCS3490
> + tristate "AMS TCS3490 color light-to-digital converter"
> + depends on I2C
Hmm... Where is the select REGMAP_I2C?
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say Y here if you have an AMS TCS3490 color light-to digital converter
> + which provides RGB color and IR light sensing.
> +
> + This driver can also be built as a module. If so, the module
> + will be called tcs3490.
...
> +struct tcs3490 {
> + struct i2c_client *client;
Why do you need this?
> + struct regmap *regmap;
> + struct regulator *vdd_supply;
> +};
...
> +static const struct regmap_config tcs3490_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
Seems you are using regmap internal serialization, but does it guarantee the
serialization on the transaction level? Or why is it not a problem?
> +};
...
> + do {
> + usleep_range(3000, 4000);
> +
> + ret = regmap_read(data->regmap, TCS3490_STATUS, &status);
> + if (ret)
> + return ret;
> + if (status & TCS3490_STATUS_RGBC_VALID)
> + break;
> + } while (--tries);
> +
> + if (!tries)
> + return -ETIMEDOUT;
regmap_read_poll_timeout()?
...
> + ret = regmap_read(data->regmap, chan->address, &val_l);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, chan->address + 1, &val_h);
> + if (ret)
> + return ret;
Why not a bulk read into __le16 val?
> + *val = (val_h << 8) | val_l;
Use le16_to_cpu().
> + ret = regmap_write(data->regmap, TCS3490_ENABLE, TCS3490_SUSPEND);
> + if (ret)
> + return ret;
> +
> + return 0;
Can be simply
return regmap_write(...);
> +}
...
> +static int tcs3490_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct tcs3490 *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = tcs3490_read_channel(data, chan, val);
> + if (ret < 0)
> + return ret;
> + ret = IIO_VAL_INT;
> + break;
return directly.
> + case IIO_CHAN_INFO_CALIBSCALE:
> + ret = tcs3490_get_gain(data, val);
Missing error check.
> + ret = IIO_VAL_INT;
> + break;
Return directly.
> + default:
> + ret = -EINVAL;
> + break;
Ditto.
> + }
> + if (ret < 0)
> + return ret;
> + return IIO_VAL_INT;
Redundant, see above.
> +}
...
> +static const struct of_device_id tcs3490_of_match[] = {
> + { .compatible = "ams,tcs3490", },
Inner comma is not needed.
> + { },
Terminator entries should go without a comma.
> +};
...
> +static struct i2c_driver tcs3490_driver = {
> + .driver = {
> + .name = "tcs3490",
> + .of_match_table = of_match_ptr(tcs3490_of_match),
Kill of_match_ptr(). Its usage is wrong in 99% of the cases.
> + },
> + .probe_new = tcs3490_probe,
> +};
> +
Redundant blank line.
> +module_i2c_driver(tcs3490_driver);
--
With Best Regards,
Andy Shevchenko
On 24/01/2023 00:10, Markuss Broks wrote:
> Add device-tree bindings for the AMS TCS3490 Color ALS.
If there is going to be new version:
Subject: drop second/last, redundant "bindings for". The "dt-bindings"
prefix is already stating that these are bindings.
Reviewed-by: Krzysztof Kozlowski <[email protected]>
>
> Signed-off-by: Markuss Broks <[email protected]>
> ---
Best regards,
Krzysztof
Hi Andy,
On 1/24/23 12:22, Andy Shevchenko wrote:
> On Tue, Jan 24, 2023 at 01:10:25AM +0200, Markuss Broks wrote:
>> Add a driver for AMS TCS3490 Color Light-to-Digital Converter. This
>> device provides color and IR (red, green, blue, clear and IR) light
>> sensing. The color sensing can be used for light source detection and
>> color temperature measurements.
> ...
>
>> +AMS TCS3490 DRIVER
>> +M: Markuss Broks <[email protected]>
>> +L: [email protected]
>> +S: Maintained
>> +F: Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml
> Shouldn't actually be added with the schema patch?
>
>> +F: drivers/iio/light/tcs3490.c
> I dunno what's the rules but it feels a bit inconsistent in case the schema
> will leave while driver got, for example, rewritten (as brand new) and reverted
> (as old one). In such (quite unlikely) circumstances we may end up with the
> dangling file.
>
> Rob, Krzysztof, Jonathan, what is yours take from this?
>
> ...
>
>> +config TCS3490
>> + tristate "AMS TCS3490 color light-to-digital converter"
>> + depends on I2C
> Hmm... Where is the select REGMAP_I2C?
>
>> + select IIO_BUFFER
>> + select IIO_TRIGGERED_BUFFER
>> + help
>> + Say Y here if you have an AMS TCS3490 color light-to digital converter
>> + which provides RGB color and IR light sensing.
>> +
>> + This driver can also be built as a module. If so, the module
>> + will be called tcs3490.
> ...
>
>> +struct tcs3490 {
>> + struct i2c_client *client;
> Why do you need this?
>
>> + struct regmap *regmap;
>> + struct regulator *vdd_supply;
>> +};
> ...
>
>> +static const struct regmap_config tcs3490_regmap_config = {
>> + .reg_bits = 8,
>> + .val_bits = 8,
> Seems you are using regmap internal serialization, but does it guarantee the
> serialization on the transaction level? Or why is it not a problem?
Well, other drivers seem to have it this way too. I don't really
understand why it should be a problem, could you please clarify?
>
>> +};
> ...
>
>> + do {
>> + usleep_range(3000, 4000);
>> +
>> + ret = regmap_read(data->regmap, TCS3490_STATUS, &status);
>> + if (ret)
>> + return ret;
>> + if (status & TCS3490_STATUS_RGBC_VALID)
>> + break;
>> + } while (--tries);
>> +
>> + if (!tries)
>> + return -ETIMEDOUT;
> regmap_read_poll_timeout()?
>
> ...
>
>> + ret = regmap_read(data->regmap, chan->address, &val_l);
>> + if (ret)
>> + return ret;
>> +
>> + ret = regmap_read(data->regmap, chan->address + 1, &val_h);
>> + if (ret)
>> + return ret;
> Why not a bulk read into __le16 val?
>
>> + *val = (val_h << 8) | val_l;
> Use le16_to_cpu().
>
>> + ret = regmap_write(data->regmap, TCS3490_ENABLE, TCS3490_SUSPEND);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
> Can be simply
>
> return regmap_write(...);
>
>> +}
> ...
>
>> +static int tcs3490_read_raw(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + int *val, int *val2, long mask)
>> +{
>> + struct tcs3490 *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + ret = tcs3490_read_channel(data, chan, val);
>> + if (ret < 0)
>> + return ret;
>> + ret = IIO_VAL_INT;
>> + break;
> return directly.
>
>> + case IIO_CHAN_INFO_CALIBSCALE:
>> + ret = tcs3490_get_gain(data, val);
> Missing error check.
>
>> + ret = IIO_VAL_INT;
>> + break;
> Return directly.
>
>> + default:
>> + ret = -EINVAL;
>> + break;
> Ditto.
>
>> + }
>> + if (ret < 0)
>> + return ret;
>> + return IIO_VAL_INT;
> Redundant, see above.
>
>> +}
> ...
>
>> +static const struct of_device_id tcs3490_of_match[] = {
>> + { .compatible = "ams,tcs3490", },
> Inner comma is not needed.
>
>> + { },
> Terminator entries should go without a comma.
>
>> +};
> ...
>
>> +static struct i2c_driver tcs3490_driver = {
>> + .driver = {
>> + .name = "tcs3490",
>> + .of_match_table = of_match_ptr(tcs3490_of_match),
> Kill of_match_ptr(). Its usage is wrong in 99% of the cases.
>
>> + },
>> + .probe_new = tcs3490_probe,
>> +};
>> +
> Redundant blank line.
>
>> +module_i2c_driver(tcs3490_driver);
- Markuss
On Thu, 26 Jan 2023 17:54:56 +0200
Markuss Broks <[email protected]> wrote:
> Hi Andy,
>
> On 1/24/23 12:22, Andy Shevchenko wrote:
> > On Tue, Jan 24, 2023 at 01:10:25AM +0200, Markuss Broks wrote:
> >> Add a driver for AMS TCS3490 Color Light-to-Digital Converter. This
> >> device provides color and IR (red, green, blue, clear and IR) light
> >> sensing. The color sensing can be used for light source detection and
> >> color temperature measurements.
> > ...
> >
> >> +AMS TCS3490 DRIVER
> >> +M: Markuss Broks <[email protected]>
> >> +L: [email protected]
> >> +S: Maintained
> >> +F: Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml
> > Shouldn't actually be added with the schema patch?
> >
> >> +F: drivers/iio/light/tcs3490.c
> > I dunno what's the rules but it feels a bit inconsistent in case the schema
> > will leave while driver got, for example, rewritten (as brand new) and reverted
> > (as old one). In such (quite unlikely) circumstances we may end up with the
> > dangling file.
> >
> > Rob, Krzysztof, Jonathan, what is yours take from this?
Ideal is update the file listing as files are added, but I rarely care
enough to fix it up or complain as the patches almost always go
together.
On Thu, Jan 26, 2023 at 05:54:56PM +0200, Markuss Broks wrote:
> On 1/24/23 12:22, Andy Shevchenko wrote:
> > On Tue, Jan 24, 2023 at 01:10:25AM +0200, Markuss Broks wrote:
First of all, I assume you agree on the comments you left unanswered,
so we will expect them all being addressed in the next version. Is it
correct perception?
...
> > > +static const struct regmap_config tcs3490_regmap_config = {
> > > + .reg_bits = 8,
> > > + .val_bits = 8,
> > Seems you are using regmap internal serialization, but does it guarantee the
> > serialization on the transaction level? Or why is it not a problem?
> Well, other drivers seem to have it this way too.
They all may be buggy, unclear, or hardware there doesn't require transaction
level locks. It means we have to avoid cargo cult.
> I don't really understand
> why it should be a problem, could you please clarify?
Because one should distinguish IO with register vs. IO transaction.
Imaging two threads which do an IO:
CPU0 CPU1
read REG2
update value
read REG1
write REG2
update value
write REG1
If in our hypothetical example the writing to REG2 has a side effect on
the values in REG1, we are doomed.
You have to check all possible scenarios and tell if it's a problem or not with
the certain hardware. According to the result, add a corresponding comment to
the code and, if required, change the locking scheme.
> > > +};
--
With Best Regards,
Andy Shevchenko
On Tue, 24 Jan 2023 01:10:25 +0200
Markuss Broks <[email protected]> wrote:
> Add a driver for AMS TCS3490 Color Light-to-Digital Converter. This
> device provides color and IR (red, green, blue, clear and IR) light
> sensing. The color sensing can be used for light source detection and
> color temperature measurements.
>
> Signed-off-by: Markuss Broks <[email protected]>
Hi Markuss,
I'll probably duplicate some of Andy's review (as it's always very
thorough!) but maybe some new stuff inline.
Jonathan
> ---
> MAINTAINERS | 7 +
> drivers/iio/light/Kconfig | 12 ++
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/tcs3490.c | 270 ++++++++++++++++++++++++++++++++++++
> 4 files changed, 290 insertions(+)
> create mode 100644 drivers/iio/light/tcs3490.c
> AMT (Automatic Multicast Tunneling)
> M: Taehee Yoo <[email protected]>
> L: [email protected]
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 0d4447df7200..582e3853e835 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -494,6 +494,18 @@ config TCS3472
> This driver can also be built as a module. If so, the module
> will be called tcs3472.
>
> +config TCS3490
> + tristate "AMS TCS3490 color light-to-digital converter"
> + depends on I2C
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
No Buffered support in here yet. So these dependencies don't make sense.
> + help
> + Say Y here if you have an AMS TCS3490 color light-to digital converter
> + which provides RGB color and IR light sensing.
> +
> + This driver can also be built as a module. If so, the module
> + will be called tcs3490.
> +
> config SENSORS_TSL2563
> diff --git a/drivers/iio/light/tcs3490.c b/drivers/iio/light/tcs3490.c
> new file mode 100644
> index 000000000000..6fa2c220a6a1
> --- /dev/null
> +++ b/drivers/iio/light/tcs3490.c
> @@ -0,0 +1,270 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * AMS TCS3490 Color Light-to-Digital Converter
> + *
> + * Copyright (c) 2023 Markuss Broks <[email protected]>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +
> +#define TCS3490_SUSPEND 0
Probably want to call
Good to make a clear difference between register defines
and fields within the register + make it clear which
register they are in. Lots of ways of doing this. e.g.
#define TSC3490_ENABLE_REG 0
#define TSC3490_ENABLE_POWER_ON_INTERNAL BIT(0)
...
> +#define TCS3490_POWER_ON_INTERNAL BIT(0)
> +#define TCS3490_ADC_ENABLE BIT(1)
> +
> +#define TCS3490_ENABLE 0x80
> +#define TCS3490_GAIN_CTRL 0x8f
> +#define TCS3490_REVID 0x91
> +#define TCS3490_ID 0x92
> +#define TCS3490_STATUS 0x93
> +
> +#define TCS3490_CLEAR_IR_BASE 0x94
> +#define TCS3490_RED_BASE 0x96
> +#define TCS3490_GREEN_BASE 0x98
> +#define TCS3490_BLUE_BASE 0x9a
> +
> +#define TCS3490_CLEAR_IR_MODE 0xc0
> +
> +#define TCS3490_STATUS_RGBC_VALID BIT(0)
> +#define TCS3490_STATUS_ALS_SAT BIT(7)
> +
> +#define TCS3490_MODE_CLEAR 0
> +#define TCS3490_MODE_IR BIT(7)
> +
> +#define TCS3490_GAIN_MASK GENMASK(1, 0)
> +
> +#define TCS3490_LIGHT_CHANNEL(_color, _idx) { \
> + .type = IIO_INTENSITY, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = \
> + BIT(IIO_CHAN_INFO_CALIBSCALE), \
I couldn't quickly figure it out from the datasheet, but calibscale
usually only applies to small 'calibration' tweaks.
For values like this I'd expect changing the scale to have
equivalent affect on the _raw output and so userspace would
need to compensate. That would make it IIO_CHAN_INFO_SCALE
So that the actual value would be calculated by userspace as
_raw * _scale
I've just noticed we don't seem to have ABI docs
for in_intensity_blue_raw or in_intensity_blue_scale etc
Documentation/ABI/testing/sysfs-bus-iio
That wants fixing though doesn't need to be in the same series
as this patch. If you want to propose such a patch that would
be great! (I might take a while to get to it if not).
> + .address = _idx, \
> + .modified = 1, \
> + .channel2 = IIO_MOD_LIGHT_##_color, \
> +} \
> +
> +struct tcs3490 {
> + struct i2c_client *client;
> + struct regmap *regmap;
> + struct regulator *vdd_supply;
> +};
> +
> +const unsigned int tcs3490_gain_multiplier[] = {1, 4, 16, 64};
> +
> +static const struct regmap_config tcs3490_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
This sort of attempt to align structure elements bites back in the long
ruin. Don't bother ;)
.reg_bits = 8,
> +};
> +
> +static const struct iio_chan_spec tcs3490_channels[] = {
> + TCS3490_LIGHT_CHANNEL(RED, TCS3490_RED_BASE),
> + TCS3490_LIGHT_CHANNEL(GREEN, TCS3490_GREEN_BASE),
> + TCS3490_LIGHT_CHANNEL(BLUE, TCS3490_BLUE_BASE),
> + TCS3490_LIGHT_CHANNEL(CLEAR, TCS3490_CLEAR_IR_BASE),
> + TCS3490_LIGHT_CHANNEL(IR, TCS3490_CLEAR_IR_BASE),
> +};
> +
> +static int tcs3490_get_gain(struct tcs3490 *data, int *val)
> +{
> + int ret;
> + unsigned int gain;
> +
> + ret = regmap_read(data->regmap, TCS3490_GAIN_CTRL, &gain);
> + if (ret)
> + return ret;
> +
> + gain &= TCS3490_GAIN_MASK;
FIELD_GET() preferred as then I don't need to look at whether GAIN_MASK
is aligned to LSB.
> +
> + *val = tcs3490_gain_multiplier[gain];
blank line before simple returns like this tends to be slightly more readable.
> + return 0;
> +}
> +
> +static int tcs3490_set_gain(struct tcs3490 *data, unsigned int gain)
> +{
> + int ret, i;
> +
> + for (i = 0; i < ARRAY_SIZE(tcs3490_gain_multiplier); i++) {
> + if (tcs3490_gain_multiplier[i] == gain)
> + break;
> + }
> +
> + if (i == ARRAY_SIZE(tcs3490_gain_multiplier))
> + return -EINVAL;
> +
> + ret = regmap_write(data->regmap, TCS3490_GAIN_CTRL, i);
return regmap_write()
This sort of unnecessary checking of ret for last entries tends to get
picked up by some of the scripts that get run on the kernel so if
you leave it like this we'll just get patches 'improving' it anyway.
Better to tidy up now.
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int tcs3490_read_channel(struct tcs3490 *data,
> + const struct iio_chan_spec *chan,
> + int *val)
> +{
> + unsigned int tries = 20;
Comment needed on why 20 is appropriate in combination with the usleep_range
values. Probably push setting tries down to just above that loop so that
the comment can cover both tries and the usleep_range parameter
> + unsigned int val_l, val_h, status;
> + int ret;
> +
Complex cycle and could be called multiple times if multiple processes read
different channels at same times, so needs a locally defined lock.
(Andy raised this)
> + ret = regmap_write(data->regmap, TCS3490_ENABLE,
> + TCS3490_POWER_ON_INTERNAL | TCS3490_ADC_ENABLE);
Perhaps consider runtime PM with autosuspend (could be a future patch set)
so we leave the device powered up for a while after a read in case someone is
doing lots of reads. Will reduce time needed to get a value (probably - I've
not checked datasheet that closely) However, won't be easy to tell you have
a fresh value as we only know it's new once after ADC_ENABLE was set.
Maybe best to leave this suggestion for a possible future follow up patch.
> + if (ret)
> + return ret;
> +
> + switch (chan->channel2) {
> + case IIO_MOD_LIGHT_RED:
> + case IIO_MOD_LIGHT_GREEN:
> + case IIO_MOD_LIGHT_BLUE:
> + break;
> + case IIO_MOD_LIGHT_CLEAR:
> + ret = regmap_write(data->regmap, TCS3490_CLEAR_IR_MODE, TCS3490_MODE_CLEAR);
check ret.
> + break;
> + case IIO_MOD_LIGHT_IR:
> + ret = regmap_write(data->regmap, TCS3490_CLEAR_IR_MODE, TCS3490_MODE_IR);
check ret.
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + do {
> + usleep_range(3000, 4000);
> +
> + ret = regmap_read(data->regmap, TCS3490_STATUS, &status);
> + if (ret)
> + return ret;
> + if (status & TCS3490_STATUS_RGBC_VALID)
> + break;
> + } while (--tries);
> +
> + if (!tries)
> + return -ETIMEDOUT;
> +
> + ret = regmap_read(data->regmap, chan->address, &val_l);
> + if (ret)
> + return ret;
> +
> + ret = regmap_read(data->regmap, chan->address + 1, &val_h);
> + if (ret)
> + return ret;
> +
> + *val = (val_h << 8) | val_l;
Andy mentioned bulk read. Definitely a good idea combined with appropriate
endian conversion.
> +
> + ret = regmap_write(data->regmap, TCS3490_ENABLE, TCS3490_SUSPEND);
> + if (ret)
> + return ret;
> +
> + return 0;
return regmap_write()
> +}
> +
> +static int tcs3490_read_raw(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + int *val, int *val2, long mask)
> +{
> + struct tcs3490 *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = tcs3490_read_channel(data, chan, val);
> + if (ret < 0)
> + return ret;
> +
> + ret = IIO_VAL_INT;
return IIO_VAL_INT;
> + break;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + ret = tcs3490_get_gain(data, val);
> + ret = IIO_VAL_INT;
return IIO_VAL_INT;
> + break;
> + default:
> + ret = -EINVAL;
return -EINVAL;
> + break;
> + }
> +
won't be able to get here so all this code goes.
> + if (ret < 0)
> + return ret;
> + return IIO_VAL_INT;
> +}
> +
> +static int tcs3490_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct tcs3490 *data = iio_priv(indio_dev);
> +
> + if (mask == IIO_CHAN_INFO_CALIBSCALE)
> + return tcs3490_set_gain(data, val);
> +
> + return -EINVAL;
> +}
> +
> +static const struct iio_info tcs3490_info = {
> + .read_raw = tcs3490_read_raw,
> + .write_raw = tcs3490_write_raw,
> +};
> +
> +static int tcs3490_probe(struct i2c_client *client)
> +{
> + struct tcs3490 *data;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> +
> + data->client = client;
> +
> + data->vdd_supply = devm_regulator_get(&client->dev, "vdd");
devm_regulator_get_enable() as you don't need to access the regulator
other than turning it on. That simplifies handling of regulators
that are only turned on with driver probe and off with remove.
> + if (IS_ERR(data->vdd_supply))
> + return dev_err_probe(&client->dev, PTR_ERR(data->vdd_supply),
> + "Unable to get regulators\n");
> +
> + data->regmap = devm_regmap_init_i2c(client, &tcs3490_regmap_config);
> + if (IS_ERR(data->regmap))
> + return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> + "Failed to register the register map\n");
> +
> + ret = regulator_enable(data->vdd_supply);
> + if (ret)
> + return dev_err_probe(&client->dev, ret,
> + "Unable to enable regulators\n");
> +
> + indio_dev->name = "tcs3490";
> + indio_dev->info = &tcs3490_info;
> + indio_dev->channels = tcs3490_channels;
> + indio_dev->num_channels = ARRAY_SIZE(tcs3490_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = regmap_write(data->regmap, TCS3490_ENABLE, TCS3490_SUSPEND);
Naming of value vs register is confusing. See above.
Writing 0 to a register called _ENABLE is probably clear enough that you
don't need the _SUSPEND define which is odd anyway as all other defines
for this register refer to specific bits whereas this is 'all bits not set'.
ret = regmap_write(data->regmap, TCS3490_ENABLE_REG, 0);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct of_device_id tcs3490_of_match[] = {
> + { .compatible = "ams,tcs3490", },
> + { },
No need for comma after anything that acts as a NULL terminator
as we'll never have anything after it.
> +};
> +MODULE_DEVICE_TABLE(of, tcs3490_of_match);
> +
> +static struct i2c_driver tcs3490_driver = {
> + .driver = {
> + .name = "tcs3490",
> + .of_match_table = of_match_ptr(tcs3490_of_match),
Drop the of_match_ptr()
> + },
> + .probe_new = tcs3490_probe,
> +};
> +
> +module_i2c_driver(tcs3490_driver);
> +
> +MODULE_DESCRIPTION("AMS TCS3490 Color Light-to-Digital Converter driver");
> +MODULE_AUTHOR("Markuss Broks <[email protected]>");
> +MODULE_LICENSE("GPL");