| Message ID | 20230123231028.26073-3-markuss.broks@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add a driver for AMS TCS3490 light sensor | expand |
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);
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
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.
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. > > > +};
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");
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