Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1734626 > unrolled thread

[PATCH 0/4] iio: light: vl6180: Several fixes and enhancements

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/4] iio: light: vl6180: Several fixes and enhancements Stefan Brüns <stefan.bruens@rwth-aachen.de> - 2017-09-19 05:20 +0200
    [PATCH 3/4] iio: light: vl6180: Cleanup als_gain lookup, avoid register readback Stefan Brüns <stefan.bruens@rwth-aachen.de> - 2017-09-19 05:20 +0200
      Re: [PATCH 3/4] iio: light: vl6180: Cleanup als_gain lookup, avoid  register readback Peter Meerwald-Stadler <pmeerw@pmeerw.net> - 2017-09-19 08:20 +0200
    [PATCH 2/4] iio: light: vl6180: Avoid readback of integration time register Stefan Brüns <stefan.bruens@rwth-aachen.de> - 2017-09-19 05:20 +0200
      Re: [PATCH 2/4] iio: light: vl6180: Avoid readback of integration  time register Jonathan Cameron <jic23@kernel.org> - 2017-09-24 17:40 +0200
    [PATCH 4/4] iio: light: vl6180: Correct ALS scale for non-default gain/integration time Stefan Brüns <stefan.bruens@rwth-aachen.de> - 2017-09-19 05:20 +0200
    Re: [PATCH 0/4] iio: light: vl6180: Several fixes and enhancements Jonathan Cameron <jic23@kernel.org> - 2017-09-24 17:50 +0200

#1734626 — [PATCH 0/4] iio: light: vl6180: Several fixes and enhancements

Currently, the vl6180 driver reports bogus in_illuminance_scale values
when the integration time or hardware gain are changed from its default
value, i.e. it always reports a fixed value.

To avoid readback of the register values in case integration time, gain
or scale is queried, save the register values. Use the saved values
to report the correct scale value.

Stefan Brüns (4):
  iio: light: vl6180: Move range check to integration time setter,
    cleanup
  iio: light: vl6180: Avoid readback of integration time register
  iio: light: vl6180: Cleanup als_gain lookup, avoid register readback
  iio: light: vl6180: Correct ALS scale for non-default gain/integration
    time

 drivers/iio/light/vl6180.c | 125 +++++++++++++++++++++++++--------------------
 1 file changed, 69 insertions(+), 56 deletions(-)

-- 
2.14.1

[toc] | [next] | [standalone]

#1734628 — [PATCH 3/4] iio: light: vl6180: Cleanup als_gain lookup, avoid register readback

Instead of manually iterating the array of allowed gain values, use
find_closest. Storing the current gain setting avoids accessing the
hardware on each query.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/iio/light/vl6180.c | 85 ++++++++++++++++++++++++----------------------
 1 file changed, 44 insertions(+), 41 deletions(-)

diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
index ae768c832d66..fcd8ef511bdc 100644
--- a/drivers/iio/light/vl6180.c
+++ b/drivers/iio/light/vl6180.c
@@ -24,6 +24,7 @@
 #include <linux/err.h>
 #include <linux/of.h>
 #include <linux/delay.h>
+#include <linux/util_macros.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -86,6 +87,7 @@
 struct vl6180_data {
 	struct i2c_client *client;
 	struct mutex lock;
+	unsigned int als_gain_milli;
 	unsigned int als_it_ms;
 };
 
@@ -276,19 +278,17 @@ static const struct iio_chan_spec vl6180_channels[] = {
 };
 
 /*
- * Columns 3 & 4 represent the same value in decimal and hex notations.
- * Kept in order to avoid the datatype conversion while reading the
- * hardware_gain.
+ * Available Ambient Light Sensor gain settings, 1/1000th, and
+ * corresponding setting for the VL6180_ALS_GAIN register
  */
-static const int vl6180_als_gain[8][4] = {
-	{ 1,	0,	70,	VL6180_ALS_GAIN_1 },
-	{ 1,    250000, 69,	VL6180_ALS_GAIN_1_25 },
-	{ 1,    670000, 68,	VL6180_ALS_GAIN_1_67 },
-	{ 2,    500000, 67,	VL6180_ALS_GAIN_2_5 },
-	{ 5,    0,      66,	VL6180_ALS_GAIN_5 },
-	{ 10,   0,      65,	VL6180_ALS_GAIN_10 },
-	{ 20,   0,      64,	VL6180_ALS_GAIN_20 },
-	{ 40,   0,      71,	VL6180_ALS_GAIN_40 }
+static const int vl6180_als_gain_tab[8] = {
+	1000, 1250, 1670, 2500, 5000, 10000, 20000, 40000
+};
+static const u8 vl6180_als_gain_tab_bits[8] = {
+	VL6180_ALS_GAIN_1,    VL6180_ALS_GAIN_1_25,
+	VL6180_ALS_GAIN_1_67, VL6180_ALS_GAIN_2_5,
+	VL6180_ALS_GAIN_5,    VL6180_ALS_GAIN_10,
+	VL6180_ALS_GAIN_20,   VL6180_ALS_GAIN_40
 };
 
 static int vl6180_read_raw(struct iio_dev *indio_dev,
@@ -296,7 +296,7 @@ static int vl6180_read_raw(struct iio_dev *indio_dev,
 				int *val, int *val2, long mask)
 {
 	struct vl6180_data *data = iio_priv(indio_dev);
-	int ret, i;
+	int ret;
 
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
@@ -328,17 +328,11 @@ static int vl6180_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT_PLUS_MICRO;
 	case IIO_CHAN_INFO_HARDWAREGAIN:
-		ret = vl6180_read_byte(data->client, VL6180_ALS_GAIN);
-		if (ret < 0)
-			return -EINVAL;
-		for (i = 0; i < ARRAY_SIZE(vl6180_als_gain); i++) {
-			if (ret == vl6180_als_gain[i][2]) {
-				*val = vl6180_als_gain[i][0];
-				*val2 = vl6180_als_gain[i][1];
-			}
-		}
+		*val = data->als_gain_milli;
+		*val2 = 1000;
+
+		return IIO_VAL_FRACTIONAL;
 
-		return IIO_VAL_INT_PLUS_MICRO;
 	default:
 		return -EINVAL;
 	}
@@ -364,25 +358,33 @@ static int vl6180_hold(struct vl6180_data *data, bool hold)
 
 static int vl6180_set_als_gain(struct vl6180_data *data, int val, int val2)
 {
-	int i, ret;
-
-	for (i = 0; i < ARRAY_SIZE(vl6180_als_gain); i++) {
-		if (val == vl6180_als_gain[i][0] &&
-			val2 == vl6180_als_gain[i][1]) {
-			mutex_lock(&data->lock);
-			ret = vl6180_hold(data, true);
-			if (ret < 0)
-				goto fail;
-			ret = vl6180_write_byte(data->client, VL6180_ALS_GAIN,
-				vl6180_als_gain[i][3]);
-fail:
-			vl6180_hold(data, false);
-			mutex_unlock(&data->lock);
-			return ret;
-		}
-	}
+	int i, ret, gain;
+
+	if (val < 1 || val > 40)
+		return -EINVAL;
+
+	gain = (val * 1000000 + val2)/1000;
+	if (gain < 1 || gain > 40000)
+		return -EINVAL;
+
+	i = find_closest(gain, vl6180_als_gain_tab,
+			 ARRAY_SIZE(vl6180_als_gain_tab));
+
+	mutex_lock(&data->lock);
+	ret = vl6180_hold(data, true);
+	if (ret < 0)
+		goto fail;
 
-	return -EINVAL;
+	ret = vl6180_write_byte(data->client, VL6180_ALS_GAIN,
+				vl6180_als_gain_tab_bits[i]);
+
+	if (ret >= 0)
+		data->als_gain_milli = vl6180_als_gain_tab[i];
+
+fail:
+	vl6180_hold(data, false);
+	mutex_unlock(&data->lock);
+	return ret;
 }
 
 static int vl6180_set_it(struct vl6180_data *data, int val, int val2)
@@ -480,6 +482,7 @@ static int vl6180_init(struct vl6180_data *data)
 		return ret;
 
 	/* ALS gain: 1 */
+	data->als_gain_milli = 1000;
 	ret = vl6180_write_byte(client, VL6180_ALS_GAIN, VL6180_ALS_GAIN_1);
 	if (ret < 0)
 		return ret;
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1734676 — Re: [PATCH 3/4] iio: light: vl6180: Cleanup als_gain lookup, avoid register readback

[Multipart message — attachments visible in raw view] — view raw

> Instead of manually iterating the array of allowed gain values, use
> find_closest. Storing the current gain setting avoids accessing the
> hardware on each query.

nitpick below
 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
> ---
>  drivers/iio/light/vl6180.c | 85 ++++++++++++++++++++++++----------------------
>  1 file changed, 44 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
> index ae768c832d66..fcd8ef511bdc 100644
> --- a/drivers/iio/light/vl6180.c
> +++ b/drivers/iio/light/vl6180.c
> @@ -24,6 +24,7 @@
>  #include <linux/err.h>
>  #include <linux/of.h>
>  #include <linux/delay.h>
> +#include <linux/util_macros.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -86,6 +87,7 @@
>  struct vl6180_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
> +	unsigned int als_gain_milli;
>  	unsigned int als_it_ms;
>  };
>  
> @@ -276,19 +278,17 @@ static const struct iio_chan_spec vl6180_channels[] = {
>  };
>  
>  /*
> - * Columns 3 & 4 represent the same value in decimal and hex notations.
> - * Kept in order to avoid the datatype conversion while reading the
> - * hardware_gain.
> + * Available Ambient Light Sensor gain settings, 1/1000th, and
> + * corresponding setting for the VL6180_ALS_GAIN register
>   */
> -static const int vl6180_als_gain[8][4] = {
> -	{ 1,	0,	70,	VL6180_ALS_GAIN_1 },
> -	{ 1,    250000, 69,	VL6180_ALS_GAIN_1_25 },
> -	{ 1,    670000, 68,	VL6180_ALS_GAIN_1_67 },
> -	{ 2,    500000, 67,	VL6180_ALS_GAIN_2_5 },
> -	{ 5,    0,      66,	VL6180_ALS_GAIN_5 },
> -	{ 10,   0,      65,	VL6180_ALS_GAIN_10 },
> -	{ 20,   0,      64,	VL6180_ALS_GAIN_20 },
> -	{ 40,   0,      71,	VL6180_ALS_GAIN_40 }
> +static const int vl6180_als_gain_tab[8] = {
> +	1000, 1250, 1670, 2500, 5000, 10000, 20000, 40000
> +};
> +static const u8 vl6180_als_gain_tab_bits[8] = {
> +	VL6180_ALS_GAIN_1,    VL6180_ALS_GAIN_1_25,
> +	VL6180_ALS_GAIN_1_67, VL6180_ALS_GAIN_2_5,
> +	VL6180_ALS_GAIN_5,    VL6180_ALS_GAIN_10,
> +	VL6180_ALS_GAIN_20,   VL6180_ALS_GAIN_40
>  };
>  
>  static int vl6180_read_raw(struct iio_dev *indio_dev,
> @@ -296,7 +296,7 @@ static int vl6180_read_raw(struct iio_dev *indio_dev,
>  				int *val, int *val2, long mask)
>  {
>  	struct vl6180_data *data = iio_priv(indio_dev);
> -	int ret, i;
> +	int ret;
>  
>  	switch (mask) {
>  	case IIO_CHAN_INFO_RAW:
> @@ -328,17 +328,11 @@ static int vl6180_read_raw(struct iio_dev *indio_dev,
>  
>  		return IIO_VAL_INT_PLUS_MICRO;
>  	case IIO_CHAN_INFO_HARDWAREGAIN:
> -		ret = vl6180_read_byte(data->client, VL6180_ALS_GAIN);
> -		if (ret < 0)
> -			return -EINVAL;
> -		for (i = 0; i < ARRAY_SIZE(vl6180_als_gain); i++) {
> -			if (ret == vl6180_als_gain[i][2]) {
> -				*val = vl6180_als_gain[i][0];
> -				*val2 = vl6180_als_gain[i][1];
> -			}
> -		}
> +		*val = data->als_gain_milli;
> +		*val2 = 1000;
> +
> +		return IIO_VAL_FRACTIONAL;
>  
> -		return IIO_VAL_INT_PLUS_MICRO;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -364,25 +358,33 @@ static int vl6180_hold(struct vl6180_data *data, bool hold)
>  
>  static int vl6180_set_als_gain(struct vl6180_data *data, int val, int val2)
>  {
> -	int i, ret;
> -
> -	for (i = 0; i < ARRAY_SIZE(vl6180_als_gain); i++) {
> -		if (val == vl6180_als_gain[i][0] &&
> -			val2 == vl6180_als_gain[i][1]) {
> -			mutex_lock(&data->lock);
> -			ret = vl6180_hold(data, true);
> -			if (ret < 0)
> -				goto fail;
> -			ret = vl6180_write_byte(data->client, VL6180_ALS_GAIN,
> -				vl6180_als_gain[i][3]);
> -fail:
> -			vl6180_hold(data, false);
> -			mutex_unlock(&data->lock);
> -			return ret;
> -		}
> -	}
> +	int i, ret, gain;
> +
> +	if (val < 1 || val > 40)
> +		return -EINVAL;
> +
> +	gain = (val * 1000000 + val2)/1000;

spaces around / operator

> +	if (gain < 1 || gain > 40000)
> +		return -EINVAL;
> +
> +	i = find_closest(gain, vl6180_als_gain_tab,
> +			 ARRAY_SIZE(vl6180_als_gain_tab));
> +
> +	mutex_lock(&data->lock);
> +	ret = vl6180_hold(data, true);
> +	if (ret < 0)
> +		goto fail;
>  
> -	return -EINVAL;
> +	ret = vl6180_write_byte(data->client, VL6180_ALS_GAIN,
> +				vl6180_als_gain_tab_bits[i]);
> +
> +	if (ret >= 0)
> +		data->als_gain_milli = vl6180_als_gain_tab[i];
> +
> +fail:
> +	vl6180_hold(data, false);
> +	mutex_unlock(&data->lock);
> +	return ret;
>  }
>  
>  static int vl6180_set_it(struct vl6180_data *data, int val, int val2)
> @@ -480,6 +482,7 @@ static int vl6180_init(struct vl6180_data *data)
>  		return ret;
>  
>  	/* ALS gain: 1 */
> +	data->als_gain_milli = 1000;
>  	ret = vl6180_write_byte(client, VL6180_ALS_GAIN, VL6180_ALS_GAIN_1);
>  	if (ret < 0)
>  		return ret;
> 

-- 

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

[toc] | [prev] | [next] | [standalone]

#1734629 — [PATCH 2/4] iio: light: vl6180: Avoid readback of integration time register

Instead of reading the value from the register on each query, store the
set value.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/iio/light/vl6180.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
index 57577d5d18ac..ae768c832d66 100644
--- a/drivers/iio/light/vl6180.c
+++ b/drivers/iio/light/vl6180.c
@@ -86,6 +86,7 @@
 struct vl6180_data {
 	struct i2c_client *client;
 	struct mutex lock;
+	unsigned int als_it_ms;
 };
 
 enum { VL6180_ALS, VL6180_RANGE, VL6180_PROX };
@@ -306,13 +307,11 @@ static int vl6180_read_raw(struct iio_dev *indio_dev,
 
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_INT_TIME:
-		ret = vl6180_read_word(data->client, VL6180_ALS_IT);
-		if (ret < 0)
-			return ret;
-		*val = 0; /* 1 count = 1ms (0 = 1ms) */
-		*val2 = (ret + 1) * 1000; /* convert to seconds */
+		*val = 0;
+		*val2 = data->als_it_ms * 1000;
 
 		return IIO_VAL_INT_PLUS_MICRO;
+
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_LIGHT:
@@ -401,6 +400,9 @@ static int vl6180_set_it(struct vl6180_data *data, int val, int val2)
 
 	ret = vl6180_write_word(data->client, VL6180_ALS_IT, it_ms - 1);
 
+	if (ret >= 0)
+		data->als_it_ms = it_ms;
+
 fail:
 	vl6180_hold(data, false);
 	mutex_unlock(&data->lock);
@@ -472,6 +474,7 @@ static int vl6180_init(struct vl6180_data *data)
 		return ret;
 
 	/* ALS integration time: 100ms */
+	data->als_it_ms = 100;
 	ret = vl6180_write_word(client, VL6180_ALS_IT, VL6180_ALS_IT_100);
 	if (ret < 0)
 		return ret;
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1738216 — Re: [PATCH 2/4] iio: light: vl6180: Avoid readback of integration time register

On Tue, 19 Sep 2017 05:11:42 +0200
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> Instead of reading the value from the register on each query, store the
> set value.
> 
> Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>

Fair enough, bit nitpick inline.

> ---
>  drivers/iio/light/vl6180.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
> index 57577d5d18ac..ae768c832d66 100644
> --- a/drivers/iio/light/vl6180.c
> +++ b/drivers/iio/light/vl6180.c
> @@ -86,6 +86,7 @@
>  struct vl6180_data {
>  	struct i2c_client *client;
>  	struct mutex lock;
> +	unsigned int als_it_ms;
>  };
>  
>  enum { VL6180_ALS, VL6180_RANGE, VL6180_PROX };
> @@ -306,13 +307,11 @@ static int vl6180_read_raw(struct iio_dev *indio_dev,
>  
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_INT_TIME:
> -		ret = vl6180_read_word(data->client, VL6180_ALS_IT);
> -		if (ret < 0)
> -			return ret;
> -		*val = 0; /* 1 count = 1ms (0 = 1ms) */
> -		*val2 = (ret + 1) * 1000; /* convert to seconds */
> +		*val = 0;
> +		*val2 = data->als_it_ms * 1000;
>  
>  		return IIO_VAL_INT_PLUS_MICRO;
> +

Unrelated white space change.  You need a separate patch
covering these trivial things to avoid adding noise to a patch
making real changes.

>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_LIGHT:
> @@ -401,6 +400,9 @@ static int vl6180_set_it(struct vl6180_data *data, int val, int val2)
>  
>  	ret = vl6180_write_word(data->client, VL6180_ALS_IT, it_ms - 1);
>  
> +	if (ret >= 0)
> +		data->als_it_ms = it_ms;
> +
>  fail:
>  	vl6180_hold(data, false);
>  	mutex_unlock(&data->lock);
> @@ -472,6 +474,7 @@ static int vl6180_init(struct vl6180_data *data)
>  		return ret;
>  
>  	/* ALS integration time: 100ms */
> +	data->als_it_ms = 100;
>  	ret = vl6180_write_word(client, VL6180_ALS_IT, VL6180_ALS_IT_100);
>  	if (ret < 0)
>  		return ret;

[toc] | [prev] | [next] | [standalone]

#1734630 — [PATCH 4/4] iio: light: vl6180: Correct ALS scale for non-default gain/integration time

The reported scale was only correct for the default settings of 100 ms
integration time and gain 1.

This aligns the reported scale with the behaviour of any other IIO driver
and the documented ABI, but may require userspace changes if someone uses
non-default settings.

Signed-off-by: Stefan Brüns <stefan.bruens@rwth-aachen.de>
---
 drivers/iio/light/vl6180.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/light/vl6180.c b/drivers/iio/light/vl6180.c
index fcd8ef511bdc..4f7c36d3df5f 100644
--- a/drivers/iio/light/vl6180.c
+++ b/drivers/iio/light/vl6180.c
@@ -315,9 +315,12 @@ static int vl6180_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_LIGHT:
-			*val = 0; /* one ALS count is 0.32 Lux */
-			*val2 = 320000;
-			break;
+			/* one ALS count is 0.32 Lux @ gain 1, IT 100 ms */
+			*val = 32000; /* 0.32 * 1000 * 100 */
+			*val2 = data->als_gain_milli * data->als_it_ms;
+
+			return IIO_VAL_FRACTIONAL;
+
 		case IIO_DISTANCE:
 			*val = 0; /* sensor reports mm, scale to meter */
 			*val2 = 1000;
-- 
2.14.1

[toc] | [prev] | [next] | [standalone]

#1738219

On Tue, 19 Sep 2017 05:11:40 +0200
Stefan Brüns <stefan.bruens@rwth-aachen.de> wrote:

> Currently, the vl6180 driver reports bogus in_illuminance_scale values
> when the integration time or hardware gain are changed from its default
> value, i.e. it always reports a fixed value.
> 
> To avoid readback of the register values in case integration time, gain
> or scale is queried, save the register values. Use the saved values
> to report the correct scale value.

Hmm. I think unfortunately that this set is too large and invasive
to send as a fix.  Hence once the little corners are tidied up I'll
take it via the slow path ready for the next merge window.

Thanks,

Jonathan

> 
> Stefan Brüns (4):
>   iio: light: vl6180: Move range check to integration time setter,
>     cleanup
>   iio: light: vl6180: Avoid readback of integration time register
>   iio: light: vl6180: Cleanup als_gain lookup, avoid register readback
>   iio: light: vl6180: Correct ALS scale for non-default gain/integration
>     time
> 
>  drivers/iio/light/vl6180.c | 125 +++++++++++++++++++++++++--------------------
>  1 file changed, 69 insertions(+), 56 deletions(-)
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web