| Message ID | ZR1P278MB111789B39F5CC0086DAFA35081E22@ZR1P278MB1117.CHEP278.PROD.OUTLOOK.COM (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [V2] iio: light: driver for Vishay VEML6040 | expand |
On Mon, 13 May 2024 14:33:39 +0000 Arthur Becker <arthur.becker@sentec.com> wrote: > Implements driver for the Vishay VEML6040 rgbw light sensor. > > Included functionality: setting the integration time and reading the raw > values for the four channels > > Not yet implemented: setting the measurements to 'Manual Force Mode' (Auto > measurements off, and adding a measurement trigger) > > Datasheet: https://www.vishay.com/docs/84276/veml6040.pdf > signed-off-by: Arthur Becker <arthur.becker@sentec.com> > --- > V1 -> V2: Addressed review comments. DT-bindings in separate patch Needs to be in the same series - with a cover-letter. That explains the confusion going on wrt to the binding patch. I was assuming this was unintentional mail server stuff but from this comment I guess it's a misunderstanding. Various comments inline. Pretty much all minor style things as in general the driver is looking nice. Jonathan > diff --git a/drivers/iio/light/veml6040.c b/drivers/iio/light/veml6040.c > new file mode 100644 > index 000000000000..9ce807d5484a > --- /dev/null > +++ b/drivers/iio/light/veml6040.c > @@ -0,0 +1,307 @@ > +#include <linux/bitfield.h> > +#include <linux/err.h> > +#include <linux/i2c.h> > +#include <linux/iio/iio.h> > +#include <linux/iio/sysfs.h> > +#include <linux/module.h> > +#include <linux/regmap.h> > + /* * VEML... > +/* VEML6040 Configuration Registers > + * > + * SD: Shutdown > + * AF: Auto / Force Mode (Auto Measurements On:0, Off:1) > + * TR: Trigger Measurement (when AF Bit is set) > + * IT: Integration Time > + */ > +#define VEML6040_CONF_REG_RW 0x000 > +#define VEML6040_CONF_SD_MSK BIT(0) > +#define VEML6040_CONF_AF_MSK BIT(1) > +#define VEML6040_CONF_TR_MSK BIT(2) > +#define VEML6040_CONF_IT_MSK GENMASK(6, 4) > +#define VEML6040_CONF_IT_40 0 > +#define VEML6040_CONF_IT_80 1 > +#define VEML6040_CONF_IT_160 2 > +#define VEML6040_CONF_IT_320 3 > +#define VEML6040_CONF_IT_640 4 > +#define VEML6040_CONF_IT_1280 5 > + > +/* VEML6040 Read Only Registers */ > +#define VEML6040_REG_R_RO 0x08 > +#define VEML6040_REG_G_RO 0x09 > +#define VEML6040_REG_B_RO 0x0A > +#define VEML6040_REG_W_RO 0x0B I'd be tempted to drop the _RW / _RO postfix. It's a nice thing in more complex drives, but feels unnecessary for a device with a small interface like this where the register usage kind of makes it obvious anyway. Channel registers are RO because writing them makes not sense and config is RW because it's for configuration. If you really like it then I don't mind that much. > +static int veml6040_read_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int *val, > + int *val2, long mask) > +{ > + int ret, reg, it_index; > + struct veml6040_data *data = iio_priv(indio_dev); > + struct regmap *regmap = data->regmap; > + struct device *dev = &data->client->dev; > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + switch (chan->channel) { Put VEML6040_REG_R_R0 etc in chan->address when you define the iio_chan_spec array as then this becomes ret = regmap_read(regmap, chan->addr, ®); if (ret) { dev_err(dev, "Data read failed: %d\n", ret); return ret; } *val = reg; return IIO_VAL_INT; The address field is there for precisely this kind of case where you just need a mapping to a specific register address. > + case CH_RED: > + ret = regmap_read(regmap, VEML6040_REG_R_RO, ®); > + break; > + case CH_GREEN: > + ret = regmap_read(regmap, VEML6040_REG_G_RO, ®); > + break; > + case CH_BLUE: > + ret = regmap_read(regmap, VEML6040_REG_B_RO, ®); > + break; > + case CH_WHITE: > + ret = regmap_read(regmap, VEML6040_REG_W_RO, ®); > + break; > + default: > + return -EINVAL; > + } > + if (ret) { > + dev_err(dev, "iio-veml6040 - Can't read data %d\n", No need to list the driver, that's easy to establish from what dev_err includes anyway. > + ret); > + return ret; > + } > + *val = reg; > + return IIO_VAL_INT; > + > + case IIO_CHAN_INFO_INT_TIME: > + ret = regmap_read(regmap, VEML6040_CONF_REG_RW, ®); > + if (ret) { > + dev_err(dev, "iio-veml6040 - Can't read data %d\n", > + ret); > + return ret; > + } > + it_index = FIELD_GET(VEML6040_CONF_IT_MSK, reg); > + if (it_index >= ARRAY_SIZE(veml6040_int_time_avail)) { > + dev_err(dev, > + "iio-veml6040 - Invalid Integration Time Set"); > + return -EINVAL; > + } > + *val = veml6040_int_time_avail[it_index]; > + return IIO_VAL_INT; > + > + default: > + return -EINVAL; > + } > +} > + > +static int veml6040_write_raw(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, int val, > + int val2, long mask) > +{ > + struct veml6040_data *data = iio_priv(indio_dev); > + > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + for (int i = 0; i < ARRAY_SIZE(veml6040_int_time_avail); i++) { > + if (veml6040_int_time_avail[i] == val) { > + return regmap_update_bits( > + data->regmap, VEML6040_CONF_REG_RW, > + VEML6040_CONF_IT_MSK, > + FIELD_PREP(VEML6040_CONF_IT_MSK, i)); > + } Trick to reduce indent - flip logic ;) if (veml6040_int_time_avail[i] != val) continue; return regmap_update_bits(regmap, VEML6040_CONF_REG_RW, VEML6040_CONF_IT_MSK, FIELD_PREP(VEML6040_CONF_IT_MSK, i)); //case where it is worth going past 80 chars for readability. > + } > + return -EINVAL; > + default: > + return -EINVAL; > + } > + return 0; Can't get here so drop this final return. > +} > + > +static int veml6040_read_avail(struct iio_dev *indio_dev, > + struct iio_chan_spec const *chan, > + const int **vals, int *type, int *length, > + long mask) > +{ > + switch (mask) { > + case IIO_CHAN_INFO_INT_TIME: > + *length = ARRAY_SIZE(veml6040_int_time_avail); > + *vals = veml6040_int_time_avail; > + *type = IIO_VAL_INT; That makes me suspicious. The integration times are measured in seconds and you only have integer values? Check all your ABI against the docs in Documentation/ABI/testing/sysfs-bus-iio > + return IIO_AVAIL_LIST; > + > + default: > + return -EINVAL; > + } > +} > + > +static int veml6040_shutdown(struct veml6040_data *data) > +{ > + return regmap_update_bits(data->regmap, VEML6040_CONF_REG_RW, > + VEML6040_CONF_SD_MSK, VEML6040_CONF_SD_MSK); > +} > + > +static void veml6040_shutdown_action(void *data) > +{ > + veml6040_shutdown(data); Combine these two functions into one as only called via this one. > +} > + > +static int veml6040_probe(struct i2c_client *client) > +{ Add a struct device *dev = client->dev; and use it throughout to avoid looking this up in every error print etc. > + struct veml6040_data *data; > + struct iio_dev *indio_dev; > + struct regmap *regmap; > + int ret; > + > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { > + return dev_err_probe(&client->dev, -EOPNOTSUPP, > + "I2C adapter doesn't support plain I2C\n"); > + } > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > + if (!indio_dev) { > + return dev_err_probe(&client->dev, -ENOMEM, > + "IIO device allocation failed\n"); > + } > + > + regmap = devm_regmap_init_i2c(client, &veml6040_regmap_config); > + if (IS_ERR(regmap)) { > + return dev_err_probe(&client->dev, PTR_ERR(regmap), > + "Regmap setup failed\n"); > + } > + > + data = iio_priv(indio_dev); > + i2c_set_clientdata(client, indio_dev); > + data->client = client; > + data->regmap = regmap; > + > + indio_dev->name = "veml6040"; > + indio_dev->info = &veml6040_info; > + indio_dev->channels = veml6040_channels; > + indio_dev->num_channels = ARRAY_SIZE(veml6040_channels); > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + ret = regmap_update_bits( > + data->regmap, VEML6040_CONF_REG_RW, VEML6040_CONF_IT_MSK, You have regmap locally, so use that instead of via data->regmap. > + FIELD_PREP(VEML6040_CONF_IT_MSK, VEML6040_CONF_IT_40)); Unusual wrapping that we tend to only use where there are individual parameters that can't be easily wrapped. Preferred as ret = regmap_update_bits(regmap, VEML6040_CONF_REG_RW, VEML6040_CONF_IT_MSK, FIELD_PREP(VEML6040_CONF_IT_MASK, VEML6040_CONF_IT_40)); > + > + if (ret) { > + return dev_err_probe(&client->dev, ret, > + "Could not set Integration Time: %d\n", > + ret); > + } if (ret) return dev_err_probe(dev, ret, "Could not set Integration Time\n"); similar in other cases. > + > + ret = regmap_update_bits(data->regmap, VEML6040_CONF_REG_RW, > + VEML6040_CONF_AF_MSK, > + FIELD_PREP(VEML6040_CONF_AF_MSK, 0)); > + if (ret) { > + return dev_err_probe(&client->dev, ret, > + "Could not set Automode: %d\n", ret); > + } You are writing multiple fields in the same register. Can you just do a single write? Build the mask and values then a single call to regmap_update_bits or just write the remaining couple of bits to their existing defaults so we know what state they are in. If there is a reason this needs to be done as multiple writes, then add some comments to explain the sequencing. > + > + ret = regmap_update_bits(data->regmap, VEML6040_CONF_REG_RW, > + VEML6040_CONF_SD_MSK, > + FIELD_PREP(VEML6040_CONF_SD_MSK, 0)); > + if (ret) { > + return dev_err_probe(&client->dev, ret, > + "Could not set Enable: %d\n", ret); > + } No need for brackets around a single statement like above. Fix all of these as there are a lot of them! > + > + ret = devm_add_action_or_reset(&client->dev, veml6040_shutdown_action, > + data); > + if (ret) { > + return dev_err_probe(&client->dev, ret, > + "Could not add shutdown action: %d\n", > + ret); You should look at what dev_err_probe() does. Don't print the ret value in your messages - dev_err_probe() does it much better! Some of these are also vanishingly unlikely to fail so you could reduce where you print messages to the ones that are more to do with bus accesses etc. This one is an example of that as it can only fail if a very small allocation fails. > + } > + > + return devm_iio_device_register(&client->dev, indio_dev); > +} > + > +static const struct i2c_device_id veml6040_id_table[] = { { "veml6040", 0 }, > + {} }; I'd prefer this in the more common formatting of static const struct i2c_device_id veml6040_id_table[] = { { "veml6040" }, //note I dropped the 0 as no point in setting that when it's unused. { } }; > +MODULE_DEVICE_TABLE(i2c, veml6040_id_table); > +
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig index fd5a9879a582..7ff517b728ec 100644 --- a/drivers/iio/light/Kconfig +++ b/drivers/iio/light/Kconfig @@ -654,6 +654,17 @@ config VEML6030 To compile this driver as a module, choose M here: the module will be called veml6030. +config VEML6040 + tristate "VEML6040 RGBW light sensor" + select REGMAP_I2C + depends on I2C + help + Say Y here if you want to build a driver for the Vishay VEML6040 + RGBW light sensor. + + To compile this driver as a module, choose M here: the + module will be called veml6040. + config VEML6070 tristate "VEML6070 UV A light sensor" depends on I2C diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile index 2e5fdb33e0e9..ae957c88aa0c 100644 --- a/drivers/iio/light/Makefile +++ b/drivers/iio/light/Makefile @@ -61,6 +61,7 @@ obj-$(CONFIG_US5182D) += us5182d.o obj-$(CONFIG_VCNL4000) += vcnl4000.o obj-$(CONFIG_VCNL4035) += vcnl4035.o obj-$(CONFIG_VEML6030) += veml6030.o +obj-$(CONFIG_VEML6040) += veml6040.o obj-$(CONFIG_VEML6070) += veml6070.o obj-$(CONFIG_VEML6075) += veml6075.o obj-$(CONFIG_VL6180) += vl6180.o diff --git a/drivers/iio/light/veml6040.c b/drivers/iio/light/veml6040.c new file mode 100644 index 000000000000..9ce807d5484a --- /dev/null +++ b/drivers/iio/light/veml6040.c @@ -0,0 +1,307 @@ +#include <linux/bitfield.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/module.h> +#include <linux/regmap.h> + +/* VEML6040 Configuration Registers + * + * SD: Shutdown + * AF: Auto / Force Mode (Auto Measurements On:0, Off:1) + * TR: Trigger Measurement (when AF Bit is set) + * IT: Integration Time + */ +#define VEML6040_CONF_REG_RW 0x000 +#define VEML6040_CONF_SD_MSK BIT(0) +#define VEML6040_CONF_AF_MSK BIT(1) +#define VEML6040_CONF_TR_MSK BIT(2) +#define VEML6040_CONF_IT_MSK GENMASK(6, 4) +#define VEML6040_CONF_IT_40 0 +#define VEML6040_CONF_IT_80 1 +#define VEML6040_CONF_IT_160 2 +#define VEML6040_CONF_IT_320 3 +#define VEML6040_CONF_IT_640 4 +#define VEML6040_CONF_IT_1280 5 + +/* VEML6040 Read Only Registers */ +#define VEML6040_REG_R_RO 0x08 +#define VEML6040_REG_G_RO 0x09 +#define VEML6040_REG_B_RO 0x0A +#define VEML6040_REG_W_RO 0x0B + +static const int veml6040_int_time_avail[] = { 40, 80, 160, 320, 640, 1280 }; + +enum veml6040_chan { + CH_RED, + CH_GREEN, + CH_BLUE, + CH_WHITE, +}; + +struct veml6040_data { + struct i2c_client *client; + struct regmap *regmap; +}; + +static const struct regmap_config veml6040_regmap_config = { + .name = "veml6040_regmap", + .reg_bits = 8, + .val_bits = 16, + .max_register = VEML6040_REG_W_RO, + .val_format_endian = REGMAP_ENDIAN_LITTLE, +}; + +static int veml6040_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val, + int *val2, long mask) +{ + int ret, reg, it_index; + struct veml6040_data *data = iio_priv(indio_dev); + struct regmap *regmap = data->regmap; + struct device *dev = &data->client->dev; + + switch (mask) { + case IIO_CHAN_INFO_RAW: + switch (chan->channel) { + case CH_RED: + ret = regmap_read(regmap, VEML6040_REG_R_RO, ®); + break; + case CH_GREEN: + ret = regmap_read(regmap, VEML6040_REG_G_RO, ®); + break; + case CH_BLUE: + ret = regmap_read(regmap, VEML6040_REG_B_RO, ®); + break; + case CH_WHITE: + ret = regmap_read(regmap, VEML6040_REG_W_RO, ®); + break; + default: + return -EINVAL; + } + if (ret) { + dev_err(dev, "iio-veml6040 - Can't read data %d\n", + ret); + return ret; + } + *val = reg; + return IIO_VAL_INT; + + case IIO_CHAN_INFO_INT_TIME: + ret = regmap_read(regmap, VEML6040_CONF_REG_RW, ®); + if (ret) { + dev_err(dev, "iio-veml6040 - Can't read data %d\n", + ret); + return ret; + } + it_index = FIELD_GET(VEML6040_CONF_IT_MSK, reg); + if (it_index >= ARRAY_SIZE(veml6040_int_time_avail)) { + dev_err(dev, + "iio-veml6040 - Invalid Integration Time Set"); + return -EINVAL; + } + *val = veml6040_int_time_avail[it_index]; + return IIO_VAL_INT; + + default: + return -EINVAL; + } +} + +static int veml6040_write_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int val, + int val2, long mask) +{ + struct veml6040_data *data = iio_priv(indio_dev); + + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + for (int i = 0; i < ARRAY_SIZE(veml6040_int_time_avail); i++) { + if (veml6040_int_time_avail[i] == val) { + return regmap_update_bits( + data->regmap, VEML6040_CONF_REG_RW, + VEML6040_CONF_IT_MSK, + FIELD_PREP(VEML6040_CONF_IT_MSK, i)); + } + } + return -EINVAL; + default: + return -EINVAL; + } + return 0; +} + +static int veml6040_read_avail(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, + const int **vals, int *type, int *length, + long mask) +{ + switch (mask) { + case IIO_CHAN_INFO_INT_TIME: + *length = ARRAY_SIZE(veml6040_int_time_avail); + *vals = veml6040_int_time_avail; + *type = IIO_VAL_INT; + return IIO_AVAIL_LIST; + + default: + return -EINVAL; + } +} + +static const struct iio_info veml6040_info = { + .read_raw = veml6040_read_raw, + .write_raw = veml6040_write_raw, + .read_avail = veml6040_read_avail, +}; + +static const struct iio_chan_spec veml6040_channels[] = { + { + .type = IIO_INTENSITY, + .channel = CH_RED, + .channel2 = IIO_MOD_LIGHT_RED, + .modified = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME), + .info_mask_shared_by_type_available = + BIT(IIO_CHAN_INFO_INT_TIME), + }, + { + .type = IIO_INTENSITY, + .channel = CH_GREEN, + .channel2 = IIO_MOD_LIGHT_GREEN, + .modified = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME), + .info_mask_shared_by_type_available = + BIT(IIO_CHAN_INFO_INT_TIME), + }, + { + .type = IIO_INTENSITY, + .channel = CH_BLUE, + .channel2 = IIO_MOD_LIGHT_BLUE, + .modified = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME), + .info_mask_shared_by_type_available = + BIT(IIO_CHAN_INFO_INT_TIME), + }, + { + .type = IIO_INTENSITY, + .channel = CH_WHITE, + .channel2 = IIO_MOD_LIGHT_CLEAR, + .modified = 1, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_INT_TIME), + .info_mask_shared_by_type_available = + BIT(IIO_CHAN_INFO_INT_TIME), + } +}; + +static int veml6040_shutdown(struct veml6040_data *data) +{ + return regmap_update_bits(data->regmap, VEML6040_CONF_REG_RW, + VEML6040_CONF_SD_MSK, VEML6040_CONF_SD_MSK); +} + +static void veml6040_shutdown_action(void *data) +{ + veml6040_shutdown(data); +} + +static int veml6040_probe(struct i2c_client *client) +{ + struct veml6040_data *data; + struct iio_dev *indio_dev; + struct regmap *regmap; + int ret; + + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) { + return dev_err_probe(&client->dev, -EOPNOTSUPP, + "I2C adapter doesn't support plain I2C\n"); + } + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); + if (!indio_dev) { + return dev_err_probe(&client->dev, -ENOMEM, + "IIO device allocation failed\n"); + } + + regmap = devm_regmap_init_i2c(client, &veml6040_regmap_config); + if (IS_ERR(regmap)) { + return dev_err_probe(&client->dev, PTR_ERR(regmap), + "Regmap setup failed\n"); + } + + data = iio_priv(indio_dev); + i2c_set_clientdata(client, indio_dev); + data->client = client; + data->regmap = regmap; + + indio_dev->name = "veml6040"; + indio_dev->info = &veml6040_info; + indio_dev->channels = veml6040_channels; + indio_dev->num_channels = ARRAY_SIZE(veml6040_channels); + indio_dev->modes = INDIO_DIRECT_MODE; + + ret = regmap_update_bits( + data->regmap, VEML6040_CONF_REG_RW, VEML6040_CONF_IT_MSK, + FIELD_PREP(VEML6040_CONF_IT_MSK, VEML6040_CONF_IT_40)); + + if (ret) { + return dev_err_probe(&client->dev, ret, + "Could not set Integration Time: %d\n", + ret); + } + + ret = regmap_update_bits(data->regmap, VEML6040_CONF_REG_RW, + VEML6040_CONF_AF_MSK, + FIELD_PREP(VEML6040_CONF_AF_MSK, 0)); + if (ret) { + return dev_err_probe(&client->dev, ret, + "Could not set Automode: %d\n", ret); + } + + ret = regmap_update_bits(data->regmap, VEML6040_CONF_REG_RW, + VEML6040_CONF_SD_MSK, + FIELD_PREP(VEML6040_CONF_SD_MSK, 0)); + if (ret) { + return dev_err_probe(&client->dev, ret, + "Could not set Enable: %d\n", ret); + } + + ret = devm_add_action_or_reset(&client->dev, veml6040_shutdown_action, + data); + if (ret) { + return dev_err_probe(&client->dev, ret, + "Could not add shutdown action: %d\n", + ret); + } + + return devm_iio_device_register(&client->dev, indio_dev); +} + +static const struct i2c_device_id veml6040_id_table[] = { { "veml6040", 0 }, + {} }; +MODULE_DEVICE_TABLE(i2c, veml6040_id_table); + +static const struct of_device_id veml6040_of_match[] = { + { .compatible = "vishay,veml6040" }, + {} +}; +MODULE_DEVICE_TABLE(of, veml6040_of_match); + +static struct i2c_driver veml6040_driver = { + .probe = veml6040_probe, + .id_table = veml6040_id_table, + .driver = { + .name = "veml6040", + .of_match_table = veml6040_of_match, + }, +}; + +module_i2c_driver(veml6040_driver); + +MODULE_DESCRIPTION("veml6040 RGBW light sensor driver"); +MODULE_AUTHOR("Arthur Becker <arthur.becker@sentec.com>"); +MODULE_LICENSE("GPL");
Implements driver for the Vishay VEML6040 rgbw light sensor. Included functionality: setting the integration time and reading the raw values for the four channels Not yet implemented: setting the measurements to 'Manual Force Mode' (Auto measurements off, and adding a measurement trigger) Datasheet: https://www.vishay.com/docs/84276/veml6040.pdf signed-off-by: Arthur Becker <arthur.becker@sentec.com> --- V1 -> V2: Addressed review comments. DT-bindings in separate patch drivers/iio/light/Kconfig | 11 ++ drivers/iio/light/Makefile | 1 + drivers/iio/light/veml6040.c | 307 +++++++++++++++++++++++++++++++++++ 3 files changed, 319 insertions(+) create mode 100644 drivers/iio/light/veml6040.c