| Message ID | 1355942232-26251-1-git-send-email-plagnioj@jcrosoft.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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@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)? 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 >
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
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 >
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
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.
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.
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@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@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@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@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@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);