[PATCH 0/3] IIO: AT91: add lowres, sleep and sample&hold time missing support

[PATCH 0/3] IIO: AT91: add lowres, sleep and sample&hold time missing support

[Jean-Christophe PLAGNIOL-VILLARD]

Hi,

	The current adc driver is missing some key feature support
	 - select high/low resolution support
	 - sleep mode
	 - Sample and Hold Time

	 This patch seris fix it
	 Without it the current driver is not usable in most of the case

Jean-Christophe PLAGNIOL-VILLARD (2):
      ARM: AT91: IIO: add sleep mode support
      ARM: AT91: IIO: fix missing Sample and Hold time

Ludovic Desroches (1):
      ARM: AT91: IIO: add low and high res support for adc

 Documentation/devicetree/bindings/arm/atmel-adc.txt |   13 ++++++++++++
 arch/arm/boot/dts/at91sam9260.dtsi                  |    3 +++
 arch/arm/boot/dts/at91sam9g45.dtsi                  |    3 +++
 arch/arm/boot/dts/at91sam9x5.dtsi                   |    3 +++
 drivers/iio/adc/at91_adc.c                          |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 5 files changed, 107 insertions(+), 5 deletions(-)

Best Regards,
J.

[PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc

[Jean-Christophe PLAGNIOL-VILLARD]

From: Ludovic Desroches <ludovic.desroches@atmel.com>

at91 adc offers the choice between two resolutions: low and high.
The low and high resolution values depends on adc IP version, as many IP
properties have been exposed through device tree, these settings have also
been added to the dt bindings.

Update at the same time the dtsi.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
[plagnioj at jcrosoft.com: udpate current adc dt support]
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-iio at vger.kernel.org
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 .../devicetree/bindings/arm/atmel-adc.txt          |   11 +++
 arch/arm/boot/dts/at91sam9260.dtsi                 |    3 +
 arch/arm/boot/dts/at91sam9g45.dtsi                 |    3 +
 arch/arm/boot/dts/at91sam9x5.dtsi                  |    3 +
 drivers/iio/adc/at91_adc.c                         |   74 ++++++++++++++++++--
 5 files changed, 90 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
index c63097d..fd2d69e 100644
--- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
@@ -14,9 +14,17 @@ Required properties:
   - atmel,adc-status-register: Offset of the Interrupt Status Register
   - atmel,adc-trigger-register: Offset of the Trigger Register
   - atmel,adc-vref: Reference voltage in millivolts for the conversions
+  - atmel,adc-res: List of resolution in bits supported by the ADC. List size
+		   must be two at least.
+  - atmel,adc-res-names: Contains one identifier string for each resolution
+			 in atmel,adc-res property. "lowres" and "highres"
+			 identifiers are required.
 
 Optional properties:
   - atmel,adc-use-external: Boolean to enable of external triggers
+  - atmel,adc-use-res: String corresponding to an identifier from
+		       atmel,adc-res-names property. If not specified, the highest
+		       resolution will be used.
  
 Optional trigger Nodes:
   - Required properties:
@@ -40,6 +48,9 @@ adc0: adc at fffb0000 {
 	atmel,adc-trigger-register = <0x08>;
 	atmel,adc-use-external;
 	atmel,adc-vref = <3300>;
+	atmel,adc-res = <8 10>;
+	atmel,adc-res-names = "lowres", "highres";
+	atmel,adc-use-res = "lowres";
 
 	trigger at 0 {
 		trigger-name = "external-rising";
diff --git a/arch/arm/boot/dts/at91sam9260.dtsi b/arch/arm/boot/dts/at91sam9260.dtsi
index cca34bc..7c605d2 100644
--- a/arch/arm/boot/dts/at91sam9260.dtsi
+++ b/arch/arm/boot/dts/at91sam9260.dtsi
@@ -506,6 +506,9 @@
 				atmel,adc-drdy-mask = <0x10000>;
 				atmel,adc-status-register = <0x1c>;
 				atmel,adc-trigger-register = <0x04>;
+				atmel,adc-res = <8 10>;
+				atmel,adc-res-names = "lowres", "highres";
+				atmel,adc-use-res = "highres";
 
 				trigger at 0 {
 					trigger-name = "timer-counter-0";
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index 253afed..70c776e 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -488,6 +488,9 @@
 				atmel,adc-drdy-mask = <0x10000>;
 				atmel,adc-status-register = <0x1c>;
 				atmel,adc-trigger-register = <0x08>;
+				atmel,adc-res = <8 10>;
+				atmel,adc-res-names = "lowres", "highres";
+				atmel,adc-use-res = "highres";
 
 				trigger at 0 {
 					trigger-name = "external-rising";
diff --git a/arch/arm/boot/dts/at91sam9x5.dtsi b/arch/arm/boot/dts/at91sam9x5.dtsi
index 70c1f5d..e52862d 100644
--- a/arch/arm/boot/dts/at91sam9x5.dtsi
+++ b/arch/arm/boot/dts/at91sam9x5.dtsi
@@ -469,6 +469,9 @@
 				atmel,adc-drdy-mask = <0x1000000>;
 				atmel,adc-status-register = <0x30>;
 				atmel,adc-trigger-register = <0xc0>;
+				atmel,adc-res = <8 10>;
+				atmel,adc-res-names = "lowres", "highres";
+				atmel,adc-use-res = "highres";
 
 				trigger at 0 {
 					trigger-name = "external-rising";
diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index 315bed1..f175a86 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -57,6 +57,8 @@ struct at91_adc_state {
 	u32			trigger_number;
 	bool			use_external;
 	u32			vref_mv;
+	u32			res;		/* resolution used for convertions */
+	bool			low_res;	/* the resolution corresponds to the lowest one */
 	wait_queue_head_t	wq_data_avail;
 };
 
@@ -138,7 +140,7 @@ static int at91_adc_channel_init(struct iio_dev *idev)
 		chan->channel = bit;
 		chan->scan_index = idx;
 		chan->scan_type.sign = 'u';
-		chan->scan_type.realbits = 10;
+		chan->scan_type.realbits = st->res;
 		chan->scan_type.storagebits = 16;
 		chan->info_mask = IIO_CHAN_INFO_SCALE_SHARED_BIT |
 			IIO_CHAN_INFO_RAW_SEPARATE_BIT;
@@ -372,6 +374,59 @@ static int at91_adc_read_raw(struct iio_dev *idev,
 	return -EINVAL;
 }
 
+static int at91_adc_of_get_resolution(struct at91_adc_state *st,
+				      struct platform_device *pdev)
+{
+	struct iio_dev *idev = iio_priv_to_dev(st);
+	struct device_node *np = pdev->dev.of_node;
+	int count, i, ret = 0;
+	char *res_name, *s;
+	u32 *resolutions;
+
+	count = of_property_count_strings(np, "atmel,adc-res-names");
+	if (count < 2) {
+		dev_err(&idev->dev, "You must specified at least two resolution names for "
+				    "adc-res-names property in the DT\n");
+		return count;
+	}
+
+	resolutions = kmalloc(count * sizeof(*resolutions), GFP_KERNEL);
+	if (!resolutions)
+		return -ENOMEM;
+
+	if (of_property_read_u32_array(np, "atmel,adc-res", resolutions, count)) {
+		dev_err(&idev->dev, "Missing adc-res property in the DT.\n");
+		ret = -ENODEV;
+		goto ret;
+	}
+
+	if (of_property_read_string(np, "atmel,adc-use-res", (const char **)&res_name))
+		res_name = "highres";
+
+	for (i = 0; i < count; i++) {
+		if (of_property_read_string_index(np, "atmel,adc-res-names", i, (const char **)&s))
+			continue;
+
+		if (strcmp(res_name, s))
+			continue;
+
+		st->res = resolutions[i];
+		if (!strcmp(res_name, "lowres"))
+			st->low_res = true;
+		else
+			st->low_res = false;
+
+		dev_info(&idev->dev, "Resolution used: %u bits\n", st->res);
+		goto ret;
+	}
+
+	dev_err(&idev->dev, "There is no resolution for %s\n", res_name);
+
+ret:
+	kfree(resolutions);
+	return ret;
+}
+
 static int at91_adc_probe_dt(struct at91_adc_state *st,
 			     struct platform_device *pdev)
 {
@@ -415,6 +470,10 @@ static int at91_adc_probe_dt(struct at91_adc_state *st,
 	}
 	st->vref_mv = prop;
 
+	ret = at91_adc_of_get_resolution(st, pdev);
+	if (ret)
+		goto error_ret;
+
 	st->registers = devm_kzalloc(&idev->dev,
 				     sizeof(struct at91_adc_reg_desc),
 				     GFP_KERNEL);
@@ -628,9 +687,16 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	 */
 	ticks = round_up((st->startup_time * adc_clk /
 			  1000000) - 1, 8) / 8;
-	at91_adc_writel(st, AT91_ADC_MR,
-			(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
-			(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
+
+	if (st->low_res)
+		at91_adc_writel(st, AT91_ADC_MR,
+				AT91_ADC_LOWRES |
+				(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
+				(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
+	else
+		at91_adc_writel(st, AT91_ADC_MR,
+				(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
+				(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
 
 	/* Setup the ADC channels available on the board */
 	ret = at91_adc_channel_init(idev);
-- 
1.7.10.4

[PATCH 2/3] ARM: AT91: IIO: add sleep mode support

[Jean-Christophe PLAGNIOL-VILLARD]

The sleep mode will allow to put the add in sleep between conversion.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-iio at vger.kernel.org
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
 drivers/iio/adc/at91_adc.c                          |   19 ++++++++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
index fd2d69e..efb6f02 100644
--- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
@@ -25,6 +25,7 @@ Optional properties:
   - atmel,adc-use-res: String corresponding to an identifier from
 		       atmel,adc-res-names property. If not specified, the highest
 		       resolution will be used.
+  - atmel,atmel,adc-sleep-mode: Boolean to enable of sleep mode when no conversion
  
 Optional trigger Nodes:
   - Required properties:
diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index f175a86..c563488 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -52,6 +52,7 @@ struct at91_adc_state {
 	void __iomem		*reg_base;
 	struct at91_adc_reg_desc *registers;
 	u8			startup_time;
+	bool			sleep_mode;
 	struct iio_trigger	**trig;
 	struct at91_adc_trigger	*trigger_list;
 	u32			trigger_number;
@@ -455,6 +456,8 @@ static int at91_adc_probe_dt(struct at91_adc_state *st,
 	}
 	st->num_channels = prop;
 
+	st->sleep_mode = of_property_read_bool(node, "atmel,adc-sleep-mode");
+
 	if (of_property_read_u32(node, "atmel,adc-startup-time", &prop)) {
 		dev_err(&idev->dev, "Missing adc-startup-time property in the DT.\n");
 		ret = -EINVAL;
@@ -580,6 +583,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	struct iio_dev *idev;
 	struct at91_adc_state *st;
 	struct resource *res;
+	u32 reg;
 
 	idev = iio_device_alloc(sizeof(struct at91_adc_state));
 	if (idev == NULL) {
@@ -687,16 +691,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	 */
 	ticks = round_up((st->startup_time * adc_clk /
 			  1000000) - 1, 8) / 8;
-
+	reg = AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL;
+	reg |= AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP;
 	if (st->low_res)
-		at91_adc_writel(st, AT91_ADC_MR,
-				AT91_ADC_LOWRES |
-				(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
-				(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
-	else
-		at91_adc_writel(st, AT91_ADC_MR,
-				(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
-				(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
+		reg |= AT91_ADC_LOWRES;
+	if (st->sleep_mode)
+		reg |= AT91_ADC_SLEEP;
+	at91_adc_writel(st, AT91_ADC_MR, reg);
 
 	/* Setup the ADC channels available on the board */
 	ret = at91_adc_channel_init(idev);
-- 
1.7.10.4

[PATCH 2/3] ARM: AT91: IIO: add sleep mode support

[Maxime Ripard]

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> The sleep mode will allow to put the add in sleep between conversion.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: linux-iio at vger.kernel.org
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
>  Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
>  drivers/iio/adc/at91_adc.c                          |   19 ++++++++++---------
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> index fd2d69e..efb6f02 100644
> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> @@ -25,6 +25,7 @@ Optional properties:
>    - atmel,adc-use-res: String corresponding to an identifier from
>  		       atmel,adc-res-names property. If not specified, the highest
>  		       resolution will be used.
> +  - atmel,atmel,adc-sleep-mode: Boolean to enable of sleep mode when no conversion
>   
>  Optional trigger Nodes:
>    - Required properties:
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index f175a86..c563488 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -52,6 +52,7 @@ struct at91_adc_state {
>  	void __iomem		*reg_base;
>  	struct at91_adc_reg_desc *registers;
>  	u8			startup_time;
> +	bool			sleep_mode;
>  	struct iio_trigger	**trig;
>  	struct at91_adc_trigger	*trigger_list;
>  	u32			trigger_number;
> @@ -455,6 +456,8 @@ static int at91_adc_probe_dt(struct at91_adc_state *st,
>  	}
>  	st->num_channels = prop;
>  
> +	st->sleep_mode = of_property_read_bool(node, "atmel,adc-sleep-mode");
> +
>  	if (of_property_read_u32(node, "atmel,adc-startup-time", &prop)) {
>  		dev_err(&idev->dev, "Missing adc-startup-time property in the DT.\n");
>  		ret = -EINVAL;
> @@ -580,6 +583,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  	struct iio_dev *idev;
>  	struct at91_adc_state *st;
>  	struct resource *res;
> +	u32 reg;
>  
>  	idev = iio_device_alloc(sizeof(struct at91_adc_state));
>  	if (idev == NULL) {
> @@ -687,16 +691,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  	 */
>  	ticks = round_up((st->startup_time * adc_clk /
>  			  1000000) - 1, 8) / 8;
> -
> +	reg = AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL;
> +	reg |= AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP;
>  	if (st->low_res)
> -		at91_adc_writel(st, AT91_ADC_MR,
> -				AT91_ADC_LOWRES |
> -				(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
> -				(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
> -	else
> -		at91_adc_writel(st, AT91_ADC_MR,
> -				(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
> -				(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
> +		reg |= AT91_ADC_LOWRES;
> +	if (st->sleep_mode)
> +		reg |= AT91_ADC_SLEEP;
> +	at91_adc_writel(st, AT91_ADC_MR, reg);

I'm fine with the code in itself, but since this also refactors what has
been added in the previous patch, maybe you can add it in the first patch.

Apart from that, you can add my Acked-by.

Maxime

-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 2/3] ARM: AT91: IIO: add sleep mode support

[Jonathan Cameron]

On 12/20/2012 10:46 AM, Maxime Ripard wrote:
> Hi,
> 
> Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
>> The sleep mode will allow to put the add in sleep between conversion.
>>
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> Cc: linux-iio at vger.kernel.org
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
>> ---
>>  Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
>>  drivers/iio/adc/at91_adc.c                          |   19 ++++++++++---------
>>  2 files changed, 11 insertions(+), 9 deletions(-)
I have no particular problem with this, but would like a brief note on what
one looses when sleep mode is enabled?  How much slower is the conversion if
we first have to come out of sleep mode?   Basically I want a justification
of why we don't always enable this.  Is this hardware specific or
should it simply be controllable from userspace?

>>
>> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
>> index fd2d69e..efb6f02 100644
>> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
>> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
>> @@ -25,6 +25,7 @@ Optional properties:
>>    - atmel,adc-use-res: String corresponding to an identifier from
>>  		       atmel,adc-res-names property. If not specified, the highest
>>  		       resolution will be used.
>> +  - atmel,atmel,adc-sleep-mode: Boolean to enable of sleep mode when no conversion
>>   
>>  Optional trigger Nodes:
>>    - Required properties:
>> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
>> index f175a86..c563488 100644
>> --- a/drivers/iio/adc/at91_adc.c
>> +++ b/drivers/iio/adc/at91_adc.c
>> @@ -52,6 +52,7 @@ struct at91_adc_state {
>>  	void __iomem		*reg_base;
>>  	struct at91_adc_reg_desc *registers;
>>  	u8			startup_time;
>> +	bool			sleep_mode;
>>  	struct iio_trigger	**trig;
>>  	struct at91_adc_trigger	*trigger_list;
>>  	u32			trigger_number;
>> @@ -455,6 +456,8 @@ static int at91_adc_probe_dt(struct at91_adc_state *st,
>>  	}
>>  	st->num_channels = prop;
>>  
>> +	st->sleep_mode = of_property_read_bool(node, "atmel,adc-sleep-mode");
>> +
>>  	if (of_property_read_u32(node, "atmel,adc-startup-time", &prop)) {
>>  		dev_err(&idev->dev, "Missing adc-startup-time property in the DT.\n");
>>  		ret = -EINVAL;
>> @@ -580,6 +583,7 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>>  	struct iio_dev *idev;
>>  	struct at91_adc_state *st;
>>  	struct resource *res;
>> +	u32 reg;
>>  
>>  	idev = iio_device_alloc(sizeof(struct at91_adc_state));
>>  	if (idev == NULL) {
>> @@ -687,16 +691,13 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>>  	 */
>>  	ticks = round_up((st->startup_time * adc_clk /
>>  			  1000000) - 1, 8) / 8;
>> -
>> +	reg = AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL;
>> +	reg |= AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP;
>>  	if (st->low_res)
>> -		at91_adc_writel(st, AT91_ADC_MR,
>> -				AT91_ADC_LOWRES |
>> -				(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
>> -				(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
>> -	else
>> -		at91_adc_writel(st, AT91_ADC_MR,
>> -				(AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL) |
>> -				(AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP));
>> +		reg |= AT91_ADC_LOWRES;
>> +	if (st->sleep_mode)
>> +		reg |= AT91_ADC_SLEEP;
>> +	at91_adc_writel(st, AT91_ADC_MR, reg);
> 
> I'm fine with the code in itself, but since this also refactors what has
> been added in the previous patch, maybe you can add it in the first patch.
> 
> Apart from that, you can add my Acked-by.
> 
> Maxime
> 

[PATCH 2/3] ARM: AT91: IIO: add sleep mode support

[Maxime Ripard]

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> The sleep mode will allow to put the add in sleep between conversion.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: linux-iio at vger.kernel.org
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
>  Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
>  drivers/iio/adc/at91_adc.c                          |   19 ++++++++++---------
>  2 files changed, 11 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> index fd2d69e..efb6f02 100644
> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> @@ -25,6 +25,7 @@ Optional properties:
>    - atmel,adc-use-res: String corresponding to an identifier from
>  		       atmel,adc-res-names property. If not specified, the highest
>  		       resolution will be used.
> +  - atmel,atmel,adc-sleep-mode: Boolean to enable of sleep mode when no conversion

I forgot it in my first review, but there's a typo in the name of the
property here (one extra "atmel,")

Maxime

-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time

[Jean-Christophe PLAGNIOL-VILLARD]

On the at91_adc a minimal Sample and Hold Time is necessary for the ADC to
guarantee the best converted final value between two channels selection.
This time has to be programmed through the bitfield SHTIM in the
Mode Register ADC_MR.

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-iio at vger.kernel.org
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
 drivers/iio/adc/at91_adc.c                          |   15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
index efb6f02..dd2ca90 100644
--- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
+++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
@@ -26,6 +26,7 @@ Optional properties:
 		       atmel,adc-res-names property. If not specified, the highest
 		       resolution will be used.
   - atmel,atmel,adc-sleep-mode: Boolean to enable of sleep mode when no conversion
+  - atmel,adc-sample-hold-time: Sample and Hold Time
  
 Optional trigger Nodes:
   - Required properties:
diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
index c563488..9481375 100644
--- a/drivers/iio/adc/at91_adc.c
+++ b/drivers/iio/adc/at91_adc.c
@@ -52,6 +52,7 @@ struct at91_adc_state {
 	void __iomem		*reg_base;
 	struct at91_adc_reg_desc *registers;
 	u8			startup_time;
+	u8			sample_hold_time;
 	bool			sleep_mode;
 	struct iio_trigger	**trig;
 	struct at91_adc_trigger	*trigger_list;
@@ -465,6 +466,9 @@ static int at91_adc_probe_dt(struct at91_adc_state *st,
 	}
 	st->startup_time = prop;
 
+	prop = 0;
+	of_property_read_u32(node, "atmel,adc-sample-hold-time", &prop);
+	st->sample_hold_time = prop;
 
 	if (of_property_read_u32(node, "atmel,adc-vref", &prop)) {
 		dev_err(&idev->dev, "Missing adc-vref property in the DT.\n");
@@ -578,7 +582,7 @@ static const struct iio_info at91_adc_info = {
 
 static int __devinit at91_adc_probe(struct platform_device *pdev)
 {
-	unsigned int prsc, mstrclk, ticks, adc_clk;
+	unsigned int prsc, mstrclk, ticks, adc_clk, shtim;
 	int ret;
 	struct iio_dev *idev;
 	struct at91_adc_state *st;
@@ -691,12 +695,21 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
 	 */
 	ticks = round_up((st->startup_time * adc_clk /
 			  1000000) - 1, 8) / 8;
+	/*
+	 * a minimal Sample and Hold Time is necessary for the ADC to guarantee
+	 * the best converted final value between two channels selection
+	 * The formula thus is : Sample and Hold Time = (shtim + 1) / ADCClock
+	 */
+	shtim = round_up((st->sample_hold_time * adc_clk /
+			  1000000) - 1, 1);
+
 	reg = AT91_ADC_PRESCAL_(prsc) & AT91_ADC_PRESCAL;
 	reg |= AT91_ADC_STARTUP_(ticks) & AT91_ADC_STARTUP;
 	if (st->low_res)
 		reg |= AT91_ADC_LOWRES;
 	if (st->sleep_mode)
 		reg |= AT91_ADC_SLEEP;
+	reg |= AT91_ADC_SHTIM_(shtim) & AT91_ADC_SHTIM;
 	at91_adc_writel(st, AT91_ADC_MR, reg);
 
 	/* Setup the ADC channels available on the board */
-- 
1.7.10.4

[PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time

[Maxime Ripard]

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> On the at91_adc a minimal Sample and Hold Time is necessary for the ADC to
> guarantee the best converted final value between two channels selection.
> This time has to be programmed through the bitfield SHTIM in the
> Mode Register ADC_MR.
> 
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: linux-iio at vger.kernel.org
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
> ---
>  Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
>  drivers/iio/adc/at91_adc.c                          |   15 ++++++++++++++-
>  2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> index efb6f02..dd2ca90 100644
> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> @@ -26,6 +26,7 @@ Optional properties:
>  		       atmel,adc-res-names property. If not specified, the highest
>  		       resolution will be used.
>    - atmel,atmel,adc-sleep-mode: Boolean to enable of sleep mode when no conversion
> +  - atmel,adc-sample-hold-time: Sample and Hold Time

In which unit? nanoseconds? number of clock ticks?

>   
>  Optional trigger Nodes:
>    - Required properties:
> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> index c563488..9481375 100644
> --- a/drivers/iio/adc/at91_adc.c
> +++ b/drivers/iio/adc/at91_adc.c
> @@ -52,6 +52,7 @@ struct at91_adc_state {
>  	void __iomem		*reg_base;
>  	struct at91_adc_reg_desc *registers;
>  	u8			startup_time;
> +	u8			sample_hold_time;
>  	bool			sleep_mode;
>  	struct iio_trigger	**trig;
>  	struct at91_adc_trigger	*trigger_list;
> @@ -465,6 +466,9 @@ static int at91_adc_probe_dt(struct at91_adc_state *st,
>  	}
>  	st->startup_time = prop;
>  
> +	prop = 0;
> +	of_property_read_u32(node, "atmel,adc-sample-hold-time", &prop);
> +	st->sample_hold_time = prop;
>  
>  	if (of_property_read_u32(node, "atmel,adc-vref", &prop)) {
>  		dev_err(&idev->dev, "Missing adc-vref property in the DT.\n");
> @@ -578,7 +582,7 @@ static const struct iio_info at91_adc_info = {
>  
>  static int __devinit at91_adc_probe(struct platform_device *pdev)
>  {
> -	unsigned int prsc, mstrclk, ticks, adc_clk;
> +	unsigned int prsc, mstrclk, ticks, adc_clk, shtim;
>  	int ret;
>  	struct iio_dev *idev;
>  	struct at91_adc_state *st;
> @@ -691,12 +695,21 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
>  	 */
>  	ticks = round_up((st->startup_time * adc_clk /
>  			  1000000) - 1, 8) / 8;
> +	/*
> +	 * a minimal Sample and Hold Time is necessary for the ADC to guarantee
> +	 * the best converted final value between two channels selection
> +	 * The formula thus is : Sample and Hold Time = (shtim + 1) / ADCClock
> +	 */
> +	shtim = round_up((st->sample_hold_time * adc_clk /
> +			  1000000) - 1, 1);
> +

Since, from all the datasheet I have (G20, G45 and X5), the minimum time
is always 500ns, do we really need to add a dt property for that?

Maxime

-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:55 Thu 20 Dec     , Maxime Ripard wrote:
> Hi,
> 
> Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> > On the at91_adc a minimal Sample and Hold Time is necessary for the ADC to
> > guarantee the best converted final value between two channels selection.
> > This time has to be programmed through the bitfield SHTIM in the
> > Mode Register ADC_MR.
> > 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: linux-iio at vger.kernel.org
> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> > Cc: Ludovic Desroches <ludovic.desroches@atmel.com>
> > ---
> >  Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
> >  drivers/iio/adc/at91_adc.c                          |   15 ++++++++++++++-
> >  2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> > index efb6f02..dd2ca90 100644
> > --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
> > +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> > @@ -26,6 +26,7 @@ Optional properties:
> >  		       atmel,adc-res-names property. If not specified, the highest
> >  		       resolution will be used.
> >    - atmel,atmel,adc-sleep-mode: Boolean to enable of sleep mode when no conversion
> > +  - atmel,adc-sample-hold-time: Sample and Hold Time
> 
> In which unit? nanoseconds? number of clock ticks?
> 
> >   
> >  Optional trigger Nodes:
> >    - Required properties:
> > diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c
> > index c563488..9481375 100644
> > --- a/drivers/iio/adc/at91_adc.c
> > +++ b/drivers/iio/adc/at91_adc.c
> > @@ -52,6 +52,7 @@ struct at91_adc_state {
> >  	void __iomem		*reg_base;
> >  	struct at91_adc_reg_desc *registers;
> >  	u8			startup_time;
> > +	u8			sample_hold_time;
> >  	bool			sleep_mode;
> >  	struct iio_trigger	**trig;
> >  	struct at91_adc_trigger	*trigger_list;
> > @@ -465,6 +466,9 @@ static int at91_adc_probe_dt(struct at91_adc_state *st,
> >  	}
> >  	st->startup_time = prop;
> >  
> > +	prop = 0;
> > +	of_property_read_u32(node, "atmel,adc-sample-hold-time", &prop);
> > +	st->sample_hold_time = prop;
> >  
> >  	if (of_property_read_u32(node, "atmel,adc-vref", &prop)) {
> >  		dev_err(&idev->dev, "Missing adc-vref property in the DT.\n");
> > @@ -578,7 +582,7 @@ static const struct iio_info at91_adc_info = {
> >  
> >  static int __devinit at91_adc_probe(struct platform_device *pdev)
> >  {
> > -	unsigned int prsc, mstrclk, ticks, adc_clk;
> > +	unsigned int prsc, mstrclk, ticks, adc_clk, shtim;
> >  	int ret;
> >  	struct iio_dev *idev;
> >  	struct at91_adc_state *st;
> > @@ -691,12 +695,21 @@ static int __devinit at91_adc_probe(struct platform_device *pdev)
> >  	 */
> >  	ticks = round_up((st->startup_time * adc_clk /
> >  			  1000000) - 1, 8) / 8;
> > +	/*
> > +	 * a minimal Sample and Hold Time is necessary for the ADC to guarantee
> > +	 * the best converted final value between two channels selection
> > +	 * The formula thus is : Sample and Hold Time = (shtim + 1) / ADCClock
> > +	 */
> > +	shtim = round_up((st->sample_hold_time * adc_clk /
> > +			  1000000) - 1, 1);
> > +
> 
> Since, from all the datasheet I have (G20, G45 and X5), the minimum time
> is always 500ns, do we really need to add a dt property for that?
you must this is hardware specific it depend on the line capacity

Best Regards,
J.

[PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time

[Maxime Ripard]

Le 20/12/2012 13:49, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> On 11:55 Thu 20 Dec     , Maxime Ripard wrote:
>> Since, from all the datasheet I have (G20, G45 and X5), the minimum time
>> is always 500ns, do we really need to add a dt property for that?
> you must this is hardware specific it depend on the line capacity

Ok, I see. The comment about adding the unit in the documentation is
still valid though.

Maxime

-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc

[Maxime Ripard]

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> From: Ludovic Desroches <ludovic.desroches@atmel.com>
> 
> at91 adc offers the choice between two resolutions: low and high.
> The low and high resolution values depends on adc IP version, as many IP
> properties have been exposed through device tree, these settings have also
> been added to the dt bindings.
> 
> Update at the same time the dtsi.
> 
> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
> [plagnioj at jcrosoft.com: udpate current adc dt support]
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: linux-iio at vger.kernel.org
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
>  .../devicetree/bindings/arm/atmel-adc.txt          |   11 +++
>  arch/arm/boot/dts/at91sam9260.dtsi                 |    3 +
>  arch/arm/boot/dts/at91sam9g45.dtsi                 |    3 +
>  arch/arm/boot/dts/at91sam9x5.dtsi                  |    3 +
>  drivers/iio/adc/at91_adc.c                         |   74 ++++++++++++++++++--
>  5 files changed, 90 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> index c63097d..fd2d69e 100644
> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
> @@ -14,9 +14,17 @@ Required properties:
>    - atmel,adc-status-register: Offset of the Interrupt Status Register
>    - atmel,adc-trigger-register: Offset of the Trigger Register
>    - atmel,adc-vref: Reference voltage in millivolts for the conversions
> +  - atmel,adc-res: List of resolution in bits supported by the ADC. List size
> +		   must be two at least.
> +  - atmel,adc-res-names: Contains one identifier string for each resolution
> +			 in atmel,adc-res property. "lowres" and "highres"
> +			 identifiers are required.
>  
>  Optional properties:
>    - atmel,adc-use-external: Boolean to enable of external triggers
> +  - atmel,adc-use-res: String corresponding to an identifier from
> +		       atmel,adc-res-names property. If not specified, the highest
> +		       resolution will be used.


I'm wondering, why are you using such a complex dt parsing code, and
bindings, when you only requires a boolean to switch between 8 and 10
bits mode (which seem to be the only thing you support)?

Maxime

-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc

[ludovic.desroches]

Hi Maxime,

On 12/20/2012 11:42 AM, Maxime Ripard wrote:
> Hi,
>
> Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
>> From: Ludovic Desroches <ludovic.desroches@atmel.com>
>>
>> at91 adc offers the choice between two resolutions: low and high.
>> The low and high resolution values depends on adc IP version, as many IP
>> properties have been exposed through device tree, these settings have also
>> been added to the dt bindings.
>>
>> Update at the same time the dtsi.
>>
>> Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
>> [plagnioj at jcrosoft.com: udpate current adc dt support]
>> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
>> Cc: linux-iio at vger.kernel.org
>> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>> ---
>>   .../devicetree/bindings/arm/atmel-adc.txt          |   11 +++
>>   arch/arm/boot/dts/at91sam9260.dtsi                 |    3 +
>>   arch/arm/boot/dts/at91sam9g45.dtsi                 |    3 +
>>   arch/arm/boot/dts/at91sam9x5.dtsi                  |    3 +
>>   drivers/iio/adc/at91_adc.c                         |   74 ++++++++++++++++++--
>>   5 files changed, 90 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt
>> index c63097d..fd2d69e 100644
>> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt
>> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt
>> @@ -14,9 +14,17 @@ Required properties:
>>     - atmel,adc-status-register: Offset of the Interrupt Status Register
>>     - atmel,adc-trigger-register: Offset of the Trigger Register
>>     - atmel,adc-vref: Reference voltage in millivolts for the conversions
>> +  - atmel,adc-res: List of resolution in bits supported by the ADC. List size
>> +		   must be two at least.
>> +  - atmel,adc-res-names: Contains one identifier string for each resolution
>> +			 in atmel,adc-res property. "lowres" and "highres"
>> +			 identifiers are required.
>>
>>   Optional properties:
>>     - atmel,adc-use-external: Boolean to enable of external triggers
>> +  - atmel,adc-use-res: String corresponding to an identifier from
>> +		       atmel,adc-res-names property. If not specified, the highest
>> +		       resolution will be used.
>
>
> I'm wondering, why are you using such a complex dt parsing code, and
> bindings, when you only requires a boolean to switch between 8 and 10
> bits mode (which seem to be the only thing you support)?

We will have a 10 and 12 bits mode on future ADCs and I would like to 
have something which could manage more than two resolutions if it 
happens one day.

Regards

Ludovic

>
> Maxime
>

[PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc

[Maxime Ripard]

Hi Ludovic,

Le 20/12/2012 11:52, ludovic.desroches a ?crit :
>> I'm wondering, why are you using such a complex dt parsing code, and
>> bindings, when you only requires a boolean to switch between 8 and 10
>> bits mode (which seem to be the only thing you support)?
> 
> We will have a 10 and 12 bits mode on future ADCs and I would like to
> have something which could manage more than two resolutions if it
> happens one day.

I see your point. I'm not fond at all of the existing bindings for the
driver (putting things like registers offset in the dt is a non-sense to
me, but hey...), so I'd like to still keep it as simple and non-bloated
as possible, but it's true that in the current situation, we probably
have no other choice.

Maxime
-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc

[ludovic.desroches]

On 12/20/2012 04:51 PM, Maxime Ripard wrote:
> Hi Ludovic,
>
> Le 20/12/2012 11:52, ludovic.desroches a ?crit :
>>> I'm wondering, why are you using such a complex dt parsing code, and
>>> bindings, when you only requires a boolean to switch between 8 and 10
>>> bits mode (which seem to be the only thing you support)?
>>
>> We will have a 10 and 12 bits mode on future ADCs and I would like to
>> have something which could manage more than two resolutions if it
>> happens one day.
>
> I see your point. I'm not fond at all of the existing bindings for the
> driver (putting things like registers offset in the dt is a non-sense to
> me, but hey...), so I'd like to still keep it as simple and non-bloated
> as possible, but it's true that in the current situation, we probably
> have no other choice.
>

I have the same feeling than you about ADC bindings, I think that there 
are too many parameters exhibited. Moreover, most of them are chip relative.

I have not found other ways to deal with it properly. I would like to 
have them hidden into the driver (depending on the compatible string) 
but it will be a mess if we split parameters into dt and driver.

So the idea was to have a binding which could allow us to manage several 
cases about resolution modes in the future.

Ludovic

> Maxime
>

[PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc

[Maxime Ripard]

Le 20/12/2012 17:13, ludovic.desroches a ?crit :
> On 12/20/2012 04:51 PM, Maxime Ripard wrote:
>> Hi Ludovic,
>>
>> Le 20/12/2012 11:52, ludovic.desroches a ?crit :
>>>> I'm wondering, why are you using such a complex dt parsing code, and
>>>> bindings, when you only requires a boolean to switch between 8 and 10
>>>> bits mode (which seem to be the only thing you support)?
>>>
>>> We will have a 10 and 12 bits mode on future ADCs and I would like to
>>> have something which could manage more than two resolutions if it
>>> happens one day.
>>
>> I see your point. I'm not fond at all of the existing bindings for the
>> driver (putting things like registers offset in the dt is a non-sense to
>> me, but hey...), so I'd like to still keep it as simple and non-bloated
>> as possible, but it's true that in the current situation, we probably
>> have no other choice.
>>
> 
> I have the same feeling than you about ADC bindings, I think that there
> are too many parameters exhibited. Moreover, most of them are chip
> relative.
> 
> I have not found other ways to deal with it properly. I would like to
> have them hidden into the driver (depending on the compatible string)
> but it will be a mess if we split parameters into dt and driver.

I'm glad we feel the same way :)

> So the idea was to have a binding which could allow us to manage several
> cases about resolution modes in the future.

Yes, I got the idea. Like I said, if we don't want to touch at the
bindings and do want you suggested, then your patch is the best solution
we have.

Maxime
-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:07 Fri 21 Dec     , Maxime Ripard wrote:
> Le 20/12/2012 17:13, ludovic.desroches a ?crit :
> > On 12/20/2012 04:51 PM, Maxime Ripard wrote:
> >> Hi Ludovic,
> >>
> >> Le 20/12/2012 11:52, ludovic.desroches a ?crit :
> >>>> I'm wondering, why are you using such a complex dt parsing code, and
> >>>> bindings, when you only requires a boolean to switch between 8 and 10
> >>>> bits mode (which seem to be the only thing you support)?
> >>>
> >>> We will have a 10 and 12 bits mode on future ADCs and I would like to
> >>> have something which could manage more than two resolutions if it
> >>> happens one day.
> >>
> >> I see your point. I'm not fond at all of the existing bindings for the
> >> driver (putting things like registers offset in the dt is a non-sense to
> >> me, but hey...), so I'd like to still keep it as simple and non-bloated
> >> as possible, but it's true that in the current situation, we probably
> >> have no other choice.
> >>
> > 
> > I have the same feeling than you about ADC bindings, I think that there
> > are too many parameters exhibited. Moreover, most of them are chip
> > relative.
> > 
> > I have not found other ways to deal with it properly. I would like to
> > have them hidden into the driver (depending on the compatible string)
> > but it will be a mess if we split parameters into dt and driver.
> 
> I'm glad we feel the same way :)
I'm not I fee; exactly this opposte way

I do want to toucht the kernel code to add new soc

so no I like this binding

Best Regards,
J.

[PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc

[Maxime Ripard]

Le 22/12/2012 18:21, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> On 10:07 Fri 21 Dec     , Maxime Ripard wrote:
>> Le 20/12/2012 17:13, ludovic.desroches a ?crit :
>>> I have not found other ways to deal with it properly. I would like to
>>> have them hidden into the driver (depending on the compatible string)
>>> but it will be a mess if we split parameters into dt and driver.
>>
>> I'm glad we feel the same way :)
> I'm not I fee; exactly this opposte way
> 
> I do want to toucht the kernel code to add new soc

This is an argument we already had, and yes, I still don't like these
bindings.

And both this patchset and what Ludovic and you said in this thread have
proven that this is something that can't be achieved, you'll always have
to modify the driver anyway.

-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/3] IIO: AT91: add lowres, sleep and sample&hold time missing support

[Maxime Ripard]

Hi,

Le 19/12/2012 19:32, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
> Hi,
> 
> 	The current adc driver is missing some key feature support
> 	 - select high/low resolution support
> 	 - sleep mode
> 	 - Sample and Hold Time
> 
> 	 This patch seris fix it
> 	 Without it the current driver is not usable in most of the case

Could you put me in Cc for the next iterations please?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 0/3] IIO: AT91: add lowres, sleep and sample&hold time missing support

[Nicolas Ferre]

On 12/19/2012 07:32 PM, Jean-Christophe PLAGNIOL-VILLARD :
> Hi,
> 
> 	The current adc driver is missing some key feature support
> 	 - select high/low resolution support
> 	 - sleep mode
> 	 - Sample and Hold Time
> 
> 	 This patch seris fix it
> 	 Without it the current driver is not usable in most of the case
> 
> Jean-Christophe PLAGNIOL-VILLARD (2):
>       ARM: AT91: IIO: add sleep mode support
>       ARM: AT91: IIO: fix missing Sample and Hold time
> 
> Ludovic Desroches (1):
>       ARM: AT91: IIO: add low and high res support for adc
> 
>  Documentation/devicetree/bindings/arm/atmel-adc.txt |   13 ++++++++++++
>  arch/arm/boot/dts/at91sam9260.dtsi                  |    3 +++
>  arch/arm/boot/dts/at91sam9g45.dtsi                  |    3 +++
>  arch/arm/boot/dts/at91sam9x5.dtsi                   |    3 +++
>  drivers/iio/adc/at91_adc.c                          |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----

As this series is touching both a driver and the DT files, we will have
to pay attention to the path that we will take for upstream.

Arm-soc is my preferred location...


Bye,

>  5 files changed, 107 insertions(+), 5 deletions(-)
> 
> Best Regards,
> J.
> 


-- 
Nicolas Ferre

[PATCH 0/3] IIO: AT91: add lowres, sleep and sample&hold time missing support

[Jonathan Cameron]

On 12/20/2012 10:51 AM, Nicolas Ferre wrote:
> On 12/19/2012 07:32 PM, Jean-Christophe PLAGNIOL-VILLARD :
>> Hi,
>>
>> 	The current adc driver is missing some key feature support
>> 	 - select high/low resolution support
>> 	 - sleep mode
>> 	 - Sample and Hold Time
>>
>> 	 This patch seris fix it
>> 	 Without it the current driver is not usable in most of the case
>>
>> Jean-Christophe PLAGNIOL-VILLARD (2):
>>       ARM: AT91: IIO: add sleep mode support
>>       ARM: AT91: IIO: fix missing Sample and Hold time
>>
>> Ludovic Desroches (1):
>>       ARM: AT91: IIO: add low and high res support for adc
>>
>>  Documentation/devicetree/bindings/arm/atmel-adc.txt |   13 ++++++++++++
>>  arch/arm/boot/dts/at91sam9260.dtsi                  |    3 +++
>>  arch/arm/boot/dts/at91sam9g45.dtsi                  |    3 +++
>>  arch/arm/boot/dts/at91sam9x5.dtsi                   |    3 +++
>>  drivers/iio/adc/at91_adc.c                          |   90 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
> 
> As this series is touching both a driver and the DT files, we will have
> to pay attention to the path that we will take for upstream.
> 
> Arm-soc is my preferred location...

I don't particularly care as I don't know of anything else that is planned for
this cycle that will touch this driver but generally I'd consider this series
driver changes with additions to the device tree to support new functionality.

The only reason to merge through the arm-soc tree I can see is that there is
likely to be more churn in the device tree files.

There is also the question of whether that missing sample and hold time is
a fix we want to push out this cycle, or whether it can wait for the next
merge window.  I'm not clear from the description.

Jonathan

> 
> 
> Bye,
> 
>>  5 files changed, 107 insertions(+), 5 deletions(-)
>>
>> Best Regards,
>> J.
>>
> 
>