* [PATCH 0/2] Add a driver for AMS TCS3490 light sensor
@ 2023-01-23 23:10 Markuss Broks
2023-01-23 23:10 ` [PATCH 1/2] dt-bindings: iio: tcs3490: Add bindings " Markuss Broks
2023-01-23 23:10 ` [PATCH 2/2] iio: light: Add support " Markuss Broks
0 siblings, 2 replies; 9+ messages in thread
From: Markuss Broks @ 2023-01-23 23:10 UTC (permalink / raw)
To: linux-kernel
Cc: Markuss Broks, Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Andy Shevchenko, Paul Gazzillo, Zhigang Shi,
Dmitry Osipenko, Shreeya Patel, linux-iio, devicetree
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
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH 1/2] dt-bindings: iio: tcs3490: Add bindings for AMS TCS3490 light sensor 2023-01-23 23:10 [PATCH 0/2] Add a driver for AMS TCS3490 light sensor Markuss Broks @ 2023-01-23 23:10 ` Markuss Broks 2023-01-24 10:32 ` Krzysztof Kozlowski 2023-01-23 23:10 ` [PATCH 2/2] iio: light: Add support " Markuss Broks 1 sibling, 1 reply; 9+ messages in thread From: Markuss Broks @ 2023-01-23 23:10 UTC (permalink / raw) To: linux-kernel Cc: Markuss Broks, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Andy Shevchenko, Paul Gazzillo, Dmitry Osipenko, Shreeya Patel, Zhigang Shi, linux-iio, devicetree Add device-tree bindings for the AMS TCS3490 Color ALS. Signed-off-by: Markuss Broks <markuss.broks@gmail.com> --- .../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 <markuss.broks@gmail.com> + +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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: tcs3490: Add bindings for AMS TCS3490 light sensor 2023-01-23 23:10 ` [PATCH 1/2] dt-bindings: iio: tcs3490: Add bindings " Markuss Broks @ 2023-01-24 10:32 ` Krzysztof Kozlowski 0 siblings, 0 replies; 9+ messages in thread From: Krzysztof Kozlowski @ 2023-01-24 10:32 UTC (permalink / raw) To: Markuss Broks, linux-kernel Cc: Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Andy Shevchenko, Paul Gazzillo, Dmitry Osipenko, Shreeya Patel, Zhigang Shi, linux-iio, devicetree 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 <krzysztof.kozlowski@linaro.org> > > Signed-off-by: Markuss Broks <markuss.broks@gmail.com> > --- Best regards, Krzysztof ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor 2023-01-23 23:10 [PATCH 0/2] Add a driver for AMS TCS3490 light sensor Markuss Broks 2023-01-23 23:10 ` [PATCH 1/2] dt-bindings: iio: tcs3490: Add bindings " Markuss Broks @ 2023-01-23 23:10 ` Markuss Broks 2023-01-24 10:22 ` Andy Shevchenko 2023-01-28 17:33 ` Jonathan Cameron 1 sibling, 2 replies; 9+ messages in thread From: Markuss Broks @ 2023-01-23 23:10 UTC (permalink / raw) To: linux-kernel Cc: Markuss Broks, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Andy Shevchenko, Zhigang Shi, Dmitry Osipenko, Paul Gazzillo, Shreeya Patel, linux-iio, devicetree 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 <markuss.broks@gmail.com> --- 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 <markuss.broks@gmail.com> +L: linux-iio@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/iio/light/ams,tcs3490.yaml +F: drivers/iio/light/tcs3490.c + AMT (Automatic Multicast Tunneling) M: Taehee Yoo <ap420073@gmail.com> L: netdev@vger.kernel.org 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 <markuss.broks@gmail.com> + */ + +#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 <markuss.broks@gmail.com>"); +MODULE_LICENSE("GPL"); -- 2.39.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor 2023-01-23 23:10 ` [PATCH 2/2] iio: light: Add support " Markuss Broks @ 2023-01-24 10:22 ` Andy Shevchenko 2023-01-26 15:54 ` Markuss Broks 2023-01-28 17:33 ` Jonathan Cameron 1 sibling, 1 reply; 9+ messages in thread From: Andy Shevchenko @ 2023-01-24 10:22 UTC (permalink / raw) To: Markuss Broks Cc: linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Zhigang Shi, Dmitry Osipenko, Paul Gazzillo, Shreeya Patel, linux-iio, devicetree 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 <markuss.broks@gmail.com> > +L: linux-iio@vger.kernel.org > +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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor 2023-01-24 10:22 ` Andy Shevchenko @ 2023-01-26 15:54 ` Markuss Broks 2023-01-26 17:41 ` Jonathan Cameron 2023-01-27 9:09 ` Andy Shevchenko 0 siblings, 2 replies; 9+ messages in thread From: Markuss Broks @ 2023-01-26 15:54 UTC (permalink / raw) To: Andy Shevchenko Cc: linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Zhigang Shi, Dmitry Osipenko, Paul Gazzillo, Shreeya Patel, linux-iio, devicetree 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 <markuss.broks@gmail.com> >> +L: linux-iio@vger.kernel.org >> +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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor 2023-01-26 15:54 ` Markuss Broks @ 2023-01-26 17:41 ` Jonathan Cameron 2023-01-27 9:09 ` Andy Shevchenko 1 sibling, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2023-01-26 17:41 UTC (permalink / raw) To: Markuss Broks Cc: Andy Shevchenko, linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Zhigang Shi, Dmitry Osipenko, Paul Gazzillo, Shreeya Patel, linux-iio, devicetree On Thu, 26 Jan 2023 17:54:56 +0200 Markuss Broks <markuss.broks@gmail.com> 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 <markuss.broks@gmail.com> > >> +L: linux-iio@vger.kernel.org > >> +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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor 2023-01-26 15:54 ` Markuss Broks 2023-01-26 17:41 ` Jonathan Cameron @ 2023-01-27 9:09 ` Andy Shevchenko 1 sibling, 0 replies; 9+ messages in thread From: Andy Shevchenko @ 2023-01-27 9:09 UTC (permalink / raw) To: Markuss Broks Cc: linux-kernel, Jonathan Cameron, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Zhigang Shi, Dmitry Osipenko, Paul Gazzillo, Shreeya Patel, linux-iio, devicetree 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] iio: light: Add support for AMS TCS3490 light sensor 2023-01-23 23:10 ` [PATCH 2/2] iio: light: Add support " Markuss Broks 2023-01-24 10:22 ` Andy Shevchenko @ 2023-01-28 17:33 ` Jonathan Cameron 1 sibling, 0 replies; 9+ messages in thread From: Jonathan Cameron @ 2023-01-28 17:33 UTC (permalink / raw) To: Markuss Broks Cc: linux-kernel, Lars-Peter Clausen, Rob Herring, Krzysztof Kozlowski, Andy Shevchenko, Zhigang Shi, Dmitry Osipenko, Paul Gazzillo, Shreeya Patel, linux-iio, devicetree On Tue, 24 Jan 2023 01:10:25 +0200 Markuss Broks <markuss.broks@gmail.com> 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 <markuss.broks@gmail.com> 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 <ap420073@gmail.com> > L: netdev@vger.kernel.org > 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 <markuss.broks@gmail.com> > + */ > + > +#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 <markuss.broks@gmail.com>"); > +MODULE_LICENSE("GPL"); ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-28 17:19 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-23 23:10 [PATCH 0/2] Add a driver for AMS TCS3490 light sensor Markuss Broks 2023-01-23 23:10 ` [PATCH 1/2] dt-bindings: iio: tcs3490: Add bindings " Markuss Broks 2023-01-24 10:32 ` Krzysztof Kozlowski 2023-01-23 23:10 ` [PATCH 2/2] iio: light: Add support " Markuss Broks 2023-01-24 10:22 ` Andy Shevchenko 2023-01-26 15:54 ` Markuss Broks 2023-01-26 17:41 ` Jonathan Cameron 2023-01-27 9:09 ` Andy Shevchenko 2023-01-28 17:33 ` Jonathan Cameron
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.