[PATCH v2 0/2] Implement sample time consideration for Vybrid's ADC

[PATCH v2 0/2] Implement sample time consideration for Vybrid's ADC

[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

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(-)

-- 
2.4.4

[PATCH v2 1/2] iio: adc: Determine sampling frequencies by using minimum sample time

[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.

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 <maitysanchayan@gmail.com>
---
 .../devicetree/bindings/iio/adc/vf610-adc.txt      |  6 ++
 drivers/iio/adc/vf610_adc.c                        | 74 ++++++++++++++++++++--
 2 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
index 3eb40e2..39b86ea 100644
--- a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
+++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
@@ -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@4003b000 {
@@ -27,5 +32,6 @@ adc0: adc@4003b000 {
 	clock-names = "adc";
 	fsl,adck-max-frequency = <30000000>, <40000000>,
 				<20000000>;
+	min-sample-time = <1000>;
 	vref-supply = <&reg_vcc_3v3_mcu>;
 };
diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
index 480f335..8322027 100644
--- a/drivers/iio/adc/vf610_adc.c
+++ b/drivers/iio/adc/vf610_adc.c
@@ -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);
 
-- 
2.4.4

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

[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 <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>;
 			};
 
 			esdhc1: esdhc@400b2000 {
-- 
2.4.4

Re: [PATCH v2 1/2] iio: adc: Determine sampling frequencies by using minimum sample time

[Jonathan Cameron]

On 24/06/15 09:33, Sanchayan Maity wrote:
> 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.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
Looking for an Ack on this from Fugang. Device tree ack also good ;)
> ---
>  .../devicetree/bindings/iio/adc/vf610-adc.txt      |  6 ++
>  drivers/iio/adc/vf610_adc.c                        | 74 ++++++++++++++++++++--
>  2 files changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> index 3eb40e2..39b86ea 100644
> --- a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> @@ -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@4003b000 {
> @@ -27,5 +32,6 @@ adc0: adc@4003b000 {
>  	clock-names = "adc";
>  	fsl,adck-max-frequency = <30000000>, <40000000>,
>  				<20000000>;
> +	min-sample-time = <1000>;
>  	vref-supply = <&reg_vcc_3v3_mcu>;
>  };
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index 480f335..8322027 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -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 0/2] Implement sample time consideration for Vybrid's ADC

[Duan Andy]

From: Sanchayan Maity <maitysanchayan@gmail.com> Sent: Wednesday, June 24, 2015 4:34 PM
> To: jic23@kernel.org
> Cc: shawn.guo@linaro.org; kernel@pengutronix.de; robh+dt@kernel.org;
> pawel.moll@arm.com; mark.rutland@arm.com; ijc+devicetree@hellion.org.uk;
> galak@codeaurora.org; Duan Fugang-B38611; devicetree@vger.kernel.org;
> linux-iio@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux-
> kernel@vger.kernel.org; stefan@agner.ch; Sanchayan Maity
> Subject: [PATCH v2 0/2] 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(-)

The patch set looks fine. Thanks Sanchayan add this new interface for user that is more options for user case.

Acked-by: Fugang Duan <B38611@freescale.com>

Re: [PATCH v2 1/2] iio: adc: Determine sampling frequencies by using minimum sample time

[Stefan Agner]

On 2015-06-24 10:33, Sanchayan Maity wrote:
> 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.
> 
> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>

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.

Acked-by: Stefan Agner <stefan@agner.ch>


> ---
>  .../devicetree/bindings/iio/adc/vf610-adc.txt      |  6 ++
>  drivers/iio/adc/vf610_adc.c                        | 74 ++++++++++++++++++++--
>  2 files changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> index 3eb40e2..39b86ea 100644
> --- a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> +++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> @@ -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@4003b000 {
> @@ -27,5 +32,6 @@ adc0: adc@4003b000 {
>  	clock-names = "adc";
>  	fsl,adck-max-frequency = <30000000>, <40000000>,
>  				<20000000>;
> +	min-sample-time = <1000>;
>  	vref-supply = <&reg_vcc_3v3_mcu>;
>  };
> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> index 480f335..8322027 100644
> --- a/drivers/iio/adc/vf610_adc.c
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -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 0/2] Implement sample time consideration for Vybrid's ADC

[Shawn Guo]

On Wed, Jun 24, 2015 at 02:03:39PM +0530, Sanchayan Maity wrote:
> 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.

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

> 
> 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(-)
> 
> -- 
> 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/
> 

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

[Shawn Guo]

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/
> 

Re: [PATCH v2 0/2] Implement sample time consideration for Vybrid's ADC

[maitysanchayan]

Hello Shawn,

On 15-07-10 16:47:04, Shawn Guo wrote:
> On Wed, Jun 24, 2015 at 02:03:39PM +0530, Sanchayan Maity wrote:
> > 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.
> 
> 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.

> 
> Shawn
> 
> > 
> > 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(-)
> > 
> > -- 
> > 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/
> > 

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

[maitysanchayan]

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.

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

[Jonathan Cameron]

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.
> 

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

[maitysanchayan]

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?

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

[Stefan Agner]

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

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

[Jonathan Cameron]

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.