Re: [PATCH 1/4] dt-bindings: sound: Add simple-iio-aux

Herve Codina
2023-05-11T07:21:17Z

Hi Rob, Krzysztof, Mark,

On Thu, 4 May 2023 13:22:35 +0900
Mark Brown <broonie@kernel.org> wrote:

> On Tue, May 02, 2023 at 09:26:32AM +0200, Krzysztof Kozlowski wrote:
> > On 26/04/2023 09:36, Herve Codina wrote:
> > > Rob Herring <robh@kernel.org> wrote:
> > >> On Fri, Apr 21, 2023 at 02:41:19PM +0200, Herve Codina wrote:
>
> > >>> simple-iio-aux allows to consider these Industrial I/O devices as
> > >>> auxliary audio devices.
>
> > >> What makes it simple? Any binding called simple or generic is a trigger
> > >> for me. Best to avoid those terms. :)
>
> > > I choose simple-iio-aux because some simple-* already exists.
> > > For instance simple-audio-amplifier or simple-audio-mux.
>
> > > Do you prefer audio-iio-aux ?
> > > Let me know if I should change.
>
> > It means that often what people call "simple" and "generic" works only
> > for their specific case, because it is not really simple and generic.
> > After some time the "simple" and "generic" becomes "complicated" and
> > "huge". Conclusion: sometimes simple and generic bindings are bad idea
> > and you should have something specific.
>
> > Your description in the binding also does not help to match it to
> > specific, real device. Provide the examples, as Rob asked.
>
> I don't understand what you are looking for here. IIO is a subsystem
> which represents generic DACs and ADCs (along with other I/O things).
> Audio devices also have DACs and ADCs, somewhat specialised for use in
> audio but more limited by specs and interfaces than by anything
> fundamental. The goal here is to map DACs and ADCs described as IIO for
> use in an audio context.
>
> ADCs are devices that convert analog signals into digital values, DACs
> are devices that convert digital values into analog signals.
>
> > >> How do support multiple instances? Say you have 2 sound cards (or 1
> > >> sound card with multiple audio paths) each with different sets of IIO
> > >> channels associated with it. You'd need a link to each 'aux' node. Why
> > >> not just add io-channels to the sound card nodes directly? That's
> > >> already just a virtual, top-level container node grouping all the
> > >> components. I don't see why we need another virtual node grouping a
> > >> subset of them.
>
> > > I don't see what you mean.
> > > I use a simple-audio-card and here is a full example using several
> > > instances:
>
> > Just like Rob said: "You'd need a link to each 'aux' node"
>
> > and you did it...
>
> > So now the rest of Rob's answer:
>
> > "Why not just add io-channels to the sound card nodes directly? That's
> > already just a virtual, top-level container node grouping all the
> > components. I don't see why we need another virtual node grouping a
> > subset of them."
>
> > Why do you need another node if it is not really representing a real,
> > separate device?
>
> If nothing else I would expect it to be useful from a comprehensibility
> point of view to bundle multiple IIO devices into a single multi-channel
> audio stream, an individual IIO device is likely to only present a
> single channel of data but it is common to group multiple channels of
> audio data.

I cannot simply add io-channels to the sound card directly. I need a node
to set at least the sound-name-prefix property. Further more having a node
and a related compatible string can be easier to maintain and add future
evolution related to these "virtual" devices.

As some subnodes are already defined for a sound card node, I propose to
group these "virtual" audio devices node in a specific bundle node.
This lead to the following example:
---- 8< ----
spi {
#address-cells = <1>;
#size-cells = <0>;
/* potentiometers present in an input amplifier design */
pot_in: potentiometer@0 {
compatible = "foo,xxx";
reg = <0>;
#io-channel-cells = <1>;
};
/* potentiometers present in an output amplifier design */
pot_out: potentiometer@1 {
compatible = "foo,xxx";
reg = <1>;
#io-channel-cells = <1>;
};
/* A codec */
codec: codec@2 {
compatible = "bar,yyy";
reg = <2>;
sound-name-prefix = "CODEC";
};

};

sound {
compatible = "simple-audio-card";
#address-cells = <1>;
#size-cells = <0>;
simple-audio-card,name = "My Sound Card";

simple-audio-card,aux-devs = <&amp_in>, <&amp_out>;
simple-audio-card,routing =
"CODEC IN0", "AMP_IN CH0 OUT",
"CODEC IN1", "AMP_IN CH1 OUT",
"AMP_OUT CH0 IN", "CODEC OUT0",
"AMP_OUT CH1 IN", "CODEC OUT1";

simple-audio-card,dai-link@0 {
...
};

...

/* A bundle for the additional devices */
simple-audio-card,additional-devs {
amp_out: aux-out {
compatible = "audio-iio-aux"; /* Instead of "simple-iio-aux */
io-channels = <&pot_out 0>, <&pot_out 1>,
io-channel-names = "CH0", "CH1";
snd-control-invert-range = <1 1>; /* Old 'invert' renamed */
sound-name-prefix = "AMP_OUT";
};

amp_in: aux-in {
compatible = "audio-iio-aux";
io-channels = <&pot_in 0>, <&pot_in 1>;
io-channel-names = "CH0", "CH1";
sound-name-prefix = "AMP_IN";
};
};
};
---- 8< ----

What do you think about this new binding ?

Best regards,
Hervé