On Wed, 13 Jun 2018 21:12:36 +0200 (CEST) Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx> 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@xxxxxxxx> > > > > 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@xxxxxxxx> > > --- > > 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@xxxxxxxx> > > + */ > > + > > +#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"); > > > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
