[PATCH v2 0/4] iio: vcnl4000: add support for vcnl4200

[PATCH v2 0/4] iio: vcnl4000: add support for vcnl4200

[Tomas Novotny]

This is the second iteration of vcnl4200 support. The first one was posted
on February, sorry for the long delay. I've implemented all notes from
reviews (thanks to Peter and Jonathan) and nothing more.

Changes in v2:
- reading light and proximity values for vcnl4200 is blocking (if you
  request new value too early)
- patches 2 and 3 were added; original patch 2 is now patch 4
- vcnl4010 id is handled (patch 2)
- warn user on incorrect usage of vcnl40{0,1}0 id (patch 3)
- minor stuff (add parenthesis, switch instead of if, rename sensors to
  channels, fix return)

Please note that I'm not sure if it is still good idea to have same driver
for vcnl4000 and vcnl4200. The amount of different parts in v2 is even
bigger. Just see below for differences and add the blocking read which is
added in v2 for vcnl4200.

Cover letter from v1:

VCNL4200 is another proximity and ambient light sensor from Vishay. I'm
adding support for that sensor to vcnl4000 driver, which currently supports
VCNL4000/10/20.

The VCNL4200 is a bit different from VCNL4000/10/20. Common things are:
- integrated proximity and ambient light sensor
- SMBus compatible I2C interface
- Vishay VCNL4xxx series...

Different things are:
- totally different register map
- 8-bit vs. 16-bit registers. The 16-bit values are in two 8-bit registers
  on VCNL4000. 16-bit value is in one register on VCNL4200.
- VCNL4000 has flags when the measurement is finished

The first patch generalizes the driver to support differencies. The second
patch adds the support for VCNL4200.

It is tested on VCNL4020 and VCNL4200.

Tomas Novotny (4):
  iio: vcnl4000: make the driver extendable
  iio: vcnl4000: add VCNL4010 device id
  iio: vcnl4000: warn on incorrectly specified device id
  iio: vcnl4000: add support for VCNL4200

 drivers/iio/light/Kconfig    |   5 +-
 drivers/iio/light/vcnl4000.c | 219 ++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 196 insertions(+), 28 deletions(-)

-- 
2.12.3

[PATCH v2 1/4] iio: vcnl4000: make the driver extendable

[Tomas Novotny]

There are similar chips in the vcnl4xxx family. The initialization and
communication is a bit different for members of the family, so this
patch makes the driver extendable for different chips.

There is no functional change.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/light/vcnl4000.c | 85 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 67 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index c599a90506ad..32c0b531395f 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -26,8 +26,8 @@
 #include <linux/iio/sysfs.h>
 
 #define VCNL4000_DRV_NAME "vcnl4000"
-#define VCNL4000_ID		0x01
-#define VCNL4010_ID		0x02 /* for VCNL4020, VCNL4010 */
+#define VCNL4000_PROD_ID	0x01
+#define VCNL4010_PROD_ID	0x02 /* for VCNL4020, VCNL4010 */
 
 #define VCNL4000_COMMAND	0x80 /* Command register */
 #define VCNL4000_PROD_REV	0x81 /* Product ID and Revision ID */
@@ -46,17 +46,50 @@
 #define VCNL4000_AL_OD		BIT(4) /* start on-demand ALS measurement */
 #define VCNL4000_PS_OD		BIT(3) /* start on-demand proximity measurement */
 
+enum vcnl4000_device_ids {
+	VCNL4000,
+};
+
 struct vcnl4000_data {
 	struct i2c_client *client;
+	enum vcnl4000_device_ids id;
+	int rev;
+	int al_scale;
+	const struct vcnl4000_chip_spec *chip_spec;
 	struct mutex lock;
 };
 
+struct vcnl4000_chip_spec {
+	const char *prod;
+	int (*init)(struct vcnl4000_data *data);
+	int (*measure_light)(struct vcnl4000_data *data, int *val);
+	int (*measure_proximity)(struct vcnl4000_data *data, int *val);
+};
+
 static const struct i2c_device_id vcnl4000_id[] = {
-	{ "vcnl4000", 0 },
+	{ "vcnl4000", VCNL4000 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
 
+static int vcnl4000_init(struct vcnl4000_data *data)
+{
+	int ret, prod_id;
+
+	ret = i2c_smbus_read_byte_data(data->client, VCNL4000_PROD_REV);
+	if (ret < 0)
+		return ret;
+
+	prod_id = ret >> 4;
+	if (prod_id != VCNL4010_PROD_ID && prod_id != VCNL4000_PROD_ID)
+		return -ENODEV;
+
+	data->rev = ret & 0xf;
+	data->al_scale = 250000;
+
+	return 0;
+};
+
 static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 				u8 rdy_mask, u8 data_reg, int *val)
 {
@@ -103,6 +136,29 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 	return ret;
 }
 
+static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
+{
+	return vcnl4000_measure(data,
+			VCNL4000_AL_OD, VCNL4000_AL_RDY,
+			VCNL4000_AL_RESULT_HI, val);
+}
+
+static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
+{
+	return vcnl4000_measure(data,
+			VCNL4000_PS_OD, VCNL4000_PS_RDY,
+			VCNL4000_PS_RESULT_HI, val);
+}
+
+static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
+	[VCNL4000] = {
+		.prod = "VCNL4000",
+		.init = vcnl4000_init,
+		.measure_light = vcnl4000_measure_light,
+		.measure_proximity = vcnl4000_measure_proximity,
+	},
+};
+
 static const struct iio_chan_spec vcnl4000_channels[] = {
 	{
 		.type = IIO_LIGHT,
@@ -125,16 +181,12 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_RAW:
 		switch (chan->type) {
 		case IIO_LIGHT:
-			ret = vcnl4000_measure(data,
-				VCNL4000_AL_OD, VCNL4000_AL_RDY,
-				VCNL4000_AL_RESULT_HI, val);
+			ret = data->chip_spec->measure_light(data, val);
 			if (ret < 0)
 				return ret;
 			return IIO_VAL_INT;
 		case IIO_PROXIMITY:
-			ret = vcnl4000_measure(data,
-				VCNL4000_PS_OD, VCNL4000_PS_RDY,
-				VCNL4000_PS_RESULT_HI, val);
+			ret = data->chip_spec->measure_proximity(data, val);
 			if (ret < 0)
 				return ret;
 			return IIO_VAL_INT;
@@ -146,7 +198,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 			return -EINVAL;
 
 		*val = 0;
-		*val2 = 250000;
+		*val2 = data->al_scale;
 		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
@@ -162,7 +214,7 @@ static int vcnl4000_probe(struct i2c_client *client,
 {
 	struct vcnl4000_data *data;
 	struct iio_dev *indio_dev;
-	int ret, prod_id;
+	int ret;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -171,19 +223,16 @@ static int vcnl4000_probe(struct i2c_client *client,
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
+	data->id = id->driver_data;
+	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
 	mutex_init(&data->lock);
 
-	ret = i2c_smbus_read_byte_data(data->client, VCNL4000_PROD_REV);
+	ret = data->chip_spec->init(data);
 	if (ret < 0)
 		return ret;
 
-	prod_id = ret >> 4;
-	if (prod_id != VCNL4010_ID && prod_id != VCNL4000_ID)
-		return -ENODEV;
-
 	dev_dbg(&client->dev, "%s Ambient light/proximity sensor, Rev: %02x\n",
-		(prod_id == VCNL4010_ID) ? "VCNL4010/4020" : "VCNL4000",
-		ret & 0xf);
+		data->chip_spec->prod, data->rev);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &vcnl4000_info;
-- 
2.12.3

[PATCH v2 2/4] iio: vcnl4000: add VCNL4010 device id

[Tomas Novotny]

The driver already supports VCNL4010/20 devices. The supported features
and detectable product id are the same, so add shared id for them.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/light/vcnl4000.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 32c0b531395f..0688214fc152 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -48,6 +48,7 @@
 
 enum vcnl4000_device_ids {
 	VCNL4000,
+	VCNL4010,
 };
 
 struct vcnl4000_data {
@@ -68,6 +69,7 @@ struct vcnl4000_chip_spec {
 
 static const struct i2c_device_id vcnl4000_id[] = {
 	{ "vcnl4000", VCNL4000 },
+	{ "vcnl4010", VCNL4010 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
@@ -157,6 +159,12 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.measure_light = vcnl4000_measure_light,
 		.measure_proximity = vcnl4000_measure_proximity,
 	},
+	[VCNL4010] = {
+		.prod = "VCNL4010/4020",
+		.init = vcnl4000_init,
+		.measure_light = vcnl4000_measure_light,
+		.measure_proximity = vcnl4000_measure_proximity,
+	},
 };
 
 static const struct iio_chan_spec vcnl4000_channels[] = {
-- 
2.12.3

[PATCH v2 3/4] iio: vcnl4000: warn on incorrectly specified device id

[Tomas Novotny]

We can detect incorrectly specified device id for some chips, so warn
user in that case.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/light/vcnl4000.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 0688214fc152..642a366c1479 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -83,8 +83,20 @@ static int vcnl4000_init(struct vcnl4000_data *data)
 		return ret;
 
 	prod_id = ret >> 4;
-	if (prod_id != VCNL4010_PROD_ID && prod_id != VCNL4000_PROD_ID)
+	switch (prod_id) {
+	case VCNL4000_PROD_ID:
+		if (data->id != VCNL4000)
+			dev_warn(&data->client->dev,
+					"wrong device id, use vcnl4000");
+		break;
+	case VCNL4010_PROD_ID:
+		if (data->id != VCNL4010)
+			dev_warn(&data->client->dev,
+					"wrong device id, use vcnl4010");
+		break;
+	default:
 		return -ENODEV;
+	}
 
 	data->rev = ret & 0xf;
 	data->al_scale = 250000;
-- 
2.12.3

[PATCH v2 4/4] iio: vcnl4000: add support for VCNL4200

[Tomas Novotny]

VCNL4200 is an integrated long distance (up to 1500mm) proximity and
ambient light sensor.

The support is very basic. There is no configuration of proximity and
ambient light sensing yet. Only the reading of both measured values is
done.

The reading of ambient light and proximity values is blocking. If you
request a new value too early, the driver waits for new value to be
ready.

Signed-off-by: Tomas Novotny <tomas@novotny.cz>
---
 drivers/iio/light/Kconfig    |   5 +-
 drivers/iio/light/vcnl4000.c | 114 ++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 109 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 074e50657366..c0344a961f54 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -430,11 +430,12 @@ config US5182D
 	 will be called us5182d.
 
 config VCNL4000
-	tristate "VCNL4000/4010/4020 combined ALS and proximity sensor"
+	tristate "VCNL4000/4010/4020/4200 combined ALS and proximity sensor"
 	depends on I2C
 	help
 	 Say Y here if you want to build a driver for the Vishay VCNL4000,
-	 VCNL4010, VCNL4020 combined ambient light and proximity sensor.
+	 VCNL4010, VCNL4020, VCNL4200 combined ambient light and proximity
+	 sensor.
 
 	 To compile this driver as a module, choose M here: the
 	 module will be called vcnl4000.
diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 642a366c1479..5de0d500c310 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -1,5 +1,5 @@
 /*
- * vcnl4000.c - Support for Vishay VCNL4000/4010/4020 combined ambient
+ * vcnl4000.c - Support for Vishay VCNL4000/4010/4020/4200 combined ambient
  * light and proximity sensor
  *
  * Copyright 2012 Peter Meerwald <pmeerw@pmeerw.net>
@@ -8,13 +8,15 @@
  * the GNU General Public License.  See the file COPYING in the main
  * directory of this archive for more details.
  *
- * IIO driver for VCNL4000 (7-bit I2C slave address 0x13)
+ * IIO driver for:
+ *   VCNL4000/10/20 (7-bit I2C slave address 0x13)
+ *   VCNL4200 (7-bit I2C slave address 0x51)
  *
  * TODO:
  *   allow to adjust IR current
  *   proximity threshold and event handling
  *   periodic ALS/proximity measurement (VCNL4010/20)
- *   interrupts (VCNL4010/20)
+ *   interrupts (VCNL4010/20, VCNL4200)
  */
 
 #include <linux/module.h>
@@ -28,6 +30,7 @@
 #define VCNL4000_DRV_NAME "vcnl4000"
 #define VCNL4000_PROD_ID	0x01
 #define VCNL4010_PROD_ID	0x02 /* for VCNL4020, VCNL4010 */
+#define VCNL4200_PROD_ID	0x58
 
 #define VCNL4000_COMMAND	0x80 /* Command register */
 #define VCNL4000_PROD_REV	0x81 /* Product ID and Revision ID */
@@ -40,6 +43,12 @@
 #define VCNL4000_PS_MEAS_FREQ	0x89 /* Proximity test signal frequency */
 #define VCNL4000_PS_MOD_ADJ	0x8a /* Proximity modulator timing adjustment */
 
+#define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
+#define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
+#define VCNL4200_PS_DATA	0x08 /* Proximity data */
+#define VCNL4200_AL_DATA	0x09 /* Ambient light data */
+#define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
+
 /* Bit masks for COMMAND register */
 #define VCNL4000_AL_RDY		BIT(6) /* ALS data ready? */
 #define VCNL4000_PS_RDY		BIT(5) /* proximity data ready? */
@@ -49,6 +58,14 @@
 enum vcnl4000_device_ids {
 	VCNL4000,
 	VCNL4010,
+	VCNL4200,
+};
+
+struct vcnl4200_channel {
+	u8 reg;
+	ktime_t last_measurement;
+	ktime_t sampling_rate;
+	struct mutex lock;
 };
 
 struct vcnl4000_data {
@@ -57,7 +74,9 @@ struct vcnl4000_data {
 	int rev;
 	int al_scale;
 	const struct vcnl4000_chip_spec *chip_spec;
-	struct mutex lock;
+	struct mutex vcnl4000_lock;
+	struct vcnl4200_channel vcnl4200_al;
+	struct vcnl4200_channel vcnl4200_ps;
 };
 
 struct vcnl4000_chip_spec {
@@ -70,6 +89,7 @@ struct vcnl4000_chip_spec {
 static const struct i2c_device_id vcnl4000_id[] = {
 	{ "vcnl4000", VCNL4000 },
 	{ "vcnl4010", VCNL4010 },
+	{ "vcnl4200", VCNL4200 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
@@ -100,6 +120,42 @@ static int vcnl4000_init(struct vcnl4000_data *data)
 
 	data->rev = ret & 0xf;
 	data->al_scale = 250000;
+	mutex_init(&data->vcnl4000_lock);
+
+	return 0;
+};
+
+static int vcnl4200_init(struct vcnl4000_data *data)
+{
+	int ret;
+
+	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
+	if (ret < 0)
+		return ret;
+
+	if ((ret & 0xff) != VCNL4200_PROD_ID)
+		return -ENODEV;
+
+	data->rev = (ret >> 8) & 0xf;
+
+	/* Set defaults and enable both channels */
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
+	if (ret < 0)
+		return ret;
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
+	if (ret < 0)
+		return ret;
+
+	data->al_scale = 24000;
+	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
+	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
+	/* Integration time is 50ms, but the experiments show 54ms in total. */
+	data->vcnl4200_al.sampling_rate = ktime_set(0, 54000 * 1000);
+	data->vcnl4200_ps.sampling_rate = ktime_set(0, 4200 * 1000);
+	data->vcnl4200_al.last_measurement = ktime_set(0, 0);
+	data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
+	mutex_init(&data->vcnl4200_al.lock);
+	mutex_init(&data->vcnl4200_ps.lock);
 
 	return 0;
 };
@@ -111,7 +167,7 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 	__be16 buf;
 	int ret;
 
-	mutex_lock(&data->lock);
+	mutex_lock(&data->vcnl4000_lock);
 
 	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
 					req_mask);
@@ -140,16 +196,43 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 	if (ret < 0)
 		goto fail;
 
-	mutex_unlock(&data->lock);
+	mutex_unlock(&data->vcnl4000_lock);
 	*val = be16_to_cpu(buf);
 
 	return 0;
 
 fail:
-	mutex_unlock(&data->lock);
+	mutex_unlock(&data->vcnl4000_lock);
 	return ret;
 }
 
+static int vcnl4200_measure(struct vcnl4000_data *data,
+		struct vcnl4200_channel *chan, int *val)
+{
+	int ret;
+	s64 delta;
+	ktime_t next_measurement;
+
+	mutex_lock(&chan->lock);
+
+	next_measurement = ktime_add(chan->last_measurement,
+			chan->sampling_rate);
+	delta = ktime_us_delta(next_measurement, ktime_get());
+	if (delta > 0)
+		usleep_range(delta, delta + 500);
+	chan->last_measurement = ktime_get();
+
+	mutex_unlock(&chan->lock);
+
+	ret = i2c_smbus_read_word_data(data->client, chan->reg);
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+
+	return 0;
+}
+
 static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
 {
 	return vcnl4000_measure(data,
@@ -157,6 +240,11 @@ static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
 			VCNL4000_AL_RESULT_HI, val);
 }
 
+static int vcnl4200_measure_light(struct vcnl4000_data *data, int *val)
+{
+	return vcnl4200_measure(data, &data->vcnl4200_al, val);
+}
+
 static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
 {
 	return vcnl4000_measure(data,
@@ -164,6 +252,11 @@ static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
 			VCNL4000_PS_RESULT_HI, val);
 }
 
+static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
+{
+	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
+}
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.prod = "VCNL4000",
@@ -177,6 +270,12 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 		.measure_light = vcnl4000_measure_light,
 		.measure_proximity = vcnl4000_measure_proximity,
 	},
+	[VCNL4200] = {
+		.prod = "VCNL4200",
+		.init = vcnl4200_init,
+		.measure_light = vcnl4200_measure_light,
+		.measure_proximity = vcnl4200_measure_proximity,
+	},
 };
 
 static const struct iio_chan_spec vcnl4000_channels[] = {
@@ -245,7 +344,6 @@ static int vcnl4000_probe(struct i2c_client *client,
 	data->client = client;
 	data->id = id->driver_data;
 	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
-	mutex_init(&data->lock);
 
 	ret = data->chip_spec->init(data);
 	if (ret < 0)
-- 
2.12.3

Re: [PATCH v2 2/4] iio: vcnl4000: add VCNL4010 device id

[Jonathan Cameron]

On Tue, 17 Jul 2018 18:46:53 +0200
Tomas Novotny <tomas@novotny.cz> wrote:

> The driver already supports VCNL4010/20 devices. The supported features
> and detectable product id are the same, so add shared id for them.
> 
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>

I'm not totally getting why we need this...  See below.

> ---
>  drivers/iio/light/vcnl4000.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 32c0b531395f..0688214fc152 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -48,6 +48,7 @@
>  
>  enum vcnl4000_device_ids {
>  	VCNL4000,
> +	VCNL4010,
>  };
>  
>  struct vcnl4000_data {
> @@ -68,6 +69,7 @@ struct vcnl4000_chip_spec {
>  
>  static const struct i2c_device_id vcnl4000_id[] = {
>  	{ "vcnl4000", VCNL4000 },
> +	{ "vcnl4010", VCNL4010 },
	{ "vcnl4010", VCLN4000 }, then rest of the patch has no purpose

Also, should list the vcnl4020 id with the same enum entry to
explicitly support that one.

It would have made sense if we had split the ID checking to
verify we had the one we thought we had...  Not sure it really
matters if they are identical in all visible ways, but at least
it would have pointed out you didn't have what you thought you had.

Jonathan


>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> @@ -157,6 +159,12 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.measure_light = vcnl4000_measure_light,
>  		.measure_proximity = vcnl4000_measure_proximity,
>  	},
> +	[VCNL4010] = {
> +		.prod = "VCNL4010/4020",
> +		.init = vcnl4000_init,
> +		.measure_light = vcnl4000_measure_light,
> +		.measure_proximity = vcnl4000_measure_proximity,
> +	},
>  };
>  
>  static const struct iio_chan_spec vcnl4000_channels[] = {

Re: [PATCH v2 3/4] iio: vcnl4000: warn on incorrectly specified device id

[Jonathan Cameron]

On Tue, 17 Jul 2018 18:46:54 +0200
Tomas Novotny <tomas@novotny.cz> wrote:

> We can detect incorrectly specified device id for some chips, so warn
> user in that case.
> 
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
Ah, now the previous patch makes more sense.   Fair enough though
always remember reviewers tend to read one patch at a time, so making
it clear this was coming in the description for that one would have
been good.

Jonathan
> ---
>  drivers/iio/light/vcnl4000.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 0688214fc152..642a366c1479 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -83,8 +83,20 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>  		return ret;
>  
>  	prod_id = ret >> 4;
> -	if (prod_id != VCNL4010_PROD_ID && prod_id != VCNL4000_PROD_ID)
> +	switch (prod_id) {
> +	case VCNL4000_PROD_ID:
> +		if (data->id != VCNL4000)
> +			dev_warn(&data->client->dev,
> +					"wrong device id, use vcnl4000");
> +		break;
> +	case VCNL4010_PROD_ID:
> +		if (data->id != VCNL4010)
> +			dev_warn(&data->client->dev,
> +					"wrong device id, use vcnl4010");
> +		break;
> +	default:
>  		return -ENODEV;
> +	}
>  
>  	data->rev = ret & 0xf;
>  	data->al_scale = 250000;

Re: [PATCH v2 4/4] iio: vcnl4000: add support for VCNL4200

[Jonathan Cameron]

On Tue, 17 Jul 2018 18:46:55 +0200
Tomas Novotny <tomas@novotny.cz> wrote:

> VCNL4200 is an integrated long distance (up to 1500mm) proximity and
> ambient light sensor.
> 
> The support is very basic. There is no configuration of proximity and
> ambient light sensing yet. Only the reading of both measured values is
> done.
> 
> The reading of ambient light and proximity values is blocking. If you
> request a new value too early, the driver waits for new value to be
> ready.
> 
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>

A comment inline, but nothing much needs changing.

Jonathan

> ---
>  drivers/iio/light/Kconfig    |   5 +-
>  drivers/iio/light/vcnl4000.c | 114 ++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 109 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 074e50657366..c0344a961f54 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -430,11 +430,12 @@ config US5182D
>  	 will be called us5182d.
>  
>  config VCNL4000
> -	tristate "VCNL4000/4010/4020 combined ALS and proximity sensor"
> +	tristate "VCNL4000/4010/4020/4200 combined ALS and proximity sensor"
>  	depends on I2C
>  	help
>  	 Say Y here if you want to build a driver for the Vishay VCNL4000,
> -	 VCNL4010, VCNL4020 combined ambient light and proximity sensor.
> +	 VCNL4010, VCNL4020, VCNL4200 combined ambient light and proximity
> +	 sensor.
>  
>  	 To compile this driver as a module, choose M here: the
>  	 module will be called vcnl4000.
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 642a366c1479..5de0d500c310 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -1,5 +1,5 @@
>  /*
> - * vcnl4000.c - Support for Vishay VCNL4000/4010/4020 combined ambient
> + * vcnl4000.c - Support for Vishay VCNL4000/4010/4020/4200 combined ambient
>   * light and proximity sensor
>   *
>   * Copyright 2012 Peter Meerwald <pmeerw@pmeerw.net>
> @@ -8,13 +8,15 @@
>   * the GNU General Public License.  See the file COPYING in the main
>   * directory of this archive for more details.
>   *
> - * IIO driver for VCNL4000 (7-bit I2C slave address 0x13)
> + * IIO driver for:
> + *   VCNL4000/10/20 (7-bit I2C slave address 0x13)
> + *   VCNL4200 (7-bit I2C slave address 0x51)
>   *
>   * TODO:
>   *   allow to adjust IR current
>   *   proximity threshold and event handling
>   *   periodic ALS/proximity measurement (VCNL4010/20)
> - *   interrupts (VCNL4010/20)
> + *   interrupts (VCNL4010/20, VCNL4200)
>   */
>  
>  #include <linux/module.h>
> @@ -28,6 +30,7 @@
>  #define VCNL4000_DRV_NAME "vcnl4000"
>  #define VCNL4000_PROD_ID	0x01
>  #define VCNL4010_PROD_ID	0x02 /* for VCNL4020, VCNL4010 */
> +#define VCNL4200_PROD_ID	0x58
>  
>  #define VCNL4000_COMMAND	0x80 /* Command register */
>  #define VCNL4000_PROD_REV	0x81 /* Product ID and Revision ID */
> @@ -40,6 +43,12 @@
>  #define VCNL4000_PS_MEAS_FREQ	0x89 /* Proximity test signal frequency */
>  #define VCNL4000_PS_MOD_ADJ	0x8a /* Proximity modulator timing adjustment */
>  
> +#define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
> +#define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
> +#define VCNL4200_PS_DATA	0x08 /* Proximity data */
> +#define VCNL4200_AL_DATA	0x09 /* Ambient light data */
> +#define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
> +
>  /* Bit masks for COMMAND register */
>  #define VCNL4000_AL_RDY		BIT(6) /* ALS data ready? */
>  #define VCNL4000_PS_RDY		BIT(5) /* proximity data ready? */
> @@ -49,6 +58,14 @@
>  enum vcnl4000_device_ids {
>  	VCNL4000,
>  	VCNL4010,
> +	VCNL4200,
> +};
> +
> +struct vcnl4200_channel {
> +	u8 reg;
> +	ktime_t last_measurement;
> +	ktime_t sampling_rate;
> +	struct mutex lock;
>  };
>  
>  struct vcnl4000_data {
> @@ -57,7 +74,9 @@ struct vcnl4000_data {
>  	int rev;
>  	int al_scale;
>  	const struct vcnl4000_chip_spec *chip_spec;
> -	struct mutex lock;
> +	struct mutex vcnl4000_lock;
> +	struct vcnl4200_channel vcnl4200_al;
> +	struct vcnl4200_channel vcnl4200_ps;
>  };
>  
>  struct vcnl4000_chip_spec {
> @@ -70,6 +89,7 @@ struct vcnl4000_chip_spec {
>  static const struct i2c_device_id vcnl4000_id[] = {
>  	{ "vcnl4000", VCNL4000 },
>  	{ "vcnl4010", VCNL4010 },
> +	{ "vcnl4200", VCNL4200 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> @@ -100,6 +120,42 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>  
>  	data->rev = ret & 0xf;
>  	data->al_scale = 250000;
> +	mutex_init(&data->vcnl4000_lock);
> +
> +	return 0;
> +};
> +
> +static int vcnl4200_init(struct vcnl4000_data *data)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
> +	if (ret < 0)
> +		return ret;
> +
> +	if ((ret & 0xff) != VCNL4200_PROD_ID)
> +		return -ENODEV;
> +
> +	data->rev = (ret >> 8) & 0xf;
> +
> +	/* Set defaults and enable both channels */
> +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
> +	if (ret < 0)
> +		return ret;
> +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
> +	if (ret < 0)
> +		return ret;
> +
> +	data->al_scale = 24000;
> +	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
> +	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
> +	/* Integration time is 50ms, but the experiments show 54ms in total. */
> +	data->vcnl4200_al.sampling_rate = ktime_set(0, 54000 * 1000);
> +	data->vcnl4200_ps.sampling_rate = ktime_set(0, 4200 * 1000);

I'm not particularly keen on the mixing of constant and non constant
stuff in these, but I guess there isn't enough constant stuff to bother
factoring that out.

> +	data->vcnl4200_al.last_measurement = ktime_set(0, 0);
> +	data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
> +	mutex_init(&data->vcnl4200_al.lock);
> +	mutex_init(&data->vcnl4200_ps.lock);
>  
>  	return 0;
>  };
> @@ -111,7 +167,7 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  	__be16 buf;
>  	int ret;
>  
> -	mutex_lock(&data->lock);
> +	mutex_lock(&data->vcnl4000_lock);
>  
>  	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
>  					req_mask);
> @@ -140,16 +196,43 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  	if (ret < 0)
>  		goto fail;
>  
> -	mutex_unlock(&data->lock);
> +	mutex_unlock(&data->vcnl4000_lock);
>  	*val = be16_to_cpu(buf);
>  
>  	return 0;
>  
>  fail:
> -	mutex_unlock(&data->lock);
> +	mutex_unlock(&data->vcnl4000_lock);
>  	return ret;
>  }
>  
> +static int vcnl4200_measure(struct vcnl4000_data *data,
> +		struct vcnl4200_channel *chan, int *val)
> +{
> +	int ret;
> +	s64 delta;
> +	ktime_t next_measurement;
> +
> +	mutex_lock(&chan->lock);
> +
> +	next_measurement = ktime_add(chan->last_measurement,
> +			chan->sampling_rate);
> +	delta = ktime_us_delta(next_measurement, ktime_get());
> +	if (delta > 0)
> +		usleep_range(delta, delta + 500);
> +	chan->last_measurement = ktime_get();
> +
> +	mutex_unlock(&chan->lock);
> +
> +	ret = i2c_smbus_read_word_data(data->client, chan->reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
> +
> +	return 0;
> +}
> +
>  static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
>  {
>  	return vcnl4000_measure(data,
> @@ -157,6 +240,11 @@ static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
>  			VCNL4000_AL_RESULT_HI, val);
>  }
>  
> +static int vcnl4200_measure_light(struct vcnl4000_data *data, int *val)
> +{
> +	return vcnl4200_measure(data, &data->vcnl4200_al, val);
> +}
> +
>  static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
>  {
>  	return vcnl4000_measure(data,
> @@ -164,6 +252,11 @@ static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
>  			VCNL4000_PS_RESULT_HI, val);
>  }
>  
> +static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
> +{
> +	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
> +}
> +
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.prod = "VCNL4000",
> @@ -177,6 +270,12 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  		.measure_light = vcnl4000_measure_light,
>  		.measure_proximity = vcnl4000_measure_proximity,
>  	},
> +	[VCNL4200] = {
> +		.prod = "VCNL4200",
> +		.init = vcnl4200_init,
> +		.measure_light = vcnl4200_measure_light,
> +		.measure_proximity = vcnl4200_measure_proximity,
> +	},
>  };
>  
>  static const struct iio_chan_spec vcnl4000_channels[] = {
> @@ -245,7 +344,6 @@ static int vcnl4000_probe(struct i2c_client *client,
>  	data->client = client;
>  	data->id = id->driver_data;
>  	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
> -	mutex_init(&data->lock);
>  
>  	ret = data->chip_spec->init(data);
>  	if (ret < 0)

Re: [PATCH v2 0/4] iio: vcnl4000: add support for vcnl4200

[Jonathan Cameron]

On Tue, 17 Jul 2018 18:46:51 +0200
Tomas Novotny <tomas@novotny.cz> wrote:

> This is the second iteration of vcnl4200 support. The first one was posted
> on February, sorry for the long delay. I've implemented all notes from
> reviews (thanks to Peter and Jonathan) and nothing more.
> 
> Changes in v2:
> - reading light and proximity values for vcnl4200 is blocking (if you
>   request new value too early)
> - patches 2 and 3 were added; original patch 2 is now patch 4
> - vcnl4010 id is handled (patch 2)
> - warn user on incorrect usage of vcnl40{0,1}0 id (patch 3)
> - minor stuff (add parenthesis, switch instead of if, rename sensors to
>   channels, fix return)
> 
> Please note that I'm not sure if it is still good idea to have same driver
> for vcnl4000 and vcnl4200. The amount of different parts in v2 is even
> bigger. Just see below for differences and add the blocking read which is
> added in v2 for vcnl4200.

Definitely marginal.  My view is it's your choice as the person doing
the work!  Which would you prefer?

I'm pretty happy with these patches if you want to go this way, but
given I assume you might add the support for other features, if you
think that is going to get even worse, then now is the time to make
the decision to not have a unified driver.

Sadly it might be a case of doing a 'hatchet' job on the code to
make a separate driver and see what it looks like.  If it's really
short and clean (which it probably is) then go with separate drivers.

Jonathan

> 
> Cover letter from v1:
> 
> VCNL4200 is another proximity and ambient light sensor from Vishay. I'm
> adding support for that sensor to vcnl4000 driver, which currently supports
> VCNL4000/10/20.
> 
> The VCNL4200 is a bit different from VCNL4000/10/20. Common things are:
> - integrated proximity and ambient light sensor
> - SMBus compatible I2C interface
> - Vishay VCNL4xxx series...
> 
> Different things are:
> - totally different register map
> - 8-bit vs. 16-bit registers. The 16-bit values are in two 8-bit registers
>   on VCNL4000. 16-bit value is in one register on VCNL4200.
> - VCNL4000 has flags when the measurement is finished
> 
> The first patch generalizes the driver to support differencies. The second
> patch adds the support for VCNL4200.
> 
> It is tested on VCNL4020 and VCNL4200.
> 
> Tomas Novotny (4):
>   iio: vcnl4000: make the driver extendable
>   iio: vcnl4000: add VCNL4010 device id
>   iio: vcnl4000: warn on incorrectly specified device id
>   iio: vcnl4000: add support for VCNL4200
> 
>  drivers/iio/light/Kconfig    |   5 +-
>  drivers/iio/light/vcnl4000.c | 219 ++++++++++++++++++++++++++++++++++++++-----
>  2 files changed, 196 insertions(+), 28 deletions(-)
> 

Re: [PATCH v2 2/4] iio: vcnl4000: add VCNL4010 device id

[Tomas Novotny]

Hi Jonathan,

On Sat, 21 Jul 2018 18:05:37 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 17 Jul 2018 18:46:53 +0200
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > The driver already supports VCNL4010/20 devices. The supported features
> > and detectable product id are the same, so add shared id for them.
> > 
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>  
> 
> I'm not totally getting why we need this...  See below.
> 
> > ---
> >  drivers/iio/light/vcnl4000.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 32c0b531395f..0688214fc152 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -48,6 +48,7 @@
> >  
> >  enum vcnl4000_device_ids {
> >  	VCNL4000,
> > +	VCNL4010,
> >  };
> >  
> >  struct vcnl4000_data {
> > @@ -68,6 +69,7 @@ struct vcnl4000_chip_spec {
> >  
> >  static const struct i2c_device_id vcnl4000_id[] = {
> >  	{ "vcnl4000", VCNL4000 },
> > +	{ "vcnl4010", VCNL4010 },  
> 	{ "vcnl4010", VCLN4000 }, then rest of the patch has no purpose
> 
> Also, should list the vcnl4020 id with the same enum entry to
> explicitly support that one.

ok, I will add it.

> It would have made sense if we had split the ID checking to
> verify we had the one we thought we had...  Not sure it really
> matters if they are identical in all visible ways, but at least
> it would have pointed out you didn't have what you thought you had.

As you pointed out in the next patch, I should explain the reason of this
patch here. I will do it in v3.

Thanks,

Tomas

> Jonathan
> 
> 
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> > @@ -157,6 +159,12 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  		.measure_light = vcnl4000_measure_light,
> >  		.measure_proximity = vcnl4000_measure_proximity,
> >  	},
> > +	[VCNL4010] = {
> > +		.prod = "VCNL4010/4020",
> > +		.init = vcnl4000_init,
> > +		.measure_light = vcnl4000_measure_light,
> > +		.measure_proximity = vcnl4000_measure_proximity,
> > +	},
> >  };
> >  
> >  static const struct iio_chan_spec vcnl4000_channels[] = {  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 4/4] iio: vcnl4000: add support for VCNL4200

[Tomas Novotny]

Hi Jonathan,

On Sat, 21 Jul 2018 18:20:21 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 17 Jul 2018 18:46:55 +0200
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > VCNL4200 is an integrated long distance (up to 1500mm) proximity and
> > ambient light sensor.
> > 
> > The support is very basic. There is no configuration of proximity and
> > ambient light sensing yet. Only the reading of both measured values is
> > done.
> > 
> > The reading of ambient light and proximity values is blocking. If you
> > request a new value too early, the driver waits for new value to be
> > ready.
> > 
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>  
> 
> A comment inline, but nothing much needs changing.
> 
> Jonathan
> 
> > ---
> >  drivers/iio/light/Kconfig    |   5 +-
> >  drivers/iio/light/vcnl4000.c | 114 ++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 109 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 074e50657366..c0344a961f54 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -430,11 +430,12 @@ config US5182D
> >  	 will be called us5182d.
> >  
> >  config VCNL4000
> > -	tristate "VCNL4000/4010/4020 combined ALS and proximity sensor"
> > +	tristate "VCNL4000/4010/4020/4200 combined ALS and proximity sensor"
> >  	depends on I2C
> >  	help
> >  	 Say Y here if you want to build a driver for the Vishay VCNL4000,
> > -	 VCNL4010, VCNL4020 combined ambient light and proximity sensor.
> > +	 VCNL4010, VCNL4020, VCNL4200 combined ambient light and proximity
> > +	 sensor.
> >  
> >  	 To compile this driver as a module, choose M here: the
> >  	 module will be called vcnl4000.
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index 642a366c1479..5de0d500c310 100644
> > --- a/drivers/iio/light/vcnl4000.c
> > +++ b/drivers/iio/light/vcnl4000.c
> > @@ -1,5 +1,5 @@
> >  /*
> > - * vcnl4000.c - Support for Vishay VCNL4000/4010/4020 combined ambient
> > + * vcnl4000.c - Support for Vishay VCNL4000/4010/4020/4200 combined ambient
> >   * light and proximity sensor
> >   *
> >   * Copyright 2012 Peter Meerwald <pmeerw@pmeerw.net>
> > @@ -8,13 +8,15 @@
> >   * the GNU General Public License.  See the file COPYING in the main
> >   * directory of this archive for more details.
> >   *
> > - * IIO driver for VCNL4000 (7-bit I2C slave address 0x13)
> > + * IIO driver for:
> > + *   VCNL4000/10/20 (7-bit I2C slave address 0x13)
> > + *   VCNL4200 (7-bit I2C slave address 0x51)
> >   *
> >   * TODO:
> >   *   allow to adjust IR current
> >   *   proximity threshold and event handling
> >   *   periodic ALS/proximity measurement (VCNL4010/20)
> > - *   interrupts (VCNL4010/20)
> > + *   interrupts (VCNL4010/20, VCNL4200)
> >   */
> >  
> >  #include <linux/module.h>
> > @@ -28,6 +30,7 @@
> >  #define VCNL4000_DRV_NAME "vcnl4000"
> >  #define VCNL4000_PROD_ID	0x01
> >  #define VCNL4010_PROD_ID	0x02 /* for VCNL4020, VCNL4010 */
> > +#define VCNL4200_PROD_ID	0x58
> >  
> >  #define VCNL4000_COMMAND	0x80 /* Command register */
> >  #define VCNL4000_PROD_REV	0x81 /* Product ID and Revision ID */
> > @@ -40,6 +43,12 @@
> >  #define VCNL4000_PS_MEAS_FREQ	0x89 /* Proximity test signal frequency */
> >  #define VCNL4000_PS_MOD_ADJ	0x8a /* Proximity modulator timing adjustment */
> >  
> > +#define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
> > +#define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
> > +#define VCNL4200_PS_DATA	0x08 /* Proximity data */
> > +#define VCNL4200_AL_DATA	0x09 /* Ambient light data */
> > +#define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
> > +
> >  /* Bit masks for COMMAND register */
> >  #define VCNL4000_AL_RDY		BIT(6) /* ALS data ready? */
> >  #define VCNL4000_PS_RDY		BIT(5) /* proximity data ready? */
> > @@ -49,6 +58,14 @@
> >  enum vcnl4000_device_ids {
> >  	VCNL4000,
> >  	VCNL4010,
> > +	VCNL4200,
> > +};
> > +
> > +struct vcnl4200_channel {
> > +	u8 reg;
> > +	ktime_t last_measurement;
> > +	ktime_t sampling_rate;
> > +	struct mutex lock;
> >  };
> >  
> >  struct vcnl4000_data {
> > @@ -57,7 +74,9 @@ struct vcnl4000_data {
> >  	int rev;
> >  	int al_scale;
> >  	const struct vcnl4000_chip_spec *chip_spec;
> > -	struct mutex lock;
> > +	struct mutex vcnl4000_lock;
> > +	struct vcnl4200_channel vcnl4200_al;
> > +	struct vcnl4200_channel vcnl4200_ps;
> >  };
> >  
> >  struct vcnl4000_chip_spec {
> > @@ -70,6 +89,7 @@ struct vcnl4000_chip_spec {
> >  static const struct i2c_device_id vcnl4000_id[] = {
> >  	{ "vcnl4000", VCNL4000 },
> >  	{ "vcnl4010", VCNL4010 },
> > +	{ "vcnl4200", VCNL4200 },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> > @@ -100,6 +120,42 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> >  
> >  	data->rev = ret & 0xf;
> >  	data->al_scale = 250000;
> > +	mutex_init(&data->vcnl4000_lock);
> > +
> > +	return 0;
> > +};
> > +
> > +static int vcnl4200_init(struct vcnl4000_data *data)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	if ((ret & 0xff) != VCNL4200_PROD_ID)
> > +		return -ENODEV;
> > +
> > +	data->rev = (ret >> 8) & 0xf;
> > +
> > +	/* Set defaults and enable both channels */
> > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
> > +	if (ret < 0)
> > +		return ret;
> > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	data->al_scale = 24000;
> > +	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
> > +	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
> > +	/* Integration time is 50ms, but the experiments show 54ms in total. */
> > +	data->vcnl4200_al.sampling_rate = ktime_set(0, 54000 * 1000);
> > +	data->vcnl4200_ps.sampling_rate = ktime_set(0, 4200 * 1000);  
> 
> I'm not particularly keen on the mixing of constant and non constant
> stuff in these, but I guess there isn't enough constant stuff to bother
> factoring that out.

If you wouldn't mind, I would leave it as it is. The sampling rate isn't
fixed - this value is just a default value after reset. There are some
settings which influence it, so it might be computed when the driver will be
extended.

Thanks,

Tomas

> > +	data->vcnl4200_al.last_measurement = ktime_set(0, 0);
> > +	data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
> > +	mutex_init(&data->vcnl4200_al.lock);
> > +	mutex_init(&data->vcnl4200_ps.lock);
> >  
> >  	return 0;
> >  };
> > @@ -111,7 +167,7 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> >  	__be16 buf;
> >  	int ret;
> >  
> > -	mutex_lock(&data->lock);
> > +	mutex_lock(&data->vcnl4000_lock);
> >  
> >  	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
> >  					req_mask);
> > @@ -140,16 +196,43 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> >  	if (ret < 0)
> >  		goto fail;
> >  
> > -	mutex_unlock(&data->lock);
> > +	mutex_unlock(&data->vcnl4000_lock);
> >  	*val = be16_to_cpu(buf);
> >  
> >  	return 0;
> >  
> >  fail:
> > -	mutex_unlock(&data->lock);
> > +	mutex_unlock(&data->vcnl4000_lock);
> >  	return ret;
> >  }
> >  
> > +static int vcnl4200_measure(struct vcnl4000_data *data,
> > +		struct vcnl4200_channel *chan, int *val)
> > +{
> > +	int ret;
> > +	s64 delta;
> > +	ktime_t next_measurement;
> > +
> > +	mutex_lock(&chan->lock);
> > +
> > +	next_measurement = ktime_add(chan->last_measurement,
> > +			chan->sampling_rate);
> > +	delta = ktime_us_delta(next_measurement, ktime_get());
> > +	if (delta > 0)
> > +		usleep_range(delta, delta + 500);
> > +	chan->last_measurement = ktime_get();
> > +
> > +	mutex_unlock(&chan->lock);
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, chan->reg);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	*val = ret;
> > +
> > +	return 0;
> > +}
> > +
> >  static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
> >  {
> >  	return vcnl4000_measure(data,
> > @@ -157,6 +240,11 @@ static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
> >  			VCNL4000_AL_RESULT_HI, val);
> >  }
> >  
> > +static int vcnl4200_measure_light(struct vcnl4000_data *data, int *val)
> > +{
> > +	return vcnl4200_measure(data, &data->vcnl4200_al, val);
> > +}
> > +
> >  static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
> >  {
> >  	return vcnl4000_measure(data,
> > @@ -164,6 +252,11 @@ static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
> >  			VCNL4000_PS_RESULT_HI, val);
> >  }
> >  
> > +static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
> > +{
> > +	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
> > +}
> > +
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.prod = "VCNL4000",
> > @@ -177,6 +270,12 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  		.measure_light = vcnl4000_measure_light,
> >  		.measure_proximity = vcnl4000_measure_proximity,
> >  	},
> > +	[VCNL4200] = {
> > +		.prod = "VCNL4200",
> > +		.init = vcnl4200_init,
> > +		.measure_light = vcnl4200_measure_light,
> > +		.measure_proximity = vcnl4200_measure_proximity,
> > +	},
> >  };
> >  
> >  static const struct iio_chan_spec vcnl4000_channels[] = {
> > @@ -245,7 +344,6 @@ static int vcnl4000_probe(struct i2c_client *client,
> >  	data->client = client;
> >  	data->id = id->driver_data;
> >  	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
> > -	mutex_init(&data->lock);
> >  
> >  	ret = data->chip_spec->init(data);
> >  	if (ret < 0)  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 0/4] iio: vcnl4000: add support for vcnl4200

[Tomas Novotny]

Hi Jonathan,

On Sat, 21 Jul 2018 18:25:26 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 17 Jul 2018 18:46:51 +0200
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > This is the second iteration of vcnl4200 support. The first one was posted
> > on February, sorry for the long delay. I've implemented all notes from
> > reviews (thanks to Peter and Jonathan) and nothing more.
> > 
> > Changes in v2:
> > - reading light and proximity values for vcnl4200 is blocking (if you
> >   request new value too early)
> > - patches 2 and 3 were added; original patch 2 is now patch 4
> > - vcnl4010 id is handled (patch 2)
> > - warn user on incorrect usage of vcnl40{0,1}0 id (patch 3)
> > - minor stuff (add parenthesis, switch instead of if, rename sensors to
> >   channels, fix return)
> > 
> > Please note that I'm not sure if it is still good idea to have same driver
> > for vcnl4000 and vcnl4200. The amount of different parts in v2 is even
> > bigger. Just see below for differences and add the blocking read which is
> > added in v2 for vcnl4200.  
> 
> Definitely marginal.  My view is it's your choice as the person doing
> the work!  Which would you prefer?

Well, I was thinking about different driver in the first place. But the reason
was quite selfish (and not the technical one), as I'm interested only in
vcnl4200. But I've seen general attitude to combine similar drivers so I've
tried that way and I'm fine with the result. The only ugly part are some chip
specific things in a shared data structure.

> I'm pretty happy with these patches if you want to go this way, but
> given I assume you might add the support for other features, if you
> think that is going to get even worse, then now is the time to make
> the decision to not have a unified driver.

I will probably add some settings, but this should be handled easily. I don't
see anything problematic in the future (well, maybe interrupts or
thresholds?), as both variants are more or less simple light and proximity
chips.

> Sadly it might be a case of doing a 'hatchet' job on the code to
> make a separate driver and see what it looks like.  If it's really
> short and clean (which it probably is) then go with separate drivers.

The separate vcnl4200 would be indeed short and clean. The structure and
size would be very similar to current vcnl4000.

Anyway, I don't have enough experience to judge what is better. I'm ok to do
any variant.

Thanks for the review,

Tomas

> Jonathan
> 
> > 
> > Cover letter from v1:
> > 
> > VCNL4200 is another proximity and ambient light sensor from Vishay. I'm
> > adding support for that sensor to vcnl4000 driver, which currently supports
> > VCNL4000/10/20.
> > 
> > The VCNL4200 is a bit different from VCNL4000/10/20. Common things are:
> > - integrated proximity and ambient light sensor
> > - SMBus compatible I2C interface
> > - Vishay VCNL4xxx series...
> > 
> > Different things are:
> > - totally different register map
> > - 8-bit vs. 16-bit registers. The 16-bit values are in two 8-bit registers
> >   on VCNL4000. 16-bit value is in one register on VCNL4200.
> > - VCNL4000 has flags when the measurement is finished
> > 
> > The first patch generalizes the driver to support differencies. The second
> > patch adds the support for VCNL4200.
> > 
> > It is tested on VCNL4020 and VCNL4200.
> > 
> > Tomas Novotny (4):
> >   iio: vcnl4000: make the driver extendable
> >   iio: vcnl4000: add VCNL4010 device id
> >   iio: vcnl4000: warn on incorrectly specified device id
> >   iio: vcnl4000: add support for VCNL4200
> > 
> >  drivers/iio/light/Kconfig    |   5 +-
> >  drivers/iio/light/vcnl4000.c | 219 ++++++++++++++++++++++++++++++++++++++-----
> >  2 files changed, 196 insertions(+), 28 deletions(-)
> >   
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 4/4] iio: vcnl4000: add support for VCNL4200

[Jonathan Cameron]

On Mon, 23 Jul 2018 19:32:31 +0200
Tomas Novotny <tomas@novotny.cz> wrote:

> Hi Jonathan,
> 
> On Sat, 21 Jul 2018 18:20:21 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Tue, 17 Jul 2018 18:46:55 +0200
> > Tomas Novotny <tomas@novotny.cz> wrote:
> >   
> > > VCNL4200 is an integrated long distance (up to 1500mm) proximity and
> > > ambient light sensor.
> > > 
> > > The support is very basic. There is no configuration of proximity and
> > > ambient light sensing yet. Only the reading of both measured values is
> > > done.
> > > 
> > > The reading of ambient light and proximity values is blocking. If you
> > > request a new value too early, the driver waits for new value to be
> > > ready.
> > > 
> > > Signed-off-by: Tomas Novotny <tomas@novotny.cz>    
> > 
> > A comment inline, but nothing much needs changing.
> > 
> > Jonathan
> >   
> > > ---
> > >  drivers/iio/light/Kconfig    |   5 +-
> > >  drivers/iio/light/vcnl4000.c | 114 ++++++++++++++++++++++++++++++++++++++++---
> > >  2 files changed, 109 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > > index 074e50657366..c0344a961f54 100644
> > > --- a/drivers/iio/light/Kconfig
> > > +++ b/drivers/iio/light/Kconfig
> > > @@ -430,11 +430,12 @@ config US5182D
> > >  	 will be called us5182d.
> > >  
> > >  config VCNL4000
> > > -	tristate "VCNL4000/4010/4020 combined ALS and proximity sensor"
> > > +	tristate "VCNL4000/4010/4020/4200 combined ALS and proximity sensor"
> > >  	depends on I2C
> > >  	help
> > >  	 Say Y here if you want to build a driver for the Vishay VCNL4000,
> > > -	 VCNL4010, VCNL4020 combined ambient light and proximity sensor.
> > > +	 VCNL4010, VCNL4020, VCNL4200 combined ambient light and proximity
> > > +	 sensor.
> > >  
> > >  	 To compile this driver as a module, choose M here: the
> > >  	 module will be called vcnl4000.
> > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > > index 642a366c1479..5de0d500c310 100644
> > > --- a/drivers/iio/light/vcnl4000.c
> > > +++ b/drivers/iio/light/vcnl4000.c
> > > @@ -1,5 +1,5 @@
> > >  /*
> > > - * vcnl4000.c - Support for Vishay VCNL4000/4010/4020 combined ambient
> > > + * vcnl4000.c - Support for Vishay VCNL4000/4010/4020/4200 combined ambient
> > >   * light and proximity sensor
> > >   *
> > >   * Copyright 2012 Peter Meerwald <pmeerw@pmeerw.net>
> > > @@ -8,13 +8,15 @@
> > >   * the GNU General Public License.  See the file COPYING in the main
> > >   * directory of this archive for more details.
> > >   *
> > > - * IIO driver for VCNL4000 (7-bit I2C slave address 0x13)
> > > + * IIO driver for:
> > > + *   VCNL4000/10/20 (7-bit I2C slave address 0x13)
> > > + *   VCNL4200 (7-bit I2C slave address 0x51)
> > >   *
> > >   * TODO:
> > >   *   allow to adjust IR current
> > >   *   proximity threshold and event handling
> > >   *   periodic ALS/proximity measurement (VCNL4010/20)
> > > - *   interrupts (VCNL4010/20)
> > > + *   interrupts (VCNL4010/20, VCNL4200)
> > >   */
> > >  
> > >  #include <linux/module.h>
> > > @@ -28,6 +30,7 @@
> > >  #define VCNL4000_DRV_NAME "vcnl4000"
> > >  #define VCNL4000_PROD_ID	0x01
> > >  #define VCNL4010_PROD_ID	0x02 /* for VCNL4020, VCNL4010 */
> > > +#define VCNL4200_PROD_ID	0x58
> > >  
> > >  #define VCNL4000_COMMAND	0x80 /* Command register */
> > >  #define VCNL4000_PROD_REV	0x81 /* Product ID and Revision ID */
> > > @@ -40,6 +43,12 @@
> > >  #define VCNL4000_PS_MEAS_FREQ	0x89 /* Proximity test signal frequency */
> > >  #define VCNL4000_PS_MOD_ADJ	0x8a /* Proximity modulator timing adjustment */
> > >  
> > > +#define VCNL4200_AL_CONF	0x00 /* Ambient light configuration */
> > > +#define VCNL4200_PS_CONF1	0x03 /* Proximity configuration */
> > > +#define VCNL4200_PS_DATA	0x08 /* Proximity data */
> > > +#define VCNL4200_AL_DATA	0x09 /* Ambient light data */
> > > +#define VCNL4200_DEV_ID		0x0e /* Device ID, slave address and version */
> > > +
> > >  /* Bit masks for COMMAND register */
> > >  #define VCNL4000_AL_RDY		BIT(6) /* ALS data ready? */
> > >  #define VCNL4000_PS_RDY		BIT(5) /* proximity data ready? */
> > > @@ -49,6 +58,14 @@
> > >  enum vcnl4000_device_ids {
> > >  	VCNL4000,
> > >  	VCNL4010,
> > > +	VCNL4200,
> > > +};
> > > +
> > > +struct vcnl4200_channel {
> > > +	u8 reg;
> > > +	ktime_t last_measurement;
> > > +	ktime_t sampling_rate;
> > > +	struct mutex lock;
> > >  };
> > >  
> > >  struct vcnl4000_data {
> > > @@ -57,7 +74,9 @@ struct vcnl4000_data {
> > >  	int rev;
> > >  	int al_scale;
> > >  	const struct vcnl4000_chip_spec *chip_spec;
> > > -	struct mutex lock;
> > > +	struct mutex vcnl4000_lock;
> > > +	struct vcnl4200_channel vcnl4200_al;
> > > +	struct vcnl4200_channel vcnl4200_ps;
> > >  };
> > >  
> > >  struct vcnl4000_chip_spec {
> > > @@ -70,6 +89,7 @@ struct vcnl4000_chip_spec {
> > >  static const struct i2c_device_id vcnl4000_id[] = {
> > >  	{ "vcnl4000", VCNL4000 },
> > >  	{ "vcnl4010", VCNL4010 },
> > > +	{ "vcnl4200", VCNL4200 },
> > >  	{ }
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> > > @@ -100,6 +120,42 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> > >  
> > >  	data->rev = ret & 0xf;
> > >  	data->al_scale = 250000;
> > > +	mutex_init(&data->vcnl4000_lock);
> > > +
> > > +	return 0;
> > > +};
> > > +
> > > +static int vcnl4200_init(struct vcnl4000_data *data)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = i2c_smbus_read_word_data(data->client, VCNL4200_DEV_ID);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	if ((ret & 0xff) != VCNL4200_PROD_ID)
> > > +		return -ENODEV;
> > > +
> > > +	data->rev = (ret >> 8) & 0xf;
> > > +
> > > +	/* Set defaults and enable both channels */
> > > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	data->al_scale = 24000;
> > > +	data->vcnl4200_al.reg = VCNL4200_AL_DATA;
> > > +	data->vcnl4200_ps.reg = VCNL4200_PS_DATA;
> > > +	/* Integration time is 50ms, but the experiments show 54ms in total. */
> > > +	data->vcnl4200_al.sampling_rate = ktime_set(0, 54000 * 1000);
> > > +	data->vcnl4200_ps.sampling_rate = ktime_set(0, 4200 * 1000);    
> > 
> > I'm not particularly keen on the mixing of constant and non constant
> > stuff in these, but I guess there isn't enough constant stuff to bother
> > factoring that out.  
> 
> If you wouldn't mind, I would leave it as it is. The sampling rate isn't
> fixed - this value is just a default value after reset. There are some
> settings which influence it, so it might be computed when the driver will be
> extended.

Fair enough. Fine as it is.

Jonathan

> 
> Thanks,
> 
> Tomas
> 
> > > +	data->vcnl4200_al.last_measurement = ktime_set(0, 0);
> > > +	data->vcnl4200_ps.last_measurement = ktime_set(0, 0);
> > > +	mutex_init(&data->vcnl4200_al.lock);
> > > +	mutex_init(&data->vcnl4200_ps.lock);
> > >  
> > >  	return 0;
> > >  };
> > > @@ -111,7 +167,7 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> > >  	__be16 buf;
> > >  	int ret;
> > >  
> > > -	mutex_lock(&data->lock);
> > > +	mutex_lock(&data->vcnl4000_lock);
> > >  
> > >  	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
> > >  					req_mask);
> > > @@ -140,16 +196,43 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> > >  	if (ret < 0)
> > >  		goto fail;
> > >  
> > > -	mutex_unlock(&data->lock);
> > > +	mutex_unlock(&data->vcnl4000_lock);
> > >  	*val = be16_to_cpu(buf);
> > >  
> > >  	return 0;
> > >  
> > >  fail:
> > > -	mutex_unlock(&data->lock);
> > > +	mutex_unlock(&data->vcnl4000_lock);
> > >  	return ret;
> > >  }
> > >  
> > > +static int vcnl4200_measure(struct vcnl4000_data *data,
> > > +		struct vcnl4200_channel *chan, int *val)
> > > +{
> > > +	int ret;
> > > +	s64 delta;
> > > +	ktime_t next_measurement;
> > > +
> > > +	mutex_lock(&chan->lock);
> > > +
> > > +	next_measurement = ktime_add(chan->last_measurement,
> > > +			chan->sampling_rate);
> > > +	delta = ktime_us_delta(next_measurement, ktime_get());
> > > +	if (delta > 0)
> > > +		usleep_range(delta, delta + 500);
> > > +	chan->last_measurement = ktime_get();
> > > +
> > > +	mutex_unlock(&chan->lock);
> > > +
> > > +	ret = i2c_smbus_read_word_data(data->client, chan->reg);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	*val = ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
> > >  {
> > >  	return vcnl4000_measure(data,
> > > @@ -157,6 +240,11 @@ static int vcnl4000_measure_light(struct vcnl4000_data *data, int *val)
> > >  			VCNL4000_AL_RESULT_HI, val);
> > >  }
> > >  
> > > +static int vcnl4200_measure_light(struct vcnl4000_data *data, int *val)
> > > +{
> > > +	return vcnl4200_measure(data, &data->vcnl4200_al, val);
> > > +}
> > > +
> > >  static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
> > >  {
> > >  	return vcnl4000_measure(data,
> > > @@ -164,6 +252,11 @@ static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
> > >  			VCNL4000_PS_RESULT_HI, val);
> > >  }
> > >  
> > > +static int vcnl4200_measure_proximity(struct vcnl4000_data *data, int *val)
> > > +{
> > > +	return vcnl4200_measure(data, &data->vcnl4200_ps, val);
> > > +}
> > > +
> > >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  	[VCNL4000] = {
> > >  		.prod = "VCNL4000",
> > > @@ -177,6 +270,12 @@ static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  		.measure_light = vcnl4000_measure_light,
> > >  		.measure_proximity = vcnl4000_measure_proximity,
> > >  	},
> > > +	[VCNL4200] = {
> > > +		.prod = "VCNL4200",
> > > +		.init = vcnl4200_init,
> > > +		.measure_light = vcnl4200_measure_light,
> > > +		.measure_proximity = vcnl4200_measure_proximity,
> > > +	},
> > >  };
> > >  
> > >  static const struct iio_chan_spec vcnl4000_channels[] = {
> > > @@ -245,7 +344,6 @@ static int vcnl4000_probe(struct i2c_client *client,
> > >  	data->client = client;
> > >  	data->id = id->driver_data;
> > >  	data->chip_spec = &vcnl4000_chip_spec_cfg[data->id];
> > > -	mutex_init(&data->lock);
> > >  
> > >  	ret = data->chip_spec->init(data);
> > >  	if (ret < 0)    
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >   

Re: [PATCH v2 0/4] iio: vcnl4000: add support for vcnl4200

[Jonathan Cameron]

On Mon, 23 Jul 2018 19:58:16 +0200
Tomas Novotny <tomas@novotny.cz> wrote:

> Hi Jonathan,
> 
> On Sat, 21 Jul 2018 18:25:26 +0100
> Jonathan Cameron <jic23@kernel.org> wrote:
> 
> > On Tue, 17 Jul 2018 18:46:51 +0200
> > Tomas Novotny <tomas@novotny.cz> wrote:
> >   
> > > This is the second iteration of vcnl4200 support. The first one was posted
> > > on February, sorry for the long delay. I've implemented all notes from
> > > reviews (thanks to Peter and Jonathan) and nothing more.
> > > 
> > > Changes in v2:
> > > - reading light and proximity values for vcnl4200 is blocking (if you
> > >   request new value too early)
> > > - patches 2 and 3 were added; original patch 2 is now patch 4
> > > - vcnl4010 id is handled (patch 2)
> > > - warn user on incorrect usage of vcnl40{0,1}0 id (patch 3)
> > > - minor stuff (add parenthesis, switch instead of if, rename sensors to
> > >   channels, fix return)
> > > 
> > > Please note that I'm not sure if it is still good idea to have same driver
> > > for vcnl4000 and vcnl4200. The amount of different parts in v2 is even
> > > bigger. Just see below for differences and add the blocking read which is
> > > added in v2 for vcnl4200.    
> > 
> > Definitely marginal.  My view is it's your choice as the person doing
> > the work!  Which would you prefer?  
> 
> Well, I was thinking about different driver in the first place. But the reason
> was quite selfish (and not the technical one), as I'm interested only in
> vcnl4200. But I've seen general attitude to combine similar drivers so I've
> tried that way and I'm fine with the result. The only ugly part are some chip
> specific things in a shared data structure.
> 
> > I'm pretty happy with these patches if you want to go this way, but
> > given I assume you might add the support for other features, if you
> > think that is going to get even worse, then now is the time to make
> > the decision to not have a unified driver.  
> 
> I will probably add some settings, but this should be handled easily. I don't
> see anything problematic in the future (well, maybe interrupts or
> thresholds?), as both variants are more or less simple light and proximity
> chips.
> 
> > Sadly it might be a case of doing a 'hatchet' job on the code to
> > make a separate driver and see what it looks like.  If it's really
> > short and clean (which it probably is) then go with separate drivers.  
> 
> The separate vcnl4200 would be indeed short and clean. The structure and
> size would be very similar to current vcnl4000.
> 
> Anyway, I don't have enough experience to judge what is better. I'm ok to do
> any variant.

Let's go with what you have here on the basis it's marginal but you have
done the work for this option already which makes this the better option!

Jonathan

> 
> Thanks for the review,
> 
> Tomas
> 
> > Jonathan
> >   
> > > 
> > > Cover letter from v1:
> > > 
> > > VCNL4200 is another proximity and ambient light sensor from Vishay. I'm
> > > adding support for that sensor to vcnl4000 driver, which currently supports
> > > VCNL4000/10/20.
> > > 
> > > The VCNL4200 is a bit different from VCNL4000/10/20. Common things are:
> > > - integrated proximity and ambient light sensor
> > > - SMBus compatible I2C interface
> > > - Vishay VCNL4xxx series...
> > > 
> > > Different things are:
> > > - totally different register map
> > > - 8-bit vs. 16-bit registers. The 16-bit values are in two 8-bit registers
> > >   on VCNL4000. 16-bit value is in one register on VCNL4200.
> > > - VCNL4000 has flags when the measurement is finished
> > > 
> > > The first patch generalizes the driver to support differencies. The second
> > > patch adds the support for VCNL4200.
> > > 
> > > It is tested on VCNL4020 and VCNL4200.
> > > 
> > > Tomas Novotny (4):
> > >   iio: vcnl4000: make the driver extendable
> > >   iio: vcnl4000: add VCNL4010 device id
> > >   iio: vcnl4000: warn on incorrectly specified device id
> > >   iio: vcnl4000: add support for VCNL4200
> > > 
> > >  drivers/iio/light/Kconfig    |   5 +-
> > >  drivers/iio/light/vcnl4000.c | 219 ++++++++++++++++++++++++++++++++++++++-----
> > >  2 files changed, 196 insertions(+), 28 deletions(-)
> > >     
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> >   

Re: [PATCH v2 0/4] iio: vcnl4000: add support for vcnl4200

[Tomas Novotny]

Hi Jonathan,

On Tue, 24 Jul 2018 22:01:17 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Mon, 23 Jul 2018 19:58:16 +0200
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > Hi Jonathan,
> > 
> > On Sat, 21 Jul 2018 18:25:26 +0100
> > Jonathan Cameron <jic23@kernel.org> wrote:
> >   
> > > On Tue, 17 Jul 2018 18:46:51 +0200
> > > Tomas Novotny <tomas@novotny.cz> wrote:
> > >     
> > > > This is the second iteration of vcnl4200 support. The first one was posted
> > > > on February, sorry for the long delay. I've implemented all notes from
> > > > reviews (thanks to Peter and Jonathan) and nothing more.
> > > > 
> > > > Changes in v2:
> > > > - reading light and proximity values for vcnl4200 is blocking (if you
> > > >   request new value too early)
> > > > - patches 2 and 3 were added; original patch 2 is now patch 4
> > > > - vcnl4010 id is handled (patch 2)
> > > > - warn user on incorrect usage of vcnl40{0,1}0 id (patch 3)
> > > > - minor stuff (add parenthesis, switch instead of if, rename sensors to
> > > >   channels, fix return)
> > > > 
> > > > Please note that I'm not sure if it is still good idea to have same driver
> > > > for vcnl4000 and vcnl4200. The amount of different parts in v2 is even
> > > > bigger. Just see below for differences and add the blocking read which is
> > > > added in v2 for vcnl4200.      
> > > 
> > > Definitely marginal.  My view is it's your choice as the person doing
> > > the work!  Which would you prefer?    
> > 
> > Well, I was thinking about different driver in the first place. But the reason
> > was quite selfish (and not the technical one), as I'm interested only in
> > vcnl4200. But I've seen general attitude to combine similar drivers so I've
> > tried that way and I'm fine with the result. The only ugly part are some chip
> > specific things in a shared data structure.
> >   
> > > I'm pretty happy with these patches if you want to go this way, but
> > > given I assume you might add the support for other features, if you
> > > think that is going to get even worse, then now is the time to make
> > > the decision to not have a unified driver.    
> > 
> > I will probably add some settings, but this should be handled easily. I don't
> > see anything problematic in the future (well, maybe interrupts or
> > thresholds?), as both variants are more or less simple light and proximity
> > chips.
> >   
> > > Sadly it might be a case of doing a 'hatchet' job on the code to
> > > make a separate driver and see what it looks like.  If it's really
> > > short and clean (which it probably is) then go with separate drivers.    
> > 
> > The separate vcnl4200 would be indeed short and clean. The structure and
> > size would be very similar to current vcnl4000.
> > 
> > Anyway, I don't have enough experience to judge what is better. I'm ok to do
> > any variant.  
> 
> Let's go with what you have here on the basis it's marginal but you have
> done the work for this option already which makes this the better option!

ok, thanks for all your comments. I will prepare v3 then.

Tomas

> Jonathan
> 
> > 
> > Thanks for the review,
> > 
> > Tomas
> >   
> > > Jonathan
> > >     
> > > > 
> > > > Cover letter from v1:
> > > > 
> > > > VCNL4200 is another proximity and ambient light sensor from Vishay. I'm
> > > > adding support for that sensor to vcnl4000 driver, which currently supports
> > > > VCNL4000/10/20.
> > > > 
> > > > The VCNL4200 is a bit different from VCNL4000/10/20. Common things are:
> > > > - integrated proximity and ambient light sensor
> > > > - SMBus compatible I2C interface
> > > > - Vishay VCNL4xxx series...
> > > > 
> > > > Different things are:
> > > > - totally different register map
> > > > - 8-bit vs. 16-bit registers. The 16-bit values are in two 8-bit registers
> > > >   on VCNL4000. 16-bit value is in one register on VCNL4200.
> > > > - VCNL4000 has flags when the measurement is finished
> > > > 
> > > > The first patch generalizes the driver to support differencies. The second
> > > > patch adds the support for VCNL4200.
> > > > 
> > > > It is tested on VCNL4020 and VCNL4200.
> > > > 
> > > > Tomas Novotny (4):
> > > >   iio: vcnl4000: make the driver extendable
> > > >   iio: vcnl4000: add VCNL4010 device id
> > > >   iio: vcnl4000: warn on incorrectly specified device id
> > > >   iio: vcnl4000: add support for VCNL4200
> > > > 
> > > >  drivers/iio/light/Kconfig    |   5 +-
> > > >  drivers/iio/light/vcnl4000.c | 219 ++++++++++++++++++++++++++++++++++++++-----
> > > >  2 files changed, 196 insertions(+), 28 deletions(-)
> > > >       
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > >     
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>