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

Groups > linux.kernel > #1519589 > unrolled thread

[PATCH 0/2] staging: iio: ad7606: move driver out of staging

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] staging: iio: ad7606: move driver out of staging Eva Rachel Retuya <eraretuya@gmail.com> - 2016-11-11 07:40 +0100
    [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale Eva Rachel Retuya <eraretuya@gmail.com> - 2016-11-11 07:40 +0100
      Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Lars-Peter Clausen <lars@metafoo.de> - 2016-11-11 15:20 +0100
        Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Jonathan Cameron <jic23@kernel.org> - 2016-11-12 15:30 +0100
          Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Lars-Peter Clausen <lars@metafoo.de> - 2016-11-14 11:40 +0100
            Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2016-11-14 18:30 +0100
          Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Linus Walleij <linus.walleij@linaro.org> - 2016-11-14 18:00 +0100
            Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Lars-Peter Clausen <lars@metafoo.de> - 2016-11-14 20:00 +0100
              Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale Jonathan Cameron <jic23@jic23.retrosnub.co.uk> - 2016-11-14 23:20 +0100
              Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Linus Walleij <linus.walleij@linaro.org> - 2016-11-15 00:20 +0100
                Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Jonathan Cameron <jic23@kernel.org> - 2016-11-19 13:40 +0100
        Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Eva Rachel Retuya <eraretuya@gmail.com> - 2016-11-12 15:50 +0100
          Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available  with corresponding scale Lars-Peter Clausen <lars@metafoo.de> - 2016-11-14 11:30 +0100
    Re: [PATCH 2/2] staging: iio: ad7606: move out of staging Lars-Peter Clausen <lars@metafoo.de> - 2016-11-11 15:30 +0100
      Re: [PATCH 2/2] staging: iio: ad7606: move out of staging Jonathan Cameron <jic23@kernel.org> - 2016-11-12 15:30 +0100
        Re: [PATCH 2/2] staging: iio: ad7606: move out of staging Eva Rachel Retuya <eraretuya@gmail.com> - 2016-11-12 15:40 +0100
          Re: [PATCH 2/2] staging: iio: ad7606: move out of staging Jonathan Cameron <jic23@kernel.org> - 2016-11-12 15:50 +0100

#1519589 — [PATCH 0/2] staging: iio: ad7606: move driver out of staging

Address the last remaining TODO [1] for this driver and move it from staging
into mainline.

[1] https://marc.info/?l=linux-iio&m=147689684332118&w=2 

Eva Rachel Retuya (2):
  staging: iio: ad7606: replace range/range_available with corresponding
    scale
  staging: iio: ad7606: move out of staging

 drivers/iio/adc/Kconfig              |  34 +++
 drivers/iio/adc/Makefile             |   3 +
 drivers/iio/adc/ad7606.c             | 519 +++++++++++++++++++++++++++++++++
 drivers/iio/adc/ad7606.h             |  78 +++++
 drivers/iio/adc/ad7606_par.c         | 112 ++++++++
 drivers/iio/adc/ad7606_spi.c         |  78 +++++
 drivers/staging/iio/adc/Kconfig      |  34 ---
 drivers/staging/iio/adc/Makefile     |   4 -
 drivers/staging/iio/adc/ad7606.c     | 543 -----------------------------------
 drivers/staging/iio/adc/ad7606.h     |  78 -----
 drivers/staging/iio/adc/ad7606_par.c | 112 --------
 drivers/staging/iio/adc/ad7606_spi.c |  78 -----
 12 files changed, 824 insertions(+), 849 deletions(-)
 create mode 100644 drivers/iio/adc/ad7606.c
 create mode 100644 drivers/iio/adc/ad7606.h
 create mode 100644 drivers/iio/adc/ad7606_par.c
 create mode 100644 drivers/iio/adc/ad7606_spi.c
 delete mode 100644 drivers/staging/iio/adc/ad7606.c
 delete mode 100644 drivers/staging/iio/adc/ad7606.h
 delete mode 100644 drivers/staging/iio/adc/ad7606_par.c
 delete mode 100644 drivers/staging/iio/adc/ad7606_spi.c

-- 
2.7.4

[toc] | [next] | [standalone]

#1519590 — [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

Eliminate the non-standard attribute in_voltage_range and move its
functionality under the scale attribute. read_raw() has been taken care
of previously so only write_raw() is handled here.

Additionally, rename the attribute in_voltage_range_available into
in_voltage_scale_available.

Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
 drivers/staging/iio/adc/ad7606.c | 56 ++++++++++++----------------------------
 1 file changed, 16 insertions(+), 40 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
index 4531908..cceb18c 100644
--- a/drivers/staging/iio/adc/ad7606.c
+++ b/drivers/staging/iio/adc/ad7606.c
@@ -161,42 +161,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
 	return -EINVAL;
 }
 
-static ssize_t ad7606_show_range(struct device *dev,
-				 struct device_attribute *attr, char *buf)
-{
-	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
-	struct ad7606_state *st = iio_priv(indio_dev);
-
-	return sprintf(buf, "%u\n", st->range);
-}
-
-static ssize_t ad7606_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 ad7606_state *st = iio_priv(indio_dev);
-	unsigned long lval;
-	int ret;
-
-	ret = kstrtoul(buf, 10, &lval);
-	if (ret)
-		return ret;
-
-	if (!(lval == 5000 || lval == 10000))
-		return -EINVAL;
-
-	mutex_lock(&indio_dev->mlock);
-	gpiod_set_value(st->gpio_range, lval == 10000);
-	st->range = lval;
-	mutex_unlock(&indio_dev->mlock);
-
-	return count;
-}
-
-static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
-		       ad7606_show_range, ad7606_store_range, 0);
-static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
+static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
 
 static int ad7606_oversampling_get_index(unsigned int val)
 {
@@ -221,6 +186,19 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 	int ret;
 
 	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		if (val2)
+			return -EINVAL;
+
+		if (!(val == 5000 || val == 10000))
+			return -EINVAL;
+
+		mutex_lock(&indio_dev->mlock);
+		gpiod_set_value(st->gpio_range, val == 10000);
+		st->range = val;
+		mutex_unlock(&indio_dev->mlock);
+
+		return 0;
 	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
 		if (val2)
 			return -EINVAL;
@@ -247,8 +225,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
 static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
 
 static struct attribute *ad7606_attributes_os_and_range[] = {
-	&iio_dev_attr_in_voltage_range.dev_attr.attr,
-	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
+	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
 	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
 	NULL,
 };
@@ -267,8 +244,7 @@ static const struct attribute_group ad7606_attribute_group_os = {
 };
 
 static struct attribute *ad7606_attributes_range[] = {
-	&iio_dev_attr_in_voltage_range.dev_attr.attr,
-	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
+	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
 	NULL,
 };
 
-- 
2.7.4

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

#1519817 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
> Eliminate the non-standard attribute in_voltage_range and move its
> functionality under the scale attribute. read_raw() has been taken care
> of previously so only write_raw() is handled here.
> 
> Additionally, rename the attribute in_voltage_range_available into
> in_voltage_scale_available.
> 
> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>

Hi,

Thanks for the patch. Unfortunately this is not quite this straight forward.

The scale is what you multiply the raw with to get the value in the standard
IIO unit. Range as implemented in this driver is the maximum output voltage.

To get the scale we need to look at the transfer function of the ADC [1].
The transfer function tells us that 1 LSB is 305uV for the 10V range and
152uV for the 5V range.

More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).

Since the default unit for IIO is mV for voltages we need to multiply this
by 1000.

The other thing we need to handle is the case where the RANGE pin is not
connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
somehow.

[1]
http://www.analog.com/media/en/technical-documentation/data-sheets/AD7606_7606-6_7606-4.pdf#page=23
(right bottom)

> ---
>  drivers/staging/iio/adc/ad7606.c | 56 ++++++++++++----------------------------
>  1 file changed, 16 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
> index 4531908..cceb18c 100644
> --- a/drivers/staging/iio/adc/ad7606.c
> +++ b/drivers/staging/iio/adc/ad7606.c
> @@ -161,42 +161,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>  	return -EINVAL;
>  }
>  
> -static ssize_t ad7606_show_range(struct device *dev,
> -				 struct device_attribute *attr, char *buf)
> -{
> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> -	struct ad7606_state *st = iio_priv(indio_dev);
> -
> -	return sprintf(buf, "%u\n", st->range);
> -}
> -
> -static ssize_t ad7606_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 ad7606_state *st = iio_priv(indio_dev);
> -	unsigned long lval;
> -	int ret;
> -
> -	ret = kstrtoul(buf, 10, &lval);
> -	if (ret)
> -		return ret;
> -
> -	if (!(lval == 5000 || lval == 10000))
> -		return -EINVAL;
> -
> -	mutex_lock(&indio_dev->mlock);
> -	gpiod_set_value(st->gpio_range, lval == 10000);
> -	st->range = lval;
> -	mutex_unlock(&indio_dev->mlock);
> -
> -	return count;
> -}
> -
> -static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
> -		       ad7606_show_range, ad7606_store_range, 0);
> -static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
> +static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
>  
>  static int ad7606_oversampling_get_index(unsigned int val)
>  {
> @@ -221,6 +186,19 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  	int ret;
>  
>  	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +		if (val2)
> +			return -EINVAL;
> +
> +		if (!(val == 5000 || val == 10000))
> +			return -EINVAL;
> +
> +		mutex_lock(&indio_dev->mlock);
> +		gpiod_set_value(st->gpio_range, val == 10000);
> +		st->range = val;
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		return 0;
>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>  		if (val2)
>  			return -EINVAL;
> @@ -247,8 +225,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>  static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
>  
>  static struct attribute *ad7606_attributes_os_and_range[] = {
> -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
> -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
> +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
>  	NULL,
>  };
> @@ -267,8 +244,7 @@ static const struct attribute_group ad7606_attribute_group_os = {
>  };
>  
>  static struct attribute *ad7606_attributes_range[] = {
> -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
> -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
> +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
>  	NULL,
>  };
>  
> 

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

#1520243 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On 11/11/16 14:18, Lars-Peter Clausen wrote:
> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>> Eliminate the non-standard attribute in_voltage_range and move its
>> functionality under the scale attribute. read_raw() has been taken care
>> of previously so only write_raw() is handled here.
>>
>> Additionally, rename the attribute in_voltage_range_available into
>> in_voltage_scale_available.
>>
>> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> 
> Hi,
> 
> Thanks for the patch. Unfortunately this is not quite this straight forward.
> 
> The scale is what you multiply the raw with to get the value in the standard
> IIO unit. Range as implemented in this driver is the maximum output voltage.
> 
> To get the scale we need to look at the transfer function of the ADC [1].
> The transfer function tells us that 1 LSB is 305uV for the 10V range and
> 152uV for the 5V range.
> 
> More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).
> 
> Since the default unit for IIO is mV for voltages we need to multiply this
> by 1000.
> 
> The other thing we need to handle is the case where the RANGE pin is not
> connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
> somehow.
Is it just me who thought, we need a fixed GPI like a fixed regulator?
Would allow this sort of fixed wiring to be simply defined.

Linus, worth exploring?

I doubt this will be the last case of this particular problem
(not exactly unusual to hard wire control lines like these as which range
makes sense is often a feature of the device).

Would be a pain to have to add code to every driver to cover the fixed
case.
> 
> [1]
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD7606_7606-6_7606-4.pdf#page=23
> (right bottom)
> 
>> ---
>>  drivers/staging/iio/adc/ad7606.c | 56 ++++++++++++----------------------------
>>  1 file changed, 16 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
>> index 4531908..cceb18c 100644
>> --- a/drivers/staging/iio/adc/ad7606.c
>> +++ b/drivers/staging/iio/adc/ad7606.c
>> @@ -161,42 +161,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
>>  	return -EINVAL;
>>  }
>>  
>> -static ssize_t ad7606_show_range(struct device *dev,
>> -				 struct device_attribute *attr, char *buf)
>> -{
>> -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
>> -	struct ad7606_state *st = iio_priv(indio_dev);
>> -
>> -	return sprintf(buf, "%u\n", st->range);
>> -}
>> -
>> -static ssize_t ad7606_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 ad7606_state *st = iio_priv(indio_dev);
>> -	unsigned long lval;
>> -	int ret;
>> -
>> -	ret = kstrtoul(buf, 10, &lval);
>> -	if (ret)
>> -		return ret;
>> -
>> -	if (!(lval == 5000 || lval == 10000))
>> -		return -EINVAL;
>> -
>> -	mutex_lock(&indio_dev->mlock);
>> -	gpiod_set_value(st->gpio_range, lval == 10000);
>> -	st->range = lval;
>> -	mutex_unlock(&indio_dev->mlock);
>> -
>> -	return count;
>> -}
>> -
>> -static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
>> -		       ad7606_show_range, ad7606_store_range, 0);
>> -static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
>> +static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
>>  
>>  static int ad7606_oversampling_get_index(unsigned int val)
>>  {
>> @@ -221,6 +186,19 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>>  	int ret;
>>  
>>  	switch (mask) {
>> +	case IIO_CHAN_INFO_SCALE:
>> +		if (val2)
>> +			return -EINVAL;
>> +
>> +		if (!(val == 5000 || val == 10000))
>> +			return -EINVAL;
>> +
>> +		mutex_lock(&indio_dev->mlock);
>> +		gpiod_set_value(st->gpio_range, val == 10000);
>> +		st->range = val;
>> +		mutex_unlock(&indio_dev->mlock);
>> +
>> +		return 0;
>>  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
>>  		if (val2)
>>  			return -EINVAL;
>> @@ -247,8 +225,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
>>  static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
>>  
>>  static struct attribute *ad7606_attributes_os_and_range[] = {
>> -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
>> -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
>> +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
>>  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
>>  	NULL,
>>  };
>> @@ -267,8 +244,7 @@ static const struct attribute_group ad7606_attribute_group_os = {
>>  };
>>  
>>  static struct attribute *ad7606_attributes_range[] = {
>> -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
>> -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
>> +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
>>  	NULL,
>>  };
>>  
>>
> 

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

#1521513 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On 11/12/2016 03:24 PM, Jonathan Cameron wrote:
> On 11/11/16 14:18, Lars-Peter Clausen wrote:
>> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>>> Eliminate the non-standard attribute in_voltage_range and move its
>>> functionality under the scale attribute. read_raw() has been taken care
>>> of previously so only write_raw() is handled here.
>>>
>>> Additionally, rename the attribute in_voltage_range_available into
>>> in_voltage_scale_available.
>>>
>>> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
>>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>>
>> Hi,
>>
>> Thanks for the patch. Unfortunately this is not quite this straight forward.
>>
>> The scale is what you multiply the raw with to get the value in the standard
>> IIO unit. Range as implemented in this driver is the maximum output voltage.
>>
>> To get the scale we need to look at the transfer function of the ADC [1].
>> The transfer function tells us that 1 LSB is 305uV for the 10V range and
>> 152uV for the 5V range.
>>
>> More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).
>>
>> Since the default unit for IIO is mV for voltages we need to multiply this
>> by 1000.
>>
>> The other thing we need to handle is the case where the RANGE pin is not
>> connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
>> somehow.
> Is it just me who thought, we need a fixed GPI like a fixed regulator?
> Would allow this sort of fixed wiring to be simply defined.
> 
> Linus, worth exploring?
> 
> I doubt this will be the last case of this particular problem
> (not exactly unusual to hard wire control lines like these as which range
> makes sense is often a feature of the device).
> 
> Would be a pain to have to add code to every driver to cover the fixed
> case.

We still have to add code to every driver to cover the fixed case since the
mode of operation is inherently different. But it would be nice to have a
coherent way of doing so with a standardized interface rather than having
every device come up with its own code and bindings.

This could either be handled directly by the GPIO API or by a small set of
helper functions on top of it.

I think the most important part for now is to agree on a standard binding to
describe hardwired GPIOs. We can still rework the kernel API later on, but
the DT bindings will be set in stone.

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

#1521900 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On 14 November 2016 10:30:50 GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 11/12/2016 03:24 PM, Jonathan Cameron wrote:
>> On 11/11/16 14:18, Lars-Peter Clausen wrote:
>>> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>>>> Eliminate the non-standard attribute in_voltage_range and move its
>>>> functionality under the scale attribute. read_raw() has been taken
>care
>>>> of previously so only write_raw() is handled here.
>>>>
>>>> Additionally, rename the attribute in_voltage_range_available into
>>>> in_voltage_scale_available.
>>>>
>>>> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
>>>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>>>
>>> Hi,
>>>
>>> Thanks for the patch. Unfortunately this is not quite this straight
>forward.
>>>
>>> The scale is what you multiply the raw with to get the value in the
>standard
>>> IIO unit. Range as implemented in this driver is the maximum output
>voltage.
>>>
>>> To get the scale we need to look at the transfer function of the ADC
>[1].
>>> The transfer function tells us that 1 LSB is 305uV for the 10V range
>and
>>> 152uV for the 5V range.
>>>
>>> More specifically this is $RANGE*2/2**16 (times two since the ADC is
>bipolar).
>>>
>>> Since the default unit for IIO is mV for voltages we need to
>multiply this
>>> by 1000.
>>>
>>> The other thing we need to handle is the case where the RANGE pin is
>not
>>> connected to a GPIO but either hardwired to 1 or 0. Which we need to
>handle
>>> somehow.
>> Is it just me who thought, we need a fixed GPI like a fixed
>regulator?
>> Would allow this sort of fixed wiring to be simply defined.
>> 
>> Linus, worth exploring?
>> 
>> I doubt this will be the last case of this particular problem
>> (not exactly unusual to hard wire control lines like these as which
>range
>> makes sense is often a feature of the device).
>> 
>> Would be a pain to have to add code to every driver to cover the
>fixed
>> case.
>
>We still have to add code to every driver to cover the fixed case since
>the
>mode of operation is inherently different. But it would be nice to have
>a
>coherent way of doing so with a standardized interface rather than
>having
>every device come up with its own code and bindings.
Agreed. Not totally obvious how to do it but definitely don't want to work around it by custom bindings all over the place.

A simple is_gpo_fixed or similar would get the driver the info it needs. Could do it with errors 
but that would perhaps be ugly!
The other flags related to type of input/output can be emulated so there doesn't seem to be anything similar...
>
>This could either be handled directly by the GPIO API or by a small set
>of
>helper functions on top of it.
>
>I think the most important part for now is to agree on a standard
>binding to
>describe hardwired GPIOs. We can still rework the kernel API later on,
>but
>the DT bindings will be set in stone.
Agreed. Some magic gpio description or a fake gpio chip similar to fixed regs?

Jonathan
>--
>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

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1521862 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On Sat, Nov 12, 2016 at 3:24 PM, Jonathan Cameron <jic23@kernel.org> wrote:

> Is it just me who thought, we need a fixed GPI like a fixed regulator?
> Would allow this sort of fixed wiring to be simply defined.
>
> Linus, worth exploring?

So if fixed regulator is for a voltage provider, this would be
pretty much the inverse: deciding for a voltage range by switching
a GPIO.

No previous experience with that, it should be outside of the
GPIO subsystem for sure...

Yours,
Linus Walleij

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

#1522026 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On 11/14/2016 05:58 PM, Linus Walleij wrote:
> On Sat, Nov 12, 2016 at 3:24 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> 
>> Is it just me who thought, we need a fixed GPI like a fixed regulator?
>> Would allow this sort of fixed wiring to be simply defined.
>>
>> Linus, worth exploring?
> 
> So if fixed regulator is for a voltage provider, this would be
> pretty much the inverse: deciding for a voltage range by switching
> a GPIO.

It's about figuring out the setting of a "GPIO" that can't be changed from
software.

Devices sometimes, instead of a configuration bus like I2C or SPI, use
simple input pins, that can either be set to high or low, to allow software
the state of the device. The GPIO API is typically used to configure these pins.

This works fine as long as the pin is connected to a GPIO. But sometimes the
system designer decides that a settings does not need to be configurable, in
this case the pin will be tied to logic low or high directly on the PCB
without any GPIO controller being involved.

Sometimes a driver wants to know how the pin is wired up so it can report to
userspace this part runs in the following mode and the mode can't be
changed. In a sense it is like a reverse GPIO hog.

Considering that this is a common usecase the question was how this can be
implemented in a driver independent way to avoid code duplication and
slightly different variations of what is effectively the same DT/ACPI binding.

E.g. lets say for a configurable pin you use

	range-gpio = <&gpio ...>;

and for a static pin

	range-gpio-fixed = <1>;

Or something similar.

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

#1522125 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On 14 November 2016 18:53:28 GMT+00:00, Lars-Peter Clausen <lars@metafoo.de> wrote:
>On 11/14/2016 05:58 PM, Linus Walleij wrote:
>> On Sat, Nov 12, 2016 at 3:24 PM, Jonathan Cameron <jic23@kernel.org>
>wrote:
>> 
>>> Is it just me who thought, we need a fixed GPI like a fixed
>regulator?
Probably didn't help clarity that I described it as an input pin whereas it's kind of like having an 
output pin whose state you can't change...

>>> Would allow this sort of fixed wiring to be simply defined.
>>>
>>> Linus, worth exploring?
>> 
>> So if fixed regulator is for a voltage provider, this would be
>> pretty much the inverse: deciding for a voltage range by switching
>> a GPIO.
>
>It's about figuring out the setting of a "GPIO" that can't be changed
>from
>software.
>
>Devices sometimes, instead of a configuration bus like I2C or SPI, use
>simple input pins, that can either be set to high or low, to allow
>software
>the state of the device. The GPIO API is typically used to configure
>these pins.
>
>This works fine as long as the pin is connected to a GPIO. But
>sometimes the
>system designer decides that a settings does not need to be
>configurable, in
>this case the pin will be tied to logic low or high directly on the PCB
>without any GPIO controller being involved.
>
>Sometimes a driver wants to know how the pin is wired up so it can
>report to
>userspace this part runs in the following mode and the mode can't be
>changed. In a sense it is like a reverse GPIO hog.
>
>Considering that this is a common usecase the question was how this can
>be
>implemented in a driver independent way to avoid code duplication and
>slightly different variations of what is effectively the same DT/ACPI
>binding.
>
>E.g. lets say for a configurable pin you use
>
>	range-gpio = <&gpio ...>;
>
>and for a static pin
>
>	range-gpio-fixed = <1>;
>
>Or something similar.
>
>--
>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

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

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

#1522172 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On Mon, Nov 14, 2016 at 7:53 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:

> It's about figuring out the setting of a "GPIO" that can't be changed from
> software.
>
> Devices sometimes, instead of a configuration bus like I2C or SPI, use
> simple input pins, that can either be set to high or low, to allow software
> the state of the device. The GPIO API is typically used to configure these pins.
>
> This works fine as long as the pin is connected to a GPIO. But sometimes the
> system designer decides that a settings does not need to be configurable, in
> this case the pin will be tied to logic low or high directly on the PCB
> without any GPIO controller being involved.
>
> Sometimes a driver wants to know how the pin is wired up so it can report to
> userspace this part runs in the following mode and the mode can't be
> changed. In a sense it is like a reverse GPIO hog.
>
> Considering that this is a common usecase the question was how this can be
> implemented in a driver independent way to avoid code duplication and
> slightly different variations of what is effectively the same DT/ACPI binding.
>
> E.g. lets say for a configurable pin you use
>
>         range-gpio = <&gpio ...>;
>
> and for a static pin
>
>         range-gpio-fixed = <1>;
>
> Or something similar.

Aha I understand.

Usually I feel we need not shoehorn stuff into GPIO because it is convenient,
it might be best to leave the GPIO optional and if it is not there, look for
a custom attribute that represents the "hogging" to 0/1. I think trying
to extend GPIO bindings to cover it is overgeneralization, instead go
for a local binding for this kind of devices.

But mainly it is a question to the DT bindings maintainers.

Yours,
Linus Walleij

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

#1525986 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On 14/11/16 23:12, Linus Walleij wrote:
> On Mon, Nov 14, 2016 at 7:53 PM, Lars-Peter Clausen <lars@metafoo.de> wrote:
> 
>> It's about figuring out the setting of a "GPIO" that can't be changed from
>> software.
>>
>> Devices sometimes, instead of a configuration bus like I2C or SPI, use
>> simple input pins, that can either be set to high or low, to allow software
>> the state of the device. The GPIO API is typically used to configure these pins.
>>
>> This works fine as long as the pin is connected to a GPIO. But sometimes the
>> system designer decides that a settings does not need to be configurable, in
>> this case the pin will be tied to logic low or high directly on the PCB
>> without any GPIO controller being involved.
>>
>> Sometimes a driver wants to know how the pin is wired up so it can report to
>> userspace this part runs in the following mode and the mode can't be
>> changed. In a sense it is like a reverse GPIO hog.
>>
>> Considering that this is a common usecase the question was how this can be
>> implemented in a driver independent way to avoid code duplication and
>> slightly different variations of what is effectively the same DT/ACPI binding.
>>
>> E.g. lets say for a configurable pin you use
>>
>>         range-gpio = <&gpio ...>;
>>
>> and for a static pin
>>
>>         range-gpio-fixed = <1>;
>>
>> Or something similar.
> 
> Aha I understand.
> 
> Usually I feel we need not shoehorn stuff into GPIO because it is convenient,
> it might be best to leave the GPIO optional and if it is not there, look for
> a custom attribute that represents the "hogging" to 0/1. I think trying
> to extend GPIO bindings to cover it is overgeneralization, instead go
> for a local binding for this kind of devices.
> 
> But mainly it is a question to the DT bindings maintainers.
That's a reasonable approach, but I'd certainly like to see a generic binding
to describe it.  It's a pretty common situation.

Seems more likely we'll get a device tree maintainer response if we cc them ;)

So Mark, Rob and thoughts on this?

Thanks,

Jonathan
> 
> Yours,
> Linus Walleij
> 

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

#1520253 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

Hello, 

Thanks for explaining it. Now I understand better why read_raw is
formatted in that manner. I have some questions in-line:

On Fri, Nov 11, 2016 at 03:18:37PM +0100, Lars-Peter Clausen wrote:
> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
> > Eliminate the non-standard attribute in_voltage_range and move its
> > functionality under the scale attribute. read_raw() has been taken care
> > of previously so only write_raw() is handled here.
> > 
> > Additionally, rename the attribute in_voltage_range_available into
> > in_voltage_scale_available.
> > 
> > Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> 
> Hi,
> 
> Thanks for the patch. Unfortunately this is not quite this straight forward.
> 
> The scale is what you multiply the raw with to get the value in the standard
> IIO unit. Range as implemented in this driver is the maximum output voltage.
> 
> To get the scale we need to look at the transfer function of the ADC [1].
> The transfer function tells us that 1 LSB is 305uV for the 10V range and
> 152uV for the 5V range.

Instead of 10000 and 5000, these will be the values for the
scale_available attribute?

> 
> More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).
> 
> Since the default unit for IIO is mV for voltages we need to multiply this
> by 1000.
> 
> The other thing we need to handle is the case where the RANGE pin is not
> connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
> somehow.

Will this be on the same patch or a separate one? Can you please give a
hint on how to check if it is hardwired?

Eva

> 
> [1]
> http://www.analog.com/media/en/technical-documentation/data-sheets/AD7606_7606-6_7606-4.pdf#page=23
> (right bottom)
> 
> > ---
> >  drivers/staging/iio/adc/ad7606.c | 56 ++++++++++++----------------------------
> >  1 file changed, 16 insertions(+), 40 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/adc/ad7606.c b/drivers/staging/iio/adc/ad7606.c
> > index 4531908..cceb18c 100644
> > --- a/drivers/staging/iio/adc/ad7606.c
> > +++ b/drivers/staging/iio/adc/ad7606.c
> > @@ -161,42 +161,7 @@ static int ad7606_read_raw(struct iio_dev *indio_dev,
> >  	return -EINVAL;
> >  }
> >  
> > -static ssize_t ad7606_show_range(struct device *dev,
> > -				 struct device_attribute *attr, char *buf)
> > -{
> > -	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> > -	struct ad7606_state *st = iio_priv(indio_dev);
> > -
> > -	return sprintf(buf, "%u\n", st->range);
> > -}
> > -
> > -static ssize_t ad7606_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 ad7606_state *st = iio_priv(indio_dev);
> > -	unsigned long lval;
> > -	int ret;
> > -
> > -	ret = kstrtoul(buf, 10, &lval);
> > -	if (ret)
> > -		return ret;
> > -
> > -	if (!(lval == 5000 || lval == 10000))
> > -		return -EINVAL;
> > -
> > -	mutex_lock(&indio_dev->mlock);
> > -	gpiod_set_value(st->gpio_range, lval == 10000);
> > -	st->range = lval;
> > -	mutex_unlock(&indio_dev->mlock);
> > -
> > -	return count;
> > -}
> > -
> > -static IIO_DEVICE_ATTR(in_voltage_range, S_IRUGO | S_IWUSR,
> > -		       ad7606_show_range, ad7606_store_range, 0);
> > -static IIO_CONST_ATTR(in_voltage_range_available, "5000 10000");
> > +static IIO_CONST_ATTR(in_voltage_scale_available, "5000 10000");
> >  
> >  static int ad7606_oversampling_get_index(unsigned int val)
> >  {
> > @@ -221,6 +186,19 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >  	int ret;
> >  
> >  	switch (mask) {
> > +	case IIO_CHAN_INFO_SCALE:
> > +		if (val2)
> > +			return -EINVAL;
> > +
> > +		if (!(val == 5000 || val == 10000))
> > +			return -EINVAL;
> > +
> > +		mutex_lock(&indio_dev->mlock);
> > +		gpiod_set_value(st->gpio_range, val == 10000);
> > +		st->range = val;
> > +		mutex_unlock(&indio_dev->mlock);
> > +
> > +		return 0;
> >  	case IIO_CHAN_INFO_OVERSAMPLING_RATIO:
> >  		if (val2)
> >  			return -EINVAL;
> > @@ -247,8 +225,7 @@ static int ad7606_write_raw(struct iio_dev *indio_dev,
> >  static IIO_CONST_ATTR(oversampling_ratio_available, "1 2 4 8 16 32 64");
> >  
> >  static struct attribute *ad7606_attributes_os_and_range[] = {
> > -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
> > -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
> > +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
> >  	&iio_const_attr_oversampling_ratio_available.dev_attr.attr,
> >  	NULL,
> >  };
> > @@ -267,8 +244,7 @@ static const struct attribute_group ad7606_attribute_group_os = {
> >  };
> >  
> >  static struct attribute *ad7606_attributes_range[] = {
> > -	&iio_dev_attr_in_voltage_range.dev_attr.attr,
> > -	&iio_const_attr_in_voltage_range_available.dev_attr.attr,
> > +	&iio_const_attr_in_voltage_scale_available.dev_attr.attr,
> >  	NULL,
> >  };
> >  
> > 
> 

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

#1521504 — Re: [PATCH 1/2] staging: iio: ad7606: replace range/range_available with corresponding scale

On 11/12/2016 03:22 PM, Eva Rachel Retuya wrote:
> Hello, 
> 
> Thanks for explaining it. Now I understand better why read_raw is
> formatted in that manner. I have some questions in-line:
> 
> On Fri, Nov 11, 2016 at 03:18:37PM +0100, Lars-Peter Clausen wrote:
>> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>>> Eliminate the non-standard attribute in_voltage_range and move its
>>> functionality under the scale attribute. read_raw() has been taken care
>>> of previously so only write_raw() is handled here.
>>>
>>> Additionally, rename the attribute in_voltage_range_available into
>>> in_voltage_scale_available.
>>>
>>> Suggested-by: Lars-Peter Clausen <lars@metafoo.de>
>>> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>>
>> Hi,
>>
>> Thanks for the patch. Unfortunately this is not quite this straight forward.
>>
>> The scale is what you multiply the raw with to get the value in the standard
>> IIO unit. Range as implemented in this driver is the maximum output voltage.
>>
>> To get the scale we need to look at the transfer function of the ADC [1].
>> The transfer function tells us that 1 LSB is 305uV for the 10V range and
>> 152uV for the 5V range.
> 
> Instead of 10000 and 5000, these will be the values for the
> scale_available attribute?

Yes. But these are slightly truncated values, we should try to minimize the
rounding error introduced by those scales by making them as accurate as
possible. The exact values are 5/2**16 and 2.5/2**16. Neither of which we
can express accurately. But we should use at least 6 digits and also round
to the closest rather than truncating.

> 
>>
>> More specifically this is $RANGE*2/2**16 (times two since the ADC is bipolar).
>>
>> Since the default unit for IIO is mV for voltages we need to multiply this
>> by 1000.
>>
>> The other thing we need to handle is the case where the RANGE pin is not
>> connected to a GPIO but either hardwired to 1 or 0. Which we need to handle
>> somehow.
> 
> Will this be on the same patch or a separate one? Can you please give a
> hint on how to check if it is hardwired?

The current driver checks if the GPIO is set and if it is not it does not
register the range attribute. As a first step we could mimic this behavior,
but long term we should find a way to discover the setting of the hardwired
pin and report the scale accordingly.

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

#1519822 — Re: [PATCH 2/2] staging: iio: ad7606: move out of staging

On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
> corresponding Makefile and Kconfig associated with the change.

This is obviously OK, but when you generate a patch that moves files use
`git format-patch -M ...`. This will generate a more compact patch.

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

#1520244 — Re: [PATCH 2/2] staging: iio: ad7606: move out of staging

On 11/11/16 14:22, Lars-Peter Clausen wrote:
> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
>> corresponding Makefile and Kconfig associated with the change.
> 
> This is obviously OK, but when you generate a patch that moves files use
> `git format-patch -M ...`. This will generate a more compact patch.
> 
I tend to make an exception for staging graduation patches.
The mere posting of the whole driver by not using the move detection
makes for really easy review of whether we have forgotten something
else that needs cleanup before the graduation.

Most of the time -M is much more sensible!

Jonathan

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

#1520249 — Re: [PATCH 2/2] staging: iio: ad7606: move out of staging

On Sat, Nov 12, 2016 at 02:26:51PM +0000, Jonathan Cameron wrote:
> On 11/11/16 14:22, Lars-Peter Clausen wrote:
> > On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
> >> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
> >> corresponding Makefile and Kconfig associated with the change.
> > 
> > This is obviously OK, but when you generate a patch that moves files use
> > `git format-patch -M ...`. This will generate a more compact patch.
> > 
> I tend to make an exception for staging graduation patches.
> The mere posting of the whole driver by not using the move detection
> makes for really easy review of whether we have forgotten something
> else that needs cleanup before the graduation.
> 
> Most of the time -M is much more sensible!
> 
> Jonathan

Should I use this -M on version 2 or still go with the previous method?

Thanks,
Eva

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

#1520254 — Re: [PATCH 2/2] staging: iio: ad7606: move out of staging

On 12/11/16 14:32, Eva Rachel Retuya wrote:
> On Sat, Nov 12, 2016 at 02:26:51PM +0000, Jonathan Cameron wrote:
>> On 11/11/16 14:22, Lars-Peter Clausen wrote:
>>> On 11/11/2016 07:34 AM, Eva Rachel Retuya wrote:
>>>> Move the ad7606 driver from staging/iio/adc to iio/adc. Also, update the
>>>> corresponding Makefile and Kconfig associated with the change.
>>>
>>> This is obviously OK, but when you generate a patch that moves files use
>>> `git format-patch -M ...`. This will generate a more compact patch.
>>>
>> I tend to make an exception for staging graduation patches.
>> The mere posting of the whole driver by not using the move detection
>> makes for really easy review of whether we have forgotten something
>> else that needs cleanup before the graduation.
>>
>> Most of the time -M is much more sensible!
>>
>> Jonathan
> 
> Should I use this -M on version 2 or still go with the previous method?
> 
> Thanks,
> Eva
> 
Go without the no -M option and face the ire of Lars ;)

J

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web