Re: [PATCH v4 2/6] dt-bindings: audio-graph-card: Add plls and sysclks properties

[Richard Fitzgerald]

On 15/01/2021 15:20, Mark Brown wrote:
> On Fri, Jan 15, 2021 at 02:42:12PM +0000, Richard Fitzgerald wrote:
> > On 15/01/2021 13:11, Mark Brown wrote:
> > > On Fri, Jan 15, 2021 at 10:35:23AM +0000, Richard Fitzgerald wrote:
> > > > On 13/01/2021 16:09, Mark Brown wrote:
> > > > > On Wed, Jan 13, 2021 at 09:22:25AM -0600, Rob Herring wrote:
>
> > > > some_codec {
> > > > 	pll: pll {
> > > > 		compatible = "fixed-clock";
> > > > 		clocks = <&audio_mclk>;
> > > > 		clock-frequency = <98304000>;
> > > > 	}
>
> > > A PLL is not a fixed clock, why would you define a fixed clock here?
>
> > It's a fixed clock if you are only setting one configuration. Call it
> > compatible="any-other-dummy-clock-type" if you like, it doesn't matter
> > what it is for the purposes of what I was describing.
>
> > This isn't a clk driver for a pll, it's just a setting to be passed to
> > snd_soc_component_set_pll() using a clock binding to specify it.
>
> So you're trying to describe a crystal on the board?  Why would this be
> a subnode of the CODEC then?  Surely it's just a standard fixed clock
> which provides some input to the CODEC in the same way you'd describe
> any other input to the CODEC.  The above doesn't look anything like the
> hardware.  But if that's what you're doing how is that related to
> configuring the FLL except possibly as the input clock you'd reference?
>
> > > Are you confusing the selection of rates on existing clocks with the use
> > > of the assigned-* properties that the clock binding provides?
>
> > I'm not at all sure what you and Rob have in mind here. Perhaps you
> > could give an example of what you are thinking the .dts would look like
> > to define some pll/sysclk settings for audio-graph-card to apply. An
> > example is worth a thousand emails.
>
> As far as I can tell you are trying to configure the FLL in the CODEC,
> telling it to take an input clock and produce a fixed output clock rate
> from that.  The FLL is a fairly basic clock, there are examples for both
> that and choosing a configuration for a clock in the clock bindings.
>
> > > That seems like a *very* surprising requirement - why would the clock
> > > binding have that requirement?  It would seem to create issues for a
> > > single device providing multiple clocks which should be a pretty common
> > > coase.
>
> > You misunderstand me. What I'm saying is that to do this:
>
> > 	sound {
> > 		clocks = <&pll>;
> > 	}
>
> > The node 'pll' must correspond to a clock provider driver. It can't be
> > just a bare node with some properties pick-n-mixed from the clock
> > binding, like this:
>
> I'm pretty sure I understand you perfectly; again, what makes you say
> that a description of a clock in the device tree has any requirement
> for a separate compatible string?

If I do:
 	sound {
 		clocks = <&clock>;
 	};

	clock: clock {
		compatible = "fixed-clock";
		clock-frequency = <98304000>;
	};

I can clk_bulk_get_all().
But if I remove the 'compatible' from the clock node, clk_bulk_get_all()
will return -EPROBE_DEFER and log:

 /sound: Failed to get clk index: 0 ret: -517

from the error case in _clk_bulk_get() in clk/clk-bulk.c.

> > So the question I'm trying to ask is: when you and Rob said use
> > the clock binding, did you mean pointing to that binding from
> > clocks=<...>, or from a custom property like my audio-graph-card,plls
> > example above.
>
> When we say to use the clock binding what we are saying is to use the
> actual clock bindings to describe the clocks, not make a custom binding
> that looks kind of like them - making a custom binding doesn't address
> the problem.

But I don't know what you mean by "use the actual clock bindings to
describe the clocks".

What is not clear to me is how you want me to use a clock binding to
describe something that isn't a clk-framework clk. If you know what you
want, then please.. an example would help explain.