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

Groups > linux.kernel > #1520243 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  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

#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] | [next] | [standalone]

#1521513

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

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

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

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

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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web