iio: light: lv0104cs: Add support for LV0104CS light sensor

PATCH Superseded Jeff LaBundy 3 participants 9 messages Feb 14, 2018 – Mar 3, 2018 IIO

lore

Share & tools

lore

Jeff LaBundy

author

2018-02-14 14:58 UTC #0 lore raw

PATCH 1/1 iio: light: lv0104cs: Add support for LV0104CS light sensor

This patch adds support for the On Semiconductor LV0104CS ambient light sensor.

Signed-off-by Jeff LaBundy <[email protected]>

---
 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/lv0104cs.c | 559 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+)
 create mode 100644 drivers/iio/light/lv0104cs.c

drivers/iio/light/Kconfig +10 -0

index 2356ed9..ca8918e 100644
@@ -275,6 +275,16 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config LV0104CS
+	tristate "LV0104CS Ambient Light Sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build support for the LV0104CS ambient
+	 light sensor.
+
+	 To compile this driver as a module, choose M here:
+	 the module will be called lv0104cs.
+
 config MAX44000
 	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index fa32fa4..77c8682 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o

drivers/iio/light/lv0104cs.c new file +559 -0

559 lines not on this page · raw message

Signature · 1 line

2.7.4

Peter Meerwald-Stadler

2018-02-14 15:58 UTC #1 lore raw

Quoted 3 lines

This patch adds support for the On Semiconductor LV0104CS ambient light sensor.

nice little driver, some comments below

Quoted 13 lines

Signed-off-by Jeff LaBundy <[email protected]>

---
 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/lv0104cs.c | 559 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+)
 create mode 100644 drivers/iio/light/lv0104cs.c

drivers/iio/light/Kconfig +0 -0

index 2356ed9..ca8918e 100644

drivers/iio/light/Kconfig config LTR501 · line 283

Quoted 9 lines

drivers/iio/light/Kconfig +6 -0

@@ -275,6 +275,16 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config LV0104CS
+	tristate "LV0104CS Ambient Light Sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build support for the LV0104CS ambient
+	 light sensor.

maybe mention On Semi somewhere?

Quoted 33 lines

drivers/iio/light/Kconfig +4 -0

+
+	 To compile this driver as a module, choose M here:
+	 the module will be called lv0104cs.
+
 config MAX44000
 	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index fa32fa4..77c8682 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o

drivers/iio/light/lv0104cs.c new file +8 -0

new file mode 100644
index 0000000..ecbba39
@@ -0,0 +1,559 @@
+/*
+ * lv0104cs.c: LV0104CS Ambient Light Sensor Driver
+ *
+ * Copyright (C) 2018 Jeff LaBundy <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 of the License
+ * as published by the Free Software Foundation.

drivers/iio/light/lv0104cs.c line 18

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * 7-bit I2C slave address: 0x13
+ *
+ * This device has just one register and it is write-only. Read operations are
+ * limited to the 16-bit ADC output.

as simple as it gets :)

a link to the documentation would be nice

Quoted 12 lines

drivers/iio/light/lv0104cs.c new file +12 -0

+ *
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define LV0104CS_REGVAL_MEASURE		0xE0

drivers/iio/light/lv0104cs.c line 40

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+#define LV0104CS_REGVAL_SLEEP		0x00
+
+#define LV0104CS_HWGAIN_0x25		0
+#define LV0104CS_HWGAIN_1x		1
+#define LV0104CS_HWGAIN_2x		2
+#define LV0104CS_HWGAIN_8x		3
+
+#define LV0104CS_INTEG_12m5		0
+#define LV0104CS_INTEG_100m		1
+#define LV0104CS_INTEG_200m		2

are these milliseconds? maybe annotate

Quoted 79 lines

drivers/iio/light/lv0104cs.c new file +79 -0

+
+#define LV0104CS_CALIB_UNITY		31
+
+struct lv0104cs_private {
+	struct i2c_client *client;
+	struct mutex lock;
+	unsigned char hardwaregain;
+	unsigned char int_time;
+	unsigned char calibscale;
+};
+
+struct lv0104cs_mapping {
+	int val;
+	int val2;
+	unsigned char regval;
+};
+
+static const struct lv0104cs_mapping lv0104cs_calibscales[] = {
+	{ 0, 666666, 0x81 },
+	{ 0, 800000, 0x82 },
+	{ 0, 857142, 0x83 },
+	{ 0, 888888, 0x84 },
+	{ 0, 909090, 0x85 },
+	{ 0, 923076, 0x86 },
+	{ 0, 933333, 0x87 },
+	{ 0, 941176, 0x88 },
+	{ 0, 947368, 0x89 },
+	{ 0, 952380, 0x8A },
+	{ 0, 956521, 0x8B },
+	{ 0, 960000, 0x8C },
+	{ 0, 962962, 0x8D },
+	{ 0, 965517, 0x8E },
+	{ 0, 967741, 0x8F },
+	{ 0, 969696, 0x90 },
+	{ 0, 971428, 0x91 },
+	{ 0, 972972, 0x92 },
+	{ 0, 974358, 0x93 },
+	{ 0, 975609, 0x94 },
+	{ 0, 976744, 0x95 },
+	{ 0, 977777, 0x96 },
+	{ 0, 978723, 0x97 },
+	{ 0, 979591, 0x98 },
+	{ 0, 980392, 0x99 },
+	{ 0, 981132, 0x9A },
+	{ 0, 981818, 0x9B },
+	{ 0, 982456, 0x9C },
+	{ 0, 983050, 0x9D },
+	{ 0, 983606, 0x9E },
+	{ 0, 984126, 0x9F },
+	{ 1, 0, 0x80 },
+	{ 1, 16129, 0xBF },
+	{ 1, 16666, 0xBE },
+	{ 1, 17241, 0xBD },
+	{ 1, 17857, 0xBC },
+	{ 1, 18518, 0xBB },
+	{ 1, 19230, 0xBA },
+	{ 1, 20000, 0xB9 },
+	{ 1, 20833, 0xB8 },
+	{ 1, 21739, 0xB7 },
+	{ 1, 22727, 0xB6 },
+	{ 1, 23809, 0xB5 },
+	{ 1, 24999, 0xB4 },
+	{ 1, 26315, 0xB3 },
+	{ 1, 27777, 0xB2 },
+	{ 1, 29411, 0xB1 },
+	{ 1, 31250, 0xB0 },
+	{ 1, 33333, 0xAF },
+	{ 1, 35714, 0xAE },
+	{ 1, 38461, 0xAD },
+	{ 1, 41666, 0xAC },
+	{ 1, 45454, 0xAB },
+	{ 1, 50000, 0xAA },
+	{ 1, 55555, 0xA9 },
+	{ 1, 62500, 0xA8 },
+	{ 1, 71428, 0xA7 },
+	{ 1, 83333, 0xA6 },
+	{ 1, 100000, 0xA5 },
+	{ 1, 125000, 0xA4 },
+	{ 1, 166666, 0xA3 },

drivers/iio/light/lv0104cs.c line 129

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+	{ 1, 250000, 0xA2 },
+	{ 1, 500000, 0xA1 },
+};
+
+static const struct lv0104cs_mapping lv0104cs_hardwaregains[] = {
+	{ 0, 250000, 0x00 },
+	{ 1, 0, 0x08 },
+	{ 2, 0, 0x10 },
+	{ 8, 0, 0x18 },
+};

it would be nice if the LV0104CS_HWGAIN_ #defines could be used here, aren't the values 0x00 .. 0x18 just LV0104CS_HWGAIN_xx << 3?

drivers/iio/light/lv0104cs.c line 135

Quoted 6 lines

drivers/iio/light/lv0104cs.c new file +6 -0

+
+static const struct lv0104cs_mapping lv0104cs_int_times[] = {
+	{ 0, 12500, 0x00 },
+	{ 0, 100000, 0x02 },
+	{ 0, 200000, 0x04 },
+};

maybe something similar here for LV0104CS_INTEG

drivers/iio/light/lv0104cs.c line 143

Quoted 8 lines

drivers/iio/light/lv0104cs.c new file +8 -0

+
+static int lv0104cs_write_reg(struct lv0104cs_private *lv0104cs,
+				unsigned char regval)
+{
+	struct i2c_client *client = lv0104cs->client;
+	int ret;
+
+	ret = i2c_master_send(client, &regval, 1);

maybe sizeof(regval) instead of 1

drivers/iio/light/lv0104cs.c line 156

Quoted 13 lines

drivers/iio/light/lv0104cs.c new file +13 -0

+	if (ret != 1) {
+		dev_err(&client->dev, "Failed to write to device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_adc(struct lv0104cs_private *lv0104cs,
+				unsigned int *adc_output)
+{
+	struct i2c_client *client = lv0104cs->client;
+	unsigned char regval[2];

use
__be16 regval;

drivers/iio/light/lv0104cs.c line 159

Quoted 3 lines

drivers/iio/light/lv0104cs.c new file +3 -0
+	int ret;
+
+	ret = i2c_master_recv(client, regval, 2);

sizeof(regval)

drivers/iio/light/lv0104cs.c line 165

Quoted 6 lines

drivers/iio/light/lv0104cs.c new file +6 -0

+	if (ret != 2) {
+		dev_err(&client->dev, "Failed to read from device: %d\n", ret);
+		return ret;
+	}
+
+	*adc_output = (regval[0] << 8) + regval[1];

use be16_to_cpu()

Quoted 151 lines

drivers/iio/light/lv0104cs.c new file +151 -0

+
+	return 0;
+}
+
+static int lv0104cs_get_lux(struct lv0104cs_private *lv0104cs,
+				unsigned int *val, unsigned int *val2)
+{
+	unsigned char regval = LV0104CS_REGVAL_MEASURE;
+	unsigned int adc_output;
+	int ret;
+
+	/* this function expects to be called while mutex is locked */
+	if (!mutex_is_locked(&lv0104cs->lock))
+		return -EACCES;
+
+	regval |= lv0104cs_hardwaregains[lv0104cs->hardwaregain].regval;
+	regval |= lv0104cs_int_times[lv0104cs->int_time].regval;
+	ret = lv0104cs_write_reg(lv0104cs, regval);
+	if (ret)
+		return ret;
+
+	/* wait for integration time to pass (with margin) */
+	switch (lv0104cs->int_time) {
+	case LV0104CS_INTEG_12m5:
+		msleep(50);
+		break;
+
+	case LV0104CS_INTEG_100m:
+		msleep(150);
+		break;
+
+	case LV0104CS_INTEG_200m:
+		msleep(250);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = lv0104cs_read_adc(lv0104cs, &adc_output);
+	if (ret)
+		return ret;
+
+	ret = lv0104cs_write_reg(lv0104cs, LV0104CS_REGVAL_SLEEP);
+	if (ret)
+		return ret;
+
+	/* normalize to lux based on applied gain */
+	switch (lv0104cs->hardwaregain) {
+	case LV0104CS_HWGAIN_0x25:
+		*val = adc_output * 4;
+		*val2 = 0;
+		break;
+
+	case LV0104CS_HWGAIN_1x:
+		*val = adc_output;
+		*val2 = 0;
+		break;
+
+	case LV0104CS_HWGAIN_2x:
+		*val = adc_output / 2;
+		*val2 = (adc_output % 2) * 500000;
+		break;
+
+	case LV0104CS_HWGAIN_8x:
+		*val = adc_output / 8;
+		*val2 = (adc_output % 8) * 125000;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = lv0104cs_get_lux(lv0104cs, val, val2);
+		if (ret)
+			goto err_mutex;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*val = lv0104cs_calibscales[lv0104cs->calibscale].val;
+		*val2 = lv0104cs_calibscales[lv0104cs->calibscale].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*val = lv0104cs_hardwaregains[lv0104cs->hardwaregain].val;
+		*val2 = lv0104cs_hardwaregains[lv0104cs->hardwaregain].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = lv0104cs_int_times[lv0104cs->int_time].val;
+		*val2 = lv0104cs_int_times[lv0104cs->int_time].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+err_mutex:
+	mutex_unlock(&lv0104cs->lock);
+
+	return ret;
+}
+
+static int lv0104cs_set_calibscale(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int calibscale = val * 1000000 + val2;
+	int floor, ceil, mid;
+	int ret, i, index;
+
+	/* round to nearest quantized sensitivity */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales) - 1; i++) {
+		floor = lv0104cs_calibscales[i].val * 1000000
+				+ lv0104cs_calibscales[i].val2;
+		ceil = lv0104cs_calibscales[i + 1].val * 1000000
+				+ lv0104cs_calibscales[i + 1].val2;
+		mid = (floor + ceil) / 2;
+
+		/* round down */
+		if (calibscale >= floor && calibscale < mid) {
+			index = i;
+			break;
+		}
+
+		/* round up */
+		if (calibscale >= mid && calibscale <= ceil) {
+			index = i + 1;
+			break;
+		}
+	}

drivers/iio/light/lv0104cs.c line 326

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+
+	if (i == ARRAY_SIZE(lv0104cs_calibscales) - 1)
+		return -EINVAL;
+
+	/* set sensitivity */
+	ret = lv0104cs_write_reg(lv0104cs, lv0104cs_calibscales[index].regval);
+	if (ret)
+		return ret;
+
+	mutex_lock(&lv0104cs->lock);

shoudn't the lock region include the set sensitivity above?

Quoted 154 lines

drivers/iio/light/lv0104cs.c new file +154 -0

+	lv0104cs->calibscale = index;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_hardwaregain(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_hardwaregains); i++) {
+		if (val != lv0104cs_hardwaregains[i].val)
+			continue;
+
+		if (val2 == lv0104cs_hardwaregains[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_hardwaregains))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->hardwaregain = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_int_time(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		if (val != lv0104cs_int_times[i].val)
+			continue;
+
+		if (val2 == lv0104cs_int_times[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_int_times))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->int_time = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_write_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = lv0104cs_set_calibscale(lv0104cs, val, val2);
+		break;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		ret = lv0104cs_set_hardwaregain(lv0104cs, val, val2);
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = lv0104cs_set_int_time(lv0104cs, val, val2);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static ssize_t lv0104cs_show_calibscale_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_calibscales[i].val,
+				lv0104cs_calibscales[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_hardwaregain_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_hardwaregains); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_hardwaregains[i].val,
+				lv0104cs_hardwaregains[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_int_time_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_int_times[i].val,
+				lv0104cs_int_times[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(calibscale_available, 0444,
+				lv0104cs_show_calibscale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(hardwaregain_available, 0444,
+				lv0104cs_show_hardwaregain_avail, NULL, 0);
+static IIO_DEV_ATTR_INT_TIME_AVAIL(lv0104cs_show_int_time_avail);
+
+static struct attribute *lv0104cs_attributes[] = {
+	&iio_dev_attr_calibscale_available.dev_attr.attr,
+	&iio_dev_attr_hardwaregain_available.dev_attr.attr,
+	&iio_dev_attr_integration_time_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lv0104cs_attribute_group = {
+	.attrs = lv0104cs_attributes,
+};
+
+static const struct iio_info lv0104cs_info = {
+	.attrs = &lv0104cs_attribute_group,

drivers/iio/light/lv0104cs.c line 490

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+	.read_raw = &lv0104cs_read_raw,
+	.write_raw = &lv0104cs_write_raw,
+};
+
+static const struct iio_chan_spec lv0104cs_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_CALIBSCALE) |
+				      BIT(IIO_CHAN_INFO_HARDWAREGAIN) |

I think this should be just _SCALE; this is not about calibration, but just sets a gain factor

I don't quite understand the need/difference between CALIBSCALE / HARDWAREGAIN

Quoted 36 lines

drivers/iio/light/lv0104cs.c new file +36 -0

+				      BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+};
+
+static int lv0104cs_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct device *dev = &client->dev;
+	struct lv0104cs_private *lv0104cs;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct lv0104cs_private));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	lv0104cs = iio_priv(indio_dev);
+
+	i2c_set_clientdata(client, lv0104cs);
+	lv0104cs->client = client;
+
+	mutex_init(&lv0104cs->lock);
+
+	lv0104cs->hardwaregain = LV0104CS_HWGAIN_1x;
+	lv0104cs->int_time = LV0104CS_INTEG_200m;
+	lv0104cs->calibscale = LV0104CS_CALIB_UNITY;
+
+	ret = lv0104cs_write_reg(lv0104cs,
+			lv0104cs_calibscales[LV0104CS_CALIB_UNITY].regval);
+	if (ret)
+		return -ENODEV;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.parent = dev;
+	indio_dev->channels = lv0104cs_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lv0104cs_channels);

drivers/iio/light/lv0104cs.c line 536

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+	indio_dev->name = client->name;
+	indio_dev->info = &lv0104cs_info;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret) {
+		dev_err(dev, "Failed to register device: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "Successfully registered device\n");

please drop this output, considered just clutter

Quoted 24 lines

drivers/iio/light/lv0104cs.c new file +23 -0

+
+	return 0;
+}
+
+static const struct i2c_device_id lv0104cs_id[] = {
+	{ "lv0104cs", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lv0104cs_id);
+
+static struct i2c_driver lv0104cs_i2c_driver = {
+	.driver = {
+		.name	= "lv0104cs",
+	},
+
+	.id_table	= lv0104cs_id,
+	.probe		= lv0104cs_probe,
+};
+module_i2c_driver(lv0104cs_i2c_driver);
+
+MODULE_AUTHOR("Jeff LaBundy <[email protected]>");
+MODULE_DESCRIPTION("LV0104CS Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL v2");

Signature · 2 lines

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Jeff LaBundy

author

2018-02-15 04:46 UTC #2 lore raw

Quoted 5 lines · Peter Meerwald-Stadler

Quoted 3 lines
This patch adds support for the On Semiconductor LV0104CS ambient light sensor.
nice little driver, some comments below

Thank you very much for the review; I agree with your feedback and will submit a v2. Comments and answers in line.

Quoted 17 lines

Signed-off-by Jeff LaBundy <[email protected]>

---
 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/lv0104cs.c | 559 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+)
 create mode 100644 drivers/iio/light/lv0104cs.c

drivers/iio/light/Kconfig +0 -0

index 2356ed9..ca8918e 100644
@@ -275,6 +275,16 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.

Quoted 8 lines

drivers/iio/light/Kconfig config LTR501 · line 283
Quoted 6 lines
drivers/iio/light/Kconfig +6 -0
+config LV0104CS
+	tristate "LV0104CS Ambient Light Sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build support for the LV0104CS ambient
+	 light sensor.
maybe mention On Semi somewhere?

Sure thing, will do.

Quoted 47 lines

Quoted 34 lines

drivers/iio/light/Kconfig +4 -0

+
+	 To compile this driver as a module, choose M here:
+	 the module will be called lv0104cs.
+
 config MAX44000
 	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index fa32fa4..77c8682 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o

drivers/iio/light/lv0104cs.c new file +8 -0

new file mode 100644
index 0000000..ecbba39
@@ -0,0 +1,559 @@
+/*
+ * lv0104cs.c: LV0104CS Ambient Light Sensor Driver
+ *
+ * Copyright (C) 2018 Jeff LaBundy <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 of the License
+ * as published by the Free Software Foundation.

drivers/iio/light/lv0104cs.c line 18

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * 7-bit I2C slave address: 0x13
+ *
+ * This device has just one register and it is write-only. Read operations are
+ * limited to the 16-bit ADC output.

as simple as it gets :)

Quoted 1 line

a link to the documentation would be nice

Sure thing, will do.

Quoted 17 lines

drivers/iio/light/lv0104cs.c new file +16 -0

+ *
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define LV0104CS_REGVAL_MEASURE		0xE0
+#define LV0104CS_REGVAL_SLEEP		0x00
+
+#define LV0104CS_HWGAIN_0x25		0
+#define LV0104CS_HWGAIN_1x		1

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 40
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+#define LV0104CS_HWGAIN_2x		2
+#define LV0104CS_HWGAIN_8x		3
+
+#define LV0104CS_INTEG_12m5		0
+#define LV0104CS_INTEG_100m		1
+#define LV0104CS_INTEG_200m		2
are these milliseconds? maybe annotate

They are, and I will.

Quoted 85 lines

drivers/iio/light/lv0104cs.c new file +84 -0

+
+#define LV0104CS_CALIB_UNITY		31
+
+struct lv0104cs_private {
+	struct i2c_client *client;
+	struct mutex lock;
+	unsigned char hardwaregain;
+	unsigned char int_time;
+	unsigned char calibscale;
+};
+
+struct lv0104cs_mapping {
+	int val;
+	int val2;
+	unsigned char regval;
+};
+
+static const struct lv0104cs_mapping lv0104cs_calibscales[] = {
+	{ 0, 666666, 0x81 },
+	{ 0, 800000, 0x82 },
+	{ 0, 857142, 0x83 },
+	{ 0, 888888, 0x84 },
+	{ 0, 909090, 0x85 },
+	{ 0, 923076, 0x86 },
+	{ 0, 933333, 0x87 },
+	{ 0, 941176, 0x88 },
+	{ 0, 947368, 0x89 },
+	{ 0, 952380, 0x8A },
+	{ 0, 956521, 0x8B },
+	{ 0, 960000, 0x8C },
+	{ 0, 962962, 0x8D },
+	{ 0, 965517, 0x8E },
+	{ 0, 967741, 0x8F },
+	{ 0, 969696, 0x90 },
+	{ 0, 971428, 0x91 },
+	{ 0, 972972, 0x92 },
+	{ 0, 974358, 0x93 },
+	{ 0, 975609, 0x94 },
+	{ 0, 976744, 0x95 },
+	{ 0, 977777, 0x96 },
+	{ 0, 978723, 0x97 },
+	{ 0, 979591, 0x98 },
+	{ 0, 980392, 0x99 },
+	{ 0, 981132, 0x9A },
+	{ 0, 981818, 0x9B },
+	{ 0, 982456, 0x9C },
+	{ 0, 983050, 0x9D },
+	{ 0, 983606, 0x9E },
+	{ 0, 984126, 0x9F },
+	{ 1, 0, 0x80 },
+	{ 1, 16129, 0xBF },
+	{ 1, 16666, 0xBE },
+	{ 1, 17241, 0xBD },
+	{ 1, 17857, 0xBC },
+	{ 1, 18518, 0xBB },
+	{ 1, 19230, 0xBA },
+	{ 1, 20000, 0xB9 },
+	{ 1, 20833, 0xB8 },
+	{ 1, 21739, 0xB7 },
+	{ 1, 22727, 0xB6 },
+	{ 1, 23809, 0xB5 },
+	{ 1, 24999, 0xB4 },
+	{ 1, 26315, 0xB3 },
+	{ 1, 27777, 0xB2 },
+	{ 1, 29411, 0xB1 },
+	{ 1, 31250, 0xB0 },
+	{ 1, 33333, 0xAF },
+	{ 1, 35714, 0xAE },
+	{ 1, 38461, 0xAD },
+	{ 1, 41666, 0xAC },
+	{ 1, 45454, 0xAB },
+	{ 1, 50000, 0xAA },
+	{ 1, 55555, 0xA9 },
+	{ 1, 62500, 0xA8 },
+	{ 1, 71428, 0xA7 },
+	{ 1, 83333, 0xA6 },
+	{ 1, 100000, 0xA5 },
+	{ 1, 125000, 0xA4 },
+	{ 1, 166666, 0xA3 },
+	{ 1, 250000, 0xA2 },
+	{ 1, 500000, 0xA1 },
+};
+
+static const struct lv0104cs_mapping lv0104cs_hardwaregains[] = {

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 129
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+	{ 0, 250000, 0x00 },
+	{ 1, 0, 0x08 },
+	{ 2, 0, 0x10 },
+	{ 8, 0, 0x18 },
+};
it would be nice if the LV0104CS_HWGAIN_ #defines could be used here, aren't the values 0x00 .. 0x18 just LV0104CS_HWGAIN_xx << 3?

Excellent idea, will do.

Quoted 9 lines

drivers/iio/light/lv0104cs.c line 135
Quoted 7 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+
+static const struct lv0104cs_mapping lv0104cs_int_times[] = {
+	{ 0, 12500, 0x00 },
+	{ 0, 100000, 0x02 },
+	{ 0, 200000, 0x04 },
+};
maybe something similar here for LV0104CS_INTEG

Excellent idea, will do.

Quoted 11 lines

drivers/iio/light/lv0104cs.c line 143
Quoted 9 lines
drivers/iio/light/lv0104cs.c new file +8 -0
+
+static int lv0104cs_write_reg(struct lv0104cs_private *lv0104cs,
+				unsigned char regval)
+{
+	struct i2c_client *client = lv0104cs->client;
+	int ret;
+
+	ret = i2c_master_send(client, &regval, 1);
maybe sizeof(regval) instead of 1

Agreed, will do.

Quoted 17 lines

Quoted 4 lines

drivers/iio/light/lv0104cs.c new file +3 -0

+	if (ret != 1) {
+		dev_err(&client->dev, "Failed to write to device: %d\n", ret);
+		return ret;

drivers/iio/light/lv0104cs.c line 156

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_adc(struct lv0104cs_private *lv0104cs,
+				unsigned int *adc_output)
+{
+	struct i2c_client *client = lv0104cs->client;
+	unsigned char regval[2];

use
__be16 regval;

Sure thing, will do.

Quoted 6 lines

drivers/iio/light/lv0104cs.c line 159
Quoted 4 lines
drivers/iio/light/lv0104cs.c new file +3 -0
+	int ret;
+
+	ret = i2c_master_recv(client, regval, 2);
sizeof(regval)

Agreed, will do.

Quoted 9 lines

drivers/iio/light/lv0104cs.c line 165
Quoted 7 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+	if (ret != 2) {
+		dev_err(&client->dev, "Failed to read from device: %d\n", ret);
+		return ret;
+	}
+
+	*adc_output = (regval[0] << 8) + regval[1];
use be16_to_cpu()

Sure thing, will do.

Quoted 156 lines

drivers/iio/light/lv0104cs.c new file +155 -0

+
+	return 0;
+}
+
+static int lv0104cs_get_lux(struct lv0104cs_private *lv0104cs,
+				unsigned int *val, unsigned int *val2)
+{
+	unsigned char regval = LV0104CS_REGVAL_MEASURE;
+	unsigned int adc_output;
+	int ret;
+
+	/* this function expects to be called while mutex is locked */
+	if (!mutex_is_locked(&lv0104cs->lock))
+		return -EACCES;
+
+	regval |= lv0104cs_hardwaregains[lv0104cs->hardwaregain].regval;
+	regval |= lv0104cs_int_times[lv0104cs->int_time].regval;
+	ret = lv0104cs_write_reg(lv0104cs, regval);
+	if (ret)
+		return ret;
+
+	/* wait for integration time to pass (with margin) */
+	switch (lv0104cs->int_time) {
+	case LV0104CS_INTEG_12m5:
+		msleep(50);
+		break;
+
+	case LV0104CS_INTEG_100m:
+		msleep(150);
+		break;
+
+	case LV0104CS_INTEG_200m:
+		msleep(250);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = lv0104cs_read_adc(lv0104cs, &adc_output);
+	if (ret)
+		return ret;
+
+	ret = lv0104cs_write_reg(lv0104cs, LV0104CS_REGVAL_SLEEP);
+	if (ret)
+		return ret;
+
+	/* normalize to lux based on applied gain */
+	switch (lv0104cs->hardwaregain) {
+	case LV0104CS_HWGAIN_0x25:
+		*val = adc_output * 4;
+		*val2 = 0;
+		break;
+
+	case LV0104CS_HWGAIN_1x:
+		*val = adc_output;
+		*val2 = 0;
+		break;
+
+	case LV0104CS_HWGAIN_2x:
+		*val = adc_output / 2;
+		*val2 = (adc_output % 2) * 500000;
+		break;
+
+	case LV0104CS_HWGAIN_8x:
+		*val = adc_output / 8;
+		*val2 = (adc_output % 8) * 125000;
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = lv0104cs_get_lux(lv0104cs, val, val2);
+		if (ret)
+			goto err_mutex;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*val = lv0104cs_calibscales[lv0104cs->calibscale].val;
+		*val2 = lv0104cs_calibscales[lv0104cs->calibscale].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*val = lv0104cs_hardwaregains[lv0104cs->hardwaregain].val;
+		*val2 = lv0104cs_hardwaregains[lv0104cs->hardwaregain].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = lv0104cs_int_times[lv0104cs->int_time].val;
+		*val2 = lv0104cs_int_times[lv0104cs->int_time].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+err_mutex:
+	mutex_unlock(&lv0104cs->lock);
+
+	return ret;
+}
+
+static int lv0104cs_set_calibscale(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int calibscale = val * 1000000 + val2;
+	int floor, ceil, mid;
+	int ret, i, index;
+
+	/* round to nearest quantized sensitivity */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales) - 1; i++) {
+		floor = lv0104cs_calibscales[i].val * 1000000
+				+ lv0104cs_calibscales[i].val2;
+		ceil = lv0104cs_calibscales[i + 1].val * 1000000
+				+ lv0104cs_calibscales[i + 1].val2;
+		mid = (floor + ceil) / 2;
+
+		/* round down */
+		if (calibscale >= floor && calibscale < mid) {
+			index = i;
+			break;
+		}
+
+		/* round up */
+		if (calibscale >= mid && calibscale <= ceil) {
+			index = i + 1;
+			break;
+		}
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_calibscales) - 1)
+		return -EINVAL;
+

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 326
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+	/* set sensitivity */
+	ret = lv0104cs_write_reg(lv0104cs, lv0104cs_calibscales[index].regval);
+	if (ret)
+		return ret;
+
+	mutex_lock(&lv0104cs->lock);
shoudn't the lock region include the set sensitivity above?

Good catch, will do.

Quoted 160 lines

drivers/iio/light/lv0104cs.c new file +159 -0

+	lv0104cs->calibscale = index;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_hardwaregain(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_hardwaregains); i++) {
+		if (val != lv0104cs_hardwaregains[i].val)
+			continue;
+
+		if (val2 == lv0104cs_hardwaregains[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_hardwaregains))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->hardwaregain = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_int_time(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		if (val != lv0104cs_int_times[i].val)
+			continue;
+
+		if (val2 == lv0104cs_int_times[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_int_times))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->int_time = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_write_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = lv0104cs_set_calibscale(lv0104cs, val, val2);
+		break;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		ret = lv0104cs_set_hardwaregain(lv0104cs, val, val2);
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = lv0104cs_set_int_time(lv0104cs, val, val2);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static ssize_t lv0104cs_show_calibscale_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_calibscales[i].val,
+				lv0104cs_calibscales[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_hardwaregain_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_hardwaregains); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_hardwaregains[i].val,
+				lv0104cs_hardwaregains[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_int_time_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_int_times[i].val,
+				lv0104cs_int_times[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(calibscale_available, 0444,
+				lv0104cs_show_calibscale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(hardwaregain_available, 0444,
+				lv0104cs_show_hardwaregain_avail, NULL, 0);
+static IIO_DEV_ATTR_INT_TIME_AVAIL(lv0104cs_show_int_time_avail);
+
+static struct attribute *lv0104cs_attributes[] = {
+	&iio_dev_attr_calibscale_available.dev_attr.attr,
+	&iio_dev_attr_hardwaregain_available.dev_attr.attr,
+	&iio_dev_attr_integration_time_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lv0104cs_attribute_group = {
+	.attrs = lv0104cs_attributes,
+};
+
+static const struct iio_info lv0104cs_info = {
+	.attrs = &lv0104cs_attribute_group,
+	.read_raw = &lv0104cs_read_raw,
+	.write_raw = &lv0104cs_write_raw,
+};
+
+static const struct iio_chan_spec lv0104cs_channels[] = {

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 490
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_CALIBSCALE) |
+				      BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
I think this should be just _SCALE; this is not about calibration, but just sets a gain factor

Agreed, will do.

Quoted 3 lines

I don't quite understand the need/difference between CALIBSCALE / HARDWAREGAIN

This device exposes two separate controls: gain (presumably a PGA of sorts in between the photodiode and the ADC) and sensitivity (a cubic trim control). These are mapped to HARDWAREGAIN and CALIBSCALE (now SCALE), respectively.

Quoted 41 lines

drivers/iio/light/lv0104cs.c new file +40 -0

+				      BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+};
+
+static int lv0104cs_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct device *dev = &client->dev;
+	struct lv0104cs_private *lv0104cs;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct lv0104cs_private));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	lv0104cs = iio_priv(indio_dev);
+
+	i2c_set_clientdata(client, lv0104cs);
+	lv0104cs->client = client;
+
+	mutex_init(&lv0104cs->lock);
+
+	lv0104cs->hardwaregain = LV0104CS_HWGAIN_1x;
+	lv0104cs->int_time = LV0104CS_INTEG_200m;
+	lv0104cs->calibscale = LV0104CS_CALIB_UNITY;
+
+	ret = lv0104cs_write_reg(lv0104cs,
+			lv0104cs_calibscales[LV0104CS_CALIB_UNITY].regval);
+	if (ret)
+		return -ENODEV;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.parent = dev;
+	indio_dev->channels = lv0104cs_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lv0104cs_channels);
+	indio_dev->name = client->name;
+	indio_dev->info = &lv0104cs_info;
+
+	ret = devm_iio_device_register(dev, indio_dev);

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 536
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+	if (ret) {
+		dev_err(dev, "Failed to register device: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "Successfully registered device\n");
please drop this output, considered just clutter

Sure thing, will do.

Quoted 31 lines

Quoted 25 lines

drivers/iio/light/lv0104cs.c new file +23 -0

+
+	return 0;
+}
+
+static const struct i2c_device_id lv0104cs_id[] = {
+	{ "lv0104cs", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lv0104cs_id);
+
+static struct i2c_driver lv0104cs_i2c_driver = {
+	.driver = {
+		.name	= "lv0104cs",
+	},
+
+	.id_table	= lv0104cs_id,
+	.probe		= lv0104cs_probe,
+};
+module_i2c_driver(lv0104cs_i2c_driver);
+
+MODULE_AUTHOR("Jeff LaBundy <[email protected]>");
+MODULE_DESCRIPTION("LV0104CS Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL v2");

Signature · 2 lines

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Jonathan Cameron

maintainer

2018-02-17 17:13 UTC #3 lore raw

Quoted 9 lines · On Wed, 14 Feb 2018 22:46:42 -0600 Jeff LaBundy <[email protected]> wrote:

Quoted 5 lines · Peter Meerwald-Stadler
Quoted 3 lines
This patch adds support for the On Semiconductor LV0104CS ambient light sensor.
nice little driver, some comments below
Thank you very much for the review; I agree with your feedback and will submit a v2. Comments and answers in line.

A few more comments from me.

Jonathan

Quoted 209 lines

Quoted 17 lines

Signed-off-by Jeff LaBundy <[email protected]>

---
 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/lv0104cs.c | 559 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+)
 create mode 100644 drivers/iio/light/lv0104cs.c

drivers/iio/light/Kconfig +0 -0

index 2356ed9..ca8918e 100644
@@ -275,6 +275,16 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.

Quoted 8 lines

drivers/iio/light/Kconfig config LTR501 · line 283
Quoted 6 lines
drivers/iio/light/Kconfig +6 -0
+config LV0104CS
+	tristate "LV0104CS Ambient Light Sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build support for the LV0104CS ambient
+	 light sensor.  
maybe mention On Semi somewhere?

Sure thing, will do.

Quoted 47 lines

Quoted 34 lines

drivers/iio/light/Kconfig +4 -0

+
+	 To compile this driver as a module, choose M here:
+	 the module will be called lv0104cs.
+
 config MAX44000
 	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index fa32fa4..77c8682 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o

drivers/iio/light/lv0104cs.c new file +8 -0

new file mode 100644
index 0000000..ecbba39
@@ -0,0 +1,559 @@
+/*
+ * lv0104cs.c: LV0104CS Ambient Light Sensor Driver
+ *
+ * Copyright (C) 2018 Jeff LaBundy <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 of the License
+ * as published by the Free Software Foundation.

drivers/iio/light/lv0104cs.c line 18

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * 7-bit I2C slave address: 0x13
+ *
+ * This device has just one register and it is write-only. Read operations are
+ * limited to the 16-bit ADC output.

as simple as it gets :)

Quoted 1 line

a link to the documentation would be nice

Sure thing, will do.

Quoted 17 lines

drivers/iio/light/lv0104cs.c new file +16 -0

+ *
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define LV0104CS_REGVAL_MEASURE		0xE0
+#define LV0104CS_REGVAL_SLEEP		0x00
+
+#define LV0104CS_HWGAIN_0x25		0
+#define LV0104CS_HWGAIN_1x		1

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 40
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+#define LV0104CS_HWGAIN_2x		2
+#define LV0104CS_HWGAIN_8x		3
+
+#define LV0104CS_INTEG_12m5		0
+#define LV0104CS_INTEG_100m		1
+#define LV0104CS_INTEG_200m		2  
are these milliseconds? maybe annotate

They are, and I will.

Quoted 85 lines

drivers/iio/light/lv0104cs.c new file +84 -0

+
+#define LV0104CS_CALIB_UNITY		31
+
+struct lv0104cs_private {
+	struct i2c_client *client;
+	struct mutex lock;
+	unsigned char hardwaregain;
+	unsigned char int_time;
+	unsigned char calibscale;
+};
+
+struct lv0104cs_mapping {
+	int val;
+	int val2;
+	unsigned char regval;
+};
+
+static const struct lv0104cs_mapping lv0104cs_calibscales[] = {
+	{ 0, 666666, 0x81 },
+	{ 0, 800000, 0x82 },
+	{ 0, 857142, 0x83 },
+	{ 0, 888888, 0x84 },
+	{ 0, 909090, 0x85 },
+	{ 0, 923076, 0x86 },
+	{ 0, 933333, 0x87 },
+	{ 0, 941176, 0x88 },
+	{ 0, 947368, 0x89 },
+	{ 0, 952380, 0x8A },
+	{ 0, 956521, 0x8B },
+	{ 0, 960000, 0x8C },
+	{ 0, 962962, 0x8D },
+	{ 0, 965517, 0x8E },
+	{ 0, 967741, 0x8F },
+	{ 0, 969696, 0x90 },
+	{ 0, 971428, 0x91 },
+	{ 0, 972972, 0x92 },
+	{ 0, 974358, 0x93 },
+	{ 0, 975609, 0x94 },
+	{ 0, 976744, 0x95 },
+	{ 0, 977777, 0x96 },
+	{ 0, 978723, 0x97 },
+	{ 0, 979591, 0x98 },
+	{ 0, 980392, 0x99 },
+	{ 0, 981132, 0x9A },
+	{ 0, 981818, 0x9B },
+	{ 0, 982456, 0x9C },
+	{ 0, 983050, 0x9D },
+	{ 0, 983606, 0x9E },
+	{ 0, 984126, 0x9F },
+	{ 1, 0, 0x80 },
+	{ 1, 16129, 0xBF },
+	{ 1, 16666, 0xBE },
+	{ 1, 17241, 0xBD },
+	{ 1, 17857, 0xBC },
+	{ 1, 18518, 0xBB },
+	{ 1, 19230, 0xBA },
+	{ 1, 20000, 0xB9 },
+	{ 1, 20833, 0xB8 },
+	{ 1, 21739, 0xB7 },
+	{ 1, 22727, 0xB6 },
+	{ 1, 23809, 0xB5 },
+	{ 1, 24999, 0xB4 },
+	{ 1, 26315, 0xB3 },
+	{ 1, 27777, 0xB2 },
+	{ 1, 29411, 0xB1 },
+	{ 1, 31250, 0xB0 },
+	{ 1, 33333, 0xAF },
+	{ 1, 35714, 0xAE },
+	{ 1, 38461, 0xAD },
+	{ 1, 41666, 0xAC },
+	{ 1, 45454, 0xAB },
+	{ 1, 50000, 0xAA },
+	{ 1, 55555, 0xA9 },
+	{ 1, 62500, 0xA8 },
+	{ 1, 71428, 0xA7 },
+	{ 1, 83333, 0xA6 },
+	{ 1, 100000, 0xA5 },
+	{ 1, 125000, 0xA4 },
+	{ 1, 166666, 0xA3 },
+	{ 1, 250000, 0xA2 },
+	{ 1, 500000, 0xA1 },
+};
+
+static const struct lv0104cs_mapping lv0104cs_hardwaregains[] = {

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 129
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+	{ 0, 250000, 0x00 },
+	{ 1, 0, 0x08 },
+	{ 2, 0, 0x10 },
+	{ 8, 0, 0x18 },
+};  
it would be nice if the LV0104CS_HWGAIN_ #defines could be used here, aren't the values 0x00 .. 0x18 just LV0104CS_HWGAIN_xx << 3?

Excellent idea, will do.

Quoted 9 lines

drivers/iio/light/lv0104cs.c line 135
Quoted 7 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+
+static const struct lv0104cs_mapping lv0104cs_int_times[] = {
+	{ 0, 12500, 0x00 },
+	{ 0, 100000, 0x02 },
+	{ 0, 200000, 0x04 },
+};  
maybe something similar here for LV0104CS_INTEG

Quoted 5 lines

Excellent idea, will do.
Quoted 4 lines
drivers/iio/light/lv0104cs.c line 138
Quoted 4 lines
drivers/iio/light/lv0104cs.c new file +3 -0
+
+static int lv0104cs_write_reg(struct lv0104cs_private *lv0104cs,
+				unsigned char regval)

I'd use a u8 instead of an unsigned char. It's not really a character as such and we have u8 to represent an 8 bit unsigned integer.

Quoted 48 lines

Quoted 7 lines
drivers/iio/light/lv0104cs.c line 143
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+{
+	struct i2c_client *client = lv0104cs->client;
+	int ret;
+
+	ret = i2c_master_send(client, &regval, 1);  
maybe sizeof(regval) instead of 1
Agreed, will do.
Quoted 17 lines
Quoted 4 lines
drivers/iio/light/lv0104cs.c new file +3 -0
+	if (ret != 1) {
+		dev_err(&client->dev, "Failed to write to device: %d\n", ret);
+		return ret;
drivers/iio/light/lv0104cs.c line 156
Quoted 10 lines
drivers/iio/light/lv0104cs.c new file +10 -0
+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_adc(struct lv0104cs_private *lv0104cs,
+				unsigned int *adc_output)
+{
+	struct i2c_client *client = lv0104cs->client;
+	unsigned char regval[2];  
use
__be16 regval;
Sure thing, will do.
Quoted 6 lines
drivers/iio/light/lv0104cs.c line 159
Quoted 4 lines
drivers/iio/light/lv0104cs.c new file +3 -0
+	int ret;
+
+	ret = i2c_master_recv(client, regval, 2);  
sizeof(regval)
Agreed, will do.
Quoted 9 lines
drivers/iio/light/lv0104cs.c line 165
Quoted 7 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+	if (ret != 2) {
+		dev_err(&client->dev, "Failed to read from device: %d\n", ret);
+		return ret;
+	}
+
+	*adc_output = (regval[0] << 8) + regval[1];  
use be16_to_cpu()
Sure thing, will do.
Quoted 1 line

Quoted 14 lines

Quoted 6 lines

drivers/iio/light/lv0104cs.c new file +6 -0

+
+	return 0;
+}
+
+static int lv0104cs_get_lux(struct lv0104cs_private *lv0104cs,
+				unsigned int *val, unsigned int *val2)

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 179

Quoted 8 lines

drivers/iio/light/lv0104cs.c new file +8 -0

+{
+	unsigned char regval = LV0104CS_REGVAL_MEASURE;
+	unsigned int adc_output;
+	int ret;
+
+	/* this function expects to be called while mutex is locked */
+	if (!mutex_is_locked(&lv0104cs->lock))
+		return -EACCES;

This seems a little unusual... Can probably just use a comment rather than a heavy weight check.

Quoted 46 lines

drivers/iio/light/lv0104cs.c new file +46 -0

+
+	regval |= lv0104cs_hardwaregains[lv0104cs->hardwaregain].regval;
+	regval |= lv0104cs_int_times[lv0104cs->int_time].regval;
+	ret = lv0104cs_write_reg(lv0104cs, regval);
+	if (ret)
+		return ret;
+
+	/* wait for integration time to pass (with margin) */
+	switch (lv0104cs->int_time) {
+	case LV0104CS_INTEG_12m5:
+		msleep(50);
+		break;
+
+	case LV0104CS_INTEG_100m:
+		msleep(150);
+		break;
+
+	case LV0104CS_INTEG_200m:
+		msleep(250);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = lv0104cs_read_adc(lv0104cs, &adc_output);
+	if (ret)
+		return ret;
+
+	ret = lv0104cs_write_reg(lv0104cs, LV0104CS_REGVAL_SLEEP);
+	if (ret)
+		return ret;
+
+	/* normalize to lux based on applied gain */
+	switch (lv0104cs->hardwaregain) {
+	case LV0104CS_HWGAIN_0x25:
+		*val = adc_output * 4;
+		*val2 = 0;
+		break;
+
+	case LV0104CS_HWGAIN_1x:
+		*val = adc_output;
+		*val2 = 0;
+		break;
+
+	case LV0104CS_HWGAIN_2x:

Quoted 8 lines

Quoted 8 lines
drivers/iio/light/lv0104cs.c line 233
Quoted 8 lines
drivers/iio/light/lv0104cs.c new file +8 -0
+		*val = adc_output / 2;
+		*val2 = (adc_output % 2) * 500000;
+		break;
+
+	case LV0104CS_HWGAIN_8x:
+		*val = adc_output / 8;
+		*val2 = (adc_output % 8) * 125000;
+		break;

could just return in each of these rather than breaks.

Quoted 158 lines

Quoted 87 lines

drivers/iio/light/lv0104cs.c new file +87 -0

+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = lv0104cs_get_lux(lv0104cs, val, val2);
+		if (ret)
+			goto err_mutex;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*val = lv0104cs_calibscales[lv0104cs->calibscale].val;
+		*val2 = lv0104cs_calibscales[lv0104cs->calibscale].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*val = lv0104cs_hardwaregains[lv0104cs->hardwaregain].val;
+		*val2 = lv0104cs_hardwaregains[lv0104cs->hardwaregain].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = lv0104cs_int_times[lv0104cs->int_time].val;
+		*val2 = lv0104cs_int_times[lv0104cs->int_time].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+err_mutex:
+	mutex_unlock(&lv0104cs->lock);
+
+	return ret;
+}
+
+static int lv0104cs_set_calibscale(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int calibscale = val * 1000000 + val2;
+	int floor, ceil, mid;
+	int ret, i, index;
+
+	/* round to nearest quantized sensitivity */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales) - 1; i++) {
+		floor = lv0104cs_calibscales[i].val * 1000000
+				+ lv0104cs_calibscales[i].val2;
+		ceil = lv0104cs_calibscales[i + 1].val * 1000000
+				+ lv0104cs_calibscales[i + 1].val2;
+		mid = (floor + ceil) / 2;
+
+		/* round down */
+		if (calibscale >= floor && calibscale < mid) {
+			index = i;
+			break;
+		}
+
+		/* round up */
+		if (calibscale >= mid && calibscale <= ceil) {
+			index = i + 1;
+			break;
+		}
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_calibscales) - 1)
+		return -EINVAL;
+

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 326
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+	/* set sensitivity */
+	ret = lv0104cs_write_reg(lv0104cs, lv0104cs_calibscales[index].regval);
+	if (ret)
+		return ret;
+
+	mutex_lock(&lv0104cs->lock);  
shoudn't the lock region include the set sensitivity above?

Good catch, will do.

Quoted 61 lines

drivers/iio/light/lv0104cs.c new file +60 -0

+	lv0104cs->calibscale = index;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_hardwaregain(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_hardwaregains); i++) {
+		if (val != lv0104cs_hardwaregains[i].val)
+			continue;
+
+		if (val2 == lv0104cs_hardwaregains[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_hardwaregains))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->hardwaregain = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_int_time(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		if (val != lv0104cs_int_times[i].val)
+			continue;
+
+		if (val2 == lv0104cs_int_times[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_int_times))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->int_time = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_write_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;

Quoted 8 lines

Quoted 8 lines
drivers/iio/light/lv0104cs.c line 394
Quoted 8 lines
drivers/iio/light/lv0104cs.c new file +8 -0
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = lv0104cs_set_calibscale(lv0104cs, val, val2);
+		break;

return directly here and below rather than breaking then returning (as nothing else to be done).

Quoted 102 lines

Quoted 91 lines

drivers/iio/light/lv0104cs.c new file +91 -0

+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		ret = lv0104cs_set_hardwaregain(lv0104cs, val, val2);
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = lv0104cs_set_int_time(lv0104cs, val, val2);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static ssize_t lv0104cs_show_calibscale_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_calibscales[i].val,
+				lv0104cs_calibscales[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_hardwaregain_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_hardwaregains); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_hardwaregains[i].val,
+				lv0104cs_hardwaregains[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_int_time_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_int_times[i].val,
+				lv0104cs_int_times[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(calibscale_available, 0444,
+				lv0104cs_show_calibscale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(hardwaregain_available, 0444,
+				lv0104cs_show_hardwaregain_avail, NULL, 0);
+static IIO_DEV_ATTR_INT_TIME_AVAIL(lv0104cs_show_int_time_avail);
+
+static struct attribute *lv0104cs_attributes[] = {
+	&iio_dev_attr_calibscale_available.dev_attr.attr,
+	&iio_dev_attr_hardwaregain_available.dev_attr.attr,
+	&iio_dev_attr_integration_time_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lv0104cs_attribute_group = {
+	.attrs = lv0104cs_attributes,
+};
+
+static const struct iio_info lv0104cs_info = {
+	.attrs = &lv0104cs_attribute_group,
+	.read_raw = &lv0104cs_read_raw,
+	.write_raw = &lv0104cs_write_raw,
+};
+
+static const struct iio_chan_spec lv0104cs_channels[] = {

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 490
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_CALIBSCALE) |
+				      BIT(IIO_CHAN_INFO_HARDWAREGAIN) |  
I think this should be just _SCALE; this is not about calibration, but just sets a gain factor

Agreed, will do.

Quoted 1 line

Quoted 7 lines

Quoted 2 lines
I don't quite understand the need/difference between CALIBSCALE / HARDWAREGAIN
This device exposes two separate controls: gain (presumably a PGA of sorts in between the photodiode and the ADC) and sensitivity (a cubic trim control). These are mapped to HARDWAREGAIN and CALIBSCALE (now SCALE), respectively.

How would user space know how to manipulate the two separate controls? Is there a 'right' answer for a particular overall gain?

Anyhow, kind of sounds like you have these better defined anyway with the trim control as calibscale and the front end gain as straight forward _SCALE (so this one is the one that userspace will change to adapt to conditions whilst the other deals with things like glass absorption on the incoming data?)

Quoted 14 lines

Quoted 4 lines

drivers/iio/light/lv0104cs.c new file +3 -0
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+};

drivers/iio/light/lv0104cs.c line 503

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+
+static int lv0104cs_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct device *dev = &client->dev;
+	struct lv0104cs_private *lv0104cs;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct lv0104cs_private));

Ever so small preference for
sizeof(*lv0104cs)

Quoted 36 lines

Quoted 27 lines

drivers/iio/light/lv0104cs.c new file +27 -0

+	if (!indio_dev)
+		return -ENOMEM;
+
+	lv0104cs = iio_priv(indio_dev);
+
+	i2c_set_clientdata(client, lv0104cs);
+	lv0104cs->client = client;
+
+	mutex_init(&lv0104cs->lock);
+
+	lv0104cs->hardwaregain = LV0104CS_HWGAIN_1x;
+	lv0104cs->int_time = LV0104CS_INTEG_200m;
+	lv0104cs->calibscale = LV0104CS_CALIB_UNITY;
+
+	ret = lv0104cs_write_reg(lv0104cs,
+			lv0104cs_calibscales[LV0104CS_CALIB_UNITY].regval);
+	if (ret)
+		return -ENODEV;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.parent = dev;
+	indio_dev->channels = lv0104cs_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lv0104cs_channels);
+	indio_dev->name = client->name;
+	indio_dev->info = &lv0104cs_info;
+
+	ret = devm_iio_device_register(dev, indio_dev);

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 536
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+	if (ret) {
+		dev_err(dev, "Failed to register device: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "Successfully registered device\n");  
please drop this output, considered just clutter

Quoted 1 line

Sure thing, will do.

Also note that without this message you can drop the return ret out of the brackets above.

Quoted 16 lines

Quoted 16 lines
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+
+	return 0;
+}
+
+static const struct i2c_device_id lv0104cs_id[] = {
drivers/iio/light/lv0104cs.c line 551
Quoted 10 lines
drivers/iio/light/lv0104cs.c new file +10 -0
+	{ "lv0104cs", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lv0104cs_id);
+
+static struct i2c_driver lv0104cs_i2c_driver = {
+	.driver = {
+		.name	= "lv0104cs",
+	},
+

Really trivial, but this blank line doesn't add anything so get rid of it.

Quoted 15 lines

Quoted 9 lines

drivers/iio/light/lv0104cs.c new file +8 -0

+	.id_table	= lv0104cs_id,
+	.probe		= lv0104cs_probe,
+};
+module_i2c_driver(lv0104cs_i2c_driver);
+
+MODULE_AUTHOR("Jeff LaBundy <[email protected]>");
+MODULE_DESCRIPTION("LV0104CS Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL v2");

Signature · 2 lines

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Jeff LaBundy

author

2018-02-17 19:18 UTC #4 lore raw

Quoted 8 lines · Jonathan Cameron

Quoted 5 lines · On Wed, 14 Feb 2018 22:46:42 -0600 Jeff LaBundy <[email protected]> wrote:
Quoted 4 lines · Peter Meerwald-Stadler
Quoted 3 lines
This patch adds support for the On Semiconductor LV0104CS ambient light sensor.

Quoted 8 lines

Quoted 4 lines
Quoted 1 line
nice little driver, some comments below
Thank you very much for the review; I agree with your feedback and will submit a v2. Comments and answers in line.
A few more comments from me.
Jonathan

Thank you very much for the additional feedback; I agree with your comments and will address them in v2 as well.

Quoted 209 lines

Quoted 17 lines

Signed-off-by Jeff LaBundy <[email protected]>

---
 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/lv0104cs.c | 559 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 570 insertions(+)
 create mode 100644 drivers/iio/light/lv0104cs.c

drivers/iio/light/Kconfig +0 -0

index 2356ed9..ca8918e 100644
@@ -275,6 +275,16 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.

Quoted 8 lines

drivers/iio/light/Kconfig config LTR501 · line 283
Quoted 6 lines
drivers/iio/light/Kconfig +6 -0
+config LV0104CS
+	tristate "LV0104CS Ambient Light Sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build support for the LV0104CS ambient
+	 light sensor.  
maybe mention On Semi somewhere?

Sure thing, will do.

Quoted 47 lines

Quoted 34 lines

drivers/iio/light/Kconfig +4 -0

+
+	 To compile this driver as a module, choose M here:
+	 the module will be called lv0104cs.
+
 config MAX44000
 	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index fa32fa4..77c8682 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o

drivers/iio/light/lv0104cs.c new file +8 -0

new file mode 100644
index 0000000..ecbba39
@@ -0,0 +1,559 @@
+/*
+ * lv0104cs.c: LV0104CS Ambient Light Sensor Driver
+ *
+ * Copyright (C) 2018 Jeff LaBundy <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 of the License
+ * as published by the Free Software Foundation.

drivers/iio/light/lv0104cs.c line 18

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * 7-bit I2C slave address: 0x13
+ *
+ * This device has just one register and it is write-only. Read operations are
+ * limited to the 16-bit ADC output.

as simple as it gets :)

Quoted 1 line

a link to the documentation would be nice

Sure thing, will do.

Quoted 17 lines

drivers/iio/light/lv0104cs.c new file +16 -0

+ *
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/mutex.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define LV0104CS_REGVAL_MEASURE		0xE0
+#define LV0104CS_REGVAL_SLEEP		0x00
+
+#define LV0104CS_HWGAIN_0x25		0
+#define LV0104CS_HWGAIN_1x		1

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 40
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+#define LV0104CS_HWGAIN_2x		2
+#define LV0104CS_HWGAIN_8x		3
+
+#define LV0104CS_INTEG_12m5		0
+#define LV0104CS_INTEG_100m		1
+#define LV0104CS_INTEG_200m		2  
are these milliseconds? maybe annotate

They are, and I will.

Quoted 85 lines

drivers/iio/light/lv0104cs.c new file +84 -0

+
+#define LV0104CS_CALIB_UNITY		31
+
+struct lv0104cs_private {
+	struct i2c_client *client;
+	struct mutex lock;
+	unsigned char hardwaregain;
+	unsigned char int_time;
+	unsigned char calibscale;
+};
+
+struct lv0104cs_mapping {
+	int val;
+	int val2;
+	unsigned char regval;
+};
+
+static const struct lv0104cs_mapping lv0104cs_calibscales[] = {
+	{ 0, 666666, 0x81 },
+	{ 0, 800000, 0x82 },
+	{ 0, 857142, 0x83 },
+	{ 0, 888888, 0x84 },
+	{ 0, 909090, 0x85 },
+	{ 0, 923076, 0x86 },
+	{ 0, 933333, 0x87 },
+	{ 0, 941176, 0x88 },
+	{ 0, 947368, 0x89 },
+	{ 0, 952380, 0x8A },
+	{ 0, 956521, 0x8B },
+	{ 0, 960000, 0x8C },
+	{ 0, 962962, 0x8D },
+	{ 0, 965517, 0x8E },
+	{ 0, 967741, 0x8F },
+	{ 0, 969696, 0x90 },
+	{ 0, 971428, 0x91 },
+	{ 0, 972972, 0x92 },
+	{ 0, 974358, 0x93 },
+	{ 0, 975609, 0x94 },
+	{ 0, 976744, 0x95 },
+	{ 0, 977777, 0x96 },
+	{ 0, 978723, 0x97 },
+	{ 0, 979591, 0x98 },
+	{ 0, 980392, 0x99 },
+	{ 0, 981132, 0x9A },
+	{ 0, 981818, 0x9B },
+	{ 0, 982456, 0x9C },
+	{ 0, 983050, 0x9D },
+	{ 0, 983606, 0x9E },
+	{ 0, 984126, 0x9F },
+	{ 1, 0, 0x80 },
+	{ 1, 16129, 0xBF },
+	{ 1, 16666, 0xBE },
+	{ 1, 17241, 0xBD },
+	{ 1, 17857, 0xBC },
+	{ 1, 18518, 0xBB },
+	{ 1, 19230, 0xBA },
+	{ 1, 20000, 0xB9 },
+	{ 1, 20833, 0xB8 },
+	{ 1, 21739, 0xB7 },
+	{ 1, 22727, 0xB6 },
+	{ 1, 23809, 0xB5 },
+	{ 1, 24999, 0xB4 },
+	{ 1, 26315, 0xB3 },
+	{ 1, 27777, 0xB2 },
+	{ 1, 29411, 0xB1 },
+	{ 1, 31250, 0xB0 },
+	{ 1, 33333, 0xAF },
+	{ 1, 35714, 0xAE },
+	{ 1, 38461, 0xAD },
+	{ 1, 41666, 0xAC },
+	{ 1, 45454, 0xAB },
+	{ 1, 50000, 0xAA },
+	{ 1, 55555, 0xA9 },
+	{ 1, 62500, 0xA8 },
+	{ 1, 71428, 0xA7 },
+	{ 1, 83333, 0xA6 },
+	{ 1, 100000, 0xA5 },
+	{ 1, 125000, 0xA4 },
+	{ 1, 166666, 0xA3 },
+	{ 1, 250000, 0xA2 },
+	{ 1, 500000, 0xA1 },
+};
+
+static const struct lv0104cs_mapping lv0104cs_hardwaregains[] = {

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 129
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+	{ 0, 250000, 0x00 },
+	{ 1, 0, 0x08 },
+	{ 2, 0, 0x10 },
+	{ 8, 0, 0x18 },
+};  
it would be nice if the LV0104CS_HWGAIN_ #defines could be used here, aren't the values 0x00 .. 0x18 just LV0104CS_HWGAIN_xx << 3?

Excellent idea, will do.

Quoted 9 lines

drivers/iio/light/lv0104cs.c line 135
Quoted 7 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+
+static const struct lv0104cs_mapping lv0104cs_int_times[] = {
+	{ 0, 12500, 0x00 },
+	{ 0, 100000, 0x02 },
+	{ 0, 200000, 0x04 },
+};  
maybe something similar here for LV0104CS_INTEG

Quoted 7 lines

Quoted 5 lines
Excellent idea, will do.
Quoted 4 lines
drivers/iio/light/lv0104cs.c line 138
Quoted 4 lines
drivers/iio/light/lv0104cs.c new file +3 -0
+
+static int lv0104cs_write_reg(struct lv0104cs_private *lv0104cs,
+				unsigned char regval)
I'd use a u8 instead of an unsigned char. It's not really a character as such and we have u8 to represent an 8 bit unsigned integer.

Sure thing, will do.

Quoted 48 lines

Quoted 48 lines
Quoted 7 lines
drivers/iio/light/lv0104cs.c line 143
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+{
+	struct i2c_client *client = lv0104cs->client;
+	int ret;
+
+	ret = i2c_master_send(client, &regval, 1);  
maybe sizeof(regval) instead of 1
Agreed, will do.
Quoted 17 lines
Quoted 4 lines
drivers/iio/light/lv0104cs.c new file +3 -0
+	if (ret != 1) {
+		dev_err(&client->dev, "Failed to write to device: %d\n", ret);
+		return ret;
drivers/iio/light/lv0104cs.c line 156
Quoted 10 lines
drivers/iio/light/lv0104cs.c new file +10 -0
+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_adc(struct lv0104cs_private *lv0104cs,
+				unsigned int *adc_output)
+{
+	struct i2c_client *client = lv0104cs->client;
+	unsigned char regval[2];  
use
__be16 regval;
Sure thing, will do.
Quoted 6 lines
drivers/iio/light/lv0104cs.c line 159
Quoted 4 lines
drivers/iio/light/lv0104cs.c new file +3 -0
+	int ret;
+
+	ret = i2c_master_recv(client, regval, 2);  
sizeof(regval)
Agreed, will do.
Quoted 9 lines
drivers/iio/light/lv0104cs.c line 165
Quoted 7 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+	if (ret != 2) {
+		dev_err(&client->dev, "Failed to read from device: %d\n", ret);
+		return ret;
+	}
+
+	*adc_output = (regval[0] << 8) + regval[1];  
use be16_to_cpu()
Sure thing, will do.
Quoted 1 line

Quoted 17 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+
+	return 0;
+}
+
+static int lv0104cs_get_lux(struct lv0104cs_private *lv0104cs,
+				unsigned int *val, unsigned int *val2)
Quoted 8 lines
Quoted 8 lines
drivers/iio/light/lv0104cs.c line 179
Quoted 8 lines
drivers/iio/light/lv0104cs.c new file +8 -0
+{
+	unsigned char regval = LV0104CS_REGVAL_MEASURE;
+	unsigned int adc_output;
+	int ret;
+
+	/* this function expects to be called while mutex is locked */
+	if (!mutex_is_locked(&lv0104cs->lock))
+		return -EACCES;
This seems a little unusual... Can probably just use a comment rather than a heavy weight check.

Sure thing, will do.

Quoted 47 lines

drivers/iio/light/lv0104cs.c new file +47 -0

+
+	regval |= lv0104cs_hardwaregains[lv0104cs->hardwaregain].regval;
+	regval |= lv0104cs_int_times[lv0104cs->int_time].regval;
+	ret = lv0104cs_write_reg(lv0104cs, regval);
+	if (ret)
+		return ret;
+
+	/* wait for integration time to pass (with margin) */
+	switch (lv0104cs->int_time) {
+	case LV0104CS_INTEG_12m5:
+		msleep(50);
+		break;
+
+	case LV0104CS_INTEG_100m:
+		msleep(150);
+		break;
+
+	case LV0104CS_INTEG_200m:
+		msleep(250);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = lv0104cs_read_adc(lv0104cs, &adc_output);
+	if (ret)
+		return ret;
+
+	ret = lv0104cs_write_reg(lv0104cs, LV0104CS_REGVAL_SLEEP);
+	if (ret)
+		return ret;
+
+	/* normalize to lux based on applied gain */
+	switch (lv0104cs->hardwaregain) {
+	case LV0104CS_HWGAIN_0x25:
+		*val = adc_output * 4;
+		*val2 = 0;
+		break;
+
+	case LV0104CS_HWGAIN_1x:
+		*val = adc_output;
+		*val2 = 0;
+		break;
+
+	case LV0104CS_HWGAIN_2x:
+		*val = adc_output / 2;

Quoted 8 lines

Quoted 7 lines
Quoted 7 lines
drivers/iio/light/lv0104cs.c line 233
Quoted 7 lines
drivers/iio/light/lv0104cs.c new file +7 -0
+		*val2 = (adc_output % 2) * 500000;
+		break;
+
+	case LV0104CS_HWGAIN_8x:
+		*val = adc_output / 8;
+		*val2 = (adc_output % 8) * 125000;
+		break;
could just return in each of these rather than breaks.

Sure thing, will do.

Quoted 161 lines

Quoted 87 lines

drivers/iio/light/lv0104cs.c new file +87 -0

+
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = lv0104cs_get_lux(lv0104cs, val, val2);
+		if (ret)
+			goto err_mutex;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*val = lv0104cs_calibscales[lv0104cs->calibscale].val;
+		*val2 = lv0104cs_calibscales[lv0104cs->calibscale].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		*val = lv0104cs_hardwaregains[lv0104cs->hardwaregain].val;
+		*val2 = lv0104cs_hardwaregains[lv0104cs->hardwaregain].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = lv0104cs_int_times[lv0104cs->int_time].val;
+		*val2 = lv0104cs_int_times[lv0104cs->int_time].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+err_mutex:
+	mutex_unlock(&lv0104cs->lock);
+
+	return ret;
+}
+
+static int lv0104cs_set_calibscale(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int calibscale = val * 1000000 + val2;
+	int floor, ceil, mid;
+	int ret, i, index;
+
+	/* round to nearest quantized sensitivity */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales) - 1; i++) {
+		floor = lv0104cs_calibscales[i].val * 1000000
+				+ lv0104cs_calibscales[i].val2;
+		ceil = lv0104cs_calibscales[i + 1].val * 1000000
+				+ lv0104cs_calibscales[i + 1].val2;
+		mid = (floor + ceil) / 2;
+
+		/* round down */
+		if (calibscale >= floor && calibscale < mid) {
+			index = i;
+			break;
+		}
+
+		/* round up */
+		if (calibscale >= mid && calibscale <= ceil) {
+			index = i + 1;
+			break;
+		}
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_calibscales) - 1)
+		return -EINVAL;
+

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 326
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+	/* set sensitivity */
+	ret = lv0104cs_write_reg(lv0104cs, lv0104cs_calibscales[index].regval);
+	if (ret)
+		return ret;
+
+	mutex_lock(&lv0104cs->lock);  
shoudn't the lock region include the set sensitivity above?

Good catch, will do.

Quoted 64 lines

drivers/iio/light/lv0104cs.c new file +63 -0

+	lv0104cs->calibscale = index;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_hardwaregain(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_hardwaregains); i++) {
+		if (val != lv0104cs_hardwaregains[i].val)
+			continue;
+
+		if (val2 == lv0104cs_hardwaregains[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_hardwaregains))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->hardwaregain = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_int_time(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		if (val != lv0104cs_int_times[i].val)
+			continue;
+
+		if (val2 == lv0104cs_int_times[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_int_times))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->int_time = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_write_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;

Quoted 9 lines

Quoted 5 lines
Quoted 5 lines
drivers/iio/light/lv0104cs.c line 394
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		ret = lv0104cs_set_calibscale(lv0104cs, val, val2);
+		break;
return directly here and below rather than breaking then returning (as nothing else to be done).

Sure thing, will do.

Quoted 105 lines

Quoted 91 lines

drivers/iio/light/lv0104cs.c new file +91 -0

+
+	case IIO_CHAN_INFO_HARDWAREGAIN:
+		ret = lv0104cs_set_hardwaregain(lv0104cs, val, val2);
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = lv0104cs_set_int_time(lv0104cs, val, val2);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static ssize_t lv0104cs_show_calibscale_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_calibscales[i].val,
+				lv0104cs_calibscales[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_hardwaregain_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_hardwaregains); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_hardwaregains[i].val,
+				lv0104cs_hardwaregains[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_int_time_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_int_times[i].val,
+				lv0104cs_int_times[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(calibscale_available, 0444,
+				lv0104cs_show_calibscale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(hardwaregain_available, 0444,
+				lv0104cs_show_hardwaregain_avail, NULL, 0);
+static IIO_DEV_ATTR_INT_TIME_AVAIL(lv0104cs_show_int_time_avail);
+
+static struct attribute *lv0104cs_attributes[] = {
+	&iio_dev_attr_calibscale_available.dev_attr.attr,
+	&iio_dev_attr_hardwaregain_available.dev_attr.attr,
+	&iio_dev_attr_integration_time_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lv0104cs_attribute_group = {
+	.attrs = lv0104cs_attributes,
+};
+
+static const struct iio_info lv0104cs_info = {
+	.attrs = &lv0104cs_attribute_group,
+	.read_raw = &lv0104cs_read_raw,
+	.write_raw = &lv0104cs_write_raw,
+};
+
+static const struct iio_chan_spec lv0104cs_channels[] = {

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 490
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |
+				      BIT(IIO_CHAN_INFO_CALIBSCALE) |
+				      BIT(IIO_CHAN_INFO_HARDWAREGAIN) |  
I think this should be just _SCALE; this is not about calibration, but just sets a gain factor

Agreed, will do.

Quoted 3 lines

I don't quite understand the need/difference between CALIBSCALE / HARDWAREGAIN

Quoted 7 lines

Quoted 4 lines
This device exposes two separate controls: gain (presumably a PGA of sorts in between the photodiode and the ADC) and sensitivity (a cubic trim control). These are mapped to HARDWAREGAIN and CALIBSCALE (now SCALE), respectively.
How would user space know how to manipulate the two separate controls? Is there a 'right' answer for a particular overall gain?

I think it would be more clear to keep CALIBSCALE as-is but change HARDWAREGAIN to SCALE (see next comment).

Quoted 6 lines

Anyhow, kind of sounds like you have these better defined anyway with the trim control as calibscale and the front end gain as straight forward _SCALE (so this one is the one that userspace will change to adapt to conditions whilst the other deals with things like glass absorption on the incoming data?)

That's correct; HARDWAREGAIN acts as a coarse gain to accommodate different lighting conditions, while CALIBSCALE acts as a fine trim to compensate for the effects of a lens (i.e. calibration). I will keep CALIBSCALE as-is but change HARDWAREGAIN to the much more common SCALE.

Quoted 18 lines

Quoted 15 lines
Quoted 15 lines
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +3 -0
+				      BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+};
drivers/iio/light/lv0104cs.c line 503
Quoted 10 lines
drivers/iio/light/lv0104cs.c new file +10 -0
+
+static int lv0104cs_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct device *dev = &client->dev;
+	struct lv0104cs_private *lv0104cs;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(struct lv0104cs_private));
Ever so small preference for
sizeof(*lv0104cs)

I like that better as well; will do.

Quoted 37 lines

Quoted 36 lines

Quoted 24 lines

drivers/iio/light/lv0104cs.c new file +23 -0

+	if (!indio_dev)
+		return -ENOMEM;
+
+	lv0104cs = iio_priv(indio_dev);
+
+	i2c_set_clientdata(client, lv0104cs);
+	lv0104cs->client = client;
+
+	mutex_init(&lv0104cs->lock);
+
+	lv0104cs->hardwaregain = LV0104CS_HWGAIN_1x;
+	lv0104cs->int_time = LV0104CS_INTEG_200m;
+	lv0104cs->calibscale = LV0104CS_CALIB_UNITY;
+
+	ret = lv0104cs_write_reg(lv0104cs,
+			lv0104cs_calibscales[LV0104CS_CALIB_UNITY].regval);
+	if (ret)
+		return -ENODEV;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.parent = dev;
+	indio_dev->channels = lv0104cs_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lv0104cs_channels);

drivers/iio/light/lv0104cs.c line 536

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+	indio_dev->name = client->name;
+	indio_dev->info = &lv0104cs_info;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret) {
+		dev_err(dev, "Failed to register device: %d\n", ret);
+		return ret;
+	}
+
+	dev_info(dev, "Successfully registered device\n");

please drop this output, considered just clutter

Quoted 4 lines

Quoted 1 line
Sure thing, will do.
Also note that without this message you can drop the return ret out of the brackets above.

Agreed, will do.

Quoted 12 lines

Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/iio/light/lv0104cs.c new file +10 -0
+
+	return 0;
+}
+
+static const struct i2c_device_id lv0104cs_id[] = {
+	{ "lv0104cs", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lv0104cs_id);
+

Quoted 8 lines

Quoted 5 lines
Quoted 5 lines
drivers/iio/light/lv0104cs.c line 551
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+static struct i2c_driver lv0104cs_i2c_driver = {
+	.driver = {
+		.name	= "lv0104cs",
+	},
+
Really trivial, but this blank line doesn't add anything so get rid of it.

Sure thing, will do.

Quoted 18 lines

Quoted 16 lines

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +8 -0

+	.id_table	= lv0104cs_id,
+	.probe		= lv0104cs_probe,
+};
+module_i2c_driver(lv0104cs_i2c_driver);
+
+MODULE_AUTHOR("Jeff LaBundy <[email protected]>");
+MODULE_DESCRIPTION("LV0104CS Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL v2");

Signature · 2 lines

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418

Jeff LaBundy

author

2018-02-18 18:25 UTC #5 lore raw

PATCH v2 1/1 iio: light: lv0104cs: Add support for LV0104CS light sensor

In reply to Jeff LaBundy

This patch adds support for the On Semiconductor LV0104CS ambient light sensor.

Signed-off-by Jeff LaBundy <[email protected]>

---
Changes in v2:

- Added manufacturer name to Kconfig entry

- Replaced description of register interface in introductory comments with link to data sheet

- Added units to integration time macros

- Replaced scale and integration time hard-coded register values with macros

- Replaced unsigned char with u8 throughout

- Replaced break statements with return statements within switch() blocks located at the end of functions throughout

- Replaced hard-coded lengths with sizeof() in calls to i2c_master_send and i2c_master_recv

- Used big-endian kernel macros to resolve 16-bit ADC output value and replaced an instance of unsigned int with u16 in lv0104cs_read_adc

- Removed unnecessary mutex lock check from lv0104cs_get_lux

- Moved register write inside mutex lock in lv0104cs_set_calibscale

- Replaced IIO_CHAN_INFO_HARDWAREGAIN with IIO_CHAN_INFO_SCALE

- Removed superfluous success/failure printing following device registration

- Collapsed sizeof(struct lv0104cs_private) to sizeof(*lv0104cs) in

    lv0104cs_probe

- Removed extraneous whitespace in lv0104cs_i2c_driver declaration

 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/lv0104cs.c | 540 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 551 insertions(+)
 create mode 100644 drivers/iio/light/lv0104cs.c

drivers/iio/light/Kconfig +10 -0

index 2356ed9..be0b313 100644
@@ -275,6 +275,16 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config LV0104CS
+	tristate "LV0104CS Ambient Light Sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build support for the On Semiconductor
+	 LV0104CS ambient light sensor.
+
+	 To compile this driver as a module, choose M here:
+	 the module will be called lv0104cs.
+
 config MAX44000
 	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index fa32fa4..77c8682 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o

drivers/iio/light/lv0104cs.c new file +540 -0

540 lines not on this page · raw message

--
2.7.4

Jonathan Cameron

maintainer

2018-02-24 12:03 UTC #6 lore raw

Quoted 4 lines · On Sun, 18 Feb 2018 12:25:07 -0600 Jeff LaBundy <[email protected]> wrote:

This patch adds support for the On Semiconductor LV0104CS ambient light sensor.
Signed-off-by Jeff LaBundy <[email protected]>

Hi Jeff,

It is a bit kernel maintainer dependant but I would always much
rather see a fresh email thread for each version. Otherwise we run
a risk of confusion if the thread gets very deep. Obviously not a problem yet!

Some really minor stuff inline, but the error handling needs fixing in at least one place.

Jonathan

Quoted 12 lines

---
Changes in v2:
- Added manufacturer name to Kconfig entry
- Replaced description of register interface in introductory comments with link to data sheet
- Added units to integration time macros
- Replaced scale and integration time hard-coded register values with macros

Quoted 7 lines

- Replaced unsigned char with u8 throughout
- Replaced break statements with return statements within switch() blocks located at the end of functions throughout
- Replaced hard-coded lengths with sizeof() in calls to i2c_master_send and i2c_master_recv

This good, but you could return 1 rather than an error in the case of a short message (a slightly horrible artefact of the i2c interface!)

Quoted 65 lines

- Used big-endian kernel macros to resolve 16-bit ADC output value and replaced an instance of unsigned int with u16 in lv0104cs_read_adc
- Removed unnecessary mutex lock check from lv0104cs_get_lux
- Moved register write inside mutex lock in lv0104cs_set_calibscale
- Replaced IIO_CHAN_INFO_HARDWAREGAIN with IIO_CHAN_INFO_SCALE
- Removed superfluous success/failure printing following device registration
- Collapsed sizeof(struct lv0104cs_private) to sizeof(*lv0104cs) in
    lv0104cs_probe
- Removed extraneous whitespace in lv0104cs_i2c_driver declaration
 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/lv0104cs.c | 540 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 551 insertions(+)
 create mode 100644 drivers/iio/light/lv0104cs.c
drivers/iio/light/Kconfig +10 -0
index 2356ed9..be0b313 100644
@@ -275,6 +275,16 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config LV0104CS
+	tristate "LV0104CS Ambient Light Sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build support for the On Semiconductor
+	 LV0104CS ambient light sensor.
+
+	 To compile this driver as a module, choose M here:
+	 the module will be called lv0104cs.
+
 config MAX44000
 	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
 	depends on I2C
drivers/iio/light/Makefile +1 -0
index fa32fa4..77c8682 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o
drivers/iio/light/lv0104cs.c new file +3 -0
new file mode 100644
index 0000000..b783f90
@@ -0,0 +1,540 @@
+/*
+ * lv0104cs.c: LV0104CS Ambient Light Sensor Driver
+ *

drivers/iio/light/lv0104cs.c line 13

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+ * Copyright (C) 2018 Jeff LaBundy <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 of the License
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.

Up to you but SPDX licence tag is a possible alternative to stating this license block.

drivers/iio/light/lv0104cs.c line 18

Quoted 5 lines

drivers/iio/light/lv0104cs.c new file +5 -0

+ *
+ * 7-bit I2C slave address: 0x13
+ *
+ * Link to data sheet: http://www.onsemi.com/pub/Collateral/LV0104CS-D.PDF
+ *

Nitpick, a blank line doesn't add anything here.

Quoted 117 lines

drivers/iio/light/lv0104cs.c new file +117 -0

+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define LV0104CS_REGVAL_MEASURE		0xE0
+#define LV0104CS_REGVAL_SLEEP		0x00
+
+#define LV0104CS_SCALE_0_25X		0
+#define LV0104CS_SCALE_1X		1
+#define LV0104CS_SCALE_2X		2
+#define LV0104CS_SCALE_8X		3
+#define LV0104CS_SCALE_SHIFT		3
+
+#define LV0104CS_INTEG_12_5MS		0
+#define LV0104CS_INTEG_100MS		1
+#define LV0104CS_INTEG_200MS		2
+#define LV0104CS_INTEG_SHIFT		1
+
+#define LV0104CS_CALIBSCALE_UNITY	31
+
+struct lv0104cs_private {
+	struct i2c_client *client;
+	struct mutex lock;
+	u8 calibscale;
+	u8 scale;
+	u8 int_time;
+};
+
+struct lv0104cs_mapping {
+	int val;
+	int val2;
+	u8 regval;
+};
+
+static const struct lv0104cs_mapping lv0104cs_calibscales[] = {
+	{ 0, 666666, 0x81 },
+	{ 0, 800000, 0x82 },
+	{ 0, 857142, 0x83 },
+	{ 0, 888888, 0x84 },
+	{ 0, 909090, 0x85 },
+	{ 0, 923076, 0x86 },
+	{ 0, 933333, 0x87 },
+	{ 0, 941176, 0x88 },
+	{ 0, 947368, 0x89 },
+	{ 0, 952380, 0x8A },
+	{ 0, 956521, 0x8B },
+	{ 0, 960000, 0x8C },
+	{ 0, 962962, 0x8D },
+	{ 0, 965517, 0x8E },
+	{ 0, 967741, 0x8F },
+	{ 0, 969696, 0x90 },
+	{ 0, 971428, 0x91 },
+	{ 0, 972972, 0x92 },
+	{ 0, 974358, 0x93 },
+	{ 0, 975609, 0x94 },
+	{ 0, 976744, 0x95 },
+	{ 0, 977777, 0x96 },
+	{ 0, 978723, 0x97 },
+	{ 0, 979591, 0x98 },
+	{ 0, 980392, 0x99 },
+	{ 0, 981132, 0x9A },
+	{ 0, 981818, 0x9B },
+	{ 0, 982456, 0x9C },
+	{ 0, 983050, 0x9D },
+	{ 0, 983606, 0x9E },
+	{ 0, 984126, 0x9F },
+	{ 1, 0, 0x80 },
+	{ 1, 16129, 0xBF },
+	{ 1, 16666, 0xBE },
+	{ 1, 17241, 0xBD },
+	{ 1, 17857, 0xBC },
+	{ 1, 18518, 0xBB },
+	{ 1, 19230, 0xBA },
+	{ 1, 20000, 0xB9 },
+	{ 1, 20833, 0xB8 },
+	{ 1, 21739, 0xB7 },
+	{ 1, 22727, 0xB6 },
+	{ 1, 23809, 0xB5 },
+	{ 1, 24999, 0xB4 },
+	{ 1, 26315, 0xB3 },
+	{ 1, 27777, 0xB2 },
+	{ 1, 29411, 0xB1 },
+	{ 1, 31250, 0xB0 },
+	{ 1, 33333, 0xAF },
+	{ 1, 35714, 0xAE },
+	{ 1, 38461, 0xAD },
+	{ 1, 41666, 0xAC },
+	{ 1, 45454, 0xAB },
+	{ 1, 50000, 0xAA },
+	{ 1, 55555, 0xA9 },
+	{ 1, 62500, 0xA8 },
+	{ 1, 71428, 0xA7 },
+	{ 1, 83333, 0xA6 },
+	{ 1, 100000, 0xA5 },
+	{ 1, 125000, 0xA4 },
+	{ 1, 166666, 0xA3 },
+	{ 1, 250000, 0xA2 },
+	{ 1, 500000, 0xA1 },
+};
+
+static const struct lv0104cs_mapping lv0104cs_scales[] = {
+	{ 0, 250000, LV0104CS_SCALE_0_25X << LV0104CS_SCALE_SHIFT },
+	{ 1, 0, LV0104CS_SCALE_1X << LV0104CS_SCALE_SHIFT },
+	{ 2, 0, LV0104CS_SCALE_2X << LV0104CS_SCALE_SHIFT },
+	{ 8, 0, LV0104CS_SCALE_8X << LV0104CS_SCALE_SHIFT },
+};
+
+static const struct lv0104cs_mapping lv0104cs_int_times[] = {
+	{ 0, 12500, LV0104CS_INTEG_12_5MS << LV0104CS_INTEG_SHIFT },
+	{ 0, 100000, LV0104CS_INTEG_100MS << LV0104CS_INTEG_SHIFT },

drivers/iio/light/lv0104cs.c line 145

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+	{ 0, 200000, LV0104CS_INTEG_200MS << LV0104CS_INTEG_SHIFT },
+};
+
+static int lv0104cs_write_reg(struct lv0104cs_private *lv0104cs, u8 regval)
+{
+	struct i2c_client *client = lv0104cs->client;
+	int ret;
+
+	ret = i2c_master_send(client, (char *)&regval, sizeof(regval));
+	if (ret != sizeof(regval)) {

Hmm. In this case if it's positive you are fine as it'll never be greater than what you try to send. However, that isn't immediately obvious so as below I would add a specific check.

Quoted 7 lines

drivers/iio/light/lv0104cs.c new file +7 -0

+		dev_err(&client->dev, "Failed to write to device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+

drivers/iio/light/lv0104cs.c line 162

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+static int lv0104cs_read_adc(struct lv0104cs_private *lv0104cs, u16 *adc_output)
+{
+	struct i2c_client *client = lv0104cs->client;
+	__be16 regval;
+	int ret;
+
+	ret = i2c_master_recv(client, (char *)&regval, sizeof(regval));
+	if (ret != sizeof(regval)) {
+		dev_err(&client->dev, "Failed to read from device: %d\n", ret);
+		return ret;

This will return a non error value > 1 in the case of a mysteriously short transfer. So you need to check that length is negative as well and return -EIO or similar if not.

Quoted 39 lines

drivers/iio/light/lv0104cs.c new file +39 -0

+	}
+
+	*adc_output = be16_to_cpu(regval);
+
+	return 0;
+}
+
+static int lv0104cs_get_lux(struct lv0104cs_private *lv0104cs,
+				int *val, int *val2)
+{
+	u8 regval = LV0104CS_REGVAL_MEASURE;
+	u16 adc_output;
+	int ret;
+
+	regval |= lv0104cs_scales[lv0104cs->scale].regval;
+	regval |= lv0104cs_int_times[lv0104cs->int_time].regval;
+	ret = lv0104cs_write_reg(lv0104cs, regval);
+	if (ret)
+		return ret;
+
+	/* wait for integration time to pass (with margin) */
+	switch (lv0104cs->int_time) {
+	case LV0104CS_INTEG_12_5MS:
+		msleep(50);
+		break;
+
+	case LV0104CS_INTEG_100MS:
+		msleep(150);
+		break;
+
+	case LV0104CS_INTEG_200MS:
+		msleep(250);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = lv0104cs_read_adc(lv0104cs, &adc_output);

drivers/iio/light/lv0104cs.c line 211

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+	if (ret)
+		return ret;
+
+	ret = lv0104cs_write_reg(lv0104cs, LV0104CS_REGVAL_SLEEP);
+	if (ret)
+		return ret;
+
+	/* convert ADC output to lux */
+	switch (lv0104cs->scale) {
+	case LV0104CS_SCALE_0_25X:

Hmm. Given how simple the scale application is and the fact that we aren't trying dynamic control (which makes this complex) we 'could' go for the IIO default option of providing the raw value and letting userspace deal with the calibration.

However, this is a slow device (fairly anyway) so the cost of conversion is trivial and we are unlikely to ever want to bother with a buffered interface on this I think things are fine as you have them.

Quoted 333 lines

drivers/iio/light/lv0104cs.c new file +329 -1

+		*val = adc_output * 4;
+		*val2 = 0;
+		return 0;
+
+	case LV0104CS_SCALE_1X:
+		*val = adc_output;
+		*val2 = 0;
+		return 0;
+
+	case LV0104CS_SCALE_2X:
+		*val = adc_output / 2;
+		*val2 = (adc_output % 2) * 500000;
+		return 0;
+
+	case LV0104CS_SCALE_8X:
+		*val = adc_output / 8;
+		*val2 = (adc_output % 8) * 125000;
+		return 0;
 
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int lv0104cs_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = lv0104cs_get_lux(lv0104cs, val, val2);
+		if (ret)
+			goto err_mutex;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*val = lv0104cs_calibscales[lv0104cs->calibscale].val;
+		*val2 = lv0104cs_calibscales[lv0104cs->calibscale].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = lv0104cs_scales[lv0104cs->scale].val;
+		*val2 = lv0104cs_scales[lv0104cs->scale].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = lv0104cs_int_times[lv0104cs->int_time].val;
+		*val2 = lv0104cs_int_times[lv0104cs->int_time].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+err_mutex:
+	mutex_unlock(&lv0104cs->lock);
+
+	return ret;
+}
+
+static int lv0104cs_set_calibscale(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int calibscale = val * 1000000 + val2;
+	int floor, ceil, mid;
+	int ret, i, index;
+
+	/* round to nearest quantized calibscale (sensitivity) */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales) - 1; i++) {
+		floor = lv0104cs_calibscales[i].val * 1000000
+				+ lv0104cs_calibscales[i].val2;
+		ceil = lv0104cs_calibscales[i + 1].val * 1000000
+				+ lv0104cs_calibscales[i + 1].val2;
+		mid = (floor + ceil) / 2;
+
+		/* round down */
+		if (calibscale >= floor && calibscale < mid) {
+			index = i;
+			break;
+		}
+
+		/* round up */
+		if (calibscale >= mid && calibscale <= ceil) {
+			index = i + 1;
+			break;
+		}
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_calibscales) - 1)
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+
+	/* set calibscale (sensitivity) */
+	ret = lv0104cs_write_reg(lv0104cs, lv0104cs_calibscales[index].regval);
+	if (ret)
+		goto err_mutex;
+
+	lv0104cs->calibscale = index;
+
+err_mutex:
+	mutex_unlock(&lv0104cs->lock);
+
+	return ret;
+}
+
+static int lv0104cs_set_scale(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_scales); i++) {
+		if (val != lv0104cs_scales[i].val)
+			continue;
+
+		if (val2 == lv0104cs_scales[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_scales))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->scale = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_int_time(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		if (val != lv0104cs_int_times[i].val)
+			continue;
+
+		if (val2 == lv0104cs_int_times[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_int_times))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->int_time = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_write_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return lv0104cs_set_calibscale(lv0104cs, val, val2);
+
+	case IIO_CHAN_INFO_SCALE:
+		return lv0104cs_set_scale(lv0104cs, val, val2);
+
+	case IIO_CHAN_INFO_INT_TIME:
+		return lv0104cs_set_int_time(lv0104cs, val, val2);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t lv0104cs_show_calibscale_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_calibscales[i].val,
+				lv0104cs_calibscales[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_scale_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_scales); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_scales[i].val,
+				lv0104cs_scales[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_int_time_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_int_times[i].val,
+				lv0104cs_int_times[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(calibscale_available, 0444,
+				lv0104cs_show_calibscale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(scale_available, 0444,
+				lv0104cs_show_scale_avail, NULL, 0);
+static IIO_DEV_ATTR_INT_TIME_AVAIL(lv0104cs_show_int_time_avail);
+
+static struct attribute *lv0104cs_attributes[] = {
+	&iio_dev_attr_calibscale_available.dev_attr.attr,
+	&iio_dev_attr_scale_available.dev_attr.attr,
+	&iio_dev_attr_integration_time_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lv0104cs_attribute_group = {
+	.attrs = lv0104cs_attributes,
+};
+
+static const struct iio_info lv0104cs_info = {
+	.attrs = &lv0104cs_attribute_group,
+	.read_raw = &lv0104cs_read_raw,
+	.write_raw = &lv0104cs_write_raw,
+};
+
+static const struct iio_chan_spec lv0104cs_channels[] = {
+	{
+		.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),
+	},
+};
+
+static int lv0104cs_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct lv0104cs_private *lv0104cs;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*lv0104cs));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	lv0104cs = iio_priv(indio_dev);
+
+	i2c_set_clientdata(client, lv0104cs);
+	lv0104cs->client = client;
+
+	mutex_init(&lv0104cs->lock);
+
+	lv0104cs->calibscale = LV0104CS_CALIBSCALE_UNITY;
+	lv0104cs->scale = LV0104CS_SCALE_1X;
+	lv0104cs->int_time = LV0104CS_INTEG_200MS;
+
+	ret = lv0104cs_write_reg(lv0104cs,
+			lv0104cs_calibscales[LV0104CS_CALIBSCALE_UNITY].regval);
+	if (ret)
+		return -ENODEV;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = lv0104cs_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lv0104cs_channels);
+	indio_dev->name = client->name;
+	indio_dev->info = &lv0104cs_info;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id lv0104cs_id[] = {
+	{ "lv0104cs", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lv0104cs_id);
+
+static struct i2c_driver lv0104cs_i2c_driver = {
+	.driver = {
+		.name	= "lv0104cs",
+	},
+	.id_table	= lv0104cs_id,
+	.probe		= lv0104cs_probe,
+};
+module_i2c_driver(lv0104cs_i2c_driver);
+
+MODULE_AUTHOR("Jeff LaBundy <[email protected]>");
+MODULE_DESCRIPTION("LV0104CS Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL v2");
--

2.7.4

Jeff LaBundy

author

2018-02-25 02:38 UTC #7 lore raw

Quoted 6 lines · Jonathan Cameron

Quoted 3 lines · On Sun, 18 Feb 2018 12:25:07 -0600 Jeff LaBundy <[email protected]> wrote:
This patch adds support for the On Semiconductor LV0104CS ambient light sensor.

Quoted 8 lines

Quoted 1 line
Signed-off-by Jeff LaBundy <[email protected]>
Hi Jeff,
It is a bit kernel maintainer dependant but I would always much
rather see a fresh email thread for each version. Otherwise we run
a risk of confusion if the thread gets very deep. Obviously not a problem yet!

Sure thing; I will omit the in-reply-to field for future versions. That certainly makes sense to me (if I have misinterpreted your comment please let me know).

Quoted 3 lines

Some really minor stuff inline, but the error handling needs fixing in at least one place.

Thank you for the review; I agree with all of your feedback and will address your comments in a v3.

Quoted 17 lines

Jonathan
Quoted 14 lines
---
Changes in v2:
- Added manufacturer name to Kconfig entry
- Replaced description of register interface in introductory comments with link to data sheet
- Added units to integration time macros
- Replaced scale and integration time hard-coded register values with macros
- Replaced unsigned char with u8 throughout

Quoted 7 lines

Quoted 5 lines
- Replaced break statements with return statements within switch() blocks located at the end of functions throughout
- Replaced hard-coded lengths with sizeof() in calls to i2c_master_send and i2c_master_recv
This good, but you could return 1 rather than an error in the case of a short message (a slightly horrible artefact of the i2c interface!)

Great catch; I will evaluate negative return values and unexpected positive return values separately.

Quoted 70 lines

- Used big-endian kernel macros to resolve 16-bit ADC output value and replaced an instance of unsigned int with u16 in lv0104cs_read_adc

- Removed unnecessary mutex lock check from lv0104cs_get_lux

- Moved register write inside mutex lock in lv0104cs_set_calibscale

- Replaced IIO_CHAN_INFO_HARDWAREGAIN with IIO_CHAN_INFO_SCALE

- Removed superfluous success/failure printing following device registration

- Collapsed sizeof(struct lv0104cs_private) to sizeof(*lv0104cs) in

    lv0104cs_probe

- Removed extraneous whitespace in lv0104cs_i2c_driver declaration

 drivers/iio/light/Kconfig    |  10 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/lv0104cs.c | 540 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 551 insertions(+)
 create mode 100644 drivers/iio/light/lv0104cs.c

drivers/iio/light/Kconfig +10 -0

index 2356ed9..be0b313 100644
@@ -275,6 +275,16 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config LV0104CS
+	tristate "LV0104CS Ambient Light Sensor"
+	depends on I2C
+	help
+	 Say Y here if you want to build support for the On Semiconductor
+	 LV0104CS ambient light sensor.
+
+	 To compile this driver as a module, choose M here:
+	 the module will be called lv0104cs.
+
 config MAX44000
 	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
 	depends on I2C

drivers/iio/light/Makefile +1 -0

index fa32fa4..77c8682 100644
@@ -25,6 +25,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_LV0104CS)		+= lv0104cs.o
 obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o

drivers/iio/light/lv0104cs.c new file +7 -0

new file mode 100644
index 0000000..b783f90
@@ -0,0 +1,540 @@
+/*
+ * lv0104cs.c: LV0104CS Ambient Light Sensor Driver
+ *
+ * Copyright (C) 2018 Jeff LaBundy <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 of the License

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 13
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +6 -0
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
Up to you but SPDX licence tag is a possible alternative to stating this license block.

Sure thing, will do.

Quoted 7 lines

drivers/iio/light/lv0104cs.c line 18
Quoted 6 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+ *
+ * 7-bit I2C slave address: 0x13
+ *
+ * Link to data sheet: http://www.onsemi.com/pub/Collateral/LV0104CS-D.PDF
+ *
Nitpick, a blank line doesn't add anything here.

Sure thing, will do.

Quoted 122 lines

drivers/iio/light/lv0104cs.c new file +122 -0

+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/mutex.h>
+#include <linux/delay.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define LV0104CS_REGVAL_MEASURE		0xE0
+#define LV0104CS_REGVAL_SLEEP		0x00
+
+#define LV0104CS_SCALE_0_25X		0
+#define LV0104CS_SCALE_1X		1
+#define LV0104CS_SCALE_2X		2
+#define LV0104CS_SCALE_8X		3
+#define LV0104CS_SCALE_SHIFT		3
+
+#define LV0104CS_INTEG_12_5MS		0
+#define LV0104CS_INTEG_100MS		1
+#define LV0104CS_INTEG_200MS		2
+#define LV0104CS_INTEG_SHIFT		1
+
+#define LV0104CS_CALIBSCALE_UNITY	31
+
+struct lv0104cs_private {
+	struct i2c_client *client;
+	struct mutex lock;
+	u8 calibscale;
+	u8 scale;
+	u8 int_time;
+};
+
+struct lv0104cs_mapping {
+	int val;
+	int val2;
+	u8 regval;
+};
+
+static const struct lv0104cs_mapping lv0104cs_calibscales[] = {
+	{ 0, 666666, 0x81 },
+	{ 0, 800000, 0x82 },
+	{ 0, 857142, 0x83 },
+	{ 0, 888888, 0x84 },
+	{ 0, 909090, 0x85 },
+	{ 0, 923076, 0x86 },
+	{ 0, 933333, 0x87 },
+	{ 0, 941176, 0x88 },
+	{ 0, 947368, 0x89 },
+	{ 0, 952380, 0x8A },
+	{ 0, 956521, 0x8B },
+	{ 0, 960000, 0x8C },
+	{ 0, 962962, 0x8D },
+	{ 0, 965517, 0x8E },
+	{ 0, 967741, 0x8F },
+	{ 0, 969696, 0x90 },
+	{ 0, 971428, 0x91 },
+	{ 0, 972972, 0x92 },
+	{ 0, 974358, 0x93 },
+	{ 0, 975609, 0x94 },
+	{ 0, 976744, 0x95 },
+	{ 0, 977777, 0x96 },
+	{ 0, 978723, 0x97 },
+	{ 0, 979591, 0x98 },
+	{ 0, 980392, 0x99 },
+	{ 0, 981132, 0x9A },
+	{ 0, 981818, 0x9B },
+	{ 0, 982456, 0x9C },
+	{ 0, 983050, 0x9D },
+	{ 0, 983606, 0x9E },
+	{ 0, 984126, 0x9F },
+	{ 1, 0, 0x80 },
+	{ 1, 16129, 0xBF },
+	{ 1, 16666, 0xBE },
+	{ 1, 17241, 0xBD },
+	{ 1, 17857, 0xBC },
+	{ 1, 18518, 0xBB },
+	{ 1, 19230, 0xBA },
+	{ 1, 20000, 0xB9 },
+	{ 1, 20833, 0xB8 },
+	{ 1, 21739, 0xB7 },
+	{ 1, 22727, 0xB6 },
+	{ 1, 23809, 0xB5 },
+	{ 1, 24999, 0xB4 },
+	{ 1, 26315, 0xB3 },
+	{ 1, 27777, 0xB2 },
+	{ 1, 29411, 0xB1 },
+	{ 1, 31250, 0xB0 },
+	{ 1, 33333, 0xAF },
+	{ 1, 35714, 0xAE },
+	{ 1, 38461, 0xAD },
+	{ 1, 41666, 0xAC },
+	{ 1, 45454, 0xAB },
+	{ 1, 50000, 0xAA },
+	{ 1, 55555, 0xA9 },
+	{ 1, 62500, 0xA8 },
+	{ 1, 71428, 0xA7 },
+	{ 1, 83333, 0xA6 },
+	{ 1, 100000, 0xA5 },
+	{ 1, 125000, 0xA4 },
+	{ 1, 166666, 0xA3 },
+	{ 1, 250000, 0xA2 },
+	{ 1, 500000, 0xA1 },
+};
+
+static const struct lv0104cs_mapping lv0104cs_scales[] = {
+	{ 0, 250000, LV0104CS_SCALE_0_25X << LV0104CS_SCALE_SHIFT },
+	{ 1, 0, LV0104CS_SCALE_1X << LV0104CS_SCALE_SHIFT },
+	{ 2, 0, LV0104CS_SCALE_2X << LV0104CS_SCALE_SHIFT },
+	{ 8, 0, LV0104CS_SCALE_8X << LV0104CS_SCALE_SHIFT },
+};
+
+static const struct lv0104cs_mapping lv0104cs_int_times[] = {
+	{ 0, 12500, LV0104CS_INTEG_12_5MS << LV0104CS_INTEG_SHIFT },
+	{ 0, 100000, LV0104CS_INTEG_100MS << LV0104CS_INTEG_SHIFT },
+	{ 0, 200000, LV0104CS_INTEG_200MS << LV0104CS_INTEG_SHIFT },
+};
+
+static int lv0104cs_write_reg(struct lv0104cs_private *lv0104cs, u8 regval)
+{

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 145
Quoted 5 lines
drivers/iio/light/lv0104cs.c new file +5 -0
+	struct i2c_client *client = lv0104cs->client;
+	int ret;
+
+	ret = i2c_master_send(client, (char *)&regval, sizeof(regval));
+	if (ret != sizeof(regval)) {
Hmm. In this case if it's positive you are fine as it'll never be greater than what you try to send. However, that isn't immediately obvious so as below I would add a specific check.

Sure thing, will do (as per above).

Quoted 14 lines

drivers/iio/light/lv0104cs.c new file +13 -0

+		dev_err(&client->dev, "Failed to write to device: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int lv0104cs_read_adc(struct lv0104cs_private *lv0104cs, u16 *adc_output)
+{
+	struct i2c_client *client = lv0104cs->client;
+	__be16 regval;
+	int ret;
+

Quoted 8 lines

drivers/iio/light/lv0104cs.c line 162
Quoted 4 lines
drivers/iio/light/lv0104cs.c new file +4 -0
+	ret = i2c_master_recv(client, (char *)&regval, sizeof(regval));
+	if (ret != sizeof(regval)) {
+		dev_err(&client->dev, "Failed to read from device: %d\n", ret);
+		return ret;
This will return a non error value > 1 in the case of a mysteriously short transfer. So you need to check that length is negative as well and return -EIO or similar if not.

Sure thing, will do (as per above).

Quoted 56 lines

Quoted 41 lines

drivers/iio/light/lv0104cs.c new file +39 -0

+	}
+
+	*adc_output = be16_to_cpu(regval);
+
+	return 0;
+}
+
+static int lv0104cs_get_lux(struct lv0104cs_private *lv0104cs,
+				int *val, int *val2)
+{
+	u8 regval = LV0104CS_REGVAL_MEASURE;
+	u16 adc_output;
+	int ret;
+
+	regval |= lv0104cs_scales[lv0104cs->scale].regval;
+	regval |= lv0104cs_int_times[lv0104cs->int_time].regval;
+	ret = lv0104cs_write_reg(lv0104cs, regval);
+	if (ret)
+		return ret;
+
+	/* wait for integration time to pass (with margin) */
+	switch (lv0104cs->int_time) {
+	case LV0104CS_INTEG_12_5MS:
+		msleep(50);
+		break;
+
+	case LV0104CS_INTEG_100MS:
+		msleep(150);
+		break;
+
+	case LV0104CS_INTEG_200MS:
+		msleep(250);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = lv0104cs_read_adc(lv0104cs, &adc_output);

drivers/iio/light/lv0104cs.c line 211

Quoted 10 lines

drivers/iio/light/lv0104cs.c new file +10 -0

+	if (ret)
+		return ret;
+
+	ret = lv0104cs_write_reg(lv0104cs, LV0104CS_REGVAL_SLEEP);
+	if (ret)
+		return ret;
+
+	/* convert ADC output to lux */
+	switch (lv0104cs->scale) {
+	case LV0104CS_SCALE_0_25X:

Quoted 4 lines

However, this is a slow device (fairly anyway) so the cost of conversion is trivial and we are unlikely to ever want to bother with a buffered interface on this I think things are fine as you have them.

Sure thing, I'll leave this as-is. The reason for having chosen PROCESSED is because the device's ADC output already includes the effects of CALIBSCALE, so the device technically does not offer a RAW output on its own. Therefore I have presented a PROCESSED value that includes the SCALE that was in place when the measurement was captured.

Quoted 336 lines

Quoted 334 lines

drivers/iio/light/lv0104cs.c new file +329 -1

+		*val = adc_output * 4;
+		*val2 = 0;
+		return 0;
+
+	case LV0104CS_SCALE_1X:
+		*val = adc_output;
+		*val2 = 0;
+		return 0;
+
+	case LV0104CS_SCALE_2X:
+		*val = adc_output / 2;
+		*val2 = (adc_output % 2) * 500000;
+		return 0;
+
+	case LV0104CS_SCALE_8X:
+		*val = adc_output / 8;
+		*val2 = (adc_output % 8) * 125000;
+		return 0;
 
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int lv0104cs_read_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int *val, int *val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+		ret = lv0104cs_get_lux(lv0104cs, val, val2);
+		if (ret)
+			goto err_mutex;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_CALIBSCALE:
+		*val = lv0104cs_calibscales[lv0104cs->calibscale].val;
+		*val2 = lv0104cs_calibscales[lv0104cs->calibscale].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = lv0104cs_scales[lv0104cs->scale].val;
+		*val2 = lv0104cs_scales[lv0104cs->scale].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = lv0104cs_int_times[lv0104cs->int_time].val;
+		*val2 = lv0104cs_int_times[lv0104cs->int_time].val2;
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+err_mutex:
+	mutex_unlock(&lv0104cs->lock);
+
+	return ret;
+}
+
+static int lv0104cs_set_calibscale(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int calibscale = val * 1000000 + val2;
+	int floor, ceil, mid;
+	int ret, i, index;
+
+	/* round to nearest quantized calibscale (sensitivity) */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales) - 1; i++) {
+		floor = lv0104cs_calibscales[i].val * 1000000
+				+ lv0104cs_calibscales[i].val2;
+		ceil = lv0104cs_calibscales[i + 1].val * 1000000
+				+ lv0104cs_calibscales[i + 1].val2;
+		mid = (floor + ceil) / 2;
+
+		/* round down */
+		if (calibscale >= floor && calibscale < mid) {
+			index = i;
+			break;
+		}
+
+		/* round up */
+		if (calibscale >= mid && calibscale <= ceil) {
+			index = i + 1;
+			break;
+		}
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_calibscales) - 1)
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+
+	/* set calibscale (sensitivity) */
+	ret = lv0104cs_write_reg(lv0104cs, lv0104cs_calibscales[index].regval);
+	if (ret)
+		goto err_mutex;
+
+	lv0104cs->calibscale = index;
+
+err_mutex:
+	mutex_unlock(&lv0104cs->lock);
+
+	return ret;
+}
+
+static int lv0104cs_set_scale(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_scales); i++) {
+		if (val != lv0104cs_scales[i].val)
+			continue;
+
+		if (val2 == lv0104cs_scales[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_scales))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->scale = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_set_int_time(struct lv0104cs_private *lv0104cs,
+				int val, int val2)
+{
+	int i;
+
+	/* hard matching */
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		if (val != lv0104cs_int_times[i].val)
+			continue;
+
+		if (val2 == lv0104cs_int_times[i].val2)
+			break;
+	}
+
+	if (i == ARRAY_SIZE(lv0104cs_int_times))
+		return -EINVAL;
+
+	mutex_lock(&lv0104cs->lock);
+	lv0104cs->int_time = i;
+	mutex_unlock(&lv0104cs->lock);
+
+	return 0;
+}
+
+static int lv0104cs_write_raw(struct iio_dev *indio_dev,
+				struct iio_chan_spec const *chan,
+				int val, int val2, long mask)
+{
+	struct lv0104cs_private *lv0104cs = iio_priv(indio_dev);
+
+	if (chan->type != IIO_LIGHT)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		return lv0104cs_set_calibscale(lv0104cs, val, val2);
+
+	case IIO_CHAN_INFO_SCALE:
+		return lv0104cs_set_scale(lv0104cs, val, val2);
+
+	case IIO_CHAN_INFO_INT_TIME:
+		return lv0104cs_set_int_time(lv0104cs, val, val2);
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static ssize_t lv0104cs_show_calibscale_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_calibscales); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_calibscales[i].val,
+				lv0104cs_calibscales[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_scale_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_scales); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_scales[i].val,
+				lv0104cs_scales[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static ssize_t lv0104cs_show_int_time_avail(struct device *dev,
+				struct device_attribute *attr, char *buf)
+{
+	ssize_t len = 0;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lv0104cs_int_times); i++) {
+		len += scnprintf(buf + len, PAGE_SIZE - len, "%d.%06d ",
+				lv0104cs_int_times[i].val,
+				lv0104cs_int_times[i].val2);
+	}
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(calibscale_available, 0444,
+				lv0104cs_show_calibscale_avail, NULL, 0);
+static IIO_DEVICE_ATTR(scale_available, 0444,
+				lv0104cs_show_scale_avail, NULL, 0);
+static IIO_DEV_ATTR_INT_TIME_AVAIL(lv0104cs_show_int_time_avail);
+
+static struct attribute *lv0104cs_attributes[] = {
+	&iio_dev_attr_calibscale_available.dev_attr.attr,
+	&iio_dev_attr_scale_available.dev_attr.attr,
+	&iio_dev_attr_integration_time_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group lv0104cs_attribute_group = {
+	.attrs = lv0104cs_attributes,
+};
+
+static const struct iio_info lv0104cs_info = {
+	.attrs = &lv0104cs_attribute_group,
+	.read_raw = &lv0104cs_read_raw,
+	.write_raw = &lv0104cs_write_raw,
+};
+
+static const struct iio_chan_spec lv0104cs_channels[] = {
+	{
+		.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),
+	},
+};
+
+static int lv0104cs_probe(struct i2c_client *client,
+				const struct i2c_device_id *id)
+{
+	struct iio_dev *indio_dev;
+	struct lv0104cs_private *lv0104cs;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*lv0104cs));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	lv0104cs = iio_priv(indio_dev);
+
+	i2c_set_clientdata(client, lv0104cs);
+	lv0104cs->client = client;
+
+	mutex_init(&lv0104cs->lock);
+
+	lv0104cs->calibscale = LV0104CS_CALIBSCALE_UNITY;
+	lv0104cs->scale = LV0104CS_SCALE_1X;
+	lv0104cs->int_time = LV0104CS_INTEG_200MS;
+
+	ret = lv0104cs_write_reg(lv0104cs,
+			lv0104cs_calibscales[LV0104CS_CALIBSCALE_UNITY].regval);
+	if (ret)
+		return -ENODEV;
+
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->channels = lv0104cs_channels;
+	indio_dev->num_channels = ARRAY_SIZE(lv0104cs_channels);
+	indio_dev->name = client->name;
+	indio_dev->info = &lv0104cs_info;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id lv0104cs_id[] = {
+	{ "lv0104cs", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, lv0104cs_id);
+
+static struct i2c_driver lv0104cs_i2c_driver = {
+	.driver = {
+		.name	= "lv0104cs",
+	},
+	.id_table	= lv0104cs_id,
+	.probe		= lv0104cs_probe,
+};
+module_i2c_driver(lv0104cs_i2c_driver);
+
+MODULE_AUTHOR("Jeff LaBundy <[email protected]>");
+MODULE_DESCRIPTION("LV0104CS Ambient Light Sensor Driver");
+MODULE_LICENSE("GPL v2");
--

2.7.4

Jonathan Cameron

maintainer

2018-03-03 14:57 UTC #8 lore raw

On Sat, 24 Feb 2018 20:38:08 -0600
Jeff LaBundy <[email protected]> wrote:

<snip>

Quoted 52 lines

Quoted 47 lines
Quoted 32 lines
drivers/iio/light/lv0104cs.c new file +32 -0
+static int lv0104cs_get_lux(struct lv0104cs_private *lv0104cs,
+				int *val, int *val2)
+{
+	u8 regval = LV0104CS_REGVAL_MEASURE;
+	u16 adc_output;
+	int ret;
+
+	regval |= lv0104cs_scales[lv0104cs->scale].regval;
+	regval |= lv0104cs_int_times[lv0104cs->int_time].regval;
+	ret = lv0104cs_write_reg(lv0104cs, regval);
+	if (ret)
+		return ret;
+
+	/* wait for integration time to pass (with margin) */
+	switch (lv0104cs->int_time) {
+	case LV0104CS_INTEG_12_5MS:
+		msleep(50);
+		break;
+
+	case LV0104CS_INTEG_100MS:
+		msleep(150);
+		break;
+
+	case LV0104CS_INTEG_200MS:
+		msleep(250);
+		break;
+
+	default:
+		return -EINVAL;
+	}
+
+	ret = lv0104cs_read_adc(lv0104cs, &adc_output);
drivers/iio/light/lv0104cs.c line 211
Quoted 10 lines
drivers/iio/light/lv0104cs.c new file +10 -0
+	if (ret)
+		return ret;
+
+	ret = lv0104cs_write_reg(lv0104cs, LV0104CS_REGVAL_SLEEP);
+	if (ret)
+		return ret;
+
+	/* convert ADC output to lux */
+	switch (lv0104cs->scale) {
+	case LV0104CS_SCALE_0_25X:  
Hmm. Given how simple the scale application is and the fact that we aren't trying dynamic control (which makes this complex) we 'could' go for the IIO default option of providing the raw value and letting userspace deal with the calibration.
Quoted 4 lines
However, this is a slow device (fairly anyway) so the cost of conversion is trivial and we are unlikely to ever want to bother with a buffered interface on this I think things are fine as you have them.

Quoted 6 lines

Sure thing, I'll leave this as-is. The reason for having chosen PROCESSED is because the device's ADC output already includes the effects of CALIBSCALE, so the device technically does not offer a RAW output on its own. Therefore I have presented a PROCESSED value that includes the SCALE that was in place when the measurement was captured.

It's normal for a device to already include the effect of a calibscale as they are often done by tweaking a DAC in the analog section of the sensor circuit. So don't let that argue against providing a _RAW value.

The answer is still fine though, just thought I'd comment on the reasoning ;)

<snip>