Re: [PATCH v4 2/2] iio: light: isl76682: Add ISL76682 driver

Marek Vasut <marex@xxxxxxx> · Thu, 23 Nov 2023 01:26:19 +0100

On 11/22/23 13:17, Matti Vaittinen wrote:
On 11/21/23 05:10, Marek Vasut wrote:
The ISL76682 is very basic ALS which only supports ALS or IR mode
in four ranges, 1k/4k/16k/64k LUX. There is no IRQ support or any
other fancy functionality.

Signed-off-by: Marek Vasut <marex@xxxxxxx>
---
Cc: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
Cc: Andre Werner <andre.werner@xxxxxxxxxxxxxxxxxxxxx>
Cc: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Cc: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
Cc: Conor Dooley <conor+dt@xxxxxxxxxx>
Cc: Fabio Estevam <festevam@xxxxxxx>
Cc: Guenter Roeck <linux@xxxxxxxxxxxx>
Cc: Jonathan Cameron <jic23@xxxxxxxxxx>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>
Cc: Lars-Peter Clausen <lars@xxxxxxxxxx>
Cc: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
Cc: Mark Brown <broonie@xxxxxxxxxx>
Cc: Matti Vaittinen <mazziesaccount@xxxxxxxxx>
Cc: Naresh Solanki <naresh.solanki@xxxxxxxxxxxxx>
Cc: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>
Cc: Rob Herring <robh+dt@xxxxxxxxxx>
Cc: Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@xxxxxxxx>
Cc: Vincent Tremblay <vincent@xxxxxxxxxxxxx>
Cc: devicetree@xxxxxxxxxxxxxxx
Cc: linux-iio@xxxxxxxxxxxxxxx
---
V2: - Overhaul the driver
     - Cache the entire 8-bit command register instead of parts of it
       and build up and rewrite the entire register if necessary
     - Fix illumination scale, add intensity scale, add integration time
V3: - Limit the read data to 16bit ADC range
     - Update Kconfig description
     - Update macros, drop bitshifts
     - Switch over to table lookup for lux ranges
     - Switch over to .read_avail instead of attributes
     - Use guard where applicable
     - Drop remove function in favor of reset action
V4: - Address feedback from Andy
     - Add missing includes
     - Change ISL76682_ADC_MAX to BIT(16) - 1
     - Drop initial ret assignment in isl76682_read_raw()
     - Move return -EINVAL to default: switch-case branch
     - Use switch-case consistenly instead of if/else
     - Drop trailing commas
     - Add comment to isl76682_clear_configure_reg on command zeroing 
on failure
     - Drop i2c_set_clientdata
     - Update devm_regmap_init_i2c return value handling
---
  drivers/iio/light/Kconfig    |  15 ++
  drivers/iio/light/Makefile   |   1 +
  drivers/iio/light/isl76682.c | 364 +++++++++++++++++++++++++++++++++++
  3 files changed, 380 insertions(+)
  create mode 100644 drivers/iio/light/isl76682.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 45edba797e4c7..9e8cdc091556d 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -252,6 +252,21 @@ config ISL29125
        To compile this driver as a module, choose M here: the module 
will be
        called isl29125.
+config ISL76682
+    tristate "Intersil ISL76682 Light Sensor"
+    depends on I2C
+    select REGMAP_I2C
+    help
+      Say Y here if you want to build a driver for the Intersil ISL76682
+      Ambient Light Sensor and IR Intensity sensor. This driver provides
+      the readouts via standard IIO sysfs and device interface. Both ALS
+      illuminance and IR illuminance are provided raw with separate 
scale
+      setting which can be configured via sysfs, the default scale is 
1000
+      lux, other options are 4000/16000/64000 lux.
+
+      To compile this driver as a module, choose M here: the module 
will be
+      called isl76682.
+
  config HID_SENSOR_ALS
      depends on HID_SENSOR_HUB
      select IIO_BUFFER
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c0db4c4c36ec9..09fa585f3109f 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_IQS621_ALS)    += iqs621-als.o
  obj-$(CONFIG_SENSORS_ISL29018)    += isl29018.o
  obj-$(CONFIG_SENSORS_ISL29028)    += isl29028.o
  obj-$(CONFIG_ISL29125)        += isl29125.o
+obj-$(CONFIG_ISL76682)        += isl76682.o
  obj-$(CONFIG_JSA1212)        += jsa1212.o
  obj-$(CONFIG_SENSORS_LM3533)    += lm3533-als.o
  obj-$(CONFIG_LTR501)        += ltr501.o
diff --git a/drivers/iio/light/isl76682.c b/drivers/iio/light/isl76682.c
new file mode 100644
index 0000000000000..7f0ccd0d37539
--- /dev/null
+++ b/drivers/iio/light/isl76682.c
@@ -0,0 +1,364 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * IIO driver for the light sensor ISL76682.
+ * ISL76682 is Ambient Light Sensor
+ *
+ * Copyright (c) 2023 Marek Vasut <marex@xxxxxxx>
+ */
+
+#include <linux/array_size.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+
+#include <linux/iio/iio.h>
+
+#define ISL76682_REG_COMMAND            0x00
+
+#define ISL76682_COMMAND_EN            BIT(7)
+#define ISL76682_COMMAND_MODE_CONTINUOUS    BIT(6)
+#define ISL76682_COMMAND_LIGHT_IR        BIT(5)
+
+#define ISL76682_COMMAND_RANGE_LUX_1K        0x0
+#define ISL76682_COMMAND_RANGE_LUX_4K        0x1
+#define ISL76682_COMMAND_RANGE_LUX_16K        0x2
+#define ISL76682_COMMAND_RANGE_LUX_64K        0x3
+#define ISL76682_COMMAND_RANGE_LUX_MASK        GENMASK(1, 0)
+
+#define ISL76682_REG_ALSIR_L            0x01
+
+#define ISL76682_REG_ALSIR_U            0x02
+
+#define ISL76682_NUM_REGS            (ISL76682_REG_ALSIR_U + 1)
+
+#define ISL76682_CONV_TIME_MS            100
+#define ISL76682_INT_TIME_US            90000
+
+#define ISL76682_ADC_MAX            (BIT(16) - 1)
+
+struct isl76682_chip {
+    /*
+     * Lock to synchronize access to device command register
+     * and the content of range variable below.
+     */
+    struct mutex            lock;
+    struct regmap            *regmap;
+    u8                range;
+    u8                command;
+};
+
+struct isl76682_range {
+    u8                range;
+    u32                als;
+    u32                ir;
+};
+
+static struct isl76682_range isl76682_range_table[] = {
+    { ISL76682_COMMAND_RANGE_LUX_1K, 15000, 10500 },
+    { ISL76682_COMMAND_RANGE_LUX_4K, 60000, 42000 },
+    { ISL76682_COMMAND_RANGE_LUX_16K, 240000, 168000 },
+    { ISL76682_COMMAND_RANGE_LUX_64K, 960000, 673000 }
+};
+
+static int isl76682_get(struct isl76682_chip *chip, bool mode_ir, int 
*data)
+{
+    u8 command;
+    int ret;
+
+    command = ISL76682_COMMAND_EN | ISL76682_COMMAND_MODE_CONTINUOUS |
+          chip->range;
+
+    if (mode_ir)
+        command |= ISL76682_COMMAND_LIGHT_IR;
+
+    if (command != chip->command) {
+        ret = regmap_write(chip->regmap, ISL76682_REG_COMMAND, command);
+        if (ret)
+            return ret;
+
+        /* Need to wait for conversion time if ALS/IR mode enabled */
+        msleep(ISL76682_CONV_TIME_MS);
+
+        chip->command = command;
+    }
+
+    ret = regmap_bulk_read(chip->regmap, ISL76682_REG_ALSIR_L, data, 2);
+    *data &= ISL76682_ADC_MAX;
+    return ret;
+}
+
+static int isl76682_write_raw(struct iio_dev *indio_dev,
+                  struct iio_chan_spec const *chan,
+                  int val, int val2, long mask)
+{
+    struct isl76682_chip *chip = iio_priv(indio_dev);
+    int i;
+
+    if (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY)
+        return -EINVAL;
+
+    if (mask != IIO_CHAN_INFO_SCALE)
+        return -EINVAL;
+
+    if (val != 0)
+        return -EINVAL;
+
+    for (i = 0; i < ARRAY_SIZE(isl76682_range_table); i++) {
+        if (chan->type == IIO_LIGHT) {
+            if (val2 != isl76682_range_table[i].als)
+                continue;
+        } else if (chan->type == IIO_INTENSITY) {
+            if (val2 != isl76682_range_table[i].ir)
+                continue;
+        }

I like this table-based look-up for write (and read) of scales. Looking 
at this I see an analogy to some of the regulator stuff, like for 
example the ramp-up values. What I do very much like in the regulator 
subsystem is the drivers/regulator/helpers.c

I wonder if similar approach would be usable in IIO as well? I mean, 
providing readily written iio_regmap_read/write_raw_<functionality>() 
and iio_available_*() helpers for the simple devices where we just have 
value-register mapping? I mean, driver would just populate something like:

struct iio_scale_desc {
     int *scale_val_table;
     int *scale_val2_table;
     int num_scales;

You'd also need type here (fractional, int+micro, ...), right ?

     int scale_reg_addr;
     int scale_reg_mask;
};

and call helper like
int iio_regmap_read_raw_scale(struct iio_dev *idev,
                   struct iio_scale_desc *sd, int *val,
                   int *val2)"
provided by IIO framework.

Similar helper for writing new scales and getting available scales.

Later this could be expanded by allowing specifying the type of provided 
values (in the example case, IIO_VAL_INT_PLUS_x - but maybe this would 
be extensible (and worth) to support also the other options?)

I know it's a bit much to be done in the context of this series. Hence I 
am definitely not insisting this to be done here! OTOH, the embedded 
Linux is not in EU next year so maybe Marek would forgive me before we 
meet next time :pondering:

toffee-- forgive++ , hehehe , no worries.

Anyways - does this sound like a sensible thing to do? I guess it could 
help simplifying some drivers a little.

The only thing I would wonder about is, should such a thing go into 
regmap so it can be reused cross-subsystem instead of making this iio 
specific ?

Oh. Only after writing of this I noticed the range is written in HW only 
together with the 'start' command. I guess this is how the IC operates - 

The IC is just simple, a few bits in command register to control it and 
nothing fancy, so it is just easier to write the 8 bits at a time 
instead of doing RMW .

you need to write all configs together with starting the measurement? Or 
is that just an optimization to avoid extra writes? If it's the first, 
then a suggested iio_regmap_*() -helper wouldn't work here. I might've 
added a comment explaining why range is written in isl76682_get() and 
not here to the isl76682_get().

It is just easier and cheaper to write it all at once instead of RMW .
It also isn't like RMW would win anything here, rather the opposite.

Anyways - this driver looks good to me. (What a long way of saying that).

Reviewed-by: Matti Vaittinen <mazziesaccount@xxxxxxxxx>

Thanks !







