| Message ID | 20130322093344.20605.49509.stgit@localhost (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Mar 22, 2013 at 4:33 AM, Martin Fuzzey <mfuzzey@parkeon.com> wrote: > 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); I just noticed that you assign 'ret' here, but you never check the value. So I think you should either: 1) Check the value of 'ret' 2) Remove the "ret = "
I'm a little confused why there need to be 3 different alternative bindings. The first is valid (phandle only), the second is redundant (clock-frequency only) - why not just REQUIRE a clock phandle? There is not a single use case where you supply a valid clock on the PCB but cannot write it's frequency into a 3-line DT node, except in allowing non-internally-consistent device trees (which I frown at :) I can think of several reasons I don't like the 3rd way (dynamic clock, phandle+frequency) because it implies lack of trust in the bootloader and/or early platform init code... that said since you need the frequency value to configure the codec, you either get it from the clock (but it may not be valid) or you force a value upon it from clock-frequency.. I would like a little warning, maybe, that the expected clock-frequency was not available at the time it read the value from the phandle, since this would allow sgtl5000-using hardware designers and linux implementers to know immediately that they have missed something important (bootloader setup of the codec clock). It's rare that any real hardware design house would leave a clock like that unconfigured as being able to test the frequency from whatever source and how clean it is and what the levels are for debugging is easier done in the bootloader than with an operating system running (re power management etc.) but for the cases where we, for want of a better way of describing it, forgot to actually set it up.. it should still really, really be in the bootloader and bitching about it. I wholeheartedly support the third method (clock+freq) in the hope that it forces bootloader developers to properly configure fixed clocks (such as the codec clock which is very hard to change at runtime) at boot, before the OS has any need for it (and to relieve it of the task of setting the clock frequency), but on the basis that eventually people would get a clue or get their task list in order for bootloader support and make it redundant (at which point it could be removed..). Second method (just the frequency alone) has to go away though in my opinion, third way should be considered a "legacy" check in migration from non-DT to DT support.
Hi Matt, On 25/03/13 21:22, Matt Sealey wrote: > I'm a little confused why there need to be 3 different alternative bindings. > > The first is valid (phandle only), the second is redundant > (clock-frequency only) - why not just REQUIRE a clock phandle? There > is not a single use case where you supply a valid clock on the PCB but > cannot write it's frequency into a 3-line DT node, except in allowing > non-internally-consistent device trees (which I frown at :) Yes this is true, however the current code already has this method (which was actually the first to be implemented). Removing it would break compatibility with existing DTs. I'm not sure what the policy on DT backwards compatibility is on the scale "never break backwards compatibility" (like userspace) to "anything goes if you fix the in tree damage" (internal kernel APIs). But clearly over time it is going to have to be more on the "never break backwards compatibility" side, especially If we are talking about hosting the DTs elsewhere than in the kernel sources. > I can think of several reasons I don't like the 3rd way (dynamic > clock, phandle+frequency) because it implies lack of trust in the > bootloader and/or early platform init code... that said since you need > the frequency value to configure the codec, you either get it from the > clock (but it may not be valid) or you force a value upon it from > clock-frequency.. I would like a little warning, maybe, that the > expected clock-frequency was not available at the time it read the > value from the phandle, since this would allow sgtl5000-using hardware > designers and linux implementers to know immediately that they have > missed something important (bootloader setup of the codec clock). I don't agree here. In my opinion the bootloader should only need to set up the hardware that is essential to boot (RAM timings and any hardware necessary to access the boot media) The rest can, and I believe should, be left to the kernel. Since: * there is only one kernel whereas there are multiple bootloaders. * on many platforms, updating the bootloader is more complicated and/or risky than updating the kernel. > It's rare that any real hardware design house would leave a clock like > that unconfigured as being able to test the frequency from whatever > source and how clean it is and what the levels are for debugging is > easier done in the bootloader than with an operating system running > (re power management etc.) but for the cases where we, for want of a > better way of describing it, forgot to actually set it up.. it should > still really, really be in the bootloader and bitching about it. Yes it may well be easier in some cases to do hardware design / board bringup from the bootloader but that doesn't mean that the bootloader code used by the hardware engineers to do it will or even should be the same as the production bootloader. So I still think the kernel should make the minimum assumptions on what the bootloader has done. > I wholeheartedly support the third method (clock+freq) in the hope > that it forces bootloader developers to properly configure fixed > clocks (such as the codec clock which is very hard to change at > runtime) at boot, before the OS has any need for it (and to relieve it > of the task of setting the clock frequency), but on the basis that > eventually people would get a clue or get their task list in order for > bootloader support and make it redundant (at which point it could be > removed..). Sorry not following you here regarding the third (clock + freq) method - first you say "I can think of several reasons I don't like the 3rd way" then here "I wholeheartedly support the third method" Regards, Martin
On 22/03/13 15:08, Timur Tabi wrote: > On Fri, Mar 22, 2013 at 4:33 AM, Martin Fuzzey <mfuzzey@parkeon.com> wrote: > >> 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); > I just noticed that you assign 'ret' here, but you never check the > value. So I think you should either: > > 1) Check the value of 'ret' > 2) Remove the "ret = " Initially I did check the value of 'ret', instead of checking data->clk_frequency. However I found that checking a generic return variable 'ret' several lines later to be not very clear (to see what 'ret' refers to you have to backtrack while reading the code). The ret can't be checked directly after assigning it as it's not always an error condition (in the phandle only case an error here is normal). Checking data->clk_frequency, IMHO makes the code more readable. If I remove the "ret =" no error code will be propagated back to the caller. Sure I could add an assignment of ret to -EINVAL in the error case like this: 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 (frequency required) */ data->codec_clk = NULL; if (!data->clk_frequency) { dev_err(&codec_dev->dev, "clock-frequency missing or invalid\n"); ret = -EINVAL; goto fail; } } else { But I don't think this is better than just propating the value from of_property_read_u32() as it loses information. Martin
diff --git a/Documentation/devicetree/bindings/sound/sgtl5000.txt b/Documentation/devicetree/bindings/sound/sgtl5000.txt index 9cc4444..f586ba7 100644 --- a/Documentation/devicetree/bindings/sound/sgtl5000.txt +++ b/Documentation/devicetree/bindings/sound/sgtl5000.txt @@ -5,9 +5,68 @@ 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 clock must support +the set_rate operation and its frequency will be set to the value specified +by clock-frequency + + +Hence 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) + +Example: +clocks { + audioclk: ext20Mz { + compatible = "fixed-clock"; + clock-frequency = <20000000>; + }; +}; + +codec: sgtl5000@0a { + compatible = "fsl,sgtl5000"; + reg = <0x0a>; + clocks = <&audioclk>; /* cko1 */ +}; + + +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. + +Example: + +codec: sgtl5000@0a { + compatible = "fsl,sgtl5000"; + reg = <0x0a>; + clock-frequency = <20000000>; +}; + + +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 + Example: codec: sgtl5000@0a { compatible = "fsl,sgtl5000"; reg = <0x0a>; + clocks = <&clks 162>; /* cko1 */ + clock-frequency = <20000000>; }; 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); }
Currently the sgtl5000 driver allows its clock to be specified either as a raw frequency using "clock-frequency" in the device tree (the original method) or, since commit 81e8e49 "ASoC: fsl: add sgtl5000 clock support for imx-sgtl5000", as a preconfigured clock from the device tree. This patch adds, in addition to the existing methods, the possibility to use a rate configurable clock from the device tree and actually set its rate. This is done by specifiying both clocks and clock-frequency in the driver's DT node. This is useful when a programmable clock is used as the codec clock. Signed-off-by: Martin Fuzzey <mfuzzey@parkeon.com> --- Changelog: V2 - Improved binding document and patch description following remarks from Timur Tabi. --- .../devicetree/bindings/sound/sgtl5000.txt | 59 ++++++++++++++++++++ sound/soc/fsl/imx-sgtl5000.c | 20 +++++-- 2 files changed, 75 insertions(+), 4 deletions(-)