Sound: sgtl5000 Allow codec clock frequency to be set.

PATCH Superseded Martin Fuzzey 2 participants 6 messages Mar 19, 2013 – Mar 21, 2013

Subsystems (5)

Device tree bindings Sound Arm i.MX and Layerscape ASoC +1 more subsystems

lore

Share & tools

lore

Martin Fuzzey

author

2013-03-19 17:03 UTC #0 lore raw

PATCH 1/1 Sound: sgtl5000 Allow codec clock frequency to be set.

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 <[email protected]>

---
 .../devicetree/bindings/sound/sgtl5000.txt         |   14 ++++++++++++++
 sound/soc/fsl/imx-sgtl5000.c                       |   20 ++++++++++++++++----
 2 files changed, 30 insertions(+), 4 deletions(-)

Documentation/devicetree/bindings/sound/sgtl5000.txt +14 -0

index 9cc4444..31e82f0 100644
@@ -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 */
 };

sound/soc/fsl/imx-sgtl5000.c +16 -4

index 424347e..71ce1f6 100644
@@ -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);
 	}

Timur Tabi

2013-03-21 01:35 UTC #1 lore raw

Quoted 5 lines · Martin Fuzzey

Documentation/devicetree/bindings/sound/sgtl5000.txt +5 -0

+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)
+

Documentation/devicetree/bindings/sound/sgtl5000.txt Required properties: · line 26

Quoted 10 lines

Documentation/devicetree/bindings/sound/sgtl5000.txt +5 -0

+If both a clock and clock-frequency are provided the clock's rate will be set.
+
+
  Example:
 
  codec: sgtl5000 at 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.

Signature · 1 line

Timur Tabi

Martin Fuzzey

author

2013-03-21 08:39 UTC #2 lore raw

Quoted 13 lines · Timur Tabi

Quoted 12 lines · Martin Fuzzey
Documentation/devicetree/bindings/sound/sgtl5000.txt +3 -0
+If a clock is provided, clock-frequency is optional
+
+If no clock is provided clock-frequency is required (this represents 
the codec
Documentation/devicetree/bindings/sound/sgtl5000.txt +3 -0
+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.
Documentation/devicetree/bindings/sound/sgtl5000.txt +2 -0
+
+
  Example:

Quoted 8 lines

Documentation/devicetree/bindings/sound/sgtl5000.txt Required properties: · line 26
Quoted 5 lines
  codec: sgtl5000@0a {
      compatible = "fsl,sgtl5000";
      reg = <0x0a>;
Documentation/devicetree/bindings/sound/sgtl5000.txt +2 -0
+    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

Timur Tabi

2013-03-21 16:11 UTC #3 lore raw

Quoted 2 lines · Martin Fuzzey

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?

Quoted 5 lines

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*.

Martin Fuzzey

author

2013-03-21 17:46 UTC #4 lore raw

Quoted 5 lines · Timur Tabi

Quoted 2 lines · Martin Fuzzey
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.

Quoted 6 lines

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

Timur Tabi

2013-03-21 17:52 UTC #5 lore raw

Quoted 15 lines · Martin Fuzzey

Quoted 5 lines · Timur Tabi
Quoted 2 lines · Martin Fuzzey
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.
Quoted 6 lines
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*.

Quoted 6 lines

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.

Quoted 16 lines

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.

Quoted 5 lines

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.

Quoted 8 lines

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.

Signature · 1 line

Timur Tabi