Re: [PATCH v4 2/2] iio: add ISL29033 Ultra-Low Lux ambient-light-sensor

[Jonathan Cameron <jic23@jic23.retrosnub.co.uk>]

On Wed, 13 Jun 2018 21:12:36 +0200 (CEST)
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:

> still some minor comments below
> 
> it would be nice to have a changelog to easily see which review comments 
> have been addressed
Definitely.

Some minor additions from me.  All trivial coding style things I think but
good to clean up before we merge this.

Jonathan

> 
> > From: Borys Movchan <borysmn@axis.com>
> > 
> > Add Intersil ISL29033 Ultra-Low Lux, Low Power, Integrated
> > Digital Ambient Light Sensor driver.
> > 
> > The ISL29033 is an integrated ambient and infrared
> > light-to-digital converter. Its advanced, self-calibrated photodiode
> > array emulates human eye response with excellent IR rejection. The
> > on-chip 16-bit ADC is capable of rejecting 50Hz and 60Hz
> > flicker caused by artificial light sources. The lux range select
> > feature allows users to program the lux range for optimized
> > counts/lux.
> > 
> > datasheet: https://www.intersil.com/content/dam/intersil/documents/isl2/isl29033.pdf
Ideal is to have this in the code rather than the patch description.  Put it
in the header for the main c file.
> > 
> > Signed-off-by: Borys Movchan <borys.movchan@axis.com>
> > ---
> >  drivers/iio/light/Kconfig    |   9 +
> >  drivers/iio/light/Makefile   |   1 +
> >  drivers/iio/light/isl29033.c | 755 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 765 insertions(+)
> >  create mode 100644 drivers/iio/light/isl29033.c
> > 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index c7ef8d1862d6..ad98a9b24a65 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -182,6 +182,15 @@ config SENSORS_ISL29028
> >  	 Proximity value via iio. The ISL29028 provides the concurrent sensing
> >  	 of ambient light and proximity.
> >  
> > +config ISL29033
> > +	tristate "Intersil ISL29033 Ultra-Low Lux, Low Power, Integrated Digital Ambient Light Sensor"
> > +	depends on I2C
> > +	select REGMAP_I2C
> > +	help
> > +	 Provides driver for the Intersil's ISL29033 device.  
> 
> either 'Intersil's ISL29033 device' or 'the Intersil ISL29033 device'
> 
> > +	 This driver supports the sysfs interface to get the ALS and IR intensity
> > +	 value via IIO.
> > +
> >  config ISL29125
> >  	tristate "Intersil ISL29125 digital color light sensor"
> >  	depends on I2C
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index 80943af5d627..4955c2eebcbd 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -22,6 +22,7 @@ obj-$(CONFIG_HID_SENSOR_ALS)	+= hid-sensor-als.o
> >  obj-$(CONFIG_HID_SENSOR_PROX)	+= hid-sensor-prox.o
> >  obj-$(CONFIG_SENSORS_ISL29018)	+= isl29018.o
> >  obj-$(CONFIG_SENSORS_ISL29028)	+= isl29028.o
> > +obj-$(CONFIG_ISL29033)		+= isl29033.o
> >  obj-$(CONFIG_ISL29125)		+= isl29125.o
> >  obj-$(CONFIG_JSA1212)		+= jsa1212.o
> >  obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
> > diff --git a/drivers/iio/light/isl29033.c b/drivers/iio/light/isl29033.c
> > new file mode 100644
> > index 000000000000..dacc94ad6d2f
> > --- /dev/null
> > +++ b/drivers/iio/light/isl29033.c
> > @@ -0,0 +1,755 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * isl29033.c: A iio driver for the light sensor ISL 29033.
> > + *
> > + * IIO driver for monitoring ambient light intensity in lux and infrared
> > + * sensing.
> > + *
> > + * Copyright (c) 2018, Axis Communication AB
> > + * Author: Borys Movchan <Borys.Movchan@axis.com>
> > + */
> > +
> > +#include <linux/module.h>

Should be consistent on doing alphabetical order if you are
going to almost do it!

> > +#include <linux/acpi.h>
> > +#include <linux/delay.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/mutex.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
> > +
> > +#define ISL29033_REG_ADD_COMMAND1	0x00
> > +#define ISL29033_CMD1_OPMODE_SHIFT	5
> > +#define ISL29033_CMD1_OPMODE_MASK	(7 << ISL29033_CMD1_OPMODE_SHIFT)
> > +#define ISL29033_CMD1_OPMODE_POWER_DOWN	0
> > +#define ISL29033_CMD1_OPMODE_ALS_CONT	5
> > +#define ISL29033_CMD1_OPMODE_IR_CONT	6
> > +
> > +#define ISL29033_REG_ADD_COMMAND2	0x01
> > +#define ISL29033_CMD2_RESOLUTION_SHIFT	2
> > +#define ISL29033_CMD2_RESOLUTION_MASK	(0x3 << ISL29033_CMD2_RESOLUTION_SHIFT)
> > +
> > +#define ISL29033_CMD2_RANGE_SHIFT	0
> > +#define ISL29033_CMD2_RANGE_MASK	(0x3 << ISL29033_CMD2_RANGE_SHIFT)
> > +
> > +#define ISL29033_CMD2_SCHEME_SHIFT	7
> > +#define ISL29033_CMD2_SCHEME_MASK	(0x1 << ISL29033_CMD2_SCHEME_SHIFT)
BIT(ISL29033...) ?

> > +
> > +#define ISL29033_REG_ADD_DATA_LSB	0x02
> > +#define ISL29033_REG_ADD_DATA_MSB	0x03
> > +
> > +#define ISL29033_REG_TEST		0x08
> > +#define ISL29033_TEST_SHIFT		0
> > +#define ISL29033_TEST_MASK		(0xFF << ISL29033_TEST_SHIFT)

Preference for GENMASK for all multibit masks.

> > +
> > +#define ISL29033_REF_REXT		499	/* In kOhm */
ISL29023_REF_REXT_KOHMS and drop the comment?
> > +
> > +#define ISL29033_POWER_OFF_DELAY_MS	5000
> > +
> > +#define ISL29033_MICRO			1000000
> > +
> > +#define isl29033_int_utime(adcmax) \  
> 
> ISL29033_INT_UTIME() maybe? uppercase for #MACROS (here and elsewhere);
> a matter of taste it is...
> 
> > +	((unsigned int)(adcmax) * (ISL29033_MICRO / 1000) / 655)
> > +
> > +static const unsigned int isl29033_int_utimes[4] = {
> > +	isl29033_int_utime(65536),
> > +	isl29033_int_utime(4096),
> > +	isl29033_int_utime(256),
> > +	isl29033_int_utime(16)
> > +};
> > +
> > +#define isl29033_mkscale(range, adcmax)				\
> > +	 { (range) / (adcmax),					\
> > +	   ((unsigned int)(range) * (ISL29033_MICRO / 10)	\
> > +			/ (adcmax) * 10) % ISL29033_MICRO }
> > +
> > +static const struct isl29033_scale {
> > +	unsigned int scale;
> > +	unsigned int uscale;
> > +} isl29033_scales[4][4] = {
> > +	{
> > +	   isl29033_mkscale(125, 65536), isl29033_mkscale(500, 65536),
> > +	   isl29033_mkscale(2000, 65536), isl29033_mkscale(8000, 65536)

I'd put these one line each, no particular reason for the grouping in pairs.

> > +	},
> > +	{
> > +	   isl29033_mkscale(125, 4096), isl29033_mkscale(500, 4096),
> > +	   isl29033_mkscale(2000, 4096), isl29033_mkscale(8000, 4096)
> > +	},
> > +	{
> > +	   isl29033_mkscale(125, 256), isl29033_mkscale(500, 256),
> > +	   isl29033_mkscale(2000, 256), isl29033_mkscale(8000, 256)
> > +	},
> > +	{
> > +	   isl29033_mkscale(125, 16), isl29033_mkscale(500, 16),
> > +	   isl29033_mkscale(2000, 16), isl29033_mkscale(8000, 16)
> > +	}
> > +};
> > +
> > +struct isl29033_chip {
> > +	struct regmap		*regmap;
> > +	struct mutex		lock;
> > +	unsigned int		int_time;
> > +	unsigned int		calibscale;
> > +	unsigned int		ucalibscale;  
> 
> why not use struct isl29033_scale also for calibscale?
> 
> > +	struct isl29033_scale	scale;
> > +	unsigned int		rext;
> > +	unsigned int		opmode;
> > +};
> > +
> > +#define isl29033_rext_normalize(chip, val) \
> > +	((val) * ISL29033_REF_REXT / (chip)->rext)  
> 
> how about a function
> struct isl29033_scale isl29033_rext_normalize(struct isl29033_chip *chip, struct isl29033_scale scale)?
> the normalize functions are always called in pairs, part of the 
> computation is redundant...
> 
> unsigned int scale, unsigned int uscale as function arguments could be 
> replaced by struct isl29033_scale scale...
> 
> > +
> > +static unsigned int isl29033_rext_normalize2(struct isl29033_chip *chip,
> > +				     unsigned int val, unsigned int val2)
> > +{
> > +	val = val - isl29033_rext_normalize(chip, val)
> > +			* chip->rext / ISL29033_REF_REXT;
> > +	val2 = (val2 + val * ISL29033_MICRO) * ISL29033_REF_REXT / chip->rext;
> > +	return val2;
> > +}
> > +
> > +static int isl29033_set_integration_time(struct isl29033_chip *chip,
> > +					 unsigned int utime)
> > +{
> > +	unsigned int i;
> > +	int ret;
> > +	unsigned int int_time, new_int_time;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(isl29033_int_utimes); ++i) {
> > +		if (utime == isl29033_int_utimes[i]
> > +				* chip->rext / ISL29033_REF_REXT) {
> > +			new_int_time = i;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (i >= ARRAY_SIZE(isl29033_int_utimes))
> > +		return -EINVAL;
> > +
> > +	ret = regmap_update_bits(chip->regmap, ISL29033_REG_ADD_COMMAND2,
> > +				 ISL29033_CMD2_RESOLUTION_MASK,
> > +				 i << ISL29033_CMD2_RESOLUTION_SHIFT);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* Keep the same range when integration time changes */
> > +	int_time = chip->int_time;
> > +	for (i = 0; i < ARRAY_SIZE(isl29033_scales[int_time]); ++i) {
> > +		if (chip->scale.scale == isl29033_scales[int_time][i].scale &&
> > +		    chip->scale.uscale == isl29033_scales[int_time][i].uscale) {
> > +			chip->scale = isl29033_scales[new_int_time][i];
> > +			break;
> > +		}
> > +	}
> > +	chip->int_time = new_int_time;
> > +
> > +	return 0;
> > +}
> > +
> > +static int isl29033_set_scale(struct isl29033_chip *chip, int scale, int uscale)
> > +{
> > +	unsigned int i;
> > +	int ret;
> > +	struct isl29033_scale new_scale;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(isl29033_scales[chip->int_time]); ++i) {
> > +		if (scale ==
> > +			isl29033_rext_normalize(chip,
> > +				isl29033_scales[chip->int_time][i].scale)
> > +		    && uscale ==
> > +			isl29033_rext_normalize2(chip,
> > +				isl29033_scales[chip->int_time][i].scale,
> > +				isl29033_scales[chip->int_time][i].uscale)) {
> > +			new_scale = isl29033_scales[chip->int_time][i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (i >= ARRAY_SIZE(isl29033_scales[chip->int_time]))
> > +		return -EINVAL;
> > +
> > +	ret = regmap_update_bits(chip->regmap, ISL29033_REG_ADD_COMMAND2,
> > +				 ISL29033_CMD2_RANGE_MASK,
> > +				 i << ISL29033_CMD2_RANGE_SHIFT);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	chip->scale = new_scale;
> > +
> > +	return 0;
> > +}
> > +
> > +static int isl29033_set_mode(struct isl29033_chip *chip, int mode)
> > +{

No blank line here.

> > +
> > +	int ret;
> > +	unsigned int utime;
> > +
> > +	if (chip->opmode == mode)  
> 
> set_mode() uses int for mode, _chip struct uses unsigned int
> maybe use u8 everywhere to denote that it is a value limited by the 
> sensor's register?
> 
> > +		return 0;
> > +
> > +	ret = regmap_update_bits(chip->regmap, ISL29033_REG_ADD_COMMAND1,
> > +				ISL29033_CMD1_OPMODE_MASK,
> > +				mode << ISL29033_CMD1_OPMODE_SHIFT);
> > +	if (ret < 0) {
> > +		struct device *dev = regmap_get_device(chip->regmap);
> > +
> > +		dev_err(dev,

Just put the regmap... bit inline and drop the local variable for dev

> > +			"Error in setting operating mode with err %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	utime = isl29033_int_utimes[chip->int_time] * chip->rext
> > +			/ ISL29033_REF_REXT;
> > +
> > +	/* Chip needs twice more time while switching between modes */
> > +	if (chip->opmode != ISL29033_CMD1_OPMODE_POWER_DOWN)
> > +		utime *= 2;
> > +
> > +	if (utime < 20000)
> > +		usleep_range(utime, utime * 2);
> > +	else
> > +		msleep(utime / 1000);
> > +
> > +	chip->opmode = mode;

blank line before a simple return statement like this preferred.

> > +	return 0;
> > +}
> > +
> > +static int isl29033_read_sensor_input(struct isl29033_chip *chip)
> > +{
> > +	int ret;
> > +	u16 val;
> > +	struct device *dev = regmap_get_device(chip->regmap);
> > +
> > +	ret = regmap_bulk_read(chip->regmap, ISL29033_REG_ADD_DATA_LSB,
> > +				(u8 *)&val, 2);  
> 
> sizeof(val)
> 
> > +	if (ret < 0) {
> > +		dev_err(dev,
> > +			"Data bulk read error %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	val = be16_to_cpu(val);
> > +	dev_vdbg(dev, "Data read: %x\n", val);
> > +
> > +	return val;
> > +}
> > +
> > +static int isl29033_read_lux(struct isl29033_chip *chip, int *lux, int *ulux)
> > +{
> > +	int ret;
> > +	unsigned int tmpres;
> > +
> > +	ret = isl29033_set_mode(chip, ISL29033_CMD1_OPMODE_ALS_CONT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = isl29033_read_sensor_input(chip);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret++;  
> 
> maybe a comment why this is done
> 
> > +
> > +	tmpres = ret * isl29033_rext_normalize2(chip, chip->scale.scale,
> > +					  chip->scale.uscale);
> > +	*lux = ret * isl29033_rext_normalize(chip, chip->scale.scale)
> > +				+ tmpres / ISL29033_MICRO;
> > +	*ulux = tmpres % ISL29033_MICRO;
> > +
> > +	*lux = *lux * chip->calibscale;
> > +	*ulux = *ulux * chip->calibscale + *lux * chip->ucalibscale
> > +				+ *ulux * chip->ucalibscale / ISL29033_MICRO;
> > +
> > +	return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +static int isl29033_read_ir(struct isl29033_chip *chip, int *ir)
> > +{
> > +	int ret;
> > +
> > +	ret = isl29033_set_mode(chip, ISL29033_CMD1_OPMODE_IR_CONT);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = isl29033_read_sensor_input(chip);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*ir = ret;
> > +
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static ssize_t isl29033_in_illuminance_scale_available
> > +			(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct isl29033_chip *chip = iio_priv(indio_dev);
> > +	unsigned int i;
> > +	int len = 0;
> > +
> > +	mutex_lock(&chip->lock);
> > +	for (i = 0; i < ARRAY_SIZE(isl29033_scales[chip->int_time]); ++i)
> > +		len += sprintf(buf + len, "%d.%06d ",
> > +			isl29033_rext_normalize(chip,
> > +				isl29033_scales[chip->int_time][i].scale),
> > +			isl29033_rext_normalize2(chip,
> > +				isl29033_scales[chip->int_time][i].scale,
> > +				isl29033_scales[chip->int_time][i].uscale));
> > +	mutex_unlock(&chip->lock);
> > +
> > +	buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static ssize_t isl29033_in_illuminance_integration_time_available

a shorter name for these functions would not make them much harder to
understand but would make for more standard alignment perhaps?

> > +			(struct device *dev, struct device_attribute *attr,
> > +			 char *buf)
> > +{
> > +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > +	struct isl29033_chip *chip = iio_priv(indio_dev);
> > +	unsigned int i;
> > +	int len = 0;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(isl29033_int_utimes); ++i)
> > +		len += sprintf(buf + len, "0.%06d ",
> > +				   isl29033_int_utimes[i]
> > +				   * chip->rext / ISL29033_REF_REXT);
> > +
> > +	buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static int isl29033_runtime_pm_get(struct isl29033_chip *chip)
> > +{
> > +	int ret;
> > +	struct device *dev = regmap_get_device(chip->regmap);
> > +
> > +	ret = pm_runtime_get(dev);
> > +	if (ret < 0)
> > +		pm_runtime_put_noidle(dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static void isl29033_runtime_pm_put(struct isl29033_chip *chip)
> > +{
> > +	struct device *dev = regmap_get_device(chip->regmap);
> > +
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_runtime_put_autosuspend(dev);
> > +}
> > +  
> 
> delete newline
> 
> > +
> > +static int isl29033_write_raw(struct iio_dev *indio_dev,
> > +				  struct iio_chan_spec const *chan,
> > +				  int val,
> > +				  int val2,

combine at least val and val2 on one line.  They group in an obvious
fashion and there is no readability advantage in having them on separate lines.

> > +				  long mask)
> > +{
> > +	int ret;
> > +	struct isl29033_chip *chip = iio_priv(indio_dev);
> > +
> > +	ret = isl29033_runtime_pm_get(chip);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = -EINVAL;
> > +
> > +	mutex_lock(&chip->lock);
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		if (chan->type == IIO_LIGHT) {
> > +			chip->calibscale = val;
> > +			chip->ucalibscale = val2;
> > +			ret = 0;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		if (chan->type == IIO_LIGHT && !val)
> > +			ret = isl29033_set_integration_time(chip, val2);
> > +		break;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (chan->type == IIO_LIGHT)
> > +			ret = isl29033_set_scale(chip, val, val2);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	mutex_unlock(&chip->lock);
> > +	isl29033_runtime_pm_put(chip);
> > +
> > +	return ret;
> > +}
> > +
> > +static int isl29033_read_raw(struct iio_dev *indio_dev,
> > +				 struct iio_chan_spec const *chan,
> > +				 int *val,
> > +				 int *val2,
> > +				 long mask)
> > +{
> > +	int ret;
> > +	struct isl29033_chip *chip = iio_priv(indio_dev);
> > +
> > +	ret = isl29033_runtime_pm_get(chip);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = -EINVAL;
> > +
> > +	mutex_lock(&chip->lock);
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (chan->type == IIO_INTENSITY)
> > +			ret = isl29033_read_ir(chip, val);
> > +		break;
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		if (chan->type == IIO_LIGHT)
> > +			ret = isl29033_read_lux(chip, val, val2);
> > +		break;
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		if (chan->type == IIO_LIGHT) {
> > +			*val = 0;
> > +			*val2 = isl29033_int_utimes[chip->int_time]
> > +					* chip->rext / ISL29033_REF_REXT;
> > +			ret = IIO_VAL_INT_PLUS_MICRO;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (chan->type == IIO_LIGHT) {
> > +			*val = isl29033_rext_normalize(chip, chip->scale.scale);
> > +			*val2 = isl29033_rext_normalize2(chip,
> > +					chip->scale.scale, chip->scale.uscale);
> > +			ret = IIO_VAL_INT_PLUS_MICRO;
> > +		}
> > +		break;
> > +	case IIO_CHAN_INFO_CALIBSCALE:
> > +		if (chan->type == IIO_LIGHT) {
> > +			*val = chip->calibscale;
> > +			*val2 = chip->ucalibscale;
> > +			ret = IIO_VAL_INT_PLUS_MICRO;
> > +		}
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +
> > +	mutex_unlock(&chip->lock);
> > +	isl29033_runtime_pm_put(chip);
> > +
> > +	return ret;
> > +}
> > +
> > +#define ISL29033_LIGHT_CHANNEL {					\
> > +	.type = IIO_LIGHT,						\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |		\
> > +	BIT(IIO_CHAN_INFO_CALIBSCALE) |					\
> > +	BIT(IIO_CHAN_INFO_SCALE) |					\
> > +	BIT(IIO_CHAN_INFO_INT_TIME),					\
> > +}
> > +
> > +#define ISL29033_IR_CHANNEL {						\
> > +	.type = IIO_INTENSITY,						\
> > +	.modified = 1,							\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
> > +	.channel2 = IIO_MOD_LIGHT_IR,					\  
> 
> maybe move .channel2 closer to the .modified line for readability
> 
> > +}
> > +
> > +static const struct iio_chan_spec isl29033_channels[] = {
> > +	ISL29033_LIGHT_CHANNEL,
> > +	ISL29033_IR_CHANNEL,

With only two of these, why have the macros above? Just fill them here directly.

> > +};
> > +
> > +static IIO_DEVICE_ATTR(in_illuminance_integration_time_available, 0444,
> > +		isl29033_in_illuminance_integration_time_available, NULL, 0);
> > +static IIO_DEVICE_ATTR(in_illuminance_scale_available, 0444,
> > +		isl29033_in_illuminance_scale_available, NULL, 0);
> > +
> > +#define ISL29033_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
> > +
> > +static struct attribute *isl29033_attributes[] = {
> > +	ISL29033_DEV_ATTR(in_illuminance_scale_available),
> > +	ISL29033_DEV_ATTR(in_illuminance_integration_time_available),
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group isl29033_group = {
> > +	.attrs = isl29033_attributes,
> > +};
> > +
> > +static int isl29033_chip_init(struct isl29033_chip *chip)
> > +{
> > +	int ret;
> > +	struct device *dev = regmap_get_device(chip->regmap);
> > +
> > +	/*
> > +	 * Code added per Intersil Application Note 1534:
> > +	 *	 When VDD sinks to approximately 1.8V or below, some of
> > +	 * the part's registers may change their state. When VDD
> > +	 * recovers to 2.25V (or greater), the part may thus be in an
> > +	 * unknown mode of operation. The user can return the part to
> > +	 * a known mode of operation either by (a) setting VDD = 0V for
> > +	 * 1 second or more and then powering back up with a slew rate
> > +	 * of 0.5V/ms or greater, or (b) via I2C disable all ALS/PROX
> > +	 * conversions, clear the test registers, and then rewrite all
> > +	 * registers to the desired values.
> > +	 * ...
> > +	 * For ISL29033, etc.
> > +	 * 1. Write 0x00 to register 0x08 (TEST)
> > +	 * 2. Write 0x00 to register 0x00 (CMD1)
> > +	 * 3. Rewrite all registers to the desired values
> > +	 */
> > +	ret = regmap_write(chip->regmap, ISL29033_REG_TEST, 0x0);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to clear isl29033 TEST reg with err %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * See Intersil AN1534 comments above.
> > +	 * "Operating Mode" (COMMAND1) register is reprogrammed when
> > +	 * data is read from the device.
> > +	 */
> > +	ret = regmap_write(chip->regmap, ISL29033_REG_ADD_COMMAND1, 0);
> > +	if (ret < 0) {
> > +		dev_err(dev, "Failed to clear isl29033 CMD1 reg with err %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	usleep_range(1000, 2000);	/* per data sheet, page 10 */
> > +
> > +	/* Set defaults */
> > +	ret = isl29033_set_scale(chip,
> > +			isl29033_rext_normalize(chip, chip->scale.scale),
> > +			isl29033_rext_normalize2(chip, chip->scale.scale,
> > +					chip->scale.uscale));
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Init of isl29033 fails (scale) with err %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = isl29033_set_integration_time(chip,
> > +			isl29033_int_utimes[chip->int_time]
> > +			* chip->rext / ISL29033_REF_REXT);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Init of isl29033 fails (integration) with err %d\n",
> > +			ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = isl29033_set_mode(chip, chip->opmode);
> > +	if (ret)
> > +		dev_err(dev,
> > +			"Init of isl29033 fails (opmode) with err %d\n", ret);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct iio_info isl29033_info = {
> > +	.attrs = &isl29033_group,
> > +	.read_raw = isl29033_read_raw,
> > +	.write_raw = isl29033_write_raw,
> > +};
> > +
> > +static bool isl29033_is_volatile_reg(struct device *dev, unsigned int reg)
> > +{
> > +	switch (reg) {
> > +	case ISL29033_REG_ADD_DATA_LSB:
> > +	case ISL29033_REG_ADD_DATA_MSB:
> > +	case ISL29033_REG_ADD_COMMAND1:
> > +	case ISL29033_REG_ADD_COMMAND2:
> > +	case ISL29033_REG_TEST:
> > +		return true;
> > +	default:
> > +		return false;
> > +	}
> > +}
> > +
> > +static const struct regmap_config isl29033_regmap_config = {
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.volatile_reg = isl29033_is_volatile_reg,
> > +	.max_register = ISL29033_REG_TEST,
> > +	.num_reg_defaults_raw = ISL29033_REG_TEST + 1,
> > +	.cache_type = REGCACHE_RBTREE,
> > +};
> > +
> > +static const char *isl29033_match_acpi_device(struct device *dev, int *data)
> > +{
> > +	const struct acpi_device_id *id;
> > +
> > +	id = acpi_match_device(dev->driver->acpi_match_table, dev);  
> 
> delete newline (extreme nitpicking)
> > +
> > +	if (!id)
> > +		return NULL;
> > +
> > +	*data = (int)id->driver_data;
> > +
> > +	return dev_name(dev);
> > +}
> > +
> > +static int isl29033_probe(struct i2c_client *client,
> > +			  const struct i2c_device_id *id)
> > +{
> > +	struct isl29033_chip *chip;
> > +	struct iio_dev *indio_dev;
> > +	int ret;
> > +	const char *name = NULL;
> > +	int dev_id = 0;
> > +
> > +	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);
> > +
> > +	if (id) {
> > +		name = id->name;
> > +		dev_id = id->driver_data;
> > +	}
> > +
> > +	if (ACPI_HANDLE(&client->dev))
> > +		name = isl29033_match_acpi_device(&client->dev, &dev_id);
> > +
> > +	mutex_init(&chip->lock);
> > +
> > +	chip->calibscale = 1;
> > +	chip->ucalibscale = 0;
> > +	chip->int_time = 0;
> > +	chip->scale = isl29033_scales[chip->int_time][0];
> > +
> > +#ifdef CONFIG_OF
> > +	ret = device_property_read_u32(&client->dev, "isil,rext-kohms",
> > +					&chip->rext);
> > +	if (ret != 0)

if (!ret)

> > +		chip->rext =  ISL29033_REF_REXT;
> > +#else
> > +	chip->rext = ISL29033_REF_REXT;
> > +#endif /* CONFIG_OF */
> > +
> > +	chip->regmap = devm_regmap_init_i2c(client, &isl29033_regmap_config);  
> 
> delete newline (extreme nitpicking)
> > +
> > +	if (IS_ERR(chip->regmap)) {
> > +		ret = PTR_ERR(chip->regmap);
> > +		dev_err(&client->dev,
> > +			"regmap initialization fails with err %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = isl29033_chip_init(chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	indio_dev->info = &isl29033_info;
> > +	indio_dev->channels = isl29033_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(isl29033_channels);
> > +	indio_dev->name = name;
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	pm_runtime_enable(&client->dev);
> > +	pm_runtime_set_autosuspend_delay(&client->dev,
> > +			ISL29033_POWER_OFF_DELAY_MS);
> > +	pm_runtime_use_autosuspend(&client->dev);
> > +

one line is enough.

> > +
> > +	return iio_device_register(indio_dev);
> > +}
> > +
> > +static int isl29033_suspend(struct device *dev)
> > +{
> > +	struct isl29033_chip *chip = iio_priv(dev_get_drvdata(dev));
> > +	int ret;
> > +
> > +	mutex_lock(&chip->lock);
> > +
> > +	ret = isl29033_set_mode(chip, ISL29033_CMD1_OPMODE_POWER_DOWN);
> > +
> > +	mutex_unlock(&chip->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int __maybe_unused isl29033_resume(struct device *dev)
> > +{
> > +	struct isl29033_chip *chip = iio_priv(dev_get_drvdata(dev));
> > +	int ret;
> > +
> > +	mutex_lock(&chip->lock);
> > +
> > +	ret = isl29033_chip_init(chip);
> > +
> > +	mutex_unlock(&chip->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int isl29033_remove(struct i2c_client *client)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> > +
> > +	iio_device_unregister(indio_dev);
> > +
> > +	isl29033_suspend(&client->dev);
> > +
> > +	pm_runtime_disable(&client->dev);
> > +	pm_runtime_set_suspended(&client->dev);
> > +	pm_runtime_put_noidle(&client->dev);
> > +
> > +	devm_iio_device_free(&client->dev, indio_dev);  
> 
> no need to explicitly call devm_iio_device_free()
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops isl29033_dev_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +			pm_runtime_force_resume)
> > +	SET_RUNTIME_PM_OPS(isl29033_suspend, isl29033_resume, NULL)
> > +};
> > +
> > +static const struct acpi_device_id isl29033_acpi_match[] = {
> > +	{"ISL29033", 0},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(acpi, isl29033_acpi_match);
> > +
> > +static const struct i2c_device_id isl29033_id[] = {
> > +	{"isl29033", 0},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, isl29033_id);
> > +
> > +static const struct of_device_id isl29033_of_match[] = {
> > +	{ .compatible = "isil,isl29033", },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, isl29033_of_match);
> > +
> > +static struct i2c_driver isl29033_driver = {
> > +	.driver	 = {
> > +		.name = "isl29033",
> > +		.acpi_match_table = ACPI_PTR(isl29033_acpi_match),
> > +		.pm = &isl29033_dev_pm_ops,
> > +		.of_match_table = isl29033_of_match,
> > +	},
> > +	.probe	 = isl29033_probe,
> > +	.remove  = isl29033_remove,

Nitpick, don't bother with trying to align - it just tends to end up inconsistent
anyway like it has done here. Also adds noise when it inevitably has to change
to allow for some new code.

> > +	.id_table = isl29033_id,
> > +};
> > +module_i2c_driver(isl29033_driver);
> > +
> > +MODULE_DESCRIPTION("ISL29033 Ambient Light Sensor driver");
> > +MODULE_LICENSE("GPL");
> >   
>