[PATCH] staging:iio:lpc32xx_adc: Add scale/offset support

[PATCH] staging:iio:lpc32xx_adc: Add scale/offset support

[Alban Bedel]

Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
---
 .../bindings/staging/iio/adc/lpc32xx-adc.txt       |    6 ++++
 drivers/staging/iio/adc/lpc32xx_adc.c              |   30 +++++++++++++++++++-
 2 files changed, 35 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
index b3629d3..515c128 100644
--- a/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
+++ b/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
@@ -6,6 +6,11 @@ Required properties:
   region.
 - interrupts: The ADC interrupt
 
+Optional properties:
+- reference-voltages: the 3 reference voltages value (in mV) to use
+  for scaling. If not given or 0 a scaling factor of 1 is used.
+- offsets: the 3 offsets to add to the raw value before scaling.
+
 Example:
 
 	adc@40048000 {
@@ -13,4 +18,5 @@ Example:
 		reg = <0x40048000 0x1000>;
 		interrupt-parent = <&mic>;
 		interrupts = <39 0>;
+		reference-voltages = <5000>, <3300>, <1200>;
 	};
diff --git a/drivers/staging/iio/adc/lpc32xx_adc.c b/drivers/staging/iio/adc/lpc32xx_adc.c
index 10c5962..aaa23ac 100644
--- a/drivers/staging/iio/adc/lpc32xx_adc.c
+++ b/drivers/staging/iio/adc/lpc32xx_adc.c
@@ -64,6 +64,8 @@ struct lpc32xx_adc_info {
 	struct completion completion;
 
 	u32 value;
+	u32 ref_voltage[3];
+	u32 offset[3];
 };
 
 static int lpc32xx_read_raw(struct iio_dev *indio_dev,
@@ -91,6 +93,26 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	}
 
+	if (mask == IIO_CHAN_INFO_SCALE) {
+		if (info->ref_voltage[chan->channel]) {
+			unsigned factor;
+			factor = div_u64((u64)info->ref_voltage[chan->channel] *
+					 (u64)1000000,
+					 ADC_VALUE_MASK);
+			*val = factor / 1000000;
+			*val2 = factor % 1000000;
+			return IIO_VAL_INT_PLUS_MICRO;
+		} else {
+			*val = 1;
+			return IIO_VAL_INT;
+		}
+	}
+
+	if (mask == IIO_CHAN_INFO_OFFSET) {
+		*val = info->offset[chan->channel];
+		return IIO_VAL_INT;
+	}
+
 	return -EINVAL;
 }
 
@@ -103,7 +125,9 @@ static const struct iio_info lpc32xx_adc_iio_info = {
 	.type = IIO_VOLTAGE,				\
 	.indexed = 1,					\
 	.channel = _index,				\
-	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT,	\
+	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |	\
+	             IIO_CHAN_INFO_SCALE_SEPARATE_BIT |	\
+	             IIO_CHAN_INFO_OFFSET_SEPARATE_BIT,	\
 	.address = AD_IN * _index,			\
 	.scan_index = _index,				\
 }
@@ -149,6 +173,10 @@ static int __devinit lpc32xx_adc_probe(struct platform_device *pdev)
 	}
 
 	info = iio_priv(iodev);
+	of_property_read_u32_array(pdev->dev.of_node, "reference-voltages",
+				   info->ref_voltage, ARRAY_SIZE(info->ref_voltage));
+	of_property_read_u32_array(pdev->dev.of_node, "offsets",
+				   info->offset, ARRAY_SIZE(info->offset));
 
 	info->adc_base = ioremap(res->start, resource_size(res));
 	if (!info->adc_base) {
-- 
1.7.0.4

Re: [PATCH] staging:iio:lpc32xx_adc: Add scale/offset support

[Roland Stigge]

Hi Alban,

thanks for the patch. I can confirm that it compiles and runs fine, but
maybe someone else can comment on the IIO part.

Some nitpicking below.

On 07/11/12 17:44, Alban Bedel wrote:
> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
> ---
>  .../bindings/staging/iio/adc/lpc32xx-adc.txt       |    6 ++++
>  drivers/staging/iio/adc/lpc32xx_adc.c              |   30 +++++++++++++++++++-
>  2 files changed, 35 insertions(+), 1 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
> index b3629d3..515c128 100644
> --- a/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
> +++ b/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
> @@ -6,6 +6,11 @@ Required properties:
>    region.
>  - interrupts: The ADC interrupt
>  
> +Optional properties:
> +- reference-voltages: the 3 reference voltages value (in mV) to use
> +  for scaling. If not given or 0 a scaling factor of 1 is used.
> +- offsets: the 3 offsets to add to the raw value before scaling.
> +
>  Example:
>  
>  	adc@40048000 {
> @@ -13,4 +18,5 @@ Example:
>  		reg = <0x40048000 0x1000>;
>  		interrupt-parent = <&mic>;
>  		interrupts = <39 0>;
> +		reference-voltages = <5000>, <3300>, <1200>;
>  	};
> diff --git a/drivers/staging/iio/adc/lpc32xx_adc.c b/drivers/staging/iio/adc/lpc32xx_adc.c
> index 10c5962..aaa23ac 100644
> --- a/drivers/staging/iio/adc/lpc32xx_adc.c
> +++ b/drivers/staging/iio/adc/lpc32xx_adc.c
> @@ -64,6 +64,8 @@ struct lpc32xx_adc_info {
>  	struct completion completion;
>  
>  	u32 value;
> +	u32 ref_voltage[3];
> +	u32 offset[3];
>  };
>  
>  static int lpc32xx_read_raw(struct iio_dev *indio_dev,
> @@ -91,6 +93,26 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  	}
>  
> +	if (mask == IIO_CHAN_INFO_SCALE) {
> +		if (info->ref_voltage[chan->channel]) {
> +			unsigned factor;
> +			factor = div_u64((u64)info->ref_voltage[chan->channel] *
> +					 (u64)1000000,
> +					 ADC_VALUE_MASK);
> +			*val = factor / 1000000;
> +			*val2 = factor % 1000000;
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		} else {
> +			*val = 1;
> +			return IIO_VAL_INT;
> +		}
> +	}
> +
> +	if (mask == IIO_CHAN_INFO_OFFSET) {
> +		*val = info->offset[chan->channel];
> +		return IIO_VAL_INT;
> +	}
> +
>  	return -EINVAL;
>  }
>  
> @@ -103,7 +125,9 @@ static const struct iio_info lpc32xx_adc_iio_info = {
>  	.type = IIO_VOLTAGE,				\
>  	.indexed = 1,					\
>  	.channel = _index,				\
> -	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT,	\
> +	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |	\
> +	             IIO_CHAN_INFO_SCALE_SEPARATE_BIT |	\
> +	             IIO_CHAN_INFO_OFFSET_SEPARATE_BIT,	\

Shouldn't here be tabs instead of spaces? You can check with
scripts/checkpatch.pl

>  	.address = AD_IN * _index,			\
>  	.scan_index = _index,				\
>  }
> @@ -149,6 +173,10 @@ static int __devinit lpc32xx_adc_probe(struct platform_device *pdev)
>  	}
>  
>  	info = iio_priv(iodev);
> +	of_property_read_u32_array(pdev->dev.of_node, "reference-voltages",
> +				   info->ref_voltage, ARRAY_SIZE(info->ref_voltage));

Considering that there is one line break already, maybe there should be
another one for keeping line length.

> +	of_property_read_u32_array(pdev->dev.of_node, "offsets",
> +				   info->offset, ARRAY_SIZE(info->offset));
>  
>  	info->adc_base = ioremap(res->start, resource_size(res));
>  	if (!info->adc_base) {

Re: [PATCH] staging:iio:lpc32xx_adc: Add scale/offset support

[Jonathan Cameron]

On 08/11/12 09:31, Roland Stigge wrote:
> Hi Alban,
>
> thanks for the patch. I can confirm that it compiles and runs fine, but
> maybe someone else can comment on the IIO part.
>
Why not do this with regulators?  That would give a more flexible
system and all that stuff is already well supported (if fixed voltages
are supplied then used fixed regulators).
> Some nitpicking below.
>
> On 07/11/12 17:44, Alban Bedel wrote:
>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de>
>> ---
>>   .../bindings/staging/iio/adc/lpc32xx-adc.txt       |    6 ++++
>>   drivers/staging/iio/adc/lpc32xx_adc.c              |   30 +++++++++++++++++++-
>>   2 files changed, 35 insertions(+), 1 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt b/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
>> index b3629d3..515c128 100644
>> --- a/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
>> +++ b/Documentation/devicetree/bindings/staging/iio/adc/lpc32xx-adc.txt
>> @@ -6,6 +6,11 @@ Required properties:
>>     region.
>>   - interrupts: The ADC interrupt
>>
>> +Optional properties:
>> +- reference-voltages: the 3 reference voltages value (in mV) to use
>> +  for scaling. If not given or 0 a scaling factor of 1 is used.
>> +- offsets: the 3 offsets to add to the raw value before scaling.
>> +
>>   Example:
>>
>>   	adc@40048000 {
>> @@ -13,4 +18,5 @@ Example:
>>   		reg = <0x40048000 0x1000>;
>>   		interrupt-parent = <&mic>;
>>   		interrupts = <39 0>;
>> +		reference-voltages = <5000>, <3300>, <1200>;
>>   	};
>> diff --git a/drivers/staging/iio/adc/lpc32xx_adc.c b/drivers/staging/iio/adc/lpc32xx_adc.c
>> index 10c5962..aaa23ac 100644
>> --- a/drivers/staging/iio/adc/lpc32xx_adc.c
>> +++ b/drivers/staging/iio/adc/lpc32xx_adc.c
>> @@ -64,6 +64,8 @@ struct lpc32xx_adc_info {
>>   	struct completion completion;
>>
>>   	u32 value;
>> +	u32 ref_voltage[3];
>> +	u32 offset[3];
>>   };
>>
>>   static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>> @@ -91,6 +93,26 @@ static int lpc32xx_read_raw(struct iio_dev *indio_dev,
>>   		return IIO_VAL_INT;
>>   	}
>>
>> +	if (mask == IIO_CHAN_INFO_SCALE) {
>> +		if (info->ref_voltage[chan->channel]) {
>> +			unsigned factor;
>> +			factor = div_u64((u64)info->ref_voltage[chan->channel] *
>> +					 (u64)1000000,
>> +					 ADC_VALUE_MASK);
>> +			*val = factor / 1000000;
>> +			*val2 = factor % 1000000;
>> +			return IIO_VAL_INT_PLUS_MICRO;
>> +		} else {
>> +			*val = 1;
>> +			return IIO_VAL_INT;
>> +		}
>> +	}
>> +
>> +	if (mask == IIO_CHAN_INFO_OFFSET) {
>> +		*val = info->offset[chan->channel];
>> +		return IIO_VAL_INT;
>> +	}
>> +
>>   	return -EINVAL;
>>   }
>>
>> @@ -103,7 +125,9 @@ static const struct iio_info lpc32xx_adc_iio_info = {
>>   	.type = IIO_VOLTAGE,				\
>>   	.indexed = 1,					\
>>   	.channel = _index,				\
>> -	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT,	\
>> +	.info_mask = IIO_CHAN_INFO_RAW_SEPARATE_BIT |	\
>> +	             IIO_CHAN_INFO_SCALE_SEPARATE_BIT |	\
>> +	             IIO_CHAN_INFO_OFFSET_SEPARATE_BIT,	\
>
> Shouldn't here be tabs instead of spaces? You can check with
> scripts/checkpatch.pl
>
>>   	.address = AD_IN * _index,			\
>>   	.scan_index = _index,				\
>>   }
>> @@ -149,6 +173,10 @@ static int __devinit lpc32xx_adc_probe(struct platform_device *pdev)
>>   	}
>>
>>   	info = iio_priv(iodev);
>> +	of_property_read_u32_array(pdev->dev.of_node, "reference-voltages",
>> +				   info->ref_voltage, ARRAY_SIZE(info->ref_voltage));
>
> Considering that there is one line break already, maybe there should be
> another one for keeping line length.
>
>> +	of_property_read_u32_array(pdev->dev.of_node, "offsets",
>> +				   info->offset, ARRAY_SIZE(info->offset));
>>
>>   	info->adc_base = ioremap(res->start, resource_size(res));
>>   	if (!info->adc_base) {
>
> --
> 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] staging:iio:lpc32xx_adc: Add scale/offset support

[Alban Bedel]

On Thu, 08 Nov 2012 15:38:01 +0000
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:

> Why not do this with regulators?  That would give a more flexible
> system and all that stuff is already well supported (if fixed voltages
> are supplied then used fixed regulators).

I agree that would probably be better. But for me that raise the
question of having this stuff in the ADC driver at all. Most ADC just
return a value from 0 to MAX and have no way to find out their supply
voltage. If we start with binding to regulators in the drivers we'll
soon get a lots of non-flexible and duplicated code.

It would be a lot nicer if the ADC channels could be bound to a
regulator and a transform function that compute a real world voltage out
of:
 - The ADC value
 - The ADC range
 - The regulator representing the ADC reference voltage

The simplest transform would be a linear curve with an optional offset,
but any kind of transform would also be possible allowing to represent
about any circuit that might be between the supply regulator and the ADC
input.

Alban

Re: [PATCH] staging:iio:lpc32xx_adc: Add scale/offset support

[Jonathan Cameron]

Alban Bedel <alban.bedel@avionic-design.de> wrote:

>On Thu, 08 Nov 2012 15:38:01 +0000
>Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
>
>> Why not do this with regulators?  That would give a more flexible
>> system and all that stuff is already well supported (if fixed
>voltages
>> are supplied then used fixed regulators).
>
>I agree that would probably be better. But for me that raise the
>question of having this stuff in the ADC driver at all. Most ADC just
>return a value from 0 to MAX and have no way to find out their supply
>voltage. 
A lot of iio drivers require a regulator for exactly this reason.  Either it is fixed and can go in device tree or it isn't and can be queried or set.


If we start with binding to regulators in the drivers we'll
>soon get a lots of non-flexible and duplicated code.
Why? Duplication maybe but I don't see where inflexibility comes from.. duplication may mean some library functions would be useful.

>
>It would be a lot nicer if the ADC channels could be bound to a
>regulator and a transform function that compute a real world voltage
>out
>of:
> - The ADC value
> - The ADC range
> - The regulator representing the ADC reference voltage
>
>The simplest transform would be a linear curve with an optional offset,
>but any kind of transform would also be possible allowing to represent
>about any circuit that might be between the supply regulator and the
>ADC
>input.
This still sits in the realm of regulators.  Far as iio driver is concerned there is a voltage on a pin.  What that means wrt to resulting scale is device dependent.  It is an obscure use case where the reference inputs are changing frequently..

Now I can see an arguement for a suitable regulator which takes one voltage in and does flexible transformations of it.  Also it can work the otherway with iio driver requesting a different reference value.

Now there may be a case for transforms on incoming adc channels as they are inherently dynamic.  Right now we promise only to give what the adc saw not some way before it on the otherside of weirdness.

So I am not yet convinced! Perhaps example configurations would help?

Remember that we deliberately leave as much data transforming to userspace as possible. So far the only non linear stuff has been slow so it hasn't mattered.

Jonathan
>
>Alban

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

Re: [PATCH] staging:iio:lpc32xx_adc: Add scale/offset support

[Alban Bedel]

On Thu, 08 Nov 2012 17:28:46 +0000
Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:

> 
> 
> Alban Bedel <alban.bedel@avionic-design.de> wrote:
> 
> >On Thu, 08 Nov 2012 15:38:01 +0000
> >Jonathan Cameron <jic23@jic23.retrosnub.co.uk> wrote:
> >
> >> Why not do this with regulators?  That would give a more flexible
> >> system and all that stuff is already well supported (if fixed
> >voltages
> >> are supplied then used fixed regulators).
> >
> >I agree that would probably be better. But for me that raise the
> >question of having this stuff in the ADC driver at all. Most ADC just
> >return a value from 0 to MAX and have no way to find out their supply
> >voltage.
>
> A lot of iio drivers require a regulator for exactly this reason.  Either
> it is fixed and can go in device tree or it isn't and can be queried
> or set. 
> 
> > If we start with binding to regulators in the drivers we'll soon get a
> > lots of non-flexible and duplicated code.

> Why? Duplication maybe but I don't see where inflexibility comes from..
> duplication may mean some library functions would be useful.

Inflexible because the drivers have to provide an information that they
inherently don't have, and the consumers can't use relative values as
they don't have access to the reference voltage.

> >It would be a lot nicer if the ADC channels could be bound to a
> >regulator and a transform function that compute a real world voltage
> >out
> >of:
> > - The ADC value
> > - The ADC range
> > - The regulator representing the ADC reference voltage
> >
> >The simplest transform would be a linear curve with an optional offset,
> >but any kind of transform would also be possible allowing to represent
> >about any circuit that might be between the supply regulator and the
> >ADC
> >input.

> This still sits in the realm of regulators.  Far as iio driver is concerned
> there is a voltage on a pin.  What that means wrt to resulting scale
> is device dependent.  It is an obscure use case where the reference
> inputs are changing frequently..

I was more thinking of an ADC measuring a value with a circuit that is
not linear. But I now see that such thing should be part of the
consumer not the ADC driver.

> Now I can see an arguement for a suitable regulator which takes one voltage in
> and does flexible transformations of it.  Also it can work the otherway with iio
> driver requesting a different reference value.

IMHO it would make much more sense if the ADC had nothing to do with
the reference voltage. The only thing an ADC can reliably provide is
the values it read and a function that convert a (value,ref_voltage)
pair to a voltage. Anything else is IMHO beyond the scope of an ADC and
shouldn't be part of the driver layer.

> Now there may be a case for transforms on incoming adc channels as they are
> inherently dynamic.  Right now we promise only to give what the adc saw not
> some way before it on the otherside of weirdness.
> 
> So I am not yet convinced! Perhaps example configurations would help?
> 
> Remember that we deliberately leave as much data transforming to userspace as
> possible. So far the only non linear stuff has been slow so it hasn't mattered.

That I fully understand, what I don't get is why the ADC drivers are
forced to provide an information they don't have.

Alban