[PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Martin Fuzzey]

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

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);
 	}

[PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Martin Fuzzey]

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

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 at 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);
 	}

Re: [PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Timur Tabi]

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.

-- 
Timur Tabi

[PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Timur Tabi]

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

-- 
Timur Tabi

Re: [PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Martin Fuzzey]

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

[PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Martin Fuzzey]

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

Re: [PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Timur Tabi]

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

[PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Timur Tabi]

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

Re: [PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Martin Fuzzey]

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

[PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Martin Fuzzey]

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

Re: [PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Timur Tabi]

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.

-- 
Timur Tabi

[PATCH] Sound: sgtl5000 Allow codec clock frequency to be set.

[Timur Tabi]

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.

-- 
Timur Tabi