mbox series

[0/4] Add support for IIO devices in ASoC

Message ID 20230421124122.324820-1-herve.codina@bootlin.com
Headers show
Series Add support for IIO devices in ASoC | expand

Message

Herve Codina April 21, 2023, 12:41 p.m. UTC
Several weeks ago, I sent a series [1] for adding a potentiometer as an
auxiliary device in ASoC. The feedback was that the potentiometer should
be directly handled in IIO (as other potentiometers) and something more
generic should be present in ASoC in order to have a binding to import
some IIO devices into sound cards.

The series related to the IIO potentiometer device is already under
review [2].

This series introduces simple-iio-aux. Its goal is to offer the binding
between IIO and ASoC.
It exposes attached IIO devices as ASoC auxiliary devices and allows to
control them through mixer controls.

On my system, the IIO device is a potentiometer and it is present in an
amplifier design present in the audio path.

Best regards,
Hervé

[1] https://lore.kernel.org/linux-kernel/20230203111422.142479-1-herve.codina@bootlin.com/
[2] https://lore.kernel.org/linux-kernel/20230421085245.302169-1-herve.codina@bootlin.com/

Herve Codina (4):
  dt-bindings: sound: Add simple-iio-aux
  iio: inkern: Add a helper to query an available minimum raw value
  ASoC: soc-dapm.h: Add a helper to build a DAPM widget dynamically
  ASoC: codecs: Add support for the generic IIO auxiliary devices

 .../bindings/sound/simple-iio-aux.yaml        |  65 ++++
 drivers/iio/inkern.c                          |  67 ++++
 include/linux/iio/consumer.h                  |  11 +
 include/sound/soc-dapm.h                      |  12 +-
 sound/soc/codecs/Kconfig                      |  12 +
 sound/soc/codecs/Makefile                     |   2 +
 sound/soc/codecs/simple-iio-aux.c             | 307 ++++++++++++++++++
 7 files changed, 475 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/sound/simple-iio-aux.yaml
 create mode 100644 sound/soc/codecs/simple-iio-aux.c

Comments

Mark Brown April 25, 2023, 5:33 p.m. UTC | #1
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...
Herve Codina April 26, 2023, 7:36 a.m. UTC | #2
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é