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

  Industrial I/O devices can be present in the audio path.
These devices needs to be viewed as audio components in order to be
fully integrated in the audio path.

simple-iio-aux allows to consider these Industrial I/O devices as
auxliary audio devices.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 .../bindings/sound/simple-iio-aux.yaml        | 65 +++++++++++++++++++
 1 file changed, 65 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-iio-aux.yaml
  

On Fri, Apr 21, 2023 at 02:41:19PM +0200, Herve Codina wrote:
> Industrial I/O devices can be present in the audio path.
> These devices needs to be viewed as audio components in order to be
> fully integrated in the audio path.
> 
> 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. :)

Examples of devices would be useful here.

> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---
>  .../bindings/sound/simple-iio-aux.yaml        | 65 +++++++++++++++++++
>  1 file changed, 65 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-iio-aux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-iio-aux.yaml b/Documentation/devicetree/bindings/sound/simple-iio-aux.yaml
> new file mode 100644
> index 000000000000..fab128fce4fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-iio-aux.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/simple-iio-aux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Simple IIO auxiliary
> +
> +maintainers:
> +  - Herve Codina <herve.codina@bootlin.com>
> +
> +description: |

Don't need '|'

> +  Auxiliary device based on Industrial I/O device channels
> +
> +allOf:
> +  - $ref: /schemas/iio/iio-consumer.yaml

You don't need to reference consumer schemas.

> +  - $ref: dai-common.yaml#
> +
> +properties:
> +  compatible:
> +    const: simple-iio-aux
> +
> +  io-channels:
> +    description:
> +      Industrial I/O device channels used
> +
> +  io-channel-names:
> +    description:
> +      Industrial I/O channel names related to io-channels.
> +      These names are used to provides sound controls, widgets and routes names.
> +
> +  invert:

Property names should globally only have 1 type definition. This is 
generic enough I'd be concerned that's not the case.

> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: |
> +      A list of 0/1 flags defining whether or not the related channel is
> +      inverted
> +    items:
> +      enum: [0, 1]
> +      default: 0
> +      description: |
> +        Invert the sound control value compared to the IIO channel raw value.
> +          - 1: The related sound control value is inverted meaning that the
> +               minimum sound control value correspond to the maximum IIO channel
> +               raw value and the maximum sound control value correspond to the
> +               minimum IIO channel raw value.
> +          - 0: The related sound control value is not inverted meaning that the
> +               minimum (resp maximum) sound control value correspond to the
> +               minimum (resp maximum) IIO channel raw value.
> +
> +required:
> +  - compatible
> +  - io-channels
> +  - io-channel-names
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    aux {
> +        compatible = "simple-iio-aux";
> +        io-channels = <&iio 0>, <&iio 1>, <&iio 2>, <&iio 3>;
> +        io-channel-names = "CH0", "CH1", "CH2", "CH3";

Not really useful names. Do you have a real example?

> +        /* Invert CH1 and CH2 */
> +        invert = <0 1 1>;

IMO, invert should be same length as io-channels.

> +    };

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.

Rob
  

On Tue, Apr 25, 2023 at 12:30:29PM -0500, Rob Herring wrote:
> On Fri, Apr 21, 2023 at 02:41:19PM +0200, Herve Codina wrote:

> > +    aux {
> > +        compatible = "simple-iio-aux";
> > +        io-channels = <&iio 0>, <&iio 1>, <&iio 2>, <&iio 3>;
> > +        io-channel-names = "CH0", "CH1", "CH2", "CH3";

> Not really useful names. Do you have a real example?

I fear those might be real names for channels on an IIO device...
  

Hi Rob,

On Tue, 25 Apr 2023 12:30:29 -0500
Rob Herring <robh@kernel.org> wrote:

> On Fri, Apr 21, 2023 at 02:41:19PM +0200, Herve Codina wrote:
> > Industrial I/O devices can be present in the audio path.
> > These devices needs to be viewed as audio components in order to be
> > fully integrated in the audio path.
> > 
> > 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.

> 
> Examples of devices would be useful here.
> 
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---
> >  .../bindings/sound/simple-iio-aux.yaml        | 65 +++++++++++++++++++
> >  1 file changed, 65 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/sound/simple-iio-aux.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/sound/simple-iio-aux.yaml b/Documentation/devicetree/bindings/sound/simple-iio-aux.yaml
> > new file mode 100644
> > index 000000000000..fab128fce4fc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/sound/simple-iio-aux.yaml
> > @@ -0,0 +1,65 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/sound/simple-iio-aux.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Simple IIO auxiliary
> > +
> > +maintainers:
> > +  - Herve Codina <herve.codina@bootlin.com>
> > +
> > +description: |  
> 
> Don't need '|'

Will be fixed.

> 
> > +  Auxiliary device based on Industrial I/O device channels
> > +
> > +allOf:
> > +  - $ref: /schemas/iio/iio-consumer.yaml  
> 
> You don't need to reference consumer schemas.

Right, will be removed.

> 
> > +  - $ref: dai-common.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    const: simple-iio-aux
> > +
> > +  io-channels:
> > +    description:
> > +      Industrial I/O device channels used
> > +
> > +  io-channel-names:
> > +    description:
> > +      Industrial I/O channel names related to io-channels.
> > +      These names are used to provides sound controls, widgets and routes names.
> > +
> > +  invert:  
> 
> Property names should globally only have 1 type definition. This is 
> generic enough I'd be concerned that's not the case.

What do you mean ?

> 
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    description: |
> > +      A list of 0/1 flags defining whether or not the related channel is
> > +      inverted
> > +    items:
> > +      enum: [0, 1]
> > +      default: 0
> > +      description: |
> > +        Invert the sound control value compared to the IIO channel raw value.
> > +          - 1: The related sound control value is inverted meaning that the
> > +               minimum sound control value correspond to the maximum IIO channel
> > +               raw value and the maximum sound control value correspond to the
> > +               minimum IIO channel raw value.
> > +          - 0: The related sound control value is not inverted meaning that the
> > +               minimum (resp maximum) sound control value correspond to the
> > +               minimum (resp maximum) IIO channel raw value.
> > +
> > +required:
> > +  - compatible
> > +  - io-channels
> > +  - io-channel-names
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    aux {
> > +        compatible = "simple-iio-aux";
> > +        io-channels = <&iio 0>, <&iio 1>, <&iio 2>, <&iio 3>;
> > +        io-channel-names = "CH0", "CH1", "CH2", "CH3";  
> 
> Not really useful names. Do you have a real example?

As Mark said, for IIO channel, using CHx makes sense.
See below, I provide a full example.

> 
> > +        /* Invert CH1 and CH2 */
> > +        invert = <0 1 1>;  
> 
> IMO, invert should be same length as io-channels.

I will update.

Related to this topic, when I wrote this binding, I wanted to add some
rules/constraints in order to:
- Have this property optional
- If present, force to have as many items in the invert array as the
  number of items present in the io-channels array.

I never succeed in writing the constraint for the invert property.
It should be possible (it is done for some 'foo' 'foo-names' pair such
as clocks).
Can you tell me if possible in my case and give me some pointers ?

> 
> > +    };  
> 
> 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:

    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";
        };

    };

    amp_out: aux-out {
        compatible = "simple-iio-aux";
        io-channels = <&pot_out 0>, <&pot_out 1>,
        io-channel-names = "CH0", "CH1";
        invert = <1 1>;
        sound-name-prefix = "AMP_OUT";
    };

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

    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 {
            ...
        };
    };


Best regards,
Hervé
  

On 26/04/2023 09:36, Herve Codina wrote:
> Hi Rob,
> 
> On Tue, 25 Apr 2023 12:30:29 -0500
> Rob Herring <robh@kernel.org> wrote:
> 
>> On Fri, Apr 21, 2023 at 02:41:19PM +0200, Herve Codina wrote:
>>> Industrial I/O devices can be present in the audio path.
>>> These devices needs to be viewed as audio components in order to be
>>> fully integrated in the audio path.
>>>
>>> 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.

...

>>> +  io-channels:
>>> +    description:
>>> +      Industrial I/O device channels used
>>> +
>>> +  io-channel-names:
>>> +    description:
>>> +      Industrial I/O channel names related to io-channels.
>>> +      These names are used to provides sound controls, widgets and routes names.
>>> +
>>> +  invert:  
>>
>> Property names should globally only have 1 type definition. This is 
>> generic enough I'd be concerned that's not the case.
> 
> What do you mean ?

It is quite likely this will interfere with other properties having the
same name but different type/definition. If you want to keep it named
generic, then please investigate how this would affect any other
bindings. So easier is to make it not that generic, some more specific name.

> 
>>
>>> +    $ref: /schemas/types.yaml#/definitions/uint32-array
>>> +    description: |
>>> +      A list of 0/1 flags defining whether or not the related channel is
>>> +      inverted
>>> +    items:
>>> +      enum: [0, 1]
>>> +      default: 0
>>> +      description: |
>>> +        Invert the sound control value compared to the IIO channel raw value.
>>> +          - 1: The related sound control value is inverted meaning that the
>>> +               minimum sound control value correspond to the maximum IIO channel
>>> +               raw value and the maximum sound control value correspond to the
>>> +               minimum IIO channel raw value.
>>> +          - 0: The related sound control value is not inverted meaning that the
>>> +               minimum (resp maximum) sound control value correspond to the
>>> +               minimum (resp maximum) IIO channel raw value.
>>> +
>>> +required:
>>> +  - compatible
>>> +  - io-channels
>>> +  - io-channel-names
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> +  - |
>>> +    aux {
>>> +        compatible = "simple-iio-aux";
>>> +        io-channels = <&iio 0>, <&iio 1>, <&iio 2>, <&iio 3>;
>>> +        io-channel-names = "CH0", "CH1", "CH2", "CH3";  
>>
>> Not really useful names. Do you have a real example?
> 
> As Mark said, for IIO channel, using CHx makes sense.
> See below, I provide a full example.
> 
>>
>>> +        /* Invert CH1 and CH2 */
>>> +        invert = <0 1 1>;  
>>
>> IMO, invert should be same length as io-channels.
> 
> I will update.
> 
> Related to this topic, when I wrote this binding, I wanted to add some
> rules/constraints in order to:
> - Have this property optional
> - If present, force to have as many items in the invert array as the
>   number of items present in the io-channels array.
> 
> I never succeed in writing the constraint for the invert property.
> It should be possible (it is done for some 'foo' 'foo-names' pair such
> as clocks).
> Can you tell me if possible in my case and give me some pointers ?
> 
>>
>>> +    };  
>>
>> 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?

Best regards,
Krzysztof
  

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.
  

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é