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

[Marek Vasut <marex@denx.de>]

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@denx.de>
>> ---
>> Cc: Alexander Stein <alexander.stein@ew.tq-group.com>
>> Cc: Andre Werner <andre.werner@systec-electronic.com>
>> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Cc: Bjorn Helgaas <bhelgaas@google.com>
>> Cc: Conor Dooley <conor+dt@kernel.org>
>> Cc: Fabio Estevam <festevam@denx.de>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Jonathan Cameron <jic23@kernel.org>
>> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Cc: Luca Ceresoli <luca.ceresoli@bootlin.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: Matti Vaittinen <mazziesaccount@gmail.com>
>> Cc: Naresh Solanki <naresh.solanki@9elements.com>
>> Cc: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Cc: Stefan Windfeldt-Prytz <stefan.windfeldt-prytz@axis.com>
>> Cc: Vincent Tremblay <vincent@vtremblay.dev>
>> Cc: devicetree@vger.kernel.org
>> Cc: linux-iio@vger.kernel.org
>> ---
>> 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@denx.de>
>> + */
>> +
>> +#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@gmail.com>

Thanks !