diff mbox series

[v3,1/2] dt-bindings: iio: light: isl76682: Document ISL76682

Message ID 20231119212515.54001-1-marex@denx.de
State Superseded
Headers show
Series [v3,1/2] dt-bindings: iio: light: isl76682: Document ISL76682 | expand

Checks

Context Check Description
robh/patch-applied success
robh/checkpatch success
robh/dtbs-check warning build log
robh/dt-meta-schema success

Commit Message

Marek Vasut Nov. 19, 2023, 9:24 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. 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
V3: No change
---
 Documentation/devicetree/bindings/trivial-devices.yaml | 2 ++
 1 file changed, 2 insertions(+)

Comments

Andy Shevchenko Nov. 20, 2023, 12:02 p.m. UTC | #1 
On Sun, Nov 19, 2023 at 10:24:35PM +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.

...

> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>

This is incomplete.

At least array_size.h, bits.h, cleanup.h, types.h are missing.

...

> +#define ISL76682_ADC_MAX			0xffff

Wouldn't the (BIT(16) - 1) be better in order to show that this is limited by
a bit field capacity. Also you can use FIELD_MAX().

...

> +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;

This is less maintainable, use it directly (see below).
Ah, the value is even not used!

> +	int i;
> +
> +	if (chan->type != IIO_LIGHT && chan->type != IIO_INTENSITY)
> +		return -EINVAL;
> +
> +	guard(mutex)(&chip->lock);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			ret = isl76682_get(chip, false, val);
> +			return (ret < 0) ? ret : IIO_VAL_INT;
> +		case IIO_INTENSITY:
> +			ret = isl76682_get(chip, true, val);
> +			return (ret < 0) ? ret : IIO_VAL_INT;
> +		default:

> +			break;

			return -EINVAL;

> +		}
> +
> +		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;

...

> +			if (chan->type == IIO_LIGHT)
> +				*val2 = isl76682_range_table[i].als;
> +			else if (chan->type == IIO_INTENSITY)
> +				*val2 = isl76682_range_table[i].ir;
> +			else
> +				return -EINVAL;

Why not switch-case for consistency's sake?

> +			return IIO_VAL_INT_PLUS_MICRO;
> +		}
> +		return -EINVAL;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		*val2 = ISL76682_INT_TIME_US;
> +		return IIO_VAL_INT_PLUS_MICRO;

> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;

Move it to default.

> +}

...

> +static int illuminance_scale_available[] = {
> +	0, 15000,
> +	0, 60000,
> +	0, 240000,
> +	0, 960000

Leave trailing comma as it's not a terminator and will be an additional burden
if this list is going to be expanded in the future.

> +};
> +
> +static int intensity_scale_available[] = {
> +	0, 10500,
> +	0, 42000,
> +	0, 168000,
> +	0, 673000

Ditto.

> +};

...

> +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:
> +		if (chan->type == IIO_LIGHT) {
> +			*vals = illuminance_scale_available;
> +			*length = ARRAY_SIZE(illuminance_scale_available);
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		} else if (chan->type == IIO_INTENSITY) {
> +			*vals = intensity_scale_available;
> +			*length = ARRAY_SIZE(intensity_scale_available);
> +			*type = IIO_VAL_INT_PLUS_MICRO;
> +			return IIO_AVAIL_LIST;
> +		}
> +		return -EINVAL;

Same Q: why not switch-case?

> +	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:
> +		break;
> +	}
> +
> +	return -EINVAL;

Move it to default.

> +}

...

> +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;

Even in an error case? Is it a problem?

> +	return ret;
> +}

...

> +static void isl76682_reset_action(void *data)
> +{
> +	isl76682_clear_configure_reg((struct isl76682_chip *)data);

Redundant casting, just name the field as you line, seems "chip" is what being
used elsewhere.

> +}

...

> +	i2c_set_clientdata(client, indio_dev);

How is this being used?

...

> +	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");
> +	}

Redundant {}

Also can be rewritten as

	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");

...

> +static const struct i2c_device_id isl76682_id[] = {
> +	{"isl76682", 0},

Unneeded 0.

> +	{}
> +};

...

> +static const struct of_device_id isl76682_of_match[] = {
> +	{ .compatible = "isil,isl76682", },
> +	{ },

Remove comma from the terminator.

> +};

...

> +

Redundant blank line.

> +module_i2c_driver(isl76682_driver);
Marek Vasut Nov. 21, 2023, 3:14 a.m. UTC | #2 
On 11/20/23 13:02, Andy Shevchenko wrote:

[...]

>> +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;
> 
> Even in an error case? Is it a problem?

I added a comment in V4, if the I2C communication fails, hopefully the 
next time user reads data the command register will be rewritten again 
and the communication would have succeeded by then (assuming this was 
some weird glitch on the I2C bus). So this is best effort attempt to 
recover from that.

>> +static const struct of_device_id isl76682_of_match[] = {
>> +	{ .compatible = "isil,isl76682", },
>> +	{ },
> 
> Remove comma from the terminator.
> 
>> +};
> 
> ...
> 
>> +
> 
> Redundant blank line.
> 
>> +module_i2c_driver(isl76682_driver);

That ^ newline is above the module_i2c_driver or below it ?

I removed the one below .
Andy Shevchenko Nov. 21, 2023, 5:20 p.m. UTC | #3 
On Tue, Nov 21, 2023 at 04:14:54AM +0100, Marek Vasut wrote:
> On 11/20/23 13:02, Andy Shevchenko wrote:

[...]

> > > +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;
> > 
> > Even in an error case? Is it a problem?
> 
> I added a comment in V4, if the I2C communication fails, hopefully the next
> time user reads data the command register will be rewritten again and the
> communication would have succeeded by then (assuming this was some weird
> glitch on the I2C bus). So this is best effort attempt to recover from that.

OK.

...

> > > +
> > 
> > Redundant blank line.
> > 
> > > +module_i2c_driver(isl76682_driver);
> 
> That ^ newline is above the module_i2c_driver or below it ?
> 
> I removed the one below .

Hmm... Comment was clearly about above one (as you see a single + there).
Marek Vasut Nov. 21, 2023, 6:05 p.m. UTC | #4 
On 11/21/23 18:20, Andy Shevchenko wrote:
> On Tue, Nov 21, 2023 at 04:14:54AM +0100, Marek Vasut wrote:
>> On 11/20/23 13:02, Andy Shevchenko wrote:
> 
> [...]
> 
>>>> +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;
>>>
>>> Even in an error case? Is it a problem?
>>
>> I added a comment in V4, if the I2C communication fails, hopefully the next
>> time user reads data the command register will be rewritten again and the
>> communication would have succeeded by then (assuming this was some weird
>> glitch on the I2C bus). So this is best effort attempt to recover from that.
> 
> OK.
> 
> ...
> 
>>>> +
>>>
>>> Redundant blank line.
>>>
>>>> +module_i2c_driver(isl76682_driver);
>>
>> That ^ newline is above the module_i2c_driver or below it ?
>>
>> I removed the one below .
> 
> Hmm... Comment was clearly about above one (as you see a single + there).

I'll fix that up in V5 then, will wait a bit for further feedback before 
posting that one.
diff mbox series

Patch

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