| Message ID | 1373789069-11604-5-git-send-email-josh.wu@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Josh, On Sun, Jul 14, 2013 at 04:04:28PM +0800, Josh Wu wrote: > Signed-off-by: Josh Wu <josh.wu@atmel.com> > --- > Documentation/devicetree/bindings/arm/atmel-adc.txt | 2 ++ > drivers/iio/adc/at91_adc.c | 8 +++++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt > index 16769d9..0db2945 100644 > --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt > +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt > @@ -27,6 +27,8 @@ Optional properties: > resolution will be used. > - atmel,adc-sleep-mode: Boolean to enable sleep mode when no conversion > - atmel,adc-sample-hold-time: Sample and Hold Time in microseconds > + - atmel,adc-clock-rate: ADC clock rate. If not specified, use the default > + adc_op_clk. > > Optional trigger Nodes: > - Required properties: > diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c > index e93a075..8f1386f 100644 > --- a/drivers/iio/adc/at91_adc.c > +++ b/drivers/iio/adc/at91_adc.c > @@ -47,6 +47,7 @@ struct at91_adc_caps { > > struct at91_adc_state { > struct clk *adc_clk; > + u32 adc_clk_rate; > u16 *buffer; > unsigned long channels_mask; > struct clk *clk; > @@ -448,6 +449,10 @@ static int at91_adc_probe_dt(struct at91_adc_state *st, > if (!node) > return -EINVAL; > > + prop = 0; > + of_property_read_u32(node, "atmel,adc-clock-rate", &prop); > + st->adc_clk_rate = prop; > + > st->use_external = of_property_read_bool(node, "atmel,adc-use-external-triggers"); > > if (of_property_read_u32(node, "atmel,adc-channels-used", &prop)) { > @@ -723,7 +728,8 @@ static int at91_adc_probe(struct platform_device *pdev) > * specified by the electrical characteristics of the board. > */ > mstrclk = clk_get_rate(st->clk); > - adc_clk = clk_get_rate(st->adc_clk); > + adc_clk = st->adc_clk_rate ? > + st->adc_clk_rate : clk_get_rate(st->adc_clk); Why is that needed? Isn't it completely redundant with the clocks property? Maxime
Hi, Maxime On 7/15/2013 9:06 PM, Maxime Ripard wrote: > Hi Josh, > > On Sun, Jul 14, 2013 at 04:04:28PM +0800, Josh Wu wrote: >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> --- >> Documentation/devicetree/bindings/arm/atmel-adc.txt | 2 ++ >> drivers/iio/adc/at91_adc.c | 8 +++++++- >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt >> index 16769d9..0db2945 100644 >> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt >> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt >> @@ -27,6 +27,8 @@ Optional properties: >> resolution will be used. >> - atmel,adc-sleep-mode: Boolean to enable sleep mode when no conversion >> - atmel,adc-sample-hold-time: Sample and Hold Time in microseconds >> + - atmel,adc-clock-rate: ADC clock rate. If not specified, use the default >> + adc_op_clk. >> >> Optional trigger Nodes: >> - Required properties: >> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c >> index e93a075..8f1386f 100644 >> --- a/drivers/iio/adc/at91_adc.c >> +++ b/drivers/iio/adc/at91_adc.c >> @@ -47,6 +47,7 @@ struct at91_adc_caps { >> >> struct at91_adc_state { >> struct clk *adc_clk; >> + u32 adc_clk_rate; >> u16 *buffer; >> unsigned long channels_mask; >> struct clk *clk; >> @@ -448,6 +449,10 @@ static int at91_adc_probe_dt(struct at91_adc_state *st, >> if (!node) >> return -EINVAL; >> >> + prop = 0; >> + of_property_read_u32(node, "atmel,adc-clock-rate", &prop); >> + st->adc_clk_rate = prop; >> + >> st->use_external = of_property_read_bool(node, "atmel,adc-use-external-triggers"); >> >> if (of_property_read_u32(node, "atmel,adc-channels-used", &prop)) { >> @@ -723,7 +728,8 @@ static int at91_adc_probe(struct platform_device *pdev) >> * specified by the electrical characteristics of the board. >> */ >> mstrclk = clk_get_rate(st->clk); >> - adc_clk = clk_get_rate(st->adc_clk); >> + adc_clk = st->adc_clk_rate ? >> + st->adc_clk_rate : clk_get_rate(st->adc_clk); > Why is that needed? Isn't it completely redundant with the clocks > property? As st->adc_clk rate is specified in arch/arm/mach-at91/sama5d3.c (take sama5d3 for example), changing the clock rate should recompile the kernel binary. Use dt parameter will let us easily specify the clock rate instead of recompile the code. And yes, it is redundant that we can define the adc_op_clk rate in two places (clock property in .c and adc-clock-rate in dts). But this can be compatible with the non-dt platform. After a further thinking of this, maybe remove the adc_op_clk is better since it is a fake clock, and only used to specify the clock rate. To specify the clock rate use a dt property or platform data parameter is better. > > Maxime > > Best Regards, Josh Wu
On Tue, Jul 16, 2013 at 03:55:28PM +0800, Josh Wu wrote: > >On Sun, Jul 14, 2013 at 04:04:28PM +0800, Josh Wu wrote: > >>diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c > >>index e93a075..8f1386f 100644 > >>--- a/drivers/iio/adc/at91_adc.c > >>+++ b/drivers/iio/adc/at91_adc.c > >>@@ -47,6 +47,7 @@ struct at91_adc_caps { > >> struct at91_adc_state { > >> struct clk *adc_clk; > >>+ u32 adc_clk_rate; > >> u16 *buffer; > >> unsigned long channels_mask; > >> struct clk *clk; > >>@@ -448,6 +449,10 @@ static int at91_adc_probe_dt(struct at91_adc_state *st, > >> if (!node) > >> return -EINVAL; > >>+ prop = 0; > >>+ of_property_read_u32(node, "atmel,adc-clock-rate", &prop); > >>+ st->adc_clk_rate = prop; > >>+ > >> st->use_external = of_property_read_bool(node, "atmel,adc-use-external-triggers"); > >> if (of_property_read_u32(node, "atmel,adc-channels-used", &prop)) { > >>@@ -723,7 +728,8 @@ static int at91_adc_probe(struct platform_device *pdev) > >> * specified by the electrical characteristics of the board. > >> */ > >> mstrclk = clk_get_rate(st->clk); > >>- adc_clk = clk_get_rate(st->adc_clk); > >>+ adc_clk = st->adc_clk_rate ? > >>+ st->adc_clk_rate : clk_get_rate(st->adc_clk); > >Why is that needed? Isn't it completely redundant with the clocks > >property? > > As st->adc_clk rate is specified in arch/arm/mach-at91/sama5d3.c > (take sama5d3 for example), changing the clock rate should recompile > the kernel binary. > Use dt parameter will let us easily specify the clock rate instead > of recompile the code. > > And yes, it is redundant that we can define the adc_op_clk rate in > two places (clock property in .c and adc-clock-rate in dts). But > this can be compatible with the non-dt platform. Yet, it's not, while, like you pointed at, the common clock framework actually *is* usable for both DT and non-DT platforms at no cost. The fact that AT91 isn't using the DT to retrieve its clock tree yet is another story (but I believe that it's a work in progress). > After a further thinking of this, maybe remove the adc_op_clk is > better since it is a fake clock, and only used to specify the clock > rate. > To specify the clock rate use a dt property or platform data > parameter is better. No, to specify *any* clock, the common clock framework is the better solution. Maxime
On 07/16/2013 12:30 PM, Maxime Ripard wrote: [...] >> After a further thinking of this, maybe remove the adc_op_clk is >> better since it is a fake clock, and only used to specify the clock >> rate. >> To specify the clock rate use a dt property or platform data >> parameter is better. > > No, to specify *any* clock, the common clock framework is the better > solution. Yep, this patch is not the right approach. It's trying to work around the limitations of the platforms clock API implementation. Please fix the at91 clock implementation instead (e.g. by switching to the common clock framework). - Lars
On 7/16/2013 7:16 PM, Lars-Peter Clausen wrote: > On 07/16/2013 12:30 PM, Maxime Ripard wrote: > [...] >>> After a further thinking of this, maybe remove the adc_op_clk is >>> better since it is a fake clock, and only used to specify the clock >>> rate. >>> To specify the clock rate use a dt property or platform data >>> parameter is better. >> No, to specify *any* clock, the common clock framework is the better >> solution. > Yep, this patch is not the right approach. It's trying to work around the > limitations of the platforms clock API implementation. Please fix the at91 > clock implementation instead (e.g. by switching to the common clock framework). Thank you and Maxime for the clarify. I will drop this patch in next version. Just FYI. The at91 clock common framework is in implementation by Boris Brezillion. > > - Lars > Best Regards, Josh Wu
Hi Josh, On 14/07/2013 10:04, Josh Wu wrote: > Signed-off-by: Josh Wu <josh.wu@atmel.com> > --- > Documentation/devicetree/bindings/arm/atmel-adc.txt | 2 ++ > drivers/iio/adc/at91_adc.c | 8 +++++++- > 2 files changed, 9 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt > index 16769d9..0db2945 100644 > --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt > +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt > @@ -27,6 +27,8 @@ Optional properties: > resolution will be used. > - atmel,adc-sleep-mode: Boolean to enable sleep mode when no conversion > - atmel,adc-sample-hold-time: Sample and Hold Time in microseconds > + - atmel,adc-clock-rate: ADC clock rate. If not specified, use the default > + adc_op_clk. > > Optional trigger Nodes: > - Required properties: > diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c > index e93a075..8f1386f 100644 > --- a/drivers/iio/adc/at91_adc.c > +++ b/drivers/iio/adc/at91_adc.c > @@ -47,6 +47,7 @@ struct at91_adc_caps { > > struct at91_adc_state { > struct clk *adc_clk; > + u32 adc_clk_rate; > u16 *buffer; > unsigned long channels_mask; > struct clk *clk; > @@ -448,6 +449,10 @@ static int at91_adc_probe_dt(struct at91_adc_state *st, > if (!node) > return -EINVAL; > > + prop = 0; > + of_property_read_u32(node, "atmel,adc-clock-rate", &prop); > + st->adc_clk_rate = prop; > + > st->use_external = of_property_read_bool(node, "atmel,adc-use-external-triggers"); > > if (of_property_read_u32(node, "atmel,adc-channels-used", &prop)) { > @@ -723,7 +728,8 @@ static int at91_adc_probe(struct platform_device *pdev) > * specified by the electrical characteristics of the board. > */ > mstrclk = clk_get_rate(st->clk); > - adc_clk = clk_get_rate(st->adc_clk); > + adc_clk = st->adc_clk_rate ? > + st->adc_clk_rate : clk_get_rate(st->adc_clk); > adc_clk_khz = adc_clk / 1000; > prsc = (mstrclk / (2 * adc_clk)) - 1; > > As said by Maxime and Lars-Peter, I think this should be handled by a proper clock implementation (adc_clock ?) using common clock framework. IMO the fake adc_op_clk should not exist. Instead the adc clock binding should define properties describing the adc clock characteristics (see pll implementation in at91 common clk series). These characteristics can be found in 'ADC characteristics' chapter in atmel's datasheets. The adc clock binding would look like this (based on sama5 datasheet): adc_clk { compatible = "sama5d3-adc-clk"; output = <1000000 20000000>; /* output clock frequency range */ }; It is up to the clk user (adc driver) to choose the appropriate frequency to use (maximum rate ?) and configure it with clk_set_rate function. The adc clk implem will take care of PRESCAL field config in ADC_MR according to parent rate (mck rate) and requested rate. It will also check if the requested rate is in the output clk range. These are just thought, feel free to comment it. Best Regards, Boris
On 25/07/2013 14:01, boris brezillon wrote: > Hi Josh, > > On 14/07/2013 10:04, Josh Wu wrote: >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> --- >> Documentation/devicetree/bindings/arm/atmel-adc.txt | 2 ++ >> drivers/iio/adc/at91_adc.c | 8 +++++++- >> 2 files changed, 9 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt >> b/Documentation/devicetree/bindings/arm/atmel-adc.txt >> index 16769d9..0db2945 100644 >> --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt >> +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt >> @@ -27,6 +27,8 @@ Optional properties: >> resolution will be used. >> - atmel,adc-sleep-mode: Boolean to enable sleep mode when no >> conversion >> - atmel,adc-sample-hold-time: Sample and Hold Time in microseconds >> + - atmel,adc-clock-rate: ADC clock rate. If not specified, use the >> default >> + adc_op_clk. >> >> Optional trigger Nodes: >> - Required properties: >> diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c >> index e93a075..8f1386f 100644 >> --- a/drivers/iio/adc/at91_adc.c >> +++ b/drivers/iio/adc/at91_adc.c >> @@ -47,6 +47,7 @@ struct at91_adc_caps { >> >> struct at91_adc_state { >> struct clk *adc_clk; >> + u32 adc_clk_rate; >> u16 *buffer; >> unsigned long channels_mask; >> struct clk *clk; >> @@ -448,6 +449,10 @@ static int at91_adc_probe_dt(struct >> at91_adc_state *st, >> if (!node) >> return -EINVAL; >> >> + prop = 0; >> + of_property_read_u32(node, "atmel,adc-clock-rate", &prop); >> + st->adc_clk_rate = prop; >> + >> st->use_external = of_property_read_bool(node, >> "atmel,adc-use-external-triggers"); >> >> if (of_property_read_u32(node, "atmel,adc-channels-used", &prop)) { >> @@ -723,7 +728,8 @@ static int at91_adc_probe(struct platform_device >> *pdev) >> * specified by the electrical characteristics of the board. >> */ >> mstrclk = clk_get_rate(st->clk); >> - adc_clk = clk_get_rate(st->adc_clk); >> + adc_clk = st->adc_clk_rate ? >> + st->adc_clk_rate : clk_get_rate(st->adc_clk); >> adc_clk_khz = adc_clk / 1000; >> prsc = (mstrclk / (2 * adc_clk)) - 1; >> >> > > As said by Maxime and Lars-Peter, I think this should be handled by a > proper clock implementation (adc_clock ?) using common clock framework. > > IMO the fake adc_op_clk should not exist. Instead the adc clock binding > should define properties describing the adc clock characteristics (see > pll implementation in at91 common clk series). > These characteristics can be found in 'ADC characteristics' chapter in > atmel's datasheets. > > The adc clock binding would look like this (based on sama5 datasheet): > > adc_clk { > compatible = "sama5d3-adc-clk"; > output = <1000000 20000000>; /* output clock frequency range */ > }; or more like this: adc_clk { compatible = "xxx-adc-clk"; #clock-cells = <0>; clocks = <&mck>; output = <1000000 20000000>; /* output clock frequency range */ }; > > It is up to the clk user (adc driver) to choose the appropriate > frequency to use (maximum rate ?) and configure it with clk_set_rate > function. > The adc clk implem will take care of PRESCAL field config in ADC_MR > according to parent rate (mck rate) and requested rate. It will also > check if the requested rate is in the output clk range. > > These are just thought, feel free to comment it. > > Best Regards, > > Boris > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
diff --git a/Documentation/devicetree/bindings/arm/atmel-adc.txt b/Documentation/devicetree/bindings/arm/atmel-adc.txt index 16769d9..0db2945 100644 --- a/Documentation/devicetree/bindings/arm/atmel-adc.txt +++ b/Documentation/devicetree/bindings/arm/atmel-adc.txt @@ -27,6 +27,8 @@ Optional properties: resolution will be used. - atmel,adc-sleep-mode: Boolean to enable sleep mode when no conversion - atmel,adc-sample-hold-time: Sample and Hold Time in microseconds + - atmel,adc-clock-rate: ADC clock rate. If not specified, use the default + adc_op_clk. Optional trigger Nodes: - Required properties: diff --git a/drivers/iio/adc/at91_adc.c b/drivers/iio/adc/at91_adc.c index e93a075..8f1386f 100644 --- a/drivers/iio/adc/at91_adc.c +++ b/drivers/iio/adc/at91_adc.c @@ -47,6 +47,7 @@ struct at91_adc_caps { struct at91_adc_state { struct clk *adc_clk; + u32 adc_clk_rate; u16 *buffer; unsigned long channels_mask; struct clk *clk; @@ -448,6 +449,10 @@ static int at91_adc_probe_dt(struct at91_adc_state *st, if (!node) return -EINVAL; + prop = 0; + of_property_read_u32(node, "atmel,adc-clock-rate", &prop); + st->adc_clk_rate = prop; + st->use_external = of_property_read_bool(node, "atmel,adc-use-external-triggers"); if (of_property_read_u32(node, "atmel,adc-channels-used", &prop)) { @@ -723,7 +728,8 @@ static int at91_adc_probe(struct platform_device *pdev) * specified by the electrical characteristics of the board. */ mstrclk = clk_get_rate(st->clk); - adc_clk = clk_get_rate(st->adc_clk); + adc_clk = st->adc_clk_rate ? + st->adc_clk_rate : clk_get_rate(st->adc_clk); adc_clk_khz = adc_clk / 1000; prsc = (mstrclk / (2 * adc_clk)) - 1;
Signed-off-by: Josh Wu <josh.wu@atmel.com> --- Documentation/devicetree/bindings/arm/atmel-adc.txt | 2 ++ drivers/iio/adc/at91_adc.c | 8 +++++++- 2 files changed, 9 insertions(+), 1 deletion(-)