[PATCH 0/2] iio: allow to set STM32 ADC resolution

[PATCH 0/2] iio: allow to set STM32 ADC resolution

[Fabrice Gasnier]

STM32 ADC supports several resolution. Add dt option so resolution
can be tuned at probe time. By default, maximum resolution is used
when it isn't set.

Fabrice Gasnier (2):
  dt-bindings: iio: stm32-adc: add option to set resolution
  iio: adc: stm32: add dt option to set resolution

 .../devicetree/bindings/iio/adc/st,stm32-adc.txt   |  4 ++
 drivers/iio/adc/stm32-adc.c                        | 50 +++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 0/2] iio: allow to set STM32 ADC resolution

[Fabrice Gasnier]

STM32 ADC supports several resolution. Add dt option so resolution
can be tuned at probe time. By default, maximum resolution is used
when it isn't set.

Fabrice Gasnier (2):
  dt-bindings: iio: stm32-adc: add option to set resolution
  iio: adc: stm32: add dt option to set resolution

 .../devicetree/bindings/iio/adc/st,stm32-adc.txt   |  4 ++
 drivers/iio/adc/stm32-adc.c                        | 50 +++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 0/2] iio: allow to set STM32 ADC resolution

[Fabrice Gasnier]

STM32 ADC supports several resolution. Add dt option so resolution
can be tuned at probe time. By default, maximum resolution is used
when it isn't set.

Fabrice Gasnier (2):
  dt-bindings: iio: stm32-adc: add option to set resolution
  iio: adc: stm32: add dt option to set resolution

 .../devicetree/bindings/iio/adc/st,stm32-adc.txt   |  4 ++
 drivers/iio/adc/stm32-adc.c                        | 50 +++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

Add documentation for 'st,adc-res' dt optional property.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
index 5dfc88e..45f7ff2 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
@@ -57,6 +57,9 @@ Optional properties:
 - dmas: Phandle to dma channel for this ADC instance.
   See ../../dma/dma.txt for details.
 - dma-names: Must be "rx" when dmas property is being used.
+- st,adc-res: Resolution (bits) to use for conversions. Must match device
+  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
+  is maximum resolution if unset.
 
 Example:
 	adc: adc@40012000 {
@@ -84,6 +87,7 @@ Example:
 			st,adc-channels = <8>;
 			dmas = <&dma2 0 0 0x400 0x0>;
 			dma-names = "rx";
+			st,adc-res = <8>;
 		};
 		...
 		other adc child nodes follow...
-- 
1.9.1

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

Add documentation for 'st,adc-res' dt optional property.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
index 5dfc88e..45f7ff2 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
@@ -57,6 +57,9 @@ Optional properties:
 - dmas: Phandle to dma channel for this ADC instance.
   See ../../dma/dma.txt for details.
 - dma-names: Must be "rx" when dmas property is being used.
+- st,adc-res: Resolution (bits) to use for conversions. Must match device
+  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
+  is maximum resolution if unset.
 
 Example:
 	adc: adc at 40012000 {
@@ -84,6 +87,7 @@ Example:
 			st,adc-channels = <8>;
 			dmas = <&dma2 0 0 0x400 0x0>;
 			dma-names = "rx";
+			st,adc-res = <8>;
 		};
 		...
 		other adc child nodes follow...
-- 
1.9.1

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

Add documentation for 'st,adc-res' dt optional property.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
index 5dfc88e..45f7ff2 100644
--- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
@@ -57,6 +57,9 @@ Optional properties:
 - dmas: Phandle to dma channel for this ADC instance.
   See ../../dma/dma.txt for details.
 - dma-names: Must be "rx" when dmas property is being used.
+- st,adc-res: Resolution (bits) to use for conversions. Must match device
+  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
+  is maximum resolution if unset.
 
 Example:
 	adc: adc@40012000 {
@@ -84,6 +87,7 @@ Example:
 			st,adc-channels = <8>;
 			dmas = <&dma2 0 0 0x400 0x0>;
 			dma-names = "rx";
+			st,adc-res = <8>;
 		};
 		...
 		other adc child nodes follow...
-- 
1.9.1

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 15/02/17 16:55, Fabrice Gasnier wrote:
> Add documentation for 'st,adc-res' dt optional property.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
I'm happy with this, but would like to leave time for a device tree review.

Ultimately we may well want to make this a generic property and call it something
like adc-resolution but perhaps we need to wait until we have a few more devices
supporting setting it via device tree to figure out what the best interface is.
It would exactly be a problem to support this as a deprecated binding at that
point.

Give me a poke if we hear nothing from Rob or Mark for say another week.

Thanks,

Jonathan

> ---
>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> index 5dfc88e..45f7ff2 100644
> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> @@ -57,6 +57,9 @@ Optional properties:
>  - dmas: Phandle to dma channel for this ADC instance.
>    See ../../dma/dma.txt for details.
>  - dma-names: Must be "rx" when dmas property is being used.
> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
> +  is maximum resolution if unset.
>  
>  Example:
>  	adc: adc@40012000 {
> @@ -84,6 +87,7 @@ Example:
>  			st,adc-channels = <8>;
>  			dmas = <&dma2 0 0 0x400 0x0>;
>  			dma-names = "rx";
> +			st,adc-res = <8>;
>  		};
>  		...
>  		other adc child nodes follow...
> 

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 15/02/17 16:55, Fabrice Gasnier wrote:
> Add documentation for 'st,adc-res' dt optional property.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
I'm happy with this, but would like to leave time for a device tree review.

Ultimately we may well want to make this a generic property and call it something
like adc-resolution but perhaps we need to wait until we have a few more devices
supporting setting it via device tree to figure out what the best interface is.
It would exactly be a problem to support this as a deprecated binding at that
point.

Give me a poke if we hear nothing from Rob or Mark for say another week.

Thanks,

Jonathan

> ---
>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> index 5dfc88e..45f7ff2 100644
> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> @@ -57,6 +57,9 @@ Optional properties:
>  - dmas: Phandle to dma channel for this ADC instance.
>    See ../../dma/dma.txt for details.
>  - dma-names: Must be "rx" when dmas property is being used.
> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
> +  is maximum resolution if unset.
>  
>  Example:
>  	adc: adc at 40012000 {
> @@ -84,6 +87,7 @@ Example:
>  			st,adc-channels = <8>;
>  			dmas = <&dma2 0 0 0x400 0x0>;
>  			dma-names = "rx";
> +			st,adc-res = <8>;
>  		};
>  		...
>  		other adc child nodes follow...
> 

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 15/02/17 16:55, Fabrice Gasnier wrote:
> Add documentation for 'st,adc-res' dt optional property.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
I'm happy with this, but would like to leave time for a device tree review.

Ultimately we may well want to make this a generic property and call it something
like adc-resolution but perhaps we need to wait until we have a few more devices
supporting setting it via device tree to figure out what the best interface is.
It would exactly be a problem to support this as a deprecated binding at that
point.

Give me a poke if we hear nothing from Rob or Mark for say another week.

Thanks,

Jonathan

> ---
>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> index 5dfc88e..45f7ff2 100644
> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
> @@ -57,6 +57,9 @@ Optional properties:
>  - dmas: Phandle to dma channel for this ADC instance.
>    See ../../dma/dma.txt for details.
>  - dma-names: Must be "rx" when dmas property is being used.
> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
> +  is maximum resolution if unset.
>  
>  Example:
>  	adc: adc@40012000 {
> @@ -84,6 +87,7 @@ Example:
>  			st,adc-channels = <8>;
>  			dmas = <&dma2 0 0 0x400 0x0>;
>  			dma-names = "rx";
> +			st,adc-res = <8>;
>  		};
>  		...
>  		other adc child nodes follow...
> 

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
> On 15/02/17 16:55, Fabrice Gasnier wrote:
>> Add documentation for 'st,adc-res' dt optional property.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> I'm happy with this, but would like to leave time for a device tree review.
>
> Ultimately we may well want to make this a generic property and call it something
> like adc-resolution but perhaps we need to wait until we have a few more devices
> supporting setting it via device tree to figure out what the best interface is.
> It would exactly be a problem to support this as a deprecated binding at that
> point.

Hi Jonathan,

I agree with you on this... It may be better to have generic property
for this, especially if you see that it will come in the near future.
May I suggest this prop to be less restrictive, e.g. like 
resolution-bits as is may also be worth for other device types, e.g. DAC 
as an example ?

>
> Give me a poke if we hear nothing from Rob or Mark for say another week.

No news yet, but it can wait a little longer.

Please advise,
Regards,
Fabrice

>
> Thanks,
>
> Jonathan
>
>> ---
>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> index 5dfc88e..45f7ff2 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> @@ -57,6 +57,9 @@ Optional properties:
>>  - dmas: Phandle to dma channel for this ADC instance.
>>    See ../../dma/dma.txt for details.
>>  - dma-names: Must be "rx" when dmas property is being used.
>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>> +  is maximum resolution if unset.
>>
>>  Example:
>>  	adc: adc@40012000 {
>> @@ -84,6 +87,7 @@ Example:
>>  			st,adc-channels = <8>;
>>  			dmas = <&dma2 0 0 0x400 0x0>;
>>  			dma-names = "rx";
>> +			st,adc-res = <8>;
>>  		};
>>  		...
>>  		other adc child nodes follow...
>>
>

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
> On 15/02/17 16:55, Fabrice Gasnier wrote:
>> Add documentation for 'st,adc-res' dt optional property.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> I'm happy with this, but would like to leave time for a device tree review.
>
> Ultimately we may well want to make this a generic property and call it something
> like adc-resolution but perhaps we need to wait until we have a few more devices
> supporting setting it via device tree to figure out what the best interface is.
> It would exactly be a problem to support this as a deprecated binding at that
> point.

Hi Jonathan,

I agree with you on this... It may be better to have generic property
for this, especially if you see that it will come in the near future.
May I suggest this prop to be less restrictive, e.g. like 
resolution-bits as is may also be worth for other device types, e.g. DAC 
as an example ?

>
> Give me a poke if we hear nothing from Rob or Mark for say another week.

No news yet, but it can wait a little longer.

Please advise,
Regards,
Fabrice

>
> Thanks,
>
> Jonathan
>
>> ---
>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> index 5dfc88e..45f7ff2 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> @@ -57,6 +57,9 @@ Optional properties:
>>  - dmas: Phandle to dma channel for this ADC instance.
>>    See ../../dma/dma.txt for details.
>>  - dma-names: Must be "rx" when dmas property is being used.
>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>> +  is maximum resolution if unset.
>>
>>  Example:
>>  	adc: adc at 40012000 {
>> @@ -84,6 +87,7 @@ Example:
>>  			st,adc-channels = <8>;
>>  			dmas = <&dma2 0 0 0x400 0x0>;
>>  			dma-names = "rx";
>> +			st,adc-res = <8>;
>>  		};
>>  		...
>>  		other adc child nodes follow...
>>
>

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
> On 15/02/17 16:55, Fabrice Gasnier wrote:
>> Add documentation for 'st,adc-res' dt optional property.
>>
>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> I'm happy with this, but would like to leave time for a device tree review.
>
> Ultimately we may well want to make this a generic property and call it something
> like adc-resolution but perhaps we need to wait until we have a few more devices
> supporting setting it via device tree to figure out what the best interface is.
> It would exactly be a problem to support this as a deprecated binding at that
> point.

Hi Jonathan,

I agree with you on this... It may be better to have generic property
for this, especially if you see that it will come in the near future.
May I suggest this prop to be less restrictive, e.g. like 
resolution-bits as is may also be worth for other device types, e.g. DAC 
as an example ?

>
> Give me a poke if we hear nothing from Rob or Mark for say another week.

No news yet, but it can wait a little longer.

Please advise,
Regards,
Fabrice

>
> Thanks,
>
> Jonathan
>
>> ---
>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> index 5dfc88e..45f7ff2 100644
>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>> @@ -57,6 +57,9 @@ Optional properties:
>>  - dmas: Phandle to dma channel for this ADC instance.
>>    See ../../dma/dma.txt for details.
>>  - dma-names: Must be "rx" when dmas property is being used.
>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>> +  is maximum resolution if unset.
>>
>>  Example:
>>  	adc: adc@40012000 {
>> @@ -84,6 +87,7 @@ Example:
>>  			st,adc-channels = <8>;
>>  			dmas = <&dma2 0 0 0x400 0x0>;
>>  			dma-names = "rx";
>> +			st,adc-res = <8>;
>>  		};
>>  		...
>>  		other adc child nodes follow...
>>
>

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 24/02/17 16:04, Fabrice Gasnier wrote:
> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>> Add documentation for 'st,adc-res' dt optional property.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> I'm happy with this, but would like to leave time for a device tree review.
>>
>> Ultimately we may well want to make this a generic property and call it something
>> like adc-resolution but perhaps we need to wait until we have a few more devices
>> supporting setting it via device tree to figure out what the best interface is.
>> It would exactly be a problem to support this as a deprecated binding at that
>> point.
> 
> Hi Jonathan,
> 
> I agree with you on this... It may be better to have generic property
> for this, especially if you see that it will come in the near future.
> May I suggest this prop to be less restrictive, e.g. like
> resolution-bits as is may also be worth for other device types, e.g.
> DAC as an example ?>
Sure, why not - no loss of meaning here.  We may never use it for anything
else but that doesn't matter.
>>
>> Give me a poke if we hear nothing from Rob or Mark for say another week.
> 
> No news yet, but it can wait a little longer.
Rob / Mark, can you take a look at this if you have time?

Thanks,

Jonathan
> 
> Please advise,
> Regards,
> Fabrice
> 
>>
>> Thanks,
>>
>> Jonathan
>>
>>> ---
>>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> index 5dfc88e..45f7ff2 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> @@ -57,6 +57,9 @@ Optional properties:
>>>  - dmas: Phandle to dma channel for this ADC instance.
>>>    See ../../dma/dma.txt for details.
>>>  - dma-names: Must be "rx" when dmas property is being used.
>>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>>> +  is maximum resolution if unset.
>>>
>>>  Example:
>>>      adc: adc@40012000 {
>>> @@ -84,6 +87,7 @@ Example:
>>>              st,adc-channels = <8>;
>>>              dmas = <&dma2 0 0 0x400 0x0>;
>>>              dma-names = "rx";
>>> +            st,adc-res = <8>;
>>>          };
>>>          ...
>>>          other adc child nodes follow...
>>>
>>
> -- 
> 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

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 24/02/17 16:04, Fabrice Gasnier wrote:
> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>> Add documentation for 'st,adc-res' dt optional property.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> I'm happy with this, but would like to leave time for a device tree review.
>>
>> Ultimately we may well want to make this a generic property and call it something
>> like adc-resolution but perhaps we need to wait until we have a few more devices
>> supporting setting it via device tree to figure out what the best interface is.
>> It would exactly be a problem to support this as a deprecated binding at that
>> point.
> 
> Hi Jonathan,
> 
> I agree with you on this... It may be better to have generic property
> for this, especially if you see that it will come in the near future.
> May I suggest this prop to be less restrictive, e.g. like
> resolution-bits as is may also be worth for other device types, e.g.
> DAC as an example ?>
Sure, why not - no loss of meaning here.  We may never use it for anything
else but that doesn't matter.
>>
>> Give me a poke if we hear nothing from Rob or Mark for say another week.
> 
> No news yet, but it can wait a little longer.
Rob / Mark, can you take a look at this if you have time?

Thanks,

Jonathan
> 
> Please advise,
> Regards,
> Fabrice
> 
>>
>> Thanks,
>>
>> Jonathan
>>
>>> ---
>>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> index 5dfc88e..45f7ff2 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> @@ -57,6 +57,9 @@ Optional properties:
>>>  - dmas: Phandle to dma channel for this ADC instance.
>>>    See ../../dma/dma.txt for details.
>>>  - dma-names: Must be "rx" when dmas property is being used.
>>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>>> +  is maximum resolution if unset.
>>>
>>>  Example:
>>>      adc: adc at 40012000 {
>>> @@ -84,6 +87,7 @@ Example:
>>>              st,adc-channels = <8>;
>>>              dmas = <&dma2 0 0 0x400 0x0>;
>>>              dma-names = "rx";
>>> +            st,adc-res = <8>;
>>>          };
>>>          ...
>>>          other adc child nodes follow...
>>>
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 24/02/17 16:04, Fabrice Gasnier wrote:
> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>> Add documentation for 'st,adc-res' dt optional property.
>>>
>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>> I'm happy with this, but would like to leave time for a device tree review.
>>
>> Ultimately we may well want to make this a generic property and call it something
>> like adc-resolution but perhaps we need to wait until we have a few more devices
>> supporting setting it via device tree to figure out what the best interface is.
>> It would exactly be a problem to support this as a deprecated binding at that
>> point.
> 
> Hi Jonathan,
> 
> I agree with you on this... It may be better to have generic property
> for this, especially if you see that it will come in the near future.
> May I suggest this prop to be less restrictive, e.g. like
> resolution-bits as is may also be worth for other device types, e.g.
> DAC as an example ?>
Sure, why not - no loss of meaning here.  We may never use it for anything
else but that doesn't matter.
>>
>> Give me a poke if we hear nothing from Rob or Mark for say another week.
> 
> No news yet, but it can wait a little longer.
Rob / Mark, can you take a look at this if you have time?

Thanks,

Jonathan
> 
> Please advise,
> Regards,
> Fabrice
> 
>>
>> Thanks,
>>
>> Jonathan
>>
>>> ---
>>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> index 5dfc88e..45f7ff2 100644
>>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>> @@ -57,6 +57,9 @@ Optional properties:
>>>  - dmas: Phandle to dma channel for this ADC instance.
>>>    See ../../dma/dma.txt for details.
>>>  - dma-names: Must be "rx" when dmas property is being used.
>>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>>> +  is maximum resolution if unset.
>>>
>>>  Example:
>>>      adc: adc@40012000 {
>>> @@ -84,6 +87,7 @@ Example:
>>>              st,adc-channels = <8>;
>>>              dmas = <&dma2 0 0 0x400 0x0>;
>>>              dma-names = "rx";
>>> +            st,adc-res = <8>;
>>>          };
>>>          ...
>>>          other adc child nodes follow...
>>>
>>
> -- 
> 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 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
> On 24/02/17 16:04, Fabrice Gasnier wrote:
>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>> I'm happy with this, but would like to leave time for a device tree review.
>>>
>>> Ultimately we may well want to make this a generic property and call it something
>>> like adc-resolution but perhaps we need to wait until we have a few more devices
>>> supporting setting it via device tree to figure out what the best interface is.
>>> It would exactly be a problem to support this as a deprecated binding at that
>>> point.
>>
>> Hi Jonathan,
>>
>> I agree with you on this... It may be better to have generic property
>> for this, especially if you see that it will come in the near future.
>> May I suggest this prop to be less restrictive, e.g. like
>> resolution-bits as is may also be worth for other device types, e.g.
>> DAC as an example ?>
> Sure, why not - no loss of meaning here.  We may never use it for anything
> else but that doesn't matter.

Hi Jonathan,

Following Rob's comment, how you see such a common property can be
integrated. Currently I don't see much devices are doing this.
Do you have ideas or preferences on this ?

- Do you think replacing 'st,adc-res' by 'resolution-bits' in current 
patchset is enough for now ?
- Or do I need to add dt parsing routine in IIO core, e.g. as simple
as of_iio_get_resolution(), or something like this ? Or parsing routine
that may fill in a common properties structure (but only resolution
will be there from the start)...

Please let me know.

Thanks,
Fabrice

>>>
>>> Give me a poke if we hear nothing from Rob or Mark for say another week.
>>
>> No news yet, but it can wait a little longer.
> Rob / Mark, can you take a look at this if you have time?
>
> Thanks,
>
> Jonathan
>>
>> Please advise,
>> Regards,
>> Fabrice
>>
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>
>>>> ---
>>>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> index 5dfc88e..45f7ff2 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> @@ -57,6 +57,9 @@ Optional properties:
>>>>  - dmas: Phandle to dma channel for this ADC instance.
>>>>    See ../../dma/dma.txt for details.
>>>>  - dma-names: Must be "rx" when dmas property is being used.
>>>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>>>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>>>> +  is maximum resolution if unset.
>>>>
>>>>  Example:
>>>>      adc: adc@40012000 {
>>>> @@ -84,6 +87,7 @@ Example:
>>>>              st,adc-channels = <8>;
>>>>              dmas = <&dma2 0 0 0x400 0x0>;
>>>>              dma-names = "rx";
>>>> +            st,adc-res = <8>;
>>>>          };
>>>>          ...
>>>>          other adc child nodes follow...
>>>>
>>>
>> --
>> 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
>

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
> On 24/02/17 16:04, Fabrice Gasnier wrote:
>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>> I'm happy with this, but would like to leave time for a device tree review.
>>>
>>> Ultimately we may well want to make this a generic property and call it something
>>> like adc-resolution but perhaps we need to wait until we have a few more devices
>>> supporting setting it via device tree to figure out what the best interface is.
>>> It would exactly be a problem to support this as a deprecated binding at that
>>> point.
>>
>> Hi Jonathan,
>>
>> I agree with you on this... It may be better to have generic property
>> for this, especially if you see that it will come in the near future.
>> May I suggest this prop to be less restrictive, e.g. like
>> resolution-bits as is may also be worth for other device types, e.g.
>> DAC as an example ?>
> Sure, why not - no loss of meaning here.  We may never use it for anything
> else but that doesn't matter.

Hi Jonathan,

Following Rob's comment, how you see such a common property can be
integrated. Currently I don't see much devices are doing this.
Do you have ideas or preferences on this ?

- Do you think replacing 'st,adc-res' by 'resolution-bits' in current 
patchset is enough for now ?
- Or do I need to add dt parsing routine in IIO core, e.g. as simple
as of_iio_get_resolution(), or something like this ? Or parsing routine
that may fill in a common properties structure (but only resolution
will be there from the start)...

Please let me know.

Thanks,
Fabrice

>>>
>>> Give me a poke if we hear nothing from Rob or Mark for say another week.
>>
>> No news yet, but it can wait a little longer.
> Rob / Mark, can you take a look at this if you have time?
>
> Thanks,
>
> Jonathan
>>
>> Please advise,
>> Regards,
>> Fabrice
>>
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>
>>>> ---
>>>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> index 5dfc88e..45f7ff2 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> @@ -57,6 +57,9 @@ Optional properties:
>>>>  - dmas: Phandle to dma channel for this ADC instance.
>>>>    See ../../dma/dma.txt for details.
>>>>  - dma-names: Must be "rx" when dmas property is being used.
>>>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>>>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>>>> +  is maximum resolution if unset.
>>>>
>>>>  Example:
>>>>      adc: adc at 40012000 {
>>>> @@ -84,6 +87,7 @@ Example:
>>>>              st,adc-channels = <8>;
>>>>              dmas = <&dma2 0 0 0x400 0x0>;
>>>>              dma-names = "rx";
>>>> +            st,adc-res = <8>;
>>>>          };
>>>>          ...
>>>>          other adc child nodes follow...
>>>>
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
> On 24/02/17 16:04, Fabrice Gasnier wrote:
>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>
>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>> I'm happy with this, but would like to leave time for a device tree review.
>>>
>>> Ultimately we may well want to make this a generic property and call it something
>>> like adc-resolution but perhaps we need to wait until we have a few more devices
>>> supporting setting it via device tree to figure out what the best interface is.
>>> It would exactly be a problem to support this as a deprecated binding at that
>>> point.
>>
>> Hi Jonathan,
>>
>> I agree with you on this... It may be better to have generic property
>> for this, especially if you see that it will come in the near future.
>> May I suggest this prop to be less restrictive, e.g. like
>> resolution-bits as is may also be worth for other device types, e.g.
>> DAC as an example ?>
> Sure, why not - no loss of meaning here.  We may never use it for anything
> else but that doesn't matter.

Hi Jonathan,

Following Rob's comment, how you see such a common property can be
integrated. Currently I don't see much devices are doing this.
Do you have ideas or preferences on this ?

- Do you think replacing 'st,adc-res' by 'resolution-bits' in current 
patchset is enough for now ?
- Or do I need to add dt parsing routine in IIO core, e.g. as simple
as of_iio_get_resolution(), or something like this ? Or parsing routine
that may fill in a common properties structure (but only resolution
will be there from the start)...

Please let me know.

Thanks,
Fabrice

>>>
>>> Give me a poke if we hear nothing from Rob or Mark for say another week.
>>
>> No news yet, but it can wait a little longer.
> Rob / Mark, can you take a look at this if you have time?
>
> Thanks,
>
> Jonathan
>>
>> Please advise,
>> Regards,
>> Fabrice
>>
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>
>>>> ---
>>>>  Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> index 5dfc88e..45f7ff2 100644
>>>> --- a/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/st,stm32-adc.txt
>>>> @@ -57,6 +57,9 @@ Optional properties:
>>>>  - dmas: Phandle to dma channel for this ADC instance.
>>>>    See ../../dma/dma.txt for details.
>>>>  - dma-names: Must be "rx" when dmas property is being used.
>>>> +- st,adc-res: Resolution (bits) to use for conversions. Must match device
>>>> +  available resolutions (e.g. can be 6, 8, 10 or 12 on stm32f4). Default
>>>> +  is maximum resolution if unset.
>>>>
>>>>  Example:
>>>>      adc: adc@40012000 {
>>>> @@ -84,6 +87,7 @@ Example:
>>>>              st,adc-channels = <8>;
>>>>              dmas = <&dma2 0 0 0x400 0x0>;
>>>>              dma-names = "rx";
>>>> +            st,adc-res = <8>;
>>>>          };
>>>>          ...
>>>>          other adc child nodes follow...
>>>>
>>>
>> --
>> 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 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Lars-Peter Clausen]

On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>
>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>
>>>> Ultimately we may well want to make this a generic property and call it
>>>> something
>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>> devices
>>>> supporting setting it via device tree to figure out what the best
>>>> interface is.
>>>> It would exactly be a problem to support this as a deprecated binding at
>>>> that
>>>> point.
>>>
>>> Hi Jonathan,
>>>
>>> I agree with you on this... It may be better to have generic property
>>> for this, especially if you see that it will come in the near future.
>>> May I suggest this prop to be less restrictive, e.g. like
>>> resolution-bits as is may also be worth for other device types, e.g.
>>> DAC as an example ?>
>> Sure, why not - no loss of meaning here.  We may never use it for anything
>> else but that doesn't matter.
> 
> Hi Jonathan,
> 
> Following Rob's comment, how you see such a common property can be
> integrated. Currently I don't see much devices are doing this.
> Do you have ideas or preferences on this ?
> 
> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
> patchset is enough for now ?

That name would be a bit confusing. Considering that the devicetree
describes th capabilities of the hardware this name would suggest that this
is the resolution supported by the device.

If we want to allow selecting runtime configuration parameters for IIO
devices from the devicetree we should probably follow the precedence set by
the clock framework and use properties with the 'assigned-' prefix.

But to be honest I'm not convinced yet that the whole approach of allowing
runtime configuration parameters to be configured via the devicetree is the
right approach. Converters and sensors often have a lot of runtime
configurable parameters, typically we expose them to the application layer
to allow maximum flexibility. Why is the resolution special in this case and
should be pre-configured via the devicetree instead?

- Lars

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Lars-Peter Clausen]

On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>
>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>
>>>> Ultimately we may well want to make this a generic property and call it
>>>> something
>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>> devices
>>>> supporting setting it via device tree to figure out what the best
>>>> interface is.
>>>> It would exactly be a problem to support this as a deprecated binding at
>>>> that
>>>> point.
>>>
>>> Hi Jonathan,
>>>
>>> I agree with you on this... It may be better to have generic property
>>> for this, especially if you see that it will come in the near future.
>>> May I suggest this prop to be less restrictive, e.g. like
>>> resolution-bits as is may also be worth for other device types, e.g.
>>> DAC as an example ?>
>> Sure, why not - no loss of meaning here.  We may never use it for anything
>> else but that doesn't matter.
> 
> Hi Jonathan,
> 
> Following Rob's comment, how you see such a common property can be
> integrated. Currently I don't see much devices are doing this.
> Do you have ideas or preferences on this ?
> 
> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
> patchset is enough for now ?

That name would be a bit confusing. Considering that the devicetree
describes th capabilities of the hardware this name would suggest that this
is the resolution supported by the device.

If we want to allow selecting runtime configuration parameters for IIO
devices from the devicetree we should probably follow the precedence set by
the clock framework and use properties with the 'assigned-' prefix.

But to be honest I'm not convinced yet that the whole approach of allowing
runtime configuration parameters to be configured via the devicetree is the
right approach. Converters and sensors often have a lot of runtime
configurable parameters, typically we expose them to the application layer
to allow maximum flexibility. Why is the resolution special in this case and
should be pre-configured via the devicetree instead?

- Lars

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Lars-Peter Clausen]

On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>
>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>
>>>> Ultimately we may well want to make this a generic property and call it
>>>> something
>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>> devices
>>>> supporting setting it via device tree to figure out what the best
>>>> interface is.
>>>> It would exactly be a problem to support this as a deprecated binding at
>>>> that
>>>> point.
>>>
>>> Hi Jonathan,
>>>
>>> I agree with you on this... It may be better to have generic property
>>> for this, especially if you see that it will come in the near future.
>>> May I suggest this prop to be less restrictive, e.g. like
>>> resolution-bits as is may also be worth for other device types, e.g.
>>> DAC as an example ?>
>> Sure, why not - no loss of meaning here.  We may never use it for anything
>> else but that doesn't matter.
> 
> Hi Jonathan,
> 
> Following Rob's comment, how you see such a common property can be
> integrated. Currently I don't see much devices are doing this.
> Do you have ideas or preferences on this ?
> 
> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
> patchset is enough for now ?

That name would be a bit confusing. Considering that the devicetree
describes th capabilities of the hardware this name would suggest that this
is the resolution supported by the device.

If we want to allow selecting runtime configuration parameters for IIO
devices from the devicetree we should probably follow the precedence set by
the clock framework and use properties with the 'assigned-' prefix.

But to be honest I'm not convinced yet that the whole approach of allowing
runtime configuration parameters to be configured via the devicetree is the
right approach. Converters and sensors often have a lot of runtime
configurable parameters, typically we expose them to the application layer
to allow maximum flexibility. Why is the resolution special in this case and
should be pre-configured via the devicetree instead?

- Lars

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/28/2017 09:34 AM, Lars-Peter Clausen wrote:
> On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
>> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>>
>>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>>
>>>>> Ultimately we may well want to make this a generic property and call it
>>>>> something
>>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>>> devices
>>>>> supporting setting it via device tree to figure out what the best
>>>>> interface is.
>>>>> It would exactly be a problem to support this as a deprecated binding at
>>>>> that
>>>>> point.
>>>>
>>>> Hi Jonathan,
>>>>
>>>> I agree with you on this... It may be better to have generic property
>>>> for this, especially if you see that it will come in the near future.
>>>> May I suggest this prop to be less restrictive, e.g. like
>>>> resolution-bits as is may also be worth for other device types, e.g.
>>>> DAC as an example ?>
>>> Sure, why not - no loss of meaning here.  We may never use it for anything
>>> else but that doesn't matter.
>>
>> Hi Jonathan,
>>
>> Following Rob's comment, how you see such a common property can be
>> integrated. Currently I don't see much devices are doing this.
>> Do you have ideas or preferences on this ?
>>
>> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
>> patchset is enough for now ?
>
> That name would be a bit confusing. Considering that the devicetree
> describes th capabilities of the hardware this name would suggest that this
> is the resolution supported by the device.
>
> If we want to allow selecting runtime configuration parameters for IIO
> devices from the devicetree we should probably follow the precedence set by
> the clock framework and use properties with the 'assigned-' prefix.

Hi Lars,

This makes sense. Then, do you see 'assigned-resolution-bits' as a
better candidate ?
Also, do you think listing all devices caps in property like 
'resolutions-bits', e.g. 'resolutions-bits = <6 8 10 12>', is
suitable/necessary ?

>
> But to be honest I'm not convinced yet that the whole approach of allowing
> runtime configuration parameters to be configured via the devicetree is the
> right approach. Converters and sensors often have a lot of runtime
> configurable parameters, typically we expose them to the application layer
> to allow maximum flexibility. Why is the resolution special in this case and
> should be pre-configured via the devicetree instead?

As I understand it, currently lots of devices have hard-coded
(maximum?) resolution, then exposed to application layer as read only 
data format. Then resolution depends on HW used (e.g. sensor, 
converter). Depending on the application board, it may be worth to offer
pre-configured intermediate/lower resolution depending on analog
sources. Is it ok to offer devicetree pre-configuration ?

Best Regards,
Fabrice
>
> - Lars
>

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/28/2017 09:34 AM, Lars-Peter Clausen wrote:
> On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
>> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>>
>>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>>
>>>>> Ultimately we may well want to make this a generic property and call it
>>>>> something
>>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>>> devices
>>>>> supporting setting it via device tree to figure out what the best
>>>>> interface is.
>>>>> It would exactly be a problem to support this as a deprecated binding at
>>>>> that
>>>>> point.
>>>>
>>>> Hi Jonathan,
>>>>
>>>> I agree with you on this... It may be better to have generic property
>>>> for this, especially if you see that it will come in the near future.
>>>> May I suggest this prop to be less restrictive, e.g. like
>>>> resolution-bits as is may also be worth for other device types, e.g.
>>>> DAC as an example ?>
>>> Sure, why not - no loss of meaning here.  We may never use it for anything
>>> else but that doesn't matter.
>>
>> Hi Jonathan,
>>
>> Following Rob's comment, how you see such a common property can be
>> integrated. Currently I don't see much devices are doing this.
>> Do you have ideas or preferences on this ?
>>
>> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
>> patchset is enough for now ?
>
> That name would be a bit confusing. Considering that the devicetree
> describes th capabilities of the hardware this name would suggest that this
> is the resolution supported by the device.
>
> If we want to allow selecting runtime configuration parameters for IIO
> devices from the devicetree we should probably follow the precedence set by
> the clock framework and use properties with the 'assigned-' prefix.

Hi Lars,

This makes sense. Then, do you see 'assigned-resolution-bits' as a
better candidate ?
Also, do you think listing all devices caps in property like 
'resolutions-bits', e.g. 'resolutions-bits = <6 8 10 12>', is
suitable/necessary ?

>
> But to be honest I'm not convinced yet that the whole approach of allowing
> runtime configuration parameters to be configured via the devicetree is the
> right approach. Converters and sensors often have a lot of runtime
> configurable parameters, typically we expose them to the application layer
> to allow maximum flexibility. Why is the resolution special in this case and
> should be pre-configured via the devicetree instead?

As I understand it, currently lots of devices have hard-coded
(maximum?) resolution, then exposed to application layer as read only 
data format. Then resolution depends on HW used (e.g. sensor, 
converter). Depending on the application board, it may be worth to offer
pre-configured intermediate/lower resolution depending on analog
sources. Is it ok to offer devicetree pre-configuration ?

Best Regards,
Fabrice
>
> - Lars
>

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Fabrice Gasnier]

On 02/28/2017 09:34 AM, Lars-Peter Clausen wrote:
> On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
>> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>>
>>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>>
>>>>> Ultimately we may well want to make this a generic property and call it
>>>>> something
>>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>>> devices
>>>>> supporting setting it via device tree to figure out what the best
>>>>> interface is.
>>>>> It would exactly be a problem to support this as a deprecated binding at
>>>>> that
>>>>> point.
>>>>
>>>> Hi Jonathan,
>>>>
>>>> I agree with you on this... It may be better to have generic property
>>>> for this, especially if you see that it will come in the near future.
>>>> May I suggest this prop to be less restrictive, e.g. like
>>>> resolution-bits as is may also be worth for other device types, e.g.
>>>> DAC as an example ?>
>>> Sure, why not - no loss of meaning here.  We may never use it for anything
>>> else but that doesn't matter.
>>
>> Hi Jonathan,
>>
>> Following Rob's comment, how you see such a common property can be
>> integrated. Currently I don't see much devices are doing this.
>> Do you have ideas or preferences on this ?
>>
>> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
>> patchset is enough for now ?
>
> That name would be a bit confusing. Considering that the devicetree
> describes th capabilities of the hardware this name would suggest that this
> is the resolution supported by the device.
>
> If we want to allow selecting runtime configuration parameters for IIO
> devices from the devicetree we should probably follow the precedence set by
> the clock framework and use properties with the 'assigned-' prefix.

Hi Lars,

This makes sense. Then, do you see 'assigned-resolution-bits' as a
better candidate ?
Also, do you think listing all devices caps in property like 
'resolutions-bits', e.g. 'resolutions-bits = <6 8 10 12>', is
suitable/necessary ?

>
> But to be honest I'm not convinced yet that the whole approach of allowing
> runtime configuration parameters to be configured via the devicetree is the
> right approach. Converters and sensors often have a lot of runtime
> configurable parameters, typically we expose them to the application layer
> to allow maximum flexibility. Why is the resolution special in this case and
> should be pre-configured via the devicetree instead?

As I understand it, currently lots of devices have hard-coded
(maximum?) resolution, then exposed to application layer as read only 
data format. Then resolution depends on HW used (e.g. sensor, 
converter). Depending on the application board, it may be worth to offer
pre-configured intermediate/lower resolution depending on analog
sources. Is it ok to offer devicetree pre-configuration ?

Best Regards,
Fabrice
>
> - Lars
>

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 28/02/17 10:14, Fabrice Gasnier wrote:
> On 02/28/2017 09:34 AM, Lars-Peter Clausen wrote:
>> On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
>>> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>>>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>>>
>>>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>>>
>>>>>> Ultimately we may well want to make this a generic property and call it
>>>>>> something
>>>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>>>> devices
>>>>>> supporting setting it via device tree to figure out what the best
>>>>>> interface is.
>>>>>> It would exactly be a problem to support this as a deprecated binding at
>>>>>> that
>>>>>> point.
>>>>>
>>>>> Hi Jonathan,
>>>>>
>>>>> I agree with you on this... It may be better to have generic property
>>>>> for this, especially if you see that it will come in the near future.
>>>>> May I suggest this prop to be less restrictive, e.g. like
>>>>> resolution-bits as is may also be worth for other device types, e.g.
>>>>> DAC as an example ?>
>>>> Sure, why not - no loss of meaning here.  We may never use it for anything
>>>> else but that doesn't matter.
>>>
>>> Hi Jonathan,
>>>
>>> Following Rob's comment, how you see such a common property can be
>>> integrated. Currently I don't see much devices are doing this.
>>> Do you have ideas or preferences on this ?
>>>
>>> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
>>> patchset is enough for now ?
>>
>> That name would be a bit confusing. Considering that the devicetree
>> describes th capabilities of the hardware this name would suggest that this
>> is the resolution supported by the device.
>>
>> If we want to allow selecting runtime configuration parameters for IIO
>> devices from the devicetree we should probably follow the precedence set by
>> the clock framework and use properties with the 'assigned-' prefix.
> 
> Hi Lars,
> 
> This makes sense. Then, do you see 'assigned-resolution-bits' as a
> better candidate ?
> Also, do you think listing all devices caps in property like 'resolutions-bits', e.g. 'resolutions-bits = <6 8 10 12>', is
> suitable/necessary ?
Only suitable / necessary if what the options are is dependent on the hardware
in some way that is not adequately described by the compatibility element.
> 
>>
>> But to be honest I'm not convinced yet that the whole approach of allowing
>> runtime configuration parameters to be configured via the devicetree is the
>> right approach. Converters and sensors often have a lot of runtime
>> configurable parameters, typically we expose them to the application layer
>> to allow maximum flexibility. Why is the resolution special in this case and
>> should be pre-configured via the devicetree instead?
> 
To be cynical because it is PITA to change the assumption of it being
fixed :)  Oh well, another design decision that we got wrong a long time
back.

> As I understand it, currently lots of devices have hard-coded 
> (maximum?) resolution, then exposed to application layer as read only
> data format. Then resolution depends on HW used (e.g. sensor,
> converter). Depending on the application board, it may be worth to
> offer pre-configured intermediate/lower resolution depending on
> analog sources. Is it ok to offer devicetree pre-configuration ?
> 
Hmm.  It is in theory acceptable to put default values that make sense
for a given set of hardware in device tree.  They are kind of a 
'well this will work' state.  Make sense if there are states that
don't make sense for a given hardware setup.

If all states make equally good sense, then perhaps we should look at
fixing up the IIO core to handle variable resolution devices.
That strikes me as a non trivial exercise, but could probably be
done.  We'd need to keep the simplicity of the current code
for the vast majority of devices, but provide some relatively clean
way of changing it when needed.

Whilst I'm happy if someone else wants to look at this, it is
nowhere near the top of my list!

Jonathan
> Best Regards,
> Fabrice
>>
>> - Lars
>>
> -- 
> 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

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 28/02/17 10:14, Fabrice Gasnier wrote:
> On 02/28/2017 09:34 AM, Lars-Peter Clausen wrote:
>> On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
>>> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>>>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>>>
>>>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
>>>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>>>
>>>>>> Ultimately we may well want to make this a generic property and call it
>>>>>> something
>>>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>>>> devices
>>>>>> supporting setting it via device tree to figure out what the best
>>>>>> interface is.
>>>>>> It would exactly be a problem to support this as a deprecated binding at
>>>>>> that
>>>>>> point.
>>>>>
>>>>> Hi Jonathan,
>>>>>
>>>>> I agree with you on this... It may be better to have generic property
>>>>> for this, especially if you see that it will come in the near future.
>>>>> May I suggest this prop to be less restrictive, e.g. like
>>>>> resolution-bits as is may also be worth for other device types, e.g.
>>>>> DAC as an example ?>
>>>> Sure, why not - no loss of meaning here.  We may never use it for anything
>>>> else but that doesn't matter.
>>>
>>> Hi Jonathan,
>>>
>>> Following Rob's comment, how you see such a common property can be
>>> integrated. Currently I don't see much devices are doing this.
>>> Do you have ideas or preferences on this ?
>>>
>>> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
>>> patchset is enough for now ?
>>
>> That name would be a bit confusing. Considering that the devicetree
>> describes th capabilities of the hardware this name would suggest that this
>> is the resolution supported by the device.
>>
>> If we want to allow selecting runtime configuration parameters for IIO
>> devices from the devicetree we should probably follow the precedence set by
>> the clock framework and use properties with the 'assigned-' prefix.
> 
> Hi Lars,
> 
> This makes sense. Then, do you see 'assigned-resolution-bits' as a
> better candidate ?
> Also, do you think listing all devices caps in property like 'resolutions-bits', e.g. 'resolutions-bits = <6 8 10 12>', is
> suitable/necessary ?
Only suitable / necessary if what the options are is dependent on the hardware
in some way that is not adequately described by the compatibility element.
> 
>>
>> But to be honest I'm not convinced yet that the whole approach of allowing
>> runtime configuration parameters to be configured via the devicetree is the
>> right approach. Converters and sensors often have a lot of runtime
>> configurable parameters, typically we expose them to the application layer
>> to allow maximum flexibility. Why is the resolution special in this case and
>> should be pre-configured via the devicetree instead?
> 
To be cynical because it is PITA to change the assumption of it being
fixed :)  Oh well, another design decision that we got wrong a long time
back.

> As I understand it, currently lots of devices have hard-coded 
> (maximum?) resolution, then exposed to application layer as read only
> data format. Then resolution depends on HW used (e.g. sensor,
> converter). Depending on the application board, it may be worth to
> offer pre-configured intermediate/lower resolution depending on
> analog sources. Is it ok to offer devicetree pre-configuration ?
> 
Hmm.  It is in theory acceptable to put default values that make sense
for a given set of hardware in device tree.  They are kind of a 
'well this will work' state.  Make sense if there are states that
don't make sense for a given hardware setup.

If all states make equally good sense, then perhaps we should look at
fixing up the IIO core to handle variable resolution devices.
That strikes me as a non trivial exercise, but could probably be
done.  We'd need to keep the simplicity of the current code
for the vast majority of devices, but provide some relatively clean
way of changing it when needed.

Whilst I'm happy if someone else wants to look at this, it is
nowhere near the top of my list!

Jonathan
> Best Regards,
> Fabrice
>>
>> - Lars
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Jonathan Cameron]

On 28/02/17 10:14, Fabrice Gasnier wrote:
> On 02/28/2017 09:34 AM, Lars-Peter Clausen wrote:
>> On 02/28/2017 09:21 AM, Fabrice Gasnier wrote:
>>> On 02/25/2017 04:11 PM, Jonathan Cameron wrote:
>>>> On 24/02/17 16:04, Fabrice Gasnier wrote:
>>>>> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
>>>>>> On 15/02/17 16:55, Fabrice Gasnier wrote:
>>>>>>> Add documentation for 'st,adc-res' dt optional property.
>>>>>>>
>>>>>>> Signed-off-by: Fabrice Gasnier <fabrice.gasnier-qxv4g6HH51o@public.gmane.org>
>>>>>> I'm happy with this, but would like to leave time for a device tree review.
>>>>>>
>>>>>> Ultimately we may well want to make this a generic property and call it
>>>>>> something
>>>>>> like adc-resolution but perhaps we need to wait until we have a few more
>>>>>> devices
>>>>>> supporting setting it via device tree to figure out what the best
>>>>>> interface is.
>>>>>> It would exactly be a problem to support this as a deprecated binding at
>>>>>> that
>>>>>> point.
>>>>>
>>>>> Hi Jonathan,
>>>>>
>>>>> I agree with you on this... It may be better to have generic property
>>>>> for this, especially if you see that it will come in the near future.
>>>>> May I suggest this prop to be less restrictive, e.g. like
>>>>> resolution-bits as is may also be worth for other device types, e.g.
>>>>> DAC as an example ?>
>>>> Sure, why not - no loss of meaning here.  We may never use it for anything
>>>> else but that doesn't matter.
>>>
>>> Hi Jonathan,
>>>
>>> Following Rob's comment, how you see such a common property can be
>>> integrated. Currently I don't see much devices are doing this.
>>> Do you have ideas or preferences on this ?
>>>
>>> - Do you think replacing 'st,adc-res' by 'resolution-bits' in current
>>> patchset is enough for now ?
>>
>> That name would be a bit confusing. Considering that the devicetree
>> describes th capabilities of the hardware this name would suggest that this
>> is the resolution supported by the device.
>>
>> If we want to allow selecting runtime configuration parameters for IIO
>> devices from the devicetree we should probably follow the precedence set by
>> the clock framework and use properties with the 'assigned-' prefix.
> 
> Hi Lars,
> 
> This makes sense. Then, do you see 'assigned-resolution-bits' as a
> better candidate ?
> Also, do you think listing all devices caps in property like 'resolutions-bits', e.g. 'resolutions-bits = <6 8 10 12>', is
> suitable/necessary ?
Only suitable / necessary if what the options are is dependent on the hardware
in some way that is not adequately described by the compatibility element.
> 
>>
>> But to be honest I'm not convinced yet that the whole approach of allowing
>> runtime configuration parameters to be configured via the devicetree is the
>> right approach. Converters and sensors often have a lot of runtime
>> configurable parameters, typically we expose them to the application layer
>> to allow maximum flexibility. Why is the resolution special in this case and
>> should be pre-configured via the devicetree instead?
> 
To be cynical because it is PITA to change the assumption of it being
fixed :)  Oh well, another design decision that we got wrong a long time
back.

> As I understand it, currently lots of devices have hard-coded 
> (maximum?) resolution, then exposed to application layer as read only
> data format. Then resolution depends on HW used (e.g. sensor,
> converter). Depending on the application board, it may be worth to
> offer pre-configured intermediate/lower resolution depending on
> analog sources. Is it ok to offer devicetree pre-configuration ?
> 
Hmm.  It is in theory acceptable to put default values that make sense
for a given set of hardware in device tree.  They are kind of a 
'well this will work' state.  Make sense if there are states that
don't make sense for a given hardware setup.

If all states make equally good sense, then perhaps we should look at
fixing up the IIO core to handle variable resolution devices.
That strikes me as a non trivial exercise, but could probably be
done.  We'd need to keep the simplicity of the current code
for the vast majority of devices, but provide some relatively clean
way of changing it when needed.

Whilst I'm happy if someone else wants to look at this, it is
nowhere near the top of my list!

Jonathan
> Best Regards,
> Fabrice
>>
>> - Lars
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Rob Herring]

On Fri, Feb 24, 2017 at 05:04:45PM +0100, Fabrice Gasnier wrote:
> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
> > On 15/02/17 16:55, Fabrice Gasnier wrote:
> > > Add documentation for 'st,adc-res' dt optional property.
> > > 
> > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > I'm happy with this, but would like to leave time for a device tree review.
> > 
> > Ultimately we may well want to make this a generic property and call it something
> > like adc-resolution but perhaps we need to wait until we have a few more devices
> > supporting setting it via device tree to figure out what the best interface is.
> > It would exactly be a problem to support this as a deprecated binding at that
> > point.
> 
> Hi Jonathan,
> 
> I agree with you on this... It may be better to have generic property
> for this, especially if you see that it will come in the near future.
> May I suggest this prop to be less restrictive, e.g. like resolution-bits as
> is may also be worth for other device types, e.g. DAC as an example ?

Yes, please make this a commmon property.

Rob

[PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Rob Herring]

On Fri, Feb 24, 2017 at 05:04:45PM +0100, Fabrice Gasnier wrote:
> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
> > On 15/02/17 16:55, Fabrice Gasnier wrote:
> > > Add documentation for 'st,adc-res' dt optional property.
> > > 
> > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > I'm happy with this, but would like to leave time for a device tree review.
> > 
> > Ultimately we may well want to make this a generic property and call it something
> > like adc-resolution but perhaps we need to wait until we have a few more devices
> > supporting setting it via device tree to figure out what the best interface is.
> > It would exactly be a problem to support this as a deprecated binding at that
> > point.
> 
> Hi Jonathan,
> 
> I agree with you on this... It may be better to have generic property
> for this, especially if you see that it will come in the near future.
> May I suggest this prop to be less restrictive, e.g. like resolution-bits as
> is may also be worth for other device types, e.g. DAC as an example ?

Yes, please make this a commmon property.

Rob

Re: [PATCH 1/2] dt-bindings: iio: stm32-adc: add option to set resolution

[Rob Herring]

On Fri, Feb 24, 2017 at 05:04:45PM +0100, Fabrice Gasnier wrote:
> On 02/19/2017 01:09 PM, Jonathan Cameron wrote:
> > On 15/02/17 16:55, Fabrice Gasnier wrote:
> > > Add documentation for 'st,adc-res' dt optional property.
> > > 
> > > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
> > I'm happy with this, but would like to leave time for a device tree review.
> > 
> > Ultimately we may well want to make this a generic property and call it something
> > like adc-resolution but perhaps we need to wait until we have a few more devices
> > supporting setting it via device tree to figure out what the best interface is.
> > It would exactly be a problem to support this as a deprecated binding at that
> > point.
> 
> Hi Jonathan,
> 
> I agree with you on this... It may be better to have generic property
> for this, especially if you see that it will come in the near future.
> May I suggest this prop to be less restrictive, e.g. like resolution-bits as
> is may also be worth for other device types, e.g. DAC as an example ?

Yes, please make this a commmon property.

Rob

[PATCH 2/2] iio: adc: stm32: add dt option to set resolution

[Fabrice Gasnier]

stm32 adc supports several resolution. Add 'st,adc-res' dt optional
property to set it. Default to maximum resolution in case it isn't set.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/iio/adc/stm32-adc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 9b49a6ad..268b457a 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -60,6 +60,8 @@
 #define STM32F4_EOC			BIT(1)
 
 /* STM32F4_ADC_CR1 - bit fields */
+#define STM32F4_RES_SHIFT		24
+#define STM32F4_RES_MASK		GENMASK(25, 24)
 #define STM32F4_SCAN			BIT(8)
 #define STM32F4_EOCIE			BIT(5)
 
@@ -141,6 +143,7 @@ struct stm32_adc_regs {
  * @lock:		spinlock
  * @bufi:		data buffer index
  * @num_conv:		expected number of scan conversions
+ * @res:		data resolution (e.g. RES bitfield value)
  * @trigger_polarity:	external trigger polarity (e.g. exten)
  * @dma_chan:		dma channel
  * @rx_buf:		dma rx buffer cpu address
@@ -157,6 +160,7 @@ struct stm32_adc {
 	spinlock_t		lock;		/* interrupt lock */
 	unsigned int		bufi;
 	unsigned int		num_conv;
+	u32			res;
 	u32			trigger_polarity;
 	struct dma_chan		*dma_chan;
 	u8			*rx_buf;
@@ -196,6 +200,11 @@ struct stm32_adc_chan_spec {
 	{ IIO_VOLTAGE, 15, "in15" },
 };
 
+static const unsigned int stm32f4_adc_resolutions[] = {
+	/* sorted values so the index matches RES[1:0] in STM32F4_ADC_CR1 */
+	12, 10, 8, 6,
+};
+
 /**
  * stm32f4_sq - describe regular sequence registers
  * - L: sequence len (register & bit field)
@@ -302,6 +311,14 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
 	stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_EOCIE);
 }
 
+static void stm32_adc_set_res(struct stm32_adc *adc)
+{
+	u32 val = stm32_adc_readl(adc, STM32F4_ADC_CR1);
+
+	val = (val & ~STM32F4_RES_MASK) | (adc->res << STM32F4_RES_SHIFT);
+	stm32_adc_writel(adc, STM32F4_ADC_CR1, val);
+}
+
 /**
  * stm32_adc_start_conv() - Start conversions for regular channels.
  * @adc: stm32 adc instance
@@ -870,11 +887,37 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
 	{},
 };
 
+static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
+{
+	struct device_node *node = indio_dev->dev.of_node;
+	struct stm32_adc *adc = iio_priv(indio_dev);
+	unsigned int i;
+	u32 res;
+
+	if (of_property_read_u32(node, "st,adc-res", &res))
+		res = stm32f4_adc_resolutions[0];
+
+	for (i = 0; i < ARRAY_SIZE(stm32f4_adc_resolutions); i++)
+		if (res == stm32f4_adc_resolutions[i])
+			break;
+	if (i >= ARRAY_SIZE(stm32f4_adc_resolutions)) {
+		dev_err(&indio_dev->dev, "Bad resolution: %u bits\n", res);
+		return -EINVAL;
+	}
+
+	dev_dbg(&indio_dev->dev, "Using %u bits resolution\n", res);
+	adc->res = i;
+
+	return 0;
+}
+
 static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
 				    struct iio_chan_spec *chan,
 				    const struct stm32_adc_chan_spec *channel,
 				    int scan_index)
 {
+	struct stm32_adc *adc = iio_priv(indio_dev);
+
 	chan->type = channel->type;
 	chan->channel = channel->channel;
 	chan->datasheet_name = channel->name;
@@ -883,7 +926,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
 	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
 	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
 	chan->scan_type.sign = 'u';
-	chan->scan_type.realbits = 12;
+	chan->scan_type.realbits = stm32f4_adc_resolutions[adc->res];
 	chan->scan_type.storagebits = 16;
 	chan->ext_info = stm32_adc_ext_info;
 }
@@ -1022,6 +1065,11 @@ static int stm32_adc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = stm32_adc_of_get_resolution(indio_dev);
+	if (ret < 0)
+		goto err_clk_disable;
+	stm32_adc_set_res(adc);
+
 	ret = stm32_adc_chan_of_init(indio_dev);
 	if (ret < 0)
 		goto err_clk_disable;
-- 
1.9.1

[PATCH 2/2] iio: adc: stm32: add dt option to set resolution

[Fabrice Gasnier]

stm32 adc supports several resolution. Add 'st,adc-res' dt optional
property to set it. Default to maximum resolution in case it isn't set.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/iio/adc/stm32-adc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 9b49a6ad..268b457a 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -60,6 +60,8 @@
 #define STM32F4_EOC			BIT(1)
 
 /* STM32F4_ADC_CR1 - bit fields */
+#define STM32F4_RES_SHIFT		24
+#define STM32F4_RES_MASK		GENMASK(25, 24)
 #define STM32F4_SCAN			BIT(8)
 #define STM32F4_EOCIE			BIT(5)
 
@@ -141,6 +143,7 @@ struct stm32_adc_regs {
  * @lock:		spinlock
  * @bufi:		data buffer index
  * @num_conv:		expected number of scan conversions
+ * @res:		data resolution (e.g. RES bitfield value)
  * @trigger_polarity:	external trigger polarity (e.g. exten)
  * @dma_chan:		dma channel
  * @rx_buf:		dma rx buffer cpu address
@@ -157,6 +160,7 @@ struct stm32_adc {
 	spinlock_t		lock;		/* interrupt lock */
 	unsigned int		bufi;
 	unsigned int		num_conv;
+	u32			res;
 	u32			trigger_polarity;
 	struct dma_chan		*dma_chan;
 	u8			*rx_buf;
@@ -196,6 +200,11 @@ struct stm32_adc_chan_spec {
 	{ IIO_VOLTAGE, 15, "in15" },
 };
 
+static const unsigned int stm32f4_adc_resolutions[] = {
+	/* sorted values so the index matches RES[1:0] in STM32F4_ADC_CR1 */
+	12, 10, 8, 6,
+};
+
 /**
  * stm32f4_sq - describe regular sequence registers
  * - L: sequence len (register & bit field)
@@ -302,6 +311,14 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
 	stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_EOCIE);
 }
 
+static void stm32_adc_set_res(struct stm32_adc *adc)
+{
+	u32 val = stm32_adc_readl(adc, STM32F4_ADC_CR1);
+
+	val = (val & ~STM32F4_RES_MASK) | (adc->res << STM32F4_RES_SHIFT);
+	stm32_adc_writel(adc, STM32F4_ADC_CR1, val);
+}
+
 /**
  * stm32_adc_start_conv() - Start conversions for regular channels.
  * @adc: stm32 adc instance
@@ -870,11 +887,37 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
 	{},
 };
 
+static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
+{
+	struct device_node *node = indio_dev->dev.of_node;
+	struct stm32_adc *adc = iio_priv(indio_dev);
+	unsigned int i;
+	u32 res;
+
+	if (of_property_read_u32(node, "st,adc-res", &res))
+		res = stm32f4_adc_resolutions[0];
+
+	for (i = 0; i < ARRAY_SIZE(stm32f4_adc_resolutions); i++)
+		if (res == stm32f4_adc_resolutions[i])
+			break;
+	if (i >= ARRAY_SIZE(stm32f4_adc_resolutions)) {
+		dev_err(&indio_dev->dev, "Bad resolution: %u bits\n", res);
+		return -EINVAL;
+	}
+
+	dev_dbg(&indio_dev->dev, "Using %u bits resolution\n", res);
+	adc->res = i;
+
+	return 0;
+}
+
 static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
 				    struct iio_chan_spec *chan,
 				    const struct stm32_adc_chan_spec *channel,
 				    int scan_index)
 {
+	struct stm32_adc *adc = iio_priv(indio_dev);
+
 	chan->type = channel->type;
 	chan->channel = channel->channel;
 	chan->datasheet_name = channel->name;
@@ -883,7 +926,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
 	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
 	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
 	chan->scan_type.sign = 'u';
-	chan->scan_type.realbits = 12;
+	chan->scan_type.realbits = stm32f4_adc_resolutions[adc->res];
 	chan->scan_type.storagebits = 16;
 	chan->ext_info = stm32_adc_ext_info;
 }
@@ -1022,6 +1065,11 @@ static int stm32_adc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = stm32_adc_of_get_resolution(indio_dev);
+	if (ret < 0)
+		goto err_clk_disable;
+	stm32_adc_set_res(adc);
+
 	ret = stm32_adc_chan_of_init(indio_dev);
 	if (ret < 0)
 		goto err_clk_disable;
-- 
1.9.1

[PATCH 2/2] iio: adc: stm32: add dt option to set resolution

[Fabrice Gasnier]

stm32 adc supports several resolution. Add 'st,adc-res' dt optional
property to set it. Default to maximum resolution in case it isn't set.

Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
---
 drivers/iio/adc/stm32-adc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
index 9b49a6ad..268b457a 100644
--- a/drivers/iio/adc/stm32-adc.c
+++ b/drivers/iio/adc/stm32-adc.c
@@ -60,6 +60,8 @@
 #define STM32F4_EOC			BIT(1)
 
 /* STM32F4_ADC_CR1 - bit fields */
+#define STM32F4_RES_SHIFT		24
+#define STM32F4_RES_MASK		GENMASK(25, 24)
 #define STM32F4_SCAN			BIT(8)
 #define STM32F4_EOCIE			BIT(5)
 
@@ -141,6 +143,7 @@ struct stm32_adc_regs {
  * @lock:		spinlock
  * @bufi:		data buffer index
  * @num_conv:		expected number of scan conversions
+ * @res:		data resolution (e.g. RES bitfield value)
  * @trigger_polarity:	external trigger polarity (e.g. exten)
  * @dma_chan:		dma channel
  * @rx_buf:		dma rx buffer cpu address
@@ -157,6 +160,7 @@ struct stm32_adc {
 	spinlock_t		lock;		/* interrupt lock */
 	unsigned int		bufi;
 	unsigned int		num_conv;
+	u32			res;
 	u32			trigger_polarity;
 	struct dma_chan		*dma_chan;
 	u8			*rx_buf;
@@ -196,6 +200,11 @@ struct stm32_adc_chan_spec {
 	{ IIO_VOLTAGE, 15, "in15" },
 };
 
+static const unsigned int stm32f4_adc_resolutions[] = {
+	/* sorted values so the index matches RES[1:0] in STM32F4_ADC_CR1 */
+	12, 10, 8, 6,
+};
+
 /**
  * stm32f4_sq - describe regular sequence registers
  * - L: sequence len (register & bit field)
@@ -302,6 +311,14 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
 	stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_EOCIE);
 }
 
+static void stm32_adc_set_res(struct stm32_adc *adc)
+{
+	u32 val = stm32_adc_readl(adc, STM32F4_ADC_CR1);
+
+	val = (val & ~STM32F4_RES_MASK) | (adc->res << STM32F4_RES_SHIFT);
+	stm32_adc_writel(adc, STM32F4_ADC_CR1, val);
+}
+
 /**
  * stm32_adc_start_conv() - Start conversions for regular channels.
  * @adc: stm32 adc instance
@@ -870,11 +887,37 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
 	{},
 };
 
+static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
+{
+	struct device_node *node = indio_dev->dev.of_node;
+	struct stm32_adc *adc = iio_priv(indio_dev);
+	unsigned int i;
+	u32 res;
+
+	if (of_property_read_u32(node, "st,adc-res", &res))
+		res = stm32f4_adc_resolutions[0];
+
+	for (i = 0; i < ARRAY_SIZE(stm32f4_adc_resolutions); i++)
+		if (res == stm32f4_adc_resolutions[i])
+			break;
+	if (i >= ARRAY_SIZE(stm32f4_adc_resolutions)) {
+		dev_err(&indio_dev->dev, "Bad resolution: %u bits\n", res);
+		return -EINVAL;
+	}
+
+	dev_dbg(&indio_dev->dev, "Using %u bits resolution\n", res);
+	adc->res = i;
+
+	return 0;
+}
+
 static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
 				    struct iio_chan_spec *chan,
 				    const struct stm32_adc_chan_spec *channel,
 				    int scan_index)
 {
+	struct stm32_adc *adc = iio_priv(indio_dev);
+
 	chan->type = channel->type;
 	chan->channel = channel->channel;
 	chan->datasheet_name = channel->name;
@@ -883,7 +926,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
 	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
 	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
 	chan->scan_type.sign = 'u';
-	chan->scan_type.realbits = 12;
+	chan->scan_type.realbits = stm32f4_adc_resolutions[adc->res];
 	chan->scan_type.storagebits = 16;
 	chan->ext_info = stm32_adc_ext_info;
 }
@@ -1022,6 +1065,11 @@ static int stm32_adc_probe(struct platform_device *pdev)
 		return ret;
 	}
 
+	ret = stm32_adc_of_get_resolution(indio_dev);
+	if (ret < 0)
+		goto err_clk_disable;
+	stm32_adc_set_res(adc);
+
 	ret = stm32_adc_chan_of_init(indio_dev);
 	if (ret < 0)
 		goto err_clk_disable;
-- 
1.9.1

Re: [PATCH 2/2] iio: adc: stm32: add dt option to set resolution

[Jonathan Cameron]

On 15/02/17 16:55, Fabrice Gasnier wrote:
> stm32 adc supports several resolution. Add 'st,adc-res' dt optional
> property to set it. Default to maximum resolution in case it isn't set.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Implementation looks fine, just waiting on the binding review.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/stm32-adc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 9b49a6ad..268b457a 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -60,6 +60,8 @@
>  #define STM32F4_EOC			BIT(1)
>  
>  /* STM32F4_ADC_CR1 - bit fields */
> +#define STM32F4_RES_SHIFT		24
> +#define STM32F4_RES_MASK		GENMASK(25, 24)
>  #define STM32F4_SCAN			BIT(8)
>  #define STM32F4_EOCIE			BIT(5)
>  
> @@ -141,6 +143,7 @@ struct stm32_adc_regs {
>   * @lock:		spinlock
>   * @bufi:		data buffer index
>   * @num_conv:		expected number of scan conversions
> + * @res:		data resolution (e.g. RES bitfield value)
>   * @trigger_polarity:	external trigger polarity (e.g. exten)
>   * @dma_chan:		dma channel
>   * @rx_buf:		dma rx buffer cpu address
> @@ -157,6 +160,7 @@ struct stm32_adc {
>  	spinlock_t		lock;		/* interrupt lock */
>  	unsigned int		bufi;
>  	unsigned int		num_conv;
> +	u32			res;
>  	u32			trigger_polarity;
>  	struct dma_chan		*dma_chan;
>  	u8			*rx_buf;
> @@ -196,6 +200,11 @@ struct stm32_adc_chan_spec {
>  	{ IIO_VOLTAGE, 15, "in15" },
>  };
>  
> +static const unsigned int stm32f4_adc_resolutions[] = {
> +	/* sorted values so the index matches RES[1:0] in STM32F4_ADC_CR1 */
> +	12, 10, 8, 6,
> +};
> +
>  /**
>   * stm32f4_sq - describe regular sequence registers
>   * - L: sequence len (register & bit field)
> @@ -302,6 +311,14 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
>  	stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_EOCIE);
>  }
>  
> +static void stm32_adc_set_res(struct stm32_adc *adc)
> +{
> +	u32 val = stm32_adc_readl(adc, STM32F4_ADC_CR1);
> +
> +	val = (val & ~STM32F4_RES_MASK) | (adc->res << STM32F4_RES_SHIFT);
> +	stm32_adc_writel(adc, STM32F4_ADC_CR1, val);
> +}
> +
>  /**
>   * stm32_adc_start_conv() - Start conversions for regular channels.
>   * @adc: stm32 adc instance
> @@ -870,11 +887,37 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
>  	{},
>  };
>  
> +static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
> +{
> +	struct device_node *node = indio_dev->dev.of_node;
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	unsigned int i;
> +	u32 res;
> +
> +	if (of_property_read_u32(node, "st,adc-res", &res))
> +		res = stm32f4_adc_resolutions[0];
> +
> +	for (i = 0; i < ARRAY_SIZE(stm32f4_adc_resolutions); i++)
> +		if (res == stm32f4_adc_resolutions[i])
> +			break;
> +	if (i >= ARRAY_SIZE(stm32f4_adc_resolutions)) {
> +		dev_err(&indio_dev->dev, "Bad resolution: %u bits\n", res);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(&indio_dev->dev, "Using %u bits resolution\n", res);
> +	adc->res = i;
> +
> +	return 0;
> +}
> +
>  static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  				    struct iio_chan_spec *chan,
>  				    const struct stm32_adc_chan_spec *channel,
>  				    int scan_index)
>  {
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +
>  	chan->type = channel->type;
>  	chan->channel = channel->channel;
>  	chan->datasheet_name = channel->name;
> @@ -883,7 +926,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
>  	chan->scan_type.sign = 'u';
> -	chan->scan_type.realbits = 12;
> +	chan->scan_type.realbits = stm32f4_adc_resolutions[adc->res];
>  	chan->scan_type.storagebits = 16;
>  	chan->ext_info = stm32_adc_ext_info;
>  }
> @@ -1022,6 +1065,11 @@ static int stm32_adc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = stm32_adc_of_get_resolution(indio_dev);
> +	if (ret < 0)
> +		goto err_clk_disable;
> +	stm32_adc_set_res(adc);
> +
>  	ret = stm32_adc_chan_of_init(indio_dev);
>  	if (ret < 0)
>  		goto err_clk_disable;
> 

[PATCH 2/2] iio: adc: stm32: add dt option to set resolution

[Jonathan Cameron]

On 15/02/17 16:55, Fabrice Gasnier wrote:
> stm32 adc supports several resolution. Add 'st,adc-res' dt optional
> property to set it. Default to maximum resolution in case it isn't set.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Implementation looks fine, just waiting on the binding review.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/stm32-adc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 9b49a6ad..268b457a 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -60,6 +60,8 @@
>  #define STM32F4_EOC			BIT(1)
>  
>  /* STM32F4_ADC_CR1 - bit fields */
> +#define STM32F4_RES_SHIFT		24
> +#define STM32F4_RES_MASK		GENMASK(25, 24)
>  #define STM32F4_SCAN			BIT(8)
>  #define STM32F4_EOCIE			BIT(5)
>  
> @@ -141,6 +143,7 @@ struct stm32_adc_regs {
>   * @lock:		spinlock
>   * @bufi:		data buffer index
>   * @num_conv:		expected number of scan conversions
> + * @res:		data resolution (e.g. RES bitfield value)
>   * @trigger_polarity:	external trigger polarity (e.g. exten)
>   * @dma_chan:		dma channel
>   * @rx_buf:		dma rx buffer cpu address
> @@ -157,6 +160,7 @@ struct stm32_adc {
>  	spinlock_t		lock;		/* interrupt lock */
>  	unsigned int		bufi;
>  	unsigned int		num_conv;
> +	u32			res;
>  	u32			trigger_polarity;
>  	struct dma_chan		*dma_chan;
>  	u8			*rx_buf;
> @@ -196,6 +200,11 @@ struct stm32_adc_chan_spec {
>  	{ IIO_VOLTAGE, 15, "in15" },
>  };
>  
> +static const unsigned int stm32f4_adc_resolutions[] = {
> +	/* sorted values so the index matches RES[1:0] in STM32F4_ADC_CR1 */
> +	12, 10, 8, 6,
> +};
> +
>  /**
>   * stm32f4_sq - describe regular sequence registers
>   * - L: sequence len (register & bit field)
> @@ -302,6 +311,14 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
>  	stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_EOCIE);
>  }
>  
> +static void stm32_adc_set_res(struct stm32_adc *adc)
> +{
> +	u32 val = stm32_adc_readl(adc, STM32F4_ADC_CR1);
> +
> +	val = (val & ~STM32F4_RES_MASK) | (adc->res << STM32F4_RES_SHIFT);
> +	stm32_adc_writel(adc, STM32F4_ADC_CR1, val);
> +}
> +
>  /**
>   * stm32_adc_start_conv() - Start conversions for regular channels.
>   * @adc: stm32 adc instance
> @@ -870,11 +887,37 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
>  	{},
>  };
>  
> +static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
> +{
> +	struct device_node *node = indio_dev->dev.of_node;
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	unsigned int i;
> +	u32 res;
> +
> +	if (of_property_read_u32(node, "st,adc-res", &res))
> +		res = stm32f4_adc_resolutions[0];
> +
> +	for (i = 0; i < ARRAY_SIZE(stm32f4_adc_resolutions); i++)
> +		if (res == stm32f4_adc_resolutions[i])
> +			break;
> +	if (i >= ARRAY_SIZE(stm32f4_adc_resolutions)) {
> +		dev_err(&indio_dev->dev, "Bad resolution: %u bits\n", res);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(&indio_dev->dev, "Using %u bits resolution\n", res);
> +	adc->res = i;
> +
> +	return 0;
> +}
> +
>  static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  				    struct iio_chan_spec *chan,
>  				    const struct stm32_adc_chan_spec *channel,
>  				    int scan_index)
>  {
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +
>  	chan->type = channel->type;
>  	chan->channel = channel->channel;
>  	chan->datasheet_name = channel->name;
> @@ -883,7 +926,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
>  	chan->scan_type.sign = 'u';
> -	chan->scan_type.realbits = 12;
> +	chan->scan_type.realbits = stm32f4_adc_resolutions[adc->res];
>  	chan->scan_type.storagebits = 16;
>  	chan->ext_info = stm32_adc_ext_info;
>  }
> @@ -1022,6 +1065,11 @@ static int stm32_adc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = stm32_adc_of_get_resolution(indio_dev);
> +	if (ret < 0)
> +		goto err_clk_disable;
> +	stm32_adc_set_res(adc);
> +
>  	ret = stm32_adc_chan_of_init(indio_dev);
>  	if (ret < 0)
>  		goto err_clk_disable;
> 

Re: [PATCH 2/2] iio: adc: stm32: add dt option to set resolution

[Jonathan Cameron]

On 15/02/17 16:55, Fabrice Gasnier wrote:
> stm32 adc supports several resolution. Add 'st,adc-res' dt optional
> property to set it. Default to maximum resolution in case it isn't set.
> 
> Signed-off-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Implementation looks fine, just waiting on the binding review.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/stm32-adc.c | 50 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 49 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index 9b49a6ad..268b457a 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -60,6 +60,8 @@
>  #define STM32F4_EOC			BIT(1)
>  
>  /* STM32F4_ADC_CR1 - bit fields */
> +#define STM32F4_RES_SHIFT		24
> +#define STM32F4_RES_MASK		GENMASK(25, 24)
>  #define STM32F4_SCAN			BIT(8)
>  #define STM32F4_EOCIE			BIT(5)
>  
> @@ -141,6 +143,7 @@ struct stm32_adc_regs {
>   * @lock:		spinlock
>   * @bufi:		data buffer index
>   * @num_conv:		expected number of scan conversions
> + * @res:		data resolution (e.g. RES bitfield value)
>   * @trigger_polarity:	external trigger polarity (e.g. exten)
>   * @dma_chan:		dma channel
>   * @rx_buf:		dma rx buffer cpu address
> @@ -157,6 +160,7 @@ struct stm32_adc {
>  	spinlock_t		lock;		/* interrupt lock */
>  	unsigned int		bufi;
>  	unsigned int		num_conv;
> +	u32			res;
>  	u32			trigger_polarity;
>  	struct dma_chan		*dma_chan;
>  	u8			*rx_buf;
> @@ -196,6 +200,11 @@ struct stm32_adc_chan_spec {
>  	{ IIO_VOLTAGE, 15, "in15" },
>  };
>  
> +static const unsigned int stm32f4_adc_resolutions[] = {
> +	/* sorted values so the index matches RES[1:0] in STM32F4_ADC_CR1 */
> +	12, 10, 8, 6,
> +};
> +
>  /**
>   * stm32f4_sq - describe regular sequence registers
>   * - L: sequence len (register & bit field)
> @@ -302,6 +311,14 @@ static void stm32_adc_conv_irq_disable(struct stm32_adc *adc)
>  	stm32_adc_clr_bits(adc, STM32F4_ADC_CR1, STM32F4_EOCIE);
>  }
>  
> +static void stm32_adc_set_res(struct stm32_adc *adc)
> +{
> +	u32 val = stm32_adc_readl(adc, STM32F4_ADC_CR1);
> +
> +	val = (val & ~STM32F4_RES_MASK) | (adc->res << STM32F4_RES_SHIFT);
> +	stm32_adc_writel(adc, STM32F4_ADC_CR1, val);
> +}
> +
>  /**
>   * stm32_adc_start_conv() - Start conversions for regular channels.
>   * @adc: stm32 adc instance
> @@ -870,11 +887,37 @@ static irqreturn_t stm32_adc_trigger_handler(int irq, void *p)
>  	{},
>  };
>  
> +static int stm32_adc_of_get_resolution(struct iio_dev *indio_dev)
> +{
> +	struct device_node *node = indio_dev->dev.of_node;
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +	unsigned int i;
> +	u32 res;
> +
> +	if (of_property_read_u32(node, "st,adc-res", &res))
> +		res = stm32f4_adc_resolutions[0];
> +
> +	for (i = 0; i < ARRAY_SIZE(stm32f4_adc_resolutions); i++)
> +		if (res == stm32f4_adc_resolutions[i])
> +			break;
> +	if (i >= ARRAY_SIZE(stm32f4_adc_resolutions)) {
> +		dev_err(&indio_dev->dev, "Bad resolution: %u bits\n", res);
> +		return -EINVAL;
> +	}
> +
> +	dev_dbg(&indio_dev->dev, "Using %u bits resolution\n", res);
> +	adc->res = i;
> +
> +	return 0;
> +}
> +
>  static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  				    struct iio_chan_spec *chan,
>  				    const struct stm32_adc_chan_spec *channel,
>  				    int scan_index)
>  {
> +	struct stm32_adc *adc = iio_priv(indio_dev);
> +
>  	chan->type = channel->type;
>  	chan->channel = channel->channel;
>  	chan->datasheet_name = channel->name;
> @@ -883,7 +926,7 @@ static void stm32_adc_chan_init_one(struct iio_dev *indio_dev,
>  	chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW);
>  	chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
>  	chan->scan_type.sign = 'u';
> -	chan->scan_type.realbits = 12;
> +	chan->scan_type.realbits = stm32f4_adc_resolutions[adc->res];
>  	chan->scan_type.storagebits = 16;
>  	chan->ext_info = stm32_adc_ext_info;
>  }
> @@ -1022,6 +1065,11 @@ static int stm32_adc_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  
> +	ret = stm32_adc_of_get_resolution(indio_dev);
> +	if (ret < 0)
> +		goto err_clk_disable;
> +	stm32_adc_set_res(adc);
> +
>  	ret = stm32_adc_chan_of_init(indio_dev);
>  	if (ret < 0)
>  		goto err_clk_disable;
>