[PATCH v2 0/9] Add support for IIO devices in ASoC

[PATCH v2 0/9] Add support for IIO devices in ASoC

[Herve Codina]

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 audio-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.

Compare to the previous iteration
  https://lore.kernel.org/linux-kernel/20230421124122.324820-1-herve.codina@bootlin.com/
This v2 series mainly:
 - updates the binding using a simple-card subnode and handles this new
   subnode in the simple-card driver.
 - Improves existing IIO code and documentation.
 - Renames simple-iio-aux to audio-iio-aux and fixes the driver itself.

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/

Changes v1 -> v2
  - Patch 1
    Rename simple-iio-aux to audio-iio-aux
    Rename invert to snd-control-invert-range
    Remove the /schemas/iio/iio-consumer.yaml reference
    Remove the unneeded '|' after description

  - Patch 2 (new in v2)
    Introduce the simple-audio-card additional-devs subnode

  - Patch 3 (new in v2)
    Check err before switch() in iio_channel_read_max()

  - Patch 4 (new in v2)
    Fix raw reads and raw writes documentation

  - Patch 5 (patch 2 in v1)
    Check err before switch() in iio_channel_read_min()
    Fix documentation

  - Patch 6 (path 3 in v1)
    No changes

  - Patch 7 (patch 4 in v1)
    Rename simple-iio-aux to audio-iio-aux
    Rename invert to snd-control-invert-range
    Remove the mask usage from audio_iio_aux_{get,put}_volsw helpers
    Use directly PTR_ERR() in dev_err_probe() parameter
    Remove the '!!' construction
    Remove of_match_ptr()

  - Patch 8 (new in v2)
    Add a missing of_node_put() in the simple-card driver

  - Patch 9 (new in v2)
    Handle additional-devs in the simple-card driver

Herve Codina (9):
  ASoC: dt-bindings: Add audio-iio-aux
  ASoC: dt-bindings: simple-card: Add additional-devs subnode
  iio: inkern: Check error explicitly in iio_channel_read_max()
  iio: consumer.h: Fix raw values documentation notes
  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
  ASoC: simple-card: Add missing of_node_put() in case of error
  ASoC: simple-card: Handle additional devices

 .../bindings/sound/audio-iio-aux.yaml         |  64 ++++
 .../bindings/sound/simple-card.yaml           |  53 +++
 drivers/iio/inkern.c                          |  75 ++++-
 include/linux/iio/consumer.h                  |  37 ++-
 include/sound/soc-dapm.h                      |  12 +-
 sound/soc/codecs/Kconfig                      |  12 +
 sound/soc/codecs/Makefile                     |   2 +
 sound/soc/codecs/audio-iio-aux.c              | 302 ++++++++++++++++++
 sound/soc/generic/simple-card.c               |  53 ++-
 9 files changed, 596 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/audio-iio-aux.yaml
 create mode 100644 sound/soc/codecs/audio-iio-aux.c

-- 
2.40.1

[PATCH v2 1/9] ASoC: dt-bindings: Add audio-iio-aux

[Herve Codina]

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.

audio-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/audio-iio-aux.yaml         | 64 +++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/sound/audio-iio-aux.yaml

diff --git a/Documentation/devicetree/bindings/sound/audio-iio-aux.yaml b/Documentation/devicetree/bindings/sound/audio-iio-aux.yaml
new file mode 100644
index 000000000000..d3cc1ea4a175
--- /dev/null
+++ b/Documentation/devicetree/bindings/sound/audio-iio-aux.yaml
@@ -0,0 +1,64 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/sound/audio-iio-aux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Audio IIO auxiliary
+
+maintainers:
+  - Herve Codina <herve.codina@bootlin.com>
+
+description:
+  Auxiliary device based on Industrial I/O device channels
+
+allOf:
+  - $ref: dai-common.yaml#
+
+properties:
+  compatible:
+    const: audio-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.
+
+  snd-control-invert-range:
+    $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:
+  - |
+    iio-aux {
+        compatible = "audio-iio-aux";
+        io-channels = <&iio 0>, <&iio 1>, <&iio 2>, <&iio 3>;
+        io-channel-names = "CH0", "CH1", "CH2", "CH3";
+        /* Invert CH1 and CH2 */
+        snd-control-invert-range = <0 1 1 0>;
+    };
-- 
2.40.1

[PATCH v2 2/9] ASoC: dt-bindings: simple-card: Add additional-devs subnode

[Herve Codina]

The additional-devs subnode allows to declared some virtual devices
as sound card children.
These virtual devices can then be used by the sound card and so be
present in the audio path.

The first virtual device supported is the audio IIO auxiliary device
in order to support an IIO device as an audio auxiliary device.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 .../bindings/sound/simple-card.yaml           | 53 +++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.yaml b/Documentation/devicetree/bindings/sound/simple-card.yaml
index b05e05c81cc4..59ac2d1d1ccf 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.yaml
+++ b/Documentation/devicetree/bindings/sound/simple-card.yaml
@@ -148,6 +148,15 @@ definitions:
     required:
       - sound-dai
 
+  additional-devs:
+    type: object
+    description:
+      Additional devices used by the simple audio card.
+    patternProperties:
+      '^iio-aux(-.+)?$':
+        type: object
+        $ref: audio-iio-aux.yaml#
+
 properties:
   compatible:
     contains:
@@ -187,6 +196,8 @@ properties:
     $ref: "#/definitions/mclk-fs"
   simple-audio-card,aux-devs:
     $ref: "#/definitions/aux-devs"
+  simple-audio-card,additional-devs:
+    $ref: "#/definitions/additional-devs"
   simple-audio-card,convert-rate:
     $ref: "#/definitions/convert-rate"
   simple-audio-card,convert-channels:
@@ -359,6 +370,48 @@ examples:
         };
     };
 
+# --------------------
+# route audio to/from a codec through an amplifier
+# designed with a potentiometer driven by IIO:
+# --------------------
+  - |
+    sound {
+        compatible = "simple-audio-card";
+
+        simple-audio-card,aux-devs = <&amp_in>, <&amp_out>;
+        simple-audio-card,routing =
+            "CODEC LEFTIN", "AMP_IN LEFT OUT",
+            "CODEC RIGHTIN", "AMP_IN RIGHT OUT",
+            "AMP_OUT LEFT IN", "CODEC LEFTOUT",
+            "AMP_OUT RIGHT IN", "CODEC RIGHTOUT";
+
+        simple-audio-card,additional-devs {
+            amp_out: iio-aux-out {
+                compatible = "audio-iio-aux";
+                io-channels = <&pot_out 0>, <&pot_out 1>;
+                io-channel-names = "LEFT", "RIGHT";
+                snd-control-invert-range = <1 1>;
+                sound-name-prefix = "AMP_OUT";
+            };
+
+            amp_in: iio_aux-in {
+                compatible = "audio-iio-aux";
+                io-channels = <&pot_in 0>, <&pot_in 1>;
+                io-channel-names = "LEFT", "RIGHT";
+                sound-name-prefix = "AMP_IN";
+            };
+        };
+
+        simple-audio-card,cpu {
+            sound-dai = <&cpu>;
+        };
+
+        simple-audio-card,codec {
+            sound-dai = <&codec>;
+            clocks = <&clocks>;
+        };
+    };
+
 # --------------------
 # Sampling Rate Conversion
 # --------------------
-- 
2.40.1

[PATCH v2 3/9] iio: inkern: Check error explicitly in iio_channel_read_max()

[Herve Codina]

The current implementation returns the error code as part of the
default switch case.
This can lead to returning an incorrect positive value in case of
iio_avail_type enum entries evolution.

In order to avoid this case, be more strict in error checking.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/iio/inkern.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index 872fd5c24147..f738db9a0c04 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -858,6 +858,9 @@ static int iio_channel_read_max(struct iio_channel *chan,
 		val2 = &unused;
 
 	ret = iio_channel_read_avail(chan, &vals, type, &length, info);
+	if (ret < 0)
+		return ret;
+
 	switch (ret) {
 	case IIO_AVAIL_RANGE:
 		switch (*type) {
@@ -888,7 +891,7 @@ static int iio_channel_read_max(struct iio_channel *chan,
 		return 0;
 
 	default:
-		return ret;
+		return -EINVAL;
 	}
 }
 
-- 
2.40.1

[PATCH v2 4/9] iio: consumer.h: Fix raw values documentation notes

[Herve Codina]

The raw values notes mention 'ADC counts' and are not fully accurate.

Reword the notes in order to remove the 'ADC counts' and describe the
conversion needed between a raw value and a value in the standard units.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 include/linux/iio/consumer.h | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index 6802596b017c..f536820b9cf2 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -201,8 +201,9 @@ struct iio_dev
  * @chan:		The channel being queried.
  * @val:		Value read back.
  *
- * Note raw reads from iio channels are in adc counts and hence
- * scale will need to be applied if standard units required.
+ * Note, if standard units are required, raw reads from iio channels
+ * need the offset (default 0) and scale (default 1) to be applied
+ * as (raw + offset) * scale.
  */
 int iio_read_channel_raw(struct iio_channel *chan,
 			 int *val);
@@ -212,8 +213,9 @@ int iio_read_channel_raw(struct iio_channel *chan,
  * @chan:		The channel being queried.
  * @val:		Value read back.
  *
- * Note raw reads from iio channels are in adc counts and hence
- * scale will need to be applied if standard units required.
+ * Note, if standard units are required, raw reads from iio channels
+ * need the offset (default 0) and scale (default 1) to be applied
+ * as (raw + offset) * scale.
  *
  * In opposit to the normal iio_read_channel_raw this function
  * returns the average of multiple reads.
@@ -281,8 +283,9 @@ int iio_read_channel_attribute(struct iio_channel *chan, int *val,
  * @chan:		The channel being queried.
  * @val:		Value being written.
  *
- * Note raw writes to iio channels are in dac counts and hence
- * scale will need to be applied if standard units required.
+ * Note that for raw writes to iio channels, if the value provided is
+ * in standard units, the affect of the scale and offset must be removed
+ * as (value / scale) - offset.
  */
 int iio_write_channel_raw(struct iio_channel *chan, int val);
 
@@ -292,8 +295,9 @@ int iio_write_channel_raw(struct iio_channel *chan, int val);
  * @chan:		The channel being queried.
  * @val:		Value read back.
  *
- * Note raw reads from iio channels are in adc counts and hence
- * scale will need to be applied if standard units are required.
+ * Note, if standard units are required, raw reads from iio channels
+ * need the offset (default 0) and scale (default 1) to be applied
+ * as (raw + offset) * scale.
  */
 int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
 
@@ -308,8 +312,9 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
  * For ranges, three vals are always returned; min, step and max.
  * For lists, all the possible values are enumerated.
  *
- * Note raw available values from iio channels are in adc counts and
- * hence scale will need to be applied if standard units are required.
+ * Note, if standard units are required, raw available values from iio
+ * channels need the offset (default 0) and scale (default 1) to be applied
+ * as (raw + offset) * scale.
  */
 int iio_read_avail_channel_raw(struct iio_channel *chan,
 			       const int **vals, int *length);
-- 
2.40.1

[PATCH v2 5/9] iio: inkern: Add a helper to query an available minimum raw value

[Herve Codina]

A helper, iio_read_max_channel_raw() exists to read the available
maximum raw value of a channel but nothing similar exists to read the
available minimum raw value.

This new helper, iio_read_min_channel_raw(), fills the hole and can be
used for reading the available minimum raw value of a channel.
It is fully based on the existing iio_read_max_channel_raw().

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 drivers/iio/inkern.c         | 70 ++++++++++++++++++++++++++++++++++++
 include/linux/iio/consumer.h | 12 +++++++
 2 files changed, 82 insertions(+)

diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
index f738db9a0c04..07b26ff8a334 100644
--- a/drivers/iio/inkern.c
+++ b/drivers/iio/inkern.c
@@ -915,6 +915,76 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val)
 }
 EXPORT_SYMBOL_GPL(iio_read_max_channel_raw);
 
+static int iio_channel_read_min(struct iio_channel *chan,
+				int *val, int *val2, int *type,
+				enum iio_chan_info_enum info)
+{
+	int unused;
+	const int *vals;
+	int length;
+	int ret;
+
+	if (!val2)
+		val2 = &unused;
+
+	ret = iio_channel_read_avail(chan, &vals, type, &length, info);
+	if (ret < 0)
+		return ret;
+
+	switch (ret) {
+	case IIO_AVAIL_RANGE:
+		switch (*type) {
+		case IIO_VAL_INT:
+			*val = vals[0];
+			break;
+		default:
+			*val = vals[0];
+			*val2 = vals[1];
+		}
+		return 0;
+
+	case IIO_AVAIL_LIST:
+		if (length <= 0)
+			return -EINVAL;
+		switch (*type) {
+		case IIO_VAL_INT:
+			*val = vals[--length];
+			while (length) {
+				if (vals[--length] < *val)
+					*val = vals[length];
+			}
+			break;
+		default:
+			/* FIXME: learn about min for other iio values */
+			return -EINVAL;
+		}
+		return 0;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+int iio_read_min_channel_raw(struct iio_channel *chan, int *val)
+{
+	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
+	int ret;
+	int type;
+
+	mutex_lock(&iio_dev_opaque->info_exist_lock);
+	if (!chan->indio_dev->info) {
+		ret = -ENODEV;
+		goto err_unlock;
+	}
+
+	ret = iio_channel_read_min(chan, val, NULL, &type, IIO_CHAN_INFO_RAW);
+err_unlock:
+	mutex_unlock(&iio_dev_opaque->info_exist_lock);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(iio_read_min_channel_raw);
+
 int iio_get_channel_type(struct iio_channel *chan, enum iio_chan_type *type)
 {
 	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
index f536820b9cf2..e9910b41d48e 100644
--- a/include/linux/iio/consumer.h
+++ b/include/linux/iio/consumer.h
@@ -301,6 +301,18 @@ int iio_write_channel_raw(struct iio_channel *chan, int val);
  */
 int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
 
+/**
+ * iio_read_min_channel_raw() - read minimum available raw value from a given
+ *				channel, i.e. the minimum possible value.
+ * @chan:		The channel being queried.
+ * @val:		Value read back.
+ *
+ * Note, if standard units are required, raw reads from iio channels
+ * need the offset (default 0) and scale (default 1) to be applied
+ * as (raw + offset) * scale.
+ */
+int iio_read_min_channel_raw(struct iio_channel *chan, int *val);
+
 /**
  * iio_read_avail_channel_raw() - read available raw values from a given channel
  * @chan:		The channel being queried.
-- 
2.40.1

[PATCH v2 6/9] ASoC: soc-dapm.h: Add a helper to build a DAPM widget dynamically

[Herve Codina]

The SND_SOC_DAPM_* helpers family are used to build widgets array in a
static way.

Introduce SND_SOC_DAPM_WIDGET() in order to use the SND_SOC_DAPM_*
helpers family in a dynamic way. The different SND_SOC_DAPM_* parameters
can be computed by the code and the widget can be built based on this
parameter computation.
For instance:
  static int create_widget(char *input_name)
  {
	struct snd_soc_dapm_widget widget;
	char name*;
	...
	name = input_name;
	if (!name)
		name = "default";

	widget = SND_SOC_DAPM_WIDGET(SND_SOC_DAPM_INPUT(name));
	...
  }

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 include/sound/soc-dapm.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/sound/soc-dapm.h b/include/sound/soc-dapm.h
index 87f8e1793af1..6b62fe5c779c 100644
--- a/include/sound/soc-dapm.h
+++ b/include/sound/soc-dapm.h
@@ -276,7 +276,17 @@ struct soc_enum;
 	.reg = SND_SOC_NOPM, .event = dapm_pinctrl_event, \
 	.event_flags = SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_POST_PMD }
 
-
+/*
+ * Helper to create a widget 'dynamically'
+ * This can be used with any of the SND_SOC_DAPM_* widget helper.
+ * For instance:
+ *  struct snd_soc_dapm_widget widget;
+ *  ...
+ *  widget = SND_SOC_DAPM_WIDGET(SND_SOC_DAPM_INPUT(input_name));
+ */
+#define SND_SOC_DAPM_WIDGET(_widget)({ \
+		struct snd_soc_dapm_widget _w[1] = { _widget }; \
+	_w[0]; })
 
 /* dapm kcontrol types */
 #define SOC_DAPM_DOUBLE(xname, reg, lshift, rshift, max, invert) \
-- 
2.40.1

[PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Herve Codina]

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

This support allows to consider these Industrial I/O devices as auxliary
audio devices and allows to control them using mixer controls.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 sound/soc/codecs/Kconfig         |  12 ++
 sound/soc/codecs/Makefile        |   2 +
 sound/soc/codecs/audio-iio-aux.c | 302 +++++++++++++++++++++++++++++++
 3 files changed, 316 insertions(+)
 create mode 100644 sound/soc/codecs/audio-iio-aux.c

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 44806bfe8ee5..92b7c417f1b2 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -53,6 +53,7 @@ config SND_SOC_ALL_CODECS
 	imply SND_SOC_AK5558
 	imply SND_SOC_ALC5623
 	imply SND_SOC_ALC5632
+	imply SND_SOC_AUDIO_IIO_AUX
 	imply SND_SOC_AW8738
 	imply SND_SOC_AW88395
 	imply SND_SOC_BT_SCO
@@ -608,6 +609,17 @@ config SND_SOC_ALC5632
 	tristate
 	depends on I2C
 
+config SND_SOC_AUDIO_IIO_AUX
+	tristate "Audio IIO Auxiliary device"
+	depends on IIO
+	help
+	  Enable support for Industrial I/O devices as audio auxiliary devices.
+	  This allows to have an IIO device present in the audio path and
+	  controlled using mixer controls.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called snd-soc-audio-iio-aux.
+
 config SND_SOC_AW8738
 	tristate "Awinic AW8738 Audio Amplifier"
 	select GPIOLIB
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index 2c45c2f97e4e..f2828d3616c5 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -45,6 +45,7 @@ snd-soc-ak4671-objs := ak4671.o
 snd-soc-ak5386-objs := ak5386.o
 snd-soc-ak5558-objs := ak5558.o
 snd-soc-arizona-objs := arizona.o arizona-jack.o
+snd-soc-audio-iio-aux-objs := audio-iio-aux.o
 snd-soc-aw8738-objs := aw8738.o
 snd-soc-aw88395-lib-objs := aw88395/aw88395_lib.o
 snd-soc-aw88395-objs := aw88395/aw88395.o \
@@ -421,6 +422,7 @@ obj-$(CONFIG_SND_SOC_AK5558)	+= snd-soc-ak5558.o
 obj-$(CONFIG_SND_SOC_ALC5623)    += snd-soc-alc5623.o
 obj-$(CONFIG_SND_SOC_ALC5632)	+= snd-soc-alc5632.o
 obj-$(CONFIG_SND_SOC_ARIZONA)	+= snd-soc-arizona.o
+obj-$(CONFIG_SND_SOC_AUDIO_IIO_AUX)	+= snd-soc-audio-iio-aux.o
 obj-$(CONFIG_SND_SOC_AW8738)	+= snd-soc-aw8738.o
 obj-$(CONFIG_SND_SOC_AW88395_LIB) += snd-soc-aw88395-lib.o
 obj-$(CONFIG_SND_SOC_AW88395)	+=snd-soc-aw88395.o
diff --git a/sound/soc/codecs/audio-iio-aux.c b/sound/soc/codecs/audio-iio-aux.c
new file mode 100644
index 000000000000..21575c4b35fd
--- /dev/null
+++ b/sound/soc/codecs/audio-iio-aux.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// audio-iio-aux.c  --  ALSA SoC glue to use IIO devices as audio components
+//
+// Copyright 2023 CS GROUP France
+//
+// Author: Herve Codina <herve.codina@bootlin.com>
+
+#include <linux/iio/consumer.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <sound/soc.h>
+#include <sound/tlv.h>
+
+struct audio_iio_aux_chan {
+	struct iio_channel *iio_chan;
+	const char *name;
+	bool is_invert_range;
+	int max;
+	int min;
+};
+
+struct audio_iio_aux {
+	struct device *dev;
+	struct audio_iio_aux_chan *chans;
+	unsigned int num_chans;
+};
+
+static int audio_iio_aux_info_volsw(struct snd_kcontrol *kcontrol,
+				    struct snd_ctl_elem_info *uinfo)
+{
+	struct audio_iio_aux_chan *chan = (struct audio_iio_aux_chan *)kcontrol->private_value;
+
+	uinfo->count = 1;
+	uinfo->value.integer.min = 0;
+	uinfo->value.integer.max = chan->max - chan->min;
+	uinfo->type = (uinfo->value.integer.max == 1) ?
+			SNDRV_CTL_ELEM_TYPE_BOOLEAN : SNDRV_CTL_ELEM_TYPE_INTEGER;
+	return 0;
+}
+
+static int audio_iio_aux_get_volsw(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct audio_iio_aux_chan *chan = (struct audio_iio_aux_chan *)kcontrol->private_value;
+	int max = chan->max;
+	int min = chan->min;
+	bool invert_range = chan->is_invert_range;
+	int ret;
+	int val;
+
+	ret = iio_read_channel_raw(chan->iio_chan, &val);
+	if (ret < 0)
+		return ret;
+
+	ucontrol->value.integer.value[0] = val - min;
+	if (invert_range)
+		ucontrol->value.integer.value[0] = max - ucontrol->value.integer.value[0];
+
+	return 0;
+}
+
+static int audio_iio_aux_put_volsw(struct snd_kcontrol *kcontrol,
+				   struct snd_ctl_elem_value *ucontrol)
+{
+	struct audio_iio_aux_chan *chan = (struct audio_iio_aux_chan *)kcontrol->private_value;
+	int max = chan->max;
+	int min = chan->min;
+	bool invert_range = chan->is_invert_range;
+	int val;
+	int ret;
+	int tmp;
+
+	val = ucontrol->value.integer.value[0];
+	if (val < 0)
+		return -EINVAL;
+	if (val > max - min)
+		return -EINVAL;
+
+	val = val + min;
+	if (invert_range)
+		val = max - val;
+
+	ret = iio_read_channel_raw(chan->iio_chan, &tmp);
+	if (ret < 0)
+		return ret;
+
+	if (tmp == val)
+		return 0;
+
+	ret = iio_write_channel_raw(chan->iio_chan, val);
+	if (ret)
+		return ret;
+
+	return 1; /* The value changed */
+}
+
+static int audio_iio_aux_add_controls(struct snd_soc_component *component,
+				      struct audio_iio_aux_chan *chan)
+{
+	struct snd_kcontrol_new control = {0};
+
+	control.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
+	control.name = chan->name;
+	control.info = audio_iio_aux_info_volsw;
+	control.get = audio_iio_aux_get_volsw;
+	control.put = audio_iio_aux_put_volsw;
+	control.private_value = (unsigned long)chan;
+
+	return snd_soc_add_component_controls(component, &control, 1);
+}
+
+/*
+ * These data could be on stack but they are pretty big.
+ * As ASoC internally copy them and protect them against concurrent accesses
+ * (snd_soc_bind_card() protects using client_mutex), keep them in the global
+ * data area.
+ */
+static struct snd_soc_dapm_widget widgets[3] = {0};
+static struct snd_soc_dapm_route routes[2] = {0};
+
+static int audio_iio_aux_add_dapms(struct snd_soc_component *component,
+				   struct audio_iio_aux_chan *chan)
+{
+	struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
+	char *input_name = NULL;
+	char *output_name = NULL;
+	char *pga_name = NULL;
+	int ret;
+
+	input_name = kasprintf(GFP_KERNEL, "%s IN", chan->name);
+	if (!input_name) {
+		ret = -ENOMEM;
+		goto end;
+	}
+	output_name = kasprintf(GFP_KERNEL, "%s OUT", chan->name);
+	if (!output_name) {
+		ret = -ENOMEM;
+		goto end;
+	}
+	pga_name = kasprintf(GFP_KERNEL, "%s PGA", chan->name);
+	if (!pga_name) {
+		ret = -ENOMEM;
+		goto end;
+	}
+
+	BUILD_BUG_ON(ARRAY_SIZE(widgets) < 3);
+	widgets[0] = SND_SOC_DAPM_WIDGET(SND_SOC_DAPM_INPUT(input_name));
+	widgets[1] = SND_SOC_DAPM_WIDGET(SND_SOC_DAPM_OUTPUT(output_name));
+	widgets[2] = SND_SOC_DAPM_WIDGET(SND_SOC_DAPM_PGA(pga_name, SND_SOC_NOPM, 0, 0, NULL, 0));
+	ret = snd_soc_dapm_new_controls(dapm, widgets, 3);
+	if (ret)
+		goto end;
+
+	BUILD_BUG_ON(ARRAY_SIZE(routes) < 2);
+	routes[0].sink = pga_name;
+	routes[0].control = NULL;
+	routes[0].source = input_name;
+	routes[1].sink = output_name;
+	routes[1].control = NULL;
+	routes[1].source = pga_name;
+	ret = snd_soc_dapm_add_routes(dapm, routes, 2);
+
+end:
+	/* Allocated names are no more needed (duplicated in ASoC internals) */
+	kfree(pga_name);
+	kfree(output_name);
+	kfree(input_name);
+
+	return ret;
+}
+
+static int audio_iio_aux_component_probe(struct snd_soc_component *component)
+{
+	struct audio_iio_aux *iio_aux = snd_soc_component_get_drvdata(component);
+	struct audio_iio_aux_chan *chan;
+	int ret;
+	int i;
+
+	for (i = 0; i < iio_aux->num_chans; i++) {
+		chan = iio_aux->chans + i;
+
+		ret = iio_read_max_channel_raw(chan->iio_chan, &chan->max);
+		if (ret) {
+			dev_err(component->dev, "chan[%d] %s: Cannot get max raw value (%d)\n",
+				i, chan->name, ret);
+			return ret;
+		}
+
+		ret = iio_read_min_channel_raw(chan->iio_chan, &chan->min);
+		if (ret) {
+			dev_err(component->dev, "chan[%d] %s: Cannot get min raw value (%d)\n",
+				i, chan->name, ret);
+			return ret;
+		}
+
+		/* Set initial value */
+		ret = iio_write_channel_raw(chan->iio_chan,
+					    chan->is_invert_range ? chan->max : chan->min);
+		if (ret) {
+			dev_err(component->dev, "chan[%d] %s: Cannot set initial value (%d)\n",
+				i, chan->name, ret);
+			return ret;
+		}
+
+		ret = audio_iio_aux_add_controls(component, chan);
+		if (ret)
+			return ret;
+
+		ret = audio_iio_aux_add_dapms(component, chan);
+		if (ret)
+			return ret;
+
+		dev_dbg(component->dev, "chan[%d]: Added %s (min=%d, max=%d, invert=%s)\n",
+			i, chan->name, chan->min, chan->max,
+			chan->is_invert_range ? "on" : "off");
+	}
+
+	return 0;
+}
+
+static const struct snd_soc_component_driver audio_iio_aux_component_driver = {
+	.probe = audio_iio_aux_component_probe,
+};
+
+static int audio_iio_aux_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct audio_iio_aux_chan *iio_aux_chan;
+	struct audio_iio_aux *iio_aux;
+	int count;
+	u32 tmp;
+	int ret;
+	int i;
+
+	iio_aux = devm_kzalloc(&pdev->dev, sizeof(*iio_aux), GFP_KERNEL);
+	if (!iio_aux)
+		return -ENOMEM;
+
+	iio_aux->dev = &pdev->dev;
+
+	count = of_property_count_strings(np, "io-channel-names");
+	if (count < 0) {
+		dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names\n", np);
+		return count;
+	}
+
+	iio_aux->chans = devm_kmalloc_array(&pdev->dev, count,
+					    sizeof(*iio_aux->chans), GFP_KERNEL);
+	if (!iio_aux->chans)
+		return -ENOMEM;
+	iio_aux->num_chans = count;
+
+	for (i = 0; i < iio_aux->num_chans; i++) {
+		iio_aux_chan = iio_aux->chans + i;
+
+		ret = of_property_read_string_index(np, "io-channel-names", i,
+						    &iio_aux_chan->name);
+		if (ret < 0) {
+			dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names[%d]\n", np, i);
+			return ret;
+		}
+
+		iio_aux_chan->iio_chan = devm_iio_channel_get(iio_aux->dev, iio_aux_chan->name);
+		if (IS_ERR(iio_aux_chan->iio_chan)) {
+			return dev_err_probe(iio_aux->dev,
+					     PTR_ERR(iio_aux_chan->iio_chan),
+					     "get IIO channel '%s' failed\n",
+					     iio_aux_chan->name);
+		}
+
+		tmp = 0;
+		of_property_read_u32_index(np, "snd-control-invert-range", i, &tmp);
+		iio_aux_chan->is_invert_range = tmp;
+	}
+
+	platform_set_drvdata(pdev, iio_aux);
+
+	return devm_snd_soc_register_component(iio_aux->dev,
+					       &audio_iio_aux_component_driver,
+					       NULL, 0);
+}
+
+static const struct of_device_id audio_iio_aux_ids[] = {
+	{ .compatible = "audio-iio-aux", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, audio_iio_aux_ids);
+
+static struct platform_driver audio_iio_aux_driver = {
+	.driver = {
+		.name = "audio-iio-aux",
+		.of_match_table = audio_iio_aux_ids,
+	},
+	.probe = audio_iio_aux_probe,
+};
+
+module_platform_driver(audio_iio_aux_driver);
+
+MODULE_AUTHOR("Herve Codina <herve.codina@bootlin.com>");
+MODULE_DESCRIPTION("IIO ALSA SoC aux driver");
+MODULE_LICENSE("GPL");
-- 
2.40.1

[PATCH v2 8/9] ASoC: simple-card: Add missing of_node_put() in case of error

[Herve Codina]

In the error path, a of_node_put() for platform is missing.
Just add it.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 sound/soc/generic/simple-card.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 6f044cc8357e..5a5e4ecd0f61 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -416,6 +416,7 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
 
 			if (ret < 0) {
 				of_node_put(codec);
+				of_node_put(plat);
 				of_node_put(np);
 				goto error;
 			}
-- 
2.40.1

[PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[Herve Codina]

An additional-devs subnode can be present in the simple-card top node.
This subnode is used to declared some "virtual" additional devices.

Create related devices from this subnode and avoid this subnode presence
to interfere with the already supported subnodes analysis.

Signed-off-by: Herve Codina <herve.codina@bootlin.com>
---
 sound/soc/generic/simple-card.c | 52 +++++++++++++++++++++++++++++++--
 1 file changed, 50 insertions(+), 2 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 5a5e4ecd0f61..4992ab433d6a 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -348,6 +348,7 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
 	struct device *dev = simple_priv_to_dev(priv);
 	struct device_node *top = dev->of_node;
 	struct device_node *node;
+	struct device_node *add_devs;
 	uintptr_t dpcm_selectable = (uintptr_t)of_device_get_match_data(dev);
 	bool is_top = 0;
 	int ret = 0;
@@ -359,6 +360,8 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
 		is_top = 1;
 	}
 
+	add_devs = of_get_child_by_name(top, PREFIX "additional-devs");
+
 	/* loop for all dai-link */
 	do {
 		struct asoc_simple_data adata;
@@ -367,6 +370,12 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
 		struct device_node *np;
 		int num = of_get_child_count(node);
 
+		/* Skip additional-devs node */
+		if (node == add_devs) {
+			node = of_get_next_child(top, node);
+			continue;
+		}
+
 		/* get codec */
 		codec = of_get_child_by_name(node, is_top ?
 					     PREFIX "codec" : "codec");
@@ -380,12 +389,15 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
 
 		/* get convert-xxx property */
 		memset(&adata, 0, sizeof(adata));
-		for_each_child_of_node(node, np)
+		for_each_child_of_node(node, np) {
+			if (np == add_devs)
+				continue;
 			simple_parse_convert(dev, np, &adata);
+		}
 
 		/* loop for all CPU/Codec node */
 		for_each_child_of_node(node, np) {
-			if (plat == np)
+			if (plat == np || add_devs == np)
 				continue;
 			/*
 			 * It is DPCM
@@ -428,6 +440,7 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
 	} while (!is_top && node);
 
  error:
+	of_node_put(add_devs);
 	of_node_put(node);
 	return ret;
 }
@@ -652,6 +665,36 @@ static int simple_soc_probe(struct snd_soc_card *card)
 	return 0;
 }
 
+static void simple_populate_aux_release(struct device *dev, void *res)
+{
+	of_platform_depopulate(*(struct device **)res);
+}
+
+static int simple_populate_aux(struct asoc_simple_priv *priv)
+{
+	struct device *dev = simple_priv_to_dev(priv);
+	struct device_node *node;
+	struct device **ptr;
+	int ret;
+
+	node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs");
+	if (!node)
+		return 0;
+
+	ptr = devres_alloc(simple_populate_aux_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return -ENOMEM;
+
+	ret = of_platform_populate(node, NULL, NULL, dev);
+	if (ret) {
+		devres_free(ptr);
+	} else {
+		*ptr = dev;
+		devres_add(dev, ptr);
+	}
+	return ret;
+}
+
 static int asoc_simple_probe(struct platform_device *pdev)
 {
 	struct asoc_simple_priv *priv;
@@ -688,6 +731,11 @@ static int asoc_simple_probe(struct platform_device *pdev)
 		return ret;
 
 	if (np && of_device_is_available(np)) {
+		ret = simple_populate_aux(priv);
+		if (ret < 0) {
+			dev_err_probe(dev, ret, "populate aux error\n");
+			goto err;
+		}
 
 		ret = simple_parse_of(priv, li);
 		if (ret < 0) {
-- 
2.40.1

Re: [PATCH v2 8/9] ASoC: simple-card: Add missing of_node_put() in case of error

[Kuninori Morimoto]

Hi

> In the error path, a of_node_put() for platform is missing.
> Just add it.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[Kuninori Morimoto]

Hi

> An additional-devs subnode can be present in the simple-card top node.
> This subnode is used to declared some "virtual" additional devices.
> 
> Create related devices from this subnode and avoid this subnode presence
> to interfere with the already supported subnodes analysis.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---

simple-card is used in many boards, but is old.
Adding new feature on audio-graph-card/audio-graph-card2 instead of simple-card
is my ideal, but it is OK.

simple-card is possible to handle multiple DAI links by using
"dai-link" node on 1 Sound Card. see

	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/sound/simple-card.yaml?h=v6.4-rc3#n294

Is this "additional-devs" available only one per 1 Card ?
If it is possible to use 1 additional-devs per 1 DAI link, I think this patch want to
care "dai-link".
Or adding temporally NOTE or FIXME message like /* NOTE: it doesn't support dai-link so far */
is good idea.

>  static int asoc_simple_probe(struct platform_device *pdev)
>  {
>  	struct asoc_simple_priv *priv;
> @@ -688,6 +731,11 @@ static int asoc_simple_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	if (np && of_device_is_available(np)) {
> +		ret = simple_populate_aux(priv);
> +		if (ret < 0) {
> +			dev_err_probe(dev, ret, "populate aux error\n");
> +			goto err;
> +		}
>  
>  		ret = simple_parse_of(priv, li);
>  		if (ret < 0) {
> -- 
> 2.40.1
> 

Calling simple_populate_aux() before calling simple_parse_of() is possible,
but looks strange for me.
see below

> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 5a5e4ecd0f61..4992ab433d6a 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
(snip)
> @@ -359,6 +360,8 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
>  		is_top = 1;
>  	}
>  
> +	add_devs = of_get_child_by_name(top, PREFIX "additional-devs");

I think better position to call simple_populate_aux() is here.
But __simple_for_each_link() will be called multiple times for CPU and for Codec.
So maybe you want to calling it under CPU turn.

	 /* NOTE: it doesn't support dai-link so far */
	add_devs = of_get_child_by_name(top, PREFIX "additional-devs");
	if (add_devs && li->cpu) {
		ret = simple_populate_aux(priv);
		...
	}

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[Kuninori Morimoto]

Hi again

> Is this "additional-devs" available only one per 1 Card ?

I could understand the point.
1 "additional-devs" per 1 Card is enough.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[Herve Codina]

Hi Kuninori,

On Wed, 24 May 2023 00:08:50 +0000
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi
> 
> > An additional-devs subnode can be present in the simple-card top node.
> > This subnode is used to declared some "virtual" additional devices.
> > 
> > Create related devices from this subnode and avoid this subnode presence
> > to interfere with the already supported subnodes analysis.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---  
> 
> simple-card is used in many boards, but is old.
> Adding new feature on audio-graph-card/audio-graph-card2 instead of simple-card
> is my ideal, but it is OK.
> 
> simple-card is possible to handle multiple DAI links by using
> "dai-link" node on 1 Sound Card. see
> 
> 	https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/sound/simple-card.yaml?h=v6.4-rc3#n294
> 
> Is this "additional-devs" available only one per 1 Card ?
> If it is possible to use 1 additional-devs per 1 DAI link, I think this patch want to
> care "dai-link".
> Or adding temporally NOTE or FIXME message like /* NOTE: it doesn't support dai-link so far */
> is good idea.

As you said on your other mail 1 "additional-devs" per 1 Card.
And further more, I think that "additional-devs" has nothing to do with
DAI link.
These additional devices are really related to the the Card itself and
not DAI links.

simple_populate_aux() needs to be called once per Card.

> 
> >  static int asoc_simple_probe(struct platform_device *pdev)
> >  {
> >  	struct asoc_simple_priv *priv;
> > @@ -688,6 +731,11 @@ static int asoc_simple_probe(struct platform_device *pdev)
> >  		return ret;
> >  
> >  	if (np && of_device_is_available(np)) {
> > +		ret = simple_populate_aux(priv);
> > +		if (ret < 0) {
> > +			dev_err_probe(dev, ret, "populate aux error\n");
> > +			goto err;
> > +		}
> >  
> >  		ret = simple_parse_of(priv, li);
> >  		if (ret < 0) {
> > -- 
> > 2.40.1
> >   
> 
> Calling simple_populate_aux() before calling simple_parse_of() is possible,
> but looks strange for me.
> see below
> 
> > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> > index 5a5e4ecd0f61..4992ab433d6a 100644
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c  
> (snip)
> > @@ -359,6 +360,8 @@ static int __simple_for_each_link(struct asoc_simple_priv *priv,
> >  		is_top = 1;
> >  	}
> >  
> > +	add_devs = of_get_child_by_name(top, PREFIX "additional-devs");  
> 
> I think better position to call simple_populate_aux() is here.
> But __simple_for_each_link() will be called multiple times for CPU and for Codec.
> So maybe you want to calling it under CPU turn.
> 
> 	 /* NOTE: it doesn't support dai-link so far */
> 	add_devs = of_get_child_by_name(top, PREFIX "additional-devs");
> 	if (add_devs && li->cpu) {
> 		ret = simple_populate_aux(priv);
> 		...
> 	}

So, IMHO, calling simple_populate_aux() from __simple_for_each_link() is
not correct as it has nothing to do with DAI links and must be call once
per Card.

simple_populate_aux() could be called by simple_parse_of() itself or after
simple_parse_of() call.

I would prefer calling it before snd_soc_of_parse_aux_devs() because
this function parses aux-devs phandles and these phandles can refer an
auxiliary device present in the additional-devs node.

The current code has no issue with this but some evolution can lead to
EPROBE_DEFER if the device related to the phandle was not probed.
If simple_populate_aux() is called after snd_soc_of_parse_aux_devs(),
an EPROBE_DEFER related to the missing probe() call has no chance to
be resolved.

Tell be what you prefer:
  a) Call before simple_parse_of() (no changes)
or
  b) Call at the very beginning of simple_parse_of()
or
  c) Other suggestion ...


Thanks a lot for your review.
Best regards,
Hervé

> 
> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto

Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[Kuninori Morimoto]

Hi Herve

Thank you for your reply.

> So, IMHO, calling simple_populate_aux() from __simple_for_each_link() is
> not correct as it has nothing to do with DAI links and must be call once
> per Card.

My biggest concern is that this code is calling same code multiple times.
It is easy to forget such thing when updating in this kind of code.
We don't forget / take mistake if these are merged.
But we have such code everywhere ;) this is just my concern, not a big deal.

	static int __simple_for_each_link (...)
	{
		...
=>		add_devs = of_get_child_by_name(top, PREFIX "additional-devs");
		...
	}

	static int simple_populate_aux(...)
	{
		...
=>		node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs");
		...
	}

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[Herve Codina]

On Thu, 25 May 2023 00:01:14 +0000
Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote:

> Hi Herve
> 
> Thank you for your reply.
> 
> > So, IMHO, calling simple_populate_aux() from __simple_for_each_link() is
> > not correct as it has nothing to do with DAI links and must be call once
> > per Card.  
> 
> My biggest concern is that this code is calling same code multiple times.
> It is easy to forget such thing when updating in this kind of code.
> We don't forget / take mistake if these are merged.
> But we have such code everywhere ;) this is just my concern, not a big deal.
> 
> 	static int __simple_for_each_link (...)
> 	{
> 		...
> =>		add_devs = of_get_child_by_name(top, PREFIX "additional-devs");  
> 		...
> 	}
> 
> 	static int simple_populate_aux(...)
> 	{
> 		...
> =>		node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs");  
> 		...
> 	}
> 

Well, of_get_child_by_name() is called twice to retrieve the additional-devs
node but for very different reason.

In __simple_for_each_link() to filter out the node as it has nothing to do with a DAI.
In simple_populate_aux() to take care of the devices declared in the node.

I am not sure that we should avoid that.
It will lead to a more complex code and flags just to avoid this call.

Not sure that it will be better.
__simple_for_each_link() is called multiple times and is supposed to look at links.
To avoid the of_get_child_by_name() filter-out call, __simple_for_each_link()
will look at link *and* populate devices calling simple_populate_aux().
And to do that correctly it will use a flag to be sure that simple_populate_aux()
was called only once.


In order to avoid some kind of duplication (at least the node name):

	static struct device_node *simple_of_get_add_devs(struct device_node *node)
	{
		return of_get_child_by_name(node, PREFIX "additional-devs");
	}

	static int __simple_for_each_link (...)
	{
		...
=>		add_devs = simple_of_get_add_devs(top);  
		...
	}

	static int simple_populate_aux(...)
	{
		...
=>		node = simple_of_get_add_devs(dev->of_node);  
		...
	}


Does it look better ?

Best regards,
Hervé

> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto



-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: (subset) [PATCH v2 0/9] Add support for IIO devices in ASoC

[Mark Brown]

On Tue, 23 May 2023 17:12:14 +0200, Herve Codina wrote:
> 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].
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[8/9] ASoC: simple-card: Add missing of_node_put() in case of error
      commit: 8938f75a5e35c597a647c28984a0304da7a33d63

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v2 4/9] iio: consumer.h: Fix raw values documentation notes

[Jonathan Cameron]

On Tue, 23 May 2023 17:12:18 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> The raw values notes mention 'ADC counts' and are not fully accurate.
> 
> Reword the notes in order to remove the 'ADC counts' and describe the
> conversion needed between a raw value and a value in the standard units.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
I'm not 100% sure what path this will take - though hopefully and immutable
branch I can pick up will be involved. On that basis I won't pick this up
now and instead give

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
>  include/linux/iio/consumer.h | 25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index 6802596b017c..f536820b9cf2 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -201,8 +201,9 @@ struct iio_dev
>   * @chan:		The channel being queried.
>   * @val:		Value read back.
>   *
> - * Note raw reads from iio channels are in adc counts and hence
> - * scale will need to be applied if standard units required.
> + * Note, if standard units are required, raw reads from iio channels
> + * need the offset (default 0) and scale (default 1) to be applied
> + * as (raw + offset) * scale.
>   */
>  int iio_read_channel_raw(struct iio_channel *chan,
>  			 int *val);
> @@ -212,8 +213,9 @@ int iio_read_channel_raw(struct iio_channel *chan,
>   * @chan:		The channel being queried.
>   * @val:		Value read back.
>   *
> - * Note raw reads from iio channels are in adc counts and hence
> - * scale will need to be applied if standard units required.
> + * Note, if standard units are required, raw reads from iio channels
> + * need the offset (default 0) and scale (default 1) to be applied
> + * as (raw + offset) * scale.
>   *
>   * In opposit to the normal iio_read_channel_raw this function
>   * returns the average of multiple reads.
> @@ -281,8 +283,9 @@ int iio_read_channel_attribute(struct iio_channel *chan, int *val,
>   * @chan:		The channel being queried.
>   * @val:		Value being written.
>   *
> - * Note raw writes to iio channels are in dac counts and hence
> - * scale will need to be applied if standard units required.
> + * Note that for raw writes to iio channels, if the value provided is
> + * in standard units, the affect of the scale and offset must be removed
> + * as (value / scale) - offset.
>   */
>  int iio_write_channel_raw(struct iio_channel *chan, int val);
>  
> @@ -292,8 +295,9 @@ int iio_write_channel_raw(struct iio_channel *chan, int val);
>   * @chan:		The channel being queried.
>   * @val:		Value read back.
>   *
> - * Note raw reads from iio channels are in adc counts and hence
> - * scale will need to be applied if standard units are required.
> + * Note, if standard units are required, raw reads from iio channels
> + * need the offset (default 0) and scale (default 1) to be applied
> + * as (raw + offset) * scale.
>   */
>  int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
>  
> @@ -308,8 +312,9 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
>   * For ranges, three vals are always returned; min, step and max.
>   * For lists, all the possible values are enumerated.
>   *
> - * Note raw available values from iio channels are in adc counts and
> - * hence scale will need to be applied if standard units are required.
> + * Note, if standard units are required, raw available values from iio
> + * channels need the offset (default 0) and scale (default 1) to be applied
> + * as (raw + offset) * scale.
>   */
>  int iio_read_avail_channel_raw(struct iio_channel *chan,
>  			       const int **vals, int *length);

Re: [PATCH v2 3/9] iio: inkern: Check error explicitly in iio_channel_read_max()

[Jonathan Cameron]

On Tue, 23 May 2023 17:12:17 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> The current implementation returns the error code as part of the
> default switch case.
> This can lead to returning an incorrect positive value in case of
> iio_avail_type enum entries evolution.
> 
> In order to avoid this case, be more strict in error checking.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
It's an odd code construct, so in the interests of robustness this
looks like a good improvement to me.

Given the later patch you need for this series will see some fuzz
I think if I pick this up separately I'll currently assume this whole
series will go together.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/iio/inkern.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index 872fd5c24147..f738db9a0c04 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -858,6 +858,9 @@ static int iio_channel_read_max(struct iio_channel *chan,
>  		val2 = &unused;
>  
>  	ret = iio_channel_read_avail(chan, &vals, type, &length, info);
> +	if (ret < 0)
> +		return ret;
> +
>  	switch (ret) {
>  	case IIO_AVAIL_RANGE:
>  		switch (*type) {
> @@ -888,7 +891,7 @@ static int iio_channel_read_max(struct iio_channel *chan,
>  		return 0;
>  
>  	default:
> -		return ret;
> +		return -EINVAL;
>  	}
>  }
>  

Re: [PATCH v2 5/9] iio: inkern: Add a helper to query an available minimum raw value

[Jonathan Cameron]

On Tue, 23 May 2023 17:12:19 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> A helper, iio_read_max_channel_raw() exists to read the available
> maximum raw value of a channel but nothing similar exists to read the
> available minimum raw value.
> 
> This new helper, iio_read_min_channel_raw(), fills the hole and can be
> used for reading the available minimum raw value of a channel.
> It is fully based on the existing iio_read_max_channel_raw().
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/iio/inkern.c         | 70 ++++++++++++++++++++++++++++++++++++
>  include/linux/iio/consumer.h | 12 +++++++
>  2 files changed, 82 insertions(+)
> 
> diff --git a/drivers/iio/inkern.c b/drivers/iio/inkern.c
> index f738db9a0c04..07b26ff8a334 100644
> --- a/drivers/iio/inkern.c
> +++ b/drivers/iio/inkern.c
> @@ -915,6 +915,76 @@ int iio_read_max_channel_raw(struct iio_channel *chan, int *val)
>  }
>  EXPORT_SYMBOL_GPL(iio_read_max_channel_raw);
>  
> +static int iio_channel_read_min(struct iio_channel *chan,
> +				int *val, int *val2, int *type,
> +				enum iio_chan_info_enum info)
> +{
> +	int unused;
> +	const int *vals;
> +	int length;
> +	int ret;
> +
> +	if (!val2)
> +		val2 = &unused;
> +
> +	ret = iio_channel_read_avail(chan, &vals, type, &length, info);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case IIO_AVAIL_RANGE:
> +		switch (*type) {
> +		case IIO_VAL_INT:
> +			*val = vals[0];
> +			break;
> +		default:
> +			*val = vals[0];
> +			*val2 = vals[1];
> +		}
> +		return 0;
> +
> +	case IIO_AVAIL_LIST:
> +		if (length <= 0)
> +			return -EINVAL;
> +		switch (*type) {
> +		case IIO_VAL_INT:
> +			*val = vals[--length];
> +			while (length) {
> +				if (vals[--length] < *val)
> +					*val = vals[length];
> +			}
> +			break;
> +		default:
> +			/* FIXME: learn about min for other iio values */
> +			return -EINVAL;
> +		}
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +int iio_read_min_channel_raw(struct iio_channel *chan, int *val)
> +{
> +	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
> +	int ret;
> +	int type;
> +
> +	mutex_lock(&iio_dev_opaque->info_exist_lock);
> +	if (!chan->indio_dev->info) {
> +		ret = -ENODEV;
> +		goto err_unlock;
> +	}
> +
> +	ret = iio_channel_read_min(chan, val, NULL, &type, IIO_CHAN_INFO_RAW);
> +err_unlock:
> +	mutex_unlock(&iio_dev_opaque->info_exist_lock);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(iio_read_min_channel_raw);
> +
>  int iio_get_channel_type(struct iio_channel *chan, enum iio_chan_type *type)
>  {
>  	struct iio_dev_opaque *iio_dev_opaque = to_iio_dev_opaque(chan->indio_dev);
> diff --git a/include/linux/iio/consumer.h b/include/linux/iio/consumer.h
> index f536820b9cf2..e9910b41d48e 100644
> --- a/include/linux/iio/consumer.h
> +++ b/include/linux/iio/consumer.h
> @@ -301,6 +301,18 @@ int iio_write_channel_raw(struct iio_channel *chan, int val);
>   */
>  int iio_read_max_channel_raw(struct iio_channel *chan, int *val);
>  
> +/**
> + * iio_read_min_channel_raw() - read minimum available raw value from a given
> + *				channel, i.e. the minimum possible value.
> + * @chan:		The channel being queried.
> + * @val:		Value read back.
> + *
> + * Note, if standard units are required, raw reads from iio channels
> + * need the offset (default 0) and scale (default 1) to be applied
> + * as (raw + offset) * scale.
> + */
> +int iio_read_min_channel_raw(struct iio_channel *chan, int *val);
> +
>  /**
>   * iio_read_avail_channel_raw() - read available raw values from a given channel
>   * @chan:		The channel being queried.

Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Jonathan Cameron]

On Tue, 23 May 2023 17:12:21 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

> Industrial I/O devices can be present in the audio path.
> These devices needs to be used as audio components in order to be fully
> integrated in the audio path.
> 
> This support allows to consider these Industrial I/O devices as auxliary
> audio devices and allows to control them using mixer controls.
> 
> Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> ---

> diff --git a/sound/soc/codecs/audio-iio-aux.c b/sound/soc/codecs/audio-iio-aux.c
> new file mode 100644
> index 000000000000..21575c4b35fd
> --- /dev/null
> +++ b/sound/soc/codecs/audio-iio-aux.c
> @@ -0,0 +1,302 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// audio-iio-aux.c  --  ALSA SoC glue to use IIO devices as audio components
> +//
> +// Copyright 2023 CS GROUP France
> +//
> +// Author: Herve Codina <herve.codina@bootlin.com>
> +
> +#include <linux/iio/consumer.h>
> +#include <linux/module.h>

#include <linux/mod_devicetable.h> ideally to pick up
the of_device_id definition without bouncing through some non 
obvious header path.


> +#include <linux/slab.h>
> +#include <sound/soc.h>
> +#include <sound/tlv.h>

Otherwise, the IIO elements of this look good.  So for those at least
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

I don't have enough knowledge of the snd stuff to review those
parts.

Jonathan

Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[Kuninori Morimoto]

Hi Herve

Thank you for the reply

> > 	static int __simple_for_each_link (...)
> > 	{
> > 		...
> > =>		add_devs = of_get_child_by_name(top, PREFIX "additional-devs");  
> > 		...
> > 	}
> > 
> > 	static int simple_populate_aux(...)
> > 	{
> > 		...
> > =>		node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs");  
> > 		...
> > 	}
> > 
> 
> Well, of_get_child_by_name() is called twice to retrieve the additional-devs
> node but for very different reason.
> 
> In __simple_for_each_link() to filter out the node as it has nothing to do with a DAI.
> In simple_populate_aux() to take care of the devices declared in the node.

I thought it is better to handling "device" and "filtering" in the same place,
if it has "additional-devs" on the DT. Because we don't need to filtering
if don't need to care about device.

But this is very small detail, not a big deal to discuss about for
a long time.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [PATCH v2 5/9] iio: inkern: Add a helper to query an available minimum raw value

[andy.shevchenko]

Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti:
> A helper, iio_read_max_channel_raw() exists to read the available
> maximum raw value of a channel but nothing similar exists to read the
> available minimum raw value.
> 
> This new helper, iio_read_min_channel_raw(), fills the hole and can be
> used for reading the available minimum raw value of a channel.
> It is fully based on the existing iio_read_max_channel_raw().

...

> +static int iio_channel_read_min(struct iio_channel *chan,
> +				int *val, int *val2, int *type,
> +				enum iio_chan_info_enum info)
> +{
> +	int unused;
> +	const int *vals;
> +	int length;
> +	int ret;

> +	if (!val2)
> +		val2 = &unused;

It's a single place, where this is used, can you move it there?

> +	ret = iio_channel_read_avail(chan, &vals, type, &length, info);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret) {
> +	case IIO_AVAIL_RANGE:
> +		switch (*type) {
> +		case IIO_VAL_INT:
> +			*val = vals[0];
> +			break;
> +		default:
> +			*val = vals[0];
> +			*val2 = vals[1];
> +		}
> +		return 0;
> +
> +	case IIO_AVAIL_LIST:
> +		if (length <= 0)
> +			return -EINVAL;
> +		switch (*type) {
> +		case IIO_VAL_INT:
> +			*val = vals[--length];

> +			while (length) {

			while (length--) {

will do the job and at the same time...


> +				if (vals[--length] < *val)
> +					*val = vals[length];

...this construction becomes less confusing (easier to parse).

> +			}
> +			break;
> +		default:
> +			/* FIXME: learn about min for other iio values */

I believe in a final version this comment won't be here.

> +			return -EINVAL;
> +		}
> +		return 0;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/9] ASoC: soc-dapm.h: Add a helper to build a DAPM widget dynamically

[andy.shevchenko]

Tue, May 23, 2023 at 05:12:20PM +0200, Herve Codina kirjoitti:
> The SND_SOC_DAPM_* helpers family are used to build widgets array in a
> static way.
> 
> Introduce SND_SOC_DAPM_WIDGET() in order to use the SND_SOC_DAPM_*
> helpers family in a dynamic way. The different SND_SOC_DAPM_* parameters
> can be computed by the code and the widget can be built based on this
> parameter computation.
> For instance:
>   static int create_widget(char *input_name)
>   {
> 	struct snd_soc_dapm_widget widget;
> 	char name*;
> 	...
> 	name = input_name;
> 	if (!name)
> 		name = "default";
> 
> 	widget = SND_SOC_DAPM_WIDGET(SND_SOC_DAPM_INPUT(name));
> 	...
>   }

Maybe instead of adding a helper, simply convert those macros to provide
a compaund literal? (See, for example,
https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/pinctrl/pinctrl.h#L42)

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[andy.shevchenko]

Tue, May 23, 2023 at 05:12:21PM +0200, Herve Codina kirjoitti:
> Industrial I/O devices can be present in the audio path.
> These devices needs to be used as audio components in order to be fully
> integrated in the audio path.
> 
> This support allows to consider these Industrial I/O devices as auxliary
> audio devices and allows to control them using mixer controls.

...

> +// audio-iio-aux.c  --  ALSA SoC glue to use IIO devices as audio components

Putting file name into file is not a good idea in case the file will be renamed
in the future.

...

> +struct audio_iio_aux_chan {
> +	struct iio_channel *iio_chan;
> +	const char *name;
> +	bool is_invert_range;

If you put bool after int:s it may save a few bytes in some cases.

> +	int max;
> +	int min;

Wondering if there is already a data type for the ranges (like linear_range.h,
but not sure it's applicable here).

> +};

...

> +	if (val < 0)
> +		return -EINVAL;
> +	if (val > max - min)

Btw, who will validate that max > min?

> +		return -EINVAL;

...

> +	return 1; /* The value changed */

Perhaps this 1 needs a definition?

...

> +static struct snd_soc_dapm_widget widgets[3] = {0};
> +static struct snd_soc_dapm_route routes[2] = {0};

0:s are not needed. Moreover, the entire assingments are redundant
as this is guaranteed by the C standard.

...

> +	char *input_name = NULL;
> +	char *output_name = NULL;
> +	char *pga_name = NULL;

Redundant assignments if you properly label the freeing.

...

> +	BUILD_BUG_ON(ARRAY_SIZE(widgets) < 3);

Use static_assert() at the place where the array is defined.

...

> +	BUILD_BUG_ON(ARRAY_SIZE(routes) < 2);

Ditto.

...

> +end:

out_free:

> +	/* Allocated names are no more needed (duplicated in ASoC internals) */
> +	kfree(pga_name);
> +	kfree(output_name);
> +	kfree(input_name);
> +
> +	return ret;

...

> +	for (i = 0; i < iio_aux->num_chans; i++) {
> +		chan = iio_aux->chans + i;
> +
> +		ret = iio_read_max_channel_raw(chan->iio_chan, &chan->max);
> +		if (ret) {
> +			dev_err(component->dev, "chan[%d] %s: Cannot get max raw value (%d)\n",
> +				i, chan->name, ret);
> +			return ret;

It sounds like a part of ->probe() flow, correct?
Can dev_err_probe() be used here?

> +		}
> +
> +		ret = iio_read_min_channel_raw(chan->iio_chan, &chan->min);
> +		if (ret) {
> +			dev_err(component->dev, "chan[%d] %s: Cannot get min raw value (%d)\n",
> +				i, chan->name, ret);
> +			return ret;

Ditto.

> +		}
> +
> +		/* Set initial value */
> +		ret = iio_write_channel_raw(chan->iio_chan,
> +					    chan->is_invert_range ? chan->max : chan->min);
> +		if (ret) {
> +			dev_err(component->dev, "chan[%d] %s: Cannot set initial value (%d)\n",
> +				i, chan->name, ret);
> +			return ret;

Ditto.

> +		}

...

> +		dev_dbg(component->dev, "chan[%d]: Added %s (min=%d, max=%d, invert=%s)\n",
> +			i, chan->name, chan->min, chan->max,
> +			chan->is_invert_range ? "on" : "off");

str_on_off()

> +	}

...

> +	count = of_property_count_strings(np, "io-channel-names");
> +	if (count < 0) {

> +		dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names\n", np);
> +		return count;

		return dev_err_probe();

> +	}

...

> +	for (i = 0; i < iio_aux->num_chans; i++) {
> +		iio_aux_chan = iio_aux->chans + i;
> +
> +		ret = of_property_read_string_index(np, "io-channel-names", i,
> +						    &iio_aux_chan->name);
> +		if (ret < 0) {
> +			dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names[%d]\n", np, i);
> +			return ret;

Ditto.

> +		}

> +		tmp = 0;
> +		of_property_read_u32_index(np, "snd-control-invert-range", i, &tmp);

> +		iio_aux_chan->is_invert_range = tmp;

You can use this variable directly.

> +	}

Btw, can you avoid using OF APIs? It's better to have device property/fwnode
API to be used from day 1.

...

> +	platform_set_drvdata(pdev, iio_aux);

Which callback is using this driver data?

...

> +static const struct of_device_id audio_iio_aux_ids[] = {
> +	{ .compatible = "audio-iio-aux", },

Inner comma is not needed.

> +	{ }
> +};

...

> +static struct platform_driver audio_iio_aux_driver = {
> +	.driver = {
> +		.name = "audio-iio-aux",
> +		.of_match_table = audio_iio_aux_ids,
> +	},
> +	.probe = audio_iio_aux_probe,
> +};

> +

Redundant blank line

> +module_platform_driver(audio_iio_aux_driver);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 9/9] ASoC: simple-card: Handle additional devices

[andy.shevchenko]

Tue, May 23, 2023 at 05:12:23PM +0200, Herve Codina kirjoitti:
> An additional-devs subnode can be present in the simple-card top node.
> This subnode is used to declared some "virtual" additional devices.
> 
> Create related devices from this subnode and avoid this subnode presence
> to interfere with the already supported subnodes analysis.

...

> +static int simple_populate_aux(struct asoc_simple_priv *priv)
> +{
> +	struct device *dev = simple_priv_to_dev(priv);
> +	struct device_node *node;
> +	struct device **ptr;
> +	int ret;
> +
> +	node = of_get_child_by_name(dev->of_node, PREFIX "additional-devs");
> +	if (!node)
> +		return 0;
> +
> +	ptr = devres_alloc(simple_populate_aux_release, sizeof(*ptr), GFP_KERNEL);
> +	if (!ptr)
> +		return -ENOMEM;
> +
> +	ret = of_platform_populate(node, NULL, NULL, dev);
> +	if (ret) {
> +		devres_free(ptr);
> +	} else {
> +		*ptr = dev;
> +		devres_add(dev, ptr);
> +	}
> +	return ret;

This can be well simplified by using devm_add_action_or_reset().

> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/9] iio: inkern: Add a helper to query an available minimum raw value

[Herve Codina]

On Sat, 3 Jun 2023 17:04:37 +0300
andy.shevchenko@gmail.com wrote:

> Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti:
> > A helper, iio_read_max_channel_raw() exists to read the available
> > maximum raw value of a channel but nothing similar exists to read the
> > available minimum raw value.
> > 
> > This new helper, iio_read_min_channel_raw(), fills the hole and can be
> > used for reading the available minimum raw value of a channel.
> > It is fully based on the existing iio_read_max_channel_raw().  
> 
> ...
> 
> > +static int iio_channel_read_min(struct iio_channel *chan,
> > +				int *val, int *val2, int *type,
> > +				enum iio_chan_info_enum info)
> > +{
> > +	int unused;
> > +	const int *vals;
> > +	int length;
> > +	int ret;  
> 
> > +	if (!val2)
> > +		val2 = &unused;  
> 
> It's a single place, where this is used, can you move it there?

I will do that in the next iteration.
Also, I will do the same modification in iio_channel_read_max() as it has
exactly the same code.

> 
> > +	ret = iio_channel_read_avail(chan, &vals, type, &length, info);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	switch (ret) {
> > +	case IIO_AVAIL_RANGE:
> > +		switch (*type) {
> > +		case IIO_VAL_INT:
> > +			*val = vals[0];
> > +			break;
> > +		default:
> > +			*val = vals[0];
> > +			*val2 = vals[1];
> > +		}
> > +		return 0;
> > +
> > +	case IIO_AVAIL_LIST:
> > +		if (length <= 0)
> > +			return -EINVAL;
> > +		switch (*type) {
> > +		case IIO_VAL_INT:
> > +			*val = vals[--length];  
> 
> > +			while (length) {  
> 
> 			while (length--) {
> 
> will do the job and at the same time...
> 
> 
> > +				if (vals[--length] < *val)
> > +					*val = vals[length];  
> 
> ...this construction becomes less confusing (easier to parse).

Indeed, I will change in the next iteration.

> 
> > +			}
> > +			break;
> > +		default:
> > +			/* FIXME: learn about min for other iio values */  
> 
> I believe in a final version this comment won't be here.

We have the same FIXME comment in the iio_channel_read_max() function I
copied to create this iio_channel_read_min() and, to be honest, I
don't really know how to handle these other cases.

In this series, I would prefer to keep this FIXME.

> 
> > +			return -EINVAL;
> > +		}
> > +		return 0;
> > +
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}  
> 

Thanks for the review,
Hervé

Re: [PATCH v2 6/9] ASoC: soc-dapm.h: Add a helper to build a DAPM widget dynamically

[Herve Codina]

On Sat, 3 Jun 2023 17:07:43 +0300
andy.shevchenko@gmail.com wrote:

> Tue, May 23, 2023 at 05:12:20PM +0200, Herve Codina kirjoitti:
> > The SND_SOC_DAPM_* helpers family are used to build widgets array in a
> > static way.
> > 
> > Introduce SND_SOC_DAPM_WIDGET() in order to use the SND_SOC_DAPM_*
> > helpers family in a dynamic way. The different SND_SOC_DAPM_* parameters
> > can be computed by the code and the widget can be built based on this
> > parameter computation.
> > For instance:
> >   static int create_widget(char *input_name)
> >   {
> > 	struct snd_soc_dapm_widget widget;
> > 	char name*;
> > 	...
> > 	name = input_name;
> > 	if (!name)
> > 		name = "default";
> > 
> > 	widget = SND_SOC_DAPM_WIDGET(SND_SOC_DAPM_INPUT(name));
> > 	...
> >   }  
> 
> Maybe instead of adding a helper, simply convert those macros to provide
> a compaund literal? (See, for example,
> https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/pinctrl/pinctrl.h#L42)
> 

Indeed, I will convert macros and remove the helper in the next iteration.

Thanks for the review,
Hervé

Re: [PATCH v2 5/9] iio: inkern: Add a helper to query an available minimum raw value

[Andy Shevchenko]

On Mon, Jun 5, 2023 at 10:46 AM Herve Codina <herve.codina@bootlin.com> wrote:
> On Sat, 3 Jun 2023 17:04:37 +0300
> andy.shevchenko@gmail.com wrote:
> > Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti:

...

> > > +           case IIO_VAL_INT:
> > > +                   *val = vals[--length];
> >
> > > +                   while (length) {
> >
> >                       while (length--) {
> >
> > will do the job and at the same time...
> >
> > > +                           if (vals[--length] < *val)
> > > +                                   *val = vals[length];
> >
> > ...this construction becomes less confusing (easier to parse).
>
> Indeed, I will change in the next iteration.

And looking into above line, this whole construction I would prefer to
have a macro in minmax.h like

#define min_array(array, len) \
{( \
  typeof(len) __len = (len); \
  typeof(*(array)) __element = (array)[--__len]; \
  while (__len--) \
    __element = min(__element, (array)[__len]); \
  __element; \
)}

(it might need more work, but you got the idea)

> > > +                   }
> > > +                   break;

...

> > > +           default:
> > > +                   /* FIXME: learn about min for other iio values */
> >
> > I believe in a final version this comment won't be here.
>
> We have the same FIXME comment in the iio_channel_read_max() function I
> copied to create this iio_channel_read_min() and, to be honest, I
> don't really know how to handle these other cases.
>
> In this series, I would prefer to keep this FIXME.

I see, Jonathan needs to be involved here then.

> > > +                   return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/9] ASoC: soc-dapm.h: Add a helper to build a DAPM widget dynamically

[Herve Codina]

Hi Andy,

On Sat, 3 Jun 2023 17:07:43 +0300
andy.shevchenko@gmail.com wrote:

> Tue, May 23, 2023 at 05:12:20PM +0200, Herve Codina kirjoitti:
> > The SND_SOC_DAPM_* helpers family are used to build widgets array in a
> > static way.
> > 
> > Introduce SND_SOC_DAPM_WIDGET() in order to use the SND_SOC_DAPM_*
> > helpers family in a dynamic way. The different SND_SOC_DAPM_* parameters
> > can be computed by the code and the widget can be built based on this
> > parameter computation.
> > For instance:
> >   static int create_widget(char *input_name)
> >   {
> > 	struct snd_soc_dapm_widget widget;
> > 	char name*;
> > 	...
> > 	name = input_name;
> > 	if (!name)
> > 		name = "default";
> > 
> > 	widget = SND_SOC_DAPM_WIDGET(SND_SOC_DAPM_INPUT(name));
> > 	...
> >   }  
> 
> Maybe instead of adding a helper, simply convert those macros to provide
> a compaund literal? (See, for example,
> https://elixir.bootlin.com/linux/v6.4-rc4/source/include/linux/pinctrl/pinctrl.h#L42)
> 

Yes, I will convert them in the next iteration.

Thanks for the review,
Hervé

Re: [PATCH v2 5/9] iio: inkern: Add a helper to query an available minimum raw value

[Herve Codina]

Hi Andy,

On Mon, 5 Jun 2023 12:45:24 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Jun 5, 2023 at 10:46 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Sat, 3 Jun 2023 17:04:37 +0300
> > andy.shevchenko@gmail.com wrote:  
> > > Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti:  
> 
> ...
> 
> > > > +           case IIO_VAL_INT:
> > > > +                   *val = vals[--length];  
> > >  
> > > > +                   while (length) {  
> > >
> > >                       while (length--) {
> > >
> > > will do the job and at the same time...
> > >  
> > > > +                           if (vals[--length] < *val)
> > > > +                                   *val = vals[length];  
> > >
> > > ...this construction becomes less confusing (easier to parse).  
> >
> > Indeed, I will change in the next iteration.  
> 
> And looking into above line, this whole construction I would prefer to
> have a macro in minmax.h like
> 
> #define min_array(array, len) \
> {( \
>   typeof(len) __len = (len); \
>   typeof(*(array)) __element = (array)[--__len]; \
>   while (__len--) \
>     __element = min(__element, (array)[__len]); \
>   __element; \
> )}
> 
> (it might need more work, but you got the idea)

I will also introduce max_array() and update both iio_channel_read_max()
and iio_channel_read_min() to use these macros.

Will be available in the next series iteration.

Thanks,
Hervé

> 
> > > > +                   }
> > > > +                   break;  
> 
> ...
> 
> > > > +           default:
> > > > +                   /* FIXME: learn about min for other iio values */  
> > >
> > > I believe in a final version this comment won't be here.  
> >
> > We have the same FIXME comment in the iio_channel_read_max() function I
> > copied to create this iio_channel_read_min() and, to be honest, I
> > don't really know how to handle these other cases.
> >
> > In this series, I would prefer to keep this FIXME.  
> 
> I see, Jonathan needs to be involved here then.
> 
> > > > +                   return -EINVAL;  
> 

Re: [PATCH v2 5/9] iio: inkern: Add a helper to query an available minimum raw value

[Jonathan Cameron]

On Mon, 5 Jun 2023 12:45:24 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

> On Mon, Jun 5, 2023 at 10:46 AM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Sat, 3 Jun 2023 17:04:37 +0300
> > andy.shevchenko@gmail.com wrote:  
> > > Tue, May 23, 2023 at 05:12:19PM +0200, Herve Codina kirjoitti:  
> 
> ...
> 
> > > > +           case IIO_VAL_INT:
> > > > +                   *val = vals[--length];  
> > >  
> > > > +                   while (length) {  
> > >
> > >                       while (length--) {
> > >
> > > will do the job and at the same time...
> > >  
> > > > +                           if (vals[--length] < *val)
> > > > +                                   *val = vals[length];  
> > >
> > > ...this construction becomes less confusing (easier to parse).  
> >
> > Indeed, I will change in the next iteration.  
> 
> And looking into above line, this whole construction I would prefer to
> have a macro in minmax.h like
> 
> #define min_array(array, len) \
> {( \
>   typeof(len) __len = (len); \
>   typeof(*(array)) __element = (array)[--__len]; \
>   while (__len--) \
>     __element = min(__element, (array)[__len]); \
>   __element; \
> )}
> 
> (it might need more work, but you got the idea)
> 
> > > > +                   }
> > > > +                   break;  
> 
> ...
> 
> > > > +           default:
> > > > +                   /* FIXME: learn about min for other iio values */  
> > >
> > > I believe in a final version this comment won't be here.  
> >
> > We have the same FIXME comment in the iio_channel_read_max() function I
> > copied to create this iio_channel_read_min() and, to be honest, I
> > don't really know how to handle these other cases.
> >
> > In this series, I would prefer to keep this FIXME.  
> 
> I see, Jonathan needs to be involved here then.

It's really more of a TODO when someone needs it than a FIXME.
I'm not particularly keen to see a bunch of code supporting cases
that no one uses, but it's useful to call out where the code would
go if other cases were to be supported.

Perhaps soften it to a note that doesn't have the work FIXME in it.

Jonathan


> 
> > > > +                   return -EINVAL;  
> 

Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Herve Codina]

Hi Jonathan,

On Sun, 28 May 2023 18:38:55 +0100
Jonathan Cameron <jic23@kernel.org> wrote:

> On Tue, 23 May 2023 17:12:21 +0200
> Herve Codina <herve.codina@bootlin.com> wrote:
> 
> > Industrial I/O devices can be present in the audio path.
> > These devices needs to be used as audio components in order to be fully
> > integrated in the audio path.
> > 
> > This support allows to consider these Industrial I/O devices as auxliary
> > audio devices and allows to control them using mixer controls.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > ---  
> 
> > diff --git a/sound/soc/codecs/audio-iio-aux.c b/sound/soc/codecs/audio-iio-aux.c
> > new file mode 100644
> > index 000000000000..21575c4b35fd
> > --- /dev/null
> > +++ b/sound/soc/codecs/audio-iio-aux.c
> > @@ -0,0 +1,302 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +//
> > +// audio-iio-aux.c  --  ALSA SoC glue to use IIO devices as audio components
> > +//
> > +// Copyright 2023 CS GROUP France
> > +//
> > +// Author: Herve Codina <herve.codina@bootlin.com>
> > +
> > +#include <linux/iio/consumer.h>
> > +#include <linux/module.h>  
> 
> #include <linux/mod_devicetable.h> ideally to pick up
> the of_device_id definition without bouncing through some non 
> obvious header path.

Right, <linux/module.h> will be replaced by <linux/mod_devicetable.h> in the
next iteration.

Thanks for the review,
Hervé

> 
> 
> > +#include <linux/slab.h>
> > +#include <sound/soc.h>
> > +#include <sound/tlv.h>  
> 
> Otherwise, the IIO elements of this look good.  So for those at least
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 
> I don't have enough knowledge of the snd stuff to review those
> parts.
> 
> Jonathan
> 
> 

Re: [PATCH v2 5/9] iio: inkern: Add a helper to query an available minimum raw value

[Herve Codina]

Hi Jonathan, Andy,

On Mon, 5 Jun 2023 18:05:47 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Mon, 5 Jun 2023 12:45:24 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> 
...
> > > > > +           default:
> > > > > +                   /* FIXME: learn about min for other iio values */    
> > > >
> > > > I believe in a final version this comment won't be here.    
> > >
> > > We have the same FIXME comment in the iio_channel_read_max() function I
> > > copied to create this iio_channel_read_min() and, to be honest, I
> > > don't really know how to handle these other cases.
> > >
> > > In this series, I would prefer to keep this FIXME.    
> > 
> > I see, Jonathan needs to be involved here then.  
> 
> It's really more of a TODO when someone needs it than a FIXME.
> I'm not particularly keen to see a bunch of code supporting cases
> that no one uses, but it's useful to call out where the code would
> go if other cases were to be supported.
> 
> Perhaps soften it to a note that doesn't have the work FIXME in it.
> 
> Jonathan
> 

Right, I will change to /* TODO: learn about max for other iio values */
in the next iteration (both iio_channel_read_max() and iio_channel_read_min())

Regards,
Hervé

> 
> >   
> > > > > +                   return -EINVAL;    
> >   
> 

Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Herve Codina]

Hi Andy,

On Sat, 3 Jun 2023 21:26:19 +0300
andy.shevchenko@gmail.com wrote:

> Tue, May 23, 2023 at 05:12:21PM +0200, Herve Codina kirjoitti:
> > Industrial I/O devices can be present in the audio path.
> > These devices needs to be used as audio components in order to be fully
> > integrated in the audio path.
> > 
> > This support allows to consider these Industrial I/O devices as auxliary
> > audio devices and allows to control them using mixer controls.  
> 
> ...
> 
> > +// audio-iio-aux.c  --  ALSA SoC glue to use IIO devices as audio components  
> 
> Putting file name into file is not a good idea in case the file will be renamed
> in the future.

Indeed, the file name will be removed in the nest iteration.

> 
> ...
> 
> > +struct audio_iio_aux_chan {
> > +	struct iio_channel *iio_chan;
> > +	const char *name;
> > +	bool is_invert_range;  
> 
> If you put bool after int:s it may save a few bytes in some cases.

I will mode is_invert_range after the int members.

> 
> > +	int max;
> > +	int min;  
> 
> Wondering if there is already a data type for the ranges (like linear_range.h,
> but not sure it's applicable here).

Seems not applicable here.
 - IIO does not use linear_range or something similar. It just uses simple int.
 - ASoC does not use linear_range or something similar. It just uses simple long.

So, I keep the simple int min and max.

> 
> > +};  
> 
> ...
> 
> > +	if (val < 0)
> > +		return -EINVAL;
> > +	if (val > max - min)  
> 
> Btw, who will validate that max > min?

By construction,
min = 0
max = iio_read_max_channel_raw() - iio_read_min_channel_raw()

and iio_read_max_channel_raw() returns a value greater or equal to
iio_read_min_channel_raw().

But to be sure, I will check the last asumption at probe() and swap
the minimum and maximum values if needed.

> 
> > +		return -EINVAL;  
> 
> ...
> 
> > +	return 1; /* The value changed */  
> 
> Perhaps this 1 needs a definition?

Yes but to be coherent, in ASoC code, many places need to be changed too
in order to use the newly defined value.
I don't think these modifications should be part of this series.

> 
> ...
> 
> > +static struct snd_soc_dapm_widget widgets[3] = {0};
> > +static struct snd_soc_dapm_route routes[2] = {0};  
> 
> 0:s are not needed. Moreover, the entire assingments are redundant
> as this is guaranteed by the C standard.

Indeed, the 0 assignment will be removed in the next iteration.

> 
> ...
> 
> > +	char *input_name = NULL;
> > +	char *output_name = NULL;
> > +	char *pga_name = NULL;  
> 
> Redundant assignments if you properly label the freeing.

I will rework the error paths (gotos) to avoid these assignement.

> 
> ...
> 
> > +	BUILD_BUG_ON(ARRAY_SIZE(widgets) < 3);  
> 
> Use static_assert() at the place where the array is defined.

Will be done in next iteration.

> 
> ...
> 
> > +	BUILD_BUG_ON(ARRAY_SIZE(routes) < 2);  
> 
> Ditto.
Will be done in next iteration.

> 
> ...
> 
> > +end:  
> 
> out_free:
> 
> > +	/* Allocated names are no more needed (duplicated in ASoC internals) */
> > +	kfree(pga_name);
> > +	kfree(output_name);
> > +	kfree(input_name);
> > +
> > +	return ret;  
> 
> ...
> 
> > +	for (i = 0; i < iio_aux->num_chans; i++) {
> > +		chan = iio_aux->chans + i;
> > +
> > +		ret = iio_read_max_channel_raw(chan->iio_chan, &chan->max);
> > +		if (ret) {
> > +			dev_err(component->dev, "chan[%d] %s: Cannot get max raw value (%d)\n",
> > +				i, chan->name, ret);
> > +			return ret;  
> 
> It sounds like a part of ->probe() flow, correct?
> Can dev_err_probe() be used here?

Will be changed in the next iteration.

> 
> > +		}
> > +
> > +		ret = iio_read_min_channel_raw(chan->iio_chan, &chan->min);
> > +		if (ret) {
> > +			dev_err(component->dev, "chan[%d] %s: Cannot get min raw value (%d)\n",
> > +				i, chan->name, ret);
> > +			return ret;  
> 
> Ditto.

Will be changed in the next iteration.

> 
> > +		}
> > +
> > +		/* Set initial value */
> > +		ret = iio_write_channel_raw(chan->iio_chan,
> > +					    chan->is_invert_range ? chan->max : chan->min);
> > +		if (ret) {
> > +			dev_err(component->dev, "chan[%d] %s: Cannot set initial value (%d)\n",
> > +				i, chan->name, ret);
> > +			return ret;  
> 
> Ditto.

Will be changed in the next iteration.

> 
> > +		}  
> 
> ...
> 
> > +		dev_dbg(component->dev, "chan[%d]: Added %s (min=%d, max=%d, invert=%s)\n",
> > +			i, chan->name, chan->min, chan->max,
> > +			chan->is_invert_range ? "on" : "off");  
> 
> str_on_off()

Indeed, I didn't know str_on_off().
Thanks for pointing.
Will be use in next iteration.

> 
> > +	}  
> 
> ...
> 
> > +	count = of_property_count_strings(np, "io-channel-names");
> > +	if (count < 0) {  
> 
> > +		dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names\n", np);
> > +		return count;  
> 
> 		return dev_err_probe();
Will be changed in next iteration.
> 
> > +	}  
> 
> ...
> 
> > +	for (i = 0; i < iio_aux->num_chans; i++) {
> > +		iio_aux_chan = iio_aux->chans + i;
> > +
> > +		ret = of_property_read_string_index(np, "io-channel-names", i,
> > +						    &iio_aux_chan->name);
> > +		if (ret < 0) {
> > +			dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names[%d]\n", np, i);
> > +			return ret;  
> 
> Ditto.
Will be changed in next iteration.
> 
> > +		}  
> 
> > +		tmp = 0;
> > +		of_property_read_u32_index(np, "snd-control-invert-range", i, &tmp);  
> 
> > +		iio_aux_chan->is_invert_range = tmp;  
> 
> You can use this variable directly.

Not sure, is_invert_range is a bool and tmp is a u32.

In previous iteration, I wrote
  iio_aux_chan->is_invert_range = !!tmp;

> 
> > +	}  
> 
> Btw, can you avoid using OF APIs? It's better to have device property/fwnode
> API to be used from day 1.

Hum, this comment was raised in the previous iteration
  https://lore.kernel.org/linux-kernel/20230501162456.3448c494@jic23-huawei/

I didn't find any equivalent to of_property_read_u32_index() in the 
device_property_read_*() function family.
I mean I did find anything available to get a value from an array using an index.

In the previous iteration it was concluded that keeping OF APIs in this series
seemed "reasonable".

> 
> ...
> 
> > +	platform_set_drvdata(pdev, iio_aux);  
> 
> Which callback is using this driver data?

None -> I will remove platform_set_drvdata().

> 
> ...
> 
> > +static const struct of_device_id audio_iio_aux_ids[] = {
> > +	{ .compatible = "audio-iio-aux", },  
> 
> Inner comma is not needed.

Will be fixed.

> 
> > +	{ }
> > +};  
> 
> ...
> 
> > +static struct platform_driver audio_iio_aux_driver = {
> > +	.driver = {
> > +		.name = "audio-iio-aux",
> > +		.of_match_table = audio_iio_aux_ids,
> > +	},
> > +	.probe = audio_iio_aux_probe,
> > +};  
> 
> > +  
> 
> Redundant blank line

Will be fixed.

> 
> > +module_platform_driver(audio_iio_aux_driver);  
> 



-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Andy Shevchenko]

On Tue, Jun 6, 2023 at 4:54 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Sat, 3 Jun 2023 21:26:19 +0300
> andy.shevchenko@gmail.com wrote:
> > Tue, May 23, 2023 at 05:12:21PM +0200, Herve Codina kirjoitti:

...

> > > +   int max;
> > > +   int min;
> >
> > Wondering if there is already a data type for the ranges (like linear_range.h,
> > but not sure it's applicable here).
>
> Seems not applicable here.
>  - IIO does not use linear_range or something similar. It just uses simple int.
>  - ASoC does not use linear_range or something similar. It just uses simple long.
>
> So, I keep the simple int min and max.

Sure.

...

> > > +   return 1; /* The value changed */
> >
> > Perhaps this 1 needs a definition?
>
> Yes but to be coherent, in ASoC code, many places need to be changed too
> in order to use the newly defined value.
> I don't think these modifications should be part of this series.

Yes, we are all for consistency.

...

> > > +   for (i = 0; i < iio_aux->num_chans; i++) {
> > > +           iio_aux_chan = iio_aux->chans + i;
> > > +
> > > +           ret = of_property_read_string_index(np, "io-channel-names", i,
> > > +                                               &iio_aux_chan->name);
> > > +           if (ret < 0) {
> > > +                   dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names[%d]\n", np, i);
> > > +                   return ret;
> >
> > Ditto.
> Will be changed in next iteration.
> >
> > > +           }
> >
> > > +           tmp = 0;
> > > +           of_property_read_u32_index(np, "snd-control-invert-range", i, &tmp);
> >
> > > +           iio_aux_chan->is_invert_range = tmp;
> >
> > You can use this variable directly.
>
> Not sure, is_invert_range is a bool and tmp is a u32.

Ah, I see.

> In previous iteration, I wrote
>   iio_aux_chan->is_invert_range = !!tmp;
>
> > > +   }
> >
> > Btw, can you avoid using OF APIs? It's better to have device property/fwnode
> > API to be used from day 1.
>
> Hum, this comment was raised in the previous iteration
>   https://lore.kernel.org/linux-kernel/20230501162456.3448c494@jic23-huawei/
>
> I didn't find any equivalent to of_property_read_u32_index() in the
> device_property_read_*() function family.
> I mean I did find anything available to get a value from an array using an index.

This is done by reading the entire array at once and then parsing as
you wish in the code, device_property_read_u32_array() is for that.

> In the previous iteration it was concluded that keeping OF APIs in this series
> seemed "reasonable".

Maybe, but consider the above.

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Herve Codina]

Hi Andy,

On Tue, 6 Jun 2023 15:54:04 +0200
Herve Codina <herve.codina@bootlin.com> wrote:

...
> >   
> > > +	platform_set_drvdata(pdev, iio_aux);    
> > 
> > Which callback is using this driver data?  
> 
> None -> I will remove platform_set_drvdata().
> 

My previous answer was not correct.
The platform_set_drvdata() call is needed.

In fact, the driver uses snd_soc_component_get_drvdata() 
  https://elixir.bootlin.com/linux/v6.4-rc5/source/include/sound/soc-component.h#L425
and this snd_soc_component_get_drvdata() get the driver data set by the
platform_set_drvdata() call.

I cannot use snd_soc_component_set_drvdata() to set the driver data because
I haven't got the struct snd_soc_component instance when I need to set the
driver data.

So, I will not remove the platform_set_drvdata() call.

The sequence is:
  --- 8< ---
  static int audio_iio_aux_probe(struct platform_device *pdev)
  {
	struct audio_iio_aux *iio_aux;

	iio_aux = devm_kzalloc(&pdev->dev, sizeof(*iio_aux), GFP_KERNEL);
	if (!iio_aux)
		return -ENOMEM;

	...

	platform_set_drvdata(pdev, iio_aux);

	return devm_snd_soc_register_component(iio_aux->dev,
					       &audio_iio_aux_component_driver,
					       NULL, 0);
  }
  --- 8< ---

The struct snd_soc_component instance will be create during the 
devm_snd_soc_register_component() call.

Regards,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Herve Codina]

Hi Andy,

On Tue, 6 Jun 2023 17:34:22 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:

...

> > >
> > > Btw, can you avoid using OF APIs? It's better to have device property/fwnode
> > > API to be used from day 1.  
> >
> > Hum, this comment was raised in the previous iteration
> >   https://lore.kernel.org/linux-kernel/20230501162456.3448c494@jic23-huawei/
> >
> > I didn't find any equivalent to of_property_read_u32_index() in the
> > device_property_read_*() function family.
> > I mean I did find anything available to get a value from an array using an index.  
> 
> This is done by reading the entire array at once and then parsing as
> you wish in the code, device_property_read_u32_array() is for that.
> 
> > In the previous iteration it was concluded that keeping OF APIs in this series
> > seemed "reasonable".  
> 
> Maybe, but consider the above.

I see.
Will switch to device_property_*() family in the next iteration.

Thanks,
Hervé

-- 
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com