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

Groups > linux.kernel > #1181396 > unrolled thread

Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample  time Shawn Guo <shawnguo@kernel.org> - 2015-07-10 11:00 +0200
    Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample  time Jonathan Cameron <jic23@kernel.org> - 2015-07-11 19:40 +0200
      Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample  time Stefan Agner <stefan@agner.ch> - 2015-07-13 08:50 +0200
      Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time Jonathan Cameron <jic23@kernel.org> - 2015-07-13 11:30 +0200

#1181396 — Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time

On Wed, Jun 24, 2015 at 02:03:41PM +0530, Sanchayan Maity wrote:
> Add a device tree property which allows to specify the minimum sample
> time which can be used to calculate the actual ADC cycles required
> depending on the hardware.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
> ---
>  arch/arm/boot/dts/vfxxx.dtsi | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
> index 90a03d5..71d9c08 100644
> --- a/arch/arm/boot/dts/vfxxx.dtsi
> +++ b/arch/arm/boot/dts/vfxxx.dtsi
> @@ -229,6 +229,7 @@
>  				status = "disabled";
>  				fsl,adck-max-frequency = <30000000>, <40000000>,
>  							<20000000>;
> +				min-sample-time = <1000>;
>  			};
>  
>  			wdoga5: wdog@4003e000 {
> @@ -447,6 +448,7 @@
>  				status = "disabled";
>  				fsl,adck-max-frequency = <30000000>, <40000000>,
>  							<20000000>;
> +				min-sample-time = <1000>;

Can we code 1000 as the default in kernel driver, so that only boards
requiring different value need to have this property?  Doing so makes
the property optional rather than required.

Shawn

>  			};
>  
>  			esdhc1: esdhc@400b2000 {
> -- 
> 2.4.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1182197

On 10/07/15 19:06, maitysanchayan@gmail.com wrote:
> Hello Shawn,
> 
> On 15-07-10 16:53:24, Shawn Guo wrote:
>> On Wed, Jun 24, 2015 at 02:03:41PM +0530, Sanchayan Maity wrote:
>>> Add a device tree property which allows to specify the minimum sample
>>> time which can be used to calculate the actual ADC cycles required
>>> depending on the hardware.
>>>
>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>> ---
>>>  arch/arm/boot/dts/vfxxx.dtsi | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
>>> index 90a03d5..71d9c08 100644
>>> --- a/arch/arm/boot/dts/vfxxx.dtsi
>>> +++ b/arch/arm/boot/dts/vfxxx.dtsi
>>> @@ -229,6 +229,7 @@
>>>  				status = "disabled";
>>>  				fsl,adck-max-frequency = <30000000>, <40000000>,
>>>  							<20000000>;
>>> +				min-sample-time = <1000>;
>>>  			};
>>>  
>>>  			wdoga5: wdog@4003e000 {
>>> @@ -447,6 +448,7 @@
>>>  				status = "disabled";
>>>  				fsl,adck-max-frequency = <30000000>, <40000000>,
>>>  							<20000000>;
>>> +				min-sample-time = <1000>;
>>
>> Can we code 1000 as the default in kernel driver, so that only boards
>> requiring different value need to have this property?  Doing so makes
>> the property optional rather than required.
>>
> 
> Not sure if hardcoding it in the driver is the right approach.
If it is a true feature of the device (i.e. if in the case of perfect
front end electronics) this is the right option, then a default makes
a lot of sense.  If that isn't the case (I suspect not) then if we
drop it be optional chances are no one will bother thinking about it
or trying to tune this at all.

Hence seems wrong to put a fairly arbitrary default value on it.
However, we do need to still work with old device trees and new kernels
so need to cope without it.

Hence to my mind, if we had started out with this in the first driver
version, then the default would be a bad idea.  As we didn't then we
really need to cope with nothing specified (as best we can) and so
we do need a sensible default (or perhaps even sensible worst
case default) in there. 
> 
> However if the maintainers and others agree on doing this, I will do
> the necessary change.
> 
> Thanks.
> 
> Regards,
> Sanchayan.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1182542

On 2015-07-12 08:47, maitysanchayan@gmail.com wrote:
> Hello Jonathan,
> 
> On 15-07-11 18:39:10, Jonathan Cameron wrote:
>> On 10/07/15 19:06, maitysanchayan@gmail.com wrote:
>> > Hello Shawn,
>> >
>> > On 15-07-10 16:53:24, Shawn Guo wrote:
>> >> On Wed, Jun 24, 2015 at 02:03:41PM +0530, Sanchayan Maity wrote:
>> >>> Add a device tree property which allows to specify the minimum sample
>> >>> time which can be used to calculate the actual ADC cycles required
>> >>> depending on the hardware.
>> >>>
>> >>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>> >>> ---
>> >>>  arch/arm/boot/dts/vfxxx.dtsi | 2 ++
>> >>>  1 file changed, 2 insertions(+)
>> >>>
>> >>> diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
>> >>> index 90a03d5..71d9c08 100644
>> >>> --- a/arch/arm/boot/dts/vfxxx.dtsi
>> >>> +++ b/arch/arm/boot/dts/vfxxx.dtsi
>> >>> @@ -229,6 +229,7 @@
>> >>>  				status = "disabled";
>> >>>  				fsl,adck-max-frequency = <30000000>, <40000000>,
>> >>>  							<20000000>;
>> >>> +				min-sample-time = <1000>;
>> >>>  			};
>> >>>
>> >>>  			wdoga5: wdog@4003e000 {
>> >>> @@ -447,6 +448,7 @@
>> >>>  				status = "disabled";
>> >>>  				fsl,adck-max-frequency = <30000000>, <40000000>,
>> >>>  							<20000000>;
>> >>> +				min-sample-time = <1000>;
>> >>
>> >> Can we code 1000 as the default in kernel driver, so that only boards
>> >> requiring different value need to have this property?  Doing so makes
>> >> the property optional rather than required.
>> >>
>> >
>> > Not sure if hardcoding it in the driver is the right approach.
>> If it is a true feature of the device (i.e. if in the case of perfect
>> front end electronics) this is the right option, then a default makes
>> a lot of sense.  If that isn't the case (I suspect not) then if we
>> drop it be optional chances are no one will bother thinking about it
>> or trying to tune this at all.
>>
>> Hence seems wrong to put a fairly arbitrary default value on it.
>> However, we do need to still work with old device trees and new kernels
>> so need to cope without it.
>>
>> Hence to my mind, if we had started out with this in the first driver
>> version, then the default would be a bad idea.  As we didn't then we
>> really need to cope with nothing specified (as best we can) and so
>> we do need a sensible default (or perhaps even sensible worst
>> case default) in there.

I agree with Jonathan's argumentation, let's add a default if the dt
propery is not there.

1000ns seems to be a good default value over a wide range of external
resistance/capacity according to the diagrams of the data sheet, so I
would vote for that value as default.

> 
> Just to be sure, do I understand you correctly that you agree with the
> property being in device tree?

I don't think that the device tree property is in discussion, it is just
about whether to add a default value in the driver or not...

> 
> If the device tree property is not specified the driver will just go on
> to use the value "3" which was hardcoded earlier. In my opinion it is
> better to allow users to change the sampling cycles by specifying or not
> specifying this in the device tree rather than have a default value coded
> in the driver. However this is just my opinion.
> 
> Also, some users might not need the somewhat worst case value of 1000. I
> guess we could keep the driver patch the way it is right now and migrate
> the property to be specified in our board dts file? So the property can
> be picked up from the vf-colibri.dtsi or vf500-colibri.dtsi in the adc
> node? Other boards can do the same?

I agree too, especially when we have a default value in the driver, the
property belongs into the board file. I suggest to add the default value
of 1000 to the vf-colibri.dtsi (even if this is the driver default, so
we explicitly request that "verified" value..)

--
Stefan

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1182634 — Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time

On 12 July 2015 07:47:53 BST, maitysanchayan@gmail.com wrote:
>Hello Jonathan,
>
>On 15-07-11 18:39:10, Jonathan Cameron wrote:
>> On 10/07/15 19:06, maitysanchayan@gmail.com wrote:
>> > Hello Shawn,
>> > 
>> > On 15-07-10 16:53:24, Shawn Guo wrote:
>> >> On Wed, Jun 24, 2015 at 02:03:41PM +0530, Sanchayan Maity wrote:
>> >>> Add a device tree property which allows to specify the minimum
>sample
>> >>> time which can be used to calculate the actual ADC cycles
>required
>> >>> depending on the hardware.
>> >>>
>> >>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>> >>> ---
>> >>>  arch/arm/boot/dts/vfxxx.dtsi | 2 ++
>> >>>  1 file changed, 2 insertions(+)
>> >>>
>> >>> diff --git a/arch/arm/boot/dts/vfxxx.dtsi
>b/arch/arm/boot/dts/vfxxx.dtsi
>> >>> index 90a03d5..71d9c08 100644
>> >>> --- a/arch/arm/boot/dts/vfxxx.dtsi
>> >>> +++ b/arch/arm/boot/dts/vfxxx.dtsi
>> >>> @@ -229,6 +229,7 @@
>> >>>  				status = "disabled";
>> >>>  				fsl,adck-max-frequency = <30000000>, <40000000>,
>> >>>  							<20000000>;
>> >>> +				min-sample-time = <1000>;
>> >>>  			};
>> >>>  
>> >>>  			wdoga5: wdog@4003e000 {
>> >>> @@ -447,6 +448,7 @@
>> >>>  				status = "disabled";
>> >>>  				fsl,adck-max-frequency = <30000000>, <40000000>,
>> >>>  							<20000000>;
>> >>> +				min-sample-time = <1000>;
>> >>
>> >> Can we code 1000 as the default in kernel driver, so that only
>boards
>> >> requiring different value need to have this property?  Doing so
>makes
>> >> the property optional rather than required.
>> >>
>> > 
>> > Not sure if hardcoding it in the driver is the right approach.
>> If it is a true feature of the device (i.e. if in the case of perfect
>> front end electronics) this is the right option, then a default makes
>> a lot of sense.  If that isn't the case (I suspect not) then if we
>> drop it be optional chances are no one will bother thinking about it
>> or trying to tune this at all.
>> 
>> Hence seems wrong to put a fairly arbitrary default value on it.
>> However, we do need to still work with old device trees and new
>kernels
>> so need to cope without it.
>> 
>> Hence to my mind, if we had started out with this in the first driver
>> version, then the default would be a bad idea.  As we didn't then we
>> really need to cope with nothing specified (as best we can) and so
>> we do need a sensible default (or perhaps even sensible worst
>> case default) in there.
>
>Just to be sure, do I understand you correctly that you agree with the
>property being in device tree?
Absolutely. I wish it had been there from the start!
>
>If the device tree property is not specified the driver will just go on
>to use the value "3" which was hardcoded earlier. In my opinion it is
>better to allow users to change the sampling cycles by specifying or
>not
>specifying this in the device tree rather than have a default value
>coded
>in the driver. However this is just my opinion.
>
>Also, some users might not need the somewhat worst case value of 1000.
>I
>guess we could keep the driver patch the way it is right now and
>migrate
>the property to be specified in our board dts file? So the property can
>be picked up from the vf-colibri.dtsi or vf500-colibri.dtsi in the adc
>node? Other boards can do the same?
The issue is device trees that don't get updated on devices. Those need a default and the property to be optional.
>
>We came up with the change after noticing huge reading discrepancies
>where
>we had a 4 wire resistive touch screen connected to the ADC channels
>and
>the driver sampled these channels at an interval of 10-20ms[1]. Once
>the
>touchscreen came into picture, readings from temperature channel or
>others
>showed deviations between 40000-60000. Somehow the temperature channel
>seemed to be the most affected.
Yikes
>
>For a while, I thought the ts driver logic was at a fault, but Stefan
>pointed
>out the discrepancies in the driver using a fixed clock cycle which was
>not
>correct along the sampling time also being hardcoded. Stefan's "respect
>ADC
>clocking limitations" and this patch are based on our above
>observations.
Fair enough. Can see how this was missed in the first place.  Good to see it fixed.
>
>[1] https://lkml.org/lkml/2015/6/30/103
>
>- Sanchayan.

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web