[PATCH 0/4] staging: iio: light: isl29018: Remove non-standard sysfs attributes

[PATCH 0/4] staging: iio: light: isl29018: Remove non-standard sysfs attributes

[Roberta Dobrescu]

This set of patches removes non-standard sysfs attributes used
by isl29018 driver.

It introduces IIO_CHAN_INFO_RANGE and then refactors isl29018 driver
code in order to use standard attributes.

The purpose of these patches is to change the isl29018 driver to use
standard IIO attributes in order to be moved out of staging.

Roberta Dobrescu (4):
  staging: iio: light: isl29018: Remove non-standard sysfs attributes
  iio: Introduce IIO_CHAN_INFO_RANGE
  staging: iio: light: isl29018: Rename lux_scale to calibscale
  staging: iio: light: isl29018: Use standard sysfs attributes for range
    and scale

 Documentation/ABI/testing/sysfs-bus-iio |  13 ++
 drivers/iio/industrialio-core.c         |   1 +
 drivers/staging/iio/light/isl29018.c    | 257 +++++++++++++++-----------------
 include/linux/iio/iio.h                 |   1 +
 4 files changed, 137 insertions(+), 135 deletions(-)

-- 
1.9.1

[PATCH 1/4] staging: iio: light: isl29018: Remove non-standard sysfs attributes

[Roberta Dobrescu]

This patch removes non-standard sysfs attributes range, range_available,
adc_resolution and adc_resolution_available. It also removes the
corresponding show and store functions.

This is in preparation for using standard IIO attributes in order to move
the code out of staging.

Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
---
 drivers/staging/iio/light/isl29018.c | 94 ------------------------------------
 1 file changed, 94 deletions(-)

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index a348918..d3d0611 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -230,87 +230,6 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
 }
 
 /* Sysfs interface */
-/* range */
-static ssize_t show_range(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct isl29018_chip *chip = iio_priv(indio_dev);
-
-	return sprintf(buf, "%u\n", chip->range);
-}
-
-static ssize_t store_range(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct isl29018_chip *chip = iio_priv(indio_dev);
-	int status;
-	unsigned long lval;
-	unsigned int new_range;
-
-	if (kstrtoul(buf, 10, &lval))
-		return -EINVAL;
-
-	if (!(lval == 1000UL || lval == 4000UL ||
-			lval == 16000UL || lval == 64000UL)) {
-		dev_err(dev, "The range is not supported\n");
-		return -EINVAL;
-	}
-
-	mutex_lock(&chip->lock);
-	status = isl29018_set_range(chip, lval, &new_range);
-	if (status < 0) {
-		mutex_unlock(&chip->lock);
-		dev_err(dev,
-			"Error in setting max range with err %d\n", status);
-		return status;
-	}
-	chip->range = new_range;
-	mutex_unlock(&chip->lock);
-
-	return count;
-}
-
-/* resolution */
-static ssize_t show_resolution(struct device *dev,
-			struct device_attribute *attr, char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct isl29018_chip *chip = iio_priv(indio_dev);
-
-	return sprintf(buf, "%u\n", chip->adc_bit);
-}
-
-static ssize_t store_resolution(struct device *dev,
-		struct device_attribute *attr, const char *buf, size_t count)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct isl29018_chip *chip = iio_priv(indio_dev);
-	int status;
-	unsigned int val;
-	unsigned int new_adc_bit;
-
-	if (kstrtouint(buf, 10, &val))
-		return -EINVAL;
-	if (!(val == 4 || val == 8 || val == 12 || val == 16)) {
-		dev_err(dev, "The resolution is not supported\n");
-		return -EINVAL;
-	}
-
-	mutex_lock(&chip->lock);
-	status = isl29018_set_resolution(chip, val, &new_adc_bit);
-	if (status < 0) {
-		mutex_unlock(&chip->lock);
-		dev_err(dev, "Error in setting resolution\n");
-		return status;
-	}
-	chip->adc_bit = new_adc_bit;
-	mutex_unlock(&chip->lock);
-
-	return count;
-}
-
 /* proximity scheme */
 static ssize_t show_prox_infrared_suppression(struct device *dev,
 			struct device_attribute *attr, char *buf)
@@ -447,11 +366,6 @@ static const struct iio_chan_spec isl29023_channels[] = {
 	ISL29018_IR_CHANNEL,
 };
 
-static IIO_DEVICE_ATTR(range, S_IRUGO | S_IWUSR, show_range, store_range, 0);
-static IIO_CONST_ATTR(range_available, "1000 4000 16000 64000");
-static IIO_CONST_ATTR(adc_resolution_available, "4 8 12 16");
-static IIO_DEVICE_ATTR(adc_resolution, S_IRUGO | S_IWUSR,
-					show_resolution, store_resolution, 0);
 static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
 					S_IRUGO | S_IWUSR,
 					show_prox_infrared_suppression,
@@ -460,19 +374,11 @@ static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
 #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
 #define ISL29018_CONST_ATTR(name) (&iio_const_attr_##name.dev_attr.attr)
 static struct attribute *isl29018_attributes[] = {
-	ISL29018_DEV_ATTR(range),
-	ISL29018_CONST_ATTR(range_available),
-	ISL29018_DEV_ATTR(adc_resolution),
-	ISL29018_CONST_ATTR(adc_resolution_available),
 	ISL29018_DEV_ATTR(proximity_on_chip_ambient_infrared_suppression),
 	NULL
 };
 
 static struct attribute *isl29023_attributes[] = {
-	ISL29018_DEV_ATTR(range),
-	ISL29018_CONST_ATTR(range_available),
-	ISL29018_DEV_ATTR(adc_resolution),
-	ISL29018_CONST_ATTR(adc_resolution_available),
 	NULL
 };
 
-- 
1.9.1

Re: [PATCH 1/4] staging: iio: light: isl29018: Remove non-standard sysfs attributes

[Jonathan Cameron]

On 23/02/15 19:35, Roberta Dobrescu wrote:
> This patch removes non-standard sysfs attributes range, range_available,
> adc_resolution and adc_resolution_available. It also removes the
> corresponding show and store functions.
> 
> This is in preparation for using standard IIO attributes in order to move
> the code out of staging.
> 
> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
As a general rule, put in the new interface first then kill off the
old ones in the final patch. Doesn't matter that much in a short
series like this though!

Jonathan
> ---
>  drivers/staging/iio/light/isl29018.c | 94 ------------------------------------
>  1 file changed, 94 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
> index a348918..d3d0611 100644
> --- a/drivers/staging/iio/light/isl29018.c
> +++ b/drivers/staging/iio/light/isl29018.c
> @@ -230,87 +230,6 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
>  }
>  
>  /* Sysfs interface */
> -/* range */
> -static ssize_t show_range(struct device *dev,
> -			struct device_attribute *attr, char *buf)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct isl29018_chip *chip = iio_priv(indio_dev);
> -
> -	return sprintf(buf, "%u\n", chip->range);
> -}
> -
> -static ssize_t store_range(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t count)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct isl29018_chip *chip = iio_priv(indio_dev);
> -	int status;
> -	unsigned long lval;
> -	unsigned int new_range;
> -
> -	if (kstrtoul(buf, 10, &lval))
> -		return -EINVAL;
> -
> -	if (!(lval == 1000UL || lval == 4000UL ||
> -			lval == 16000UL || lval == 64000UL)) {
> -		dev_err(dev, "The range is not supported\n");
> -		return -EINVAL;
> -	}
> -
> -	mutex_lock(&chip->lock);
> -	status = isl29018_set_range(chip, lval, &new_range);
> -	if (status < 0) {
> -		mutex_unlock(&chip->lock);
> -		dev_err(dev,
> -			"Error in setting max range with err %d\n", status);
> -		return status;
> -	}
> -	chip->range = new_range;
> -	mutex_unlock(&chip->lock);
> -
> -	return count;
> -}
> -
> -/* resolution */
> -static ssize_t show_resolution(struct device *dev,
> -			struct device_attribute *attr, char *buf)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct isl29018_chip *chip = iio_priv(indio_dev);
> -
> -	return sprintf(buf, "%u\n", chip->adc_bit);
> -}
> -
> -static ssize_t store_resolution(struct device *dev,
> -		struct device_attribute *attr, const char *buf, size_t count)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct isl29018_chip *chip = iio_priv(indio_dev);
> -	int status;
> -	unsigned int val;
> -	unsigned int new_adc_bit;
> -
> -	if (kstrtouint(buf, 10, &val))
> -		return -EINVAL;
> -	if (!(val == 4 || val == 8 || val == 12 || val == 16)) {
> -		dev_err(dev, "The resolution is not supported\n");
> -		return -EINVAL;
> -	}
> -
> -	mutex_lock(&chip->lock);
> -	status = isl29018_set_resolution(chip, val, &new_adc_bit);
> -	if (status < 0) {
> -		mutex_unlock(&chip->lock);
> -		dev_err(dev, "Error in setting resolution\n");
> -		return status;
> -	}
> -	chip->adc_bit = new_adc_bit;
> -	mutex_unlock(&chip->lock);
> -
> -	return count;
> -}
> -
>  /* proximity scheme */
>  static ssize_t show_prox_infrared_suppression(struct device *dev,
>  			struct device_attribute *attr, char *buf)
> @@ -447,11 +366,6 @@ static const struct iio_chan_spec isl29023_channels[] = {
>  	ISL29018_IR_CHANNEL,
>  };
>  
> -static IIO_DEVICE_ATTR(range, S_IRUGO | S_IWUSR, show_range, store_range, 0);
> -static IIO_CONST_ATTR(range_available, "1000 4000 16000 64000");
> -static IIO_CONST_ATTR(adc_resolution_available, "4 8 12 16");
> -static IIO_DEVICE_ATTR(adc_resolution, S_IRUGO | S_IWUSR,
> -					show_resolution, store_resolution, 0);
>  static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
>  					S_IRUGO | S_IWUSR,
>  					show_prox_infrared_suppression,
> @@ -460,19 +374,11 @@ static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
>  #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
>  #define ISL29018_CONST_ATTR(name) (&iio_const_attr_##name.dev_attr.attr)
>  static struct attribute *isl29018_attributes[] = {
> -	ISL29018_DEV_ATTR(range),
> -	ISL29018_CONST_ATTR(range_available),
> -	ISL29018_DEV_ATTR(adc_resolution),
> -	ISL29018_CONST_ATTR(adc_resolution_available),
>  	ISL29018_DEV_ATTR(proximity_on_chip_ambient_infrared_suppression),
>  	NULL
>  };
>  
>  static struct attribute *isl29023_attributes[] = {
> -	ISL29018_DEV_ATTR(range),
> -	ISL29018_CONST_ATTR(range_available),
> -	ISL29018_DEV_ATTR(adc_resolution),
> -	ISL29018_CONST_ATTR(adc_resolution_available),
>  	NULL
>  };
>  
> 

[PATCH 2/4] iio: Introduce IIO_CHAN_INFO_RANGE

[Roberta Dobrescu]

Some ambient light sensors have hardware dependent ranges and
resolutions. In this case user won't be able to calculate illuminance
(lux) using only scale attribute.

For instance, a device that uses a Full Scale Range is the
light sensor ISL29018. In this case, IIO_CHAN_INFO_RANGE is
needed since the number of ADC bits can be 4, 8, 12 or 16 and
using just scales would result in too similar values for different
ranges and resolutions.

Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
---
 Documentation/ABI/testing/sysfs-bus-iio | 13 +++++++++++++
 drivers/iio/industrialio-core.c         |  1 +
 include/linux/iio/iio.h                 |  1 +
 3 files changed, 15 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
index 9a70c31..ad1541f 100644
--- a/Documentation/ABI/testing/sysfs-bus-iio
+++ b/Documentation/ABI/testing/sysfs-bus-iio
@@ -1249,3 +1249,16 @@ Contact:	linux-iio@vger.kernel.org
 Description:
 		Specifies number of seconds in which we compute the steps
 		that occur in order to decide if the consumer is making steps.
+
+What:		/sys/bus/iio/devices/deviceX/in_illuminance0_range
+KernelVersion:	3.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Hardware dependent ADC Full Scale Range used for some ambient
+		light sensors in calculating lux.
+
+What:		/sys/bus/iio/devices/deviceX/in_illuminance_range_available
+KernelVersion:	3.21
+Contact:	linux-iio@vger.kernel.org
+Description:
+		Hardware dependent supported values for ADC Full Scale Range.
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index aaba9d3..4138042 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -128,6 +128,7 @@ static const char * const iio_chan_info_postfix[] = {
 	[IIO_CHAN_INFO_CALIBWEIGHT] = "calibweight",
 	[IIO_CHAN_INFO_DEBOUNCE_COUNT] = "debounce_count",
 	[IIO_CHAN_INFO_DEBOUNCE_TIME] = "debounce_time",
+	[IIO_CHAN_INFO_RANGE] = "range",
 };
 
 /**
diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
index 80d8550..fbdd434 100644
--- a/include/linux/iio/iio.h
+++ b/include/linux/iio/iio.h
@@ -43,6 +43,7 @@ enum iio_chan_info_enum {
 	IIO_CHAN_INFO_CALIBWEIGHT,
 	IIO_CHAN_INFO_DEBOUNCE_COUNT,
 	IIO_CHAN_INFO_DEBOUNCE_TIME,
+	IIO_CHAN_INFO_RANGE,
 };
 
 enum iio_shared_by {
-- 
1.9.1

Re: [PATCH 2/4] iio: Introduce IIO_CHAN_INFO_RANGE

[Jonathan Cameron]

On 23/02/15 19:35, Roberta Dobrescu wrote:
> Some ambient light sensors have hardware dependent ranges and
> resolutions. In this case user won't be able to calculate illuminance
> (lux) using only scale attribute.
> 
> For instance, a device that uses a Full Scale Range is the
> light sensor ISL29018. In this case, IIO_CHAN_INFO_RANGE is
> needed since the number of ADC bits can be 4, 8, 12 or 16 and
> using just scales would result in too similar values for different
> ranges and resolutions.
This is always an interesting corner.  The big question is what is
to be gained by ever running these sensors in their lower resolutions?

The reason I've always resisted range is that it's too easy
for people to be lazy and go with which ever of scale / range
is presented in the datasheet.  Most of the time they are really
the same thing.

The exception as you've noted here is in variable resolution devices.
There is no way that any generic code is ever going to know the right
option for the combinations of scaling and adc resolution vs time
requirements.  So we are dealing here with parameters that might
be hand tweaked for a particular board.

Anyhow, I'd like more opinions on this before I take it.

Lars, Peter, Hartmut - what you guys think on this?


> 
> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-iio | 13 +++++++++++++
>  drivers/iio/industrialio-core.c         |  1 +
>  include/linux/iio/iio.h                 |  1 +
>  3 files changed, 15 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 9a70c31..ad1541f 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -1249,3 +1249,16 @@ Contact:	linux-iio@vger.kernel.org
>  Description:
>  		Specifies number of seconds in which we compute the steps
>  		that occur in order to decide if the consumer is making steps.
> +
> +What:		/sys/bus/iio/devices/deviceX/in_illuminance0_range
> +KernelVersion:	3.21
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Hardware dependent ADC Full Scale Range used for some ambient
> +		light sensors in calculating lux.
> +
> +What:		/sys/bus/iio/devices/deviceX/in_illuminance_range_available
> +KernelVersion:	3.21
> +Contact:	linux-iio@vger.kernel.org
> +Description:
> +		Hardware dependent supported values for ADC Full Scale Range.
> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
> index aaba9d3..4138042 100644
> --- a/drivers/iio/industrialio-core.c
> +++ b/drivers/iio/industrialio-core.c
> @@ -128,6 +128,7 @@ static const char * const iio_chan_info_postfix[] = {
>  	[IIO_CHAN_INFO_CALIBWEIGHT] = "calibweight",
>  	[IIO_CHAN_INFO_DEBOUNCE_COUNT] = "debounce_count",
>  	[IIO_CHAN_INFO_DEBOUNCE_TIME] = "debounce_time",
> +	[IIO_CHAN_INFO_RANGE] = "range",
>  };
>  
>  /**
> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
> index 80d8550..fbdd434 100644
> --- a/include/linux/iio/iio.h
> +++ b/include/linux/iio/iio.h
> @@ -43,6 +43,7 @@ enum iio_chan_info_enum {
>  	IIO_CHAN_INFO_CALIBWEIGHT,
>  	IIO_CHAN_INFO_DEBOUNCE_COUNT,
>  	IIO_CHAN_INFO_DEBOUNCE_TIME,
> +	IIO_CHAN_INFO_RANGE,
>  };
>  
>  enum iio_shared_by {
> 

Re: [PATCH 2/4] iio: Introduce IIO_CHAN_INFO_RANGE

[Jonathan Cameron]

On 08/03/15 11:42, Jonathan Cameron wrote:
> On 23/02/15 19:35, Roberta Dobrescu wrote:
>> Some ambient light sensors have hardware dependent ranges and
>> resolutions. In this case user won't be able to calculate illuminance
>> (lux) using only scale attribute.
>>
>> For instance, a device that uses a Full Scale Range is the
>> light sensor ISL29018. In this case, IIO_CHAN_INFO_RANGE is
>> needed since the number of ADC bits can be 4, 8, 12 or 16 and
>> using just scales would result in too similar values for different
>> ranges and resolutions.
> This is always an interesting corner.  The big question is what is
> to be gained by ever running these sensors in their lower resolutions?
> 
> The reason I've always resisted range is that it's too easy
> for people to be lazy and go with which ever of scale / range
> is presented in the datasheet.  Most of the time they are really
> the same thing.
> 
> The exception as you've noted here is in variable resolution devices.
> There is no way that any generic code is ever going to know the right
> option for the combinations of scaling and adc resolution vs time
> requirements.  So we are dealing here with parameters that might
> be hand tweaked for a particular board.
> 
> Anyhow, I'd like more opinions on this before I take it.
> 
> Lars, Peter, Hartmut - what you guys think on this?
> 
Actually please see my comments on patch 4.
This device has tight coupling between the integration time
and the adc resolution.  Hence that's the parameter to provide
/ control in my view.

Jonathan
> 
>>
>> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
>> ---
>>  Documentation/ABI/testing/sysfs-bus-iio | 13 +++++++++++++
>>  drivers/iio/industrialio-core.c         |  1 +
>>  include/linux/iio/iio.h                 |  1 +
>>  3 files changed, 15 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
>> index 9a70c31..ad1541f 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-iio
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio
>> @@ -1249,3 +1249,16 @@ Contact:	linux-iio@vger.kernel.org
>>  Description:
>>  		Specifies number of seconds in which we compute the steps
>>  		that occur in order to decide if the consumer is making steps.
>> +
>> +What:		/sys/bus/iio/devices/deviceX/in_illuminance0_range
>> +KernelVersion:	3.21
>> +Contact:	linux-iio@vger.kernel.org
>> +Description:
>> +		Hardware dependent ADC Full Scale Range used for some ambient
>> +		light sensors in calculating lux.
>> +
>> +What:		/sys/bus/iio/devices/deviceX/in_illuminance_range_available
>> +KernelVersion:	3.21
>> +Contact:	linux-iio@vger.kernel.org
>> +Description:
>> +		Hardware dependent supported values for ADC Full Scale Range.
>> diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
>> index aaba9d3..4138042 100644
>> --- a/drivers/iio/industrialio-core.c
>> +++ b/drivers/iio/industrialio-core.c
>> @@ -128,6 +128,7 @@ static const char * const iio_chan_info_postfix[] = {
>>  	[IIO_CHAN_INFO_CALIBWEIGHT] = "calibweight",
>>  	[IIO_CHAN_INFO_DEBOUNCE_COUNT] = "debounce_count",
>>  	[IIO_CHAN_INFO_DEBOUNCE_TIME] = "debounce_time",
>> +	[IIO_CHAN_INFO_RANGE] = "range",
>>  };
>>  
>>  /**
>> diff --git a/include/linux/iio/iio.h b/include/linux/iio/iio.h
>> index 80d8550..fbdd434 100644
>> --- a/include/linux/iio/iio.h
>> +++ b/include/linux/iio/iio.h
>> @@ -43,6 +43,7 @@ enum iio_chan_info_enum {
>>  	IIO_CHAN_INFO_CALIBWEIGHT,
>>  	IIO_CHAN_INFO_DEBOUNCE_COUNT,
>>  	IIO_CHAN_INFO_DEBOUNCE_TIME,
>> +	IIO_CHAN_INFO_RANGE,
>>  };
>>  
>>  enum iio_shared_by {
>>
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/4] iio: Introduce IIO_CHAN_INFO_RANGE

[Daniel Baluta]

On Sun, Mar 8, 2015 at 1:42 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 23/02/15 19:35, Roberta Dobrescu wrote:
>> Some ambient light sensors have hardware dependent ranges and
>> resolutions. In this case user won't be able to calculate illuminance
>> (lux) using only scale attribute.
>>
>> For instance, a device that uses a Full Scale Range is the
>> light sensor ISL29018. In this case, IIO_CHAN_INFO_RANGE is
>> needed since the number of ADC bits can be 4, 8, 12 or 16 and
>> using just scales would result in too similar values for different
>> ranges and resolutions.
> This is always an interesting corner.  The big question is what is
> to be gained by ever running these sensors in their lower resolutions?

I think the big answer here is power consumption.

>
> The reason I've always resisted range is that it's too easy
> for people to be lazy and go with which ever of scale / range
> is presented in the datasheet.  Most of the time they are really
> the same thing.
>
> The exception as you've noted here is in variable resolution devices.
> There is no way that any generic code is ever going to know the right
> option for the combinations of scaling and adc resolution vs time
> requirements.  So we are dealing here with parameters that might
> be hand tweaked for a particular board.
>
> Anyhow, I'd like more opinions on this before I take it.
>
> Lars, Peter, Hartmut - what you guys think on this?

Thanks for your answer Jonathan!

Daniel.

[PATCH 3/4] staging: iio: light: isl29018: Rename lux_scale to calibscale

[Roberta Dobrescu]

This patch renames lux_scale to calibscale and lux_uscale to
ucalibscale.

This is done in order to avoid confusion since these parameters are
used for hardware applied calibration.

Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
---
 drivers/staging/iio/light/isl29018.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index d3d0611..ffc3d1b 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -71,8 +71,8 @@ struct isl29018_chip {
 	struct regmap		*regmap;
 	struct mutex		lock;
 	int			type;
-	unsigned int		lux_scale;
-	unsigned int		lux_uscale;
+	unsigned int		calibscale;
+	unsigned int		ucalibscale;
 	unsigned int		range;
 	unsigned int		adc_bit;
 	int			prox_scheme;
@@ -165,12 +165,12 @@ static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
 
 	/* To support fractional scaling, separate the unshifted lux
 	 * into two calculations: int scaling and micro-scaling.
-	 * lux_uscale ranges from 0-999999, so about 20 bits.  Split
+	 * ucalibscale ranges from 0-999999, so about 20 bits.  Split
 	 * the /1,000,000 in two to reduce the risk of over/underflow.
 	 */
 	data_x_range = lux_data * chip->range;
-	lux_unshifted = data_x_range * chip->lux_scale;
-	lux_unshifted += data_x_range / 1000 * chip->lux_uscale / 1000;
+	lux_unshifted = data_x_range * chip->calibscale;
+	lux_unshifted += data_x_range / 1000 * chip->ucalibscale / 1000;
 	*lux = lux_unshifted >> chip->adc_bit;
 
 	return 0;
@@ -277,9 +277,9 @@ static int isl29018_write_raw(struct iio_dev *indio_dev,
 
 	mutex_lock(&chip->lock);
 	if (mask == IIO_CHAN_INFO_CALIBSCALE && chan->type == IIO_LIGHT) {
-		chip->lux_scale = val;
+		chip->calibscale = val;
 		/* With no write_raw_get_fmt(), val2 is a MICRO fraction. */
-		chip->lux_uscale = val2;
+		chip->ucalibscale = val2;
 		ret = 0;
 	}
 	mutex_unlock(&chip->lock);
@@ -323,8 +323,8 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
 		break;
 	case IIO_CHAN_INFO_CALIBSCALE:
 		if (chan->type == IIO_LIGHT) {
-			*val = chip->lux_scale;
-			*val2 = chip->lux_uscale;
+			*val = chip->calibscale;
+			*val2 = chip->ucalibscale;
 			ret = IIO_VAL_INT_PLUS_MICRO;
 		}
 		break;
@@ -607,8 +607,8 @@ static int isl29018_probe(struct i2c_client *client,
 	mutex_init(&chip->lock);
 
 	chip->type = dev_id;
-	chip->lux_scale = 1;
-	chip->lux_uscale = 0;
+	chip->calibscale = 1;
+	chip->ucalibscale = 0;
 	chip->range = 1000;
 	chip->adc_bit = 16;
 	chip->suspended = false;
-- 
1.9.1

[PATCH 4/4] staging: iio: light: isl29018: Use standard sysfs attributes for range and scale

[Roberta Dobrescu]

This patch refactors the isl29018 driver code in order to use standard
sysfs attributes for range and scale.

ISL29018 light sensor uses four ranges and four ADC's resolutions
which influence the calculated lux.

This patch eliminates the resolution (adc bits) and introduces scale.
Each range (1k, 4k, 16k or 64k) has a corresponding set of 4 scales
(for 16, 12, 8 or 4 bits). Both range and scale can be changed by the
user according to the corresponding set of values (exposed by the attributes
in_illuminance_range_available and in_illuminance_scale_available).

When the range is changed, the set of available scales is set accordingly and
by default the scale used is the one for 16 adc bits. Scale can be changed
anytime with one of the available values.

Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
---
 drivers/staging/iio/light/isl29018.c | 167 ++++++++++++++++++++++++++---------
 1 file changed, 124 insertions(+), 43 deletions(-)

diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
index ffc3d1b..1a91483 100644
--- a/drivers/staging/iio/light/isl29018.c
+++ b/drivers/staging/iio/light/isl29018.c
@@ -66,6 +66,25 @@
 #define ISL29035_BOUT_SHIFT		0x07
 #define ISL29035_BOUT_MASK		(0x01 << ISL29035_BOUT_SHIFT)
 
+enum isl29018_range {
+	ISL29018_RANGE_1,
+	ISL29018_RANGE_2,
+	ISL29018_RANGE_3,
+	ISL29018_RANGE_4,
+};
+
+static const unsigned long isl29018_ranges[] = {1000, 4000, 16000, 64000};
+
+static const struct isl29018_scale {
+	unsigned int scale;
+	unsigned int uscale;
+} isl29018_scales[4][4] = {
+		{ {0, 15258}, {0, 244140}, {3, 906250}, {62, 500000} },
+		{ {0, 61035}, {0, 976562}, {15, 625000}, {250, 0} },
+		{ {0, 244140}, {3, 906250}, {62, 500000}, {1000, 0} },
+		{ {0, 976562}, {15, 625000}, {250, 0}, {4000, 0} }
+		};
+
 struct isl29018_chip {
 	struct device		*dev;
 	struct regmap		*regmap;
@@ -74,50 +93,66 @@ struct isl29018_chip {
 	unsigned int		calibscale;
 	unsigned int		ucalibscale;
 	unsigned int		range;
-	unsigned int		adc_bit;
+	struct isl29018_scale scale;
 	int			prox_scheme;
 	bool			suspended;
 };
 
-static int isl29018_set_range(struct isl29018_chip *chip, unsigned long range,
-		unsigned int *new_range)
+/* when range is set, resolution is set by default at 16 bits */
+static int isl29018_set_range(struct isl29018_chip *chip, unsigned long range)
 {
-	static const unsigned long supp_ranges[] = {1000, 4000, 16000, 64000};
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(supp_ranges); ++i) {
-		if (range <= supp_ranges[i]) {
-			*new_range = (unsigned int)supp_ranges[i];
+	int i, ret;
+	unsigned long new_range;
+	struct isl29018_scale new_scale;
+
+	for (i = 0; i < ARRAY_SIZE(isl29018_ranges); ++i) {
+		if (range == isl29018_ranges[i]) {
+			new_range = i;
+			new_scale = isl29018_scales[i][0];
 			break;
 		}
 	}
 
-	if (i >= ARRAY_SIZE(supp_ranges))
+	if (i >= ARRAY_SIZE(isl29018_ranges))
 		return -EINVAL;
 
-	return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
-			COMMANDII_RANGE_MASK, i << COMMANDII_RANGE_SHIFT);
+	ret = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
+							 COMMANDII_RESOLUTION_MASK | COMMANDII_RANGE_MASK,
+							 i << COMMANDII_RANGE_SHIFT);
+	if (ret < 0)
+		return ret;
+
+	chip->range = new_range;
+	chip->scale = new_scale;
+
+	return 0;
 }
 
-static int isl29018_set_resolution(struct isl29018_chip *chip,
-			unsigned long adcbit, unsigned int *conf_adc_bit)
+static int isl29018_set_scale(struct isl29018_chip *chip, int scale, int uscale)
 {
-	static const unsigned long supp_adcbit[] = {16, 12, 8, 4};
-	int i;
+	int i, ret;
+	struct isl29018_scale new_scale;
 
-	for (i = 0; i < ARRAY_SIZE(supp_adcbit); ++i) {
-		if (adcbit >= supp_adcbit[i]) {
-			*conf_adc_bit = (unsigned int)supp_adcbit[i];
+	for (i = 0; i < ARRAY_SIZE(isl29018_scales[chip->range]); ++i) {
+		if (scale == isl29018_scales[chip->range][i].scale &&
+			uscale == isl29018_scales[chip->range][i].uscale) {
+			new_scale = isl29018_scales[chip->range][i];
 			break;
 		}
 	}
 
-	if (i >= ARRAY_SIZE(supp_adcbit))
+	if (i >= ARRAY_SIZE(isl29018_scales[chip->range]))
 		return -EINVAL;
 
-	return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
-			COMMANDII_RESOLUTION_MASK,
-			i << COMMANDII_RESOLUTION_SHIFT);
+	ret = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
+							 COMMANDII_RESOLUTION_MASK,
+							 i << COMMANDII_RESOLUTION_SHIFT);
+	if (ret < 0)
+		return ret;
+
+	chip->scale = new_scale;
+
+	return 0;
 }
 
 static int isl29018_read_sensor_input(struct isl29018_chip *chip, int mode)
@@ -156,7 +191,7 @@ static int isl29018_read_sensor_input(struct isl29018_chip *chip, int mode)
 static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
 {
 	int lux_data;
-	unsigned int data_x_range, lux_unshifted;
+	unsigned int data_x_range;
 
 	lux_data = isl29018_read_sensor_input(chip, COMMMAND1_OPMODE_ALS_ONCE);
 
@@ -168,10 +203,11 @@ static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
 	 * ucalibscale ranges from 0-999999, so about 20 bits.  Split
 	 * the /1,000,000 in two to reduce the risk of over/underflow.
 	 */
-	data_x_range = lux_data * chip->range;
-	lux_unshifted = data_x_range * chip->calibscale;
-	lux_unshifted += data_x_range / 1000 * chip->ucalibscale / 1000;
-	*lux = lux_unshifted >> chip->adc_bit;
+	data_x_range = lux_data * chip->scale.scale;
+	data_x_range += lux_data / 1000 * chip->scale.uscale / 1000;
+	data_x_range *= chip->calibscale;
+	data_x_range += data_x_range / 1000 * chip->ucalibscale / 1000;
+	*lux = data_x_range;
 
 	return 0;
 }
@@ -229,7 +265,23 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
 	return 0;
 }
 
-/* Sysfs interface */
+static ssize_t show_scale_available(struct device *dev,
+			struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct isl29018_chip *chip = iio_priv(indio_dev);
+	int i, len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(isl29018_scales[chip->range]); ++i)
+		len += sprintf(buf + len, "%d.%06d ",
+					   isl29018_scales[chip->range][i].scale,
+					   isl29018_scales[chip->range][i].uscale);
+
+	buf[len - 1] = '\n';
+
+	return len;
+}
+
 /* proximity scheme */
 static ssize_t show_prox_infrared_suppression(struct device *dev,
 			struct device_attribute *attr, char *buf)
@@ -276,11 +328,24 @@ static int isl29018_write_raw(struct iio_dev *indio_dev,
 	int ret = -EINVAL;
 
 	mutex_lock(&chip->lock);
-	if (mask == IIO_CHAN_INFO_CALIBSCALE && chan->type == IIO_LIGHT) {
-		chip->calibscale = val;
-		/* With no write_raw_get_fmt(), val2 is a MICRO fraction. */
-		chip->ucalibscale = val2;
-		ret = 0;
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBSCALE:
+		if (chan->type == IIO_LIGHT) {
+			chip->calibscale = val;
+			chip->ucalibscale = val2;
+			ret = 0;
+		}
+		break;
+	case IIO_CHAN_INFO_RANGE:
+		if (chan->type == IIO_LIGHT)
+			ret = isl29018_set_range(chip, val);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_LIGHT)
+			ret = isl29018_set_scale(chip, val, val2);
+		break;
+	default:
+		break;
 	}
 	mutex_unlock(&chip->lock);
 
@@ -321,6 +386,19 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
 		if (!ret)
 			ret = IIO_VAL_INT;
 		break;
+	case IIO_CHAN_INFO_RANGE:
+		if (chan->type == IIO_LIGHT) {
+			*val = isl29018_ranges[chip->range];
+			ret = IIO_VAL_INT;
+		}
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		if (chan->type == IIO_LIGHT) {
+			*val = chip->scale.scale;
+			*val2 = chip->scale.uscale;
+			ret = IIO_VAL_INT_PLUS_MICRO;
+		}
+		break;
 	case IIO_CHAN_INFO_CALIBSCALE:
 		if (chan->type == IIO_LIGHT) {
 			*val = chip->calibscale;
@@ -340,7 +418,9 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
 	.indexed = 1,							\
 	.channel = 0,							\
 	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |		\
-	BIT(IIO_CHAN_INFO_CALIBSCALE),					\
+	BIT(IIO_CHAN_INFO_CALIBSCALE) |				\
+	BIT(IIO_CHAN_INFO_SCALE) |				\
+	BIT(IIO_CHAN_INFO_RANGE),					\
 }
 
 #define ISL29018_IR_CHANNEL {						\
@@ -366,6 +446,9 @@ static const struct iio_chan_spec isl29023_channels[] = {
 	ISL29018_IR_CHANNEL,
 };
 
+static IIO_DEVICE_ATTR(in_illuminance_scale_available, S_IRUGO | S_IWUSR,
+		show_scale_available, NULL, 0);
+static IIO_CONST_ATTR(in_illuminance_range_available, "1000 4000 16000 64000");
 static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
 					S_IRUGO | S_IWUSR,
 					show_prox_infrared_suppression,
@@ -374,11 +457,15 @@ static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
 #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
 #define ISL29018_CONST_ATTR(name) (&iio_const_attr_##name.dev_attr.attr)
 static struct attribute *isl29018_attributes[] = {
+	ISL29018_DEV_ATTR(in_illuminance_scale_available),
+	ISL29018_CONST_ATTR(in_illuminance_range_available),
 	ISL29018_DEV_ATTR(proximity_on_chip_ambient_infrared_suppression),
 	NULL
 };
 
 static struct attribute *isl29023_attributes[] = {
+	ISL29018_DEV_ATTR(in_illuminance_scale_available),
+	ISL29018_CONST_ATTR(in_illuminance_range_available),
 	NULL
 };
 
@@ -422,8 +509,6 @@ enum {
 static int isl29018_chip_init(struct isl29018_chip *chip)
 {
 	int status;
-	unsigned int new_adc_bit;
-	unsigned int new_range;
 
 	if (chip->type == isl29035) {
 		status = isl29035_detect(chip);
@@ -472,15 +557,12 @@ static int isl29018_chip_init(struct isl29018_chip *chip)
 	usleep_range(1000, 2000);	/* per data sheet, page 10 */
 
 	/* set defaults */
-	status = isl29018_set_range(chip, chip->range, &new_range);
+	status = isl29018_set_range(chip, isl29018_ranges[chip->range]);
 	if (status < 0) {
 		dev_err(chip->dev, "Init of isl29018 fails\n");
 		return status;
 	}
 
-	status = isl29018_set_resolution(chip, chip->adc_bit,
-						&new_adc_bit);
-
 	return 0;
 }
 
@@ -609,8 +691,7 @@ static int isl29018_probe(struct i2c_client *client,
 	chip->type = dev_id;
 	chip->calibscale = 1;
 	chip->ucalibscale = 0;
-	chip->range = 1000;
-	chip->adc_bit = 16;
+	chip->range = ISL29018_RANGE_1;
 	chip->suspended = false;
 
 	chip->regmap = devm_regmap_init_i2c(client,
-- 
1.9.1

Re: [PATCH 4/4] staging: iio: light: isl29018: Use standard sysfs attributes for range and scale

[Jonathan Cameron]

On 23/02/15 19:35, Roberta Dobrescu wrote:
> This patch refactors the isl29018 driver code in order to use standard
> sysfs attributes for range and scale.
> 
> ISL29018 light sensor uses four ranges and four ADC's resolutions
> which influence the calculated lux.
> 
> This patch eliminates the resolution (adc bits) and introduces scale.
> Each range (1k, 4k, 16k or 64k) has a corresponding set of 4 scales
> (for 16, 12, 8 or 4 bits). Both range and scale can be changed by the
> user according to the corresponding set of values (exposed by the attributes
> in_illuminance_range_available and in_illuminance_scale_available).
> 
> When the range is changed, the set of available scales is set accordingly and
> by default the scale used is the one for 16 adc bits. Scale can be changed
> anytime with one of the available values.
So we have 3 things interacting here.

range - absolutely controlled for the device
scale - from current range, with the adc resolution confusing things - we could just
        multiply the output result up and not need to report this to userspace at all
integration_time - This is the one element that's really being controlled by the adc
                   resolution. This adc resolution is really effecting our sensitivity
                   not the accuracy of measurement of the signal.

Hence I'd have your adc resolution controlled by in_illuminance0_integration_time
and the full scale range controlled by scale.

For the lower adc resolutions simply apply the relevant scaling to get the output
value to be on the same scale (assuming the range has not changed)

How does that sound?

Jonathan
> 
> Signed-off-by: Roberta Dobrescu <roberta.dobrescu@gmail.com>
> ---
>  drivers/staging/iio/light/isl29018.c | 167 ++++++++++++++++++++++++++---------
>  1 file changed, 124 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/staging/iio/light/isl29018.c b/drivers/staging/iio/light/isl29018.c
> index ffc3d1b..1a91483 100644
> --- a/drivers/staging/iio/light/isl29018.c
> +++ b/drivers/staging/iio/light/isl29018.c
> @@ -66,6 +66,25 @@
>  #define ISL29035_BOUT_SHIFT		0x07
>  #define ISL29035_BOUT_MASK		(0x01 << ISL29035_BOUT_SHIFT)
>  
> +enum isl29018_range {
> +	ISL29018_RANGE_1,
> +	ISL29018_RANGE_2,
> +	ISL29018_RANGE_3,
> +	ISL29018_RANGE_4,
> +};
> +
> +static const unsigned long isl29018_ranges[] = {1000, 4000, 16000, 64000};
> +
> +static const struct isl29018_scale {
> +	unsigned int scale;
> +	unsigned int uscale;
> +} isl29018_scales[4][4] = {
> +		{ {0, 15258}, {0, 244140}, {3, 906250}, {62, 500000} },
> +		{ {0, 61035}, {0, 976562}, {15, 625000}, {250, 0} },
> +		{ {0, 244140}, {3, 906250}, {62, 500000}, {1000, 0} },
> +		{ {0, 976562}, {15, 625000}, {250, 0}, {4000, 0} }
> +		};
> +
>  struct isl29018_chip {
>  	struct device		*dev;
>  	struct regmap		*regmap;
> @@ -74,50 +93,66 @@ struct isl29018_chip {
>  	unsigned int		calibscale;
>  	unsigned int		ucalibscale;
>  	unsigned int		range;
> -	unsigned int		adc_bit;
> +	struct isl29018_scale scale;
>  	int			prox_scheme;
>  	bool			suspended;
>  };
>  
> -static int isl29018_set_range(struct isl29018_chip *chip, unsigned long range,
> -		unsigned int *new_range)
> +/* when range is set, resolution is set by default at 16 bits */
> +static int isl29018_set_range(struct isl29018_chip *chip, unsigned long range)
>  {
> -	static const unsigned long supp_ranges[] = {1000, 4000, 16000, 64000};
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(supp_ranges); ++i) {
> -		if (range <= supp_ranges[i]) {
> -			*new_range = (unsigned int)supp_ranges[i];
> +	int i, ret;
> +	unsigned long new_range;
> +	struct isl29018_scale new_scale;
> +
> +	for (i = 0; i < ARRAY_SIZE(isl29018_ranges); ++i) {
> +		if (range == isl29018_ranges[i]) {
> +			new_range = i;
> +			new_scale = isl29018_scales[i][0];
>  			break;
>  		}
>  	}
>  
> -	if (i >= ARRAY_SIZE(supp_ranges))
> +	if (i >= ARRAY_SIZE(isl29018_ranges))
>  		return -EINVAL;
>  
> -	return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
> -			COMMANDII_RANGE_MASK, i << COMMANDII_RANGE_SHIFT);
> +	ret = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
> +							 COMMANDII_RESOLUTION_MASK | COMMANDII_RANGE_MASK,
> +							 i << COMMANDII_RANGE_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	chip->range = new_range;
> +	chip->scale = new_scale;
> +
> +	return 0;
>  }
>  
> -static int isl29018_set_resolution(struct isl29018_chip *chip,
> -			unsigned long adcbit, unsigned int *conf_adc_bit)
> +static int isl29018_set_scale(struct isl29018_chip *chip, int scale, int uscale)
>  {
> -	static const unsigned long supp_adcbit[] = {16, 12, 8, 4};
> -	int i;
> +	int i, ret;
> +	struct isl29018_scale new_scale;
>  
> -	for (i = 0; i < ARRAY_SIZE(supp_adcbit); ++i) {
> -		if (adcbit >= supp_adcbit[i]) {
> -			*conf_adc_bit = (unsigned int)supp_adcbit[i];
> +	for (i = 0; i < ARRAY_SIZE(isl29018_scales[chip->range]); ++i) {
> +		if (scale == isl29018_scales[chip->range][i].scale &&
> +			uscale == isl29018_scales[chip->range][i].uscale) {
> +			new_scale = isl29018_scales[chip->range][i];
>  			break;
>  		}
>  	}
>  
> -	if (i >= ARRAY_SIZE(supp_adcbit))
> +	if (i >= ARRAY_SIZE(isl29018_scales[chip->range]))
>  		return -EINVAL;
>  
> -	return regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
> -			COMMANDII_RESOLUTION_MASK,
> -			i << COMMANDII_RESOLUTION_SHIFT);
> +	ret = regmap_update_bits(chip->regmap, ISL29018_REG_ADD_COMMANDII,
> +							 COMMANDII_RESOLUTION_MASK,
> +							 i << COMMANDII_RESOLUTION_SHIFT);
> +	if (ret < 0)
> +		return ret;
> +
> +	chip->scale = new_scale;
> +
> +	return 0;
>  }
>  
>  static int isl29018_read_sensor_input(struct isl29018_chip *chip, int mode)
> @@ -156,7 +191,7 @@ static int isl29018_read_sensor_input(struct isl29018_chip *chip, int mode)
>  static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
>  {
>  	int lux_data;
> -	unsigned int data_x_range, lux_unshifted;
> +	unsigned int data_x_range;
>  
>  	lux_data = isl29018_read_sensor_input(chip, COMMMAND1_OPMODE_ALS_ONCE);
>  
> @@ -168,10 +203,11 @@ static int isl29018_read_lux(struct isl29018_chip *chip, int *lux)
>  	 * ucalibscale ranges from 0-999999, so about 20 bits.  Split
>  	 * the /1,000,000 in two to reduce the risk of over/underflow.
>  	 */
> -	data_x_range = lux_data * chip->range;
> -	lux_unshifted = data_x_range * chip->calibscale;
> -	lux_unshifted += data_x_range / 1000 * chip->ucalibscale / 1000;
> -	*lux = lux_unshifted >> chip->adc_bit;
> +	data_x_range = lux_data * chip->scale.scale;
> +	data_x_range += lux_data / 1000 * chip->scale.uscale / 1000;
> +	data_x_range *= chip->calibscale;
> +	data_x_range += data_x_range / 1000 * chip->ucalibscale / 1000;
> +	*lux = data_x_range;
>  
>  	return 0;
>  }
> @@ -229,7 +265,23 @@ static int isl29018_read_proximity_ir(struct isl29018_chip *chip, int scheme,
>  	return 0;
>  }
>  
> -/* Sysfs interface */
> +static ssize_t show_scale_available(struct device *dev,
> +			struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +	struct isl29018_chip *chip = iio_priv(indio_dev);
> +	int i, len = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(isl29018_scales[chip->range]); ++i)
> +		len += sprintf(buf + len, "%d.%06d ",
> +					   isl29018_scales[chip->range][i].scale,
> +					   isl29018_scales[chip->range][i].uscale);
> +
> +	buf[len - 1] = '\n';
> +
> +	return len;
> +}
> +
>  /* proximity scheme */
>  static ssize_t show_prox_infrared_suppression(struct device *dev,
>  			struct device_attribute *attr, char *buf)
> @@ -276,11 +328,24 @@ static int isl29018_write_raw(struct iio_dev *indio_dev,
>  	int ret = -EINVAL;
>  
>  	mutex_lock(&chip->lock);
> -	if (mask == IIO_CHAN_INFO_CALIBSCALE && chan->type == IIO_LIGHT) {
> -		chip->calibscale = val;
> -		/* With no write_raw_get_fmt(), val2 is a MICRO fraction. */
> -		chip->ucalibscale = val2;
> -		ret = 0;
> +	switch (mask) {
> +	case IIO_CHAN_INFO_CALIBSCALE:
> +		if (chan->type == IIO_LIGHT) {
> +			chip->calibscale = val;
> +			chip->ucalibscale = val2;
> +			ret = 0;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_RANGE:
> +		if (chan->type == IIO_LIGHT)
> +			ret = isl29018_set_range(chip, val);
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_LIGHT)
> +			ret = isl29018_set_scale(chip, val, val2);
> +		break;
> +	default:
> +		break;
>  	}
>  	mutex_unlock(&chip->lock);
>  
> @@ -321,6 +386,19 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
>  		if (!ret)
>  			ret = IIO_VAL_INT;
>  		break;
> +	case IIO_CHAN_INFO_RANGE:
> +		if (chan->type == IIO_LIGHT) {
> +			*val = isl29018_ranges[chip->range];
> +			ret = IIO_VAL_INT;
> +		}
> +		break;
> +	case IIO_CHAN_INFO_SCALE:
> +		if (chan->type == IIO_LIGHT) {
> +			*val = chip->scale.scale;
> +			*val2 = chip->scale.uscale;
> +			ret = IIO_VAL_INT_PLUS_MICRO;
> +		}
> +		break;
>  	case IIO_CHAN_INFO_CALIBSCALE:
>  		if (chan->type == IIO_LIGHT) {
>  			*val = chip->calibscale;
> @@ -340,7 +418,9 @@ static int isl29018_read_raw(struct iio_dev *indio_dev,
>  	.indexed = 1,							\
>  	.channel = 0,							\
>  	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |		\
> -	BIT(IIO_CHAN_INFO_CALIBSCALE),					\
> +	BIT(IIO_CHAN_INFO_CALIBSCALE) |				\
> +	BIT(IIO_CHAN_INFO_SCALE) |				\
> +	BIT(IIO_CHAN_INFO_RANGE),					\
>  }
>  
>  #define ISL29018_IR_CHANNEL {						\
> @@ -366,6 +446,9 @@ static const struct iio_chan_spec isl29023_channels[] = {
>  	ISL29018_IR_CHANNEL,
>  };
>  
> +static IIO_DEVICE_ATTR(in_illuminance_scale_available, S_IRUGO | S_IWUSR,
> +		show_scale_available, NULL, 0);
> +static IIO_CONST_ATTR(in_illuminance_range_available, "1000 4000 16000 64000");
>  static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
>  					S_IRUGO | S_IWUSR,
>  					show_prox_infrared_suppression,
> @@ -374,11 +457,15 @@ static IIO_DEVICE_ATTR(proximity_on_chip_ambient_infrared_suppression,
>  #define ISL29018_DEV_ATTR(name) (&iio_dev_attr_##name.dev_attr.attr)
>  #define ISL29018_CONST_ATTR(name) (&iio_const_attr_##name.dev_attr.attr)
>  static struct attribute *isl29018_attributes[] = {
> +	ISL29018_DEV_ATTR(in_illuminance_scale_available),
> +	ISL29018_CONST_ATTR(in_illuminance_range_available),
>  	ISL29018_DEV_ATTR(proximity_on_chip_ambient_infrared_suppression),
>  	NULL
>  };
>  
>  static struct attribute *isl29023_attributes[] = {
> +	ISL29018_DEV_ATTR(in_illuminance_scale_available),
> +	ISL29018_CONST_ATTR(in_illuminance_range_available),
>  	NULL
>  };
>  
> @@ -422,8 +509,6 @@ enum {
>  static int isl29018_chip_init(struct isl29018_chip *chip)
>  {
>  	int status;
> -	unsigned int new_adc_bit;
> -	unsigned int new_range;
>  
>  	if (chip->type == isl29035) {
>  		status = isl29035_detect(chip);
> @@ -472,15 +557,12 @@ static int isl29018_chip_init(struct isl29018_chip *chip)
>  	usleep_range(1000, 2000);	/* per data sheet, page 10 */
>  
>  	/* set defaults */
> -	status = isl29018_set_range(chip, chip->range, &new_range);
> +	status = isl29018_set_range(chip, isl29018_ranges[chip->range]);
>  	if (status < 0) {
>  		dev_err(chip->dev, "Init of isl29018 fails\n");
>  		return status;
>  	}
>  
> -	status = isl29018_set_resolution(chip, chip->adc_bit,
> -						&new_adc_bit);
> -
>  	return 0;
>  }
>  
> @@ -609,8 +691,7 @@ static int isl29018_probe(struct i2c_client *client,
>  	chip->type = dev_id;
>  	chip->calibscale = 1;
>  	chip->ucalibscale = 0;
> -	chip->range = 1000;
> -	chip->adc_bit = 16;
> +	chip->range = ISL29018_RANGE_1;
>  	chip->suspended = false;
>  
>  	chip->regmap = devm_regmap_init_i2c(client,
> 

Re: [PATCH 4/4] staging: iio: light: isl29018: Use standard sysfs attributes for range and scale

[Daniel Baluta]

On Sun, Mar 8, 2015 at 1:50 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 23/02/15 19:35, Roberta Dobrescu wrote:
>> This patch refactors the isl29018 driver code in order to use standard
>> sysfs attributes for range and scale.
>>
>> ISL29018 light sensor uses four ranges and four ADC's resolutions
>> which influence the calculated lux.
>>
>> This patch eliminates the resolution (adc bits) and introduces scale.
>> Each range (1k, 4k, 16k or 64k) has a corresponding set of 4 scales
>> (for 16, 12, 8 or 4 bits). Both range and scale can be changed by the
>> user according to the corresponding set of values (exposed by the attributes
>> in_illuminance_range_available and in_illuminance_scale_available).
>>
>> When the range is changed, the set of available scales is set accordingly and
>> by default the scale used is the one for 16 adc bits. Scale can be changed
>> anytime with one of the available values.
> So we have 3 things interacting here.
>
> range - absolutely controlled for the device
> scale - from current range, with the adc resolution confusing things - we could just
>         multiply the output result up and not need to report this to userspace at all
> integration_time - This is the one element that's really being controlled by the adc
>                    resolution. This adc resolution is really effecting our sensitivity
>                    not the accuracy of measurement of the signal.
>
> Hence I'd have your adc resolution controlled by in_illuminance0_integration_time
> and the full scale range controlled by scale.
>
> For the lower adc resolutions simply apply the relevant scaling to get the output
> value to be on the same scale (assuming the range has not changed)
>
> How does that sound?

Sounds good to me. I think it's the cleanest solution, and we also avoid
releasing IIO_CHAN_INFO_RANGE kraken to user space :).

thanks,
daniel.

Re: [PATCH 4/4] staging: iio: light: isl29018: Use standard sysfs attributes for range and scale

[Roberta Dobrescu]

On 08.03.2015 13:50, Jonathan Cameron wrote:
> On 23/02/15 19:35, Roberta Dobrescu wrote:
>> This patch refactors the isl29018 driver code in order to use standard
>> sysfs attributes for range and scale.
>>
>> ISL29018 light sensor uses four ranges and four ADC's resolutions
>> which influence the calculated lux.
>>
>> This patch eliminates the resolution (adc bits) and introduces scale.
>> Each range (1k, 4k, 16k or 64k) has a corresponding set of 4 scales
>> (for 16, 12, 8 or 4 bits). Both range and scale can be changed by the
>> user according to the corresponding set of values (exposed by the attributes
>> in_illuminance_range_available and in_illuminance_scale_available).
>>
>> When the range is changed, the set of available scales is set accordingly and
>> by default the scale used is the one for 16 adc bits. Scale can be changed
>> anytime with one of the available values.
> So we have 3 things interacting here.
>
> range - absolutely controlled for the device
> scale - from current range, with the adc resolution confusing things - we could just
>          multiply the output result up and not need to report this to userspace at all
> integration_time - This is the one element that's really being controlled by the adc
>                     resolution. This adc resolution is really effecting our sensitivity
>                     not the accuracy of measurement of the signal.
>
> Hence I'd have your adc resolution controlled by in_illuminance0_integration_time
> and the full scale range controlled by scale.
>
> For the lower adc resolutions simply apply the relevant scaling to get the output
> value to be on the same scale (assuming the range has not changed)
>
> How does that sound?

Sounds good :D.

Thanks for reviewing this.

Roberta