Thank you Jonathan, Krzysztof and Conor for your comments on my previous
submission attempts. I hope that I addressed all points!
Changes since v2:
- formatting, renaming of variables
- power supply added in dt-binding file and turned on in driver
(- using b4!)
Signed-off-by: Arthur Becker <[email protected]>
---
Arthur Becker (2):
iio: light: driver for Vishay VEML6040
dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
.../bindings/iio/light/vishay,veml6040.yaml | 44 ++++
drivers/iio/light/Kconfig | 11 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/veml6040.c | 283 +++++++++++++++++++++
4 files changed, 339 insertions(+)
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240527-veml6040-0314fc054337
Best regards,
--
Arthur Becker <[email protected]>
From: Arthur Becker <[email protected]>
Device tree bindings for the vishay VEML6040 RGBW light sensor iio
driver
Signed-off-by: Arthur Becker <[email protected]>
---
V1 -> V3: Addressed review comments (v1 of the dt-bindings was sent
along with v2 of the driver but not in a set)
---
.../bindings/iio/light/vishay,veml6040.yaml | 44 ++++++++++++++++++++++
1 file changed, 44 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml6040.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6040.yaml
new file mode 100644
index 000000000000..101c2cc6506e
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6040.yaml
@@ -0,0 +1,44 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/vishay,veml6040.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: VEML6040 RGBW Light Sensor
+
+maintainers:
+ - Arthur Becker <[email protected]>
+
+description:
+ Datasheet at https://www.vishay.com/docs/84276/veml6040.pdf
+
+properties:
+ compatible:
+ const: vishay,veml6040
+
+ reg:
+ enum:
+ - 0x10
+
+ vdd-supply: true
+
+required:
+ - compatible
+ - reg
+ - vdd-supply
+
+additionalProperties: false
+
+examples:
+ - |
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ light-sensor@10 {
+ compatible = "vishay,veml6040";
+ reg = <0x10>;
+ vdd-supply = <&vdd_reg>;
+ };
+ };
+...
--
2.34.1
From: Arthur Becker <[email protected]>
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 <[email protected]>
---
V1 -> V2: Addressed review comments.
V2 -> V3: Addressed further review comments; Sending in patch thread
with dt-bindings.
---
drivers/iio/light/Kconfig | 11 ++
drivers/iio/light/Makefile | 1 +
drivers/iio/light/veml6040.c | 283 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 295 insertions(+)
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 9a587d403118..b68dcc1fbaca 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -666,6 +666,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 a30f906e91ba..1a071a8e9f8e 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -62,6 +62,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..2ea00d57c38b
--- /dev/null
+++ b/drivers/iio/light/veml6040.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Vishay VEML6040 RGBW light sensor driver
+ *
+ * Copyright (C) 2024 Sentec AG
+ * Author: Arthur Becker <[email protected]>
+ *
+ */
+
+#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 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_MS 0
+#define VEML6040_CONF_IT_80_MS 1
+#define VEML6040_CONF_IT_160_MS 2
+#define VEML6040_CONF_IT_320_MS 3
+#define VEML6040_CONF_IT_640_MS 4
+#define VEML6040_CONF_IT_1280_MS 5
+
+/* VEML6040 Read Only Registers */
+#define VEML6040_REG_R 0x08
+#define VEML6040_REG_G 0x09
+#define VEML6040_REG_B 0x0A
+#define VEML6040_REG_W 0x0B
+
+static const int veml6040_it_ms[] = { 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,
+ .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:
+ ret = regmap_read(regmap, chan->address, ®);
+ if (ret) {
+ dev_err(dev, "Data read failed: %d\n", ret);
+ return ret;
+ }
+ *val = reg;
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_INT_TIME:
+ ret = regmap_read(regmap, VEML6040_CONF_REG, ®);
+ if (ret) {
+ dev_err(dev, "Data read failed: %d\n", ret);
+ return ret;
+ }
+ it_index = FIELD_GET(VEML6040_CONF_IT_MSK, reg);
+ if (it_index >= ARRAY_SIZE(veml6040_it_ms)) {
+ dev_err(dev, "Invalid Integration Time Set");
+ return -EINVAL;
+ }
+ *val = veml6040_it_ms[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_it_ms); i++) {
+ if (veml6040_it_ms[i] != val)
+ continue;
+
+ return regmap_update_bits(data->regmap,
+ VEML6040_CONF_REG,
+ VEML6040_CONF_IT_MSK,
+ FIELD_PREP(VEML6040_CONF_IT_MSK, i));
+ }
+ return -EINVAL;
+ default:
+ return -EINVAL;
+ }
+}
+
+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_it_ms);
+ *vals = veml6040_it_ms;
+ *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,
+ .address = VEML6040_REG_R,
+ .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,
+ .address = VEML6040_REG_G,
+ .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,
+ .address = VEML6040_REG_B,
+ .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,
+ .address = VEML6040_REG_W,
+ .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 void veml6040_shutdown_action(void *data)
+{
+ struct veml6040_data *veml6040_data = data;
+
+ regmap_update_bits(veml6040_data->regmap, VEML6040_CONF_REG,
+ VEML6040_CONF_SD_MSK, VEML6040_CONF_SD_MSK);
+}
+
+static int veml6040_probe(struct i2c_client *client)
+{
+ struct device *dev = &client->dev;
+ 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(dev, -EOPNOTSUPP,
+ "I2C adapter doesn't support plain I2C\n");
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+ if (!indio_dev)
+ return dev_err_probe(dev, -ENOMEM,
+ "IIO device allocation failed\n");
+
+ regmap = devm_regmap_init_i2c(client, &veml6040_regmap_config);
+ if (IS_ERR(regmap))
+ return dev_err_probe(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 = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return ret;
+
+ int init_config =
+ FIELD_PREP(VEML6040_CONF_IT_MSK, VEML6040_CONF_IT_40_MS) |
+ FIELD_PREP(VEML6040_CONF_AF_MSK, 0) |
+ FIELD_PREP(VEML6040_CONF_SD_MSK, 0);
+
+ ret = regmap_write(regmap, VEML6040_CONF_REG, init_config);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Could not set initial config\n");
+
+ ret = devm_add_action_or_reset(dev, veml6040_shutdown_action, data);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct i2c_device_id veml6040_id_table[] = {
+ {"veml6040"},
+ {}
+};
+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 <[email protected]>");
+MODULE_LICENSE("GPL");
--
2.34.1
On Mon, May 27, 2024 at 05:12:09PM +0200, Arthur Becker via B4 Relay wrote:
> From: Arthur Becker <[email protected]>
>
> Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> driver
>
> Signed-off-by: Arthur Becker <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Cheers,
Conor.
On 27/05/2024 17:12, Arthur Becker via B4 Relay wrote:
> From: Arthur Becker <[email protected]>
>
> Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> driver
>
> Signed-off-by: Arthur Becker <[email protected]>
> ---
> V1 -> V3: Addressed review comments (v1 of the dt-bindings was sent
> along with v2 of the driver but not in a set)
It's basically the same as veml6075, so should be put there...
Eh,
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
Thanks for the Review!
Cheers,
Arthur
________________________________________
From:?Conor Dooley
Sent:?Monday, May 27, 2024 18:30
To:?Arthur Becker
Cc:?Jonathan Cameron; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley; [email protected]; [email protected]; [email protected]
Subject:?[EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
On Mon, May 27, 2024 at 05:12:09PM +0200, Arthur Becker via B4 Relay wrote:
> From: Arthur Becker <[email protected]>
>
> Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> driver
>
> Signed-off-by: Arthur Becker <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Cheers,
Conor.
Thanks for the Review!
Right, I wasn't sure if and how to add the veml6040 to the veml6075 dt-binding file.
I'll modify that the next time I make adjustments to the driver.
Kind regards,
Arthur
________________________________________
From: Krzysztof Kozlowski <[email protected]>
Sent: 27 May 2024 18:31
To: Arthur Becker; Jonathan Cameron; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley
Cc: [email protected]; [email protected]; [email protected]
Subject: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
On 27/05/2024 17:12, Arthur Becker via B4 Relay wrote:
> From: Arthur Becker <[email protected]>
>
> Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> driver
>
> Signed-off-by: Arthur Becker <[email protected]>
> ---
> V1 -> V3: Addressed review comments (v1 of the dt-bindings was sent
> along with v2 of the driver but not in a set)
It's basically the same as veml6075, so should be put there...
Eh,
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Tue, 28 May 2024 07:23:03 +0000
Arthur Becker <[email protected]> wrote:
> Thanks for the Review!
> Right, I wasn't sure if and how to add the veml6040 to the veml6075 dt-binding file.
> I'll modify that the next time I make adjustments to the driver.
It's absolutely fine to have shared bindings even if the
drivers (because of different register interface etc) are completely
separate. It's a good way to keep bindings aligned between
similar devices.
Jonathan
>
> Kind regards,
> Arthur
>
> ________________________________________
> From: Krzysztof Kozlowski <[email protected]>
> Sent: 27 May 2024 18:31
> To: Arthur Becker; Jonathan Cameron; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley
> Cc: [email protected]; [email protected]; [email protected]
> Subject: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
>
> On 27/05/2024 17:12, Arthur Becker via B4 Relay wrote:
> > From: Arthur Becker <[email protected]>
> >
> > Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> > driver
> >
> > Signed-off-by: Arthur Becker <[email protected]>
> > ---
> > V1 -> V3: Addressed review comments (v1 of the dt-bindings was sent
> > along with v2 of the driver but not in a set)
>
> It's basically the same as veml6075, so should be put there...
>
> Eh,
>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>
> Best regards,
> Krzysztof
>
>
On Tue, May 28, 2024 at 07:23:03AM +0000, Arthur Becker wrote:
> Thanks for the Review!
> Right, I wasn't sure if and how to add the veml6040 to the veml6075 dt-binding file.
> I'll modify that the next time I make adjustments to the driver.
Please don't top post, quote replies correctly, and fix your mail setup
to not add 'EXTERNAL' to the subject.
Rob
>
> Kind regards,
> Arthur
>
> ________________________________________
> From: Krzysztof Kozlowski <[email protected]>
> Sent: 27 May 2024 18:31
> To: Arthur Becker; Jonathan Cameron; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley
> Cc: [email protected]; [email protected]; [email protected]
> Subject: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
>
> On 27/05/2024 17:12, Arthur Becker via B4 Relay wrote:
> > From: Arthur Becker <[email protected]>
> >
> > Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> > driver
> >
> > Signed-off-by: Arthur Becker <[email protected]>
> > ---
> > V1 -> V3: Addressed review comments (v1 of the dt-bindings was sent
> > along with v2 of the driver but not in a set)
>
> It's basically the same as veml6075, so should be put there...
>
> Eh,
>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>
> Best regards,
> Krzysztof
>
>On Tue, May 28, 2024 at 07:23:03AM +0000, Arthur Becker wrote:
>> Thanks for the Review!
>> Right, I wasn't sure if and how to add the veml6040 to the veml6075 dt-binding file.
>> I'll modify that the next time I make adjustments to the driver.
>
>Please don't top post, quote replies correctly, and fix your mail setup
>to not add 'EXTERNAL' to the subject.
>
>Rob
Hello Rob,
Sorry about that, I'll keep that in mind in the future. Having to work with oulook can be a bit frustrating.
Kind Regards,
Arthur
> From: Jonathan Cameron <[email protected]>
> Sent: 28 May 2024 12:05
> To: Arthur Becker
> Cc: Krzysztof Kozlowski; Jonathan Cameron; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley; [email protected]; [email protected]; [email protected]
> Subject: Re: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
>
> On Tue, 28 May 2024 07:23:03 +0000
> Arthur Becker <[email protected]> wrote:
>
> > Thanks for the Review!
> > Right, I wasn't sure if and how to add the veml6040 to the veml6075 dt-binding file.
> > I'll modify that the next time I make adjustments to the driver.
>
> It's absolutely fine to have shared bindings even if the
> drivers (because of different register interface etc) are completely
> separate. It's a good way to keep bindings aligned between
> similar devices.
>
> Jonathan
Thanks for the answer, I'll keep that in mind!
Kind regards,
Arthur
On Tue, 28 May 2024 07:23:03 +0000
Arthur Becker <[email protected]> wrote:
> Thanks for the Review!
> Right, I wasn't sure if and how to add the veml6040 to the veml6075 dt-binding file.
> I'll modify that the next time I make adjustments to the driver.
Hi Arthur,
If I read the above correctly you are hoping this merges as it stands and
we come back later. If we are going to combine them long term,
I'd rather we avoided the churn and had a combined DT binding from the start.
Jonathan
>
> Kind regards,
> Arthur
>
> ________________________________________
> From: Krzysztof Kozlowski <[email protected]>
> Sent: 27 May 2024 18:31
> To: Arthur Becker; Jonathan Cameron; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley
> Cc: [email protected]; [email protected]; [email protected]
> Subject: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
>
> On 27/05/2024 17:12, Arthur Becker via B4 Relay wrote:
> > From: Arthur Becker <[email protected]>
> >
> > Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> > driver
> >
> > Signed-off-by: Arthur Becker <[email protected]>
> > ---
> > V1 -> V3: Addressed review comments (v1 of the dt-bindings was sent
> > along with v2 of the driver but not in a set)
>
> It's basically the same as veml6075, so should be put there...
>
> Eh,
>
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
>
> Best regards,
> Krzysztof
>
On Mon, 27 May 2024 17:12:08 +0200
Arthur Becker via B4 Relay <[email protected]> wrote:
> From: Arthur Becker <[email protected]>
>
> 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 <[email protected]>
Hi Arthur,
A few really trivial things inline. I'd have just tidied them up whilst
applying, but I think you are doing a v4 anyway to merge the bindings
so I'll leave the requested tweaks to you.
Jonathan
> diff --git a/drivers/iio/light/veml6040.c b/drivers/iio/light/veml6040.c
> new file mode 100644
> index 000000000000..2ea00d57c38b
> --- /dev/null
> +++ b/drivers/iio/light/veml6040.c
> +static int veml6040_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + 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(dev, -EOPNOTSUPP,
> + "I2C adapter doesn't support plain I2C\n");
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!indio_dev)
> + return dev_err_probe(dev, -ENOMEM,
> + "IIO device allocation failed\n");
> +
> + regmap = devm_regmap_init_i2c(client, &veml6040_regmap_config);
> + if (IS_ERR(regmap))
> + return dev_err_probe(dev, PTR_ERR(regmap),
> + "Regmap setup failed\n");
> +
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
I don't think this is used. If it isn't, don't set it.
> + 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 = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return ret;
> +
> + int init_config =
Generally we are still sticking to traditional C rules so keep the
local variable definition at the top of the file.
The only common exception is when cleanup.h functionality is involved and
we want to ensure ordering by moving the variable definitions into the code.
> + FIELD_PREP(VEML6040_CONF_IT_MSK, VEML6040_CONF_IT_40_MS) |
> + FIELD_PREP(VEML6040_CONF_AF_MSK, 0) |
> + FIELD_PREP(VEML6040_CONF_SD_MSK, 0);
> +
> + ret = regmap_write(regmap, VEML6040_CONF_REG, init_config);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Could not set initial config\n");
> +
> + ret = devm_add_action_or_reset(dev, veml6040_shutdown_action, data);
> + if (ret)
> + return ret;
> +
> + return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static struct i2c_driver veml6040_driver = {
> + .probe = veml6040_probe,
> + .id_table = veml6040_id_table,
> + .driver = {
> + .name = "veml6040",
> + .of_match_table = veml6040_of_match,
> + },
> +};
> +
Trivial: Common practice is no new line here as the macro is
very tightly coupled to the structure.
> +module_i2c_driver(veml6040_driver);
> +
> +MODULE_DESCRIPTION("veml6040 RGBW light sensor driver");
> +MODULE_AUTHOR("Arthur Becker <[email protected]>");
> +MODULE_LICENSE("GPL");
>
From: Jonathan Cameron <[email protected]>
Sent: 02 June 2024 15:16
To: Arthur Becker
Cc: Krzysztof Kozlowski; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley; [email protected]; [email protected]; [email protected]
Subject: Re: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
> On Tue, 28 May 2024 07:23:03 +0000
> Arthur Becker <[email protected]> wrote:
>
> > Thanks for the Review!
> > Right, I wasn't sure if and how to add the veml6040 to the veml6075 dt-binding file.
> > I'll modify that the next time I make adjustments to the driver.
>
> Hi Arthur,
>
> If I read the above correctly you are hoping this merges as it stands and
> we come back later. If we are going to combine them long term,
> I'd rather we avoided the churn and had a combined DT binding from the start.
Hi Jonathan,
I could have phrased that better, what I meant was that I was waiting for the next
feedback on the driver to make the adjustments all at once.
I'll get to it shortly!
Kind Regards,
Arthur
>
> Jonathan
>
> >
> > Kind regards,
> > Arthur
> >
> > ________________________________________
> > From: Krzysztof Kozlowski <[email protected]>
> > Sent: 27 May 2024 18:31
> > To: Arthur Becker; Jonathan Cameron; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley
> > Cc: [email protected]; [email protected]; [email protected]
> > Subject: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
> >
> > On 27/05/2024 17:12, Arthur Becker via B4 Relay wrote:
> > > From: Arthur Becker <[email protected]>
> > >
> > > Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> > > driver
> > >
> > > Signed-off-by: Arthur Becker <[email protected]>
> > > ---
> > > V1 -> V3: Addressed review comments (v1 of the dt-bindings was sent
> > > along with v2 of the driver but not in a set)
> >
> > It's basically the same as veml6075, so should be put there...
> >
> > Eh,
> >
> > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> >
> > Best regards,
> > Krzysztof
> >
On Mon, 3 Jun 2024 08:23:55 +0000
Arthur Becker <[email protected]> wrote:
> From: Jonathan Cameron <[email protected]>
> Sent: 02 June 2024 15:16
> To: Arthur Becker
> Cc: Krzysztof Kozlowski; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley; [email protected]; [email protected]; [email protected]
> Subject: Re: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
>
> > On Tue, 28 May 2024 07:23:03 +0000
> > Arthur Becker <[email protected]> wrote:
> >
> > > Thanks for the Review!
> > > Right, I wasn't sure if and how to add the veml6040 to the veml6075 dt-binding file.
> > > I'll modify that the next time I make adjustments to the driver.
> >
> > Hi Arthur,
> >
> > If I read the above correctly you are hoping this merges as it stands and
> > we come back later. If we are going to combine them long term,
> > I'd rather we avoided the churn and had a combined DT binding from the start.
>
> Hi Jonathan,
>
> I could have phrased that better, what I meant was that I was waiting for the next
> feedback on the driver to make the adjustments all at once.
> I'll get to it shortly!
Great :)
Thanks,
Jonathan
>
> Kind Regards,
> Arthur
>
> >
> > Jonathan
> >
> > >
> > > Kind regards,
> > > Arthur
> > >
> > > ________________________________________
> > > From: Krzysztof Kozlowski <[email protected]>
> > > Sent: 27 May 2024 18:31
> > > To: Arthur Becker; Jonathan Cameron; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley
> > > Cc: [email protected]; [email protected]; [email protected]
> > > Subject: [EXTERNAL]Re: [PATCH v3 2/2] dt-bindings: iio: light: add VEML6040 RGBW-LS bindings
> > >
> > > On 27/05/2024 17:12, Arthur Becker via B4 Relay wrote:
> > > > From: Arthur Becker <[email protected]>
> > > >
> > > > Device tree bindings for the vishay VEML6040 RGBW light sensor iio
> > > > driver
> > > >
> > > > Signed-off-by: Arthur Becker <[email protected]>
> > > > ---
> > > > V1 -> V3: Addressed review comments (v1 of the dt-bindings was sent
> > > > along with v2 of the driver but not in a set)
> > >
> > > It's basically the same as veml6075, so should be put there...
> > >
> > > Eh,
> > >
> > > Reviewed-by: Krzysztof Kozlowski <[email protected]>
> > >
> > > Best regards,
> > > Krzysztof
> > >
>
From: Jonathan Cameron <[email protected]>
Sent: 02 June 2024 15:22
To: Arthur Becker via B4 Relay
Cc: Arthur Becker; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley; [email protected]; [email protected]; [email protected]
Subject: [EXTERNAL]Re: [PATCH v3 1/2] iio: light: driver for Vishay VEML6040
> Hi Arthur,
>
> A few really trivial things inline. I'd have just tidied them up whilst
> applying, but I think you are doing a v4 anyway to merge the bindings
> so I'll leave the requested tweaks to you.
>
> Jonathan
Hi Jonathan,
Thanks for the feedback!
Small question about that comment:
> > +
> > + ret = devm_regulator_get_enable(dev, "vdd");
> > + if (ret)
> > + return ret;
> > +
> > + int init_config =
>
> Generally we are still sticking to traditional C rules so keep the
> local variable definition at the top of the file.
> The only common exception is when cleanup.h functionality is involved and
> we want to ensure ordering by moving the variable definitions into the code.
Do you mean at the start of the function, or rather a #define at the top of the
file after the masks and register definitions?
The use of 'FIELD_PREP' discards the 'static const int' option to initialise the
variable at the top of the file.
Kind regards,
Arthur
>
> > + FIELD_PREP(VEML6040_CONF_IT_MSK, VEML6040_CONF_IT_40_MS) |
> > + FIELD_PREP(VEML6040_CONF_AF_MSK, 0) |
> > + FIELD_PREP(VEML6040_CONF_SD_MSK, 0);
> > +
> > + ret = regmap_write(regmap, VEML6040_CONF_REG, init_config);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Could not set initial config\n");
> > +
On Mon, 3 Jun 2024 12:18:58 +0000
Arthur Becker <[email protected]> wrote:
> From: Jonathan Cameron <[email protected]>
> Sent: 02 June 2024 15:22
> To: Arthur Becker via B4 Relay
> Cc: Arthur Becker; Lars-Peter Clausen; Rob Herring; Krzysztof Kozlowski; Conor Dooley; [email protected]; [email protected]; [email protected]
> Subject: [EXTERNAL]Re: [PATCH v3 1/2] iio: light: driver for Vishay VEML6040
>
> > Hi Arthur,
> >
> > A few really trivial things inline. I'd have just tidied them up whilst
> > applying, but I think you are doing a v4 anyway to merge the bindings
> > so I'll leave the requested tweaks to you.
> >
> > Jonathan
>
> Hi Jonathan,
>
> Thanks for the feedback!
> Small question about that comment:
>
> > > +
> > > + ret = devm_regulator_get_enable(dev, "vdd");
> > > + if (ret)
> > > + return ret;
> > > +
> > > + int init_config =
> >
> > Generally we are still sticking to traditional C rules so keep the
> > local variable definition at the top of the file.
> > The only common exception is when cleanup.h functionality is involved and
> > we want to ensure ordering by moving the variable definitions into the code.
>
> Do you mean at the start of the function, or rather a #define at the top of the
> file after the masks and register definitions?
> The use of 'FIELD_PREP' discards the 'static const int' option to initialise the
> variable at the top of the file.
start of the function for the variable.
e.g.
int init_config;
stuff..
init_config = FIELD_PREP() where you have it
Thanks,
Jonathan
>
> Kind regards,
> Arthur
>
> >
> > > + FIELD_PREP(VEML6040_CONF_IT_MSK, VEML6040_CONF_IT_40_MS) |
> > > + FIELD_PREP(VEML6040_CONF_AF_MSK, 0) |
> > > + FIELD_PREP(VEML6040_CONF_SD_MSK, 0);
> > > +
> > > + ret = regmap_write(regmap, VEML6040_CONF_REG, init_config);
> > > + if (ret)
> > > + return dev_err_probe(dev, ret,
> > > + "Could not set initial config\n");
> > > +
>
>