[PATCH 0/4] iio: light: vcnl4000: cleanup and fixes

[PATCH 0/4] iio: light: vcnl4000: cleanup and fixes

[Peter Meerwald-Stadler]

This series adds some cleanup to the vcnl4000 driver (use of BIT() macro,
rework of read_raw() return path), adds logging for VCNL4010 and VCNL4020
chips, and adds missing locking of the I2C measurement function

Peter Meerwald-Stadler (4):
  iio: light: vcnl4000: Mention and check support for VCNL4010 and
    VCNL4020
  iio: light: vcnl4000: Use BIT() macro
  iio: light: vcnl4000: Cleanup read_raw() returns
  iio: light: vcnl4000: Add missing locking

 drivers/iio/light/Kconfig    |    6 ++--
 drivers/iio/light/vcnl4000.c |   72 +++++++++++++++++++++++++-----------------
 2 files changed, 46 insertions(+), 32 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] iio: light: vcnl4000: Mention and check support for VCNL4010 and VCNL4020

[Peter Meerwald-Stadler]

VCNL4000, VCNL4010 and VCNL4020 chips are fairly compatible from a software
point of view, added features are not yet supported by the driver

patch adds a check for the product ID and demotes the corresponding
dev_info() to dev_dbg()

Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
---
 drivers/iio/light/Kconfig    |    6 +++---
 drivers/iio/light/vcnl4000.c |   19 ++++++++++++++-----
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 7c566f5..2821747 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -333,11 +333,11 @@ config US5182D
 	 will be called us5182d.
 
 config VCNL4000
-	tristate "VCNL4000 combined ALS and proximity sensor"
+	tristate "VCNL4000/4010/4020 combined ALS and proximity sensor"
 	depends on I2C
 	help
-	 Say Y here if you want to build a driver for the Vishay VCNL4000
-	 combined ambient light and proximity sensor.
+	 Say Y here if you want to build a driver for the Vishay VCNL4000,
+	 VCNL4010, VCNL4020 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 c9d85bb..071ff97 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -1,6 +1,6 @@
 /*
- * vcnl4000.c - Support for Vishay VCNL4000 combined ambient light and
- * proximity sensor
+ * vcnl4000.c - Support for Vishay VCNL4000/4010/4020 combined ambient
+ * light and proximity sensor
  *
  * Copyright 2012 Peter Meerwald <pmeerw@pmeerw.net>
  *
@@ -13,6 +13,8 @@
  * TODO:
  *   allow to adjust IR current
  *   proximity threshold and event handling
+ *   periodic ALS/proximity measurement (VCNL4010/20)
+ *   interrupts (VCNL4010/20)
  */
 
 #include <linux/module.h>
@@ -24,6 +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_COMMAND	0x80 /* Command register */
 #define VCNL4000_PROD_REV	0x81 /* Product ID and Revision ID */
@@ -155,7 +159,7 @@ static int vcnl4000_probe(struct i2c_client *client,
 {
 	struct vcnl4000_data *data;
 	struct iio_dev *indio_dev;
-	int ret;
+	int ret, prod_id;
 
 	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
 	if (!indio_dev)
@@ -169,8 +173,13 @@ static int vcnl4000_probe(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	dev_info(&client->dev, "VCNL4000 Ambient light/proximity sensor, Prod %02x, Rev: %02x\n",
-		ret >> 4, ret & 0xf);
+	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);
 
 	indio_dev->dev.parent = &client->dev;
 	indio_dev->info = &vcnl4000_info;
-- 
1.7.10.4

Re: [PATCH 1/4] iio: light: vcnl4000: Mention and check support for VCNL4010 and VCNL4020

[Jonathan Cameron]

On 05/07/16 11:23, Peter Meerwald-Stadler wrote:
> VCNL4000, VCNL4010 and VCNL4020 chips are fairly compatible from a software
> point of view, added features are not yet supported by the driver
> 
> patch adds a check for the product ID and demotes the corresponding
> dev_info() to dev_dbg()
> 
> Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Applied to the togreg branch of iio.git. As usual will be
pushed out as testing for the autobuilders to play with it.

Probably missed the upcoming merge window though.

Jonathan
> ---
>  drivers/iio/light/Kconfig    |    6 +++---
>  drivers/iio/light/vcnl4000.c |   19 ++++++++++++++-----
>  2 files changed, 17 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 7c566f5..2821747 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -333,11 +333,11 @@ config US5182D
>  	 will be called us5182d.
>  
>  config VCNL4000
> -	tristate "VCNL4000 combined ALS and proximity sensor"
> +	tristate "VCNL4000/4010/4020 combined ALS and proximity sensor"
>  	depends on I2C
>  	help
> -	 Say Y here if you want to build a driver for the Vishay VCNL4000
> -	 combined ambient light and proximity sensor.
> +	 Say Y here if you want to build a driver for the Vishay VCNL4000,
> +	 VCNL4010, VCNL4020 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 c9d85bb..071ff97 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -1,6 +1,6 @@
>  /*
> - * vcnl4000.c - Support for Vishay VCNL4000 combined ambient light and
> - * proximity sensor
> + * vcnl4000.c - Support for Vishay VCNL4000/4010/4020 combined ambient
> + * light and proximity sensor
>   *
>   * Copyright 2012 Peter Meerwald <pmeerw@pmeerw.net>
>   *
> @@ -13,6 +13,8 @@
>   * TODO:
>   *   allow to adjust IR current
>   *   proximity threshold and event handling
> + *   periodic ALS/proximity measurement (VCNL4010/20)
> + *   interrupts (VCNL4010/20)
>   */
>  
>  #include <linux/module.h>
> @@ -24,6 +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_COMMAND	0x80 /* Command register */
>  #define VCNL4000_PROD_REV	0x81 /* Product ID and Revision ID */
> @@ -155,7 +159,7 @@ static int vcnl4000_probe(struct i2c_client *client,
>  {
>  	struct vcnl4000_data *data;
>  	struct iio_dev *indio_dev;
> -	int ret;
> +	int ret, prod_id;
>  
>  	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>  	if (!indio_dev)
> @@ -169,8 +173,13 @@ static int vcnl4000_probe(struct i2c_client *client,
>  	if (ret < 0)
>  		return ret;
>  
> -	dev_info(&client->dev, "VCNL4000 Ambient light/proximity sensor, Prod %02x, Rev: %02x\n",
> -		ret >> 4, ret & 0xf);
> +	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);
>  
>  	indio_dev->dev.parent = &client->dev;
>  	indio_dev->info = &vcnl4000_info;
> 

[PATCH 2/4] iio: light: vcnl4000: Use BIT() macro

[Peter Meerwald-Stadler]

Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
---
 drivers/iio/light/vcnl4000.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 071ff97..7f247ed 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -41,10 +41,10 @@
 #define VCNL4000_PS_MOD_ADJ	0x8a /* Proximity modulator timing adjustment */
 
 /* Bit masks for COMMAND register */
-#define VCNL4000_AL_RDY		0x40 /* ALS data ready? */
-#define VCNL4000_PS_RDY		0x20 /* proximity data ready? */
-#define VCNL4000_AL_OD		0x10 /* start on-demand ALS measurement */
-#define VCNL4000_PS_OD		0x08 /* start on-demand proximity measurement */
+#define VCNL4000_AL_RDY		BIT(6) /* ALS data ready? */
+#define VCNL4000_PS_RDY		BIT(5) /* proximity data ready? */
+#define VCNL4000_AL_OD		BIT(4) /* start on-demand ALS measurement */
+#define VCNL4000_PS_OD		BIT(3) /* start on-demand proximity measurement */
 
 struct vcnl4000_data {
 	struct i2c_client *client;
-- 
1.7.10.4

Re: [PATCH 2/4] iio: light: vcnl4000: Use BIT() macro

[Jonathan Cameron]

On 05/07/16 11:23, Peter Meerwald-Stadler wrote:
> Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Applied.  

J
> ---
>  drivers/iio/light/vcnl4000.c |    8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 071ff97..7f247ed 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -41,10 +41,10 @@
>  #define VCNL4000_PS_MOD_ADJ	0x8a /* Proximity modulator timing adjustment */
>  
>  /* Bit masks for COMMAND register */
> -#define VCNL4000_AL_RDY		0x40 /* ALS data ready? */
> -#define VCNL4000_PS_RDY		0x20 /* proximity data ready? */
> -#define VCNL4000_AL_OD		0x10 /* start on-demand ALS measurement */
> -#define VCNL4000_PS_OD		0x08 /* start on-demand proximity measurement */
> +#define VCNL4000_AL_RDY		BIT(6) /* ALS data ready? */
> +#define VCNL4000_PS_RDY		BIT(5) /* proximity data ready? */
> +#define VCNL4000_AL_OD		BIT(4) /* start on-demand ALS measurement */
> +#define VCNL4000_PS_OD		BIT(3) /* start on-demand proximity measurement */
>  
>  struct vcnl4000_data {
>  	struct i2c_client *client;
> 

[PATCH 3/4] iio: light: vcnl4000: Cleanup read_raw() returns

[Peter Meerwald-Stadler]

Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
---
 drivers/iio/light/vcnl4000.c |   27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 7f247ed..9f94b6b 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -109,7 +109,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 				struct iio_chan_spec const *chan,
 				int *val, int *val2, long mask)
 {
-	int ret = -EINVAL;
+	int ret;
 	struct vcnl4000_data *data = iio_priv(indio_dev);
 
 	switch (mask) {
@@ -121,32 +121,27 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
 				VCNL4000_AL_RESULT_HI, val);
 			if (ret < 0)
 				return ret;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		case IIO_PROXIMITY:
 			ret = vcnl4000_measure(data,
 				VCNL4000_PS_OD, VCNL4000_PS_RDY,
 				VCNL4000_PS_RESULT_HI, val);
 			if (ret < 0)
 				return ret;
-			ret = IIO_VAL_INT;
-			break;
+			return IIO_VAL_INT;
 		default:
-			break;
+			return -EINVAL;
 		}
-		break;
 	case IIO_CHAN_INFO_SCALE:
-		if (chan->type == IIO_LIGHT) {
-			*val = 0;
-			*val2 = 250000;
-			ret = IIO_VAL_INT_PLUS_MICRO;
-		}
-		break;
+		if (chan->type != IIO_LIGHT)
+			return -EINVAL;
+
+		*val = 0;
+		*val2 = 250000;
+		return IIO_VAL_INT_PLUS_MICRO;
 	default:
-		break;
+		return -EINVAL;
 	}
-
-	return ret;
 }
 
 static const struct iio_info vcnl4000_info = {
-- 
1.7.10.4

Re: [PATCH 3/4] iio: light: vcnl4000: Cleanup read_raw() returns

[Jonathan Cameron]

On 05/07/16 11:23, Peter Meerwald-Stadler wrote:
> Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
Nice little cleanup.

Applied. Thanks,

Jonathan
> ---
>  drivers/iio/light/vcnl4000.c |   27 +++++++++++----------------
>  1 file changed, 11 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 7f247ed..9f94b6b 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -109,7 +109,7 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
>  				struct iio_chan_spec const *chan,
>  				int *val, int *val2, long mask)
>  {
> -	int ret = -EINVAL;
> +	int ret;
>  	struct vcnl4000_data *data = iio_priv(indio_dev);
>  
>  	switch (mask) {
> @@ -121,32 +121,27 @@ static int vcnl4000_read_raw(struct iio_dev *indio_dev,
>  				VCNL4000_AL_RESULT_HI, val);
>  			if (ret < 0)
>  				return ret;
> -			ret = IIO_VAL_INT;
> -			break;
> +			return IIO_VAL_INT;
>  		case IIO_PROXIMITY:
>  			ret = vcnl4000_measure(data,
>  				VCNL4000_PS_OD, VCNL4000_PS_RDY,
>  				VCNL4000_PS_RESULT_HI, val);
>  			if (ret < 0)
>  				return ret;
> -			ret = IIO_VAL_INT;
> -			break;
> +			return IIO_VAL_INT;
>  		default:
> -			break;
> +			return -EINVAL;
>  		}
> -		break;
>  	case IIO_CHAN_INFO_SCALE:
> -		if (chan->type == IIO_LIGHT) {
> -			*val = 0;
> -			*val2 = 250000;
> -			ret = IIO_VAL_INT_PLUS_MICRO;
> -		}
> -		break;
> +		if (chan->type != IIO_LIGHT)
> +			return -EINVAL;
> +
> +		*val = 0;
> +		*val2 = 250000;
> +		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
> -		break;
> +		return -EINVAL;
>  	}
> -
> -	return ret;
>  }
>  
>  static const struct iio_info vcnl4000_info = {
> 

[PATCH 4/4] iio: light: vcnl4000: Add missing locking

[Peter Meerwald-Stadler]

Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
---
 drivers/iio/light/vcnl4000.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
index 9f94b6b..360b6e9 100644
--- a/drivers/iio/light/vcnl4000.c
+++ b/drivers/iio/light/vcnl4000.c
@@ -48,6 +48,7 @@
 
 struct vcnl4000_data {
 	struct i2c_client *client;
+	struct mutex lock;
 };
 
 static const struct i2c_device_id vcnl4000_id[] = {
@@ -63,16 +64,18 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 	__be16 buf;
 	int ret;
 
+	mutex_lock(&data->lock);
+
 	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
 					req_mask);
 	if (ret < 0)
-		return ret;
+		goto fail;
 
 	/* wait for data to become ready */
 	while (tries--) {
 		ret = i2c_smbus_read_byte_data(data->client, VCNL4000_COMMAND);
 		if (ret < 0)
-			return ret;
+			goto fail;
 		if (ret & rdy_mask)
 			break;
 		msleep(20); /* measurement takes up to 100 ms */
@@ -81,17 +84,23 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
 	if (tries < 0) {
 		dev_err(&data->client->dev,
 			"vcnl4000_measure() failed, data not ready\n");
-		return -EIO;
+		ret = -EIO;
+		goto fail;
 	}
 
 	ret = i2c_smbus_read_i2c_block_data(data->client,
 		data_reg, sizeof(buf), (u8 *) &buf);
 	if (ret < 0)
-		return ret;
+		goto fail;
 
+	mutex_unlock(&data->lock);
 	*val = be16_to_cpu(buf);
 
 	return 0;
+
+fail:
+	mutex_unlock(&data->lock);
+	return ret;
 }
 
 static const struct iio_chan_spec vcnl4000_channels[] = {
@@ -163,6 +172,7 @@ static int vcnl4000_probe(struct i2c_client *client,
 	data = iio_priv(indio_dev);
 	i2c_set_clientdata(client, indio_dev);
 	data->client = client;
+	mutex_init(&data->lock);
 
 	ret = i2c_smbus_read_byte_data(data->client, VCNL4000_PROD_REV);
 	if (ret < 0)
-- 
1.7.10.4

Re: [PATCH 4/4] iio: light: vcnl4000: Add missing locking

[Jonathan Cameron]

On 05/07/16 11:23, Peter Meerwald-Stadler wrote:
> Signed-off-by: Peter Meerwald-Stadler <pmeerw@pmeerw.net>
As this was never right, it's not a regression so whilst this will fix
some potential race conditions, I'm not going to consider it for stable.

Thanks for the whole series.  Always nice to see continued work on a
driver years after it initially went in!

Jonathan
> ---
>  drivers/iio/light/vcnl4000.c |   18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/vcnl4000.c b/drivers/iio/light/vcnl4000.c
> index 9f94b6b..360b6e9 100644
> --- a/drivers/iio/light/vcnl4000.c
> +++ b/drivers/iio/light/vcnl4000.c
> @@ -48,6 +48,7 @@
>  
>  struct vcnl4000_data {
>  	struct i2c_client *client;
> +	struct mutex lock;
>  };
>  
>  static const struct i2c_device_id vcnl4000_id[] = {
> @@ -63,16 +64,18 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  	__be16 buf;
>  	int ret;
>  
> +	mutex_lock(&data->lock);
> +
>  	ret = i2c_smbus_write_byte_data(data->client, VCNL4000_COMMAND,
>  					req_mask);
>  	if (ret < 0)
> -		return ret;
> +		goto fail;
>  
>  	/* wait for data to become ready */
>  	while (tries--) {
>  		ret = i2c_smbus_read_byte_data(data->client, VCNL4000_COMMAND);
>  		if (ret < 0)
> -			return ret;
> +			goto fail;
>  		if (ret & rdy_mask)
>  			break;
>  		msleep(20); /* measurement takes up to 100 ms */
> @@ -81,17 +84,23 @@ static int vcnl4000_measure(struct vcnl4000_data *data, u8 req_mask,
>  	if (tries < 0) {
>  		dev_err(&data->client->dev,
>  			"vcnl4000_measure() failed, data not ready\n");
> -		return -EIO;
> +		ret = -EIO;
> +		goto fail;
>  	}
>  
>  	ret = i2c_smbus_read_i2c_block_data(data->client,
>  		data_reg, sizeof(buf), (u8 *) &buf);
>  	if (ret < 0)
> -		return ret;
> +		goto fail;
>  
> +	mutex_unlock(&data->lock);
>  	*val = be16_to_cpu(buf);
>  
>  	return 0;
> +
> +fail:
> +	mutex_unlock(&data->lock);
> +	return ret;
>  }
>  
>  static const struct iio_chan_spec vcnl4000_channels[] = {
> @@ -163,6 +172,7 @@ static int vcnl4000_probe(struct i2c_client *client,
>  	data = iio_priv(indio_dev);
>  	i2c_set_clientdata(client, indio_dev);
>  	data->client = client;
> +	mutex_init(&data->lock);
>  
>  	ret = i2c_smbus_read_byte_data(data->client, VCNL4000_PROD_REV);
>  	if (ret < 0)
>