diff mbox

iio: light: Add driver for IDT ZOPT2201 ambient light and UVB sensor

Message ID 20171010142959.4723-1-pmeerw@pmeerw.net (mailing list archive)
State New, archived
Headers show

Commit Message

Peter Meerwald-Stadler Oct. 10, 2017, 2:29 p.m. UTC
Driver for 20-bit ALS and UV B sensor with I2C interface exposing
the following API:
  in_uvindex_input
  in_illuminance_raw
  in_illuminance_scale
  in_illuminance_scale_available
  in_intensity_uv_raw
  in_intensity_uv_scale
  in_intensity_uv_scale_available
  integration_time
  integration_time_available

Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
---
 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/zopt2201.c | 563 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 574 insertions(+)
 create mode 100644 drivers/iio/light/zopt2201.c

Comments

Jonathan Cameron Oct. 14, 2017, 6:06 p.m. UTC | #1
On Tue, 10 Oct 2017 16:29:59 +0200
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:

> Driver for 20-bit ALS and UV B sensor with I2C interface exposing
> the following API:
>   in_uvindex_input
>   in_illuminance_raw
>   in_illuminance_scale
>   in_illuminance_scale_available
>   in_intensity_uv_raw
>   in_intensity_uv_scale
>   in_intensity_uv_scale_available
>   integration_time
>   integration_time_available

Nice to have this list.

> 
> Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>

Another very nice driver.  A couple of refactoring suggestions inline
that I think would slightly (and it is slightly!) improve readability.
I think we need some additional docs for the uv intensity channel as I can't
find any documented units for that - so far they have been fairly randomly
filtered and no docs have existed to map these to units.

As such I'm thinking w/m^2 would be a nicer base unit.  If I'm missing
a prior example of this then let me know - I haven't really looked :)

Jonathan

> ---
>  drivers/iio/light/Kconfig    |  10 +
>  drivers/iio/light/Makefile   |   1 +
>  drivers/iio/light/zopt2201.c | 563 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 574 insertions(+)
>  create mode 100644 drivers/iio/light/zopt2201.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 2356ed9285df..6a5835fab32e 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -425,4 +425,14 @@ config VL6180
>  	 To compile this driver as a module, choose M here: the
>  	 module will be called vl6180.
>  
> +config ZOPT2201
> +	tristate "ZOPT2201 ALS and UV B sensor"
> +	depends on I2C
> +	help
> +	 Say Y here if you want to build a driver for the IDT
> +	 ZOPT2201 ambient light and UV B sensor.
> +
> +	 To compile this driver as a module, choose M here: the
> +	 module will be called zopt2201.
> +
>  endmenu
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index fa32fa459e2e..d99abdf6b746 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -40,3 +40,4 @@ obj-$(CONFIG_US5182D)		+= us5182d.o
>  obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
>  obj-$(CONFIG_VEML6070)		+= veml6070.o
>  obj-$(CONFIG_VL6180)		+= vl6180.o
> +obj-$(CONFIG_ZOPT2201)		+= zopt2201.o
> diff --git a/drivers/iio/light/zopt2201.c b/drivers/iio/light/zopt2201.c
> new file mode 100644
> index 000000000000..07b299642ac3
> --- /dev/null
> +++ b/drivers/iio/light/zopt2201.c
> @@ -0,0 +1,563 @@
> +/*
> + * zopt2201.c - Support for IDT ZOPT2201 ambient light and UV B sensor
> + *
> + * Copyright 2017 Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License.  See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * Datasheet: https://www.idt.com/document/dst/zopt2201-datasheet
> + * 7-bit I2C slave addresses 0x53 (default) or 0x52 (programmed)
> + *
> + * TODO: interrupt support, ALS/UVB raw mode
> + */
> +
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/err.h>
> +#include <linux/delay.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +
> +#define ZOPT2201_DRV_NAME "zopt2201"
> +
> +/* Registers */
> +#define ZOPT2201_MAIN_CTRL		0x00
> +#define ZOPT2201_LS_MEAS_RATE		0x04
> +#define ZOPT2201_LS_GAIN		0x05
> +#define ZOPT2201_PART_ID		0x06
> +#define ZOPT2201_MAIN_STATUS		0x07
> +#define ZOPT2201_ALS_DATA		0x0d /* LSB first, 13 to 20 bits */
> +#define ZOPT2201_UVB_DATA		0x10 /* LSB first, 13 to 20 bits */
> +#define ZOPT2201_UV_COMP_DATA		0x13 /* LSB first, 13 to 20 bits */
> +#define ZOPT2201_COMP_DATA		0x16 /* LSB first, 13 to 20 bits */
> +#define ZOPT2201_INT_CFG		0x19
> +#define ZOPT2201_INT_PST		0x1a
> +
> +#define ZOPT2201_MAIN_CTRL_LS_MODE	BIT(3) /* 0 .. ALS, 1 .. UV B */
> +#define ZOPT2201_MAIN_CTRL_LS_EN	BIT(1)
> +
> +/* Values for ZOPT2201_LS_MEAS_RATE resolution / bit width */
> +#define ZOPT2201_MEAS_RES_20BIT		0 /* takes 400 ms */
> +#define ZOPT2201_MEAS_RES_19BIT		1 /* takes 200 ms */
> +#define ZOPT2201_MEAS_RES_18BIT		2 /* takes 100 ms, default */
> +#define ZOPT2201_MEAS_RES_17BIT		3 /* takes 50 ms */
> +#define ZOPT2201_MEAS_RES_16BIT		4 /* takes 25 ms */
> +#define ZOPT2201_MEAS_RES_13BIT		5 /* takes 3.125 ms */
> +#define ZOPT2201_MEAS_RES_SHIFT		4
> +
> +/* Values for ZOPT2201_LS_MEAS_RATE measurement rate */
> +#define ZOPT2201_MEAS_FREQ_25MS		0
> +#define ZOPT2201_MEAS_FREQ_50MS		1
> +#define ZOPT2201_MEAS_FREQ_100MS	2 /* default */
> +#define ZOPT2201_MEAS_FREQ_200MS	3
> +#define ZOPT2201_MEAS_FREQ_500MS	4
> +#define ZOPT2201_MEAS_FREQ_1000MS	5
> +#define ZOPT2201_MEAS_FREQ_2000MS	6
> +
> +/* Values for ZOPT2201_LS_GAIN */
> +#define ZOPT2201_LS_GAIN_1		0
> +#define ZOPT2201_LS_GAIN_3		1
> +#define ZOPT2201_LS_GAIN_6		2
> +#define ZOPT2201_LS_GAIN_9		3
> +#define ZOPT2201_LS_GAIN_18		4
> +
> +/* Values for ZOPT2201_MAIN_STATUS */
> +#define ZOPT2201_MAIN_STATUS_POWERON	BIT(5)
> +#define ZOPT2201_MAIN_STATUS_INT	BIT(4)
> +#define ZOPT2201_MAIN_STATUS_DRDY	BIT(3)
> +
> +#define ZOPT2201_PART_NUMBER		0xb2
> +
> +struct zopt2201_data {
> +	struct i2c_client *client;
> +	struct mutex lock;
> +	u8 gain;
> +	u8 res;
> +	u8 rate;
> +};
> +
> +enum zopt2201_mode {
> +	ZOPT2201_MODE_ALS,
> +	ZOPT2201_MODE_UVB,
> +};
> +
> +static const struct {
> +	unsigned int gain; /* gain factor */
> +	unsigned int scale; /* micro lux per count */
> +} zopt2201_gain_als[] = {
> +	{  1, 19200000 },
> +	{  3,  6400000 },
> +	{  6,  3200000 },
> +	{  9,  2133333 },
> +	{ 18,  1066666 },
> +};
> +
> +static const struct {
> +	unsigned int gain; /* gain factor */
> +	unsigned int scale; /* micro W/cm2 per count */

I think docs need to be updated to cover having units for
uv. Certainly good to assign units if we can to intensity
channels.

Hmm. micro W / cm2 so 0.001W/((0.01*0.01)m^2)
= 10W/m^2

If we are going to define a unit I'd prefer we ended up with
w/m^2 and hence SI unit.

> +} zopt2201_gain_uvb[] = {
> +	{  1, 46080000 },
> +	{  3, 15360000 },
> +	{  6,  7680000 },
> +	{  9,  5120000 },
> +	{ 18,  2560000 },
> +};
> +
> +static const struct {
> +	unsigned int bits; /* sensor resolution in bits */
> +	unsigned long us; /* measurement time in micro seconds */
> +} zopt2201_resolution[] = {
> +	{ 20, 400000 },
> +	{ 19, 200000 },
> +	{ 18, 100000 },
> +	{ 17,  50000 },
> +	{ 16,  25000 },
> +	{ 13,   3125 },
> +};
> +
> +static const struct {
> +	unsigned int scale, uscale; /* scale factor as integer + micro */
> +	u8 gain; /* gain register value */
> +	u8 res; /* resolution register value */
> +} zopt2201_scale_als[] = {
> +	{ 19, 200000, 0, 5 },
> +	{  6, 400000, 1, 5 },
> +	{  3, 200000, 2, 5 },
> +	{  2, 400000, 0, 4 },
> +	{  2, 133333, 3, 5 },
> +	{  1, 200000, 0, 3 },
> +	{  1,  66666, 4, 5 },
> +	{  0, 800000, 1, 4 },
> +	{  0, 600000, 0, 2 },
> +	{  0, 400000, 2, 4 },
> +	{  0, 300000, 0, 1 },
> +	{  0, 266666, 3, 4 },
> +	{  0, 200000, 2, 3 },
> +	{  0, 150000, 0, 0 },
> +	{  0, 133333, 4, 4 },
> +	{  0, 100000, 2, 2 },
> +	{  0,  66666, 4, 3 },
> +	{  0,  50000, 2, 1 },
> +	{  0,  33333, 4, 2 },
> +	{  0,  25000, 2, 0 },
> +	{  0,  16666, 4, 1 },
> +	{  0,   8333, 4, 0 },
> +};
> +
> +static const struct {
> +	unsigned int scale, uscale; /* scale factor as integer + micro */
> +	u8 gain; /* gain register value */
> +	u8 res; /* resolution register value */
> +} zopt2201_scale_uvb[] = {
> +	{ 46,  80000, 0, 5 },
> +	{ 15, 360000, 1, 5 },
> +	{  7, 680000, 2, 5 },
> +	{  5, 760000, 0, 4 },
> +	{  5, 120000, 3, 5 },
> +	{  2, 880000, 0, 3 },
> +	{  2, 560000, 4, 5 },
> +	{  1, 920000, 1, 4 },
> +	{  1, 440000, 0, 2 },
> +	{  0, 960000, 2, 4 },
> +	{  0, 720000, 0, 1 },
> +	{  0, 640000, 3, 4 },
> +	{  0, 480000, 2, 3 },
> +	{  0, 360000, 0, 0 },
> +	{  0, 320000, 4, 4 },
> +	{  0, 240000, 2, 2 },
> +	{  0, 160000, 4, 3 },
> +	{  0, 120000, 2, 1 },
> +	{  0,  80000, 4, 2 },
> +	{  0,  60000, 2, 0 },
> +	{  0,  40000, 4, 1 },
> +	{  0,  20000, 4, 0 },
> +};
> +
> +static int zopt2201_enable_mode(struct zopt2201_data *data,
> +				enum zopt2201_mode mode)
> +{
> +	u8 out = ZOPT2201_MAIN_CTRL_LS_EN;
> +
> +	if (mode == ZOPT2201_MODE_UVB)
> +		out |= ZOPT2201_MAIN_CTRL_LS_MODE;
> +
> +	return i2c_smbus_write_byte_data(data->client, ZOPT2201_MAIN_CTRL, out);
> +}
> +
> +static int zopt2201_read(struct zopt2201_data *data, enum zopt2201_mode mode)
> +{
> +	struct i2c_client *client = data->client;
> +	int tries = 10;
> +	u8 buf[3];
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = zopt2201_enable_mode(data, mode);
> +	if (ret < 0)
> +		goto fail;
> +
> +	while (tries--) {
> +		unsigned long t = zopt2201_resolution[data->res].us;
> +
> +		if (t <= 20000)
> +			usleep_range(t, t + 1000);
> +		else
> +			msleep(t / 1000);
> +		ret = i2c_smbus_read_byte_data(client, ZOPT2201_MAIN_STATUS);
> +		if (ret < 0)
> +			goto fail;
> +		if (ret & ZOPT2201_MAIN_STATUS_DRDY)
> +			break;
> +	}
> +
> +	if (tries < 0) {
> +		ret = -ETIMEDOUT;
> +		goto fail;
> +	}
> +
> +	ret = i2c_smbus_read_i2c_block_data(client, mode == ZOPT2201_MODE_UVB ?
> +		ZOPT2201_UVB_DATA : ZOPT2201_ALS_DATA, sizeof(buf), buf);
Having a ternary buried in there is a little hard to read.  Please pull it
out as a local variable assignment.

> +	if (ret < 0)
> +		goto fail;
> +
> +	ret = i2c_smbus_write_byte_data(client, ZOPT2201_MAIN_CTRL, 0x00);
> +	if (ret < 0)
> +		goto fail;
> +	mutex_unlock(&data->lock);
> +
> +	return (buf[2] << 16) | (buf[1] << 8) | buf[0];
> +
> +fail:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static const struct iio_chan_spec zopt2201_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.address = ZOPT2201_MODE_ALS,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_UV,
> +		.address = ZOPT2201_MODE_UVB,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +	},
> +	{
> +		.type = IIO_UVINDEX,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +	},
> +};
> +
> +static int zopt2201_read_raw(struct iio_dev *indio_dev,
> +				struct iio_chan_spec const *chan,
> +				int *val, int *val2, long mask)
> +{
> +	struct zopt2201_data *data = iio_priv(indio_dev);
> +	u64 tmp;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = zopt2201_read(data, chan->address);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = zopt2201_read(data, ZOPT2201_MODE_UVB);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret * 18 *
> +			(1 << (20 - zopt2201_resolution[data->res].bits)) /
> +			zopt2201_gain_uvb[data->gain].gain;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->address) {
> +		case ZOPT2201_MODE_ALS:
> +			*val = zopt2201_gain_als[data->gain].scale;

This is documented above as micro lux, but illuminance ABI is in
lux.

> +			break;
> +		case ZOPT2201_MODE_UVB:
> +			*val = zopt2201_gain_uvb[data->gain].scale;
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +		*val2 = 1000000;
> +		*val2 *= (1 << (zopt2201_resolution[data->res].bits - 13));
> +		tmp = div_s64(*val * 1000000ULL, *val2);
> +		*val = div_s64_rem(tmp, 1000000, val2);
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*val = 0;
> +		*val2 = zopt2201_resolution[data->res].us;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int zopt2201_set_resolution(struct zopt2201_data *data, u8 res)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, ZOPT2201_LS_MEAS_RATE,
> +					(res << ZOPT2201_MEAS_RES_SHIFT) |
> +					data->rate);
> +	if (ret >= 0)
> +		data->res = res;

Same as the case below - prefer the error case indented.  Just that
tiny bit nicer to read and means that when a reviewer scan's past it
they won't stop to check what is going on...

> +
> +	return ret;
> +}
> +
> +static int zopt2201_write_resolution(struct zopt2201_data *data,
> +				     int val, int val2)
> +{
> +	int i, ret;
> +
> +	if (val != 0)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(zopt2201_resolution); i++)
> +		if (val2 == zopt2201_resolution[i].us) {
> +			mutex_lock(&data->lock);
> +			ret = zopt2201_set_resolution(data, i);
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +
> +	return -EINVAL;
> +}
> +
> +static int zopt2201_set_gain(struct zopt2201_data *data, u8 gain)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_write_byte_data(data->client, ZOPT2201_LS_GAIN, gain);
> +	if (ret >= 0)
> +		data->gain = gain;

I'm a fuss pot about this, but I always like the indented one to be the
error path.  It adds a line of code, but it's easier to follow

	if (ret < 0)
		return ret;

	data->gain = gain;

	return 0;
> +
> +	return ret;
> +}
> +
> +static int zopt2201_write_scale_als(struct zopt2201_data *data,
> +				     int val, int val2)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
> +		if (val == zopt2201_scale_als[i].scale &&
> +		    val2 == zopt2201_scale_als[i].uscale) {
> +
This is very deeply indented and it hurting readability.
Perhaps a little utility function to handle the actual
configuring might help?

static int zopt2201_write_scale_als_by_idx(struct zopt2201_data *data, int index)
{
	int ret;

	mutex_lock(&data->lock);
	ret = zopt2201_set_resolution(data, zopt2201_scale_als[i].res);
	if (ret < 0)
		goto unlock;

	ret = zopt2201_set_gain(data, zopt2201_scale_als[i].gain);

unlock:
	mutex_unlock(&data->lock);
	return ret;	
}

static int zopt2201_write_scale_als(struct zopt2201_data *data,
				     int val, int val2)
{
	int i, ret;

	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
		if (val == zopt2201_scale_als[i].scale &&
		    val2 == zopt2201_scale_als[i].uscale) {
			return zopt2201_write_scale_als_by_idx(data, i);
	return -EINVAL;
}

Plus equivalent for the next one below.

> +			mutex_lock(&data->lock);
> +			ret = zopt2201_set_resolution(data,
> +				zopt2201_scale_als[i].res);
> +			if (ret < 0)
> +				goto fail;
> +			ret = zopt2201_set_gain(data,
> +				zopt2201_scale_als[i].gain);
> +
> +fail:
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +
> +	return -EINVAL;
> +}
> +
> +static int zopt2201_write_scale_uvb(struct zopt2201_data *data,
> +				     int val, int val2)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_uvb); i++)
> +		if (val == zopt2201_scale_uvb[i].scale &&
> +		    val2 == zopt2201_scale_uvb[i].uscale) {
> +			mutex_lock(&data->lock);
> +			ret = zopt2201_set_resolution(data,
> +				zopt2201_scale_uvb[i].res);
> +			if (ret < 0)
> +				goto fail;
> +			ret = zopt2201_set_gain(data,
> +				zopt2201_scale_uvb[i].gain);
> +
> +fail:
> +			mutex_unlock(&data->lock);
> +			return ret;
> +		}
> +
> +	return -EINVAL;
> +}
> +
> +static int zopt2201_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct zopt2201_data *data = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return zopt2201_write_resolution(data, val, val2);
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->address) {
> +		case ZOPT2201_MODE_ALS:
> +			return zopt2201_write_scale_als(data, val, val2);
> +		case ZOPT2201_MODE_UVB:
> +			return zopt2201_write_scale_uvb(data, val, val2);
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static ssize_t zopt2201_show_int_time_available(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	size_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(zopt2201_resolution); i++)
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06lu ",
> +				 zopt2201_resolution[i].us);
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static IIO_DEV_ATTR_INT_TIME_AVAIL(zopt2201_show_int_time_available);
> +

One blank line is enough...

> +
> +static ssize_t zopt2201_show_als_scale_avail(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	ssize_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
> +				 zopt2201_scale_als[i].scale,
> +				 zopt2201_scale_als[i].uscale);
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static ssize_t zopt2201_show_uvb_scale_avail(struct device *dev,
> +					     struct device_attribute *attr,
> +					     char *buf)
> +{
> +	ssize_t len = 0;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_uvb); i++)
> +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
> +				 zopt2201_scale_uvb[i].scale,
> +				 zopt2201_scale_uvb[i].uscale);
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
> +static IIO_DEVICE_ATTR(in_illuminance_scale_available, 0444,
> +		       zopt2201_show_als_scale_avail, NULL, 0);
> +static IIO_DEVICE_ATTR(in_intensity_uv_scale_available, 0444,
> +		       zopt2201_show_uvb_scale_avail, NULL, 0);
> +
> +static struct attribute *zopt2201_attributes[] = {
> +	&iio_dev_attr_integration_time_available.dev_attr.attr,
> +	&iio_dev_attr_in_illuminance_scale_available.dev_attr.attr,
> +	&iio_dev_attr_in_intensity_uv_scale_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group zopt2201_attribute_group = {
> +	.attrs = zopt2201_attributes,
> +};
> +

Only 1 blank line.

> +
> +static const struct iio_info zopt2201_info = {
> +	.read_raw = zopt2201_read_raw,
> +	.write_raw = zopt2201_write_raw,
> +	.attrs = &zopt2201_attribute_group,
> +};
> +
> +static int zopt2201_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct zopt2201_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter,
> +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> +		return -EOPNOTSUPP;
> +
> +	ret = i2c_smbus_read_byte_data(client, ZOPT2201_PART_ID);
> +	if (ret < 0)
> +		return ret;
> +	if (ret != ZOPT2201_PART_NUMBER)
> +		return -ENODEV;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +	mutex_init(&data->lock);
> +
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &zopt2201_info;
> +	indio_dev->channels = zopt2201_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(zopt2201_channels);
> +	indio_dev->name = ZOPT2201_DRV_NAME;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	data->rate = ZOPT2201_MEAS_FREQ_100MS;
> +	ret = zopt2201_set_resolution(data, ZOPT2201_MEAS_RES_18BIT);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = zopt2201_set_gain(data, ZOPT2201_LS_GAIN_3);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id zopt2201_id[] = {
> +	{ "zopt2201", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, zopt2201_id);

Hmm. I wonder if we should be adding device tree bindings for
pretty much all drivers now.  There is on going effort to
drop the compatibility code that loads them based on the
i2c device table.  Anyhow, can be a follow up patch so
not a problem right now.

> +
> +static struct i2c_driver zopt2201_driver = {
> +	.driver = {
> +		.name   = ZOPT2201_DRV_NAME,
> +	},
> +	.probe  = zopt2201_probe,
> +	.id_table = zopt2201_id,
> +};
> +
> +module_i2c_driver(zopt2201_driver);
> +
> +MODULE_AUTHOR("Peter Meerwald-Stadler <pmeerw@pmeerw.net>");
> +MODULE_DESCRIPTION("IDT ZOPT2201 ambient light and UV B sensor driver");
> +MODULE_LICENSE("GPL");

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Meerwald-Stadler Nov. 5, 2017, 10:48 p.m. UTC | #2
Hello Jonathan,

> Another very nice driver.  A couple of refactoring suggestions inline
> that I think would slightly (and it is slightly!) improve readability.
> I think we need some additional docs for the uv intensity channel as I can't
> find any documented units for that - so far they have been fairly randomly
> filtered and no docs have existed to map these to units.

UV intensity is already documented (around line 1288) as
/sys/.../iio:deviceX/in_intensityY_uv_raw
however
/sys/.../iio:deviceX/in_intensity_raw and 
/sys/.../iio:deviceX/in_intensity_uv_raw are missing, should these be 
added for completeness??

> As such I'm thinking w/m^2 would be a nicer base unit.  If I'm missing
> a prior example of this then let me know - I haven't really looked :)

intensity_uv is documented as unit-less currently;
not sure if we can suggest a unit after-the-fact; drivers may use _SCALE 
to set a gain, and not care about the unit

other chips such as the AS7262 are using micro W/cm2;
our channel modifiers (both, clear, uv, red, green blue) may not be 
sufficient for spectral sensors (6 visible channels at 450, 500, 550, 570, 
600, 650 -- AS7262; NIR channels at 610, 680, 730, 760, 810, 860 -- 
AS7263)

will post a v2 with your suggestions shortly, thanks for the review!
comments to your suggestions below...

regards, p.

> > ---
> >  drivers/iio/light/Kconfig    |  10 +
> >  drivers/iio/light/Makefile   |   1 +
> >  drivers/iio/light/zopt2201.c | 563 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 574 insertions(+)
> >  create mode 100644 drivers/iio/light/zopt2201.c
> > 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 2356ed9285df..6a5835fab32e 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -425,4 +425,14 @@ config VL6180
> >  	 To compile this driver as a module, choose M here: the
> >  	 module will be called vl6180.
> >  
> > +config ZOPT2201
> > +	tristate "ZOPT2201 ALS and UV B sensor"
> > +	depends on I2C
> > +	help
> > +	 Say Y here if you want to build a driver for the IDT
> > +	 ZOPT2201 ambient light and UV B sensor.
> > +
> > +	 To compile this driver as a module, choose M here: the
> > +	 module will be called zopt2201.
> > +
> >  endmenu
> > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > index fa32fa459e2e..d99abdf6b746 100644
> > --- a/drivers/iio/light/Makefile
> > +++ b/drivers/iio/light/Makefile
> > @@ -40,3 +40,4 @@ obj-$(CONFIG_US5182D)		+= us5182d.o
> >  obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
> >  obj-$(CONFIG_VEML6070)		+= veml6070.o
> >  obj-$(CONFIG_VL6180)		+= vl6180.o
> > +obj-$(CONFIG_ZOPT2201)		+= zopt2201.o
> > diff --git a/drivers/iio/light/zopt2201.c b/drivers/iio/light/zopt2201.c
> > new file mode 100644
> > index 000000000000..07b299642ac3
> > --- /dev/null
> > +++ b/drivers/iio/light/zopt2201.c
> > @@ -0,0 +1,563 @@
> > +/*
> > + * zopt2201.c - Support for IDT ZOPT2201 ambient light and UV B sensor
> > + *
> > + * Copyright 2017 Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> > + *
> > + * This file is subject to the terms and conditions of version 2 of
> > + * the GNU General Public License.  See the file COPYING in the main
> > + * directory of this archive for more details.
> > + *
> > + * Datasheet: https://www.idt.com/document/dst/zopt2201-datasheet
> > + * 7-bit I2C slave addresses 0x53 (default) or 0x52 (programmed)
> > + *
> > + * TODO: interrupt support, ALS/UVB raw mode
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mutex.h>
> > +#include <linux/err.h>
> > +#include <linux/delay.h>
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +
> > +#define ZOPT2201_DRV_NAME "zopt2201"
> > +
> > +/* Registers */
> > +#define ZOPT2201_MAIN_CTRL		0x00
> > +#define ZOPT2201_LS_MEAS_RATE		0x04
> > +#define ZOPT2201_LS_GAIN		0x05
> > +#define ZOPT2201_PART_ID		0x06
> > +#define ZOPT2201_MAIN_STATUS		0x07
> > +#define ZOPT2201_ALS_DATA		0x0d /* LSB first, 13 to 20 bits */
> > +#define ZOPT2201_UVB_DATA		0x10 /* LSB first, 13 to 20 bits */
> > +#define ZOPT2201_UV_COMP_DATA		0x13 /* LSB first, 13 to 20 bits */
> > +#define ZOPT2201_COMP_DATA		0x16 /* LSB first, 13 to 20 bits */
> > +#define ZOPT2201_INT_CFG		0x19
> > +#define ZOPT2201_INT_PST		0x1a
> > +
> > +#define ZOPT2201_MAIN_CTRL_LS_MODE	BIT(3) /* 0 .. ALS, 1 .. UV B */
> > +#define ZOPT2201_MAIN_CTRL_LS_EN	BIT(1)
> > +
> > +/* Values for ZOPT2201_LS_MEAS_RATE resolution / bit width */
> > +#define ZOPT2201_MEAS_RES_20BIT		0 /* takes 400 ms */
> > +#define ZOPT2201_MEAS_RES_19BIT		1 /* takes 200 ms */
> > +#define ZOPT2201_MEAS_RES_18BIT		2 /* takes 100 ms, default */
> > +#define ZOPT2201_MEAS_RES_17BIT		3 /* takes 50 ms */
> > +#define ZOPT2201_MEAS_RES_16BIT		4 /* takes 25 ms */
> > +#define ZOPT2201_MEAS_RES_13BIT		5 /* takes 3.125 ms */
> > +#define ZOPT2201_MEAS_RES_SHIFT		4
> > +
> > +/* Values for ZOPT2201_LS_MEAS_RATE measurement rate */
> > +#define ZOPT2201_MEAS_FREQ_25MS		0
> > +#define ZOPT2201_MEAS_FREQ_50MS		1
> > +#define ZOPT2201_MEAS_FREQ_100MS	2 /* default */
> > +#define ZOPT2201_MEAS_FREQ_200MS	3
> > +#define ZOPT2201_MEAS_FREQ_500MS	4
> > +#define ZOPT2201_MEAS_FREQ_1000MS	5
> > +#define ZOPT2201_MEAS_FREQ_2000MS	6
> > +
> > +/* Values for ZOPT2201_LS_GAIN */
> > +#define ZOPT2201_LS_GAIN_1		0
> > +#define ZOPT2201_LS_GAIN_3		1
> > +#define ZOPT2201_LS_GAIN_6		2
> > +#define ZOPT2201_LS_GAIN_9		3
> > +#define ZOPT2201_LS_GAIN_18		4
> > +
> > +/* Values for ZOPT2201_MAIN_STATUS */
> > +#define ZOPT2201_MAIN_STATUS_POWERON	BIT(5)
> > +#define ZOPT2201_MAIN_STATUS_INT	BIT(4)
> > +#define ZOPT2201_MAIN_STATUS_DRDY	BIT(3)
> > +
> > +#define ZOPT2201_PART_NUMBER		0xb2
> > +
> > +struct zopt2201_data {
> > +	struct i2c_client *client;
> > +	struct mutex lock;
> > +	u8 gain;
> > +	u8 res;
> > +	u8 rate;
> > +};
> > +
> > +enum zopt2201_mode {
> > +	ZOPT2201_MODE_ALS,
> > +	ZOPT2201_MODE_UVB,
> > +};
> > +
> > +static const struct {
> > +	unsigned int gain; /* gain factor */
> > +	unsigned int scale; /* micro lux per count */
> > +} zopt2201_gain_als[] = {
> > +	{  1, 19200000 },
> > +	{  3,  6400000 },
> > +	{  6,  3200000 },
> > +	{  9,  2133333 },
> > +	{ 18,  1066666 },
> > +};
> > +
> > +static const struct {
> > +	unsigned int gain; /* gain factor */
> > +	unsigned int scale; /* micro W/cm2 per count */
> 
> I think docs need to be updated to cover having units for
> uv. Certainly good to assign units if we can to intensity
> channels.
> 
> Hmm. micro W / cm2 so 0.001W/((0.01*0.01)m^2)

micro W/cm2 so 0.000001W/cm2 so 0.01W/m^2 ?

> If we are going to define a unit I'd prefer we ended up with
> w/m^2 and hence SI unit.

I have changed the scale factor so that the driver now reports W/m^2;
the comment above ("micro W/cm2 per count") was incorrect, in fact the 
values related to pico W/cm2
 
> > +} zopt2201_gain_uvb[] = {
> > +	{  1, 46080000 },
> > +	{  3, 15360000 },
> > +	{  6,  7680000 },
> > +	{  9,  5120000 },
> > +	{ 18,  2560000 },
> > +};
> > +
> > +static const struct {
> > +	unsigned int bits; /* sensor resolution in bits */
> > +	unsigned long us; /* measurement time in micro seconds */
> > +} zopt2201_resolution[] = {
> > +	{ 20, 400000 },
> > +	{ 19, 200000 },
> > +	{ 18, 100000 },
> > +	{ 17,  50000 },
> > +	{ 16,  25000 },
> > +	{ 13,   3125 },
> > +};
> > +
> > +static const struct {
> > +	unsigned int scale, uscale; /* scale factor as integer + micro */
> > +	u8 gain; /* gain register value */
> > +	u8 res; /* resolution register value */
> > +} zopt2201_scale_als[] = {
> > +	{ 19, 200000, 0, 5 },
> > +	{  6, 400000, 1, 5 },
> > +	{  3, 200000, 2, 5 },
> > +	{  2, 400000, 0, 4 },
> > +	{  2, 133333, 3, 5 },
> > +	{  1, 200000, 0, 3 },
> > +	{  1,  66666, 4, 5 },
> > +	{  0, 800000, 1, 4 },
> > +	{  0, 600000, 0, 2 },
> > +	{  0, 400000, 2, 4 },
> > +	{  0, 300000, 0, 1 },
> > +	{  0, 266666, 3, 4 },
> > +	{  0, 200000, 2, 3 },
> > +	{  0, 150000, 0, 0 },
> > +	{  0, 133333, 4, 4 },
> > +	{  0, 100000, 2, 2 },
> > +	{  0,  66666, 4, 3 },
> > +	{  0,  50000, 2, 1 },
> > +	{  0,  33333, 4, 2 },
> > +	{  0,  25000, 2, 0 },
> > +	{  0,  16666, 4, 1 },
> > +	{  0,   8333, 4, 0 },
> > +};
> > +
> > +static const struct {
> > +	unsigned int scale, uscale; /* scale factor as integer + micro */
> > +	u8 gain; /* gain register value */
> > +	u8 res; /* resolution register value */
> > +} zopt2201_scale_uvb[] = {
> > +	{ 46,  80000, 0, 5 },
> > +	{ 15, 360000, 1, 5 },
> > +	{  7, 680000, 2, 5 },
> > +	{  5, 760000, 0, 4 },
> > +	{  5, 120000, 3, 5 },
> > +	{  2, 880000, 0, 3 },
> > +	{  2, 560000, 4, 5 },
> > +	{  1, 920000, 1, 4 },
> > +	{  1, 440000, 0, 2 },
> > +	{  0, 960000, 2, 4 },
> > +	{  0, 720000, 0, 1 },
> > +	{  0, 640000, 3, 4 },
> > +	{  0, 480000, 2, 3 },
> > +	{  0, 360000, 0, 0 },
> > +	{  0, 320000, 4, 4 },
> > +	{  0, 240000, 2, 2 },
> > +	{  0, 160000, 4, 3 },
> > +	{  0, 120000, 2, 1 },
> > +	{  0,  80000, 4, 2 },
> > +	{  0,  60000, 2, 0 },
> > +	{  0,  40000, 4, 1 },
> > +	{  0,  20000, 4, 0 },
> > +};
> > +
> > +static int zopt2201_enable_mode(struct zopt2201_data *data,
> > +				enum zopt2201_mode mode)
> > +{
> > +	u8 out = ZOPT2201_MAIN_CTRL_LS_EN;
> > +
> > +	if (mode == ZOPT2201_MODE_UVB)
> > +		out |= ZOPT2201_MAIN_CTRL_LS_MODE;
> > +
> > +	return i2c_smbus_write_byte_data(data->client, ZOPT2201_MAIN_CTRL, out);
> > +}
> > +
> > +static int zopt2201_read(struct zopt2201_data *data, enum zopt2201_mode mode)
> > +{
> > +	struct i2c_client *client = data->client;
> > +	int tries = 10;
> > +	u8 buf[3];
> > +	int ret;
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = zopt2201_enable_mode(data, mode);
> > +	if (ret < 0)
> > +		goto fail;
> > +
> > +	while (tries--) {
> > +		unsigned long t = zopt2201_resolution[data->res].us;
> > +
> > +		if (t <= 20000)
> > +			usleep_range(t, t + 1000);
> > +		else
> > +			msleep(t / 1000);
> > +		ret = i2c_smbus_read_byte_data(client, ZOPT2201_MAIN_STATUS);
> > +		if (ret < 0)
> > +			goto fail;
> > +		if (ret & ZOPT2201_MAIN_STATUS_DRDY)
> > +			break;
> > +	}
> > +
> > +	if (tries < 0) {
> > +		ret = -ETIMEDOUT;
> > +		goto fail;
> > +	}
> > +
> > +	ret = i2c_smbus_read_i2c_block_data(client, mode == ZOPT2201_MODE_UVB ?
> > +		ZOPT2201_UVB_DATA : ZOPT2201_ALS_DATA, sizeof(buf), buf);
> Having a ternary buried in there is a little hard to read.  Please pull it
> out as a local variable assignment.

OK, actually I've dropped the mode enum and will use the corresponding 
register address directly (saves some lines)
 
> > +	if (ret < 0)
> > +		goto fail;
> > +
> > +	ret = i2c_smbus_write_byte_data(client, ZOPT2201_MAIN_CTRL, 0x00);
> > +	if (ret < 0)
> > +		goto fail;
> > +	mutex_unlock(&data->lock);
> > +
> > +	return (buf[2] << 16) | (buf[1] << 8) | buf[0];
> > +
> > +fail:
> > +	mutex_unlock(&data->lock);
> > +	return ret;
> > +}
> > +
> > +static const struct iio_chan_spec zopt2201_channels[] = {
> > +	{
> > +		.type = IIO_LIGHT,
> > +		.address = ZOPT2201_MODE_ALS,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> > +	},
> > +	{
> > +		.type = IIO_INTENSITY,
> > +		.modified = 1,
> > +		.channel2 = IIO_MOD_LIGHT_UV,
> > +		.address = ZOPT2201_MODE_UVB,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +				      BIT(IIO_CHAN_INFO_SCALE),
> > +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> > +	},
> > +	{
> > +		.type = IIO_UVINDEX,
> > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > +	},
> > +};
> > +
> > +static int zopt2201_read_raw(struct iio_dev *indio_dev,
> > +				struct iio_chan_spec const *chan,
> > +				int *val, int *val2, long mask)
> > +{
> > +	struct zopt2201_data *data = iio_priv(indio_dev);
> > +	u64 tmp;
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		ret = zopt2201_read(data, chan->address);
> > +		if (ret < 0)
> > +			return ret;
> > +		*val = ret;
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_PROCESSED:
> > +		ret = zopt2201_read(data, ZOPT2201_MODE_UVB);
> > +		if (ret < 0)
> > +			return ret;
> > +		*val = ret * 18 *
> > +			(1 << (20 - zopt2201_resolution[data->res].bits)) /
> > +			zopt2201_gain_uvb[data->gain].gain;
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->address) {
> > +		case ZOPT2201_MODE_ALS:
> > +			*val = zopt2201_gain_als[data->gain].scale;
> 
> This is documented above as micro lux, but illuminance ABI is in
> lux.

the scale factors above are given in micro lux

the code below the switch statement basically divides by 1000000 (I could 
have used IIO_VAL_FRACTIONAL, but that would use format string "%d.%09u" while 
everything else here uses "%d.%06u")

the resulting unit is Lux in fact

> > +			break;
> > +		case ZOPT2201_MODE_UVB:
> > +			*val = zopt2201_gain_uvb[data->gain].scale;
> > +			break;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +
> > +		*val2 = 1000000;
> > +		*val2 *= (1 << (zopt2201_resolution[data->res].bits - 13));
> > +		tmp = div_s64(*val * 1000000ULL, *val2);
> > +		*val = div_s64_rem(tmp, 1000000, val2);
> > +
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		*val = 0;
> > +		*val2 = zopt2201_resolution[data->res].us;
> > +		return IIO_VAL_INT_PLUS_MICRO;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int zopt2201_set_resolution(struct zopt2201_data *data, u8 res)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, ZOPT2201_LS_MEAS_RATE,
> > +					(res << ZOPT2201_MEAS_RES_SHIFT) |
> > +					data->rate);
> > +	if (ret >= 0)
> > +		data->res = res;
> 
> Same as the case below - prefer the error case indented.  Just that
> tiny bit nicer to read and means that when a reviewer scan's past it
> they won't stop to check what is going on...

OK
 
> > +
> > +	return ret;
> > +}
> > +
> > +static int zopt2201_write_resolution(struct zopt2201_data *data,
> > +				     int val, int val2)
> > +{
> > +	int i, ret;
> > +
> > +	if (val != 0)
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(zopt2201_resolution); i++)
> > +		if (val2 == zopt2201_resolution[i].us) {
> > +			mutex_lock(&data->lock);
> > +			ret = zopt2201_set_resolution(data, i);
> > +			mutex_unlock(&data->lock);
> > +			return ret;
> > +		}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int zopt2201_set_gain(struct zopt2201_data *data, u8 gain)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_write_byte_data(data->client, ZOPT2201_LS_GAIN, gain);
> > +	if (ret >= 0)
> > +		data->gain = gain;
> 
> I'm a fuss pot about this, but I always like the indented one to be the
> error path.  It adds a line of code, but it's easier to follow

fair enough
 
> 	if (ret < 0)
> 		return ret;
> 
> 	data->gain = gain;
> 
> 	return 0;
> > +
> > +	return ret;
> > +}
> > +
> > +static int zopt2201_write_scale_als(struct zopt2201_data *data,
> > +				     int val, int val2)
> > +{
> > +	int i, ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
> > +		if (val == zopt2201_scale_als[i].scale &&
> > +		    val2 == zopt2201_scale_als[i].uscale) {
> > +
> This is very deeply indented and it hurting readability.
> Perhaps a little utility function to handle the actual
> configuring might help?

yes, code is a bit clearer
 
> static int zopt2201_write_scale_als_by_idx(struct zopt2201_data *data, int index)
> {
> 	int ret;
> 
> 	mutex_lock(&data->lock);
> 	ret = zopt2201_set_resolution(data, zopt2201_scale_als[i].res);
> 	if (ret < 0)
> 		goto unlock;
> 
> 	ret = zopt2201_set_gain(data, zopt2201_scale_als[i].gain);
> 
> unlock:
> 	mutex_unlock(&data->lock);
> 	return ret;	
> }
> 
> static int zopt2201_write_scale_als(struct zopt2201_data *data,
> 				     int val, int val2)
> {
> 	int i, ret;
> 
> 	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
> 		if (val == zopt2201_scale_als[i].scale &&
> 		    val2 == zopt2201_scale_als[i].uscale) {
> 			return zopt2201_write_scale_als_by_idx(data, i);
> 	return -EINVAL;
> }
> 
> Plus equivalent for the next one below.
> 
> > +			mutex_lock(&data->lock);
> > +			ret = zopt2201_set_resolution(data,
> > +				zopt2201_scale_als[i].res);
> > +			if (ret < 0)
> > +				goto fail;
> > +			ret = zopt2201_set_gain(data,
> > +				zopt2201_scale_als[i].gain);
> > +
> > +fail:
> > +			mutex_unlock(&data->lock);
> > +			return ret;
> > +		}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int zopt2201_write_scale_uvb(struct zopt2201_data *data,
> > +				     int val, int val2)
> > +{
> > +	int i, ret;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_uvb); i++)
> > +		if (val == zopt2201_scale_uvb[i].scale &&
> > +		    val2 == zopt2201_scale_uvb[i].uscale) {
> > +			mutex_lock(&data->lock);
> > +			ret = zopt2201_set_resolution(data,
> > +				zopt2201_scale_uvb[i].res);
> > +			if (ret < 0)
> > +				goto fail;
> > +			ret = zopt2201_set_gain(data,
> > +				zopt2201_scale_uvb[i].gain);
> > +
> > +fail:
> > +			mutex_unlock(&data->lock);
> > +			return ret;
> > +		}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static int zopt2201_write_raw(struct iio_dev *indio_dev,
> > +			      struct iio_chan_spec const *chan,
> > +			      int val, int val2, long mask)
> > +{
> > +	struct zopt2201_data *data = iio_priv(indio_dev);
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_INT_TIME:
> > +		return zopt2201_write_resolution(data, val, val2);
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->address) {
> > +		case ZOPT2201_MODE_ALS:
> > +			return zopt2201_write_scale_als(data, val, val2);
> > +		case ZOPT2201_MODE_UVB:
> > +			return zopt2201_write_scale_uvb(data, val, val2);
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	}
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static ssize_t zopt2201_show_int_time_available(struct device *dev,
> > +						struct device_attribute *attr,
> > +						char *buf)
> > +{
> > +	size_t len = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(zopt2201_resolution); i++)
> > +		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06lu ",
> > +				 zopt2201_resolution[i].us);
> > +	buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static IIO_DEV_ATTR_INT_TIME_AVAIL(zopt2201_show_int_time_available);
> > +
> 
> One blank line is enough...

Sure
 
> > +
> > +static ssize_t zopt2201_show_als_scale_avail(struct device *dev,
> > +					     struct device_attribute *attr,
> > +					     char *buf)
> > +{
> > +	ssize_t len = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
> > +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
> > +				 zopt2201_scale_als[i].scale,
> > +				 zopt2201_scale_als[i].uscale);
> > +	buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static ssize_t zopt2201_show_uvb_scale_avail(struct device *dev,
> > +					     struct device_attribute *attr,
> > +					     char *buf)
> > +{
> > +	ssize_t len = 0;
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_uvb); i++)
> > +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
> > +				 zopt2201_scale_uvb[i].scale,
> > +				 zopt2201_scale_uvb[i].uscale);
> > +	buf[len - 1] = '\n';
> > +
> > +	return len;
> > +}
> > +
> > +static IIO_DEVICE_ATTR(in_illuminance_scale_available, 0444,
> > +		       zopt2201_show_als_scale_avail, NULL, 0);
> > +static IIO_DEVICE_ATTR(in_intensity_uv_scale_available, 0444,
> > +		       zopt2201_show_uvb_scale_avail, NULL, 0);
> > +
> > +static struct attribute *zopt2201_attributes[] = {
> > +	&iio_dev_attr_integration_time_available.dev_attr.attr,
> > +	&iio_dev_attr_in_illuminance_scale_available.dev_attr.attr,
> > +	&iio_dev_attr_in_intensity_uv_scale_available.dev_attr.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group zopt2201_attribute_group = {
> > +	.attrs = zopt2201_attributes,
> > +};
> > +
> 
> Only 1 blank line.

OK
 
> > +
> > +static const struct iio_info zopt2201_info = {
> > +	.read_raw = zopt2201_read_raw,
> > +	.write_raw = zopt2201_write_raw,
> > +	.attrs = &zopt2201_attribute_group,
> > +};
> > +
> > +static int zopt2201_probe(struct i2c_client *client,
> > +			  const struct i2c_device_id *id)
> > +{
> > +	struct zopt2201_data *data;
> > +	struct iio_dev *indio_dev;
> > +	int ret;
> > +
> > +	if (!i2c_check_functionality(client->adapter,
> > +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> > +		return -EOPNOTSUPP;
> > +
> > +	ret = i2c_smbus_read_byte_data(client, ZOPT2201_PART_ID);
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != ZOPT2201_PART_NUMBER)
> > +		return -ENODEV;
> > +
> > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	data->client = client;
> > +	mutex_init(&data->lock);
> > +
> > +	indio_dev->dev.parent = &client->dev;
> > +	indio_dev->info = &zopt2201_info;
> > +	indio_dev->channels = zopt2201_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(zopt2201_channels);
> > +	indio_dev->name = ZOPT2201_DRV_NAME;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	data->rate = ZOPT2201_MEAS_FREQ_100MS;
> > +	ret = zopt2201_set_resolution(data, ZOPT2201_MEAS_RES_18BIT);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = zopt2201_set_gain(data, ZOPT2201_LS_GAIN_3);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(&client->dev, indio_dev);
> > +}
> > +
> > +static const struct i2c_device_id zopt2201_id[] = {
> > +	{ "zopt2201", 0 },
> > +	{ }
> > +};
> > +MODULE_DEVICE_TABLE(i2c, zopt2201_id);
> 
> Hmm. I wonder if we should be adding device tree bindings for
> pretty much all drivers now.  There is on going effort to
> drop the compatibility code that loads them based on the
> i2c device table.  Anyhow, can be a follow up patch so
> not a problem right now.

agree
 
> > +
> > +static struct i2c_driver zopt2201_driver = {
> > +	.driver = {
> > +		.name   = ZOPT2201_DRV_NAME,
> > +	},
> > +	.probe  = zopt2201_probe,
> > +	.id_table = zopt2201_id,
> > +};
> > +
> > +module_i2c_driver(zopt2201_driver);
> > +
> > +MODULE_AUTHOR("Peter Meerwald-Stadler <pmeerw@pmeerw.net>");
> > +MODULE_DESCRIPTION("IDT ZOPT2201 ambient light and UV B sensor driver");
> > +MODULE_LICENSE("GPL");
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
Jonathan Cameron Nov. 19, 2017, 4:54 p.m. UTC | #3
On Sun, 5 Nov 2017 23:48:13 +0100 (CET)
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:

> Hello Jonathan,
> 
> > Another very nice driver.  A couple of refactoring suggestions inline
> > that I think would slightly (and it is slightly!) improve readability.
> > I think we need some additional docs for the uv intensity channel as I can't
> > find any documented units for that - so far they have been fairly randomly
> > filtered and no docs have existed to map these to units.  
> 
> UV intensity is already documented (around line 1288) as
> /sys/.../iio:deviceX/in_intensityY_uv_raw
> however
> /sys/.../iio:deviceX/in_intensity_raw and 
> /sys/.../iio:deviceX/in_intensity_uv_raw are missing, should these be 
> added for completeness??

Yes, if we have drivers using those elements they should be in the ABI docs.
> 
> > As such I'm thinking w/m^2 would be a nicer base unit.  If I'm missing
> > a prior example of this then let me know - I haven't really looked :)  
> 
> intensity_uv is documented as unit-less currently;
> not sure if we can suggest a unit after-the-fact; drivers may use _SCALE 
> to set a gain, and not care about the unit
we can if we don't have current users specifying a scale ;)  Which they
shouldn't be if they are unit less.

> 
> other chips such as the AS7262 are using micro W/cm2;
> our channel modifiers (both, clear, uv, red, green blue) may not be 
> sufficient for spectral sensors (6 visible channels at 450, 500, 550, 570, 
> 600, 650 -- AS7262; NIR channels at 610, 680, 730, 760, 810, 860 -- 
> AS7263)
Hmm. When we originally came up with the infrared terms I wondered
if we would be better off with more explicit description of wavelength
/ frequency.  Perhaps we need to add that as additional ABI?

> 
> will post a v2 with your suggestions shortly, thanks for the review!
> comments to your suggestions below...
> 
> regards, p.
> 
> > > ---
> > >  drivers/iio/light/Kconfig    |  10 +
> > >  drivers/iio/light/Makefile   |   1 +
> > >  drivers/iio/light/zopt2201.c | 563 +++++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 574 insertions(+)
> > >  create mode 100644 drivers/iio/light/zopt2201.c
> > > 
> > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > > index 2356ed9285df..6a5835fab32e 100644
> > > --- a/drivers/iio/light/Kconfig
> > > +++ b/drivers/iio/light/Kconfig
> > > @@ -425,4 +425,14 @@ config VL6180
> > >  	 To compile this driver as a module, choose M here: the
> > >  	 module will be called vl6180.
> > >  
> > > +config ZOPT2201
> > > +	tristate "ZOPT2201 ALS and UV B sensor"
> > > +	depends on I2C
> > > +	help
> > > +	 Say Y here if you want to build a driver for the IDT
> > > +	 ZOPT2201 ambient light and UV B sensor.
> > > +
> > > +	 To compile this driver as a module, choose M here: the
> > > +	 module will be called zopt2201.
> > > +
> > >  endmenu
> > > diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> > > index fa32fa459e2e..d99abdf6b746 100644
> > > --- a/drivers/iio/light/Makefile
> > > +++ b/drivers/iio/light/Makefile
> > > @@ -40,3 +40,4 @@ obj-$(CONFIG_US5182D)		+= us5182d.o
> > >  obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
> > >  obj-$(CONFIG_VEML6070)		+= veml6070.o
> > >  obj-$(CONFIG_VL6180)		+= vl6180.o
> > > +obj-$(CONFIG_ZOPT2201)		+= zopt2201.o
> > > diff --git a/drivers/iio/light/zopt2201.c b/drivers/iio/light/zopt2201.c
> > > new file mode 100644
> > > index 000000000000..07b299642ac3
> > > --- /dev/null
> > > +++ b/drivers/iio/light/zopt2201.c
> > > @@ -0,0 +1,563 @@
> > > +/*
> > > + * zopt2201.c - Support for IDT ZOPT2201 ambient light and UV B sensor
> > > + *
> > > + * Copyright 2017 Peter Meerwald-Stadler <pmeerw@pmeerw.net>
> > > + *
> > > + * This file is subject to the terms and conditions of version 2 of
> > > + * the GNU General Public License.  See the file COPYING in the main
> > > + * directory of this archive for more details.
> > > + *
> > > + * Datasheet: https://www.idt.com/document/dst/zopt2201-datasheet
> > > + * 7-bit I2C slave addresses 0x53 (default) or 0x52 (programmed)
> > > + *
> > > + * TODO: interrupt support, ALS/UVB raw mode
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/i2c.h>
> > > +#include <linux/mutex.h>
> > > +#include <linux/err.h>
> > > +#include <linux/delay.h>
> > > +
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/sysfs.h>
> > > +
> > > +#define ZOPT2201_DRV_NAME "zopt2201"
> > > +
> > > +/* Registers */
> > > +#define ZOPT2201_MAIN_CTRL		0x00
> > > +#define ZOPT2201_LS_MEAS_RATE		0x04
> > > +#define ZOPT2201_LS_GAIN		0x05
> > > +#define ZOPT2201_PART_ID		0x06
> > > +#define ZOPT2201_MAIN_STATUS		0x07
> > > +#define ZOPT2201_ALS_DATA		0x0d /* LSB first, 13 to 20 bits */
> > > +#define ZOPT2201_UVB_DATA		0x10 /* LSB first, 13 to 20 bits */
> > > +#define ZOPT2201_UV_COMP_DATA		0x13 /* LSB first, 13 to 20 bits */
> > > +#define ZOPT2201_COMP_DATA		0x16 /* LSB first, 13 to 20 bits */
> > > +#define ZOPT2201_INT_CFG		0x19
> > > +#define ZOPT2201_INT_PST		0x1a
> > > +
> > > +#define ZOPT2201_MAIN_CTRL_LS_MODE	BIT(3) /* 0 .. ALS, 1 .. UV B */
> > > +#define ZOPT2201_MAIN_CTRL_LS_EN	BIT(1)
> > > +
> > > +/* Values for ZOPT2201_LS_MEAS_RATE resolution / bit width */
> > > +#define ZOPT2201_MEAS_RES_20BIT		0 /* takes 400 ms */
> > > +#define ZOPT2201_MEAS_RES_19BIT		1 /* takes 200 ms */
> > > +#define ZOPT2201_MEAS_RES_18BIT		2 /* takes 100 ms, default */
> > > +#define ZOPT2201_MEAS_RES_17BIT		3 /* takes 50 ms */
> > > +#define ZOPT2201_MEAS_RES_16BIT		4 /* takes 25 ms */
> > > +#define ZOPT2201_MEAS_RES_13BIT		5 /* takes 3.125 ms */
> > > +#define ZOPT2201_MEAS_RES_SHIFT		4
> > > +
> > > +/* Values for ZOPT2201_LS_MEAS_RATE measurement rate */
> > > +#define ZOPT2201_MEAS_FREQ_25MS		0
> > > +#define ZOPT2201_MEAS_FREQ_50MS		1
> > > +#define ZOPT2201_MEAS_FREQ_100MS	2 /* default */
> > > +#define ZOPT2201_MEAS_FREQ_200MS	3
> > > +#define ZOPT2201_MEAS_FREQ_500MS	4
> > > +#define ZOPT2201_MEAS_FREQ_1000MS	5
> > > +#define ZOPT2201_MEAS_FREQ_2000MS	6
> > > +
> > > +/* Values for ZOPT2201_LS_GAIN */
> > > +#define ZOPT2201_LS_GAIN_1		0
> > > +#define ZOPT2201_LS_GAIN_3		1
> > > +#define ZOPT2201_LS_GAIN_6		2
> > > +#define ZOPT2201_LS_GAIN_9		3
> > > +#define ZOPT2201_LS_GAIN_18		4
> > > +
> > > +/* Values for ZOPT2201_MAIN_STATUS */
> > > +#define ZOPT2201_MAIN_STATUS_POWERON	BIT(5)
> > > +#define ZOPT2201_MAIN_STATUS_INT	BIT(4)
> > > +#define ZOPT2201_MAIN_STATUS_DRDY	BIT(3)
> > > +
> > > +#define ZOPT2201_PART_NUMBER		0xb2
> > > +
> > > +struct zopt2201_data {
> > > +	struct i2c_client *client;
> > > +	struct mutex lock;
> > > +	u8 gain;
> > > +	u8 res;
> > > +	u8 rate;
> > > +};
> > > +
> > > +enum zopt2201_mode {
> > > +	ZOPT2201_MODE_ALS,
> > > +	ZOPT2201_MODE_UVB,
> > > +};
> > > +
> > > +static const struct {
> > > +	unsigned int gain; /* gain factor */
> > > +	unsigned int scale; /* micro lux per count */
> > > +} zopt2201_gain_als[] = {
> > > +	{  1, 19200000 },
> > > +	{  3,  6400000 },
> > > +	{  6,  3200000 },
> > > +	{  9,  2133333 },
> > > +	{ 18,  1066666 },
> > > +};
> > > +
> > > +static const struct {
> > > +	unsigned int gain; /* gain factor */
> > > +	unsigned int scale; /* micro W/cm2 per count */  
> > 
> > I think docs need to be updated to cover having units for
> > uv. Certainly good to assign units if we can to intensity
> > channels.
> > 
> > Hmm. micro W / cm2 so 0.001W/((0.01*0.01)m^2)  
> 
> micro W/cm2 so 0.000001W/cm2 so 0.01W/m^2 ?
> 
> > If we are going to define a unit I'd prefer we ended up with
> > w/m^2 and hence SI unit.  
> 
> I have changed the scale factor so that the driver now reports W/m^2;
> the comment above ("micro W/cm2 per count") was incorrect, in fact the 
> values related to pico W/cm2
>  
> > > +} zopt2201_gain_uvb[] = {
> > > +	{  1, 46080000 },
> > > +	{  3, 15360000 },
> > > +	{  6,  7680000 },
> > > +	{  9,  5120000 },
> > > +	{ 18,  2560000 },
> > > +};
> > > +
> > > +static const struct {
> > > +	unsigned int bits; /* sensor resolution in bits */
> > > +	unsigned long us; /* measurement time in micro seconds */
> > > +} zopt2201_resolution[] = {
> > > +	{ 20, 400000 },
> > > +	{ 19, 200000 },
> > > +	{ 18, 100000 },
> > > +	{ 17,  50000 },
> > > +	{ 16,  25000 },
> > > +	{ 13,   3125 },
> > > +};
> > > +
> > > +static const struct {
> > > +	unsigned int scale, uscale; /* scale factor as integer + micro */
> > > +	u8 gain; /* gain register value */
> > > +	u8 res; /* resolution register value */
> > > +} zopt2201_scale_als[] = {
> > > +	{ 19, 200000, 0, 5 },
> > > +	{  6, 400000, 1, 5 },
> > > +	{  3, 200000, 2, 5 },
> > > +	{  2, 400000, 0, 4 },
> > > +	{  2, 133333, 3, 5 },
> > > +	{  1, 200000, 0, 3 },
> > > +	{  1,  66666, 4, 5 },
> > > +	{  0, 800000, 1, 4 },
> > > +	{  0, 600000, 0, 2 },
> > > +	{  0, 400000, 2, 4 },
> > > +	{  0, 300000, 0, 1 },
> > > +	{  0, 266666, 3, 4 },
> > > +	{  0, 200000, 2, 3 },
> > > +	{  0, 150000, 0, 0 },
> > > +	{  0, 133333, 4, 4 },
> > > +	{  0, 100000, 2, 2 },
> > > +	{  0,  66666, 4, 3 },
> > > +	{  0,  50000, 2, 1 },
> > > +	{  0,  33333, 4, 2 },
> > > +	{  0,  25000, 2, 0 },
> > > +	{  0,  16666, 4, 1 },
> > > +	{  0,   8333, 4, 0 },
> > > +};
> > > +
> > > +static const struct {
> > > +	unsigned int scale, uscale; /* scale factor as integer + micro */
> > > +	u8 gain; /* gain register value */
> > > +	u8 res; /* resolution register value */
> > > +} zopt2201_scale_uvb[] = {
> > > +	{ 46,  80000, 0, 5 },
> > > +	{ 15, 360000, 1, 5 },
> > > +	{  7, 680000, 2, 5 },
> > > +	{  5, 760000, 0, 4 },
> > > +	{  5, 120000, 3, 5 },
> > > +	{  2, 880000, 0, 3 },
> > > +	{  2, 560000, 4, 5 },
> > > +	{  1, 920000, 1, 4 },
> > > +	{  1, 440000, 0, 2 },
> > > +	{  0, 960000, 2, 4 },
> > > +	{  0, 720000, 0, 1 },
> > > +	{  0, 640000, 3, 4 },
> > > +	{  0, 480000, 2, 3 },
> > > +	{  0, 360000, 0, 0 },
> > > +	{  0, 320000, 4, 4 },
> > > +	{  0, 240000, 2, 2 },
> > > +	{  0, 160000, 4, 3 },
> > > +	{  0, 120000, 2, 1 },
> > > +	{  0,  80000, 4, 2 },
> > > +	{  0,  60000, 2, 0 },
> > > +	{  0,  40000, 4, 1 },
> > > +	{  0,  20000, 4, 0 },
> > > +};
> > > +
> > > +static int zopt2201_enable_mode(struct zopt2201_data *data,
> > > +				enum zopt2201_mode mode)
> > > +{
> > > +	u8 out = ZOPT2201_MAIN_CTRL_LS_EN;
> > > +
> > > +	if (mode == ZOPT2201_MODE_UVB)
> > > +		out |= ZOPT2201_MAIN_CTRL_LS_MODE;
> > > +
> > > +	return i2c_smbus_write_byte_data(data->client, ZOPT2201_MAIN_CTRL, out);
> > > +}
> > > +
> > > +static int zopt2201_read(struct zopt2201_data *data, enum zopt2201_mode mode)
> > > +{
> > > +	struct i2c_client *client = data->client;
> > > +	int tries = 10;
> > > +	u8 buf[3];
> > > +	int ret;
> > > +
> > > +	mutex_lock(&data->lock);
> > > +	ret = zopt2201_enable_mode(data, mode);
> > > +	if (ret < 0)
> > > +		goto fail;
> > > +
> > > +	while (tries--) {
> > > +		unsigned long t = zopt2201_resolution[data->res].us;
> > > +
> > > +		if (t <= 20000)
> > > +			usleep_range(t, t + 1000);
> > > +		else
> > > +			msleep(t / 1000);
> > > +		ret = i2c_smbus_read_byte_data(client, ZOPT2201_MAIN_STATUS);
> > > +		if (ret < 0)
> > > +			goto fail;
> > > +		if (ret & ZOPT2201_MAIN_STATUS_DRDY)
> > > +			break;
> > > +	}
> > > +
> > > +	if (tries < 0) {
> > > +		ret = -ETIMEDOUT;
> > > +		goto fail;
> > > +	}
> > > +
> > > +	ret = i2c_smbus_read_i2c_block_data(client, mode == ZOPT2201_MODE_UVB ?
> > > +		ZOPT2201_UVB_DATA : ZOPT2201_ALS_DATA, sizeof(buf), buf);  
> > Having a ternary buried in there is a little hard to read.  Please pull it
> > out as a local variable assignment.  
> 
> OK, actually I've dropped the mode enum and will use the corresponding 
> register address directly (saves some lines)
>  
> > > +	if (ret < 0)
> > > +		goto fail;
> > > +
> > > +	ret = i2c_smbus_write_byte_data(client, ZOPT2201_MAIN_CTRL, 0x00);
> > > +	if (ret < 0)
> > > +		goto fail;
> > > +	mutex_unlock(&data->lock);
> > > +
> > > +	return (buf[2] << 16) | (buf[1] << 8) | buf[0];
> > > +
> > > +fail:
> > > +	mutex_unlock(&data->lock);
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct iio_chan_spec zopt2201_channels[] = {
> > > +	{
> > > +		.type = IIO_LIGHT,
> > > +		.address = ZOPT2201_MODE_ALS,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > +				      BIT(IIO_CHAN_INFO_SCALE),
> > > +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> > > +	},
> > > +	{
> > > +		.type = IIO_INTENSITY,
> > > +		.modified = 1,
> > > +		.channel2 = IIO_MOD_LIGHT_UV,
> > > +		.address = ZOPT2201_MODE_UVB,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > > +				      BIT(IIO_CHAN_INFO_SCALE),
> > > +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> > > +	},
> > > +	{
> > > +		.type = IIO_UVINDEX,
> > > +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> > > +	},
> > > +};
> > > +
> > > +static int zopt2201_read_raw(struct iio_dev *indio_dev,
> > > +				struct iio_chan_spec const *chan,
> > > +				int *val, int *val2, long mask)
> > > +{
> > > +	struct zopt2201_data *data = iio_priv(indio_dev);
> > > +	u64 tmp;
> > > +	int ret;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW:
> > > +		ret = zopt2201_read(data, chan->address);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +		*val = ret;
> > > +		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_PROCESSED:
> > > +		ret = zopt2201_read(data, ZOPT2201_MODE_UVB);
> > > +		if (ret < 0)
> > > +			return ret;
> > > +		*val = ret * 18 *
> > > +			(1 << (20 - zopt2201_resolution[data->res].bits)) /
> > > +			zopt2201_gain_uvb[data->gain].gain;
> > > +		return IIO_VAL_INT;
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		switch (chan->address) {
> > > +		case ZOPT2201_MODE_ALS:
> > > +			*val = zopt2201_gain_als[data->gain].scale;  
> > 
> > This is documented above as micro lux, but illuminance ABI is in
> > lux.  
> 
> the scale factors above are given in micro lux
> 
> the code below the switch statement basically divides by 1000000 (I could 
> have used IIO_VAL_FRACTIONAL, but that would use format string "%d.%09u" while 
> everything else here uses "%d.%06u")
> 
> the resulting unit is Lux in fact
> 
> > > +			break;
> > > +		case ZOPT2201_MODE_UVB:
> > > +			*val = zopt2201_gain_uvb[data->gain].scale;
> > > +			break;
> > > +		default:
> > > +			return -EINVAL;
> > > +		}
> > > +
> > > +		*val2 = 1000000;
> > > +		*val2 *= (1 << (zopt2201_resolution[data->res].bits - 13));
> > > +		tmp = div_s64(*val * 1000000ULL, *val2);
> > > +		*val = div_s64_rem(tmp, 1000000, val2);
> > > +
> > > +		return IIO_VAL_INT_PLUS_MICRO;
> > > +	case IIO_CHAN_INFO_INT_TIME:
> > > +		*val = 0;
> > > +		*val2 = zopt2201_resolution[data->res].us;
> > > +		return IIO_VAL_INT_PLUS_MICRO;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +}
> > > +
> > > +static int zopt2201_set_resolution(struct zopt2201_data *data, u8 res)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = i2c_smbus_write_byte_data(data->client, ZOPT2201_LS_MEAS_RATE,
> > > +					(res << ZOPT2201_MEAS_RES_SHIFT) |
> > > +					data->rate);
> > > +	if (ret >= 0)
> > > +		data->res = res;  
> > 
> > Same as the case below - prefer the error case indented.  Just that
> > tiny bit nicer to read and means that when a reviewer scan's past it
> > they won't stop to check what is going on...  
> 
> OK
>  
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int zopt2201_write_resolution(struct zopt2201_data *data,
> > > +				     int val, int val2)
> > > +{
> > > +	int i, ret;
> > > +
> > > +	if (val != 0)
> > > +		return -EINVAL;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(zopt2201_resolution); i++)
> > > +		if (val2 == zopt2201_resolution[i].us) {
> > > +			mutex_lock(&data->lock);
> > > +			ret = zopt2201_set_resolution(data, i);
> > > +			mutex_unlock(&data->lock);
> > > +			return ret;
> > > +		}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int zopt2201_set_gain(struct zopt2201_data *data, u8 gain)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = i2c_smbus_write_byte_data(data->client, ZOPT2201_LS_GAIN, gain);
> > > +	if (ret >= 0)
> > > +		data->gain = gain;  
> > 
> > I'm a fuss pot about this, but I always like the indented one to be the
> > error path.  It adds a line of code, but it's easier to follow  
> 
> fair enough
>  
> > 	if (ret < 0)
> > 		return ret;
> > 
> > 	data->gain = gain;
> > 
> > 	return 0;  
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int zopt2201_write_scale_als(struct zopt2201_data *data,
> > > +				     int val, int val2)
> > > +{
> > > +	int i, ret;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
> > > +		if (val == zopt2201_scale_als[i].scale &&
> > > +		    val2 == zopt2201_scale_als[i].uscale) {
> > > +  
> > This is very deeply indented and it hurting readability.
> > Perhaps a little utility function to handle the actual
> > configuring might help?  
> 
> yes, code is a bit clearer
>  
> > static int zopt2201_write_scale_als_by_idx(struct zopt2201_data *data, int index)
> > {
> > 	int ret;
> > 
> > 	mutex_lock(&data->lock);
> > 	ret = zopt2201_set_resolution(data, zopt2201_scale_als[i].res);
> > 	if (ret < 0)
> > 		goto unlock;
> > 
> > 	ret = zopt2201_set_gain(data, zopt2201_scale_als[i].gain);
> > 
> > unlock:
> > 	mutex_unlock(&data->lock);
> > 	return ret;	
> > }
> > 
> > static int zopt2201_write_scale_als(struct zopt2201_data *data,
> > 				     int val, int val2)
> > {
> > 	int i, ret;
> > 
> > 	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
> > 		if (val == zopt2201_scale_als[i].scale &&
> > 		    val2 == zopt2201_scale_als[i].uscale) {
> > 			return zopt2201_write_scale_als_by_idx(data, i);
> > 	return -EINVAL;
> > }
> > 
> > Plus equivalent for the next one below.
> >   
> > > +			mutex_lock(&data->lock);
> > > +			ret = zopt2201_set_resolution(data,
> > > +				zopt2201_scale_als[i].res);
> > > +			if (ret < 0)
> > > +				goto fail;
> > > +			ret = zopt2201_set_gain(data,
> > > +				zopt2201_scale_als[i].gain);
> > > +
> > > +fail:
> > > +			mutex_unlock(&data->lock);
> > > +			return ret;
> > > +		}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int zopt2201_write_scale_uvb(struct zopt2201_data *data,
> > > +				     int val, int val2)
> > > +{
> > > +	int i, ret;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_uvb); i++)
> > > +		if (val == zopt2201_scale_uvb[i].scale &&
> > > +		    val2 == zopt2201_scale_uvb[i].uscale) {
> > > +			mutex_lock(&data->lock);
> > > +			ret = zopt2201_set_resolution(data,
> > > +				zopt2201_scale_uvb[i].res);
> > > +			if (ret < 0)
> > > +				goto fail;
> > > +			ret = zopt2201_set_gain(data,
> > > +				zopt2201_scale_uvb[i].gain);
> > > +
> > > +fail:
> > > +			mutex_unlock(&data->lock);
> > > +			return ret;
> > > +		}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static int zopt2201_write_raw(struct iio_dev *indio_dev,
> > > +			      struct iio_chan_spec const *chan,
> > > +			      int val, int val2, long mask)
> > > +{
> > > +	struct zopt2201_data *data = iio_priv(indio_dev);
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_INT_TIME:
> > > +		return zopt2201_write_resolution(data, val, val2);
> > > +	case IIO_CHAN_INFO_SCALE:
> > > +		switch (chan->address) {
> > > +		case ZOPT2201_MODE_ALS:
> > > +			return zopt2201_write_scale_als(data, val, val2);
> > > +		case ZOPT2201_MODE_UVB:
> > > +			return zopt2201_write_scale_uvb(data, val, val2);
> > > +		default:
> > > +			return -EINVAL;
> > > +		}
> > > +	}
> > > +
> > > +	return -EINVAL;
> > > +}
> > > +
> > > +static ssize_t zopt2201_show_int_time_available(struct device *dev,
> > > +						struct device_attribute *attr,
> > > +						char *buf)
> > > +{
> > > +	size_t len = 0;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(zopt2201_resolution); i++)
> > > +		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06lu ",
> > > +				 zopt2201_resolution[i].us);
> > > +	buf[len - 1] = '\n';
> > > +
> > > +	return len;
> > > +}
> > > +
> > > +static IIO_DEV_ATTR_INT_TIME_AVAIL(zopt2201_show_int_time_available);
> > > +  
> > 
> > One blank line is enough...  
> 
> Sure
>  
> > > +
> > > +static ssize_t zopt2201_show_als_scale_avail(struct device *dev,
> > > +					     struct device_attribute *attr,
> > > +					     char *buf)
> > > +{
> > > +	ssize_t len = 0;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
> > > +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
> > > +				 zopt2201_scale_als[i].scale,
> > > +				 zopt2201_scale_als[i].uscale);
> > > +	buf[len - 1] = '\n';
> > > +
> > > +	return len;
> > > +}
> > > +
> > > +static ssize_t zopt2201_show_uvb_scale_avail(struct device *dev,
> > > +					     struct device_attribute *attr,
> > > +					     char *buf)
> > > +{
> > > +	ssize_t len = 0;
> > > +	int i;
> > > +
> > > +	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_uvb); i++)
> > > +		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
> > > +				 zopt2201_scale_uvb[i].scale,
> > > +				 zopt2201_scale_uvb[i].uscale);
> > > +	buf[len - 1] = '\n';
> > > +
> > > +	return len;
> > > +}
> > > +
> > > +static IIO_DEVICE_ATTR(in_illuminance_scale_available, 0444,
> > > +		       zopt2201_show_als_scale_avail, NULL, 0);
> > > +static IIO_DEVICE_ATTR(in_intensity_uv_scale_available, 0444,
> > > +		       zopt2201_show_uvb_scale_avail, NULL, 0);
> > > +
> > > +static struct attribute *zopt2201_attributes[] = {
> > > +	&iio_dev_attr_integration_time_available.dev_attr.attr,
> > > +	&iio_dev_attr_in_illuminance_scale_available.dev_attr.attr,
> > > +	&iio_dev_attr_in_intensity_uv_scale_available.dev_attr.attr,
> > > +	NULL
> > > +};
> > > +
> > > +static const struct attribute_group zopt2201_attribute_group = {
> > > +	.attrs = zopt2201_attributes,
> > > +};
> > > +  
> > 
> > Only 1 blank line.  
> 
> OK
>  
> > > +
> > > +static const struct iio_info zopt2201_info = {
> > > +	.read_raw = zopt2201_read_raw,
> > > +	.write_raw = zopt2201_write_raw,
> > > +	.attrs = &zopt2201_attribute_group,
> > > +};
> > > +
> > > +static int zopt2201_probe(struct i2c_client *client,
> > > +			  const struct i2c_device_id *id)
> > > +{
> > > +	struct zopt2201_data *data;
> > > +	struct iio_dev *indio_dev;
> > > +	int ret;
> > > +
> > > +	if (!i2c_check_functionality(client->adapter,
> > > +				     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> > > +		return -EOPNOTSUPP;
> > > +
> > > +	ret = i2c_smbus_read_byte_data(client, ZOPT2201_PART_ID);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	if (ret != ZOPT2201_PART_NUMBER)
> > > +		return -ENODEV;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	data = iio_priv(indio_dev);
> > > +	i2c_set_clientdata(client, indio_dev);
> > > +	data->client = client;
> > > +	mutex_init(&data->lock);
> > > +
> > > +	indio_dev->dev.parent = &client->dev;
> > > +	indio_dev->info = &zopt2201_info;
> > > +	indio_dev->channels = zopt2201_channels;
> > > +	indio_dev->num_channels = ARRAY_SIZE(zopt2201_channels);
> > > +	indio_dev->name = ZOPT2201_DRV_NAME;
> > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > +
> > > +	data->rate = ZOPT2201_MEAS_FREQ_100MS;
> > > +	ret = zopt2201_set_resolution(data, ZOPT2201_MEAS_RES_18BIT);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	ret = zopt2201_set_gain(data, ZOPT2201_LS_GAIN_3);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	return devm_iio_device_register(&client->dev, indio_dev);
> > > +}
> > > +
> > > +static const struct i2c_device_id zopt2201_id[] = {
> > > +	{ "zopt2201", 0 },
> > > +	{ }
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, zopt2201_id);  
> > 
> > Hmm. I wonder if we should be adding device tree bindings for
> > pretty much all drivers now.  There is on going effort to
> > drop the compatibility code that loads them based on the
> > i2c device table.  Anyhow, can be a follow up patch so
> > not a problem right now.  
> 
> agree
>  
> > > +
> > > +static struct i2c_driver zopt2201_driver = {
> > > +	.driver = {
> > > +		.name   = ZOPT2201_DRV_NAME,
> > > +	},
> > > +	.probe  = zopt2201_probe,
> > > +	.id_table = zopt2201_id,
> > > +};
> > > +
> > > +module_i2c_driver(zopt2201_driver);
> > > +
> > > +MODULE_AUTHOR("Peter Meerwald-Stadler <pmeerw@pmeerw.net>");
> > > +MODULE_DESCRIPTION("IDT ZOPT2201 ambient light and UV B sensor driver");
> > > +MODULE_LICENSE("GPL");  
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >   
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 2356ed9285df..6a5835fab32e 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -425,4 +425,14 @@  config VL6180
 	 To compile this driver as a module, choose M here: the
 	 module will be called vl6180.
 
+config ZOPT2201
+	tristate "ZOPT2201 ALS and UV B sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build a driver for the IDT
+	 ZOPT2201 ambient light and UV B sensor.
+
+	 To compile this driver as a module, choose M here: the
+	 module will be called zopt2201.
+
 endmenu
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index fa32fa459e2e..d99abdf6b746 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -40,3 +40,4 @@  obj-$(CONFIG_US5182D)		+= us5182d.o
 obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
 obj-$(CONFIG_VEML6070)		+= veml6070.o
 obj-$(CONFIG_VL6180)		+= vl6180.o
+obj-$(CONFIG_ZOPT2201)		+= zopt2201.o
diff --git a/drivers/iio/light/zopt2201.c b/drivers/iio/light/zopt2201.c
new file mode 100644
index 000000000000..07b299642ac3
--- /dev/null
+++ b/drivers/iio/light/zopt2201.c
@@ -0,0 +1,563 @@ 
+/*
+ * zopt2201.c - Support for IDT ZOPT2201 ambient light and UV B sensor
+ *
+ * Copyright 2017 Peter Meerwald-Stadler <pmeerw@pmeerw.net>
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License.  See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * Datasheet: https://www.idt.com/document/dst/zopt2201-datasheet
+ * 7-bit I2C slave addresses 0x53 (default) or 0x52 (programmed)
+ *
+ * TODO: interrupt support, ALS/UVB raw mode
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define ZOPT2201_DRV_NAME "zopt2201"
+
+/* Registers */
+#define ZOPT2201_MAIN_CTRL		0x00
+#define ZOPT2201_LS_MEAS_RATE		0x04
+#define ZOPT2201_LS_GAIN		0x05
+#define ZOPT2201_PART_ID		0x06
+#define ZOPT2201_MAIN_STATUS		0x07
+#define ZOPT2201_ALS_DATA		0x0d /* LSB first, 13 to 20 bits */
+#define ZOPT2201_UVB_DATA		0x10 /* LSB first, 13 to 20 bits */
+#define ZOPT2201_UV_COMP_DATA		0x13 /* LSB first, 13 to 20 bits */
+#define ZOPT2201_COMP_DATA		0x16 /* LSB first, 13 to 20 bits */
+#define ZOPT2201_INT_CFG		0x19
+#define ZOPT2201_INT_PST		0x1a
+
+#define ZOPT2201_MAIN_CTRL_LS_MODE	BIT(3) /* 0 .. ALS, 1 .. UV B */
+#define ZOPT2201_MAIN_CTRL_LS_EN	BIT(1)
+
+/* Values for ZOPT2201_LS_MEAS_RATE resolution / bit width */
+#define ZOPT2201_MEAS_RES_20BIT		0 /* takes 400 ms */
+#define ZOPT2201_MEAS_RES_19BIT		1 /* takes 200 ms */
+#define ZOPT2201_MEAS_RES_18BIT		2 /* takes 100 ms, default */
+#define ZOPT2201_MEAS_RES_17BIT		3 /* takes 50 ms */
+#define ZOPT2201_MEAS_RES_16BIT		4 /* takes 25 ms */
+#define ZOPT2201_MEAS_RES_13BIT		5 /* takes 3.125 ms */
+#define ZOPT2201_MEAS_RES_SHIFT		4
+
+/* Values for ZOPT2201_LS_MEAS_RATE measurement rate */
+#define ZOPT2201_MEAS_FREQ_25MS		0
+#define ZOPT2201_MEAS_FREQ_50MS		1
+#define ZOPT2201_MEAS_FREQ_100MS	2 /* default */
+#define ZOPT2201_MEAS_FREQ_200MS	3
+#define ZOPT2201_MEAS_FREQ_500MS	4
+#define ZOPT2201_MEAS_FREQ_1000MS	5
+#define ZOPT2201_MEAS_FREQ_2000MS	6
+
+/* Values for ZOPT2201_LS_GAIN */
+#define ZOPT2201_LS_GAIN_1		0
+#define ZOPT2201_LS_GAIN_3		1
+#define ZOPT2201_LS_GAIN_6		2
+#define ZOPT2201_LS_GAIN_9		3
+#define ZOPT2201_LS_GAIN_18		4
+
+/* Values for ZOPT2201_MAIN_STATUS */
+#define ZOPT2201_MAIN_STATUS_POWERON	BIT(5)
+#define ZOPT2201_MAIN_STATUS_INT	BIT(4)
+#define ZOPT2201_MAIN_STATUS_DRDY	BIT(3)
+
+#define ZOPT2201_PART_NUMBER		0xb2
+
+struct zopt2201_data {
+	struct i2c_client *client;
+	struct mutex lock;
+	u8 gain;
+	u8 res;
+	u8 rate;
+};
+
+enum zopt2201_mode {
+	ZOPT2201_MODE_ALS,
+	ZOPT2201_MODE_UVB,
+};
+
+static const struct {
+	unsigned int gain; /* gain factor */
+	unsigned int scale; /* micro lux per count */
+} zopt2201_gain_als[] = {
+	{  1, 19200000 },
+	{  3,  6400000 },
+	{  6,  3200000 },
+	{  9,  2133333 },
+	{ 18,  1066666 },
+};
+
+static const struct {
+	unsigned int gain; /* gain factor */
+	unsigned int scale; /* micro W/cm2 per count */
+} zopt2201_gain_uvb[] = {
+	{  1, 46080000 },
+	{  3, 15360000 },
+	{  6,  7680000 },
+	{  9,  5120000 },
+	{ 18,  2560000 },
+};
+
+static const struct {
+	unsigned int bits; /* sensor resolution in bits */
+	unsigned long us; /* measurement time in micro seconds */
+} zopt2201_resolution[] = {
+	{ 20, 400000 },
+	{ 19, 200000 },
+	{ 18, 100000 },
+	{ 17,  50000 },
+	{ 16,  25000 },
+	{ 13,   3125 },
+};
+
+static const struct {
+	unsigned int scale, uscale; /* scale factor as integer + micro */
+	u8 gain; /* gain register value */
+	u8 res; /* resolution register value */
+} zopt2201_scale_als[] = {
+	{ 19, 200000, 0, 5 },
+	{  6, 400000, 1, 5 },
+	{  3, 200000, 2, 5 },
+	{  2, 400000, 0, 4 },
+	{  2, 133333, 3, 5 },
+	{  1, 200000, 0, 3 },
+	{  1,  66666, 4, 5 },
+	{  0, 800000, 1, 4 },
+	{  0, 600000, 0, 2 },
+	{  0, 400000, 2, 4 },
+	{  0, 300000, 0, 1 },
+	{  0, 266666, 3, 4 },
+	{  0, 200000, 2, 3 },
+	{  0, 150000, 0, 0 },
+	{  0, 133333, 4, 4 },
+	{  0, 100000, 2, 2 },
+	{  0,  66666, 4, 3 },
+	{  0,  50000, 2, 1 },
+	{  0,  33333, 4, 2 },
+	{  0,  25000, 2, 0 },
+	{  0,  16666, 4, 1 },
+	{  0,   8333, 4, 0 },
+};
+
+static const struct {
+	unsigned int scale, uscale; /* scale factor as integer + micro */
+	u8 gain; /* gain register value */
+	u8 res; /* resolution register value */
+} zopt2201_scale_uvb[] = {
+	{ 46,  80000, 0, 5 },
+	{ 15, 360000, 1, 5 },
+	{  7, 680000, 2, 5 },
+	{  5, 760000, 0, 4 },
+	{  5, 120000, 3, 5 },
+	{  2, 880000, 0, 3 },
+	{  2, 560000, 4, 5 },
+	{  1, 920000, 1, 4 },
+	{  1, 440000, 0, 2 },
+	{  0, 960000, 2, 4 },
+	{  0, 720000, 0, 1 },
+	{  0, 640000, 3, 4 },
+	{  0, 480000, 2, 3 },
+	{  0, 360000, 0, 0 },
+	{  0, 320000, 4, 4 },
+	{  0, 240000, 2, 2 },
+	{  0, 160000, 4, 3 },
+	{  0, 120000, 2, 1 },
+	{  0,  80000, 4, 2 },
+	{  0,  60000, 2, 0 },
+	{  0,  40000, 4, 1 },
+	{  0,  20000, 4, 0 },
+};
+
+static int zopt2201_enable_mode(struct zopt2201_data *data,
+				enum zopt2201_mode mode)
+{
+	u8 out = ZOPT2201_MAIN_CTRL_LS_EN;
+
+	if (mode == ZOPT2201_MODE_UVB)
+		out |= ZOPT2201_MAIN_CTRL_LS_MODE;
+
+	return i2c_smbus_write_byte_data(data->client, ZOPT2201_MAIN_CTRL, out);
+}
+
+static int zopt2201_read(struct zopt2201_data *data, enum zopt2201_mode mode)
+{
+	struct i2c_client *client = data->client;
+	int tries = 10;
+	u8 buf[3];
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = zopt2201_enable_mode(data, mode);
+	if (ret < 0)
+		goto fail;
+
+	while (tries--) {
+		unsigned long t = zopt2201_resolution[data->res].us;
+
+		if (t <= 20000)
+			usleep_range(t, t + 1000);
+		else
+			msleep(t / 1000);
+		ret = i2c_smbus_read_byte_data(client, ZOPT2201_MAIN_STATUS);
+		if (ret < 0)
+			goto fail;
+		if (ret & ZOPT2201_MAIN_STATUS_DRDY)
+			break;
+	}
+
+	if (tries < 0) {
+		ret = -ETIMEDOUT;
+		goto fail;
+	}
+
+	ret = i2c_smbus_read_i2c_block_data(client, mode == ZOPT2201_MODE_UVB ?
+		ZOPT2201_UVB_DATA : ZOPT2201_ALS_DATA, sizeof(buf), buf);
+	if (ret < 0)
+		goto fail;
+
+	ret = i2c_smbus_write_byte_data(client, ZOPT2201_MAIN_CTRL, 0x00);
+	if (ret < 0)
+		goto fail;
+	mutex_unlock(&data->lock);
+
+	return (buf[2] << 16) | (buf[1] << 8) | buf[0];
+
+fail:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static const struct iio_chan_spec zopt2201_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.address = ZOPT2201_MODE_ALS,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+	{
+		.type = IIO_INTENSITY,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_UV,
+		.address = ZOPT2201_MODE_UVB,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+	{
+		.type = IIO_UVINDEX,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
+	},
+};
+
+static int zopt2201_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct zopt2201_data *data = iio_priv(indio_dev);
+	u64 tmp;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = zopt2201_read(data, chan->address);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = zopt2201_read(data, ZOPT2201_MODE_UVB);
+		if (ret < 0)
+			return ret;
+		*val = ret * 18 *
+			(1 << (20 - zopt2201_resolution[data->res].bits)) /
+			zopt2201_gain_uvb[data->gain].gain;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+		case ZOPT2201_MODE_ALS:
+			*val = zopt2201_gain_als[data->gain].scale;
+			break;
+		case ZOPT2201_MODE_UVB:
+			*val = zopt2201_gain_uvb[data->gain].scale;
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		*val2 = 1000000;
+		*val2 *= (1 << (zopt2201_resolution[data->res].bits - 13));
+		tmp = div_s64(*val * 1000000ULL, *val2);
+		*val = div_s64_rem(tmp, 1000000, val2);
+
+		return IIO_VAL_INT_PLUS_MICRO;
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		*val2 = zopt2201_resolution[data->res].us;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int zopt2201_set_resolution(struct zopt2201_data *data, u8 res)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, ZOPT2201_LS_MEAS_RATE,
+					(res << ZOPT2201_MEAS_RES_SHIFT) |
+					data->rate);
+	if (ret >= 0)
+		data->res = res;
+
+	return ret;
+}
+
+static int zopt2201_write_resolution(struct zopt2201_data *data,
+				     int val, int val2)
+{
+	int i, ret;
+
+	if (val != 0)
+		return -EINVAL;
+
+	for (i = 0; i < ARRAY_SIZE(zopt2201_resolution); i++)
+		if (val2 == zopt2201_resolution[i].us) {
+			mutex_lock(&data->lock);
+			ret = zopt2201_set_resolution(data, i);
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+
+	return -EINVAL;
+}
+
+static int zopt2201_set_gain(struct zopt2201_data *data, u8 gain)
+{
+	int ret;
+
+	ret = i2c_smbus_write_byte_data(data->client, ZOPT2201_LS_GAIN, gain);
+	if (ret >= 0)
+		data->gain = gain;
+
+	return ret;
+}
+
+static int zopt2201_write_scale_als(struct zopt2201_data *data,
+				     int val, int val2)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
+		if (val == zopt2201_scale_als[i].scale &&
+		    val2 == zopt2201_scale_als[i].uscale) {
+
+			mutex_lock(&data->lock);
+			ret = zopt2201_set_resolution(data,
+				zopt2201_scale_als[i].res);
+			if (ret < 0)
+				goto fail;
+			ret = zopt2201_set_gain(data,
+				zopt2201_scale_als[i].gain);
+
+fail:
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+
+	return -EINVAL;
+}
+
+static int zopt2201_write_scale_uvb(struct zopt2201_data *data,
+				     int val, int val2)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_uvb); i++)
+		if (val == zopt2201_scale_uvb[i].scale &&
+		    val2 == zopt2201_scale_uvb[i].uscale) {
+			mutex_lock(&data->lock);
+			ret = zopt2201_set_resolution(data,
+				zopt2201_scale_uvb[i].res);
+			if (ret < 0)
+				goto fail;
+			ret = zopt2201_set_gain(data,
+				zopt2201_scale_uvb[i].gain);
+
+fail:
+			mutex_unlock(&data->lock);
+			return ret;
+		}
+
+	return -EINVAL;
+}
+
+static int zopt2201_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct zopt2201_data *data = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return zopt2201_write_resolution(data, val, val2);
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->address) {
+		case ZOPT2201_MODE_ALS:
+			return zopt2201_write_scale_als(data, val, val2);
+		case ZOPT2201_MODE_UVB:
+			return zopt2201_write_scale_uvb(data, val, val2);
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static ssize_t zopt2201_show_int_time_available(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	size_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(zopt2201_resolution); i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "0.%06lu ",
+				 zopt2201_resolution[i].us);
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEV_ATTR_INT_TIME_AVAIL(zopt2201_show_int_time_available);
+
+
+static ssize_t zopt2201_show_als_scale_avail(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_als); i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
+				 zopt2201_scale_als[i].scale,
+				 zopt2201_scale_als[i].uscale);
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t zopt2201_show_uvb_scale_avail(struct device *dev,
+					     struct device_attribute *attr,
+					     char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(zopt2201_scale_uvb); i++)
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06u ",
+				 zopt2201_scale_uvb[i].scale,
+				 zopt2201_scale_uvb[i].uscale);
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(in_illuminance_scale_available, 0444,
+		       zopt2201_show_als_scale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(in_intensity_uv_scale_available, 0444,
+		       zopt2201_show_uvb_scale_avail, NULL, 0);
+
+static struct attribute *zopt2201_attributes[] = {
+	&iio_dev_attr_integration_time_available.dev_attr.attr,
+	&iio_dev_attr_in_illuminance_scale_available.dev_attr.attr,
+	&iio_dev_attr_in_intensity_uv_scale_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group zopt2201_attribute_group = {
+	.attrs = zopt2201_attributes,
+};
+
+
+static const struct iio_info zopt2201_info = {
+	.read_raw = zopt2201_read_raw,
+	.write_raw = zopt2201_write_raw,
+	.attrs = &zopt2201_attribute_group,
+};
+
+static int zopt2201_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct zopt2201_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	if (!i2c_check_functionality(client->adapter,
+				     I2C_FUNC_SMBUS_READ_I2C_BLOCK))
+		return -EOPNOTSUPP;
+
+	ret = i2c_smbus_read_byte_data(client, ZOPT2201_PART_ID);
+	if (ret < 0)
+		return ret;
+	if (ret != ZOPT2201_PART_NUMBER)
+		return -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	mutex_init(&data->lock);
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &zopt2201_info;
+	indio_dev->channels = zopt2201_channels;
+	indio_dev->num_channels = ARRAY_SIZE(zopt2201_channels);
+	indio_dev->name = ZOPT2201_DRV_NAME;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	data->rate = ZOPT2201_MEAS_FREQ_100MS;
+	ret = zopt2201_set_resolution(data, ZOPT2201_MEAS_RES_18BIT);
+	if (ret < 0)
+		return ret;
+
+	ret = zopt2201_set_gain(data, ZOPT2201_LS_GAIN_3);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id zopt2201_id[] = {
+	{ "zopt2201", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, zopt2201_id);
+
+static struct i2c_driver zopt2201_driver = {
+	.driver = {
+		.name   = ZOPT2201_DRV_NAME,
+	},
+	.probe  = zopt2201_probe,
+	.id_table = zopt2201_id,
+};
+
+module_i2c_driver(zopt2201_driver);
+
+MODULE_AUTHOR("Peter Meerwald-Stadler <pmeerw@pmeerw.net>");
+MODULE_DESCRIPTION("IDT ZOPT2201 ambient light and UV B sensor driver");
+MODULE_LICENSE("GPL");