LinuxLists.cc - [PATCH v2 2/2] iio: light: driver for Lite-On ltr390

2023-11-17 07:48:10

Subject: [PATCH v2 2/2] iio: light: driver for Lite-On ltr390

Implements driver for the Ambient/UV Light sensor LTR390.
The driver exposes two ways of getting sensor readings:
1. Raw UV Counts directly from the sensor
2. The computed UV Index value with a percision of 2 decimal places

NOTE: Ambient light sensing has not been implemented yet.

Datasheet:
https://optoelectronics.liteon.com/upload/download/DS86-2015-0004/LTR-390UV_Final_%20DS_V1%201.pdf

Driver tested on RPi Zero 2W

Signed-off-by: Anshul Dalal <[email protected]>
---

Changes for v2:
- Fixed typo in `LTR390_FRACTIONAL_PRECISION`
- Added of_device_id
---
MAINTAINERS | 7 ++
drivers/iio/light/Kconfig | 11 ++
drivers/iio/light/Makefile | 1 +
drivers/iio/light/ltr390.c | 232 +++++++++++++++++++++++++++++++++++++
4 files changed, 251 insertions(+)
create mode 100644 drivers/iio/light/ltr390.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 81d5fc0bba68..c9f2238673f0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12484,6 +12484,13 @@ S: Maintained
W: http://linux-test-project.github.io/
T: git https://github.com/linux-test-project/ltp.git

+LTR390 AMBIENT/UV LIGHT SENSOR DRIVER
+M: Anshul Dalal <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/iio/light/liteon,ltr390.yaml
+F: drivers/iio/light/ltr390.c
+
LYNX 28G SERDES PHY DRIVER
M: Ioana Ciornei <[email protected]>
L: [email protected]
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 45edba797e4c..61993ae79afe 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -347,6 +347,17 @@ config SENSORS_LM3533
changes. The ALS-control output values can be set per zone for the
three current output channels.

+config LTR390
+ tristate "LTR-390UV-01 ambient light and UV sensor"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ If you say yes here you get support for the Lite-On LTR-390UV-01
+ ambient light and UV sensor.
+
+ This driver can also be built as a module. If so, the module
+ will be called ltr390.
+
config LTR501
tristate "LTR-501ALS-01 light sensor"
depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c0db4c4c36ec..550f8b408bc2 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -30,6 +30,7 @@ obj-$(CONFIG_SENSORS_ISL29028) += isl29028.o
obj-$(CONFIG_ISL29125) += isl29125.o
obj-$(CONFIG_JSA1212) += jsa1212.o
obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
+obj-$(CONFIG_LTR390) += ltr390.o
obj-$(CONFIG_LTR501) += ltr501.o
obj-$(CONFIG_LTRF216A) += ltrf216a.o
obj-$(CONFIG_LV0104CS) += lv0104cs.o
diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
new file mode 100644
index 000000000000..67ca028ce828
--- /dev/null
+++ b/drivers/iio/light/ltr390.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * IIO driver for Lite-On LTR390 ALS and UV sensor
+ * (7-bit I2C slave address 0x53)
+ *
+ * Based on the work of:
+ * Shreeya Patel and Shi Zhigang (LTRF216 Driver)
+ *
+ * Copyright (C) 2023 Anshul Dalal <[email protected]>
+ *
+ * Datasheet:
+ * https://optoelectronics.liteon.com/upload/download/DS86-2015-0004/LTR-390UV_Final_%20DS_V1%201.pdf
+ *
+ * TODO:
+ * - Support for configurable gain and resolution
+ * - Sensor suspend/resume support
+ * - Add support for reading the ALS
+ * - Interrupt support
+ */
+
+#include <asm/unaligned.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/math.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+
+#define LTR390_DEVICE_NAME "ltr390"
+
+#define LTR390_MAIN_CTRL 0x00
+#define LTR390_PART_ID 0x06
+#define LTR390_UVS_DATA 0x10
+
+#define LTR390_SW_RESET BIT(4)
+#define LTR390_UVS_MODE BIT(3)
+#define LTR390_SENSOR_ENABLE BIT(1)
+
+#define LTR390_PART_NUMBER_ID 0xb
+#define LTR390_FRACTIONAL_PERCISION 100
+
+/*
+ * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
+ * the sensor are equal to 1 UV Index [Datasheet Page#8].
+ *
+ * For the default resolution of 18-bit (integration time: 100ms) and default
+ * gain of 3x, the counts/uvi are calculated as follows:
+ * 2300 / ((3/18) * (100/400)) = 95.83
+ */
+#define LTR390_COUNTS_PER_UVI 96
+
+/*
+ * Window Factor is needed when the device is under Window glass with coated
+ * tinted ink. This is to compensate for the light loss due to the lower
+ * transmission rate of the window glass and helps * in calculating lux.
+ */
+#define LTR390_WINDOW_FACTOR 1
+
+struct ltr390_data {
+ struct regmap *regmap;
+ struct i2c_client *client;
+ struct mutex lock;
+};
+
+static const struct regmap_config ltr390_regmap_config = {
+ .name = LTR390_DEVICE_NAME,
+ .reg_bits = 8,
+ .reg_stride = 1,
+ .val_bits = 8,
+};
+
+static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
+{
+ struct device *dev = &data->client->dev;
+ int ret;
+ u8 recieve_buffer[3];
+
+ mutex_lock(&data->lock);
+
+ ret = regmap_bulk_read(data->regmap, register_address, recieve_buffer,
+ sizeof(recieve_buffer));
+ if (ret) {
+ dev_err(dev, "failed to read measurement data: %d\n", ret);
+ mutex_unlock(&data->lock);
+ return ret;
+ }
+
+ mutex_unlock(&data->lock);
+ return get_unaligned_le24(recieve_buffer);
+}
+
+static int ltr390_get_uv_index(struct ltr390_data *data)
+{
+ int ret;
+ int uv_index;
+
+ ret = ltr390_register_read(data, LTR390_UVS_DATA);
+ if (ret < 0)
+ return ret;
+
+ uv_index = DIV_ROUND_CLOSEST(ret * LTR390_FRACTIONAL_PERCISION *
+ LTR390_WINDOW_FACTOR,
+ LTR390_COUNTS_PER_UVI);
+
+ return uv_index;
+}
+
+static int ltr390_read_raw(struct iio_dev *iio_device,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ int ret;
+ struct ltr390_data *data = iio_priv(iio_device);
+
+ switch (mask) {
+ case IIO_CHAN_INFO_PROCESSED:
+ ret = ltr390_get_uv_index(data);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ *val2 = LTR390_FRACTIONAL_PERCISION;
+ return IIO_VAL_FRACTIONAL;
+ case IIO_CHAN_INFO_RAW:
+ ret = ltr390_register_read(data, LTR390_UVS_DATA);
+ if (ret < 0)
+ return ret;
+ *val = ret;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info ltr390_info = {
+ .read_raw = ltr390_read_raw,
+};
+
+static const struct iio_chan_spec ltr390_channels[] = {
+ {
+ .type = IIO_UVINDEX,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED)
+ },
+ {
+ .type = IIO_INTENSITY,
+ .channel2 = IIO_MOD_LIGHT_UV,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
+ },
+};
+
+static int ltr390_probe(struct i2c_client *client)
+{
+ struct ltr390_data *data;
+ struct iio_dev *indio_dev;
+ int ret, part_number;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ data = iio_priv(indio_dev);
+
+ data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
+ if (IS_ERR(data->regmap))
+ return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
+ "regmap initialization failed\n");
+
+ data->client = client;
+ i2c_set_clientdata(client, indio_dev);
+ mutex_init(&data->lock);
+
+ indio_dev->info = &ltr390_info;
+ indio_dev->channels = ltr390_channels;
+ indio_dev->num_channels = ARRAY_SIZE(ltr390_channels);
+ indio_dev->name = LTR390_DEVICE_NAME;
+
+ ret = regmap_read(data->regmap, LTR390_PART_ID, &part_number);
+ if (ret) {
+ dev_err(&client->dev, "failed to get sensor's part id: %d",
+ ret);
+ return ret;
+ }
+ /* Lower 4 bits of `part_number` change with hardware revisions */
+ if (part_number >> 4 != LTR390_PART_NUMBER_ID) {
+ dev_err(&client->dev, "received invalid product id: 0x%x",
+ part_number);
+ return -ENODEV;
+ }
+ dev_dbg(&client->dev, "LTR390, product id: 0x%x\n", part_number);
+
+ /* reset sensor, chip fails to respond to this, so ignore any errors */
+ regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SW_RESET);
+
+ /* Wait for the registers to reset before proceeding */
+ usleep_range(1000, 2000);
+
+ ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
+ LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
+ if (ret) {
+ dev_err(&client->dev, "failed to enable the sensor: %d\n", ret);
+ return ret;
+ }
+
+ return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id ltr390_id[] = {
+ { LTR390_DEVICE_NAME, 0 },
+ { /* Sentinel */ },
+};
+MODULE_DEVICE_TABLE(i2c, ltr390_id);
+
+static const struct of_device_id ltr390_of_table[] = {
+ { .compatible = "liteon,ltr390"},
+ { /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ltr390_id_table);
+
+static struct i2c_driver ltr390_driver = {
+ .driver = {
+ .name = LTR390_DEVICE_NAME,
+ .of_match_table = ltr390_of_table,
+ },
+ .probe = ltr390_probe,
+ .id_table = ltr390_id,
+};
+
+module_i2c_driver(ltr390_driver);
+
+MODULE_AUTHOR("Anshul Dalal <[email protected]>");
+MODULE_DESCRIPTION("Lite-On LTR390 ALS and UV sensor Driver");
+MODULE_LICENSE("GPL");
--
2.42.1

2023-11-17 14:24:57

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: light: driver for Lite-On ltr390

2023-11-17 18:14:40

by kernel test robot

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: light: driver for Lite-On ltr390

Hi Anshul,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v6.7-rc1 next-20231117]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url: https://github.com/intel-lab-lkp/linux/commits/Anshul-Dalal/iio-light-driver-for-Lite-On-ltr390/20231117-154922
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20231117074554.700970-2-anshulusr%40gmail.com
patch subject: [PATCH v2 2/2] iio: light: driver for Lite-On ltr390
config: hexagon-allmodconfig (https://download.01.org/0day-ci/archive/20231118/[email protected]/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231118/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All errors (new ones prefixed by >>):

In file included from drivers/iio/light/ltr390.c:23:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/iio/light/ltr390.c:23:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/iio/light/ltr390.c:23:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:337:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> drivers/iio/light/ltr390.c:217:25: error: use of undeclared identifier 'ltr390_id_table'; did you mean 'ltr390_of_table'?
217 | MODULE_DEVICE_TABLE(of, ltr390_id_table);
| ^~~~~~~~~~~~~~~
| ltr390_of_table
include/linux/module.h:244:15: note: expanded from macro 'MODULE_DEVICE_TABLE'
244 | extern typeof(name) __mod_##type##__##name##_device_table \
| ^
drivers/iio/light/ltr390.c:213:34: note: 'ltr390_of_table' declared here
213 | static const struct of_device_id ltr390_of_table[] = {
| ^
6 warnings and 1 error generated.

vim +217 drivers/iio/light/ltr390.c

212
213 static const struct of_device_id ltr390_of_table[] = {
214 { .compatible = "liteon,ltr390"},
215 { /* Sentinel */ }
216 };
> 217 MODULE_DEVICE_TABLE(of, ltr390_id_table);
218

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

2023-11-25 14:07:05

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: light: driver for Lite-On ltr390

On Fri, 17 Nov 2023 13:15:53 +0530
Anshul Dalal <[email protected]> wrote:

> Implements driver for the Ambient/UV Light sensor LTR390.
> The driver exposes two ways of getting sensor readings:
> 1. Raw UV Counts directly from the sensor
> 2. The computed UV Index value with a percision of 2 decimal places
>
> NOTE: Ambient light sensing has not been implemented yet.
>
> Datasheet:
> https://optoelectronics.liteon.com/upload/download/DS86-2015-0004/LTR-390UV_Final_%20DS_V1%201.pdf
Make this a formal Datasheet tag, just before the Signed-off-by below

>
> Driver tested on RPi Zero 2W
>
> Signed-off-by: Anshul Dalal <[email protected]>

Hi Anshul,

Some comments on this one inline. Some of these overlap with comments on the
other drivers you've submitted. Normally I'd moan about sending too many drivers
at a time, but fair enough given you sent them out over a couple of weeks and just
happened to hit time when I was travelling.

My main question in here is why have the two channels - conversion looks linear
so you should be fine exposing IIO_CHAN_INFO_RAW + IIO_CHAN_INFO_SCALE on a
single channel and leaving userspace to do the maths.

Jonathan

> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
> new file mode 100644
> index 000000000000..67ca028ce828
> --- /dev/null
> +++ b/drivers/iio/light/ltr390.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * IIO driver for Lite-On LTR390 ALS and UV sensor
> + * (7-bit I2C slave address 0x53)
> + *
> + * Based on the work of:
> + * Shreeya Patel and Shi Zhigang (LTRF216 Driver)
> + *
> + * Copyright (C) 2023 Anshul Dalal <[email protected]>
> + *
> + * Datasheet:
> + * https://optoelectronics.liteon.com/upload/download/DS86-2015-0004/LTR-390UV_Final_%20DS_V1%201.pdf
> + *
> + * TODO:
> + * - Support for configurable gain and resolution

Not using PROCESSED will help with this, so I'd drop that even in this initial
driver.

> + * - Sensor suspend/resume support
> + * - Add support for reading the ALS
> + * - Interrupt support

> + */
> +
> +#include <asm/unaligned.h>

Put this in a block of includes after the linux ones.

> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +
> +#define LTR390_DEVICE_NAME "ltr390"
> +
> +#define LTR390_MAIN_CTRL 0x00
> +#define LTR390_PART_ID 0x06
> +#define LTR390_UVS_DATA 0x10
> +
> +#define LTR390_SW_RESET BIT(4)
> +#define LTR390_UVS_MODE BIT(3)
> +#define LTR390_SENSOR_ENABLE BIT(1)
> +
> +#define LTR390_PART_NUMBER_ID 0xb
> +#define LTR390_FRACTIONAL_PERCISION 100
> +
> +/*
> + * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
> + * the sensor are equal to 1 UV Index [Datasheet Page#8].
> + *
> + * For the default resolution of 18-bit (integration time: 100ms) and default
> + * gain of 3x, the counts/uvi are calculated as follows:
> + * 2300 / ((3/18) * (100/400)) = 95.83
> + */
> +#define LTR390_COUNTS_PER_UVI 96
> +
> +/*
> + * Window Factor is needed when the device is under Window glass with coated
> + * tinted ink. This is to compensate for the light loss due to the lower
> + * transmission rate of the window glass and helps * in calculating lux.
> + */
> +#define LTR390_WINDOW_FACTOR 1
> +
> +struct ltr390_data {
> + struct regmap *regmap;
> + struct i2c_client *client;
> + struct mutex lock;

All locks need a comment explaining the scope of data they protect.
Note that regmap and the i2c bus will have their own locks by default
so I'm not sure you need one here at all as I'm not seeing read modify write
cycles or anything like that (I might be missing one though!)

> +};
> +
> +static const struct regmap_config ltr390_regmap_config = {
> + .name = LTR390_DEVICE_NAME,
> + .reg_bits = 8,
> + .reg_stride = 1,
> + .val_bits = 8,
> +};
> +
> +static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
> +{
> + struct device *dev = &data->client->dev;
> + int ret;
> + u8 recieve_buffer[3];
> +
> + mutex_lock(&data->lock);

guard(mutex)(&data->lock);
here then you can just return directly without worrying about manual
unlocking of the mutex. This stuff is fairly new in the kernel but very
useful for cases like this! I have a set of driver cleanup that I'll send
out that does this in a few IIO drivers, once I've caught up with reviews etc.

This isn't a critical path where we have to care that we will then unlock
after the maths to extract value in the final line of the function.

> +
> + ret = regmap_bulk_read(data->regmap, register_address, recieve_buffer,
> + sizeof(recieve_buffer));
> + if (ret) {
> + dev_err(dev, "failed to read measurement data: %d\n", ret);
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + mutex_unlock(&data->lock);
> + return get_unaligned_le24(recieve_buffer);
> +}
> +
> +static int ltr390_get_uv_index(struct ltr390_data *data)
> +{
> + int ret;
> + int uv_index;
> +
> + ret = ltr390_register_read(data, LTR390_UVS_DATA);
> + if (ret < 0)
> + return ret;
> +
> + uv_index = DIV_ROUND_CLOSEST(ret * LTR390_FRACTIONAL_PERCISION *
> + LTR390_WINDOW_FACTOR,
> + LTR390_COUNTS_PER_UVI);

This looks like a linear conversion. Perhaps make it a userspace problem
as division much easier in userspace where floating point is available.

> +
> + return uv_index;
> +}
> +

> +
> +static const struct iio_chan_spec ltr390_channels[] = {
> + {
> + .type = IIO_UVINDEX,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED)
> + },
> + {
> + .type = IIO_INTENSITY,
> + .channel2 = IIO_MOD_LIGHT_UV,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
> + },
> +};
> +
> +static int ltr390_probe(struct i2c_client *client)
> +{
> + struct ltr390_data *data;
> + struct iio_dev *indio_dev;
> + int ret, part_number;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> +
> + data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
> + if (IS_ERR(data->regmap))
> + return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
There are quite a few &client->dev in here. I'd introduce
struct device *dev = &client->dev;
as a local variable then use that to shorten all those lines a little.

> + "regmap initialization failed\n");
> +
> + data->client = client;
> + i2c_set_clientdata(client, indio_dev);

Why set this? I don' think you are using it.

> + mutex_init(&data->lock);
> +
> + indio_dev->info = &ltr390_info;
> + indio_dev->channels = ltr390_channels;
> + indio_dev->num_channels = ARRAY_SIZE(ltr390_channels);
> + indio_dev->name = LTR390_DEVICE_NAME;

I prefer to see strings for names like this rather than making a reviewer
find the define to make sure it's of appropriate format etc. Just duplicate
the string in the few places it's used.

> +
> + ret = regmap_read(data->regmap, LTR390_PART_ID, &part_number);
> + if (ret) {
> + dev_err(&client->dev, "failed to get sensor's part id: %d",
> + ret);
dev_err_probe() for all error prints in probe(). Some of the functionality
won't matter in cases like this, but its cleaner anyway so use it everywhere
+ then a reviewer doesn't need to consider what might defer.

> + return ret;
> + }
> + /* Lower 4 bits of `part_number` change with hardware revisions */
> + if (part_number >> 4 != LTR390_PART_NUMBER_ID) {
> + dev_err(&client->dev, "received invalid product id: 0x%x",
> + part_number);
return dev_err_probe()

Also, to enable device tree fallback compatibles (used when a new part is
released that is completely compatible with a driver for an older one)
we don't want to error out here. Fine to print a dev_info() message though
to indicate we don't know if the driver is compatible or not.
Please check your other drivers for this (I may have missed it!)

> + return -ENODEV;
> + }
> + dev_dbg(&client->dev, "LTR390, product id: 0x%x\n", part_number);
> +
> + /* reset sensor, chip fails to respond to this, so ignore any errors */
> + regmap_set_bits(data->regmap, LTR390_MAIN_CTRL, LTR390_SW_RESET);
> +
> + /* Wait for the registers to reset before proceeding */
> + usleep_range(1000, 2000);
> +
> + ret = regmap_set_bits(data->regmap, LTR390_MAIN_CTRL,
> + LTR390_SENSOR_ENABLE | LTR390_UVS_MODE);
> + if (ret) {
> + dev_err(&client->dev, "failed to enable the sensor: %d\n", ret);
return dev_err_probe()
> + return ret;
> + }
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id ltr390_id[] = {
> + { LTR390_DEVICE_NAME, 0 },

That 0 adds nothing useful so drop it.

> + { /* Sentinel */ },
No comma after sentinal.
> +};
> +MODULE_DEVICE_TABLE(i2c, ltr390_id);
> +
> +static const struct of_device_id ltr390_of_table[] = {
> + { .compatible = "liteon,ltr390"},
uneven spacing. Should be one before }
> + { /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ltr390_id_table);

Always build test as a module. 0-day pointed this out - name here is
wrong.

> +
> +static struct i2c_driver ltr390_driver = {
> + .driver = {
> + .name = LTR390_DEVICE_NAME,
> + .of_match_table = ltr390_of_table,
> + },
> + .probe = ltr390_probe,
> + .id_table = ltr390_id,
> +};
> +
trivial, but we often don't have a blank line here as the
structure only exists to pass to module_i2c_driver() macro so
we might as well group them together.
> +module_i2c_driver(ltr390_driver);
> +
> +MODULE_AUTHOR("Anshul Dalal <[email protected]>");
> +MODULE_DESCRIPTION("Lite-On LTR390 ALS and UV sensor Driver");
> +MODULE_LICENSE("GPL");

2023-11-27 15:49:37

by Anshul Dalal

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: light: driver for Lite-On ltr390

On 11/25/23 19:36, Jonathan Cameron wrote:
> On Fri, 17 Nov 2023 13:15:53 +0530
> Anshul Dalal <[email protected]> wrote:
>
>> Implements driver for the Ambient/UV Light sensor LTR390.
>> The driver exposes two ways of getting sensor readings:
>> 1. Raw UV Counts directly from the sensor
>> 2. The computed UV Index value with a percision of 2 decimal places
>>
>> NOTE: Ambient light sensing has not been implemented yet.
>>
>> Datasheet:
>> https://optoelectronics.liteon.com/upload/download/DS86-2015-0004/LTR-390UV_Final_%20DS_V1%201.pdf
> Make this a formal Datasheet tag, just before the Signed-off-by below

Thanks for pointing it out, I was unaware of the standard formal tags.
Is there any resource that lists out the formal tags that I can refer to.

>>
>> Driver tested on RPi Zero 2W
>>
>> Signed-off-by: Anshul Dalal <[email protected]>
>
> Hi Anshul,
>
> Some comments on this one inline. Some of these overlap with comments on the
> other drivers you've submitted. Normally I'd moan about sending too many drivers
> at a time, but fair enough given you sent them out over a couple of weeks and just
> happened to hit time when I was travelling.
>
> My main question in here is why have the two channels - conversion looks linear
> so you should be fine exposing IIO_CHAN_INFO_RAW + IIO_CHAN_INFO_SCALE on a
> single channel and leaving userspace to do the maths.
>
> Jonathan

This driver was my first time working with the IIO subsystem and wasn't
sure about the correct way to use the channels. This should be fixed in
the next revision with the UV index being reported in the following channel:

static const struct iio_chan_spec ltr390_channel = {
.type = IIO_UVINDEX,
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)
};

With data being reported as:

case IIO_CHAN_INFO_RAW:
ret = ltr390_register_read(data, LTR390_UVS_DATA);
if (ret < 0)
return ret;
*val = ret;
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
*val = LTR390_WINDOW_FACTOR;
*val2 = LTR390_COUNTS_PER_UVI;
return IIO_VAL_FRACTIONAL;

>> diff --git a/drivers/iio/light/ltr390.c b/drivers/iio/light/ltr390.c
>> new file mode 100644
>> index 000000000000..67ca028ce828
>> --- /dev/null
>> +++ b/drivers/iio/light/ltr390.c
>> @@ -0,0 +1,232 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * IIO driver for Lite-On LTR390 ALS and UV sensor
>> + * (7-bit I2C slave address 0x53)
>> + *
>> + * Based on the work of:
>> + * Shreeya Patel and Shi Zhigang (LTRF216 Driver)
>> + *
>> + * Copyright (C) 2023 Anshul Dalal <[email protected]>
>> + *
>> + * Datasheet:
>> + * https://optoelectronics.liteon.com/upload/download/DS86-2015-0004/LTR-390UV_Final_%20DS_V1%201.pdf
>> + *
>> + * TODO:
>> + * - Support for configurable gain and resolution
>
> Not using PROCESSED will help with this, so I'd drop that even in this initial
> driver.
>>> + * - Sensor suspend/resume support
>> + * - Add support for reading the ALS
>> + * - Interrupt support
>
>> + */
>> +
>> +#include <asm/unaligned.h>
>
> Put this in a block of includes after the linux ones.
>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/math.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/regmap.h>
>> +
>> +#define LTR390_DEVICE_NAME "ltr390"
>> +
>> +#define LTR390_MAIN_CTRL 0x00
>> +#define LTR390_PART_ID 0x06
>> +#define LTR390_UVS_DATA 0x10
>> +
>> +#define LTR390_SW_RESET BIT(4)
>> +#define LTR390_UVS_MODE BIT(3)
>> +#define LTR390_SENSOR_ENABLE BIT(1)
>> +
>> +#define LTR390_PART_NUMBER_ID 0xb
>> +#define LTR390_FRACTIONAL_PERCISION 100
>> +
>> +/*
>> + * At 20-bit resolution (integration time: 400ms) and 18x gain, 2300 counts of
>> + * the sensor are equal to 1 UV Index [Datasheet Page#8].
>> + *
>> + * For the default resolution of 18-bit (integration time: 100ms) and default
>> + * gain of 3x, the counts/uvi are calculated as follows:
>> + * 2300 / ((3/18) * (100/400)) = 95.83
>> + */
>> +#define LTR390_COUNTS_PER_UVI 96
>> +
>> +/*
>> + * Window Factor is needed when the device is under Window glass with coated
>> + * tinted ink. This is to compensate for the light loss due to the lower
>> + * transmission rate of the window glass and helps * in calculating lux.
>> + */
>> +#define LTR390_WINDOW_FACTOR 1
>> +
>> +struct ltr390_data {
>> + struct regmap *regmap;
>> + struct i2c_client *client;
>> + struct mutex lock;
>
> All locks need a comment explaining the scope of data they protect.
> Note that regmap and the i2c bus will have their own locks by default
> so I'm not sure you need one here at all as I'm not seeing read modify write
> cycles or anything like that (I might be missing one though!)

My goal with the mutex was to protect the sysfs though that might be
unnecessary.

>> +};
>> +
>> +static const struct regmap_config ltr390_regmap_config = {
>> + .name = LTR390_DEVICE_NAME,
>> + .reg_bits = 8,
>> + .reg_stride = 1,
>> + .val_bits = 8,
>> +};
>> +
>> +static int ltr390_register_read(struct ltr390_data *data, u8 register_address)
>> +{
>> + struct device *dev = &data->client->dev;
>> + int ret;
>> + u8 recieve_buffer[3];
>> +
>> + mutex_lock(&data->lock);
>
> guard(mutex)(&data->lock);
> here then you can just return directly without worrying about manual
> unlocking of the mutex. This stuff is fairly new in the kernel but very
> useful for cases like this! I have a set of driver cleanup that I'll send
> out that does this in a few IIO drivers, once I've caught up with reviews etc.
>
> This isn't a critical path where we have to care that we will then unlock
> after the maths to extract value in the final line of the function.
>
>> +
>> + ret = regmap_bulk_read(data->regmap, register_address, recieve_buffer,
>> + sizeof(recieve_buffer));
>> + if (ret) {
>> + dev_err(dev, "failed to read measurement data: %d\n", ret);
>> + mutex_unlock(&data->lock);
>> + return ret;
>> + }
>> +
>> + mutex_unlock(&data->lock);
>> + return get_unaligned_le24(recieve_buffer);
>> +}
>> +
>> +static int ltr390_get_uv_index(struct ltr390_data *data)
>> +{
>> + int ret;
>> + int uv_index;
>> +
>> + ret = ltr390_register_read(data, LTR390_UVS_DATA);
>> + if (ret < 0)
>> + return ret;
>> +
>> + uv_index = DIV_ROUND_CLOSEST(ret * LTR390_FRACTIONAL_PERCISION *
>> + LTR390_WINDOW_FACTOR,
>> + LTR390_COUNTS_PER_UVI);
>
> This looks like a linear conversion. Perhaps make it a userspace problem
> as division much easier in userspace where floating point is available.
>
>> +
>> + return uv_index;
>> +}
>> +
>
>> +
>> +static const struct iio_chan_spec ltr390_channels[] = {
>> + {
>> + .type = IIO_UVINDEX,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED)
>> + },
>> + {
>> + .type = IIO_INTENSITY,
>> + .channel2 = IIO_MOD_LIGHT_UV,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW)
>> + },
>> +};
>> +
>> +static int ltr390_probe(struct i2c_client *client)
>> +{
>> + struct ltr390_data *data;
>> + struct iio_dev *indio_dev;
>> + int ret, part_number;
>> +
>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + data = iio_priv(indio_dev);
>> +
>> + data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
>> + if (IS_ERR(data->regmap))
>> + return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> There are quite a few &client->dev in here. I'd introduce
> struct device *dev = &client->dev;
> as a local variable then use that to shorten all those lines a little.
>
>> + "regmap initialization failed\n");
>> +
>> + data->client = client;
>> + i2c_set_clientdata(client, indio_dev);
>
> Why set this? I don' think you are using it.
>

It seems to be necessary for regmap to work properly, I tested without
it and I get an EREMOTEIO(121) when reading the part id.

>> [..]

Thanks for the review,
Best regards,
Anshul

2023-12-04 10:05:50

by Jonathan Cameron

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] iio: light: driver for Lite-On ltr390

> >> +struct ltr390_data {
> >> + struct regmap *regmap;
> >> + struct i2c_client *client;
> >> + struct mutex lock;
> >
> > All locks need a comment explaining the scope of data they protect.
> > Note that regmap and the i2c bus will have their own locks by default
> > so I'm not sure you need one here at all as I'm not seeing read modify write
> > cycles or anything like that (I might be missing one though!)
>
> My goal with the mutex was to protect the sysfs though that might be
> unnecessary.

Ok. So, there is nothing stopping multiple parallel sysfs accesses, but
what you'll actually be protecting is either device or driver state, not
sysfs as such.

>
> >> +};
> >> +
> >> +static const struct regmap_config ltr390_regmap_config = {
> >> + .name = LTR390_DEVICE_NAME,
> >> + .reg_bits = 8,
> >> + .reg_stride = 1,
> >> + .val_bits = 8,
> >> +};
> >> +

> >> +static int ltr390_probe(struct i2c_client *client)
> >> +{
> >> + struct ltr390_data *data;
> >> + struct iio_dev *indio_dev;
> >> + int ret, part_number;
> >> +
> >> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> >> + if (!indio_dev)
> >> + return -ENOMEM;
> >> +
> >> + data = iio_priv(indio_dev);
> >> +
> >> + data->regmap = devm_regmap_init_i2c(client, &ltr390_regmap_config);
> >> + if (IS_ERR(data->regmap))
> >> + return dev_err_probe(&client->dev, PTR_ERR(data->regmap),
> > There are quite a few &client->dev in here. I'd introduce
> > struct device *dev = &client->dev;
> > as a local variable then use that to shorten all those lines a little.
> >
> >> + "regmap initialization failed\n");
> >> +
> >> + data->client = client;
> >> + i2c_set_clientdata(client, indio_dev);
> >
> > Why set this? I don' think you are using it.
> >
>
> It seems to be necessary for regmap to work properly, I tested without
> it and I get an EREMOTEIO(121) when reading the part id.

That's weird given regmap will have no understanding of an iio_dev.

If you can do some more debugging on where that error is coming from
in regmap that would be great.

I suspect it's coming from down in the bus master which should not
be touching this at all. What is the i2c master in this case?

Jonathan

>
> >> [..]
>
> Thanks for the review,
> Best regards,
> Anshul