diff mbox series

[v2,2/7] ASoC: audio-graph-card: Add plls and sysclks DT bindings

Message ID 20201016173541.21180-3-rf@opensource.cirrus.com
State New
Headers show
Series Add support for Rpi4b + Cirrus Lochnagar2 and CS47L15 | expand

Commit Message

Richard Fitzgerald Oct. 16, 2020, 5:35 p.m. UTC
This adds the two new properties 'plls' and 'sysclks' to the dt bindings.
These add the ability to set values that will be
passed to snd_soc_component_set_sysclk() and snd_soc_component_set_pll().

Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
---
 .../bindings/sound/audio-graph-card.txt       | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)

Comments

Rob Herring (Arm) Oct. 26, 2020, 1:27 p.m. UTC | #1
On Fri, Oct 16, 2020 at 06:35:36PM +0100, Richard Fitzgerald wrote:
> This adds the two new properties 'plls' and 'sysclks' to the dt bindings.
> These add the ability to set values that will be
> passed to snd_soc_component_set_sysclk() and snd_soc_component_set_pll().

I worry this looks like Linux implementation details leaking into the 
binding.

> 
> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
> ---
>  .../bindings/sound/audio-graph-card.txt       | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/sound/audio-graph-card.txt b/Documentation/devicetree/bindings/sound/audio-graph-card.txt
> index d5f6919a2d69..59bbd5b55b59 100644
> --- a/Documentation/devicetree/bindings/sound/audio-graph-card.txt
> +++ b/Documentation/devicetree/bindings/sound/audio-graph-card.txt
> @@ -32,6 +32,19 @@ Required properties:
>  Optional properties:
>  - pa-gpios: GPIO used to control external amplifier.
>  
> +- plls: A list of component pll settings that will be applied with
> +      snd_soc_component_set_pll. Each entry is a phandle to the node of the
> +      codec or cpu component, followed by the four arguments id, source,
> +      frequency_in, frequency_out. Multiple entries can have the same phandle
> +      so that several plls can be set in the same component.

Where do the values of id and source come from?

> +
> +- sysclks: A list of component sysclk settings that will be applied with
> +      snd_soc_component_set_sysclk. Each entry is a phandle to the node of
> +      the codec or cpu component, followed by the four arguments id, source,
> +      frequency, direction. Direction is 0 if the clock is an input, 1 if it
> +      is an output. Multiple entries can have the same phandle so that several
> +      clocks can be set in the same component.

Are these really common properties? They seem kind of Cirrus specific 
and perhaps should be located in the codec node(s).

> +
>  -----------------------
>  Example: Single DAI case
>  -----------------------
> @@ -335,3 +348,34 @@ Example: Multi DAI with DPCM
>  			};
>  		};
>  	};
> +
> +-----------------------
> +Example: Set component sysclks and PLLs
> +-----------------------
> +
> +	sound {
> +		compatible = "audio-graph-card";
> +
> +		sysclks = <
> +			&cs47l15 1 4 98304000 0
> +			&cs47l15 8 4 147456000 0
> +		>;
> +		plls = <
> +			&cs47l15 1 0 24576000 98304000
> +		>;
> +
> +		dais = <&cpu_i2s_port>;
> +	};
> +
> +	cs47l15: codec@0 {
> +		...
> +		ports {
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			cs47l15_aif1_port: port@0 {
> +				reg = <0>;
> +				cs47l15_aif1: endpoint {
> +					remote-endpoint = <&cpu_i2s_endpoint>;
> +				};
> +			};
> +	};
> -- 
> 2.20.1
>
Mark Brown Oct. 26, 2020, 1:38 p.m. UTC | #2
On Mon, Oct 26, 2020 at 08:27:04AM -0500, Rob Herring wrote:
> On Fri, Oct 16, 2020 at 06:35:36PM +0100, Richard Fitzgerald wrote:

> > +- plls: A list of component pll settings that will be applied with
> > +      snd_soc_component_set_pll. Each entry is a phandle to the node of the
> > +      codec or cpu component, followed by the four arguments id, source,
> > +      frequency_in, frequency_out. Multiple entries can have the same phandle
> > +      so that several plls can be set in the same component.

> Where do the values of id and source come from?

The device bindings will need to define them.

> > +- sysclks: A list of component sysclk settings that will be applied with
> > +      snd_soc_component_set_sysclk. Each entry is a phandle to the node of
> > +      the codec or cpu component, followed by the four arguments id, source,
> > +      frequency, direction. Direction is 0 if the clock is an input, 1 if it
> > +      is an output. Multiple entries can have the same phandle so that several
> > +      clocks can be set in the same component.

> Are these really common properties? They seem kind of Cirrus specific 
> and perhaps should be located in the codec node(s).

It's very common for audio devices to have very flexible clocking, to
the exetent this is Linux specific it's issues with the clock API not
being able to handle clock controllers on buses that need clock control
to access rather than conceptually.
Richard Fitzgerald Nov. 2, 2020, 11:48 a.m. UTC | #3
On 26/10/2020 13:27, Rob Herring wrote:
> On Fri, Oct 16, 2020 at 06:35:36PM +0100, Richard Fitzgerald wrote:
>> This adds the two new properties 'plls' and 'sysclks' to the dt bindings.
>> These add the ability to set values that will be
>> passed to snd_soc_component_set_sysclk() and snd_soc_component_set_pll().
> 
> I worry this looks like Linux implementation details leaking into the
> binding.
>

I guess what you mean is referring to a function to explain the cells.
I thought it would simplify the description but it yes, it does mean
that the binding is tied to details of the kernel APIs. I can rewrite
this description to be explicit about the cells instead of being in
terms of kernel APIs.

>>
>> Signed-off-by: Richard Fitzgerald <rf@opensource.cirrus.com>
>> ---
>>   .../bindings/sound/audio-graph-card.txt       | 44 +++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/sound/audio-graph-card.txt b/Documentation/devicetree/bindings/sound/audio-graph-card.txt
>> index d5f6919a2d69..59bbd5b55b59 100644
>> --- a/Documentation/devicetree/bindings/sound/audio-graph-card.txt
>> +++ b/Documentation/devicetree/bindings/sound/audio-graph-card.txt
>> @@ -32,6 +32,19 @@ Required properties:
>>   Optional properties:
>>   - pa-gpios: GPIO used to control external amplifier.
>>   
>> +- plls: A list of component pll settings that will be applied with
>> +      snd_soc_component_set_pll. Each entry is a phandle to the node of the
>> +      codec or cpu component, followed by the four arguments id, source,
>> +      frequency_in, frequency_out. Multiple entries can have the same phandle
>> +      so that several plls can be set in the same component.
> 
> Where do the values of id and source come from?
>

They are specific to each codec driver, and ultimately depend on the
hardware inside the codec. Compare with for example GPIO numbers being
specific to hardware. I didn't say that because the description refers
to the underlying kernel API, but if I update to not be in terms of an
API I'll also add some more info about the fields.

>> +
>> +- sysclks: A list of component sysclk settings that will be applied with
>> +      snd_soc_component_set_sysclk. Each entry is a phandle to the node of
>> +      the codec or cpu component, followed by the four arguments id, source,
>> +      frequency, direction. Direction is 0 if the clock is an input, 1 if it
>> +      is an output. Multiple entries can have the same phandle so that several
>> +      clocks can be set in the same component.
> 
> Are these really common properties? They seem kind of Cirrus specific
> and perhaps should be located in the codec node(s).
> 

I'm not sure what about this description makes you think it is Cirrus
specific. They are standard ALSA ASoC subsystem APIs. I can find them
used in drivers for Analog Devices, Dialog, Realtek and others, and this
binding could be used for an audio-graph-card driver using those codecs.

It is the ASoC machine driver (in this case audio-graph-card) that
handles this stuff so makes sense for them to be in its node, not the
codec driver. The ASoC structure is somewhat complex but in short the
codec driver provides an implementation for setting the hardware
registers but doesn't know about use-cases or other audio components, so
can't decide clocking. The "machine driver" sits above all the audio
drivers and has a view of the whole audio subsystem so can decide on
use-cases and clocking.

Having said that, we wouldn't need to do this if the kernel clock
framework could support clock controllers on I2C/SPI buses. But years
have gone by and nobody has managed to fix that yet.

>> +
>>   -----------------------
>>   Example: Single DAI case
>>   -----------------------
>> @@ -335,3 +348,34 @@ Example: Multi DAI with DPCM
>>   			};
>>   		};
>>   	};
>> +
>> +-----------------------
>> +Example: Set component sysclks and PLLs
>> +-----------------------
>> +
>> +	sound {
>> +		compatible = "audio-graph-card";
>> +
>> +		sysclks = <
>> +			&cs47l15 1 4 98304000 0
>> +			&cs47l15 8 4 147456000 0
>> +		>;
>> +		plls = <
>> +			&cs47l15 1 0 24576000 98304000
>> +		>;
>> +
>> +		dais = <&cpu_i2s_port>;
>> +	};
>> +
>> +	cs47l15: codec@0 {
>> +		...
>> +		ports {
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +			cs47l15_aif1_port: port@0 {
>> +				reg = <0>;
>> +				cs47l15_aif1: endpoint {
>> +					remote-endpoint = <&cpu_i2s_endpoint>;
>> +				};
>> +			};
>> +	};
>> -- 
>> 2.20.1
>>
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/sound/audio-graph-card.txt b/Documentation/devicetree/bindings/sound/audio-graph-card.txt
index d5f6919a2d69..59bbd5b55b59 100644
--- a/Documentation/devicetree/bindings/sound/audio-graph-card.txt
+++ b/Documentation/devicetree/bindings/sound/audio-graph-card.txt
@@ -32,6 +32,19 @@  Required properties:
 Optional properties:
 - pa-gpios: GPIO used to control external amplifier.
 
+- plls: A list of component pll settings that will be applied with
+      snd_soc_component_set_pll. Each entry is a phandle to the node of the
+      codec or cpu component, followed by the four arguments id, source,
+      frequency_in, frequency_out. Multiple entries can have the same phandle
+      so that several plls can be set in the same component.
+
+- sysclks: A list of component sysclk settings that will be applied with
+      snd_soc_component_set_sysclk. Each entry is a phandle to the node of
+      the codec or cpu component, followed by the four arguments id, source,
+      frequency, direction. Direction is 0 if the clock is an input, 1 if it
+      is an output. Multiple entries can have the same phandle so that several
+      clocks can be set in the same component.
+
 -----------------------
 Example: Single DAI case
 -----------------------
@@ -335,3 +348,34 @@  Example: Multi DAI with DPCM
 			};
 		};
 	};
+
+-----------------------
+Example: Set component sysclks and PLLs
+-----------------------
+
+	sound {
+		compatible = "audio-graph-card";
+
+		sysclks = <
+			&cs47l15 1 4 98304000 0
+			&cs47l15 8 4 147456000 0
+		>;
+		plls = <
+			&cs47l15 1 0 24576000 98304000
+		>;
+
+		dais = <&cpu_i2s_port>;
+	};
+
+	cs47l15: codec@0 {
+		...
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			cs47l15_aif1_port: port@0 {
+				reg = <0>;
+				cs47l15_aif1: endpoint {
+					remote-endpoint = <&cpu_i2s_endpoint>;
+				};
+			};
+	};