* [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
@ 2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-19 18:37 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 2/3] ARM: AT91: IIO: add sleep mode support
2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-19 18:37 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Jean-Christophe PLAGNIOL-VILLARD, linux-iio, Nicolas Ferre,
Ludovic Desroches
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@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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 2/3] ARM: AT91: IIO: add sleep mode support
@ 2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-19 18:37 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 2/3] ARM: AT91: IIO: add sleep mode support
2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-12-20 10:46 ` Maxime Ripard
-1 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-20 10:46 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: linux-arm-kernel, linux-iio, Nicolas Ferre, Ludovic Desroches
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@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
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH 2/3] ARM: AT91: IIO: add sleep mode support
@ 2012-12-20 10:46 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-20 10:46 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 2/3] ARM: AT91: IIO: add sleep mode support
2012-12-20 10:46 ` Maxime Ripard
@ 2012-12-27 12:09 ` Jonathan Cameron
-1 siblings, 0 replies; 38+ messages in thread
From: Jonathan Cameron @ 2012-12-27 12:09 UTC (permalink / raw)
To: Maxime Ripard
Cc: Jean-Christophe PLAGNIOL-VILLARD, linux-arm-kernel, linux-iio,
Nicolas Ferre, Ludovic Desroches
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@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
>
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH 2/3] ARM: AT91: IIO: add sleep mode support
@ 2012-12-27 12:09 ` Jonathan Cameron
0 siblings, 0 replies; 38+ messages in thread
From: Jonathan Cameron @ 2012-12-27 12:09 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 2/3] ARM: AT91: IIO: add sleep mode support
2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-12-27 17:54 ` Maxime Ripard
-1 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-27 17:54 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: linux-arm-kernel, linux-iio, Nicolas Ferre, Ludovic Desroches
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@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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 2/3] ARM: AT91: IIO: add sleep mode support
@ 2012-12-27 17:54 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-27 17:54 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time
2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-19 18:37 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Jean-Christophe PLAGNIOL-VILLARD, linux-iio, Nicolas Ferre,
Ludovic Desroches
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@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
^ permalink raw reply related [flat|nested] 38+ messages in thread* [PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time
@ 2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-19 18:37 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time
2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-12-20 10:55 ` Maxime Ripard
-1 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-20 10:55 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: linux-arm-kernel, linux-iio, Nicolas Ferre, Ludovic Desroches
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@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
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time
@ 2012-12-20 10:55 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-20 10:55 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time
2012-12-20 10:55 ` Maxime Ripard
@ 2012-12-20 12:49 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-20 12:49 UTC (permalink / raw)
To: Maxime Ripard
Cc: linux-arm-kernel, linux-iio, Nicolas Ferre, Ludovic Desroches
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@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.
^ permalink raw reply [flat|nested] 38+ messages in thread* [PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time
@ 2012-12-20 12:49 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-20 12:49 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 38+ messages in thread* Re: [PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time
2012-12-20 12:49 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-12-21 9:08 ` Maxime Ripard
-1 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-21 9:08 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: linux-arm-kernel, linux-iio, Nicolas Ferre, Ludovic Desroches
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 3/3] ARM: AT91: IIO: fix missing Sample and Hold time
@ 2012-12-21 9:08 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-21 9:08 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
2012-12-19 18:37 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-12-20 10:42 ` Maxime Ripard
-1 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-20 10:42 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: linux-arm-kernel, Ludovic Desroches, linux-iio, Nicolas Ferre
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@jcrosoft.com: udpate current adc dt support]
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Cc: linux-iio@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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
@ 2012-12-20 10:42 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-20 10:42 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
2012-12-20 10:42 ` Maxime Ripard
(?)
@ 2012-12-20 10:52 ` ludovic.desroches
2012-12-20 15:51 ` Maxime Ripard
-1 siblings, 1 reply; 38+ messages in thread
From: ludovic.desroches @ 2012-12-20 10:52 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
2012-12-20 10:52 ` ludovic.desroches
@ 2012-12-20 15:51 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-20 15:51 UTC (permalink / raw)
To: ludovic.desroches
Cc: Jean-Christophe PLAGNIOL-VILLARD, linux-arm-kernel, linux-iio,
Nicolas Ferre
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
@ 2012-12-20 15:51 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-20 15:51 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
2012-12-20 15:51 ` Maxime Ripard
(?)
@ 2012-12-20 16:13 ` ludovic.desroches
2012-12-21 9:07 ` Maxime Ripard
-1 siblings, 1 reply; 38+ messages in thread
From: ludovic.desroches @ 2012-12-20 16:13 UTC (permalink / raw)
To: linux-arm-kernel
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
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
2012-12-20 16:13 ` ludovic.desroches
@ 2012-12-21 9:07 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-21 9:07 UTC (permalink / raw)
To: ludovic.desroches
Cc: Jean-Christophe PLAGNIOL-VILLARD, linux-arm-kernel, linux-iio,
Nicolas Ferre
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
@ 2012-12-21 9:07 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2012-12-21 9:07 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
2012-12-21 9:07 ` Maxime Ripard
@ 2012-12-22 17:21 ` Jean-Christophe PLAGNIOL-VILLARD
-1 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-22 17:21 UTC (permalink / raw)
To: Maxime Ripard
Cc: ludovic.desroches, linux-arm-kernel, linux-iio, Nicolas Ferre
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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
@ 2012-12-22 17:21 ` Jean-Christophe PLAGNIOL-VILLARD
0 siblings, 0 replies; 38+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-12-22 17:21 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
2012-12-22 17:21 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2013-01-07 9:20 ` Maxime Ripard
-1 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2013-01-07 9:20 UTC (permalink / raw)
To: Jean-Christophe PLAGNIOL-VILLARD
Cc: ludovic.desroches, linux-arm-kernel, linux-iio, Nicolas Ferre
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 1/3] ARM: AT91: IIO: add low and high res support for adc
@ 2013-01-07 9:20 ` Maxime Ripard
0 siblings, 0 replies; 38+ messages in thread
From: Maxime Ripard @ 2013-01-07 9:20 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 38+ messages in thread