[PATCH v2 1/2] dt-bindings: iio: light: isl76682: Document ISL76682

[PATCH v2 1/2] dt-bindings: iio: light: isl76682: Document ISL76682

[Marek Vasut]

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. Document it as trivial device.

Acked-by: Conor Dooley <conor.dooley@microchip.com>
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: Add AB from Conor
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

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
-- 
2.42.0

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

[Marek Vasut]

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
---
 drivers/iio/light/Kconfig    |   8 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/isl76682.c | 336 +++++++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/iio/light/isl76682.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 45edba797e4c7..124c45f509754 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -252,6 +252,14 @@ 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
+	  Provides driver for the Intersil's ISL76682 device. This driver
+	  supports the sysfs interface to get the ALS and IR intensity.
+
 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..64ec8b8fe71ee
--- /dev/null
+++ b/drivers/iio/light/isl76682.c
@@ -0,0 +1,336 @@
+// 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/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/regmap.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.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		0
+#define ISL76682_COMMAND_RANGE_LUX_4K		(1 << 0)
+#define ISL76682_COMMAND_RANGE_LUX_16K		(2 << 0)
+#define ISL76682_COMMAND_RANGE_LUX_64K		(3 << 0)
+#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_ADC_MAX			0xffff
+
+struct isl76682_chip {
+	struct mutex			lock;
+	struct regmap			*regmap;
+	u8				range;
+	u8				command;
+};
+
+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;
+	}
+
+	return regmap_bulk_read(chip->regmap, ISL76682_REG_ALSIR_L, data, 2);
+}
+
+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);
+	u8 range;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	if (mask != IIO_CHAN_INFO_SCALE)
+		return -EINVAL;
+
+	if (val != 0)
+		return -EINVAL;
+
+	if (chan->type == IIO_LIGHT) {
+		if (val2 == 15000)		/* 0.015 ... 1000 lux */
+			range = ISL76682_COMMAND_RANGE_LUX_1K;
+		else if (val2 == 60000)		/* 0.060 ... 4000 lux */
+			range = ISL76682_COMMAND_RANGE_LUX_4K;
+		else if (val2 == 240000)	/* 0.240 ... 16000 lux */
+			range = ISL76682_COMMAND_RANGE_LUX_16K;
+		else if (val2 == 960000)	/* 0.960 ... 64000 lux */
+			range = ISL76682_COMMAND_RANGE_LUX_64K;
+		else
+			return -EINVAL;
+	} else if (chan->type == IIO_INTENSITY) {
+		if (val2 == 10500)		/* 0.0105 .. 1000 lux */
+			range = ISL76682_COMMAND_RANGE_LUX_1K;
+		else if (val2 == 42000)		/* 0.042 ... 4000 lux */
+			range = ISL76682_COMMAND_RANGE_LUX_4K;
+		else if (val2 == 168000)	/* 0.168 ... 16000 lux */
+			range = ISL76682_COMMAND_RANGE_LUX_16K;
+		else if (val2 == 673000)	/* 0.673 ... 64000 lux */
+			range = ISL76682_COMMAND_RANGE_LUX_64K;
+		else
+			return -EINVAL;
+	} else {
+		return -EINVAL;
+	}
+
+	mutex_lock(&chip->lock);
+	chip->range = range;
+	mutex_unlock(&chip->lock);
+
+	return 0;
+}
+
+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 = -EINVAL;
+
+	mutex_lock(&chip->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			ret = isl76682_get(chip, false, val);
+			break;
+		case IIO_INTENSITY:
+			ret = isl76682_get(chip, true, val);
+			break;
+		default:
+			break;
+		}
+
+		if (ret < 0)
+			break;
+
+		ret = IIO_VAL_INT;
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		*val = 0;
+		switch (chan->type) {
+		case IIO_LIGHT:
+			if (chip->range == ISL76682_COMMAND_RANGE_LUX_1K)
+				*val2 = 15000;
+			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_4K)
+				*val2 = 60000;
+			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_16K)
+				*val2 = 240000;
+			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_64K)
+				*val2 = 960000;
+			else
+				break;
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			break;
+		case IIO_INTENSITY:
+			if (chip->range == ISL76682_COMMAND_RANGE_LUX_1K)
+				*val2 = 10500;
+			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_4K)
+				*val2 = 42000;
+			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_16K)
+				*val2 = 168000;
+			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_64K)
+				*val2 = 673000;
+			else
+				break;
+			ret = IIO_VAL_INT_PLUS_MICRO;
+			break;
+		default:
+			break;
+		}
+		break;
+	default:
+		break;
+	}
+
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+static IIO_CONST_ATTR(in_illuminance_scale_available, "0.015 0.06 0.24 0.96");
+static IIO_CONST_ATTR(in_intensity_scale_available, "0.0105 0.042 0.168 0.673");
+static IIO_CONST_ATTR(integration_time_available, "0.090");	/* 90 ms */
+
+static struct attribute *isl76682_attributes[] = {
+	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
+	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
+	&iio_const_attr_integration_time_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group isl29108_group = {
+	.attrs = isl76682_attributes,
+};
+
+static const struct iio_chan_spec isl76682_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	}, {
+		.type = IIO_INTENSITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+	}
+};
+
+static const struct iio_info isl76682_info = {
+	.attrs		= &isl29108_group,
+	.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);
+
+	chip->command = 0;
+
+	return ret;
+}
+
+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);
+
+	i2c_set_clientdata(client, indio_dev);
+	mutex_init(&chip->lock);
+
+	chip->regmap = devm_regmap_init_i2c(client, &isl76682_regmap_config);
+	if (IS_ERR(chip->regmap)) {
+		return dev_err_probe(dev, PTR_ERR(chip->regmap),
+				     "Error initializing regmap\n");
+	}
+
+	chip->range = ISL76682_COMMAND_RANGE_LUX_1K;
+
+	ret = isl76682_clear_configure_reg(chip);
+	if (ret < 0)
+		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;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Device registration failed\n");
+
+	return 0;
+}
+
+static void isl76682_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct isl76682_chip *chip = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+
+	isl76682_clear_configure_reg(chip);
+}
+
+static const struct i2c_device_id isl76682_id[] = {
+	{"isl76682", 0},
+	{}
+};
+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,
+	.remove		= isl76682_remove,
+	.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>");
-- 
2.42.0

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

[Jonathan Cameron]

On Sat, 18 Nov 2023 01:06:10 +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>

Hi Marek

A few comments inline,

Jonathan

>  drivers/iio/light/Kconfig    |   8 +
>  drivers/iio/light/Makefile   |   1 +
>  drivers/iio/light/isl76682.c | 336 +++++++++++++++++++++++++++++++++++
>  3 files changed, 345 insertions(+)
>  create mode 100644 drivers/iio/light/isl76682.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 45edba797e4c7..124c45f509754 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -252,6 +252,14 @@ 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
> +	  Provides driver for the Intersil's ISL76682 device. This driver
> +	  supports the sysfs interface to get the ALS and IR intensity.
> +
One of the bots tends to moan about short descriptions + it can be useful
to let someone know the module name. Hence I'd add the usual boilerplate description
for that here.

> diff --git a/drivers/iio/light/isl76682.c b/drivers/iio/light/isl76682.c
> new file mode 100644
> index 0000000000000..64ec8b8fe71ee
> --- /dev/null
> +++ b/drivers/iio/light/isl76682.c
> @@ -0,0 +1,336 @@
> +// 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/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

After changes below, this sysfs.h include should not be needed.

> +
> +#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		0
> +#define ISL76682_COMMAND_RANGE_LUX_4K		(1 << 0)
> +#define ISL76682_COMMAND_RANGE_LUX_16K		(2 << 0)
> +#define ISL76682_COMMAND_RANGE_LUX_64K		(3 << 0)
As you are writing these into a field with a mask, 0, 1, 2, 3 should
be fine as the shift is done by FIELD_PREP()

> +#define ISL76682_COMMAND_RANGE_LUX_MASK		GENMASK(1, 0)

> +struct isl76682_chip {
> +	struct mutex			lock;
A lock needs a comment on what scope it covers.  Be as specific as you can
as you need to distinguish it from the locks in regmap or the underlying bus code.
> +	struct regmap			*regmap;
> +	u8				range;
> +	u8				command;
> +};

> +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);
> +	u8 range;
> +
> +	if (chan->type != IIO_LIGHT)
> +		return -EINVAL;
> +
> +	if (mask != IIO_CHAN_INFO_SCALE)
> +		return -EINVAL;
> +
> +	if (val != 0)
> +		return -EINVAL;
> +
> +	if (chan->type == IIO_LIGHT) {
> +		if (val2 == 15000)		/* 0.015 ... 1000 lux */

As below, I'd prefer these to be lookups in the same table used for read_avail()
callback.

> +			range = ISL76682_COMMAND_RANGE_LUX_1K;
> +		else if (val2 == 60000)		/* 0.060 ... 4000 lux */
> +			range = ISL76682_COMMAND_RANGE_LUX_4K;
> +		else if (val2 == 240000)	/* 0.240 ... 16000 lux */
> +			range = ISL76682_COMMAND_RANGE_LUX_16K;
> +		else if (val2 == 960000)	/* 0.960 ... 64000 lux */
> +			range = ISL76682_COMMAND_RANGE_LUX_64K;
> +		else
> +			return -EINVAL;
> +	} else if (chan->type == IIO_INTENSITY) {
> +		if (val2 == 10500)		/* 0.0105 .. 1000 lux */
> +			range = ISL76682_COMMAND_RANGE_LUX_1K;
> +		else if (val2 == 42000)		/* 0.042 ... 4000 lux */
> +			range = ISL76682_COMMAND_RANGE_LUX_4K;
> +		else if (val2 == 168000)	/* 0.168 ... 16000 lux */
> +			range = ISL76682_COMMAND_RANGE_LUX_16K;
> +		else if (val2 == 673000)	/* 0.673 ... 64000 lux */
> +			range = ISL76682_COMMAND_RANGE_LUX_64K;
> +		else
> +			return -EINVAL;
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	mutex_lock(&chip->lock);

scoped_guard(mutex, &chip->lock); or guard(mutex)(&chip->lock);
nice here as well.

> +	chip->range = range;
> +	mutex_unlock(&chip->lock);
> +
> +	return 0;
> +}
> +
> +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 = -EINVAL;
> +
> +	mutex_lock(&chip->lock);
guard(mutex)(&chip->lock);

Will allow you to make direct returns instead of breaking to where the
unlock is, generally simplifying the code flow.

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			ret = isl76682_get(chip, false, val);
> +			break;
> +		case IIO_INTENSITY:
> +			ret = isl76682_get(chip, true, val);
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		if (ret < 0)
> +			break;
> +
> +		ret = IIO_VAL_INT;
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			if (chip->range == ISL76682_COMMAND_RANGE_LUX_1K)
> +				*val2 = 15000;
> +			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_4K)
> +				*val2 = 60000;
> +			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_16K)
> +				*val2 = 240000;
> +			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_64K)
> +				*val2 = 960000;
> +			else
> +				break;
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +			break;
> +		case IIO_INTENSITY:
> +			if (chip->range == ISL76682_COMMAND_RANGE_LUX_1K)
> +				*val2 = 10500;
> +			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_4K)
> +				*val2 = 42000;
> +			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_16K)
> +				*val2 = 168000;
> +			else if (chip->range == ISL76682_COMMAND_RANGE_LUX_64K)
> +				*val2 = 673000;
> +			else
> +				break;
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +			break;
> +		default:
> +			break;
> +		}
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	mutex_unlock(&chip->lock);
> +
> +	return ret;
> +}
> +
> +static IIO_CONST_ATTR(in_illuminance_scale_available, "0.015 0.06 0.24 0.96");
> +static IIO_CONST_ATTR(in_intensity_scale_available, "0.0105 0.042 0.168 0.673");
> +static IIO_CONST_ATTR(integration_time_available, "0.090");	/* 90 ms */
> +
> +static struct attribute *isl76682_attributes[] = {
> +	&iio_const_attr_in_illuminance_scale_available.dev_attr.attr,
> +	&iio_const_attr_in_intensity_scale_available.dev_attr.attr,
> +	&iio_const_attr_integration_time_available.dev_attr.attr,
Please use the read_avail() callback and matching bitmaps for these.
Doing them with custom attributes is something we are trying to move away
from as it prevents any sensible in kernel access to the values and tends
to end up with duplication of data (and potential inconsistencies)
given you also have to match against the values.

There are lots of older drivers that predate that callback being added
that we haven't converted over yet.

> +	NULL,
> +};
> +

...

> +static void isl76682_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct isl76682_chip *chip = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +
> +	isl76682_clear_configure_reg(chip);

Use a devm_add_action_or_reset() call with a custom callback to unwind this
automatically... As they you can use devm_iio_device_register() and
get rid of the need for any remove function at all.

I am a little curious as to why it needs to be called here as it doesn't obviously
match a call in probe that is being unwound.  As such a comment on 'why' would
also be good.


> +}
> +

Re: [PATCH v2 1/2] dt-bindings: iio: light: isl76682: Document ISL76682

[Jonathan Cameron]

On Sat, 18 Nov 2023 01:06:09 +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. Document it as trivial device.
> 
> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> Signed-off-by: Marek Vasut <marex@denx.de>

I'm always a bit in 2 minds about trivial-devices as it doesn't
include a basic power supply.  In this case vdd which I'd prefer
to see documented even on such a simple device as this.
Not everyone is going to leave it powered up all the time!

Still I'm not that fussed. Maybe it's worth allowing vdd-supply in
trivial-devices.yaml?

Jonathan

> ---
> 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: Add AB from Conor
> ---
>  Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> 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

Re: [PATCH v2 1/2] dt-bindings: iio: light: isl76682: Document ISL76682

[Marek Vasut]

On 11/19/23 16:07, Jonathan Cameron wrote:
> On Sat, 18 Nov 2023 01:06:09 +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. Document it as trivial device.
>>
>> Acked-by: Conor Dooley <conor.dooley@microchip.com>
>> Signed-off-by: Marek Vasut <marex@denx.de>
> 
> I'm always a bit in 2 minds about trivial-devices as it doesn't
> include a basic power supply.  In this case vdd which I'd prefer
> to see documented even on such a simple device as this.
> Not everyone is going to leave it powered up all the time!
> 
> Still I'm not that fussed. Maybe it's worth allowing vdd-supply in
> trivial-devices.yaml?

The hardware I have is powered all the time, so I'd be adding completely 
untested code which is no less useless for me (at least right now). Lets 
postpone this until there is a need for it, can we ?

I addressed all the other comments in 2/2.

Re: [PATCH v2 1/2] dt-bindings: iio: light: isl76682: Document ISL76682

[Jonathan Cameron]

On Sun, 19 Nov 2023 19:08:45 +0100
Marek Vasut <marex@denx.de> wrote:

> On 11/19/23 16:07, Jonathan Cameron wrote:
> > On Sat, 18 Nov 2023 01:06:09 +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. Document it as trivial device.
> >>
> >> Acked-by: Conor Dooley <conor.dooley@microchip.com>
> >> Signed-off-by: Marek Vasut <marex@denx.de>  
> > 
> > I'm always a bit in 2 minds about trivial-devices as it doesn't
> > include a basic power supply.  In this case vdd which I'd prefer
> > to see documented even on such a simple device as this.
> > Not everyone is going to leave it powered up all the time!
> > 
> > Still I'm not that fussed. Maybe it's worth allowing vdd-supply in
> > trivial-devices.yaml?  
> 
> The hardware I have is powered all the time, so I'd be adding completely 
> untested code which is no less useless for me (at least right now). Lets 
> postpone this until there is a need for it, can we ?

It's untested but trivial code (one call and an error code check) but fair enough.
Previous experience says that we'll soon see it added... Its probably the most common
follow up patch for a new driver.

> 
> I addressed all the other comments in 2/2.
>