| Message ID | 20130319170322.27828.7691.stgit@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Martin Fuzzey wrote: > +If a clock is provided, clock-frequency is optional > + > +If no clock is provided clock-frequency is required (this represents the codec > +being clocked by an external signal not present in the clock tree) > + > +If both a clock and clock-frequency are provided the clock's rate will be set. > + > + > Example: > > codec: sgtl5000@0a { > compatible = "fsl,sgtl5000"; > reg = <0x0a>; > + clock-frequency = <20000000>; > + clocks = <&clks 162>; /* cko1 */ The code looks okay, but I'm not sure this example is right. The binding says that clock-frequency is optional if 'clocks' is provided, but here you list both. You should probably have two examples.
On 21/03/13 02:35, Timur Tabi wrote: > Martin Fuzzey wrote: >> +If a clock is provided, clock-frequency is optional >> + >> +If no clock is provided clock-frequency is required (this represents >> the codec >> +being clocked by an external signal not present in the clock tree) >> + >> +If both a clock and clock-frequency are provided the clock's rate >> will be set. >> + >> + >> Example: >> >> codec: sgtl5000@0a { >> compatible = "fsl,sgtl5000"; >> reg = <0x0a>; >> + clock-frequency = <20000000>; >> + clocks = <&clks 162>; /* cko1 */ > > The code looks okay, but I'm not sure this example is right. The > binding says that clock-frequency is optional if 'clocks' is provided, > but here you list both. You should probably have two examples. > Hello and thank you for your review. No the example is correct. With this patch and that DT example the frequency of clock 162 will be _set_ to 20MHz If clock-frequency is omitted the binding is still correct (hence the optional) but the frequency of clock 162 would not be modified. In the documentation I wrote "If both a clock and clock-frequency are provided the clock's rate will be set. " maybe this is not clear enough? Regards, Martin
On Thu, Mar 21, 2013 at 3:39 AM, Martin Fuzzey <mfuzzey@parkeon.com> wrote: > With this patch and that DT example the frequency of clock 162 will be _set_ > to 20MHz Does that mean that it ignores the ' clocks' parameter? > If clock-frequency is omitted the binding is still correct (hence the > optional) but the frequency of clock 162 would not be modified. > > In the documentation I wrote "If both a clock and clock-frequency are > provided the clock's rate will be set. " maybe this is not clear enough? It's not clear what it will be set *to*.
On 21/03/13 17:11, Timur Tabi wrote: > On Thu, Mar 21, 2013 at 3:39 AM, Martin Fuzzey <mfuzzey@parkeon.com> wrote: > >> With this patch and that DT example the frequency of clock 162 will be _set_ >> to 20MHz > Does that mean that it ignores the ' clocks' parameter? No, in this case the clocks parameter must contain the phandle of a clock whose rate is configurable. > If clock-frequency is omitted the binding is still correct (hence the > optional) but the frequency of clock 162 would not be modified. > > In the documentation I wrote "If both a clock and clock-frequency are > provided the clock's rate will be set. " maybe this is not clear enough? > It's not clear what it will be set *to*. It will be set to the frequency specified by the clock-frquency attribute. Would "If both a clock and clock-frequency are provided clock must support the set_rate operation and its frequency will be set to the value specified by clock-frequency" be better? The possible configurations and their use cases are: 1) only 'clocks' specified clock points to a clock specified in the DT which already has an appropriate frequency and is configured by some other means external to the sgtl5000 driver (bootloader, board setup code, or just a fixed rate clock) 2) only 'clock-frequency' specified The chip is assumed to be clocked by a signal having the given frequency, which may even be generated by a clock unknown to linux. This could actually be represented as a special case of 1) by defining a fixed-rate clock in the DT. 3) Both 'clocks' and 'clock-frequency' specified The chip is assumed to be clocked by a rate programmable clock defined in the clock tree. clk_set_rate() will be called for this clock to set its rate to that specified by clock-frequency Cases 1 and 2 exist in the current code, this patch adds support for case 3. Prior to commit 81e8e4926167ab32593bbb915b45a42024ca1020 "ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000" only case 2 was supported This explains why 2 is not implemented as a special case of 1) plain 'clock-frequency' was supported before the driver learnt how to get a clock from the DT Regards, Martin
On 03/21/2013 12:46 PM, Martin Fuzzey wrote: > On 21/03/13 17:11, Timur Tabi wrote: >> On Thu, Mar 21, 2013 at 3:39 AM, Martin Fuzzey <mfuzzey@parkeon.com> wrote: >> >>> With this patch and that DT example the frequency of clock 162 will be _set_ >>> to 20MHz >> Does that mean that it ignores the ' clocks' parameter? > No, in this case the clocks parameter must contain the phandle of a clock > whose rate is configurable. > >> If clock-frequency is omitted the binding is still correct (hence the >> optional) but the frequency of clock 162 would not be modified. >> >> In the documentation I wrote "If both a clock and clock-frequency are >> provided the clock's rate will be set. " maybe this is not clear enough? >> It's not clear what it will be set *to*. > It will be set to the frequency specified by the clock-frquency attribute. > > Would > "If both a clock and clock-frequency are provided clock must support > the set_rate operation and its frequency will be set to the value specified > by clock-frequency" be better? Yes, that's much better. > > > The possible configurations and their use cases are: > > 1) only 'clocks' specified > clock points to a clock specified in the DT which already has an > appropriate frequency and is > configured by some other means external to the sgtl5000 driver > (bootloader, board setup code, or just a fixed rate clock) > > 2) only 'clock-frequency' specified > The chip is assumed to be clocked by a signal having the given > frequency, which may even be generated by a clock unknown to linux. > This could actually be represented as a special case of 1) by defining a > fixed-rate clock in the DT. > > 3) Both 'clocks' and 'clock-frequency' specified > The chip is assumed to be clocked by a rate programmable clock defined > in the clock tree. > clk_set_rate() will be called for this clock to set its rate to that > specified by clock-frequency This should all be in the binding document. > Cases 1 and 2 exist in the current code, this patch adds support for case 3. > > Prior to commit 81e8e4926167ab32593bbb915b45a42024ca1020 "ASoC: fsl: add > sgtl5000 clock support for imx-sgtl5000" > only case 2 was supported > This explains why 2 is not implemented as a special case of 1) > plain 'clock-frequency' was supported before the driver learnt how to > get a clock from the DT And this should be in the patch description.
diff --git a/Documentation/devicetree/bindings/sound/sgtl5000.txt b/Documentation/devicetree/bindings/sound/sgtl5000.txt index 9cc4444..31e82f0 100644 --- a/Documentation/devicetree/bindings/sound/sgtl5000.txt +++ b/Documentation/devicetree/bindings/sound/sgtl5000.txt @@ -5,9 +5,23 @@ Required properties: - reg : the I2C address of the device +Optional properties: +- clocks: phandle to of codec clock +- clock-frequency: clock frequency to set or use (see below) + +If a clock is provided, clock-frequency is optional + +If no clock is provided clock-frequency is required (this represents the codec +being clocked by an external signal not present in the clock tree) + +If both a clock and clock-frequency are provided the clock's rate will be set. + + Example: codec: sgtl5000@0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; + clock-frequency = <20000000>; + clocks = <&clks 162>; /* cko1 */ }; diff --git a/sound/soc/fsl/imx-sgtl5000.c b/sound/soc/fsl/imx-sgtl5000.c index 424347e..71ce1f6 100644 --- a/sound/soc/fsl/imx-sgtl5000.c +++ b/sound/soc/fsl/imx-sgtl5000.c @@ -128,18 +128,30 @@ static int imx_sgtl5000_probe(struct platform_device *pdev) goto fail; } + ret = of_property_read_u32(codec_np, "clock-frequency", + &data->clk_frequency); data->codec_clk = clk_get(&codec_dev->dev, NULL); + if (IS_ERR(data->codec_clk)) { - /* assuming clock enabled by default */ + /* assuming clock enabled by default (frequency required) */ data->codec_clk = NULL; - ret = of_property_read_u32(codec_np, "clock-frequency", - &data->clk_frequency); - if (ret) { + if (!data->clk_frequency) { dev_err(&codec_dev->dev, "clock-frequency missing or invalid\n"); goto fail; } } else { + if (data->clk_frequency) { + ret = clk_set_rate( + data->codec_clk, data->clk_frequency); + if (ret) { + dev_err(&codec_dev->dev, + "failed to set clock-frequency to %u\n", + data->clk_frequency); + goto fail; + } + } + data->clk_frequency = clk_get_rate(data->codec_clk); clk_prepare_enable(data->codec_clk); }
If both clocks and clock-frequency DT properties are given set the clock frequency. This is useful when a programmable clock is used as the codec clock. Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com> --- .../devicetree/bindings/sound/sgtl5000.txt | 14 ++++++++++++++ sound/soc/fsl/imx-sgtl5000.c | 20 ++++++++++++++++---- 2 files changed, 30 insertions(+), 4 deletions(-)