| Message ID | 20231121031043.327614-2-marex@denx.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v4,1/2] dt-bindings: iio: light: isl76682: Document ISL76682 | expand |
On Tue, Nov 21, 2023 at 04:10:40AM +0100, 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. ... > + 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; > + } Redundant 'else' and you can combine if:s. if (chan->type == IIO_LIGHT && val2 != isl76682_range_table[i].als) continue; if (chan->type == IIO_INTENSITY && val2 != isl76682_range_table[i].ir) continue; But it's up to you and Jonathan. > + scoped_guard(mutex, &chip->lock) > + chip->range = isl76682_range_table[i].range; > + return 0; > + } > + > + return -EINVAL; > +} ... > + case IIO_CHAN_INFO_RAW: > + switch (chan->type) { > + case IIO_LIGHT: > + ret = isl76682_get(chip, false, val); > + return (ret < 0) ? ret : IIO_VAL_INT; > + case IIO_INTENSITY: > + ret = isl76682_get(chip, true, val); > + return (ret < 0) ? ret : IIO_VAL_INT; > + default: > + break; > + } > + > + return -EINVAL; default: return -EINVAL; ... > +static const struct of_device_id isl76682_of_match[] = { > + { .compatible = "isil,isl76682", }, Inner comma is not needed. > + { } > +}; ... > + > +module_i2c_driver(isl76682_driver); > +MODULE_DESCRIPTION("ISL76682 Ambient Light Sensor driver"); ...other way around: }; module_i2c_driver(isl76682_driver); MODULE_DESCRIPTION("ISL76682 Ambient Light Sensor driver"); ... Assuming you address all these, Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
On 11/21/23 19:09, Andy Shevchenko wrote: > On Tue, Nov 21, 2023 at 04:10:40AM +0100, 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. > > ... > >> + 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; >> + } > > Redundant 'else' and you can combine if:s. > > if (chan->type == IIO_LIGHT && val2 != isl76682_range_table[i].als) > continue; > if (chan->type == IIO_INTENSITY && val2 != isl76682_range_table[i].ir) > continue; > > But it's up to you and Jonathan. > >> + scoped_guard(mutex, &chip->lock) >> + chip->range = isl76682_range_table[i].range; >> + return 0; >> + } >> + >> + return -EINVAL; >> +} > > ... > >> + case IIO_CHAN_INFO_RAW: >> + switch (chan->type) { >> + case IIO_LIGHT: >> + ret = isl76682_get(chip, false, val); >> + return (ret < 0) ? ret : IIO_VAL_INT; >> + case IIO_INTENSITY: >> + ret = isl76682_get(chip, true, val); >> + return (ret < 0) ? ret : IIO_VAL_INT; > >> + default: >> + break; >> + } >> + >> + return -EINVAL; > > default: > return -EINVAL; > > ... > >> +static const struct of_device_id isl76682_of_match[] = { >> + { .compatible = "isil,isl76682", }, > > Inner comma is not needed. > >> + { } >> +}; > > ... > >> + >> +module_i2c_driver(isl76682_driver); >> +MODULE_DESCRIPTION("ISL76682 Ambient Light Sensor driver"); > > ...other way around: > > }; > module_i2c_driver(isl76682_driver); > > MODULE_DESCRIPTION("ISL76682 Ambient Light Sensor driver"); > > ... > > Assuming you address all these, > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> All that added to V5 , thanks. I'll still wait for a few days with sending that V5 though.
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; 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: Anyways - does this sound like a sensible thing to do? I guess it could help simplifying some drivers a little. 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 - 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(). Anyways - this driver looks good to me. (What a long way of saying that). Reviewed-by: Matti Vaittinen <mazziesaccount@gmail.com> > + > + scoped_guard(mutex, &chip->lock) > + chip->range = isl76682_range_table[i].range; > + return 0; > + } > + > + return -EINVAL; > +} > + Yours, -- Matti
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 !
On 11/23/23 02:26, Marek Vasut wrote: > 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> >>> --- ... >> >> 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 ? Well, my thinking was to go with baby-steps. Eg, start by supporting just int+micro - but yes. As I wrote below, this can be expanded by allowing specifying the type. >> 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. At least I know what I need to pack in the suitcase! ;) Oh, by the way, the wafers were great! >> 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 ? I definitely think a relation "register value" <=> "item from a table" is very much used also outside the IIO. So yes, a generic regmap helper for doing write as a "look value from table and write corresponding value to a register" and "read value from register and return me a corresponding item from a table" would be very usable. There is a tradeoff when doing a generic one instead of making it targeted for IIO use. Supporting different types of data is likely to make the code a bit hairy. Also, the IIO way of having these IIO_VAL_* flags does probably require IIO - specific wrappers in any case. >> 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. Yep. Kind of makes sense. It's just me who is already used to see the regmap writes straight in the scale setting function - and RMW provided by regmap is really pretty simple when regmap lock is used. But this is very much Ok for me just like you wrote it. Yours, -- Matti
On Tue, 21 Nov 2023 04:10:40 +0100 Marek Vasut <marex@denx.de> 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> Nice little driver. A few comments inline. Thanks, Jonathan > 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 @@ > + > +static int isl76682_read_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 ret; > + int i; > + > + if (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY) > + return -EINVAL; > + > + guard(mutex)(&chip->lock); > + > + switch (mask) { > + case IIO_CHAN_INFO_RAW: > + switch (chan->type) { > + case IIO_LIGHT: > + ret = isl76682_get(chip, false, val); > + return (ret < 0) ? ret : IIO_VAL_INT; > + case IIO_INTENSITY: > + ret = isl76682_get(chip, true, val); > + return (ret < 0) ? ret : IIO_VAL_INT; > + default: > + break; return here and drop the one below. > + } > + > + return -EINVAL; > + case IIO_CHAN_INFO_SCALE: > + for (i = 0; i < ARRAY_SIZE(isl76682_range_table); i++) { > + if (chip->range != isl76682_range_table[i].range) > + continue; > + > + *val = 0; > + switch (chan->type) { > + case IIO_LIGHT: > + *val2 = isl76682_range_table[i].als; > + return IIO_VAL_INT_PLUS_MICRO; > + case IIO_INTENSITY: > + *val2 = isl76682_range_table[i].ir; > + return IIO_VAL_INT_PLUS_MICRO; > + default: > + return -EINVAL; > + } > + } > + return -EINVAL; > + case IIO_CHAN_INFO_INT_TIME: > + *val = 0; > + *val2 = ISL76682_INT_TIME_US; > + return IIO_VAL_INT_PLUS_MICRO; > + default: > + return -EINVAL; > + } > +} > +static int isl76682_probe(struct i2c_client *client) > +{ > + const struct i2c_device_id *id = i2c_client_get_device_id(client); For reason below, we should avoid accessing this directly. If you need the associated data later, then use the access functions that try the property.h accessors first and fallback to this only if this fails. That ends up much less fragile. > + struct device *dev = &client->dev; > + struct isl76682_chip *chip; > + struct iio_dev *indio_dev; > + int ret; > + > + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); > + if (!indio_dev) > + return -ENOMEM; > + > + chip = iio_priv(indio_dev); > + > + mutex_init(&chip->lock); > + > + chip->regmap = devm_regmap_init_i2c(client, &isl76682_regmap_config); > + ret = PTR_ERR_OR_ZERO(chip->regmap); > + if (ret) > + return dev_err_probe(dev, ret, "Error initializing regmap\n"); > + > + chip->range = ISL76682_COMMAND_RANGE_LUX_1K; > + > + ret = isl76682_clear_configure_reg(chip); > + if (ret < 0) > + return ret; > + > + ret = devm_add_action_or_reset(dev, isl76682_reset_action, chip); > + if (ret) > + return ret; > + > + indio_dev->info = &isl76682_info; > + indio_dev->channels = isl76682_channels; > + indio_dev->num_channels = ARRAY_SIZE(isl76682_channels); > + indio_dev->name = id->name; This tends to be fragile as drivers gain more entrees in the id and of table. So the name should always be retrieved from a chip_info structure. Given you don't need one of those yet, I'd prefer just having the string here directly. We've been bitten by this before so I get paranoid about it :) > + indio_dev->modes = INDIO_DIRECT_MODE; > + > + return devm_iio_device_register(dev, indio_dev); > +}
On 11/23/23 09:24, Matti Vaittinen wrote: > On 11/23/23 02:26, Marek Vasut wrote: >> On 11/22/23 13:17, Matti Vaittinen wrote: >>> On 11/21/23 05:10, Marek Vasut wrote: ..snip >>> 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 ? > > Well, my thinking was to go with baby-steps. Eg, start by supporting > just int+micro - but yes. As I wrote below, this can be expanded by > allowing specifying the type. > >>> 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?) >>> ... snip >> >> 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 ? > > I definitely think a relation "register value" <=> "item from a table" > is very much used also outside the IIO. So yes, a generic regmap helper > for doing write as a "look value from table and write corresponding > value to a register" and "read value from register and return me a > corresponding item from a table" would be very usable. > > There is a tradeoff when doing a generic one instead of making it > targeted for IIO use. Supporting different types of data is likely to > make the code a bit hairy. Also, the IIO way of having these IIO_VAL_* > flags does probably require IIO - specific wrappers in any case. I had some spare time so drafted following: +struct reg_val_table { + int *reg_vals; + int *vals; + int num_vals; +}; ... +/** + * regtable_find_val - find a value matching register setting + * + * Search given table for value mathcing a register setting. + * + * @table: Table from which the register setting - value pairs are + * searched. + * @reg: Register value for which the matching physical value is + * searched. + * @val: Pointer to location where the found value will be stored. + * + * returns: 0 on success, negative errno if table is invalid or match is + * not found. + */ +int regtable_find_val(const struct reg_val_table *table, int reg, int *val) +/** + * regtable_find_reg - find a register setting matching given value. + * + * Search given table for a register setting matching a value. + * + * @table: Table from which the register setting - value pairs are + * searched. + * @val: Value for which the matching register setting is searched. + * @reg: Pointer to location where the found register value will be + * stored. + * + * returns: 0 on success, negative errno if table is invalid or match is + * not found. + */ +int regtable_find_reg(const struct reg_val_table *table, int val, int *reg) +/** + * regtable_find_greater_than_val - find the closest greater val and reg + * + * Search given table for the smallest value which is still greater than + * the given value. Both the found value and corresponding register + * setting are returned unless given pointers are NULL. + * + * @table: Table from which the register setting - value pairs are + * searched. + * @val_cmp: Value to which the values stored in table are compared to. + * @reg: NULL or pointer to location where the matching register + * setting value will be stored. + * @val: NULL or pointer to location where the found value will be + * stored. + * + * returns: 0 on success, negative errno if table is invalid or match is + * not found. + */ +int regtable_find_greater_than_val(const struct reg_val_table *table, int val_cmp, + int *reg, int *val) +/** + * regtable_find_smaller_than_val - find the closest smaller val and reg + * + * Search given table for the greatest value which is still smaller than + * the given value. Both the found value and corresponding register + * setting are returned unless given pointers are NULL. + * + * @table: Table from which the register setting - value pairs are + * searched. + * @val_cmp: Value to which the values stored in table are compared to. + * @reg: NULL or pointer to location where the matching register + * setting value will be stored. + * @val: NULL or pointer to location where the found value will be + * stored. + * + * returns: 0 on success, negative errno if table is invalid or match is + * not found. + */ +int regtable_find_smaller_than_val(const struct reg_val_table *table, + int val_cmp, int *reg, int *val) and +struct regmap_regval_table { + const struct reg_val_table table; + int reg; + int mask; +}; +/** + * regmap_table_value_set - update register to match human-understandable value + * @map: Register map + * @table: Table describing register-value, human-readable value relation + * value: Human understandable value to configure in hardware. + * + * Return: 0 on success, negative errno on error. + */ +int regmap_table_value_set(struct regmap *map, + const struct regmap_regval_table *table, int value) +/** + * regmap_table_value_get - return human-understandable configuration + * + * Reads hardware or regmap cache for current hardware configuration and + * converts the read register value to human understandable entity. + * @map: Register map + * @table: Table describing register-value, human-readable value relation + * value: Human understandable value to configure in hardware. + * + * Return: 0 on success, negative errno on error. + */ +int regmap_table_value_get(struct regmap *map, + const struct regmap_regval_table *table, int *value) (for anyone interested, whole thing + tests can be found from: https://github.com/M-Vaittinen/linux/commits/regtable/ Just last 3 commits.) I am however having difficulties in seeing how this could be utilized by IIO, which tends to rely on values being represented by two integers (val and val2). Any suggestions regarding this idea? I'm wondering if I should just scrap this and try seeing if I can make an IIO-specific helper(s) - or if someone sees this would bring additional value worth an proper RFC? I don't want to sen an RFC for people to properly review if this idea is just plain stupid :) Yours, -- Matti
On Fri, 15 Dec 2023 14:06:32 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > On 11/23/23 09:24, Matti Vaittinen wrote: > > On 11/23/23 02:26, Marek Vasut wrote: > >> On 11/22/23 13:17, Matti Vaittinen wrote: > >>> On 11/21/23 05:10, Marek Vasut wrote: > > ..snip > > >>> 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 ? > > > > Well, my thinking was to go with baby-steps. Eg, start by supporting > > just int+micro - but yes. As I wrote below, this can be expanded by > > allowing specifying the type. > > > >>> 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?) > >>> > > ... snip > > >> > >> 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 ? > > > > I definitely think a relation "register value" <=> "item from a table" > > is very much used also outside the IIO. So yes, a generic regmap helper > > for doing write as a "look value from table and write corresponding > > value to a register" and "read value from register and return me a > > corresponding item from a table" would be very usable. > > > > There is a tradeoff when doing a generic one instead of making it > > targeted for IIO use. Supporting different types of data is likely to > > make the code a bit hairy. Also, the IIO way of having these IIO_VAL_* > > flags does probably require IIO - specific wrappers in any case. > > I had some spare time so drafted following: > > +struct reg_val_table { > + int *reg_vals; > + int *vals; > + int num_vals; > +}; > > ... > > +/** > + * regtable_find_val - find a value matching register setting > + * > + * Search given table for value mathcing a register setting. > + * > + * @table: Table from which the register setting - value pairs are > + * searched. > + * @reg: Register value for which the matching physical value is > + * searched. > + * @val: Pointer to location where the found value will be stored. > + * > + * returns: 0 on success, negative errno if table is invalid or match is > + * not found. > + */ > +int regtable_find_val(const struct reg_val_table *table, int reg, int *val) > > > +/** > + * regtable_find_reg - find a register setting matching given value. > + * > + * Search given table for a register setting matching a value. > + * > + * @table: Table from which the register setting - value pairs are > + * searched. > + * @val: Value for which the matching register setting is searched. > + * @reg: Pointer to location where the found register value will be > + * stored. > + * > + * returns: 0 on success, negative errno if table is invalid or match is > + * not found. > + */ > +int regtable_find_reg(const struct reg_val_table *table, int val, int *reg) > > > +/** > + * regtable_find_greater_than_val - find the closest greater val and reg Maybe use rounding terminology rather than greater than? regtable_find_val_roundup()? > + * > + * Search given table for the smallest value which is still greater than > + * the given value. Both the found value and corresponding register > + * setting are returned unless given pointers are NULL. > + * > + * @table: Table from which the register setting - value pairs are > + * searched. > + * @val_cmp: Value to which the values stored in table are compared to. > + * @reg: NULL or pointer to location where the matching register > + * setting value will be stored. > + * @val: NULL or pointer to location where the found value will be > + * stored. > + * > + * returns: 0 on success, negative errno if table is invalid or match is > + * not found. > + */ > +int regtable_find_greater_than_val(const struct reg_val_table *table, > int val_cmp, > + int *reg, int *val) > > regtable_find_val_rounddown()? > +/** > + * regtable_find_smaller_than_val - find the closest smaller val and reg > + * > + * Search given table for the greatest value which is still smaller than > + * the given value. Both the found value and corresponding register > + * setting are returned unless given pointers are NULL. > + * > + * @table: Table from which the register setting - value pairs are > + * searched. > + * @val_cmp: Value to which the values stored in table are compared to. > + * @reg: NULL or pointer to location where the matching register > + * setting value will be stored. > + * @val: NULL or pointer to location where the found value will be > + * stored. > + * > + * returns: 0 on success, negative errno if table is invalid or match is > + * not found. > + */ > +int regtable_find_smaller_than_val(const struct reg_val_table *table, > + int val_cmp, int *reg, int *val) > > > and > > +struct regmap_regval_table { > + const struct reg_val_table table; > + int reg; > + int mask; > +}; > > +/** > + * regmap_table_value_set - update register to match > human-understandable value > + * @map: Register map > + * @table: Table describing register-value, human-readable value > relation > + * value: Human understandable value to configure in hardware. > + * > + * Return: 0 on success, negative errno on error. > + */ > +int regmap_table_value_set(struct regmap *map, > + const struct regmap_regval_table *table, int > value) > > > +/** > + * regmap_table_value_get - return human-understandable configuration > + * > + * Reads hardware or regmap cache for current hardware configuration and > + * converts the read register value to human understandable entity. > + * @map: Register map > + * @table: Table describing register-value, human-readable value > relation > + * value: Human understandable value to configure in hardware. > + * > + * Return: 0 on success, negative errno on error. > + */ > +int regmap_table_value_get(struct regmap *map, > + const struct regmap_regval_table *table, int > *value) > > > (for anyone interested, whole thing + tests can be found from: > https://github.com/M-Vaittinen/linux/commits/regtable/ > Just last 3 commits.) > > I am however having difficulties in seeing how this could be utilized by > IIO, which tends to rely on values being represented by two integers > (val and val2). Two integers and a type to make it harder still... IIO_VAL_INT_PLUS_MICRO etc though I guess that might not need representing as generally the caller would know what that was. Fixed point (ish) is a pain, but not come up with a better presentation yet :( > > Any suggestions regarding this idea? I'm wondering if I should just > scrap this and try seeing if I can make an IIO-specific helper(s) - or > if someone sees this would bring additional value worth an proper RFC? I > don't want to sen an RFC for people to properly review if this idea is > just plain stupid :) It seems useful in general but I guess it's a question of whether you can find enough users to justify it. > > Yours, > -- Matti >
On 12/17/23 15:06, Jonathan Cameron wrote: > On Fri, 15 Dec 2023 14:06:32 +0200 > Matti Vaittinen <mazziesaccount@gmail.com> wrote: > >> On 11/23/23 09:24, Matti Vaittinen wrote: >>> On 11/23/23 02:26, Marek Vasut wrote: >>>> On 11/22/23 13:17, Matti Vaittinen wrote: >>>>> On 11/21/23 05:10, Marek Vasut wrote: >> >> ..snip >> >>>>> 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 ? >>> >>> Well, my thinking was to go with baby-steps. Eg, start by supporting >>> just int+micro - but yes. As I wrote below, this can be expanded by >>> allowing specifying the type. >>> >>>>> 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?) >>>>> >> >> ... snip >> >>>> >>>> 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 ? >>> >>> I definitely think a relation "register value" <=> "item from a table" >>> is very much used also outside the IIO. So yes, a generic regmap helper >>> for doing write as a "look value from table and write corresponding >>> value to a register" and "read value from register and return me a >>> corresponding item from a table" would be very usable. >>> >>> There is a tradeoff when doing a generic one instead of making it >>> targeted for IIO use. Supporting different types of data is likely to >>> make the code a bit hairy. Also, the IIO way of having these IIO_VAL_* >>> flags does probably require IIO - specific wrappers in any case. >> >> I had some spare time so drafted following: >> >> +struct reg_val_table { >> + int *reg_vals; >> + int *vals; >> + int num_vals; >> +}; >> >> ... >> >> +/** >> + * regtable_find_val - find a value matching register setting >> + * >> + * Search given table for value mathcing a register setting. >> + * >> + * @table: Table from which the register setting - value pairs are >> + * searched. >> + * @reg: Register value for which the matching physical value is >> + * searched. >> + * @val: Pointer to location where the found value will be stored. >> + * >> + * returns: 0 on success, negative errno if table is invalid or match is >> + * not found. >> + */ >> +int regtable_find_val(const struct reg_val_table *table, int reg, int *val) >> >> >> +/** >> + * regtable_find_reg - find a register setting matching given value. >> + * >> + * Search given table for a register setting matching a value. >> + * >> + * @table: Table from which the register setting - value pairs are >> + * searched. >> + * @val: Value for which the matching register setting is searched. >> + * @reg: Pointer to location where the found register value will be >> + * stored. >> + * >> + * returns: 0 on success, negative errno if table is invalid or match is >> + * not found. >> + */ >> +int regtable_find_reg(const struct reg_val_table *table, int val, int *reg) >> >> >> +/** >> + * regtable_find_greater_than_val - find the closest greater val and reg > Maybe use rounding terminology rather than greater than? > > regtable_find_val_roundup()? Would be much better indeed. Thanks! >> + * Search given table for the smallest value which is still greater than >> + * the given value. Both the found value and corresponding register >> + * setting are returned unless given pointers are NULL. >> + * >> + * @table: Table from which the register setting - value pairs are >> + * searched. >> + * @val_cmp: Value to which the values stored in table are compared to. >> + * @reg: NULL or pointer to location where the matching register >> + * setting value will be stored. >> + * @val: NULL or pointer to location where the found value will be >> + * stored. >> + * >> + * returns: 0 on success, negative errno if table is invalid or match is >> + * not found. >> + */ >> +int regtable_find_greater_than_val(const struct reg_val_table *table, >> int val_cmp, >> + int *reg, int *val) > > regtable_find_val_rounddown()? Yes. >> +/** >> + * regtable_find_smaller_than_val - find the closest smaller val and reg >> + * >> + * Search given table for the greatest value which is still smaller than >> + * the given value. Both the found value and corresponding register >> + * setting are returned unless given pointers are NULL. >> + * >> + * @table: Table from which the register setting - value pairs are >> + * searched. >> + * @val_cmp: Value to which the values stored in table are compared to. >> + * @reg: NULL or pointer to location where the matching register >> + * setting value will be stored. >> + * @val: NULL or pointer to location where the found value will be >> + * stored. >> + * >> + * returns: 0 on success, negative errno if table is invalid or match is >> + * not found. >> + */ >> +int regtable_find_smaller_than_val(const struct reg_val_table *table, >> + int val_cmp, int *reg, int *val) >> >> >> and >> >> +struct regmap_regval_table { >> + const struct reg_val_table table; >> + int reg; >> + int mask; >> +}; >> >> +/** >> + * regmap_table_value_set - update register to match >> human-understandable value >> + * @map: Register map >> + * @table: Table describing register-value, human-readable value >> relation >> + * value: Human understandable value to configure in hardware. >> + * >> + * Return: 0 on success, negative errno on error. >> + */ >> +int regmap_table_value_set(struct regmap *map, >> + const struct regmap_regval_table *table, int >> value) >> >> >> +/** >> + * regmap_table_value_get - return human-understandable configuration >> + * >> + * Reads hardware or regmap cache for current hardware configuration and >> + * converts the read register value to human understandable entity. >> + * @map: Register map >> + * @table: Table describing register-value, human-readable value >> relation >> + * value: Human understandable value to configure in hardware. >> + * >> + * Return: 0 on success, negative errno on error. >> + */ >> +int regmap_table_value_get(struct regmap *map, >> + const struct regmap_regval_table *table, int >> *value) >> >> >> (for anyone interested, whole thing + tests can be found from: >> https://github.com/M-Vaittinen/linux/commits/regtable/ >> Just last 3 commits.) >> >> I am however having difficulties in seeing how this could be utilized by >> IIO, which tends to rely on values being represented by two integers >> (val and val2). > > Two integers and a type to make it harder still... IIO_VAL_INT_PLUS_MICRO etc > though I guess that might not need representing as generally the caller > would know what that was. Fixed point (ish) is a pain, but not come up with a better > presentation yet :( I think the IIO-representation is fine. Sure it sucks that the real world is set up in such a imperfect way that we do need fractions, but as we do, IIO-way is just fine. The thing IIO 'stuff' requires (and is not available in this draft) is 64 storage bits for the values. I guess I could increase the size in tables to use u64 - and have an IIO-specific layer which could pack/unpack the val and val2 in that 64 bits appropriately - but I'm not sure it's worth the hassle. Besides, when users only need 32bits, 64bit tables would be waste. Adding support for both 32 and 64 bit tables would probably work - but maybe this is just an overkill for a simple task. Creating just IIO-specific helpers would be much leaner. >> Any suggestions regarding this idea? I'm wondering if I should just >> scrap this and try seeing if I can make an IIO-specific helper(s) - or >> if someone sees this would bring additional value worth an proper RFC? I >> don't want to sen an RFC for people to properly review if this idea is >> just plain stupid :) > > It seems useful in general but I guess it's a question of whether you can find > enough users to justify it. You're right of course. I think my question should've been if someone can instantly think a type of devices that could benefit from these helpers :) Anyways, bit Thank You for the input! Your help is much appreciated as always! :) Let's see if my "hands NOT full" time continues so I can rethink this. Yours, -- Matti
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; + } + + scoped_guard(mutex, &chip->lock) + chip->range = isl76682_range_table[i].range; + return 0; + } + + return -EINVAL; +} + +static int isl76682_read_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 ret; + int i; + + if (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY) + return -EINVAL; + + guard(mutex)(&chip->lock); + + switch (mask) { + case IIO_CHAN_INFO_RAW: + switch (chan->type) { + case IIO_LIGHT: + ret = isl76682_get(chip, false, val); + return (ret < 0) ? ret : IIO_VAL_INT; + case IIO_INTENSITY: + ret = isl76682_get(chip, true, val); + return (ret < 0) ? ret : IIO_VAL_INT; + default: + break; + } + + return -EINVAL; + case IIO_CHAN_INFO_SCALE: + for (i = 0; i < ARRAY_SIZE(isl76682_range_table); i++) { + if (chip->range != isl76682_range_table[i].range) + continue; + + *val = 0; + switch (chan->type) { + case IIO_LIGHT: + *val2 = isl76682_range_table[i].als; + return IIO_VAL_INT_PLUS_MICRO; + case IIO_INTENSITY: + *val2 = isl76682_range_table[i].ir; + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } + } + return -EINVAL; + case IIO_CHAN_INFO_INT_TIME: + *val = 0; + *val2 = ISL76682_INT_TIME_US; + return IIO_VAL_INT_PLUS_MICRO; + default: + return -EINVAL; + } +} + +static int illuminance_scale_available[] = { + 0, 15000, + 0, 60000, + 0, 240000, + 0, 960000, +}; + +static int intensity_scale_available[] = { + 0, 10500, + 0, 42000, + 0, 168000, + 0, 673000, +}; + +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US }; + +static int isl76682_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_SCALE: + switch (chan->type) { + case IIO_LIGHT: + *vals = illuminance_scale_available; + *length = ARRAY_SIZE(illuminance_scale_available); + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + case IIO_INTENSITY: + *vals = intensity_scale_available; + *length = ARRAY_SIZE(intensity_scale_available); + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } + case IIO_CHAN_INFO_INT_TIME: + *vals = integration_time_available; + *length = ARRAY_SIZE(integration_time_available); + *type = IIO_VAL_INT_PLUS_MICRO; + return IIO_AVAIL_LIST; + default: + return -EINVAL; + } +} + +static const struct iio_chan_spec isl76682_channels[] = { + { + .type = IIO_LIGHT, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), + }, { + .type = IIO_INTENSITY, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_type_available = BIT(IIO_CHAN_INFO_SCALE), + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME), + } +}; + +static const struct iio_info isl76682_info = { + .read_avail = isl76682_read_avail, + .read_raw = isl76682_read_raw, + .write_raw = isl76682_write_raw, +}; + +static int isl76682_clear_configure_reg(struct isl76682_chip *chip) +{ + struct device *dev = regmap_get_device(chip->regmap); + int ret; + + ret = regmap_write(chip->regmap, ISL76682_REG_COMMAND, 0x0); + if (ret < 0) + dev_err(dev, "Error %d clearing the CONFIGURE register\n", ret); + + /* + * In the success case, the command register was zeroed out. + * + * In the error case, we do not know in which state the command + * register is, so we assume it is zeroed out, so that it would + * be reprogrammed at the next data read out, and at that time + * we hope it would be reprogrammed successfully. That is very + * much a best effort approach. + */ + chip->command = 0; + + return ret; +} + +static void isl76682_reset_action(void *chip) +{ + isl76682_clear_configure_reg(chip); +} + +static bool isl76682_is_volatile_reg(struct device *dev, unsigned int reg) +{ + switch (reg) { + case ISL76682_REG_ALSIR_L: + case ISL76682_REG_ALSIR_U: + return true; + default: + return false; + } +} + +static const struct regmap_config isl76682_regmap_config = { + .reg_bits = 8, + .val_bits = 8, + .volatile_reg = isl76682_is_volatile_reg, + .max_register = ISL76682_NUM_REGS - 1, + .num_reg_defaults_raw = ISL76682_NUM_REGS, + .cache_type = REGCACHE_FLAT, +}; + +static int isl76682_probe(struct i2c_client *client) +{ + const struct i2c_device_id *id = i2c_client_get_device_id(client); + struct device *dev = &client->dev; + struct isl76682_chip *chip; + struct iio_dev *indio_dev; + int ret; + + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*chip)); + if (!indio_dev) + return -ENOMEM; + + chip = iio_priv(indio_dev); + + mutex_init(&chip->lock); + + chip->regmap = devm_regmap_init_i2c(client, &isl76682_regmap_config); + ret = PTR_ERR_OR_ZERO(chip->regmap); + if (ret) + return dev_err_probe(dev, ret, "Error initializing regmap\n"); + + chip->range = ISL76682_COMMAND_RANGE_LUX_1K; + + ret = isl76682_clear_configure_reg(chip); + if (ret < 0) + return ret; + + ret = devm_add_action_or_reset(dev, isl76682_reset_action, chip); + if (ret) + return ret; + + indio_dev->info = &isl76682_info; + indio_dev->channels = isl76682_channels; + indio_dev->num_channels = ARRAY_SIZE(isl76682_channels); + indio_dev->name = id->name; + indio_dev->modes = INDIO_DIRECT_MODE; + + return devm_iio_device_register(dev, indio_dev); +} + +static const struct i2c_device_id isl76682_id[] = { + { "isl76682" }, + {} +}; +MODULE_DEVICE_TABLE(i2c, isl76682_id); + +static const struct of_device_id isl76682_of_match[] = { + { .compatible = "isil,isl76682", }, + { } +}; +MODULE_DEVICE_TABLE(of, isl76682_of_match); + +static struct i2c_driver isl76682_driver = { + .driver = { + .name = "isl76682", + .of_match_table = isl76682_of_match, + }, + .probe = isl76682_probe, + .id_table = isl76682_id, +}; + +module_i2c_driver(isl76682_driver); +MODULE_DESCRIPTION("ISL76682 Ambient Light Sensor driver"); +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
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