[PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for tscadc

[PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for tscadc

[Vignesh R]

Hi,

This patch adds optional DT properties for tscadc to set open delay,
sample delay and number of averages per sample for each adc channel.
Open delay, sample delay and averaging are some of the parameters that
affect the sampling rate and accuracy of the tscadc. Decreasing delays
and averaging helps to achieve higher sampling rates, while increasing
this parameters provides greater accuracy. Hence, this patch provides DT
properties to set the initial values for delays and number of averages
per sample. User space control via sysfs can be added later.


Vignesh R (2):
  iio: adc: ti_am335x_adc: refactor DT parsing into a function
  iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT
    parameters

 .../bindings/input/touchscreen/ti-tsc-adc.txt      | 24 +++++++
 drivers/iio/adc/ti_am335x_adc.c                    | 83 +++++++++++++++++-----
 2 files changed, 91 insertions(+), 16 deletions(-)

-- 
1.9.1

[PATCH 1/2] iio: adc: ti_am335x_adc: refactor DT parsing into a function

[Vignesh R]

Refactor DT parsing into a separate function from probe() to
help addition of more DT parameters later.

No functional changes.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 2e5cc4409f78..2f818405ffbe 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -396,16 +396,30 @@ static const struct iio_info tiadc_info = {
 	.driver_module = THIS_MODULE,
 };
 
+static int tiadc_parse_dt(struct platform_device *pdev,
+			  struct tiadc_device *adc_dev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct property *prop;
+	const __be32 *cur;
+	int channels = 0;
+	u32 val;
+
+	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
+		adc_dev->channel_line[channels] = val;
+		channels++;
+	}
+
+	adc_dev->channels = channels;
+	return 0;
+}
+
 static int tiadc_probe(struct platform_device *pdev)
 {
 	struct iio_dev		*indio_dev;
 	struct tiadc_device	*adc_dev;
 	struct device_node	*node = pdev->dev.of_node;
-	struct property		*prop;
-	const __be32		*cur;
 	int			err;
-	u32			val;
-	int			channels = 0;
 
 	if (!node) {
 		dev_err(&pdev->dev, "Could not find valid DT data.\n");
@@ -421,12 +435,7 @@ static int tiadc_probe(struct platform_device *pdev)
 	adc_dev = iio_priv(indio_dev);
 
 	adc_dev->mfd_tscadc = ti_tscadc_dev_get(pdev);
-
-	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
-		adc_dev->channel_line[channels] = val;
-		channels++;
-	}
-	adc_dev->channels = channels;
+	tiadc_parse_dt(pdev, adc_dev);
 
 	indio_dev->dev.parent = &pdev->dev;
 	indio_dev->name = dev_name(&pdev->dev);
-- 
1.9.1

Re: [PATCH 1/2] iio: adc: ti_am335x_adc: refactor DT parsing into a function

[Jonathan Cameron]

On 31/03/15 12:12, Vignesh R wrote:
> Refactor DT parsing into a separate function from probe() to
> help addition of more DT parameters later.
> 
> No functional changes.
> 
> Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
Clearly going to need this whatever the outcome of the second patch being
reviewed.

Applied to the togreg branch of iio.git - initially pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/adc/ti_am335x_adc.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 2e5cc4409f78..2f818405ffbe 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -396,16 +396,30 @@ static const struct iio_info tiadc_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +static int tiadc_parse_dt(struct platform_device *pdev,
> +			  struct tiadc_device *adc_dev)
> +{
> +	struct device_node *node = pdev->dev.of_node;
> +	struct property *prop;
> +	const __be32 *cur;
> +	int channels = 0;
> +	u32 val;
> +
> +	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
> +		adc_dev->channel_line[channels] = val;
> +		channels++;
> +	}
> +
> +	adc_dev->channels = channels;
> +	return 0;
> +}
> +
>  static int tiadc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev		*indio_dev;
>  	struct tiadc_device	*adc_dev;
>  	struct device_node	*node = pdev->dev.of_node;
> -	struct property		*prop;
> -	const __be32		*cur;
>  	int			err;
> -	u32			val;
> -	int			channels = 0;
>  
>  	if (!node) {
>  		dev_err(&pdev->dev, "Could not find valid DT data.\n");
> @@ -421,12 +435,7 @@ static int tiadc_probe(struct platform_device *pdev)
>  	adc_dev = iio_priv(indio_dev);
>  
>  	adc_dev->mfd_tscadc = ti_tscadc_dev_get(pdev);
> -
> -	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
> -		adc_dev->channel_line[channels] = val;
> -		channels++;
> -	}
> -	adc_dev->channels = channels;
> +	tiadc_parse_dt(pdev, adc_dev);
>  
>  	indio_dev->dev.parent = &pdev->dev;
>  	indio_dev->name = dev_name(&pdev->dev);
> 

[PATCH 2/2] iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT parameters

[Vignesh R]

Add optional DT properties to set open delay, sample delay and number
of averages per sample for each adc step. Open delay, sample delay
and averaging are some of the parameters that affect the sampling rate
and accuracy of the sample. Making these parameters configurable via
DT will help in balancing speed vs accuracy.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 .../bindings/input/touchscreen/ti-tsc-adc.txt      | 24 ++++++++++
 drivers/iio/adc/ti_am335x_adc.c                    | 54 +++++++++++++++++++---
 2 files changed, 72 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index 6c4fb34823d3..8aafbe87f0eb 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -42,6 +42,27 @@ Optional properties:
 			 hardware knob for adjusting the amount of "settling
 			 time".
 
+- child "adc"
+	ti,chan-step-opendelay: List of open delays for each channel of
+				ADC in the order of ti,adc-channels. The
+				value corresponds to the number of ADC
+				clock cycles to wait after applying the
+				step configuration registers and before
+				sending the start of ADC conversion.
+				Maximum value is 0x3FFFF.
+       ti,chan-step-sampledelay: List of sample delays for each channel
+				  of ADC in the order of ti,adc-channels.
+				  The value corresponds to the number of
+				  ADC clock cycles to sample (to hold
+				  start of conversion high).
+				  Maximum value is 0xFF.
+       ti,chan-step-avg: Number of averages to be performed for each
+			  channel of ADC. If average is 16 then input
+			  is sampled 16 times and averaged to get more
+			  accurate value. This increases the time taken
+			  by ADC to generate a sample. Valid range is 0
+			  average to 16 averages. Maximum value is 16.
+
 Example:
 	tscadc: tscadc@44e0d000 {
 		compatible = "ti,am3359-tscadc";
@@ -55,5 +76,8 @@ Example:
 
 		adc {
 			ti,adc-channels = <4 5 6 7>;
+			ti,chan-step-opendelay = <0x098 0x3ffff 0x098 0x0>;
+			ti,chan-step-sampledelay = <0xff 0x0 0xf 0x0>;
+			ti,chan-step-avg = <16 2 4 8>;
 		};
 	}
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 2f818405ffbe..5ee597b4a1af 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -37,6 +37,7 @@ struct tiadc_device {
 	u8 channel_step[8];
 	int buffer_en_ch_steps;
 	u16 data[8];
+	u32 open_delay[8], sample_delay[8], step_avg[8];
 };
 
 static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
@@ -85,6 +86,7 @@ static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
 static void tiadc_step_config(struct iio_dev *indio_dev)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
+	struct device *dev = adc_dev->mfd_tscadc->dev;
 	unsigned int stepconfig;
 	int i, steps = 0;
 
@@ -98,20 +100,47 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 	 * needs to be given to ADC to digitalize data.
 	 */
 
-	if (iio_buffer_enabled(indio_dev))
-		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
-					| STEPCONFIG_MODE_SWCNT;
-	else
-		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
 
 	for (i = 0; i < adc_dev->channels; i++) {
 		int chan;
 
 		chan = adc_dev->channel_line[i];
+
+		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
+			dev_warn(dev, "chan %d step_avg truncating to %d\n",
+				 chan, STEPCONFIG_AVG_16);
+			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
+		}
+
+		if (adc_dev->step_avg[i])
+			stepconfig =
+			STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
+			STEPCONFIG_FIFO1;
+		else
+			stepconfig = STEPCONFIG_FIFO1;
+
+		if (iio_buffer_enabled(indio_dev))
+			stepconfig |= STEPCONFIG_MODE_SWCNT;
+
 		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
 				stepconfig | STEPCONFIG_INP(chan));
+
+		if (adc_dev->open_delay[i] > STEPDELAY_OPEN_MASK) {
+			dev_warn(dev, "chan %d open delay truncating to 0x3FFFF\n",
+				 chan);
+			adc_dev->open_delay[i] = STEPDELAY_OPEN_MASK;
+		}
+
+		if (adc_dev->sample_delay[i] > 0xFF) {
+			dev_warn(dev, "chan %d sample delay truncating to 0xFF\n",
+				 chan);
+			adc_dev->sample_delay[i] = 0xFF;
+		}
+
 		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
-				STEPCONFIG_OPENDLY);
+				STEPDELAY_OPEN(adc_dev->open_delay[i]) |
+				STEPDELAY_SAMPLE(adc_dev->sample_delay[i]));
+
 		adc_dev->channel_step[i] = steps;
 		steps++;
 	}
@@ -407,9 +436,22 @@ static int tiadc_parse_dt(struct platform_device *pdev,
 
 	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
 		adc_dev->channel_line[channels] = val;
+
+		/* Set Default values for optional DT parameters */
+		adc_dev->open_delay[channels] = STEPCONFIG_OPENDLY;
+		adc_dev->sample_delay[channels] = STEPCONFIG_SAMPLEDLY;
+		adc_dev->step_avg[channels] = 16;
+
 		channels++;
 	}
 
+	of_property_read_u32_array(node, "ti,chan-step-avg",
+				   adc_dev->step_avg, channels);
+	of_property_read_u32_array(node, "ti,chan-step-opendelay",
+				   adc_dev->open_delay, channels);
+	of_property_read_u32_array(node, "ti,chan-step-sampledelay",
+				   adc_dev->sample_delay, channels);
+
 	adc_dev->channels = channels;
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 2/2] iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT parameters

[Jonathan Cameron]

On 31/03/15 12:12, Vignesh R wrote:
> Add optional DT properties to set open delay, sample delay and number
> of averages per sample for each adc step. Open delay, sample delay
> and averaging are some of the parameters that affect the sampling rate
> and accuracy of the sample. Making these parameters configurable via
> DT will help in balancing speed vs accuracy.
> 
> Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
All looks fine to me, but I would ideally like a devicetree
ack on this one.

Jonathan
> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      | 24 ++++++++++
>  drivers/iio/adc/ti_am335x_adc.c                    | 54 +++++++++++++++++++---
>  2 files changed, 72 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> index 6c4fb34823d3..8aafbe87f0eb 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -42,6 +42,27 @@ Optional properties:
>  			 hardware knob for adjusting the amount of "settling
>  			 time".
>  
> +- child "adc"
> +	ti,chan-step-opendelay: List of open delays for each channel of
> +				ADC in the order of ti,adc-channels. The
> +				value corresponds to the number of ADC
> +				clock cycles to wait after applying the
> +				step configuration registers and before
> +				sending the start of ADC conversion.
> +				Maximum value is 0x3FFFF.
> +       ti,chan-step-sampledelay: List of sample delays for each channel
> +				  of ADC in the order of ti,adc-channels.
> +				  The value corresponds to the number of
> +				  ADC clock cycles to sample (to hold
> +				  start of conversion high).
> +				  Maximum value is 0xFF.
> +       ti,chan-step-avg: Number of averages to be performed for each
> +			  channel of ADC. If average is 16 then input
> +			  is sampled 16 times and averaged to get more
> +			  accurate value. This increases the time taken
> +			  by ADC to generate a sample. Valid range is 0
> +			  average to 16 averages. Maximum value is 16.
> +
>  Example:
>  	tscadc: tscadc@44e0d000 {
>  		compatible = "ti,am3359-tscadc";
> @@ -55,5 +76,8 @@ Example:
>  
>  		adc {
>  			ti,adc-channels = <4 5 6 7>;
> +			ti,chan-step-opendelay = <0x098 0x3ffff 0x098 0x0>;
> +			ti,chan-step-sampledelay = <0xff 0x0 0xf 0x0>;
> +			ti,chan-step-avg = <16 2 4 8>;
>  		};
>  	}
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 2f818405ffbe..5ee597b4a1af 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -37,6 +37,7 @@ struct tiadc_device {
>  	u8 channel_step[8];
>  	int buffer_en_ch_steps;
>  	u16 data[8];
> +	u32 open_delay[8], sample_delay[8], step_avg[8];
>  };
>  
>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
> @@ -85,6 +86,7 @@ static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
>  static void tiadc_step_config(struct iio_dev *indio_dev)
>  {
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> +	struct device *dev = adc_dev->mfd_tscadc->dev;
>  	unsigned int stepconfig;
>  	int i, steps = 0;
>  
> @@ -98,20 +100,47 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>  	 * needs to be given to ADC to digitalize data.
>  	 */
>  
> -	if (iio_buffer_enabled(indio_dev))
> -		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
> -					| STEPCONFIG_MODE_SWCNT;
> -	else
> -		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>  
>  	for (i = 0; i < adc_dev->channels; i++) {
>  		int chan;
>  
>  		chan = adc_dev->channel_line[i];
> +
> +		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
> +			dev_warn(dev, "chan %d step_avg truncating to %d\n",
> +				 chan, STEPCONFIG_AVG_16);
> +			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
> +		}
> +
> +		if (adc_dev->step_avg[i])
> +			stepconfig =
> +			STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
> +			STEPCONFIG_FIFO1;
> +		else
> +			stepconfig = STEPCONFIG_FIFO1;
> +
> +		if (iio_buffer_enabled(indio_dev))
> +			stepconfig |= STEPCONFIG_MODE_SWCNT;
> +
>  		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
>  				stepconfig | STEPCONFIG_INP(chan));
> +
> +		if (adc_dev->open_delay[i] > STEPDELAY_OPEN_MASK)alue corresponds to the number of ADC
> +				clock cycles to wait after applying the
> +				step configuration registers and before
> +				sending the start of ADC conversion.
> +				Maximum value is 0x3FFFF.
> +       ti,chan-step-sampledelay: List of sample delays for each channel
> +				  of ADC in the order of ti,adc-channels.
> +				  The value corresponds to the number of
> +				  ADC clock cycles to sample (to hold
> +				  start of conversion high).
> +				  Maximum value is 0xFF.
> +       ti,chan-step-avg: Number of averages to be performed for each
> +			  channel of ADC. If average is 16 then input
> +			  is sampled 16 times and averaged to get more
> +			  accurate value. This increases the time taken
> +			  by ADC to generate a sample. Valid range is 0
> +			  average to 16 averages. Maximum value is 16. {
> +			dev_warn(dev, "chan %d open delay truncating to 0x3FFFF\n",
> +				 chan);
> +			adc_dev->open_delay[i] = STEPDELAY_OPEN_MASK;
> +		}
> +
> +		if (adc_dev->sample_delay[i] > 0xFF) {
> +			dev_warn(dev, "chan %d sample delay truncating to 0xFF\n",
> +				 chan);
> +			adc_dev->sample_delay[i] = 0xFF;
> +		}
> +
>  		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
> -				STEPCONFIG_OPENDLY);
> +				STEPDELAY_OPEN(adc_dev->open_delay[i]) |
> +				STEPDELAY_SAMPLE(adc_dev->sample_delay[i]));
> +
>  		adc_dev->channel_step[i] = steps;
>  		steps++;
>  	}
> @@ -407,9 +436,22 @@ static int tiadc_parse_dt(struct platform_device *pdev,
>  
>  	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
>  		adc_dev->channel_line[channels] = val;
> +
> +		/* Set Default values for optional DT parameters */
> +		adc_dev->open_delay[channels] = STEPCONFIG_OPENDLY;
> +		adc_dev->sample_delay[channels] = STEPCONFIG_SAMPLEDLY;
> +		adc_dev->step_avg[channels] = 16;
> +
>  		channels++;
>  	}
>  
> +	of_property_read_u32_array(node, "ti,chan-step-avg",
> +				   adc_dev->step_avg, channels);
> +	of_property_read_u32_array(node, "ti,chan-step-opendelay",
> +				   adc_dev->open_delay, channels);
> +	of_property_read_u32_array(node, "ti,chan-step-sampledelay",
> +				   adc_dev->sample_delay, channels);
> +
>  	adc_dev->channels = channels;
>  	return 0;
>  }
> 

Re: [PATCH 2/2] iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT parameters

[Vignesh R]

On Thursday 09 April 2015 07:49 PM, Jonathan Cameron wrote:
> On 31/03/15 12:12, Vignesh R wrote:
>> Add optional DT properties to set open delay, sample delay and number
>> of averages per sample for each adc step. Open delay, sample delay
>> and averaging are some of the parameters that affect the sampling rate
>> and accuracy of the sample. Making these parameters configurable via
>> DT will help in balancing speed vs accuracy.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
> All looks fine to me, but I would ideally like a devicetree
> ack on this one.
> 
> Jonathan

Gentle ping...

Regards
Vignesh
>> ---
>>  .../bindings/input/touchscreen/ti-tsc-adc.txt      | 24 ++++++++++
>>  drivers/iio/adc/ti_am335x_adc.c                    | 54 +++++++++++++++++++---
>>  2 files changed, 72 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> index 6c4fb34823d3..8aafbe87f0eb 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> @@ -42,6 +42,27 @@ Optional properties:
>>  			 hardware knob for adjusting the amount of "settling
>>  			 time".
>>  
>> +- child "adc"
>> +	ti,chan-step-opendelay: List of open delays for each channel of
>> +				ADC in the order of ti,adc-channels. The
>> +				value corresponds to the number of ADC
>> +				clock cycles to wait after applying the
>> +				step configuration registers and before
>> +				sending the start of ADC conversion.
>> +				Maximum value is 0x3FFFF.
>> +       ti,chan-step-sampledelay: List of sample delays for each channel
>> +				  of ADC in the order of ti,adc-channels.
>> +				  The value corresponds to the number of
>> +				  ADC clock cycles to sample (to hold
>> +				  start of conversion high).
>> +				  Maximum value is 0xFF.
>> +       ti,chan-step-avg: Number of averages to be performed for each
>> +			  channel of ADC. If average is 16 then input
>> +			  is sampled 16 times and averaged to get more
>> +			  accurate value. This increases the time taken
>> +			  by ADC to generate a sample. Valid range is 0
>> +			  average to 16 averages. Maximum value is 16.
>> +
>>  Example:
>>  	tscadc: tscadc@44e0d000 {
>>  		compatible = "ti,am3359-tscadc";
>> @@ -55,5 +76,8 @@ Example:
>>  
>>  		adc {
>>  			ti,adc-channels = <4 5 6 7>;
>> +			ti,chan-step-opendelay = <0x098 0x3ffff 0x098 0x0>;
>> +			ti,chan-step-sampledelay = <0xff 0x0 0xf 0x0>;
>> +			ti,chan-step-avg = <16 2 4 8>;
>>  		};
>>  	}
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index 2f818405ffbe..5ee597b4a1af 100644
>> --- a/drivers/iio/adc/ti_am335x_adc.c
>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>> @@ -37,6 +37,7 @@ struct tiadc_device {
>>  	u8 channel_step[8];
>>  	int buffer_en_ch_steps;
>>  	u16 data[8];
>> +	u32 open_delay[8], sample_delay[8], step_avg[8];
>>  };
>>  
>>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
>> @@ -85,6 +86,7 @@ static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
>>  static void tiadc_step_config(struct iio_dev *indio_dev)
>>  {
>>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>> +	struct device *dev = adc_dev->mfd_tscadc->dev;
>>  	unsigned int stepconfig;
>>  	int i, steps = 0;
>>  
>> @@ -98,20 +100,47 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>>  	 * needs to be given to ADC to digitalize data.
>>  	 */
>>  
>> -	if (iio_buffer_enabled(indio_dev))
>> -		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
>> -					| STEPCONFIG_MODE_SWCNT;
>> -	else
>> -		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>>  
>>  	for (i = 0; i < adc_dev->channels; i++) {
>>  		int chan;
>>  
>>  		chan = adc_dev->channel_line[i];
>> +
>> +		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
>> +			dev_warn(dev, "chan %d step_avg truncating to %d\n",
>> +				 chan, STEPCONFIG_AVG_16);
>> +			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
>> +		}
>> +
>> +		if (adc_dev->step_avg[i])
>> +			stepconfig =
>> +			STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
>> +			STEPCONFIG_FIFO1;
>> +		else
>> +			stepconfig = STEPCONFIG_FIFO1;
>> +
>> +		if (iio_buffer_enabled(indio_dev))
>> +			stepconfig |= STEPCONFIG_MODE_SWCNT;
>> +
>>  		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
>>  				stepconfig | STEPCONFIG_INP(chan));
>> +
>> +		if (adc_dev->open_delay[i] > STEPDELAY_OPEN_MASK)alue corresponds to the number of ADC
>> +				clock cycles to wait after applying the
>> +				step configuration registers and before
>> +				sending the start of ADC conversion.
>> +				Maximum value is 0x3FFFF.
>> +       ti,chan-step-sampledelay: List of sample delays for each channel
>> +				  of ADC in the order of ti,adc-channels.
>> +				  The value corresponds to the number of
>> +				  ADC clock cycles to sample (to hold
>> +				  start of conversion high).
>> +				  Maximum value is 0xFF.
>> +       ti,chan-step-avg: Number of averages to be performed for each
>> +			  channel of ADC. If average is 16 then input
>> +			  is sampled 16 times and averaged to get more
>> +			  accurate value. This increases the time taken
>> +			  by ADC to generate a sample. Valid range is 0
>> +			  average to 16 averages. Maximum value is 16. {
>> +			dev_warn(dev, "chan %d open delay truncating to 0x3FFFF\n",
>> +				 chan);
>> +			adc_dev->open_delay[i] = STEPDELAY_OPEN_MASK;
>> +		}
>> +
>> +		if (adc_dev->sample_delay[i] > 0xFF) {
>> +			dev_warn(dev, "chan %d sample delay truncating to 0xFF\n",
>> +				 chan);
>> +			adc_dev->sample_delay[i] = 0xFF;
>> +		}
>> +
>>  		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
>> -				STEPCONFIG_OPENDLY);
>> +				STEPDELAY_OPEN(adc_dev->open_delay[i]) |
>> +				STEPDELAY_SAMPLE(adc_dev->sample_delay[i]));
>> +
>>  		adc_dev->channel_step[i] = steps;
>>  		steps++;
>>  	}
>> @@ -407,9 +436,22 @@ static int tiadc_parse_dt(struct platform_device *pdev,
>>  
>>  	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
>>  		adc_dev->channel_line[channels] = val;
>> +
>> +		/* Set Default values for optional DT parameters */
>> +		adc_dev->open_delay[channels] = STEPCONFIG_OPENDLY;
>> +		adc_dev->sample_delay[channels] = STEPCONFIG_SAMPLEDLY;
>> +		adc_dev->step_avg[channels] = 16;
>> +
>>  		channels++;
>>  	}
>>  
>> +	of_property_read_u32_array(node, "ti,chan-step-avg",
>> +				   adc_dev->step_avg, channels);
>> +	of_property_read_u32_array(node, "ti,chan-step-opendelay",
>> +				   adc_dev->open_delay, channels);
>> +	of_property_read_u32_array(node, "ti,chan-step-sampledelay",
>> +				   adc_dev->sample_delay, channels);
>> +
>>  	adc_dev->channels = channels;
>>  	return 0;
>>  }
>>
> 

Re: [PATCH 2/2] iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT parameters

[Jonathan Cameron]

On 13/05/15 08:42, Vignesh R wrote:
> 
> 
> On Thursday 09 April 2015 07:49 PM, Jonathan Cameron wrote:
>> On 31/03/15 12:12, Vignesh R wrote:
>>> Add optional DT properties to set open delay, sample delay and number
>>> of averages per sample for each adc step. Open delay, sample delay
>>> and averaging are some of the parameters that affect the sampling rate
>>> and accuracy of the sample. Making these parameters configurable via
>>> DT will help in balancing speed vs accuracy.
>>>
>>> Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
>> All looks fine to me, but I would ideally like a devicetree
>> ack on this one.
>>
>> Jonathan
> 
> Gentle ping...
Thanks. Had forgotten about this one.  Ah well, no device tree response...

Applied to the togreg branch of iio.git, initially pushed out as testing.

Thanks,

Jonathan
> 
> Regards
> Vignesh
>>> ---
>>>  .../bindings/input/touchscreen/ti-tsc-adc.txt      | 24 ++++++++++
>>>  drivers/iio/adc/ti_am335x_adc.c                    | 54 +++++++++++++++++++---
>>>  2 files changed, 72 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>>> index 6c4fb34823d3..8aafbe87f0eb 100644
>>> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>>> @@ -42,6 +42,27 @@ Optional properties:
>>>  			 hardware knob for adjusting the amount of "settling
>>>  			 time".
>>>  
>>> +- child "adc"
>>> +	ti,chan-step-opendelay: List of open delays for each channel of
>>> +				ADC in the order of ti,adc-channels. The
>>> +				value corresponds to the number of ADC
>>> +				clock cycles to wait after applying the
>>> +				step configuration registers and before
>>> +				sending the start of ADC conversion.
>>> +				Maximum value is 0x3FFFF.
>>> +       ti,chan-step-sampledelay: List of sample delays for each channel
>>> +				  of ADC in the order of ti,adc-channels.
>>> +				  The value corresponds to the number of
>>> +				  ADC clock cycles to sample (to hold
>>> +				  start of conversion high).
>>> +				  Maximum value is 0xFF.
>>> +       ti,chan-step-avg: Number of averages to be performed for each
>>> +			  channel of ADC. If average is 16 then input
>>> +			  is sampled 16 times and averaged to get more
>>> +			  accurate value. This increases the time taken
>>> +			  by ADC to generate a sample. Valid range is 0
>>> +			  average to 16 averages. Maximum value is 16.
>>> +
>>>  Example:
>>>  	tscadc: tscadc@44e0d000 {
>>>  		compatible = "ti,am3359-tscadc";
>>> @@ -55,5 +76,8 @@ Example:
>>>  
>>>  		adc {
>>>  			ti,adc-channels = <4 5 6 7>;
>>> +			ti,chan-step-opendelay = <0x098 0x3ffff 0x098 0x0>;
>>> +			ti,chan-step-sampledelay = <0xff 0x0 0xf 0x0>;
>>> +			ti,chan-step-avg = <16 2 4 8>;
>>>  		};
>>>  	}
>>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>>> index 2f818405ffbe..5ee597b4a1af 100644
>>> --- a/drivers/iio/adc/ti_am335x_adc.c
>>> +++ b/drivers/iio/adc/ti_am335x_adc.c
>>> @@ -37,6 +37,7 @@ struct tiadc_device {
>>>  	u8 channel_step[8];
>>>  	int buffer_en_ch_steps;
>>>  	u16 data[8];
>>> +	u32 open_delay[8], sample_delay[8], step_avg[8];
>>>  };
>>>  
>>>  static unsigned int tiadc_readl(struct tiadc_device *adc, unsigned int reg)
>>> @@ -85,6 +86,7 @@ static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
>>>  static void tiadc_step_config(struct iio_dev *indio_dev)
>>>  {
>>>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>>> +	struct device *dev = adc_dev->mfd_tscadc->dev;
>>>  	unsigned int stepconfig;
>>>  	int i, steps = 0;
>>>  
>>> @@ -98,20 +100,47 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>>>  	 * needs to be given to ADC to digitalize data.
>>>  	 */
>>>  
>>> -	if (iio_buffer_enabled(indio_dev))
>>> -		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1
>>> -					| STEPCONFIG_MODE_SWCNT;
>>> -	else
>>> -		stepconfig = STEPCONFIG_AVG_16 | STEPCONFIG_FIFO1;
>>>  
>>>  	for (i = 0; i < adc_dev->channels; i++) {
>>>  		int chan;
>>>  
>>>  		chan = adc_dev->channel_line[i];
>>> +
>>> +		if (adc_dev->step_avg[i] > STEPCONFIG_AVG_16) {
>>> +			dev_warn(dev, "chan %d step_avg truncating to %d\n",
>>> +				 chan, STEPCONFIG_AVG_16);
>>> +			adc_dev->step_avg[i] = STEPCONFIG_AVG_16;
>>> +		}
>>> +
>>> +		if (adc_dev->step_avg[i])
>>> +			stepconfig =
>>> +			STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
>>> +			STEPCONFIG_FIFO1;
>>> +		else
>>> +			stepconfig = STEPCONFIG_FIFO1;
>>> +
>>> +		if (iio_buffer_enabled(indio_dev))
>>> +			stepconfig |= STEPCONFIG_MODE_SWCNT;
>>> +
>>>  		tiadc_writel(adc_dev, REG_STEPCONFIG(steps),
>>>  				stepconfig | STEPCONFIG_INP(chan));
>>> +
>>> +		if (adc_dev->open_delay[i] > STEPDELAY_OPEN_MASK)alue corresponds to the number of ADC
>>> +				clock cycles to wait after applying the
>>> +				step configuration registers and before
>>> +				sending the start of ADC conversion.
>>> +				Maximum value is 0x3FFFF.
>>> +       ti,chan-step-sampledelay: List of sample delays for each channel
>>> +				  of ADC in the order of ti,adc-channels.
>>> +				  The value corresponds to the number of
>>> +				  ADC clock cycles to sample (to hold
>>> +				  start of conversion high).
>>> +				  Maximum value is 0xFF.
>>> +       ti,chan-step-avg: Number of averages to be performed for each
>>> +			  channel of ADC. If average is 16 then input
>>> +			  is sampled 16 times and averaged to get more
>>> +			  accurate value. This increases the time taken
>>> +			  by ADC to generate a sample. Valid range is 0
>>> +			  average to 16 averages. Maximum value is 16. {
>>> +			dev_warn(dev, "chan %d open delay truncating to 0x3FFFF\n",
>>> +				 chan);
>>> +			adc_dev->open_delay[i] = STEPDELAY_OPEN_MASK;
>>> +		}
>>> +
>>> +		if (adc_dev->sample_delay[i] > 0xFF) {
>>> +			dev_warn(dev, "chan %d sample delay truncating to 0xFF\n",
>>> +				 chan);
>>> +			adc_dev->sample_delay[i] = 0xFF;
>>> +		}
>>> +
>>>  		tiadc_writel(adc_dev, REG_STEPDELAY(steps),
>>> -				STEPCONFIG_OPENDLY);
>>> +				STEPDELAY_OPEN(adc_dev->open_delay[i]) |
>>> +				STEPDELAY_SAMPLE(adc_dev->sample_delay[i]));
>>> +
>>>  		adc_dev->channel_step[i] = steps;
>>>  		steps++;
>>>  	}
>>> @@ -407,9 +436,22 @@ static int tiadc_parse_dt(struct platform_device *pdev,
>>>  
>>>  	of_property_for_each_u32(node, "ti,adc-channels", prop, cur, val) {
>>>  		adc_dev->channel_line[channels] = val;
>>> +
>>> +		/* Set Default values for optional DT parameters */
>>> +		adc_dev->open_delay[channels] = STEPCONFIG_OPENDLY;
>>> +		adc_dev->sample_delay[channels] = STEPCONFIG_SAMPLEDLY;
>>> +		adc_dev->step_avg[channels] = 16;
>>> +
>>>  		channels++;
>>>  	}
>>>  
>>> +	of_property_read_u32_array(node, "ti,chan-step-avg",
>>> +				   adc_dev->step_avg, channels);
>>> +	of_property_read_u32_array(node, "ti,chan-step-opendelay",
>>> +				   adc_dev->open_delay, channels);
>>> +	of_property_read_u32_array(node, "ti,chan-step-sampledelay",
>>> +				   adc_dev->sample_delay, channels);
>>> +
>>>  	adc_dev->channels = channels;
>>>  	return 0;
>>>  }
>>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Fwd: Fwd: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for tscadc

[Hannes Petermaier]

Hi Vignesh,

did you recognize my last email ? whats your opinion around that =

best regards,
Hannes
----- Forwarded by Hannes Petermaier/Eggelsberg/AT/B&R on 14.04.2015 13:14 
-----

Hannes Petermaier/Eggelsberg/AT/B&R schrieb am 07.04.2015 14:30:19:

> From: Hannes Petermaier/Eggelsberg/AT/B&R
> To: vigneshr-l0cyMroinI0@public.gmane.org
> Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Date: 07.04.2015 14:30
> Subject: WG: Fwd: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT 
properties for tscadc
> 
> Hi Vignesh,
> 
> would it be possible to add some more channel-specific settings ?
> 
> It would be nice to have allmost full control to the STEPCONFIGx 
register.
> 
> At least we need to write the bits
> 
> SEL_RFM_SWC_1_0
> SEL_INM_SWC_3_0 
> SEL_RFP_SWC_2_0 
> 
> In the current mainline version only (SEL_INP_SWC_3_0) is written.
> So for the other bits "0" is value is used, for my point of view this is 
not correct.
> 
> For example if we want to read a value from AIN5 the negative pin from 
adc is 
> muxed allways to AIN0.
> In fact i can readout heavy jitter even if AIN5 is connected to ground - 
after
> setting up negative adc pin within code (to use REFN)  the readout value 
is 0 
> as expected without nameable jitter.
> If i short AIN0 also to ground, jitter is also eliminated.
> 
> Maybe this is also some fault of TI SoC ... in normal case somebody 
could 
> expect, that negative adc pin is equal even the Diff_CNTRL bit isn't set 
- but
> in practice it isn't.
> 
> Also actually it isn't possible to make some accurate measurement due to 
the 
> fact that allways VDDA_ADC is used as positive reference.
> 
> So it would be nice to have control around this bits.
> Whats your opinion around that?
> 
> best regards,
> Hannes
> ----- Forwarded by Hannes Petermaier/Eggelsberg/AT/B&R on 07.04.2015 
14:17 -----
> 
> From: Johannes Pointner <johannes.pointner-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> To: hannes.petermaier-5O2GiQo/Ci2aMPzRcYMCawC/G2K4zDHf@public.gmane.org
> Date: 07.04.2015 13:58
> Subject: Fwd: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties 
for tscadc
> 
> ---------- Forwarded message ----------
> From: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
> Date: 2015-03-31 13:12 GMT+02:00
> Subject: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for 
tscadc
> To: Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>,
> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Ian Campbell
> <ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>,
> Jonathan Cameron <jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Cc: Hartmut Knaack <knaack.h-Mmb7MZpHnFY@public.gmane.org>, Lars-Peter Clausen
> <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>, Peter Meerwald <pmeerw-jW+XmwGofnusTnJN9+BGXg@public.gmane.org>, Dmitry Torokhov
> <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, Karol Wrona <k.wrona-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>, Jan
> Kardell <jan.kardell-KSZdJiTw9mzQT0dZR+AlfA@public.gmane.org>, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
> linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
> fcooper-l0cyMroinI0@public.gmane.org, Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
> 
> 
> 
> Hi,
> 
> This patch adds optional DT properties for tscadc to set open delay,
> sample delay and number of averages per sample for each adc channel.
> Open delay, sample delay and averaging are some of the parameters that
> affect the sampling rate and accuracy of the tscadc. Decreasing delays
> and averaging helps to achieve higher sampling rates, while increasing
> this parameters provides greater accuracy. Hence, this patch provides DT
> properties to set the initial values for delays and number of averages
> per sample. User space control via sysfs can be added later.
> 
> 
> Vignesh R (2):
>   iio: adc: ti_am335x_adc: refactor DT parsing into a function
>   iio: adc: ti_am335x_adc: make sample delay, open delay, averaging DT
>     parameters
> 
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      | 24 +++++++
>  drivers/iio/adc/ti_am335x_adc.c                    | 83 
+++++++++++++++++-----
>  2 files changed, 91 insertions(+), 16 deletions(-)
> 
> --
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for tscadc

[Vignesh R]

Hi Hannes,

On Tuesday 14 April 2015 04:46 PM, Hannes Petermaier wrote:
> Hi Vignesh,
> 
> did you recognize my last email ? whats your opinion around that =

Apologies...

> 
> best regards,
> Hannes
> ----- Forwarded by Hannes Petermaier/Eggelsberg/AT/B&R on 14.04.2015 13:14 
> -----
> 
> Hannes Petermaier/Eggelsberg/AT/B&R schrieb am 07.04.2015 14:30:19:
> 
>> From: Hannes Petermaier/Eggelsberg/AT/B&R
>> To: vigneshr-l0cyMroinI0@public.gmane.org
>> Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Date: 07.04.2015 14:30
>> Subject: WG: Fwd: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT 
> properties for tscadc
>>
>> Hi Vignesh,
>>
>> would it be possible to add some more channel-specific settings ?
>>
>> It would be nice to have allmost full control to the STEPCONFIGx 
> register.
>>
>> At least we need to write the bits
>>
>> SEL_RFM_SWC_1_0
>> SEL_INM_SWC_3_0 
>> SEL_RFP_SWC_2_0 
>>
>> In the current mainline version only (SEL_INP_SWC_3_0) is written.
>> So for the other bits "0" is value is used, for my point of view this is 
> not correct.
>>
>> For example if we want to read a value from AIN5 the negative pin from 
> adc is 
>> muxed allways to AIN0.

Sorry... I didn't understand what you meant by"AIN5 is muxed always with
AIN0"?

>> In fact i can readout heavy jitter even if AIN5 is connected to ground - 
> after
>> setting up negative adc pin within code (to use REFN)  the readout value 
> is 0 
>> as expected without nameable jitter.
>> If i short AIN0 also to ground, jitter is also eliminated.

Hmmm... nobody has reported such behavior before. ADC support for
am335x-evm/beaglebone has been there for quite long time, but nobody
reported any jitter on AIN5 line. I think this may be specific to
your setup. Can you provide more info with regard to your setup?
Which kernel? Is it am335x-evm or beaglebone or a custom board?

>>
>> Maybe this is also some fault of TI SoC ... in normal case somebody 
> could 
>> expect, that negative adc pin is equal even the Diff_CNTRL bit isn't set 
> - but
>> in practice it isn't.
>>
>> Also actually it isn't possible to make some accurate measurement due to 
> the 
>> fact that allways VDDA_ADC is used as positive reference.
>>
>> So it would be nice to have control around this bits.
>> Whats your opinion around that?

Sorry, I am not yet clear on your bug/use-case.

Please comment inline while replying on mailing list

Regards
Vignesh

Re: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for tscadc

[Hannes Petermaier]

> Hi Hannes,
Hi Vignesh,
thanks for answer.

> >>
> >> would it be possible to add some more channel-specific settings ?
> >>
> >> It would be nice to have allmost full control to the STEPCONFIGx 
> > register.
> >>
> >> At least we need to write the bits
> >>
> >> SEL_RFM_SWC_1_0
> >> SEL_INM_SWC_3_0 
> >> SEL_RFP_SWC_2_0 
> >>
> >> In the current mainline version only (SEL_INP_SWC_3_0) is written.
> >> So for the other bits "0" is value is used, for my point of view this 
is 
> > not correct.
> >>
> >> For example if we want to read a value from AIN5 the negative pin 
from 
> > adc is 
> >> muxed allways to AIN0.
> 
> Sorry... I didn't understand what you meant by"AIN5 is muxed always with
> AIN0"?
Have a look to the TRM (spruh73k.pdf) Page 1740 / Figure 12-2. Functional 
Block Diagram.
There you can see that the ADC-cell which has two inputs, one positive and 
one negative.
Also there are two reference inputs, one positive - one negative.

All this "pins" are muxed, because only one channel per time can be 
sampled.
This muxes are controlled through the STEPCONFIGx registers.

If you want for example take some measurement from AIN5 the driver muxes 
the positive input from the ADC to AIN5 by setting the bits for 
SEL_INP<3:0> - this is ok.
But the bits for SEL_INM<3:0> are still 'zero'. 
In summary this results in following mux-setting (regarding page 1771 in 
TRM):

positive-reference muxed to   VDDA
negative-reference muxed to   VSSA
positive-input muxed to       AIN5
negative-input muxed to       AIN0

>From this setup we run into 2 problems:
- the negative input terminal is muxed maybe to wrong potential
In much cases we have a single-ended signal so this setup looks good at 
first, because the "Diff_CNTRL" bit is also false. 
In fact there is an influence to the reading if the negative 
input-terminal isn't setup correctly (to VSSA or REFN). 
Maybe i interpret the "Diff_CNTRL" not correctly, there is no detailed 
description within the TRM - maybe some of your workmates can explain you 
the functionality of this bit.

- reference is allways taken from VDDA/VSSA
For a precision measurement you dont't use in normal case the 
analog-supply.
This rail brings noise, drift - all things whicht we don't need for 
accurate measurement.

> 
> >> In fact i can readout heavy jitter even if AIN5 is connected to 
ground - 
> > after
> >> setting up negative adc pin within code (to use REFN)  the readout 
value 
> > is 0 
> >> as expected without nameable jitter.
> >> If i short AIN0 also to ground, jitter is also eliminated.
> 
> Hmmm... nobody has reported such behavior before. ADC support for
> am335x-evm/beaglebone has been there for quite long time, but nobody
> reported any jitter on AIN5 line. I think this may be specific to
> your setup. Can you provide more info with regard to your setup?
> Which kernel? Is it am335x-evm or beaglebone or a custom board?
Maybe nobody does some precision measurement with beaglebone.
For operating some touchscreen or readout a potentiometer for evaluation 
purpose it is still good enough.

Kernel is current mainline, 4.0
Board is some custom board of my company.
But all this parameters shouldn't have some influence to the case.

> >>
> >> Maybe this is also some fault of TI SoC ... in normal case somebody 
> > could 
> >> expect, that negative adc pin is equal even the Diff_CNTRL bit isn't 
set 
> > - but
> >> in practice it isn't.
> >>
> >> Also actually it isn't possible to make some accurate measurement due 
to 
> > the 
> >> fact that allways VDDA_ADC is used as positive reference.
> >>
> >> So it would be nice to have control around this bits.
> >> Whats your opinion around that?
> 
> Sorry, I am not yet clear on your bug/use-case.
> 
> Please comment inline while replying on mailing list
okay so ?
 
> Regards
> Vignesh
best regards,
Hannes



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

Re: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for tscadc

[Vignesh R]

Hi Hannes,

On Tuesday 14 April 2015 04:46 PM, Hannes Petermaier wrote:
> Hi Vignesh,
> 
> did you recognize my last email ? whats your opinion around that =
> 
> best regards,
> Hannes
> ----- Forwarded by Hannes Petermaier/Eggelsberg/AT/B&R on 14.04.2015 13:14 
> -----
> 
> Hannes Petermaier/Eggelsberg/AT/B&R schrieb am 07.04.2015 14:30:19:
> 
>> From: Hannes Petermaier/Eggelsberg/AT/B&R
>> To: vigneshr-l0cyMroinI0@public.gmane.org
>> Cc: linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Date: 07.04.2015 14:30
>> Subject: WG: Fwd: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT 
> properties for tscadc
>>
>> Hi Vignesh,
>>
>> would it be possible to add some more channel-specific settings ?
>>
>> It would be nice to have allmost full control to the STEPCONFIGx 
> register.
>>
>> At least we need to write the bits
>>
>> SEL_RFM_SWC_1_0
>> SEL_INM_SWC_3_0 
>> SEL_RFP_SWC_2_0 
>>
>> In the current mainline version only (SEL_INP_SWC_3_0) is written.
>> So for the other bits "0" is value is used, for my point of view this is 
> not correct.
>>
>> For example if we want to read a value from AIN5 the negative pin from 
> adc is 
>> muxed allways to AIN0.

Sorry... I didn't understand what you meant by"AIN5 is muxed always with
AIN0"?

>> In fact i can readout heavy jitter even if AIN5 is connected to ground - 
> after
>> setting up negative adc pin within code (to use REFN)  the readout value 
> is 0 
>> as expected without nameable jitter.
>> If i short AIN0 also to ground, jitter is also eliminated.

Hmmm... nobody has reported such behavior before. ADC support for
am335x-evm/beaglebone has been there for quite long time, but nobody
reported any jitter on AIN5 line. I think this may be specific to
your setup. Can you provide more info with regard to your setup?
Which kernel? Is it am335x-evm or beaglebone or a custom board?

>>
>> Maybe this is also some fault of TI SoC ... in normal case somebody 
> could 
>> expect, that negative adc pin is equal even the Diff_CNTRL bit isn't set 
> - but
>> in practice it isn't.
>>
>> Also actually it isn't possible to make some accurate measurement due to 
> the 
>> fact that allways VDDA_ADC is used as positive reference.
>>
>> So it would be nice to have control around this bits.
>> Whats your opinion around that?

Sorry, I am not yet clear on your bug/use-case.

Please comment inline while replying on mailing list

Regards
Vignesh

WG: Re: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for tscadc

[Hannes Petermaier]

Hi Vignesh,

any comments on this ?
I didn't hear anything last 2 weeks from you.

best regards,
Hannes

----- Forwarded by Hannes Petermaier/Eggelsberg/AT/B&R on 29.04.2015 07:03 
-----

> From: Hannes Petermaier/Eggelsberg/AT/B&R
> To: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Dmitry Torokhov 
<dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, 
> fcooper-l0cyMroinI0@public.gmane.org, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Ian Campbell<ijc
> +devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>, Jan Kardell <jan.kardell-KSZdJiTw9mzQT0dZR+AlfA@public.gmane.org>, 
Johannes 
> Pointner <Johannes.Pointner-5O2GiQo/Ci2aMPzRcYMCawC/G2K4zDHf@public.gmane.org>, Hartmut Knaack 
> <knaack.h-Mmb7MZpHnFY@public.gmane.org>, Karol Wrona <k.wrona-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>, Lars-Peter Clausen 

> <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>, linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, 
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, 
> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>, 
Peter 
> Meerwald <pmeerw-jW+XmwGofnusTnJN9+BGXg@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Date: 15.04.2015 07:33
> Subject: Re: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties 
for tscadc
> 
> > Hi Hannes,
> Hi Vignesh,
> thanks for answer.
> 
> > >>
> > >> would it be possible to add some more channel-specific settings ?
> > >>
> > >> It would be nice to have allmost full control to the STEPCONFIGx 
> > > register.
> > >>
> > >> At least we need to write the bits
> > >>
> > >> SEL_RFM_SWC_1_0
> > >> SEL_INM_SWC_3_0 
> > >> SEL_RFP_SWC_2_0 
> > >>
> > >> In the current mainline version only (SEL_INP_SWC_3_0) is written.
> > >> So for the other bits "0" is value is used, for my point of view 
this is 
> > > not correct.
> > >>
> > >> For example if we want to read a value from AIN5 the negative pin 
from 
> > > adc is 
> > >> muxed allways to AIN0.
> > 
> > Sorry... I didn't understand what you meant by"AIN5 is muxed always 
with
> > AIN0"?
> Have a look to the TRM (spruh73k.pdf) Page 1740 / Figure 12-2. 
Functional Block Diagram.
> There you can see that the ADC-cell which has two inputs, one positive 
and onenegative.
> Also there are two reference inputs, one positive - one negative.
> 
> All this "pins" are muxed, because only one channel per time can be 
sampled.
> This muxes are controlled through the STEPCONFIGx registers.
> 
> If you want for example take some measurement from AIN5 the driver muxes 
the 
> positive input from the ADC to AIN5 by setting the bits for SEL_INP<3:0> 
- this is ok.
> But the bits for SEL_INM<3:0> are still 'zero'. 
> In summary this results in following mux-setting (regarding page 1771 in 
TRM):
> 
> positive-reference muxed to   VDDA
> negative-reference muxed to   VSSA
> positive-input muxed to       AIN5
> negative-input muxed to       AIN0
> 
> From this setup we run into 2 problems:
> - the negative input terminal is muxed maybe to wrong potential
> In much cases we have a single-ended signal so this setup looks good at 
first,
> because the "Diff_CNTRL" bit is also false. 
> In fact there is an influence to the reading if the negative 
input-terminal 
> isn't setup correctly (to VSSA or REFN). 
> Maybe i interpret the "Diff_CNTRL" not correctly, there is no detailed 
> description within the TRM - maybe some of your workmates can explain 
you the 
> functionality of this bit.
> 
> - reference is allways taken from VDDA/VSSA
> For a precision measurement you dont't use in normal case the 
analog-supply.
> This rail brings noise, drift - all things whicht we don't need for 
accurate 
> measurement.
> 
> > 
> > >> In fact i can readout heavy jitter even if AIN5 is connected to 
ground - 
> > > after
> > >> setting up negative adc pin within code (to use REFN)  the readout 
value 
> > > is 0 
> > >> as expected without nameable jitter.
> > >> If i short AIN0 also to ground, jitter is also eliminated.
> > 
> > Hmmm... nobody has reported such behavior before. ADC support for
> > am335x-evm/beaglebone has been there for quite long time, but nobody
> > reported any jitter on AIN5 line. I think this may be specific to
> > your setup. Can you provide more info with regard to your setup?
> > Which kernel? Is it am335x-evm or beaglebone or a custom board?
> Maybe nobody does some precision measurement with beaglebone.
> For operating some touchscreen or readout a potentiometer for evaluation 

> purpose it is still good enough.
> 
> Kernel is current mainline, 4.0
> Board is some custom board of my company.
> But all this parameters shouldn't have some influence to the case.
> 
> > >>
> > >> Maybe this is also some fault of TI SoC ... in normal case somebody 

> > > could 
> > >> expect, that negative adc pin is equal even the Diff_CNTRL bit 
isn't set 
> > > - but
> > >> in practice it isn't.
> > >>
> > >> Also actually it isn't possible to make some accurate measurement 
due to 
> > > the 
> > >> fact that allways VDDA_ADC is used as positive reference.
> > >>
> > >> So it would be nice to have control around this bits.
> > >> Whats your opinion around that?
> > 
> > Sorry, I am not yet clear on your bug/use-case.
> > 
> > Please comment inline while replying on mailing list
> okay so ?
> 
> > Regards
> > Vignesh
> best regards,
> Hannes
> 
> 


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

Re: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties for tscadc

[Vignesh R]

Hi Hannes,

On Wednesday 29 April 2015 10:36 AM, Hannes Petermaier wrote:
> Hi Vignesh,
> 
> any comments on this ?
> I didn't hear anything last 2 weeks from you.

Apologies...  For some reason my mail client classified your reply mails
as junk, hence I never look into it.

I agree that making SEL_INM_SWC_3_0, SEL_RFM_SWC_1_0 and SEL_RFP_SWC_2_0
configurable is good to have. But I don't think I will be able to work
on it anytime sooner.

Regards
Vignesh

> 
> best regards,
> Hannes
> 
> ----- Forwarded by Hannes Petermaier/Eggelsberg/AT/B&R on 29.04.2015 07:03 
> -----
> 
>> From: Hannes Petermaier/Eggelsberg/AT/B&R
>> To: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
>> Cc: devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Dmitry Torokhov 
> <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>, 
>> fcooper-l0cyMroinI0@public.gmane.org, Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Ian Campbell<ijc
>> +devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org>, Jan Kardell <jan.kardell-KSZdJiTw9mzQT0dZR+AlfA@public.gmane.org>, 
> Johannes 
>> Pointner <Johannes.Pointner-5O2GiQo/Ci2aMPzRcYMCawC/G2K4zDHf@public.gmane.org>, Hartmut Knaack 
>> <knaack.h-Mmb7MZpHnFY@public.gmane.org>, Karol Wrona <k.wrona-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>, Lars-Peter Clausen 
> 
>> <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>, linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, 
> linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, 
>> Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, Pawel Moll <pawel.moll-5wv7dgnIgG8@public.gmane.org>, 
> Peter 
>> Meerwald <pmeerw-jW+XmwGofnusTnJN9+BGXg@public.gmane.org>, Rob Herring <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> Date: 15.04.2015 07:33
>> Subject: Re: [PATCH 0/2] iio: ti_am335x_adc: Add optional DT properties 
> for tscadc
>>
>>> Hi Hannes,
>> Hi Vignesh,
>> thanks for answer.
>>
>>>>>
>>>>> would it be possible to add some more channel-specific settings ?
>>>>>
>>>>> It would be nice to have allmost full control to the STEPCONFIGx 
>>>> register.
>>>>>
>>>>> At least we need to write the bits
>>>>>
>>>>> SEL_RFM_SWC_1_0
>>>>> SEL_INM_SWC_3_0 
>>>>> SEL_RFP_SWC_2_0 
>>>>>
>>>>> In the current mainline version only (SEL_INP_SWC_3_0) is written.
>>>>> So for the other bits "0" is value is used, for my point of view 
> this is 
>>>> not correct.
>>>>>
>>>>> For example if we want to read a value from AIN5 the negative pin 
> from 
>>>> adc is 
>>>>> muxed allways to AIN0.
>>>
>>> Sorry... I didn't understand what you meant by"AIN5 is muxed always 
> with
>>> AIN0"?
>> Have a look to the TRM (spruh73k.pdf) Page 1740 / Figure 12-2. 
> Functional Block Diagram.
>> There you can see that the ADC-cell which has two inputs, one positive 
> and onenegative.
>> Also there are two reference inputs, one positive - one negative.
>>
>> All this "pins" are muxed, because only one channel per time can be 
> sampled.
>> This muxes are controlled through the STEPCONFIGx registers.
>>
>> If you want for example take some measurement from AIN5 the driver muxes 
> the 
>> positive input from the ADC to AIN5 by setting the bits for SEL_INP<3:0> 
> - this is ok.
>> But the bits for SEL_INM<3:0> are still 'zero'. 
>> In summary this results in following mux-setting (regarding page 1771 in 
> TRM):
>>
>> positive-reference muxed to   VDDA
>> negative-reference muxed to   VSSA
>> positive-input muxed to       AIN5
>> negative-input muxed to       AIN0
>>
>> From this setup we run into 2 problems:
>> - the negative input terminal is muxed maybe to wrong potential
>> In much cases we have a single-ended signal so this setup looks good at 
> first,
>> because the "Diff_CNTRL" bit is also false. 
>> In fact there is an influence to the reading if the negative 
> input-terminal 
>> isn't setup correctly (to VSSA or REFN). 
>> Maybe i interpret the "Diff_CNTRL" not correctly, there is no detailed 
>> description within the TRM - maybe some of your workmates can explain 
> you the 
>> functionality of this bit.
>>
>> - reference is allways taken from VDDA/VSSA
>> For a precision measurement you dont't use in normal case the 
> analog-supply.
>> This rail brings noise, drift - all things whicht we don't need for 
> accurate 
>> measurement.
>>
>>>
>>>>> In fact i can readout heavy jitter even if AIN5 is connected to 
> ground - 
>>>> after
>>>>> setting up negative adc pin within code (to use REFN)  the readout 
> value 
>>>> is 0 
>>>>> as expected without nameable jitter.
>>>>> If i short AIN0 also to ground, jitter is also eliminated.
>>>
>>> Hmmm... nobody has reported such behavior before. ADC support for
>>> am335x-evm/beaglebone has been there for quite long time, but nobody
>>> reported any jitter on AIN5 line. I think this may be specific to
>>> your setup. Can you provide more info with regard to your setup?
>>> Which kernel? Is it am335x-evm or beaglebone or a custom board?
>> Maybe nobody does some precision measurement with beaglebone.
>> For operating some touchscreen or readout a potentiometer for evaluation 
> 
>> purpose it is still good enough.
>>
>> Kernel is current mainline, 4.0
>> Board is some custom board of my company.
>> But all this parameters shouldn't have some influence to the case.
>>
>>>>>
>>>>> Maybe this is also some fault of TI SoC ... in normal case somebody 
> 
>>>> could 
>>>>> expect, that negative adc pin is equal even the Diff_CNTRL bit 
> isn't set 
>>>> - but
>>>>> in practice it isn't.
>>>>>
>>>>> Also actually it isn't possible to make some accurate measurement 
> due to 
>>>> the 
>>>>> fact that allways VDDA_ADC is used as positive reference.
>>>>>
>>>>> So it would be nice to have control around this bits.
>>>>> Whats your opinion around that?
>>>
>>> Sorry, I am not yet clear on your bug/use-case.
>>>
>>> Please comment inline while replying on mailing list
>> okay so ?
>>
>>> Regards
>>> Vignesh
>> best regards,
>> Hannes
>>
>>
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html