| Message ID | 20231121031043.327614-1-marex@denx.de |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v4,1/2] dt-bindings: iio: light: isl76682: Document ISL76682 | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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/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 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 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 >
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml index c3190f2a168a2..27164e9219276 100644 --- a/Documentation/devicetree/bindings/trivial-devices.yaml +++ b/Documentation/devicetree/bindings/trivial-devices.yaml @@ -179,6 +179,8 @@ properties: - isil,isl29030 # Intersil ISL68137 Digital Output Configurable PWM Controller - isil,isl68137 + # Intersil ISL76682 Ambient Light Sensor + - isil,isl76682 # Linear Technology LTC2488 - lineartechnology,ltc2488 # 5 Bit Programmable, Pulse-Width Modulator