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

[Herve Codina <herve.codina@bootlin.com>]

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é