IIO: AT91: add lowres, sleep and sample&hold time missing support

PATCH New Jean-Christophe PLAGNIOL-VILLARD 5 participants 20 messages Dec 19, 2012 – Jan 7, 2013

Subsystems (3)

Device tree bindings IIO Arm/Microchip (AT91) SoC support

lore

Share & tools

lore

Jean-Christophe PLAGNIOL-VILLARD

author

2012-12-19 18:32 UTC #0 lore raw

cover IIO: AT91: add lowres, sleep and sample&hold time missing support

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.

Jean-Christophe PLAGNIOL-VILLARD

author

2012-12-19 18:37 UTC #1 lore raw

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

From: Ludovic Desroches <[email protected]>

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 <[email protected]>

[plagnioj at jcrosoft.com: udpate current adc dt support]

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

Cc linux-iio at vger.kernel.org

Cc Nicolas Ferre <[email protected]>

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

Documentation/devicetree/bindings/arm/atmel-adc.txt +11 -0

index c63097d..fd2d69e 100644
@@ -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";

arch/arm/boot/dts/at91sam9260.dtsi +3 -0

index cca34bc..7c605d2 100644
@@ -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";

arch/arm/boot/dts/at91sam9g45.dtsi +3 -0

index 253afed..70c776e 100644
@@ -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";

arch/arm/boot/dts/at91sam9x5.dtsi +3 -0

index 70c1f5d..e52862d 100644
@@ -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";

drivers/iio/adc/at91_adc.c +70 -4

104 lines not on this page · raw message

Signature · 1 line

1.7.10.4

Jean-Christophe PLAGNIOL-VILLARD

author

2012-12-19 18:37 UTC #2 lore raw

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

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

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

Cc linux-iio at vger.kernel.org

Cc Nicolas Ferre <[email protected]>

Cc Ludovic Desroches <[email protected]>

---
 Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
 drivers/iio/adc/at91_adc.c                          |   19 ++++++++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

Documentation/devicetree/bindings/arm/atmel-adc.txt +1 -0

index fd2d69e..efb6f02 100644
@@ -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:

drivers/iio/adc/at91_adc.c +10 -9

index f175a86..c563488 100644
@@ -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);

Signature · 1 line

1.7.10.4

Maxime Ripard

2012-12-20 10:46 UTC #3 lore raw

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

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :

Quoted 62 lines

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

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>

Cc linux-iio at vger.kernel.org

Cc Nicolas Ferre <[email protected]>

Cc Ludovic Desroches <[email protected]>

---
 Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
 drivers/iio/adc/at91_adc.c                          |   19 ++++++++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)

Documentation/devicetree/bindings/arm/atmel-adc.txt +1 -0

index fd2d69e..efb6f02 100644
@@ -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:

drivers/iio/adc/at91_adc.c +6 -3

index f175a86..c563488 100644
@@ -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 |

drivers/iio/adc/at91_adc.c static int __devinit at91_adc_probe(struct platform_device *pdev) · line 700

Quoted 10 lines

drivers/iio/adc/at91_adc.c +4 -6

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

Signature · 4 lines

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

Jonathan Cameron

maintainer

2012-12-27 12:09 UTC #4 lore raw

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

Quoted 13 lines · Maxime Ripard

Hi,
Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
Quoted 10 lines
The sleep mode will allow to put the add in sleep between conversion.
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc linux-iio at vger.kernel.org
Cc Nicolas Ferre <[email protected]>
Cc Ludovic Desroches <[email protected]>
---
 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?

Quoted 70 lines

Quoted 52 lines

Documentation/devicetree/bindings/arm/atmel-adc.txt +1 -0

index fd2d69e..efb6f02 100644
@@ -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:

drivers/iio/adc/at91_adc.c +6 -3

index f175a86..c563488 100644
@@ -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 |

drivers/iio/adc/at91_adc.c static int __devinit at91_adc_probe(struct platform_device *pdev) · line 700

Quoted 10 lines

drivers/iio/adc/at91_adc.c +4 -6

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

2012-12-27 17:54 UTC #5 lore raw

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

In reply to Jean-Christophe PLAGNIOL-VILLARD

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :

Quoted 16 lines

The sleep mode will allow to put the add in sleep between conversion.
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc linux-iio at vger.kernel.org
Cc Nicolas Ferre <[email protected]>
Cc Ludovic Desroches <[email protected]>
---
 Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
 drivers/iio/adc/at91_adc.c                          |   19 ++++++++++---------
 2 files changed, 11 insertions(+), 9 deletions(-)
Documentation/devicetree/bindings/arm/atmel-adc.txt +0 -0
index fd2d69e..efb6f02 100644

Documentation/devicetree/bindings/arm/atmel-adc.txt Optional properties: · line 28

Quoted 4 lines

Documentation/devicetree/bindings/arm/atmel-adc.txt +1 -0

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

Signature · 4 lines

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

Jean-Christophe PLAGNIOL-VILLARD

author

2012-12-19 18:37 UTC #6 lore raw

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

In reply to 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 <[email protected]>

Cc linux-iio at vger.kernel.org

Cc Nicolas Ferre <[email protected]>

Cc Ludovic Desroches <[email protected]>

---
 Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
 drivers/iio/adc/at91_adc.c                          |   15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/arm/atmel-adc.txt +1 -0

index efb6f02..dd2ca90 100644
@@ -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:

drivers/iio/adc/at91_adc.c +14 -1

index c563488..9481375 100644
@@ -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 */

Signature · 1 line

1.7.10.4

Maxime Ripard

2012-12-20 10:55 UTC #7 lore raw

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

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :

Quoted 19 lines

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 <[email protected]>
Cc linux-iio at vger.kernel.org
Cc Nicolas Ferre <[email protected]>
Cc Ludovic Desroches <[email protected]>
---
 Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
 drivers/iio/adc/at91_adc.c                          |   15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/arm/atmel-adc.txt +0 -0
index efb6f02..dd2ca90 100644

Documentation/devicetree/bindings/arm/atmel-adc.txt Optional properties: · line 29

Quoted 4 lines

Documentation/devicetree/bindings/arm/atmel-adc.txt +1 -0

@@ -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?

Quoted 36 lines

Documentation/devicetree/bindings/arm/atmel-adc.txt +0 -0

 Optional trigger Nodes:
   - Required properties:

drivers/iio/adc/at91_adc.c +5 -1

index c563488..9481375 100644
@@ -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)
 	 */

drivers/iio/adc/at91_adc.c static int __devinit at91_adc_probe(struct platform_device *pdev) · line 705

Quoted 10 lines

drivers/iio/adc/at91_adc.c +8 -0

 	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

Signature · 4 lines

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

Jean-Christophe PLAGNIOL-VILLARD

author

2012-12-20 12:49 UTC #8 lore raw

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

Quoted 70 lines · Maxime Ripard

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :

Quoted 19 lines

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 <[email protected]>
Cc linux-iio at vger.kernel.org
Cc Nicolas Ferre <[email protected]>
Cc Ludovic Desroches <[email protected]>
---
 Documentation/devicetree/bindings/arm/atmel-adc.txt |    1 +
 drivers/iio/adc/at91_adc.c                          |   15 ++++++++++++++-
 2 files changed, 15 insertions(+), 1 deletion(-)
Documentation/devicetree/bindings/arm/atmel-adc.txt +0 -0
index efb6f02..dd2ca90 100644

Documentation/devicetree/bindings/arm/atmel-adc.txt Optional properties: · line 29

Quoted 4 lines

Documentation/devicetree/bindings/arm/atmel-adc.txt +1 -0

@@ -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?

Quoted 41 lines

Documentation/devicetree/bindings/arm/atmel-adc.txt +0 -0

 Optional trigger Nodes:
   - Required properties:

drivers/iio/adc/at91_adc.c +8 -1

index c563488..9481375 100644
@@ -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

Quoted 8 lines

drivers/iio/adc/at91_adc.c static int __devinit at91_adc_probe(struct platform_device *pdev) · line 705
Quoted 5 lines
drivers/iio/adc/at91_adc.c +5 -0
+	 * 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.

Maxime Ripard

2012-12-21 09:08 UTC #9 lore raw

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

Le 20/12/2012 13:49, Jean-Christophe PLAGNIOL-VILLARD a ?crit :

Quoted 4 lines · Maxime Ripard

Quoted 2 lines
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

Signature · 4 lines

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

Maxime Ripard

2012-12-20 10:42 UTC #10 lore raw

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

In reply to Jean-Christophe PLAGNIOL-VILLARD

Hi,

Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :

Quoted 31 lines

From: Ludovic Desroches <[email protected]>
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 <[email protected]>
[plagnioj at jcrosoft.com: udpate current adc dt support]
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc linux-iio at vger.kernel.org
Cc Nicolas Ferre <[email protected]>
---
 .../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(-)
Documentation/devicetree/bindings/arm/atmel-adc.txt +1 -0
index c63097d..fd2d69e 100644
@@ -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

Documentation/devicetree/bindings/arm/atmel-adc.txt Required properties: · line 27

Quoted 10 lines

Documentation/devicetree/bindings/arm/atmel-adc.txt +7 -0

+		   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

Signature · 4 lines

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

ludovic.desroches

2012-12-20 10:52 UTC #11 lore raw

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

Hi Maxime,

Quoted 39 lines · Maxime Ripard

Hi,
Le 19/12/2012 19:37, Jean-Christophe PLAGNIOL-VILLARD a ?crit :
Quoted 36 lines
From: Ludovic Desroches <[email protected]>
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 <[email protected]>
[plagnioj at jcrosoft.com: udpate current adc dt support]
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc linux-iio at vger.kernel.org
Cc Nicolas Ferre <[email protected]>
---
  .../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(-)
Documentation/devicetree/bindings/arm/atmel-adc.txt +5 -0
index c63097d..fd2d69e 100644
@@ -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.
 

Quoted 10 lines

Documentation/devicetree/bindings/arm/atmel-adc.txt Required properties: · line 27
Quoted 5 lines
Documentation/devicetree/bindings/arm/atmel-adc.txt +3 -0
  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

Quoted 3 lines

Maxime

Maxime Ripard

2012-12-20 15:51 UTC #12 lore raw

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

Hi Ludovic,

Le 20/12/2012 11:52, ludovic.desroches a ?crit :

Quoted 7 lines

Quoted 3 lines
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

Signature · 4 lines

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

ludovic.desroches

2012-12-20 16:13 UTC #13 lore raw

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

Quoted 11 lines · Maxime Ripard

Hi Ludovic,
Le 20/12/2012 11:52, ludovic.desroches a ?crit :
Quoted 7 lines
Quoted 3 lines
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.

Quoted 6 lines

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

Quoted 2 lines

Maxime

Maxime Ripard

2012-12-21 09:07 UTC #14 lore raw

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

Le 20/12/2012 17:13, ludovic.desroches a ?crit :

Quoted 19 lines · Maxime Ripard

Quoted 11 lines
Hi Ludovic,
Le 20/12/2012 11:52, ludovic.desroches a ?crit :
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
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.

Quoted 7 lines

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

Quoted 2 lines

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

Signature · 4 lines

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

Jean-Christophe PLAGNIOL-VILLARD

author

2012-12-22 17:21 UTC #15 lore raw

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

Quoted 24 lines · Maxime Ripard

Le 20/12/2012 17:13, ludovic.desroches a ?crit :
Quoted 23 lines · Maxime Ripard
Quoted 11 lines
Hi Ludovic,
Le 20/12/2012 11:52, ludovic.desroches a ?crit :
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
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.

Quoted 5 lines

Quoted 3 lines
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.

Maxime Ripard

2013-01-07 09:20 UTC #16 lore raw

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

Le 22/12/2012 18:21, Jean-Christophe PLAGNIOL-VILLARD a ?crit :

Quoted 10 lines · Maxime Ripard

Quoted 6 lines
Le 20/12/2012 17:13, ludovic.desroches a ?crit :
Quoted 3 lines
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.

Signature · 4 lines

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

Maxime Ripard

2012-12-20 10:40 UTC #17 lore raw

cover IIO: AT91: add lowres, sleep and sample&hold time missing support

In reply to Jean-Christophe PLAGNIOL-VILLARD

Hi,

Le 19/12/2012 19:32, Jean-Christophe PLAGNIOL-VILLARD a ?crit :

Quoted 9 lines

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

Signature · 4 lines

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

Nicolas Ferre

2012-12-20 10:51 UTC #18 lore raw

cover IIO: AT91: add lowres, sleep and sample&hold time missing support

In reply to Jean-Christophe PLAGNIOL-VILLARD

On 12/19/2012 07:32 PM, Jean-Christophe PLAGNIOL-VILLARD :

Quoted 14 lines

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

Quoted 8 lines

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,

Quoted 5 lines

 5 files changed, 107 insertions(+), 5 deletions(-)

Best Regards,
J.

Signature · 1 line

Nicolas Ferre

Jonathan Cameron

maintainer

2012-12-27 12:14 UTC #19 lore raw

cover IIO: AT91: add lowres, sleep and sample&hold time missing support

Quoted 24 lines · Nicolas Ferre

On 12/19/2012 07:32 PM, Jean-Christophe PLAGNIOL-VILLARD :
Quoted 14 lines
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
Quoted 8 lines
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 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----

Quoted 4 lines

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

Quoted 11 lines

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