Re: [PATCH v4 1/1] ASoc: kirkwood: add DT support to the mvebu audio subsystem

Jean-Francois Moine
2013-08-09T09:05:29Z

On Fri, 09 Aug 2013 10:23:50 +0200
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> On 08/08/2013 01:22 PM, Jean-Francois Moine wrote:
> > This patch adds DT support to the audio subsystem of the mvebu family
> > (Kirkwood, Dove, Armada 370).
> >
> > Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> > ---
> > .../devicetree/bindings/sound/mvebu-audio.txt | 29 ++++++++++++++++++++++
> > sound/soc/kirkwood/kirkwood-i2s.c | 26 +++++++++++++++++-----
> > 2 files changed, 49 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/sound/mvebu-audio.txt b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
> > new file mode 100644
> > index 0000000..7e5fd37
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/mvebu-audio.txt
> > @@ -0,0 +1,29 @@
> > +* mvebu (Kirkwood, Dove, Armada 370) audio controller
> > +
> > +Required properties:
> > +
> > +- compatible: "mrvl,mvebu-audio"
>
> Jean-Francois,
>
> we need at least two more compatibles for the audio controller found on
> Dove and Kirkwood respectively. This is how we are going to distinguish
> those two, e.g. Kirkwood has SPDIF in which Dove hasn't.

Sebastian,

s/has/hasn't & s/hasn't/has

Are 2 compatibles enough, i.e. "mvebu-audio" and "mbevu-audio-spdif"?

> Also, we have used "marvell" as prefix for a long time. I know there has
> been discussion about the stock ticker appreviation, has there been any
> decision on that already?

Don't know, but, sure, there are still a lot of "mrvl".

> > +- reg: physical base address of the controller and length of memory mapped
> > + region.
> > +
> > +- interrupts: list of two irq numbers.
> > + The first irq is used for data flow and the second one is used for errors.
> > +
> > +- clocks: one or two phandles.
> > + The first one is mandatory and defines the internal clock.
> > + The second one is optional and defines an external clock.
> > +
> > +- clock-names: names associated to the clocks:
> > + "internal" for the internal clock
>
> s/internal/dcoclk/

Once, I got this message:

On Fri, 26 Jul 2013 10:21:56 +0100 Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> On Fri, Jul 26, 2013 at 11:09:13AM +0200, Jean-Francois Moine wrote:
> > The A510 documentation uses the names "DCO PLL" for the internal clock
> > and "AU_EXTCLK" for the external clock. So, what about "dcopll" and
> > "extclk"?
>
> Stop naming them according to their source. Their _consumer_ names
> not _source_ names.

But Russell did not tell clearly which name could be the best.

BTW, as we are naming the clocks, the 'clk' in "xxxclk" seems redondant...

> > + "extclk" for the external clock
> > +
> > +Example:
> > +
> > +i2s1: audio-controller@b4000 {
> > + compatible = "mrvl,mvebu-audio";
> > + reg = <0xb4000 0x2210>;
> > + interrupts = <21>, <22>;
> > + clocks = <&gate_clk 13>;
> > + clock-names = "internal";
> > +};
>
> Also we will need some phandle reference to the audio codec here. As
> this property is ongoing work in ASoC core, I suggest we wait for it
> and propose a binding afterwards.

I don't think that we need any reference to the codec here. The glue is
done by the audio device. For example, using the (soon?) extended
simple audio card:

spdif: spdif {
compatible = "linux,spdif-dit";
};
sound {
compatible = "linux,simple-audio";
audio-controller = <&i2s1>;
audio-codec = <&spdif>;
codec-dai-name = "dit-hifi";
};

--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/