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

[Richard Fitzgerald]

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@xxxxxxxxxxxxxxxxxxxxx>
> > ---
> >   .../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