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

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

[Vignesh R]

Refactor DT parsing into a seperate 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 63b2bb6..dfb0db0 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -407,16 +407,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");
@@ -432,12 +446,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.7.9.5

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

[Vignesh R]

Number of averaging, open delay, sample delay are made DT parameters.
By decreasing averaging and delays more samples can be obtained per
second increasing performance of ADC. Previously the number of
averages per step was fixed to 16. Making these parameters
configurable will help in balancing speed vs accuracy.
For each ADC step provide DT based paramters to set open delay,
sample delay and number of averaging. One configurable step is
used per ADC channel. Since there can be atmost 8 ADC channels,
steps 16 to 8 are used for ADC.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 .../bindings/input/touchscreen/ti-tsc-adc.txt      |   18 +++++++
 drivers/iio/adc/ti_am335x_adc.c                    |   49 +++++++++++++++++---
 include/linux/mfd/ti_am335x_tscadc.h               |    3 ++
 3 files changed, 63 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
index 878549b..09ac097 100644
--- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
+++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
@@ -27,6 +27,21 @@ Required properties:
 - child "adc"
 	ti,adc-channels: List of analog inputs available for ADC.
 			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
+Optional properties:
+- child "adc"
+	ti,step-opendelay: List of open delays for each channel of ADC in the order
+			   of ti,adc-channels. This value is written to corresponding
+			   step-config register. Default value is 0x098.
+			   Maximum value is 0x3FFF.
+	ti,step-sampledelay: List of sample delays for each channel of ADC in the order
+			     of ti,adc-channels. This value is written to corresponding
+			     step-config register. Default value is 0x0.
+			     Maximum value is 0xFF.
+	ti,step-average: 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.
+			 Default value is 16. Maximum value is 16.
 
 Example:
 	tscadc: tscadc@44e0d000 {
@@ -40,5 +55,8 @@ Example:
 
 		adc {
 			ti,adc-channels = <4 5 6 7>;
+			ti,step-opendelay = <0x098 0x3FFFF 0x098 0x0>;
+			ti,step-sampledelay = <0xFF 0x0 0xF 0x0>;
+			ti,step-average = <16 2 4 8>;
 		};
 	}
diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index dfb0db0..2b31ef2 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_line[8];
 	u8 channel_step[8];
 	int buffer_en_ch_steps;
+	u32 open_delay[8], sample_delay[8], step_avg[8];
 	u16 data[8];
 };
 
@@ -86,6 +87,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;
 
@@ -100,20 +102,41 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
 	 */
 
 	steps = TOTAL_STEPS - adc_dev->channels;
-	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_MAX) {
+			dev_warn(dev, "channel %d step average exceeds max value, truncating to %d\n",
+				 chan, STEPCONFIG_AVG_MAX);
+			adc_dev->step_avg[i] = STEPCONFIG_AVG_MAX;
+		}
+
+		stepconfig = STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
+			     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_MAX) {
+			dev_warn(dev, "channel %d step delay open exceeds max value, truncating to %d\n",
+				 chan, STEPDELAY_OPEN_MAX);
+			adc_dev->open_delay[i] = STEPDELAY_OPEN_MAX;
+		}
+
+		if (adc_dev->sample_delay[i] > STEPDELAY_SAMPLE_MAX) {
+			dev_warn(dev, "channel %d step delay sample max value, truncating to %d\n",
+				 chan, STEPDELAY_SAMPLE_MAX);
+			adc_dev->sample_delay[i] = STEPDELAY_SAMPLE_MAX;
+		}
+
 		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++;
 	}
@@ -418,10 +441,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++;
 	}
 
 	adc_dev->channels = channels;
+	of_property_read_u32_array(node, "ti,step-average",
+				   adc_dev->step_avg, channels);
+	of_property_read_u32_array(node, "ti,step-opendelay",
+				   adc_dev->open_delay, channels);
+	of_property_read_u32_array(node, "ti,step-sampledelay",
+				   adc_dev->sample_delay, channels);
+
 	return 0;
 }
 
diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
index fb96c84..26d3e84 100644
--- a/include/linux/mfd/ti_am335x_tscadc.h
+++ b/include/linux/mfd/ti_am335x_tscadc.h
@@ -82,14 +82,17 @@
 #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
 #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
 #define STEPCONFIG_FIFO1	BIT(26)
+#define STEPCONFIG_AVG_MAX	16
 
 /* Delay register */
 #define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
 #define STEPDELAY_OPEN(val)	((val) << 0)
 #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
+#define STEPDELAY_OPEN_MAX	0x3FFFF
 #define STEPDELAY_SAMPLE_MASK	(0xFF << 24)
 #define STEPDELAY_SAMPLE(val)	((val) << 24)
 #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
+#define STEPDELAY_SAMPLE_MAX	0xFF
 
 /* Charge Config */
 #define STEPCHARGE_RFP_MASK	(7 << 12)
-- 
1.7.9.5

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

[Lee Jones]

On Wed, 27 Aug 2014, Vignesh R wrote:

> Number of averaging, open delay, sample delay are made DT parameters.
> By decreasing averaging and delays more samples can be obtained per
> second increasing performance of ADC. Previously the number of
> averages per step was fixed to 16. Making these parameters
> configurable will help in balancing speed vs accuracy.
> For each ADC step provide DT based paramters to set open delay,
> sample delay and number of averaging. One configurable step is
> used per ADC channel. Since there can be atmost 8 ADC channels,
> steps 16 to 8 are used for ADC.
> 
> Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   18 +++++++
>  drivers/iio/adc/ti_am335x_adc.c                    |   49 +++++++++++++++++---
>  include/linux/mfd/ti_am335x_tscadc.h               |    3 ++
>  3 files changed, 63 insertions(+), 7 deletions(-)

[...]

> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index fb96c84..26d3e84 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -82,14 +82,17 @@

There are so many different formats at play here, it's difficult to
see what's happening where!

>  #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
>  #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
>  #define STEPCONFIG_FIFO1	BIT(26)
> +#define STEPCONFIG_AVG_MAX	16
>  
>  /* Delay register */
>  #define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
>  #define STEPDELAY_OPEN(val)	((val) << 0)

What's the point in shifting by zero?

>  #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
> +#define STEPDELAY_OPEN_MAX	0x3FFFF

Put this up the top, then you can use it when defining
STEPDELAY_OPEN_MASK.

>  #define STEPDELAY_SAMPLE_MASK	(0xFF << 24)
>  #define STEPDELAY_SAMPLE(val)	((val) << 24)
>  #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
> +#define STEPDELAY_SAMPLE_MAX	0xFF

>  /* Charge Config */
>  #define STEPCHARGE_RFP_MASK	(7 << 12)

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Vignesh R]

Hi,

On Wednesday 27 August 2014 07:26 PM, Lee Jones wrote:
> On Wed, 27 Aug 2014, Vignesh R wrote:
> 
>> Number of averaging, open delay, sample delay are made DT parameters.
>> By decreasing averaging and delays more samples can be obtained per
>> second increasing performance of ADC. Previously the number of
>> averages per step was fixed to 16. Making these parameters
>> configurable will help in balancing speed vs accuracy.
>> For each ADC step provide DT based paramters to set open delay,
>> sample delay and number of averaging. One configurable step is
>> used per ADC channel. Since there can be atmost 8 ADC channels,
>> steps 16 to 8 are used for ADC.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
>> ---
>>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   18 +++++++
>>  drivers/iio/adc/ti_am335x_adc.c                    |   49 +++++++++++++++++---
>>  include/linux/mfd/ti_am335x_tscadc.h               |    3 ++
>>  3 files changed, 63 insertions(+), 7 deletions(-)
> 
> [...]
> 
>> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
>> index fb96c84..26d3e84 100644
>> --- a/include/linux/mfd/ti_am335x_tscadc.h
>> +++ b/include/linux/mfd/ti_am335x_tscadc.h
>> @@ -82,14 +82,17 @@
> 
> There are so many different formats at play here, it's difficult to
> see what's happening where!

I did not understand "different formats". Please explain.

> 
>>  #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
>>  #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
>>  #define STEPCONFIG_FIFO1	BIT(26)
>> +#define STEPCONFIG_AVG_MAX	16
>>  
>>  /* Delay register */
>>  #define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
>>  #define STEPDELAY_OPEN(val)	((val) << 0)
> 
> What's the point in shifting by zero?

I agree. But my patch did not add these lines. I can remove them. Do you
want me to do this change as part of current patch or as a separate
cleanup patch?

> 
>>  #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
>> +#define STEPDELAY_OPEN_MAX	0x3FFFF
> 
> Put this up the top, then you can use it when defining
> STEPDELAY_OPEN_MASK.

Done.

> 
>>  #define STEPDELAY_SAMPLE_MASK	(0xFF << 24)
>>  #define STEPDELAY_SAMPLE(val)	((val) << 24)
>>  #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
>> +#define STEPDELAY_SAMPLE_MAX	0xFF
> 
>>  /* Charge Config */
>>  #define STEPCHARGE_RFP_MASK	(7 << 12)
> 

Regards
Vignesh R

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

[Lee Jones]

On Thu, 28 Aug 2014, Vignesh R wrote:
> On Wednesday 27 August 2014 07:26 PM, Lee Jones wrote:
> > On Wed, 27 Aug 2014, Vignesh R wrote:
> > 
> >> Number of averaging, open delay, sample delay are made DT parameters.
> >> By decreasing averaging and delays more samples can be obtained per
> >> second increasing performance of ADC. Previously the number of
> >> averages per step was fixed to 16. Making these parameters
> >> configurable will help in balancing speed vs accuracy.
> >> For each ADC step provide DT based paramters to set open delay,
> >> sample delay and number of averaging. One configurable step is
> >> used per ADC channel. Since there can be atmost 8 ADC channels,
> >> steps 16 to 8 are used for ADC.
> >>
> >> Signed-off-by: Vignesh R <vigneshr@ti.com>
> >> ---
> >>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   18 +++++++
> >>  drivers/iio/adc/ti_am335x_adc.c                    |   49 +++++++++++++++++---
> >>  include/linux/mfd/ti_am335x_tscadc.h               |    3 ++
> >>  3 files changed, 63 insertions(+), 7 deletions(-)
> > 
> > [...]
> > 
> >> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> >> index fb96c84..26d3e84 100644
> >> --- a/include/linux/mfd/ti_am335x_tscadc.h
> >> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> >> @@ -82,14 +82,17 @@
> > 
> > There are so many different formats at play here, it's difficult to
> > see what's happening where!
> 
> I did not understand "different formats". Please explain.

My complaint is with the existing layout, rather than your patch:

> #define STEPCONFIG_INP_AN4     STEPCONFIG_INP(4)
> #define STEPCONFIG_INP_ADCREFM STEPCONFIG_INP(8)
> #define STEPCONFIG_FIFO1       BIT(26)

Base10 MACROS.

#define STEPCONFIG_AVG_MAX     16

Base10 int.

> /* Delay register */
> #define STEPDELAY_OPEN_MASK    (0x3FFFF << 0)

Base16 shift.

> #define STEPDELAY_OPEN(val)    ((val) << 0)

MACRO shift.

> #define STEPCONFIG_OPENDLY     STEPDELAY_OPEN(0x098)

Base16 MACRO.

> #define STEPDELAY_OPEN_MAX     0x3FFFF

Base16 int.

> #define STEPDELAY_SAMPLE_MASK  (0xFF << 24)

Base16 shift.

> #define STEPDELAY_SAMPLE(val)  ((val) << 24)

MACRO shift.

> #define STEPCONFIG_SAMPLEDLY   STEPDELAY_SAMPLE(0)

24 shift of 0 (still zero?).

> #define STEPDELAY_SAMPLE_MAX   0xFF

Base16 int.

There is no consistency here, it's just a bunch of 'stuff', which
makes making sense of them all difficult.

> >>  #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
> >>  #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
> >>  #define STEPCONFIG_FIFO1	BIT(26)
> >> +#define STEPCONFIG_AVG_MAX	16
> >>  
> >>  /* Delay register */
> >>  #define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
> >>  #define STEPDELAY_OPEN(val)	((val) << 0)
> > 
> > What's the point in shifting by zero?
> 
> I agree. But my patch did not add these lines. I can remove them. Do you
> want me to do this change as part of current patch or as a separate
> cleanup patch?

That's up to Jonathan.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

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

[Jonathan Cameron]

On 28/08/14 08:11, Lee Jones wrote:
> On Thu, 28 Aug 2014, Vignesh R wrote:
>> On Wednesday 27 August 2014 07:26 PM, Lee Jones wrote:
>>> On Wed, 27 Aug 2014, Vignesh R wrote:
>>>
>>>> Number of averaging, open delay, sample delay are made DT parameters.
>>>> By decreasing averaging and delays more samples can be obtained per
>>>> second increasing performance of ADC. Previously the number of
>>>> averages per step was fixed to 16. Making these parameters
>>>> configurable will help in balancing speed vs accuracy.
>>>> For each ADC step provide DT based paramters to set open delay,
>>>> sample delay and number of averaging. One configurable step is
>>>> used per ADC channel. Since there can be atmost 8 ADC channels,
>>>> steps 16 to 8 are used for ADC.
>>>>
>>>> Signed-off-by: Vignesh R <vigneshr-l0cyMroinI0@public.gmane.org>
>>>> ---
>>>>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   18 +++++++
>>>>  drivers/iio/adc/ti_am335x_adc.c                    |   49 +++++++++++++++++---
>>>>  include/linux/mfd/ti_am335x_tscadc.h               |    3 ++
>>>>  3 files changed, 63 insertions(+), 7 deletions(-)
>>>
>>> [...]
>>>
>>>> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
>>>> index fb96c84..26d3e84 100644
>>>> --- a/include/linux/mfd/ti_am335x_tscadc.h
>>>> +++ b/include/linux/mfd/ti_am335x_tscadc.h
>>>> @@ -82,14 +82,17 @@
>>>
>>> There are so many different formats at play here, it's difficult to
>>> see what's happening where!
>>
>> I did not understand "different formats". Please explain.
> 
> My complaint is with the existing layout, rather than your patch:
> 
>> #define STEPCONFIG_INP_AN4     STEPCONFIG_INP(4)
>> #define STEPCONFIG_INP_ADCREFM STEPCONFIG_INP(8)
>> #define STEPCONFIG_FIFO1       BIT(26)
> 
> Base10 MACROS.
> 
> #define STEPCONFIG_AVG_MAX     16
> 
> Base10 int.
> 
>> /* Delay register */
>> #define STEPDELAY_OPEN_MASK    (0x3FFFF << 0)
> 
> Base16 shift.
> 
>> #define STEPDELAY_OPEN(val)    ((val) << 0)
> 
> MACRO shift.
> 
>> #define STEPCONFIG_OPENDLY     STEPDELAY_OPEN(0x098)
> 
> Base16 MACRO.
> 
>> #define STEPDELAY_OPEN_MAX     0x3FFFF
> 
> Base16 int.
> 
>> #define STEPDELAY_SAMPLE_MASK  (0xFF << 24)
> 
> Base16 shift.
> 
>> #define STEPDELAY_SAMPLE(val)  ((val) << 24)
> 
> MACRO shift.
> 
>> #define STEPCONFIG_SAMPLEDLY   STEPDELAY_SAMPLE(0)
> 
> 24 shift of 0 (still zero?).
> 
>> #define STEPDELAY_SAMPLE_MAX   0xFF
> 
> Base16 int.
> 
> There is no consistency here, it's just a bunch of 'stuff', which
> makes making sense of them all difficult.
> 
>>>>  #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
>>>>  #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
>>>>  #define STEPCONFIG_FIFO1	BIT(26)
>>>> +#define STEPCONFIG_AVG_MAX	16
>>>>  
>>>>  /* Delay register */
>>>>  #define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
>>>>  #define STEPDELAY_OPEN(val)	((val) << 0)
>>>
>>> What's the point in shifting by zero?
>>
>> I agree. But my patch did not add these lines. I can remove them. Do you
>> want me to do this change as part of current patch or as a separate
>> cleanup patch?
> 
> That's up to Jonathan.
I'm always happy to take sane cleanups like this.  Separate cleanup patch please.
That way it's easy to verify that there are no functional changes without it
getting in the way of the 'interesting' patch.

Thanks,

Jonathan
> 

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

[Jonathan Cameron]

On 27/08/14 13:19, Vignesh R wrote:
> Number of averaging, open delay, sample delay are made DT parameters.
> By decreasing averaging and delays more samples can be obtained per
> second increasing performance of ADC. Previously the number of
> averages per step was fixed to 16. Making these parameters
> configurable will help in balancing speed vs accuracy.
> For each ADC step provide DT based paramters to set open delay,
> sample delay and number of averaging. One configurable step is
> used per ADC channel. Since there can be atmost 8 ADC channels,
> steps 16 to 8 are used for ADC.
> 
> Signed-off-by: Vignesh R <vigneshr@ti.com>

My main question here is whether these make sense purely as DT parameters
or whether userspace control would be useful?  Still, even if we add that
later, there is nothing wrong with specifying an initial value in the DT.

Average in particular strikes me as a straight forward parameter where userspace
control might make sense...

Mostly fine, but I think your example has a stray F.

J
> ---
>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   18 +++++++
>  drivers/iio/adc/ti_am335x_adc.c                    |   49 +++++++++++++++++---
>  include/linux/mfd/ti_am335x_tscadc.h               |    3 ++
>  3 files changed, 63 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> index 878549b..09ac097 100644
> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
> @@ -27,6 +27,21 @@ Required properties:
>  - child "adc"
>  	ti,adc-channels: List of analog inputs available for ADC.
>  			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
> +Optional properties:
> +- child "adc"
> +	ti,step-opendelay: List of open delays for each channel of ADC in the order
> +			   of ti,adc-channels. This value is written to corresponding
> +			   step-config register. Default value is 0x098.
> +			   Maximum value is 0x3FFF.

Any chance of real numbers in here rather than magic constants? If they are real
numbers, then some explanation of how they relate to time would be good. I also wonder if we
need to name them after steps here, given they correspond directly to adc channels.

> +	ti,step-sampledelay: List of sample delays for each channel of ADC in the order
> +			     of ti,adc-channels. This value is written to corresponding
> +			     step-config register. Default value is 0x0.
> +			     Maximum value is 0xFF.
> +	ti,step-average: 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.
> +			 Default value is 16. Maximum value is 16.
>  
>  Example:
>  	tscadc: tscadc@44e0d000 {
> @@ -40,5 +55,8 @@ Example:
>  
>  		adc {
>  			ti,adc-channels = <4 5 6 7>;
> +			ti,step-opendelay = <0x098 0x3FFFF 0x098 0x0>;
too many F's?

> +			ti,step-sampledelay = <0xFF 0x0 0xF 0x0>;
> +			ti,step-average = <16 2 4 8>;
>  		};
>  	}
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index dfb0db0..2b31ef2 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_line[8];
>  	u8 channel_step[8];
>  	int buffer_en_ch_steps;
> +	u32 open_delay[8], sample_delay[8], step_avg[8];
>  	u16 data[8];
>  };
>  
> @@ -86,6 +87,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;
>  
> @@ -100,20 +102,41 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>  	 */
>  
>  	steps = TOTAL_STEPS - adc_dev->channels;
> -	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_MAX) {
> +			dev_warn(dev, "channel %d step average exceeds max value, truncating to %d\n",
> +				 chan, STEPCONFIG_AVG_MAX);
> +			adc_dev->step_avg[i] = STEPCONFIG_AVG_MAX;
> +		}
> +
> +		stepconfig = STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
> +			     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_MAX) {
> +			dev_warn(dev, "channel %d step delay open exceeds max value, truncating to %d\n",
> +				 chan, STEPDELAY_OPEN_MAX);
> +			adc_dev->open_delay[i] = STEPDELAY_OPEN_MAX;
> +		}
> +
> +		if (adc_dev->sample_delay[i] > STEPDELAY_SAMPLE_MAX) {
> +			dev_warn(dev, "channel %d step delay sample max value, truncating to %d\n",
> +				 chan, STEPDELAY_SAMPLE_MAX);
> +			adc_dev->sample_delay[i] = STEPDELAY_SAMPLE_MAX;
> +		}
> +
>  		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++;
>  	}
> @@ -418,10 +441,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++;
>  	}
>  
>  	adc_dev->channels = channels;
> +	of_property_read_u32_array(node, "ti,step-average",
> +				   adc_dev->step_avg, channels);
> +	of_property_read_u32_array(node, "ti,step-opendelay",
> +				   adc_dev->open_delay, channels);
> +	of_property_read_u32_array(node, "ti,step-sampledelay",
> +				   adc_dev->sample_delay, channels);
> +
>  	return 0;
>  }
>  
> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
> index fb96c84..26d3e84 100644
> --- a/include/linux/mfd/ti_am335x_tscadc.h
> +++ b/include/linux/mfd/ti_am335x_tscadc.h
> @@ -82,14 +82,17 @@
>  #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
>  #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
>  #define STEPCONFIG_FIFO1	BIT(26)
> +#define STEPCONFIG_AVG_MAX	16
>  
>  /* Delay register */
>  #define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
>  #define STEPDELAY_OPEN(val)	((val) << 0)
>  #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
> +#define STEPDELAY_OPEN_MAX	0x3FFFF
>  #define STEPDELAY_SAMPLE_MASK	(0xFF << 24)
>  #define STEPDELAY_SAMPLE(val)	((val) << 24)
>  #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
> +#define STEPDELAY_SAMPLE_MAX	0xFF
>  
>  /* Charge Config */
>  #define STEPCHARGE_RFP_MASK	(7 << 12)
> 

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

[Vignesh R]

Hi,

On Saturday 30 August 2014 03:13 PM, Jonathan Cameron wrote:
> On 27/08/14 13:19, Vignesh R wrote:
>> Number of averaging, open delay, sample delay are made DT parameters.
>> By decreasing averaging and delays more samples can be obtained per
>> second increasing performance of ADC. Previously the number of
>> averages per step was fixed to 16. Making these parameters
>> configurable will help in balancing speed vs accuracy.
>> For each ADC step provide DT based paramters to set open delay,
>> sample delay and number of averaging. One configurable step is
>> used per ADC channel. Since there can be atmost 8 ADC channels,
>> steps 16 to 8 are used for ADC.
>>
>> Signed-off-by: Vignesh R <vigneshr@ti.com>
> 
> My main question here is whether these make sense purely as DT parameters
> or whether userspace control would be useful?  Still, even if we add that
> later, there is nothing wrong with specifying an initial value in the DT.
> 

I agree. These value can act as initial hardware parameters. Will look
at adding userspace control in another patch.

> Average in particular strikes me as a straight forward parameter where userspace
> control might make sense...
> 

I looked at Documentation/ABI/testing/sysfs-bus-iio but could not find
any framework support to make step-average a sysfs entry. Looks like I
need to create a new (my hardware specific) sysfs entry?

> Mostly fine, but I think your example has a stray F.

Actually maximum value for step-opendelay is 0x3FFFF (18 bits). I will
correct the documentation.

> 
> J
>> ---
>>  .../bindings/input/touchscreen/ti-tsc-adc.txt      |   18 +++++++
>>  drivers/iio/adc/ti_am335x_adc.c                    |   49 +++++++++++++++++---
>>  include/linux/mfd/ti_am335x_tscadc.h               |    3 ++
>>  3 files changed, 63 insertions(+), 7 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> index 878549b..09ac097 100644
>> --- a/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> +++ b/Documentation/devicetree/bindings/input/touchscreen/ti-tsc-adc.txt
>> @@ -27,6 +27,21 @@ Required properties:
>>  - child "adc"
>>  	ti,adc-channels: List of analog inputs available for ADC.
>>  			 AIN0 = 0, AIN1 = 1 and so on till AIN7 = 7.
>> +Optional properties:
>> +- child "adc"
>> +	ti,step-opendelay: List of open delays for each channel of ADC in the order
>> +			   of ti,adc-channels. This value is written to corresponding
>> +			   step-config register. Default value is 0x098.
>> +			   Maximum value is 0x3FFF.
> 
> Any chance of real numbers in here rather than magic constants? If they are real
> numbers, then some explanation of how they relate to time would be good. 

The values correspond to number of ADC clock cycles. I will document the
same. I will also drop the mention of "default value" - thats really a
driver implementation detail and not a DT careabout.

>I also wonder if we need to name them after steps here, given they correspond directly to adc channels.

These values are written to corresponding STEPDELAYX and STEPCONFIGX
registers. The technical reference manual for am335x also says "number
of averages per step". In order to maintain consistency with above
naming conventions I have named these parameters after steps.

I can make it ti,chan-step-average if that feels more right.

> 
>> +	ti,step-sampledelay: List of sample delays for each channel of ADC in the order
>> +			     of ti,adc-channels. This value is written to corresponding
>> +			     step-config register. Default value is 0x0.
>> +			     Maximum value is 0xFF.
>> +	ti,step-average: 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.
>> +			 Default value is 16. Maximum value is 16.
>>  
>>  Example:
>>  	tscadc: tscadc@44e0d000 {
>> @@ -40,5 +55,8 @@ Example:
>>  
>>  		adc {
>>  			ti,adc-channels = <4 5 6 7>;
>> +			ti,step-opendelay = <0x098 0x3FFFF 0x098 0x0>;
> too many F's?
> 
>> +			ti,step-sampledelay = <0xFF 0x0 0xF 0x0>;
>> +			ti,step-average = <16 2 4 8>;
>>  		};
>>  	}
>> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>> index dfb0db0..2b31ef2 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_line[8];
>>  	u8 channel_step[8];
>>  	int buffer_en_ch_steps;
>> +	u32 open_delay[8], sample_delay[8], step_avg[8];
>>  	u16 data[8];
>>  };
>>  
>> @@ -86,6 +87,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;
>>  
>> @@ -100,20 +102,41 @@ static void tiadc_step_config(struct iio_dev *indio_dev)
>>  	 */
>>  
>>  	steps = TOTAL_STEPS - adc_dev->channels;
>> -	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_MAX) {
>> +			dev_warn(dev, "channel %d step average exceeds max value, truncating to %d\n",
>> +				 chan, STEPCONFIG_AVG_MAX);
>> +			adc_dev->step_avg[i] = STEPCONFIG_AVG_MAX;
>> +		}
>> +
>> +		stepconfig = STEPCONFIG_AVG(ffs(adc_dev->step_avg[i]) - 1) |
>> +			     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_MAX) {
>> +			dev_warn(dev, "channel %d step delay open exceeds max value, truncating to %d\n",
>> +				 chan, STEPDELAY_OPEN_MAX);
>> +			adc_dev->open_delay[i] = STEPDELAY_OPEN_MAX;
>> +		}
>> +
>> +		if (adc_dev->sample_delay[i] > STEPDELAY_SAMPLE_MAX) {
>> +			dev_warn(dev, "channel %d step delay sample max value, truncating to %d\n",
>> +				 chan, STEPDELAY_SAMPLE_MAX);
>> +			adc_dev->sample_delay[i] = STEPDELAY_SAMPLE_MAX;
>> +		}
>> +
>>  		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++;
>>  	}
>> @@ -418,10 +441,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++;
>>  	}
>>  
>>  	adc_dev->channels = channels;
>> +	of_property_read_u32_array(node, "ti,step-average",
>> +				   adc_dev->step_avg, channels);
>> +	of_property_read_u32_array(node, "ti,step-opendelay",
>> +				   adc_dev->open_delay, channels);
>> +	of_property_read_u32_array(node, "ti,step-sampledelay",
>> +				   adc_dev->sample_delay, channels);
>> +
>>  	return 0;
>>  }
>>  
>> diff --git a/include/linux/mfd/ti_am335x_tscadc.h b/include/linux/mfd/ti_am335x_tscadc.h
>> index fb96c84..26d3e84 100644
>> --- a/include/linux/mfd/ti_am335x_tscadc.h
>> +++ b/include/linux/mfd/ti_am335x_tscadc.h
>> @@ -82,14 +82,17 @@
>>  #define STEPCONFIG_INP_AN4	STEPCONFIG_INP(4)
>>  #define STEPCONFIG_INP_ADCREFM	STEPCONFIG_INP(8)
>>  #define STEPCONFIG_FIFO1	BIT(26)
>> +#define STEPCONFIG_AVG_MAX	16
>>  
>>  /* Delay register */
>>  #define STEPDELAY_OPEN_MASK	(0x3FFFF << 0)
>>  #define STEPDELAY_OPEN(val)	((val) << 0)
>>  #define STEPCONFIG_OPENDLY	STEPDELAY_OPEN(0x098)
>> +#define STEPDELAY_OPEN_MAX	0x3FFFF
>>  #define STEPDELAY_SAMPLE_MASK	(0xFF << 24)
>>  #define STEPDELAY_SAMPLE(val)	((val) << 24)
>>  #define STEPCONFIG_SAMPLEDLY	STEPDELAY_SAMPLE(0)
>> +#define STEPDELAY_SAMPLE_MAX	0xFF
>>  
>>  /* Charge Config */
>>  #define STEPCHARGE_RFP_MASK	(7 << 12)
>>

Regards
Vignesh R