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

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

[Tomas Novotny]

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 (2):
  iio: vcnl4000: make the driver extendable
  iio: vcnl4000: add support for VCNL4200

 drivers/iio/light/Kconfig    |   5 +-
 drivers/iio/light/vcnl4000.c | 158 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 140 insertions(+), 23 deletions(-)

-- 
2.12.3

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

[Tomas Novotny]

There are similar chips in a vcnl4xxx family. The initialization and
communication is a bit different for another 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 | 86 ++++++++++++++++++++++++++++++++++----------
 1 file changed, 68 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index c599a90506ad..d7e43fd9333e 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,52 @@
 #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;
+	const char *prod;
+	int rev;
+	int al_scale;
+	const struct vcnl4000_chip_spec *chip_spec;
 	struct mutex lock;
 };
 
+struct vcnl4000_chip_spec {
+	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->prod = prod_id == VCNL4010_PROD_ID ? "VCNL4010/4020" : "VCNL4000";
+	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 +138,28 @@ 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] = {
+		.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 +182,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 +199,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 +215,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 +224,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->prod, data->rev);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &vcnl4000_info;
-- 
2.12.3

Re: [PATCH 1/2] iio: vcnl4000: make the driver extendable

[Peter Meerwald-Stadler]

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

> There is no functional change.

looks good to me, suggestion below but not mandatory
 
> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> ---
>  drivers/iio/light/vcnl4000.c | 86 ++++++++++++++++++++++++++++++++++----------
>  1 file changed, 68 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index c599a90506ad..d7e43fd9333e 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,52 @@
>  #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,

maybe add VCNL4010 here and in vcnl4000_chip_spec_cfg just for 
completeness, so that all IDs are actually used?

> +};
> +
>  struct vcnl4000_data {
>  	struct i2c_client *client;
> +	enum vcnl4000_device_ids id;
> +	const char *prod;
> +	int rev;
> +	int al_scale;
> +	const struct vcnl4000_chip_spec *chip_spec;
>  	struct mutex lock;
>  };
>  
> +struct vcnl4000_chip_spec {
> +	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->prod = prod_id == VCNL4010_PROD_ID ? "VCNL4010/4020" : "VCNL4000";
> +	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 +138,28 @@ 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] = {
> +		.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 +182,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 +199,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 +215,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 +224,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->prod, data->rev);
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &vcnl4000_info;
> 

-- 

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

Re: [PATCH 1/2] iio: vcnl4000: make the driver extendable

[Tomas Novotny]

Hi Peter,

On Mon, 5 Feb 2018 21:00:48 +0100 (CET)
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:

> > There are similar chips in a vcnl4xxx family. The initialization and
> > communication is a bit different for another members of the family, so
> > this patch makes the driver extendable for different chips.  
> 
> > There is no functional change.  
> 
> looks good to me, suggestion below but not mandatory
>  
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > ---
> >  drivers/iio/light/vcnl4000.c | 86 ++++++++++++++++++++++++++++++++++----------
> >  1 file changed, 68 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > index c599a90506ad..d7e43fd9333e 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,52 @@
> >  #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,  
> 
> maybe add VCNL4010 here and in vcnl4000_chip_spec_cfg just for 
> completeness, so that all IDs are actually used?

ok, I will do. Does it make sense to be strict and return -ENODEV (with
appropriate message) when vcnl4000 i2c device is specified but
VCNL4010_PROD_ID is found (and the same for VCNL4000_PROD_ID)? There is only
a general check now, see below.

> > +};
> > +
> >  struct vcnl4000_data {
> >  	struct i2c_client *client;
> > +	enum vcnl4000_device_ids id;
> > +	const char *prod;
> > +	int rev;
> > +	int al_scale;
> > +	const struct vcnl4000_chip_spec *chip_spec;
> >  	struct mutex lock;
> >  };
> >  
> > +struct vcnl4000_chip_spec {
> > +	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;

here ^^^

Thanks,

Tomas

> > +	data->prod = prod_id == VCNL4010_PROD_ID ? "VCNL4010/4020" : "VCNL4000";
> > +	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 +138,28 @@ 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] = {
> > +		.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 +182,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 +199,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 +215,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 +224,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->prod, data->rev);
> >  
> >  	indio_dev->dev.parent = &client->dev;
> >  	indio_dev->info = &vcnl4000_info;
> >   
> 

Re: [PATCH 1/2] iio: vcnl4000: make the driver extendable

[Jonathan Cameron]

On Tue, 6 Feb 2018 14:17:40 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Hi Peter,
> 
> On Mon, 5 Feb 2018 21:00:48 +0100 (CET)
> Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> 
> > > There are similar chips in a vcnl4xxx family. The initialization and
> > > communication is a bit different for another members of the family, so
> > > this patch makes the driver extendable for different chips.    
> >   
> > > There is no functional change.    
> > 
> > looks good to me, suggestion below but not mandatory
> >    
> > > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > > ---
> > >  drivers/iio/light/vcnl4000.c | 86 ++++++++++++++++++++++++++++++++++----------
> > >  1 file changed, 68 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > > index c599a90506ad..d7e43fd9333e 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,52 @@
> > >  #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,    
> > 
> > maybe add VCNL4010 here and in vcnl4000_chip_spec_cfg just for 
> > completeness, so that all IDs are actually used?  
> 
> ok, I will do. Does it make sense to be strict and return -ENODEV (with
> appropriate message) when vcnl4000 i2c device is specified but
> VCNL4010_PROD_ID is found (and the same for VCNL4000_PROD_ID)? There is only
> a general check now, see below.

Probably better to just put a warning out.  People have an irritating habit
of having a dt file for their board based on the wrong compatible.
Given the driver can detect this and 'fix' it I think it should.
Please do put a suitably worded warning in the logs though as better
to fix it properly!

> 
> > > +};
> > > +
> > >  struct vcnl4000_data {
> > >  	struct i2c_client *client;
> > > +	enum vcnl4000_device_ids id;
> > > +	const char *prod;
> > > +	int rev;
> > > +	int al_scale;
> > > +	const struct vcnl4000_chip_spec *chip_spec;
> > >  	struct mutex lock;
> > >  };
> > >  
> > > +struct vcnl4000_chip_spec {
> > > +	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;  
> 
As a general scalability thing, I would do that as a switch statement.
These sorts of if statements have a habit of growing every time a new
variant comes along and in the end get changed to a switch statement
which causes more noise than it would have done if we had allowed for
the future in the first place.

> here ^^^
> 
> Thanks,
> 
> Tomas
> 
> > > +	data->prod = prod_id == VCNL4010_PROD_ID ? "VCNL4010/4020" : "VCNL4000";
> > > +	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 +138,28 @@ 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] = {
> > > +		.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 +182,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 +199,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 +215,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 +224,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->prod, data->rev);
> > >  
> > >  	indio_dev->dev.parent = &client->dev;
> > >  	indio_dev->info = &vcnl4000_info;
> > >     
> >   

Re: [PATCH 1/2] iio: vcnl4000: make the driver extendable

[Tomas Novotny]

Hi Jonathan,

On Sat, 10 Feb 2018 14:59:32 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 6 Feb 2018 14:17:40 +0100
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > Hi Peter,
> > 
> > On Mon, 5 Feb 2018 21:00:48 +0100 (CET)
> > Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> >   
> > > > There are similar chips in a vcnl4xxx family. The initialization and
> > > > communication is a bit different for another members of the family, so
> > > > this patch makes the driver extendable for different chips.      
> > >     
> > > > There is no functional change.      
> > > 
> > > looks good to me, suggestion below but not mandatory
> > >      
> > > > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > > > ---
> > > >  drivers/iio/light/vcnl4000.c | 86 ++++++++++++++++++++++++++++++++++----------
> > > >  1 file changed, 68 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> > > > index c599a90506ad..d7e43fd9333e 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,52 @@
> > > >  #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,      
> > > 
> > > maybe add VCNL4010 here and in vcnl4000_chip_spec_cfg just for 
> > > completeness, so that all IDs are actually used?    
> > 
> > ok, I will do. Does it make sense to be strict and return -ENODEV (with
> > appropriate message) when vcnl4000 i2c device is specified but
> > VCNL4010_PROD_ID is found (and the same for VCNL4000_PROD_ID)? There is only
> > a general check now, see below.  
> 
> Probably better to just put a warning out.  People have an irritating habit
> of having a dt file for their board based on the wrong compatible.
> Given the driver can detect this and 'fix' it I think it should.
> Please do put a suitably worded warning in the logs though as better
> to fix it properly!

ok.

> > > > +};
> > > > +
> > > >  struct vcnl4000_data {
> > > >  	struct i2c_client *client;
> > > > +	enum vcnl4000_device_ids id;
> > > > +	const char *prod;
> > > > +	int rev;
> > > > +	int al_scale;
> > > > +	const struct vcnl4000_chip_spec *chip_spec;
> > > >  	struct mutex lock;
> > > >  };
> > > >  
> > > > +struct vcnl4000_chip_spec {
> > > > +	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;    
> >   
> As a general scalability thing, I would do that as a switch statement.
> These sorts of if statements have a habit of growing every time a new
> variant comes along and in the end get changed to a switch statement
> which causes more noise than it would have done if we had allowed for
> the future in the first place.

I will retain that part in this patch. It is copy&pasted from the original
probe(). I will add another patch to fulfil Peter's comment on adding
VCNL4010 id and that part will be reworked.

Thanks,

Tomas

> > here ^^^
> > 
> > Thanks,
> > 
> > Tomas
> >   
> > > > +	data->prod = prod_id == VCNL4010_PROD_ID ? "VCNL4010/4020" : "VCNL4000";
> > > > +	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 +138,28 @@ 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] = {
> > > > +		.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 +182,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 +199,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 +215,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 +224,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->prod, data->rev);
> > > >  
> > > >  	indio_dev->dev.parent = &client->dev;
> > > >  	indio_dev->info = &vcnl4000_info;
> > > >       
> > >     
> 

[PATCH 2/2] 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.

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

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 2356ed9285df..cab222fa8d18 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -396,11 +396,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 d7e43fd9333e..41f5fad50f63 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/200)
  */
 
 #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? */
@@ -48,6 +57,7 @@
 
 enum vcnl4000_device_ids {
 	VCNL4000,
+	VCNL4200,
 };
 
 struct vcnl4000_data {
@@ -68,6 +78,7 @@ struct vcnl4000_chip_spec {
 
 static const struct i2c_device_id vcnl4000_id[] = {
 	{ "vcnl4000", VCNL4000 },
+	{ "vcnl4200", VCNL4200 },
 	{ }
 };
 MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
@@ -92,6 +103,33 @@ static int vcnl4000_init(struct vcnl4000_data *data)
 	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->prod = "VCNL4200";
+	data->rev = ret >> 8 & 0xf;
+
+	/* Set defaults and enable both sensors */
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
+	if (ret < 0)
+		return -EIO;
+	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
+	if (ret < 0)
+		return -EIO;
+
+	data->al_scale = 24000;
+
+	return 0;
+};
+
 static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 				u8 rdy_mask, u8 data_reg, int *val)
 {
@@ -138,6 +176,19 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 	return ret;
 }
 
+static int vcnl4200_measure(struct vcnl4000_data *data, u8 reg, int *val)
+{
+	int ret;
+
+	ret = i2c_smbus_read_word_data(data->client, 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,
@@ -145,6 +196,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, VCNL4200_AL_DATA, val);
+}
+
 static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
 {
 	return vcnl4000_measure(data,
@@ -152,12 +208,22 @@ 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, VCNL4200_PS_DATA, val);
+}
+
 static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
 	[VCNL4000] = {
 		.init = vcnl4000_init,
 		.measure_light = vcnl4000_measure_light,
 		.measure_proximity = vcnl4000_measure_proximity,
 	},
+	[VCNL4200] = {
+		.init = vcnl4200_init,
+		.measure_light = vcnl4200_measure_light,
+		.measure_proximity = vcnl4200_measure_proximity,
+	},
 };
 
 static const struct iio_chan_spec vcnl4000_channels[] = {
-- 
2.12.3

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

[Peter Meerwald-Stadler]

On Mon, 5 Feb 2018, Tomas Novotny 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.

I think the main issue is the lack of a data ready flag; most drivers 
return new samples (and wait for a new sample if necessary) -- the 
VCNL4200 hardware does not seem to support that directly

one way would be to use a timer and wait the integration time if 
necessary, not sure if this mandatory for IIO -- having both semantics in 
one driver may be confusing

some more suggestions below

regards, p.

> Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> ---
>  drivers/iio/light/Kconfig    |  5 +--
>  drivers/iio/light/vcnl4000.c | 72 ++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 72 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 2356ed9285df..cab222fa8d18 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -396,11 +396,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 d7e43fd9333e..41f5fad50f63 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/200)

not sure if this notations is very clear; maybe better
* 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? */
> @@ -48,6 +57,7 @@
>  
>  enum vcnl4000_device_ids {
>  	VCNL4000,
> +	VCNL4200,
>  };
>  
>  struct vcnl4000_data {
> @@ -68,6 +78,7 @@ struct vcnl4000_chip_spec {
>  
>  static const struct i2c_device_id vcnl4000_id[] = {
>  	{ "vcnl4000", VCNL4000 },
> +	{ "vcnl4200", VCNL4200 },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> @@ -92,6 +103,33 @@ static int vcnl4000_init(struct vcnl4000_data *data)
>  	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->prod = "VCNL4200";
> +	data->rev = ret >> 8 & 0xf;

I'd add parenthesis for readability (and those who are not superfamiliar 
with C operator precedence), i.e. (ret >> 8)

> +
> +	/* Set defaults and enable both sensors */

maybe: both channels, instead of sensors?

> +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
> +	if (ret < 0)
> +		return -EIO;

why not simply return ret? an error value should in general not be 
modified but returned as-is

> +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
> +	if (ret < 0)
> +		return -EIO;
> +
> +	data->al_scale = 24000;
> +
> +	return 0;
> +};
> +
>  static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  				u8 rdy_mask, u8 data_reg, int *val)
>  {
> @@ -138,6 +176,19 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  	return ret;
>  }
>  
> +static int vcnl4200_measure(struct vcnl4000_data *data, u8 reg, int *val)
> +{
> +	int ret;
> +
> +	ret = i2c_smbus_read_word_data(data->client, 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,
> @@ -145,6 +196,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, VCNL4200_AL_DATA, val);
> +}
> +
>  static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
>  {
>  	return vcnl4000_measure(data,
> @@ -152,12 +208,22 @@ 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, VCNL4200_PS_DATA, val);
> +}
> +
>  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
>  	[VCNL4000] = {
>  		.init = vcnl4000_init,
>  		.measure_light = vcnl4000_measure_light,
>  		.measure_proximity = vcnl4000_measure_proximity,
>  	},
> +	[VCNL4200] = {
> +		.init = vcnl4200_init,
> +		.measure_light = vcnl4200_measure_light,
> +		.measure_proximity = vcnl4200_measure_proximity,
> +	},
>  };
>  
>  static const struct iio_chan_spec vcnl4000_channels[] = {
> 

-- 

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

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

[Tomas Novotny]

Hi Peter,

On Mon, 5 Feb 2018 20:55:30 +0100 (CET)
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:

> On Mon, 5 Feb 2018, Tomas Novotny 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.  
> 
> I think the main issue is the lack of a data ready flag; most drivers 
> return new samples (and wait for a new sample if necessary) -- the 
> VCNL4200 hardware does not seem to support that directly

There are only integration times defined in the datasheet.

> one way would be to use a timer and wait the integration time if 
> necessary, not sure if this mandatory for IIO -- having both semantics in 

me neither... Would be enough just to export the integration time via channel
info?

> one driver may be confusing
>
> some more suggestions below
> 
> regards, p.
> 
> > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > ---
> >  drivers/iio/light/Kconfig    |  5 +--
> >  drivers/iio/light/vcnl4000.c | 72 ++++++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 72 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > index 2356ed9285df..cab222fa8d18 100644
> > --- a/drivers/iio/light/Kconfig
> > +++ b/drivers/iio/light/Kconfig
> > @@ -396,11 +396,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 d7e43fd9333e..41f5fad50f63 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/200)  
> 
> not sure if this notations is very clear; maybe better
> * interrupts (VCNL4010/20, VCNL4200)

ok.

> >   */
> >  
> >  #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? */
> > @@ -48,6 +57,7 @@
> >  
> >  enum vcnl4000_device_ids {
> >  	VCNL4000,
> > +	VCNL4200,
> >  };
> >  
> >  struct vcnl4000_data {
> > @@ -68,6 +78,7 @@ struct vcnl4000_chip_spec {
> >  
> >  static const struct i2c_device_id vcnl4000_id[] = {
> >  	{ "vcnl4000", VCNL4000 },
> > +	{ "vcnl4200", VCNL4200 },
> >  	{ }
> >  };
> >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> > @@ -92,6 +103,33 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> >  	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->prod = "VCNL4200";
> > +	data->rev = ret >> 8 & 0xf;  
> 
> I'd add parenthesis for readability (and those who are not superfamiliar 
> with C operator precedence), i.e. (ret >> 8)

I must admit that I was looking at C operator precedence manual to strip the
unnecessary parenthesis. I will add them back.

> > +
> > +	/* Set defaults and enable both sensors */  
> 
> maybe: both channels, instead of sensors?

ok.

> > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
> > +	if (ret < 0)
> > +		return -EIO;  
> 
> why not simply return ret? an error value should in general not be 
> modified but returned as-is

My mistake, I will fix it.

I will post v2.

Thanks for the review,

Tomas

> > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
> > +	if (ret < 0)
> > +		return -EIO;
> > +
> > +	data->al_scale = 24000;
> > +
> > +	return 0;
> > +};
> > +
> >  static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> >  				u8 rdy_mask, u8 data_reg, int *val)
> >  {
> > @@ -138,6 +176,19 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> >  	return ret;
> >  }
> >  
> > +static int vcnl4200_measure(struct vcnl4000_data *data, u8 reg, int *val)
> > +{
> > +	int ret;
> > +
> > +	ret = i2c_smbus_read_word_data(data->client, 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,
> > @@ -145,6 +196,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, VCNL4200_AL_DATA, val);
> > +}
> > +
> >  static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
> >  {
> >  	return vcnl4000_measure(data,
> > @@ -152,12 +208,22 @@ 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, VCNL4200_PS_DATA, val);
> > +}
> > +
> >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> >  	[VCNL4000] = {
> >  		.init = vcnl4000_init,
> >  		.measure_light = vcnl4000_measure_light,
> >  		.measure_proximity = vcnl4000_measure_proximity,
> >  	},
> > +	[VCNL4200] = {
> > +		.init = vcnl4200_init,
> > +		.measure_light = vcnl4200_measure_light,
> > +		.measure_proximity = vcnl4200_measure_proximity,
> > +	},
> >  };
> >  
> >  static const struct iio_chan_spec vcnl4000_channels[] = {
> >   
> 

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

[Jonathan Cameron]

On Tue, 6 Feb 2018 14:46:47 +0100
Tomas Novotny <tomas@novotny.cz> wrote:

> Hi Peter,
> 
> On Mon, 5 Feb 2018 20:55:30 +0100 (CET)
> Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> 
> > On Mon, 5 Feb 2018, Tomas Novotny 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.    
> > 
> > I think the main issue is the lack of a data ready flag; most drivers 
> > return new samples (and wait for a new sample if necessary) -- the 
> > VCNL4200 hardware does not seem to support that directly  
> 
> There are only integration times defined in the datasheet.
> 
> > one way would be to use a timer and wait the integration time if 
> > necessary, not sure if this mandatory for IIO -- having both semantics in   
> 
> me neither... Would be enough just to export the integration time via channel
> info?

This is not an unheard of situation unfortunately. Right now I'm
struggling to remember when we last hit this but has definitely come up before.

So I 'think' we are putting the sensor in a free running mode here
and the issue Peter raises is that we can't tell if we have
'fresh' data or not.

For that you can take the approach taken in various sensors which
are on demand triggered - but with a restriction in the minimum time between
readings - this is common in humidity and gas sensors IIRC.

So what you do is record the time of a particular reading and then
check if the new reading is 'too' close to the previous time.  If it is
you sleep a bit.  This is only ever approximate though as with clock
drifts you will eventually either miss a reading or get get a repeat
(just not the vast majority of the time).
Supporting both forms in driver is fine - semantically from a userspace
point of view it won't be able to tell the difference.

Exporting integration time (and being able to control it) is good
but doesn't prevent userspace from ignoring it and getting invalid
data.

Looks like for the proximity we can run it in an ondemand mode
but not the ALS..

Strange question though - what is the mysterious 'white' channel?
(in the datasheet)
> 
> > one driver may be confusing
> >
> > some more suggestions below
> > 
> > regards, p.
> >   
> > > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > > ---
> > >  drivers/iio/light/Kconfig    |  5 +--
> > >  drivers/iio/light/vcnl4000.c | 72 ++++++++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 72 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > > index 2356ed9285df..cab222fa8d18 100644
> > > --- a/drivers/iio/light/Kconfig
> > > +++ b/drivers/iio/light/Kconfig
> > > @@ -396,11 +396,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 d7e43fd9333e..41f5fad50f63 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/200)    
> > 
> > not sure if this notations is very clear; maybe better
> > * interrupts (VCNL4010/20, VCNL4200)  
> 
> ok.
> 
> > >   */
> > >  
> > >  #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? */
> > > @@ -48,6 +57,7 @@
> > >  
> > >  enum vcnl4000_device_ids {
> > >  	VCNL4000,
> > > +	VCNL4200,
> > >  };
> > >  
> > >  struct vcnl4000_data {
> > > @@ -68,6 +78,7 @@ struct vcnl4000_chip_spec {
> > >  
> > >  static const struct i2c_device_id vcnl4000_id[] = {
> > >  	{ "vcnl4000", VCNL4000 },
> > > +	{ "vcnl4200", VCNL4200 },
> > >  	{ }
> > >  };
> > >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> > > @@ -92,6 +103,33 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> > >  	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->prod = "VCNL4200";
> > > +	data->rev = ret >> 8 & 0xf;    
> > 
> > I'd add parenthesis for readability (and those who are not superfamiliar 
> > with C operator precedence), i.e. (ret >> 8)  
> 
> I must admit that I was looking at C operator precedence manual to strip the
> unnecessary parenthesis. I will add them back.
> 
> > > +
> > > +	/* Set defaults and enable both sensors */    
> > 
> > maybe: both channels, instead of sensors?  
> 
> ok.
> 
> > > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
> > > +	if (ret < 0)
> > > +		return -EIO;    
> > 
> > why not simply return ret? an error value should in general not be 
> > modified but returned as-is  
> 
> My mistake, I will fix it.
> 
> I will post v2.
> 
> Thanks for the review,
> 
> Tomas
> 
> > > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
> > > +	if (ret < 0)
> > > +		return -EIO;
> > > +
> > > +	data->al_scale = 24000;
> > > +
> > > +	return 0;
> > > +};
> > > +
> > >  static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> > >  				u8 rdy_mask, u8 data_reg, int *val)
> > >  {
> > > @@ -138,6 +176,19 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> > >  	return ret;
> > >  }
> > >  
> > > +static int vcnl4200_measure(struct vcnl4000_data *data, u8 reg, int *val)
> > > +{
> > > +	int ret;
> > > +
> > > +	ret = i2c_smbus_read_word_data(data->client, 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,
> > > @@ -145,6 +196,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, VCNL4200_AL_DATA, val);
> > > +}
> > > +
> > >  static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
> > >  {
> > >  	return vcnl4000_measure(data,
> > > @@ -152,12 +208,22 @@ 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, VCNL4200_PS_DATA, val);
> > > +}
> > > +
> > >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > >  	[VCNL4000] = {
> > >  		.init = vcnl4000_init,
> > >  		.measure_light = vcnl4000_measure_light,
> > >  		.measure_proximity = vcnl4000_measure_proximity,
> > >  	},
> > > +	[VCNL4200] = {
> > > +		.init = vcnl4200_init,
> > > +		.measure_light = vcnl4200_measure_light,
> > > +		.measure_proximity = vcnl4200_measure_proximity,
> > > +	},
> > >  };
> > >  
> > >  static const struct iio_chan_spec vcnl4000_channels[] = {
> > >     
> >   

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

[Tomas Novotny]

Hi Jonathan,

On Sat, 10 Feb 2018 15:16:11 +0000
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 6 Feb 2018 14:46:47 +0100
> Tomas Novotny <tomas@novotny.cz> wrote:
> 
> > Hi Peter,
> > 
> > On Mon, 5 Feb 2018 20:55:30 +0100 (CET)
> > Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:
> >   
> > > On Mon, 5 Feb 2018, Tomas Novotny 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.      
> > > 
> > > I think the main issue is the lack of a data ready flag; most drivers 
> > > return new samples (and wait for a new sample if necessary) -- the 
> > > VCNL4200 hardware does not seem to support that directly    
> > 
> > There are only integration times defined in the datasheet.
> >   
> > > one way would be to use a timer and wait the integration time if 
> > > necessary, not sure if this mandatory for IIO -- having both semantics in     
> > 
> > me neither... Would be enough just to export the integration time via channel
> > info?  
> 
> This is not an unheard of situation unfortunately. Right now I'm
> struggling to remember when we last hit this but has definitely come up before.
> 
> So I 'think' we are putting the sensor in a free running mode here
> and the issue Peter raises is that we can't tell if we have
> 'fresh' data or not.
> 
> For that you can take the approach taken in various sensors which
> are on demand triggered - but with a restriction in the minimum time between
> readings - this is common in humidity and gas sensors IIRC.
> 
> So what you do is record the time of a particular reading and then
> check if the new reading is 'too' close to the previous time.  If it is
> you sleep a bit.  This is only ever approximate though as with clock
> drifts you will eventually either miss a reading or get get a repeat
> (just not the vast majority of the time).
> Supporting both forms in driver is fine - semantically from a userspace
> point of view it won't be able to tell the difference.

Ok, I will sleep a bit in case of a too quick consumer.
 
> Exporting integration time (and being able to control it) is good
> but doesn't prevent userspace from ignoring it and getting invalid
> data.
>
> Looks like for the proximity we can run it in an ondemand mode
> but not the ALS..

Yes, right.

> Strange question though - what is the mysterious 'white' channel?
> (in the datasheet)

I cannot find more information. So I did a totally unprofessional measurement
with my cheap multicolour LED lamp. ALS and white channels (unscaled) are
compared on a few colours on three levels of intensity.

The results are:
- ALS and white channel "somehow" correlates
- I wasn't able to saturate white ch.; it is easy to saturate ALS
- ALS is smooth, white is noisy (depending on the colour)

Here are the numbers if you are curious:
Columns: colour, white, ALS

Low intensity:
white   3810    31994

red     1981     6483
orange  2747    17635
yellow  3216    34003
green   1613    48201
cyan    3495    45924
blue    2157    27083
magenta 3401    31453

Medium intesity:
white   9756    65535

red     6192    19647
orange  8097    49621
yellow  9589    65535
green   4966    65535
cyan    9104    65535
blue    5946    65535
magenta 8919    65535

High intensity:
white   14445   65535

red      9285   30276
orange  12623   65535
yellow  15026   65535
green    7166   65535
cyan    15265   65535
blue     9705   65535
magenta 14110   65535

And:
dark       1        3

I will send v2.

Thanks,

Tomas

> > > one driver may be confusing
> > >
> > > some more suggestions below
> > > 
> > > regards, p.
> > >     
> > > > Signed-off-by: Tomas Novotny <tomas@novotny.cz>
> > > > ---
> > > >  drivers/iio/light/Kconfig    |  5 +--
> > > >  drivers/iio/light/vcnl4000.c | 72 ++++++++++++++++++++++++++++++++++++++++++--
> > > >  2 files changed, 72 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> > > > index 2356ed9285df..cab222fa8d18 100644
> > > > --- a/drivers/iio/light/Kconfig
> > > > +++ b/drivers/iio/light/Kconfig
> > > > @@ -396,11 +396,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 d7e43fd9333e..41f5fad50f63 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/200)      
> > > 
> > > not sure if this notations is very clear; maybe better
> > > * interrupts (VCNL4010/20, VCNL4200)    
> > 
> > ok.
> >   
> > > >   */
> > > >  
> > > >  #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? */
> > > > @@ -48,6 +57,7 @@
> > > >  
> > > >  enum vcnl4000_device_ids {
> > > >  	VCNL4000,
> > > > +	VCNL4200,
> > > >  };
> > > >  
> > > >  struct vcnl4000_data {
> > > > @@ -68,6 +78,7 @@ struct vcnl4000_chip_spec {
> > > >  
> > > >  static const struct i2c_device_id vcnl4000_id[] = {
> > > >  	{ "vcnl4000", VCNL4000 },
> > > > +	{ "vcnl4200", VCNL4200 },
> > > >  	{ }
> > > >  };
> > > >  MODULE_DEVICE_TABLE(i2c, vcnl4000_id);
> > > > @@ -92,6 +103,33 @@ static int vcnl4000_init(struct vcnl4000_data *data)
> > > >  	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->prod = "VCNL4200";
> > > > +	data->rev = ret >> 8 & 0xf;      
> > > 
> > > I'd add parenthesis for readability (and those who are not superfamiliar 
> > > with C operator precedence), i.e. (ret >> 8)    
> > 
> > I must admit that I was looking at C operator precedence manual to strip the
> > unnecessary parenthesis. I will add them back.
> >   
> > > > +
> > > > +	/* Set defaults and enable both sensors */      
> > > 
> > > maybe: both channels, instead of sensors?    
> > 
> > ok.
> >   
> > > > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_AL_CONF, 0x00);
> > > > +	if (ret < 0)
> > > > +		return -EIO;      
> > > 
> > > why not simply return ret? an error value should in general not be 
> > > modified but returned as-is    
> > 
> > My mistake, I will fix it.
> > 
> > I will post v2.
> > 
> > Thanks for the review,
> > 
> > Tomas
> >   
> > > > +	ret = i2c_smbus_write_byte_data(data->client, VCNL4200_PS_CONF1, 0x00);
> > > > +	if (ret < 0)
> > > > +		return -EIO;
> > > > +
> > > > +	data->al_scale = 24000;
> > > > +
> > > > +	return 0;
> > > > +};
> > > > +
> > > >  static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> > > >  				u8 rdy_mask, u8 data_reg, int *val)
> > > >  {
> > > > @@ -138,6 +176,19 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
> > > >  	return ret;
> > > >  }
> > > >  
> > > > +static int vcnl4200_measure(struct vcnl4000_data *data, u8 reg, int *val)
> > > > +{
> > > > +	int ret;
> > > > +
> > > > +	ret = i2c_smbus_read_word_data(data->client, 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,
> > > > @@ -145,6 +196,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, VCNL4200_AL_DATA, val);
> > > > +}
> > > > +
> > > >  static int vcnl4000_measure_proximity(struct vcnl4000_data *data, int *val)
> > > >  {
> > > >  	return vcnl4000_measure(data,
> > > > @@ -152,12 +208,22 @@ 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, VCNL4200_PS_DATA, val);
> > > > +}
> > > > +
> > > >  static const struct vcnl4000_chip_spec vcnl4000_chip_spec_cfg[] = {
> > > >  	[VCNL4000] = {
> > > >  		.init = vcnl4000_init,
> > > >  		.measure_light = vcnl4000_measure_light,
> > > >  		.measure_proximity = vcnl4000_measure_proximity,
> > > >  	},
> > > > +	[VCNL4200] = {
> > > > +		.init = vcnl4200_init,
> > > > +		.measure_light = vcnl4200_measure_light,
> > > > +		.measure_proximity = vcnl4200_measure_proximity,
> > > > +	},
> > > >  };
> > > >  
> > > >  static const struct iio_chan_spec vcnl4000_channels[] = {
> > > >       
> > >     
>