[v5,2/2] iio: light: isl76682: Add ISL76682 driver

Commit Message

Marek Vasut Nov. 25, 2023, 10:26 p.m. UTC

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.

Reviewed-by: Andy Shevchenko <[e-mail]>
Reviewed-by: Matti Vaittinen <[e-mail]>
Signed-off-by: Marek Vasut <[e-mail]>
---
Cc: Alexander Stein <[e-mail]>
Cc: Andre Werner <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Bjorn Helgaas <[e-mail]>
Cc: Conor Dooley <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Guenter Roeck <[e-mail]>
Cc: Jonathan Cameron <[e-mail]>
Cc: Krzysztof Kozlowski <[e-mail]>
Cc: Lars-Peter Clausen <[e-mail]>
Cc: Luca Ceresoli <[e-mail]>
Cc: Mark Brown <[e-mail]>
Cc: Matti Vaittinen <[e-mail]>
Cc: Naresh Solanki <[e-mail]>
Cc: Patrick Rudolph <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Stefan Windfeldt-Prytz <[e-mail]>
Cc: Vincent Tremblay <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
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
V5: - Remove newline, add newline, around module_i2c_driver
    - Combine if:s
    - Drop inner comma in isl76682_of_match
    - Replace one more break with return -EINVAL
    - Add RB from Andy and Matti
    - Dispose of dependency on i2c_device_id, use ISL76682_DRIVER_NAME
---
 drivers/iio/light/Kconfig    |  15 ++
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/isl76682.c | 361 +++++++++++++++++++++++++++++++++++
 3 files changed, 377 insertions(+)
 create mode 100644 drivers/iio/light/isl76682.c

Comments

Jonathan Cameron Nov. 26, 2023, 6:16 p.m. UTC | #1

On Sat, 25 Nov 2023 23:26:23 +0100
Marek Vasut <[e-mail]> 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.
> 
> Reviewed-by: Andy Shevchenko <[e-mail]>
> Reviewed-by: Matti Vaittinen <[e-mail]>
> Signed-off-by: Marek Vasut <[e-mail]>

Hi Marek,

One last question + a comment in general. Act on that if you like.

Thanks,

Jonathan


> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US };

Why have an available attribute for a single value. Is it useful for anything?


> +static int isl76682_probe(struct i2c_client *client)
> +{

...

> +	indio_dev->info = &isl76682_info;
> +	indio_dev->channels = isl76682_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(isl76682_channels);
> +	indio_dev->name = ISL76682_DRIVER_NAME;
Trivial but I'm not a fan of using defines in cases like this. It just makes
me go find the define when I could see the string directly here.

In cases where matching or similar strictly requires the naming to be the same
in various places a define is useful. In this case less so.

Anyhow, it's a very minor comment so never mind if you prefer to leave it
as it stands.

> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Andre Werner Nov. 26, 2023, 7:33 p.m. UTC | #2

Dear Marek,

see remarks below.

Best regards.

André

----- Original Message -----
From: "Marek Vasut" <[e-mail]>
To: [e-mail]
Cc: "Marek Vasut" <[e-mail]>, "Andy Shevchenko" <[e-mail]>, "Matti Vaittinen" <[e-mail]>, "Alexander Stein" <[e-mail]>, "Werner Andre" <[e-mail]>, "Bjorn Helgaas" <[e-mail]>, "Conor Dooley" <[e-mail]>, "Fabio Estevam" <[e-mail]>, "Guenter Roeck" <[e-mail]>, "Jonathan Cameron" <[e-mail]>, "Krzysztof Kozlowski" <[e-mail]>, "Lars-Peter Clausen" <[e-mail]>, "Luca Ceresoli" <[e-mail]>, "Mark Brown" <[e-mail]>, "Naresh Solanki" <[e-mail]>, "Patrick Rudolph" <[e-mail]>, "Rob Herring" <[e-mail]>, "Stefan Windfeldt-Prytz" <[e-mail]>, "Vincent Tremblay" <[e-mail]>, [e-mail]
Sent: Saturday, November 25, 2023 11:26:23 PM
Subject: [PATCH v5 2/2] iio: light: isl76682: Add ISL76682 driver

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.

Reviewed-by: Andy Shevchenko <[e-mail]>
Reviewed-by: Matti Vaittinen <[e-mail]>
Signed-off-by: Marek Vasut <[e-mail]>
---
Cc: Alexander Stein <[e-mail]>
Cc: Andre Werner <[e-mail]>
Cc: Andy Shevchenko <[e-mail]>
Cc: Bjorn Helgaas <[e-mail]>
Cc: Conor Dooley <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Guenter Roeck <[e-mail]>
Cc: Jonathan Cameron <[e-mail]>
Cc: Krzysztof Kozlowski <[e-mail]>
Cc: Lars-Peter Clausen <[e-mail]>
Cc: Luca Ceresoli <[e-mail]>
Cc: Mark Brown <[e-mail]>
Cc: Matti Vaittinen <[e-mail]>
Cc: Naresh Solanki <[e-mail]>
Cc: Patrick Rudolph <[e-mail]>
Cc: Rob Herring <[e-mail]>
Cc: Stefan Windfeldt-Prytz <[e-mail]>
Cc: Vincent Tremblay <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
---
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
V5: - Remove newline, add newline, around module_i2c_driver
    - Combine if:s
    - Drop inner comma in isl76682_of_match
    - Replace one more break with return -EINVAL
    - Add RB from Andy and Matti
    - Dispose of dependency on i2c_device_id, use ISL76682_DRIVER_NAME
---
 drivers/iio/light/Kconfig    |  15 ++
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/isl76682.c | 361 +++++++++++++++++++++++++++++++++++
 3 files changed, 377 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..1bb44950fbc3e
--- /dev/null
+++ b/drivers/iio/light/isl76682.c
@@ -0,0 +1,361 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * IIO driver for the light sensor ISL76682.
+ * ISL76682 is Ambient Light Sensor
+ *
+ * Copyright (c) 2023 Marek Vasut <[e-mail]>
+ */
+
+#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)
+
+#define ISL76682_DRIVER_NAME			"isl76682"
+
+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;

In my understanding, this is not necessary as you use that 'iio_chan_spec' to specify 
the channels available at all. So there will be no other channel types created by the IIO environment, I think.

+
+	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 && val2 != isl76682_range_table[i].als)
+				continue;
+		if (chan->type == IIO_INTENSITY && 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;

Same here as described above.

+
+	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:
+			return -EINVAL;
+		}
+		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)
+{
+	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 = ISL76682_DRIVER_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_DRIVER_NAME,
+		.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 <[e-mail]>");

Marek Vasut Nov. 26, 2023, 10:09 p.m. UTC | #3

On 11/26/23 19:16, Jonathan Cameron wrote:
> On Sat, 25 Nov 2023 23:26:23 +0100
> Marek Vasut <[e-mail]> 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.
>>
>> Reviewed-by: Andy Shevchenko <[e-mail]>
>> Reviewed-by: Matti Vaittinen <[e-mail]>
>> Signed-off-by: Marek Vasut <[e-mail]>
> 
> Hi Marek,
> 
> One last question + a comment in general. Act on that if you like.
> 
> Thanks,
> 
> Jonathan
> 
> 
>> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US };
> 
> Why have an available attribute for a single value. Is it useful for anything?

To report it to userspace, iio-sensor-proxy uses that to control the ALS 
poll interval .

>> +static int isl76682_probe(struct i2c_client *client)
>> +{
> 
> ...
> 
>> +	indio_dev->info = &isl76682_info;
>> +	indio_dev->channels = isl76682_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(isl76682_channels);
>> +	indio_dev->name = ISL76682_DRIVER_NAME;
> Trivial but I'm not a fan of using defines in cases like this. It just makes
> me go find the define when I could see the string directly here.
> 
> In cases where matching or similar strictly requires the naming to be the same
> in various places a define is useful. In this case less so.
> 
> Anyhow, it's a very minor comment so never mind if you prefer to leave it
> as it stands.

I added it to V6 .

I'll wait for the integration time reply above and then send V6 .

Marek Vasut Nov. 26, 2023, 10:17 p.m. UTC | #4

On 11/26/23 20:33, Dr.-Ing. Andre Werner wrote:

[...]

> +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;
> 
> In my understanding, this is not necessary as you use that 'iio_chan_spec' to specify
> the channels available at all. So there will be no other channel types created by the IIO environment, I think.

That's a good point, I think we can drop it in this case and the next one.

Jonathan , is there any reason you can think of why this shouldn't be 
dropped ?

I'll add it into V6, thanks.

Andy Shevchenko Nov. 27, 2023, 4:11 p.m. UTC | #5

On Sun, Nov 26, 2023 at 08:33:26PM +0100, Dr.-Ing. Andre Werner wrote:
> Dear Marek,
> 
> see remarks below.

Do not top-post!
Do not overquote hundreds of lines just for a single comment!

Andy Shevchenko Nov. 27, 2023, 4:13 p.m. UTC | #6

On Sat, Nov 25, 2023 at 11:26:23PM +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 && val2 != isl76682_range_table[i].als)
> +				continue;
> +		if (chan->type == IIO_INTENSITY && val2 != isl76682_range_table[i].ir)
> +				continue;

You forgot to drop indentation level for 'continue' lines.

> +		scoped_guard(mutex, &chip->lock)
> +			chip->range = isl76682_range_table[i].range;
> +		return 0;
> +	}

Marek Vasut Nov. 27, 2023, 8:52 p.m. UTC | #7

On 11/27/23 17:13, Andy Shevchenko wrote:
> On Sat, Nov 25, 2023 at 11:26:23PM +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 && val2 != isl76682_range_table[i].als)
>> +				continue;
>> +		if (chan->type == IIO_INTENSITY && val2 != isl76682_range_table[i].ir)
>> +				continue;
> 
> You forgot to drop indentation level for 'continue' lines.

I noticed that too and already fixed it in v6 .

Jonathan Cameron Dec. 1, 2023, 6:17 p.m. UTC | #8

On Sun, 26 Nov 2023 23:09:36 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/26/23 19:16, Jonathan Cameron wrote:
> > On Sat, 25 Nov 2023 23:26:23 +0100
> > Marek Vasut <[e-mail]> 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.
> >>
> >> Reviewed-by: Andy Shevchenko <[e-mail]>
> >> Reviewed-by: Matti Vaittinen <[e-mail]>
> >> Signed-off-by: Marek Vasut <[e-mail]>  
> > 
> > Hi Marek,
> > 
> > One last question + a comment in general. Act on that if you like.
> > 
> > Thanks,
> > 
> > Jonathan
> > 
> >   
> >> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US };  
> > 
> > Why have an available attribute for a single value. Is it useful for anything?  
> 
> To report it to userspace, iio-sensor-proxy uses that to control the ALS 
> poll interval .

It should use integration_time, not the associated available attribute.

> 
> >> +static int isl76682_probe(struct i2c_client *client)
> >> +{  
> > 
> > ...
> >   
> >> +	indio_dev->info = &isl76682_info;
> >> +	indio_dev->channels = isl76682_channels;
> >> +	indio_dev->num_channels = ARRAY_SIZE(isl76682_channels);
> >> +	indio_dev->name = ISL76682_DRIVER_NAME;  
> > Trivial but I'm not a fan of using defines in cases like this. It just makes
> > me go find the define when I could see the string directly here.
> > 
> > In cases where matching or similar strictly requires the naming to be the same
> > in various places a define is useful. In this case less so.
> > 
> > Anyhow, it's a very minor comment so never mind if you prefer to leave it
> > as it stands.  
> 
> I added it to V6 .
> 
> I'll wait for the integration time reply above and then send V6 .
>

Jonathan Cameron Dec. 1, 2023, 6:20 p.m. UTC | #9

On Sun, 26 Nov 2023 23:17:55 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/26/23 20:33, Dr.-Ing. Andre Werner wrote:
> 
> [...]
> 
> > +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;
> > 
> > In my understanding, this is not necessary as you use that 'iio_chan_spec' to specify
> > the channels available at all. So there will be no other channel types created by the IIO environment, I think.  
> 
> That's a good point, I think we can drop it in this case and the next one.
> 
> Jonathan , is there any reason you can think of why this shouldn't be 
> dropped ?

I don't mind a little paranoia, but fine with dropping it as well.

The reason paranoia is fine is that if other channels are added in future
we might not remember to fix this up if they happen to have no writeable attributes.
I leave that assessment up to driver authors.

Jonathan

> 
> I'll add it into V6, thanks.
>

Marek Vasut Dec. 2, 2023, 6:46 a.m. UTC | #10

On 12/1/23 19:20, Jonathan Cameron wrote:
> On Sun, 26 Nov 2023 23:17:55 +0100
> Marek Vasut <[e-mail]> wrote:
> 
>> On 11/26/23 20:33, Dr.-Ing. Andre Werner wrote:
>>
>> [...]
>>
>>> +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;
>>>
>>> In my understanding, this is not necessary as you use that 'iio_chan_spec' to specify
>>> the channels available at all. So there will be no other channel types created by the IIO environment, I think.
>>
>> That's a good point, I think we can drop it in this case and the next one.
>>
>> Jonathan , is there any reason you can think of why this shouldn't be
>> dropped ?
> 
> I don't mind a little paranoia, but fine with dropping it as well.
> 
> The reason paranoia is fine is that if other channels are added in future
> we might not remember to fix this up if they happen to have no writeable attributes.
> I leave that assessment up to driver authors.

So, dropped, since this chip seems to be very much legacy hardware.

Marek Vasut Dec. 2, 2023, 6:48 a.m. UTC | #11

On 12/1/23 19:17, Jonathan Cameron wrote:
> On Sun, 26 Nov 2023 23:09:36 +0100
> Marek Vasut <[e-mail]> wrote:
> 
>> On 11/26/23 19:16, Jonathan Cameron wrote:
>>> On Sat, 25 Nov 2023 23:26:23 +0100
>>> Marek Vasut <[e-mail]> 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.
>>>>
>>>> Reviewed-by: Andy Shevchenko <[e-mail]>
>>>> Reviewed-by: Matti Vaittinen <[e-mail]>
>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>
>>> Hi Marek,
>>>
>>> One last question + a comment in general. Act on that if you like.
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>
>>>    
>>>> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US };
>>>
>>> Why have an available attribute for a single value. Is it useful for anything?
>>
>> To report it to userspace, iio-sensor-proxy uses that to control the ALS
>> poll interval .
> 
> It should use integration_time, not the associated available attribute.

So, what should I do with this feedback ?

I did send the V6 already, so shall I change anything in V7 ?

Jonathan Cameron Dec. 4, 2023, 11:13 a.m. UTC | #12

On Sat, 2 Dec 2023 07:48:12 +0100
Marek Vasut <[e-mail]> wrote:

> On 12/1/23 19:17, Jonathan Cameron wrote:
> > On Sun, 26 Nov 2023 23:09:36 +0100
> > Marek Vasut <[e-mail]> wrote:
> >   
> >> On 11/26/23 19:16, Jonathan Cameron wrote:  
> >>> On Sat, 25 Nov 2023 23:26:23 +0100
> >>> Marek Vasut <[e-mail]> 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.
> >>>>
> >>>> Reviewed-by: Andy Shevchenko <[e-mail]>
> >>>> Reviewed-by: Matti Vaittinen <[e-mail]>
> >>>> Signed-off-by: Marek Vasut <[e-mail]>  
> >>>
> >>> Hi Marek,
> >>>
> >>> One last question + a comment in general. Act on that if you like.
> >>>
> >>> Thanks,
> >>>
> >>> Jonathan
> >>>
> >>>      
> >>>> +static int integration_time_available[] = { 0, ISL76682_INT_TIME_US };  
> >>>
> >>> Why have an available attribute for a single value. Is it useful for anything?  
> >>
> >> To report it to userspace, iio-sensor-proxy uses that to control the ALS
> >> poll interval .  
> > 
> > It should use integration_time, not the associated available attribute.  
> 
> So, what should I do with this feedback ?
> 
> I did send the V6 already, so shall I change anything in V7 ?

I'll reply there.  Just noticed some other oddities around available.

Patch

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..1bb44950fbc3e
--- /dev/null
+++ b/drivers/iio/light/isl76682.c
@@ -0,0 +1,361 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * IIO driver for the light sensor ISL76682.
+ * ISL76682 is Ambient Light Sensor
+ *
+ * Copyright (c) 2023 Marek Vasut <[e-mail]>
+ */
+
+#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)
+
+#define ISL76682_DRIVER_NAME			"isl76682"
+
+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 && val2 != isl76682_range_table[i].als)
+				continue;
+		if (chan->type == IIO_INTENSITY && 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:
+			return -EINVAL;
+		}
+		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)
+{
+	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 = ISL76682_DRIVER_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_DRIVER_NAME,
+		.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 <[e-mail]>");