Implement sample time consideration for Vybrid's ADC
v2 cover Implement sample time consideration for Vybrid's ADC
Hello,
This patchset adds a dt binding for specifying sample time for the vybrid adc driver and takes this into account for sampling frequency calculation and related configuration in the driver.
The patchset is based on top of Stefan's patches here
http://lkml.iu.edu/hypermail/linux/kernel/1505.3/02043.html
which got recently applied. Tested with shawn's for-next branch.
Changes since v1:
Change from a vendor specific fsl,min-sample-time to non vendor specific min-sample-time.
Version 1 of the patchset can be found here
http://lkml.iu.edu/hypermail/linux/kernel/1506.1/00026.html
- Sanchayan.
Sanchayan Maity (2):
iio: adc: Determine sampling frequencies by using minimum sample time
ARM: dts: vfxxx: Add property for minimum sample time
.../devicetree/bindings/iio/adc/vf610-adc.txt | 6 ++ arch/arm/boot/dts/vfxxx.dtsi | 2 + drivers/iio/adc/vf610_adc.c | 74 ++++++++++++++++++++-- 3 files changed, 78 insertions(+), 4 deletions(-)
Signature · 1 line
2.4.4
PATCH v2 1/2 iio: adc: Determine sampling frequencies by using minimum sample time
The driver currently does not take into account the minimum sample time as per the Figure 6-8 Chapter 9.1.1 12-bit ADC electrical characteristics. We set a static amount of cycles instead of considering the sample time as a given value, which depends on hardware characteristics.
Determine sampling frequencies by first reading the device tree property node and then calculating the required Long Sample Time Adder (LSTAdder) value based on the ADC clock frequency and sample time value obtained from the device tree, this LSTAdder value is then used for calculating the sampling frequencies possible.
--- .../devicetree/bindings/iio/adc/vf610-adc.txt | 6 ++ drivers/iio/adc/vf610_adc.c | 74 ++++++++++++++++++++-- 2 files changed, 76 insertions(+), 4 deletions(-)
Documentation/devicetree/bindings/iio/adc/vf610-adc.txt +6 -0
@@ -17,6 +17,11 @@ Recommended properties: - Frequency in normal mode (ADLPC=0, ADHSC=0) - Frequency in high-speed mode (ADLPC=0, ADHSC=1) - Frequency in low-power mode (ADLPC=1, ADHSC=0) + - min-sample-time: Minimum sampling time in nanoseconds. This value has + to be chosen according to the conversion mode and the connected analog + source resistance (R_as) and capacitance (C_as). Refer the datasheet's + operating requirements. A safe default across a wide range of R_as and + C_as as well as conversion modes is 1000ns. Example: adc0: adc at 4003b000 {
@@ -27,5 +32,6 @@ adc0: adc at 4003b000 { clock-names = "adc"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; vref-supply = <®_vcc_3v3_mcu>; };
drivers/iio/adc/vf610_adc.c +70 -4
131 lines not on this page · raw message
Signature · 1 line
2.4.4
Jonathan Cameron
maintainerRe: [PATCH v2 1/2] iio: adc: Determine sampling frequencies by using minimum sample time
Quoted 5 lines · Sanchayan Maity
The driver currently does not take into account the minimum sample time as per the Figure 6-8 Chapter 9.1.1 12-bit ADC electrical characteristics. We set a static amount of cycles instead of considering the sample time as a given value, which depends on hardware characteristics.
Quoted 7 lines
Determine sampling frequencies by first reading the device tree property node and then calculating the required Long Sample Time Adder (LSTAdder) value based on the ADC clock frequency and sample time value obtained from the device tree, this LSTAdder value is then used for calculating the sampling frequencies possible.
Signed-off-by Sanchayan Maity <[email protected]>
Looking for an Ack on this from Fugang. Device tree ack also good ;)
Quoted 172 lines
--- .../devicetree/bindings/iio/adc/vf610-adc.txt | 6 ++ drivers/iio/adc/vf610_adc.c | 74 ++++++++++++++++++++-- 2 files changed, 76 insertions(+), 4 deletions(-)Documentation/devicetree/bindings/iio/adc/vf610-adc.txt +6
-0@@ -17,6 +17,11 @@ Recommended properties: - Frequency in normal mode (ADLPC=0, ADHSC=0) - Frequency in high-speed mode (ADLPC=0, ADHSC=1) - Frequency in low-power mode (ADLPC=1, ADHSC=0) + - min-sample-time: Minimum sampling time in nanoseconds. This value has + to be chosen according to the conversion mode and the connected analog + source resistance (R_as) and capacitance (C_as). Refer the datasheet's + operating requirements. A safe default across a wide range of R_as and + C_as as well as conversion modes is 1000ns. Example: adc0: adc at 4003b000 {@@ -27,5 +32,6 @@ adc0: adc at 4003b000 { clock-names = "adc"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; vref-supply = <®_vcc_3v3_mcu>; };drivers/iio/adc/vf610_adc.c +70
-4@@ -68,6 +68,9 @@#define VF610_ADC_CLK_DIV8 0x60#define VF610_ADC_CLK_MASK 0x60#define VF610_ADC_ADLSMP_LONG 0x10 +#define VF610_ADC_ADSTS_SHORT 0x100 +#define VF610_ADC_ADSTS_NORMAL 0x200 +#define VF610_ADC_ADSTS_LONG 0x300#define VF610_ADC_ADSTS_MASK 0x300#define VF610_ADC_ADLPC_EN 0x80#define VF610_ADC_ADHSC_EN 0x400@@ -124,6 +127,17 @@ enum conversion_mode_sel { VF610_ADC_CONV_LOW_POWER, }; +enum lst_adder_sel { +VF610_ADCK_CYCLES_3 , +VF610_ADCK_CYCLES_5 , +VF610_ADCK_CYCLES_7 , +VF610_ADCK_CYCLES_9 , +VF610_ADCK_CYCLES_13 , +VF610_ADCK_CYCLES_17 , +VF610_ADCK_CYCLES_21 , +VF610_ADCK_CYCLES_25 , +}; +struct vf610_adc_feature {enum clk_sel clk_sel;enum vol_ref vol_ref;@@ -132,6 +146,8 @@ struct vf610_adc_feature {int clk_div;int sample_rate;int res_mode; +u32 lst_adder_index; +u32 default_sample_time;bool calibration;bool ovwren;@@ -155,11 +171,13 @@ struct vf610_adc { };static const u32 vf610_hw_avgs[] = {1 ,4 ,8 ,16 ,32 }; +static const u32 vf610_lst_adder[] = {3 ,5 ,7 ,9 ,13 ,17 ,21 ,25 };static inline void vf610_adc_calculate_rates (struct vf610_adc *info) {struct vf610_adc_feature *adc_feature = &info->adc_feature;unsigned long adck_rate, ipg_rate =clk_get_rate (info->clk); +u32 adck_period, lst_addr_min;int divisor, i; adck_rate = info->max_adck_rate[adc_feature->conv_mode];@@ -174,6 +192,19 @@ static inline void vf610_adc_calculate_rates(struct vf610_adc *info) }/* +* Determine the long sample time adder value to be used based +* on the default minimum sample time provided. +*/ + adck_period =NSEC_PER_SEC / adck_rate; + lst_addr_min = adc_feature->default_sample_time / adck_period; +for (i =0 ; i <ARRAY_SIZE (vf610_lst_adder); i++) { +if (vf610_lst_adder[i] > lst_addr_min) { + adc_feature->lst_adder_index = i; +break ; + } + } + +/* * Calculate ADC sample frequencies * Sample time unit is ADCK cycles. ADCK clk source is ipg clock, * which is the same as bus clock. @@ -182,12 +213,13 @@ static inline void vf610_adc_calculate_rates(struct vf610_adc *info)* SFCAdder: fixed to 6 ADCK cycles * AverageNum: 1, 4, 8, 16, 32 samples for hardware average. * BCT (Base Conversion Time): fixed to 25 ADCK cycles for 12 bit mode -* LSTAdder(Long Sample Time): fixed to 3 ADCK cycles +* LSTAdder(Long Sample Time): 3, 5, 7, 9, 13, 17, 21, 25 ADCK cycles */ adck_rate = ipg_rate / info->adc_feature.clk_div;for (i =0 ; i <ARRAY_SIZE (vf610_hw_avgs); i++) info->sample_freq_avail[i] = - adck_rate / (6 + vf610_hw_avgs[i] * (25 +3 )); + adck_rate / (6 + vf610_hw_avgs[i] * + (25 + vf610_lst_adder[adc_feature->lst_adder_index])); }static inline void vf610_adc_cfg_init (struct vf610_adc *info)@@ -347,8 +379,40 @@ static void vf610_adc_sample_set(struct vf610_adc *info)break ; } -/* Use the short sample mode */ - cfg_data &= ~(VF610_ADC_ADLSMP_LONG |VF610_ADC_ADSTS_MASK ); +/* +* Set ADLSMP and ADSTS based on the Long Sample Time Adder value +* determined. +*/ +switch (adc_feature->lst_adder_index) { +case VF610_ADCK_CYCLES_3 : +break ; +case VF610_ADCK_CYCLES_5 : + cfg_data |=VF610_ADC_ADSTS_SHORT ; +break ; +case VF610_ADCK_CYCLES_7 : + cfg_data |=VF610_ADC_ADSTS_NORMAL ; +break ; +case VF610_ADCK_CYCLES_9 : + cfg_data |=VF610_ADC_ADSTS_LONG ; +break ; +case VF610_ADCK_CYCLES_13 : + cfg_data |=VF610_ADC_ADLSMP_LONG ; +break ; +case VF610_ADCK_CYCLES_17 : + cfg_data |=VF610_ADC_ADLSMP_LONG ; + cfg_data |=VF610_ADC_ADSTS_SHORT ; +break ; +case VF610_ADCK_CYCLES_21 : + cfg_data |=VF610_ADC_ADLSMP_LONG ; + cfg_data |=VF610_ADC_ADSTS_NORMAL ; +break ; +case VF610_ADCK_CYCLES_25 : + cfg_data |=VF610_ADC_ADLSMP_LONG ; + cfg_data |=VF610_ADC_ADSTS_NORMAL ; +break ; +default : +dev_err (info->dev,"error in sample time select\n" ); + }/* update hardware average selection */ cfg_data &= ~VF610_ADC_AVGS_MASK ;@@ -712,6 +776,8 @@ static int vf610_adc_probe(struct platform_device *pdev)of_property_read_u32_array (pdev->dev.of_node,"fsl,adck-max-frequency" , info->max_adck_rate,3 ); +of_property_read_u32 (pdev->dev.of_node,"min-sample-time" , + &info->adc_feature.default_sample_time);platform_set_drvdata (pdev, indio_dev);
Re: [PATCH v2 1/2] iio: adc: Determine sampling frequencies by using minimum sample time
In reply to Sanchayan MaityQuoted 5 lines · Sanchayan Maity
The driver currently does not take into account the minimum sample time as per the Figure 6-8 Chapter 9.1.1 12-bit ADC electrical characteristics. We set a static amount of cycles instead of considering the sample time as a given value, which depends on hardware characteristics.
Quoted 7 lines
Determine sampling frequencies by first reading the device tree property node and then calculating the required Long Sample Time Adder (LSTAdder) value based on the ADC clock frequency and sample time value obtained from the device tree, this LSTAdder value is then used for calculating the sampling frequencies possible.
Signed-off-by Sanchayan Maity <[email protected]>
Not respecting the sampling time leads to issues especially when using multiple channel: You basically still have some capacity from the last measurement, which distorts the next measurements. We noticed this when using the touch screen while measuring the SoC temperature.
The required sampling time is partly given by the SoC, but depends on the connected analog source too. A device tree property is the right approach here.
Quoted 173 lines
--- .../devicetree/bindings/iio/adc/vf610-adc.txt | 6 ++ drivers/iio/adc/vf610_adc.c | 74 ++++++++++++++++++++-- 2 files changed, 76 insertions(+), 4 deletions(-)Documentation/devicetree/bindings/iio/adc/vf610-adc.txt +6
-0@@ -17,6 +17,11 @@ Recommended properties: - Frequency in normal mode (ADLPC=0, ADHSC=0) - Frequency in high-speed mode (ADLPC=0, ADHSC=1) - Frequency in low-power mode (ADLPC=1, ADHSC=0) + - min-sample-time: Minimum sampling time in nanoseconds. This value has + to be chosen according to the conversion mode and the connected analog + source resistance (R_as) and capacitance (C_as). Refer the datasheet's + operating requirements. A safe default across a wide range of R_as and + C_as as well as conversion modes is 1000ns. Example: adc0: adc at 4003b000 {@@ -27,5 +32,6 @@ adc0: adc at 4003b000 { clock-names = "adc"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; vref-supply = <®_vcc_3v3_mcu>; };drivers/iio/adc/vf610_adc.c +31
-0@@ -68,6 +68,9 @@#define VF610_ADC_CLK_DIV8 0x60#define VF610_ADC_CLK_MASK 0x60#define VF610_ADC_ADLSMP_LONG 0x10 +#define VF610_ADC_ADSTS_SHORT 0x100 +#define VF610_ADC_ADSTS_NORMAL 0x200 +#define VF610_ADC_ADSTS_LONG 0x300#define VF610_ADC_ADSTS_MASK 0x300#define VF610_ADC_ADLPC_EN 0x80#define VF610_ADC_ADHSC_EN 0x400@@ -124,6 +127,17 @@ enum conversion_mode_sel { VF610_ADC_CONV_LOW_POWER, }; +enum lst_adder_sel { +VF610_ADCK_CYCLES_3 , +VF610_ADCK_CYCLES_5 , +VF610_ADCK_CYCLES_7 , +VF610_ADCK_CYCLES_9 , +VF610_ADCK_CYCLES_13 , +VF610_ADCK_CYCLES_17 , +VF610_ADCK_CYCLES_21 , +VF610_ADCK_CYCLES_25 , +}; +struct vf610_adc_feature {enum clk_sel clk_sel;enum vol_ref vol_ref;@@ -132,6 +146,8 @@ struct vf610_adc_feature {int clk_div;int sample_rate;int res_mode; +u32 lst_adder_index; +u32 default_sample_time;bool calibration;bool ovwren;@@ -155,11 +171,13 @@ struct vf610_adc { };static const u32 vf610_hw_avgs[] = {1 ,4 ,8 ,16 ,32 }; +static const u32 vf610_lst_adder[] = {3 ,5 ,7 ,9 ,13 ,17 ,21 ,25 };static inline void vf610_adc_calculate_rates (struct vf610_adc *info) {struct vf610_adc_feature *adc_feature = &info->adc_feature;unsigned long adck_rate, ipg_rate =clk_get_rate (info->clk); +u32 adck_period, lst_addr_min;int divisor, i; adck_rate = info->max_adck_rate[adc_feature->conv_mode];@@ -174,6 +192,19 @@ static inline void vf610_adc_calculate_rates(struct vf610_adc *info) }/* +* Determine the long sample time adder value to be used based +* on the default minimum sample time provided. +*/ + adck_period =NSEC_PER_SEC / adck_rate; + lst_addr_min = adc_feature->default_sample_time / adck_period; +for (i =0 ; i <ARRAY_SIZE (vf610_lst_adder); i++) { +if (vf610_lst_adder[i] > lst_addr_min) { + adc_feature->lst_adder_index = i; +break ; + } + } + +/* * Calculate ADC sample frequencies * Sample time unit is ADCK cycles. ADCK clk source is ipg clock, drivers/iio/adc/vf610_adc.c +3
-2* which is the same as bus clock. @@ -182,12 +213,13 @@ static inline voidvf610_adc_calculate_rates(struct vf610_adc *info) * SFCAdder: fixed to 6 ADCK cycles * AverageNum: 1, 4, 8, 16, 32 samples for hardware average. * BCT (Base Conversion Time): fixed to 25 ADCK cycles for 12 bit mode -* LSTAdder(Long Sample Time): fixed to 3 ADCK cycles +* LSTAdder(Long Sample Time): 3, 5, 7, 9, 13, 17, 21, 25 ADCK cycles */ adck_rate = ipg_rate / info->adc_feature.clk_div;for (i =0 ; i <ARRAY_SIZE (vf610_hw_avgs); i++) info->sample_freq_avail[i] = - adck_rate / (6 + vf610_hw_avgs[i] * (25 +3 )); + adck_rate / (6 + vf610_hw_avgs[i] * + (25 + vf610_lst_adder[adc_feature->lst_adder_index])); }static inline void
vf610_adc_cfg_init(struct vf610_adc *info)drivers/iio/adc/vf610_adc.c +36
-2@@ -347,8 +379,40 @@ static void vf610_adc_sample_set(struct vf610_adc *info)break ; } -/* Use the short sample mode */ - cfg_data &= ~(VF610_ADC_ADLSMP_LONG |VF610_ADC_ADSTS_MASK ); +/* +* Set ADLSMP and ADSTS based on the Long Sample Time Adder value +* determined. +*/ +switch (adc_feature->lst_adder_index) { +case VF610_ADCK_CYCLES_3 : +break ; +case VF610_ADCK_CYCLES_5 : + cfg_data |=VF610_ADC_ADSTS_SHORT ; +break ; +case VF610_ADCK_CYCLES_7 : + cfg_data |=VF610_ADC_ADSTS_NORMAL ; +break ; +case VF610_ADCK_CYCLES_9 : + cfg_data |=VF610_ADC_ADSTS_LONG ; +break ; +case VF610_ADCK_CYCLES_13 : + cfg_data |=VF610_ADC_ADLSMP_LONG ; +break ; +case VF610_ADCK_CYCLES_17 : + cfg_data |=VF610_ADC_ADLSMP_LONG ; + cfg_data |=VF610_ADC_ADSTS_SHORT ; +break ; +case VF610_ADCK_CYCLES_21 : + cfg_data |=VF610_ADC_ADLSMP_LONG ; + cfg_data |=VF610_ADC_ADSTS_NORMAL ; +break ; +case VF610_ADCK_CYCLES_25 : + cfg_data |=VF610_ADC_ADLSMP_LONG ; + cfg_data |=VF610_ADC_ADSTS_NORMAL ; +break ; +default : +dev_err (info->dev,"error in sample time select\n" ); + }/* update hardware average selection */ cfg_data &= ~VF610_ADC_AVGS_MASK ;@@ -712,6 +776,8 @@ static int vf610_adc_probe(struct platform_device *pdev)of_property_read_u32_array (pdev->dev.of_node,"fsl,adck-max-frequency" , info->max_adck_rate,3 ); +of_property_read_u32 (pdev->dev.of_node,"min-sample-time" , + &info->adc_feature.default_sample_time);platform_set_drvdata (pdev, indio_dev);
PATCH v2 2/2 ARM: dts: vfxxx: Add property for minimum sample time
In reply to Sanchayan MaityAdd 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.
--- arch/arm/boot/dts/vfxxx.dtsi | 2 ++ 1 file changed, 2 insertions(+)
arch/arm/boot/dts/vfxxx.dtsi +2 -0
@@ -229,6 +229,7 @@ status = "disabled"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; }; wdoga5: wdog at 4003e000 {
@@ -447,6 +448,7 @@ status = "disabled"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; }; esdhc1: esdhc at 400b2000 {
Signature · 1 line
2.4.4
Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time
Quoted 22 lines · Sanchayan Maity
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 <[email protected]>--- arch/arm/boot/dts/vfxxx.dtsi | 2 ++ 1 file changed, 2 insertions(+)arch/arm/boot/dts/vfxxx.dtsi +1
-0@@ -229,6 +229,7 @@ status = "disabled"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; }; wdoga5: wdog at 4003e000 {
arch/arm/boot/dts/vfxxx.dtsi line 451
Quoted 4 lines
arch/arm/boot/dts/vfxxx.dtsi +1
-0@@ -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
Quoted 12 lines
arch/arm/boot/dts/vfxxx.dtsi +0
-0}; esdhc1: esdhc at 400b2000 {Signature · 7 lines
2.4.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time
Hello Shawn,
Quoted 21 lines · Shawn Guo
Quoted 20 lines · Sanchayan Maity
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 <[email protected]>--- arch/arm/boot/dts/vfxxx.dtsi | 2 ++ 1 file changed, 2 insertions(+)arch/arm/boot/dts/vfxxx.dtsi +1
-0@@ -229,6 +229,7 @@ status = "disabled"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; };
Quoted 11 lines
arch/arm/boot/dts/vfxxx.dtsi line 451
Quoted 6 lines
arch/arm/boot/dts/vfxxx.dtsi +1
-0wdoga5: wdog at 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.
However if the maintainers and others agree on doing this, I will do the necessary change.
Thanks.
Regards,
Sanchayan.
Jonathan Cameron
maintainerRe: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time
Quoted 36 lines · maitysanchayan at gmail.com
Hello Shawn,
Quoted 21 lines · Shawn Guo
Quoted 20 lines · Sanchayan Maity
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 <[email protected]>--- arch/arm/boot/dts/vfxxx.dtsi | 2 ++ 1 file changed, 2 insertions(+)arch/arm/boot/dts/vfxxx.dtsi +1
-0@@ -229,6 +229,7 @@ status = "disabled"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; };Quoted 11 lines
arch/arm/boot/dts/vfxxx.dtsi line 451
Quoted 6 lines
arch/arm/boot/dts/vfxxx.dtsi +1
-0wdoga5: wdog at 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.
Quoted 1 line
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.
Quoted 9 lines
However if the maintainers and others agree on doing this, I will do the necessary change.
Thanks.
Regards,
Sanchayan.
Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time
Hello Jonathan,
Quoted 48 lines · Jonathan Cameron
Quoted 36 lines · maitysanchayan at gmail.com
Hello Shawn,
Quoted 21 lines · Shawn Guo
Quoted 20 lines · Sanchayan Maity
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 <[email protected]>--- arch/arm/boot/dts/vfxxx.dtsi | 2 ++ 1 file changed, 2 insertions(+)arch/arm/boot/dts/vfxxx.dtsi +1
-0@@ -229,6 +229,7 @@ status = "disabled"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; };Quoted 11 lines
arch/arm/boot/dts/vfxxx.dtsi line 451
Quoted 6 lines
arch/arm/boot/dts/vfxxx.dtsi +1
-0wdoga5: wdog at 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.
Quoted 1 line
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.
Quoted 5 lines
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?
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?
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.
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.
[1] https://lkml.org/lkml/2015/6/30/103
- Sanchayan.
Re: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time
Quoted 56 lines · maitysanchayan at gmail.com
Hello Jonathan,
Quoted 53 lines · Jonathan Cameron
Quoted 36 lines · maitysanchayan at gmail.com
Hello Shawn,
Quoted 21 lines · Shawn Guo
Quoted 20 lines · Sanchayan Maity
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 <[email protected]>--- arch/arm/boot/dts/vfxxx.dtsi | 2 ++ 1 file changed, 2 insertions(+)arch/arm/boot/dts/vfxxx.dtsi +1
-0@@ -229,6 +229,7 @@ status = "disabled"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; };Quoted 11 lines
arch/arm/boot/dts/vfxxx.dtsi line 451
Quoted 6 lines
arch/arm/boot/dts/vfxxx.dtsi +1
-0wdoga5: wdog at 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.
Quoted 1 line
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.
Quoted 3 lines
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...
Quoted 7 lines
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.
Quoted 5 lines
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.dtsiorvf500-colibri.dtsiin 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
Jonathan Cameron
maintainerRe: [PATCH v2 2/2] ARM: dts: vfxxx: Add property for minimum sample time
In reply to [email protected]Quoted 57 lines · maitysanchayan at gmail.com
Hello Jonathan,
Quoted 6 lines · Jonathan Cameron
Quoted 5 lines · maitysanchayan at gmail.com
Hello Shawn,
Quoted 2 lines · Shawn Guo
Quoted 1 line · Sanchayan Maity
Add a device tree property which allows to specify the minimum
sample
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
time which can be used to calculate the actual ADC cycles
required
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
depending on the hardware.
Signed-off-by Sanchayan Maity <[email protected]>--- arch/arm/boot/dts/vfxxx.dtsi | 2 ++ 1 file changed, 2 insertions(+)(quoted diff) +0
-0
b/arch/arm/boot/dts/vfxxx.dtsiQuoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
index 90a03d5..71d9c08 100644
arch/arm/boot/dts/vfxxx.dtsi +1
-0@@ -229,6 +229,7 @@ status = "disabled"; fsl,adck-max-frequency = <30000000>, <40000000>, <20000000>; + min-sample-time = <1000>; };Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
arch/arm/boot/dts/vfxxx.dtsi line 451
Quoted 6 lines
arch/arm/boot/dts/vfxxx.dtsi +1
-0wdoga5: wdog at 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
Quoted 1 line
Quoted 1 line
Quoted 1 line
requiring different value need to have this property? Doing so
makes
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
the property optional rather than required.
Not sure if hardcoding it in the driver is the right approach.
Quoted 8 lines
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
Quoted 2 lines
so need to cope without it.
Quoted 8 lines
Quoted 5 lines
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!
Quoted 9 lines
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.
Quoted 7 lines
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 thevf-colibri.dtsiorvf500-colibri.dtsiin 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.
Quoted 11 lines
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
Quoted 9 lines
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.
Quoted 4 lines
[1] https://lkml.org/lkml/2015/6/30/103
- Sanchayan.
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
From: Sanchayan Maity <[email protected]> Sent: Wednesday, June 24, 2015 4:34 PM
Quoted 34 lines
To: jic23 at kernel.org
Cc: shawn.guo at linaro.org; kernel at pengutronix.de; robh+dt at kernel.org; pawel.moll at arm.com; mark.rutland at arm.com; ijc+devicetree at hellion.org.uk; galak at codeaurora.org; Duan Fugang-B38611; devicetree at vger.kernel.org; linux-iio at vger.kernel.org; linux-arm-kernel at lists.infradead.org; linux- kernel at vger.kernel.org; stefan at agner.ch; Sanchayan Maity
Subject: [PATCH v2 0/2] Implement sample time consideration for Vybrid's ADCHello,
This patchset adds a dt binding for specifying sample time for the vybrid adc driver and takes this into account for sampling frequency calculation and related configuration in the driver.
The patchset is based on top of Stefan's patches here
http://lkml.iu.edu/hypermail/linux/kernel/1505.3/02043.htmlwhich got recently applied. Tested with shawn's for-next branch.
Changes since v1:
Change from a vendor specific fsl,min-sample-time to non vendor specific min-sample-time.
Version 1 of the patchset can be found here
http://lkml.iu.edu/hypermail/linux/kernel/1506.1/00026.html- Sanchayan.
Sanchayan Maity (2):
iio: adc: Determine sampling frequencies by using minimum sample time
ARM: dts: vfxxx: Add property for minimum sample time
Quoted 5 lines
.../devicetree/bindings/iio/adc/vf610-adc.txt | 6 ++ arch/arm/boot/dts/vfxxx.dtsi | 2 + drivers/iio/adc/vf610_adc.c | 74 ++++++++++++++++++++-- 3 files changed, 78 insertions(+), 4 deletions(-)
The patch set looks fine. Thanks Sanchayan add this new interface for user that is more options for user case.
Quoted 10 lines · Sanchayan Maity
Hello,
This patchset adds a dt binding for specifying sample time for the vybrid adc driver and takes this into account for sampling frequency calculation and related configuration in the driver.
The patchset is based on top of Stefan's patches here
http://lkml.iu.edu/hypermail/linux/kernel/1505.3/02043.html
Quoted 7 lines
which got recently applied. Tested with shawn's for-next branch.
Changes since v1:
Change from a vendor specific fsl,min-sample-time to non vendor specific min-sample-time.
What's the reason for that? Property without vendor prefix would be the generic one, which should be defined by generic ADC bindings, not vf610-adc.txt.
Shawn
Quoted 24 lines
Version 1 of the patchset can be found here
http://lkml.iu.edu/hypermail/linux/kernel/1506.1/00026.html- Sanchayan.
Sanchayan Maity (2):
iio: adc: Determine sampling frequencies by using minimum sample time
ARM: dts: vfxxx: Add property for minimum sample time.../devicetree/bindings/iio/adc/vf610-adc.txt | 6 ++ arch/arm/boot/dts/vfxxx.dtsi | 2 + drivers/iio/adc/vf610_adc.c | 74 ++++++++++++++++++++-- 3 files changed, 78 insertions(+), 4 deletions(-)Signature · 7 lines
2.4.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Hello Shawn,
Quoted 14 lines · Shawn Guo
Quoted 13 lines · Sanchayan Maity
Hello,
This patchset adds a dt binding for specifying sample time for the vybrid adc driver and takes this into account for sampling frequency calculation and related configuration in the driver.
The patchset is based on top of Stefan's patches here
http://lkml.iu.edu/hypermail/linux/kernel/1505.3/02043.htmlwhich got recently applied. Tested with shawn's for-next branch.
Quoted 8 lines
Quoted 4 lines
Changes since v1:
Change from a vendor specific fsl,min-sample-time to non vendor specific min-sample-time.
What's the reason for that? Property without vendor prefix would be the generic one, which should be defined by generic ADC bindings, not vf610-adc.txt.
The reason for going with a generic one was discussed in the first series, as though there might not be any devices at the moment using this property, some might appear or use it in the future.
I did consider putting this in the generic ADC bindings, however for lack of one like one present for touchscreen, I kept it in the vf610 specific driver binding. Thought there should not be any harm with this.
- Sanchayan.
Quoted 27 lines
Shawn
Quoted 24 lines
Version 1 of the patchset can be found here
http://lkml.iu.edu/hypermail/linux/kernel/1506.1/00026.html- Sanchayan.
Sanchayan Maity (2):
iio: adc: Determine sampling frequencies by using minimum sample time
ARM: dts: vfxxx: Add property for minimum sample time.../devicetree/bindings/iio/adc/vf610-adc.txt | 6 ++ arch/arm/boot/dts/vfxxx.dtsi | 2 + drivers/iio/adc/vf610_adc.c | 74 ++++++++++++++++++++-- 3 files changed, 78 insertions(+), 4 deletions(-)Signature · 7 lines
2.4.4 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/