[v3,11/12] ASoC: codecs: Add support for the generic IIO auxiliary devices
Commit Message
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 | 334 +++++++++++++++++++++++++++++++
3 files changed, 348 insertions(+)
create mode 100644 sound/soc/codecs/audio-iio-aux.c
Comments
On Mon, Jun 12, 2023 at 3:30 PM 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
auxiliary
> audio devices and allows to control them using mixer controls.
allows one to control?
...
> +#include <linux/iio/consumer.h>
> +#include <linux/minmax.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <sound/soc.h>
> +#include <linux/string_helpers.h>
Perhaps a bit of order? And maybe a blank line between linux/* and sound/*?
> +#include <sound/tlv.h>
...
> + struct snd_kcontrol_new control = {0};
0 is not needed.
...
> +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;
Now these assignments can be dropped.
> + int ret;
> +
> + input_name = kasprintf(GFP_KERNEL, "%s IN", chan->name);
> + if (!input_name) {
> + ret = -ENOMEM;
> + goto out;
> + }
> + output_name = kasprintf(GFP_KERNEL, "%s OUT", chan->name);
> + if (!output_name) {
> + ret = -ENOMEM;
> + goto out_free_input_name;
> + }
> + pga_name = kasprintf(GFP_KERNEL, "%s PGA", chan->name);
> + if (!pga_name) {
> + ret = -ENOMEM;
> + goto out_free_output_name;
> + }
> +
> + widgets[0] = SND_SOC_DAPM_INPUT(input_name);
> + widgets[1] = SND_SOC_DAPM_OUTPUT(output_name);
> + widgets[2] = 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 out_free_pga_name;
> +
> + 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);
> +
> + /* Allocated names are no more needed (duplicated in ASoC internals) */
> +
> +out_free_pga_name:
> + kfree(pga_name);
> +out_free_output_name:
> + kfree(output_name);
> +out_free_input_name:
> + kfree(input_name);
> +out:
Seems redundant label, you can return directly.
> + return ret;
> +}
...
With
struct device *dev = &pdev->dev;
> + iio_aux = devm_kzalloc(&pdev->dev, sizeof(*iio_aux), GFP_KERNEL);
> + if (!iio_aux)
> + return -ENOMEM;
You can make this kind of call neater.
...
> + iio_aux->dev = &pdev->dev;
> +
> + count = device_property_string_array_count(iio_aux->dev, "io-channel-names");
It's not recommended to switch over inside one function to a new
pointer even if they are the same. With dev here as a parameters it's
much easier to understand where the property is taken from.
> + if (count < 0)
> + return dev_err_probe(iio_aux->dev, count, "failed to count io-channel-names\n");
Ditto. And for the rest.
...
> + iio_aux->chans = devm_kmalloc_array(iio_aux->dev, iio_aux->num_chans,
Esp. in this case, it will add confusion, because we have been having
the object lifetime issues with devm_*() APIs from the past and
then...
> + sizeof(*iio_aux->chans), GFP_KERNEL);
> + if (!iio_aux->chans)
> + return -ENOMEM;
...
> + /*
> + * snd-control-invert-range is optional and can contain fewer items
> + * than the number of channel. Unset values default to 0.
channels
> + */
> + count = device_property_count_u32(iio_aux->dev, "snd-control-invert-range");
> + if (count > 0) {
> + count = min_t(unsigned int, count, iio_aux->num_chans);
> + device_property_read_u32_array(iio_aux->dev, "snd-control-invert-range",
> + invert_ranges, count);
Probably good to check an error, while it might be almost a dead code.
If something happens during this we will at least know.
> + }
Hi Andy,
On Mon, 12 Jun 2023 17:37:00 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> ...
>
> > + struct snd_kcontrol_new control = {0};
>
> 0 is not needed.
Not for this one.
The variable is in stack.
Some of the structure members will be set in the code but we need to ensure
that all others are set to 0.
The full context:
--- 8< ---
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);
}
--- 8< ---
Thanks for the review,
Hervé
On Tue, Jun 13, 2023 at 12:37 PM Herve Codina <herve.codina@bootlin.com> wrote:
> On Mon, 12 Jun 2023 17:37:00 +0300
> Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
...
> > > + struct snd_kcontrol_new control = {0};
> >
> > 0 is not needed.
>
> Not for this one.
>
> The variable is in stack.
> Some of the structure members will be set in the code but we need to ensure
> that all others are set to 0.
Yes, and as I said, 0 is not needed. Compiler assumes that if you just
use plain {}.
On Tue, 13 Jun 2023 16:24:58 +0300
Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> On Tue, Jun 13, 2023 at 12:37 PM Herve Codina <herve.codina@bootlin.com> wrote:
> > On Mon, 12 Jun 2023 17:37:00 +0300
> > Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> ...
>
> > > > + struct snd_kcontrol_new control = {0};
> > >
> > > 0 is not needed.
> >
> > Not for this one.
> >
> > The variable is in stack.
> > Some of the structure members will be set in the code but we need to ensure
> > that all others are set to 0.
>
> Yes, and as I said, 0 is not needed. Compiler assumes that if you just
> use plain {}.
>
My bad, sorry, I misunderstood.
Will be update to an empty {} in the next iteration.
Thanks,
Hervé
@@ -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
@@ -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
new file mode 100644
@@ -0,0 +1,334 @@
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// 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/minmax.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <sound/soc.h>
+#include <linux/string_helpers.h>
+#include <sound/tlv.h>
+
+struct audio_iio_aux_chan {
+ struct iio_channel *iio_chan;
+ const char *name;
+ int max;
+ int min;
+ bool is_invert_range;
+};
+
+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];
+static struct snd_soc_dapm_route routes[2];
+
+/* Be sure sizes are correct (need 3 widgets and 2 routes) */
+static_assert(ARRAY_SIZE(widgets) >= 3, "3 widgets are needed");
+static_assert(ARRAY_SIZE(routes) >= 2, "2 routes are needed");
+
+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 out;
+ }
+ output_name = kasprintf(GFP_KERNEL, "%s OUT", chan->name);
+ if (!output_name) {
+ ret = -ENOMEM;
+ goto out_free_input_name;
+ }
+ pga_name = kasprintf(GFP_KERNEL, "%s PGA", chan->name);
+ if (!pga_name) {
+ ret = -ENOMEM;
+ goto out_free_output_name;
+ }
+
+ widgets[0] = SND_SOC_DAPM_INPUT(input_name);
+ widgets[1] = SND_SOC_DAPM_OUTPUT(output_name);
+ widgets[2] = 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 out_free_pga_name;
+
+ 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);
+
+ /* Allocated names are no more needed (duplicated in ASoC internals) */
+
+out_free_pga_name:
+ kfree(pga_name);
+out_free_output_name:
+ kfree(output_name);
+out_free_input_name:
+ kfree(input_name);
+out:
+ 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)
+ return dev_err_probe(component->dev, ret,
+ "chan[%d] %s: Cannot get max raw value\n",
+ i, chan->name);
+
+ ret = iio_read_min_channel_raw(chan->iio_chan, &chan->min);
+ if (ret)
+ return dev_err_probe(component->dev, ret,
+ "chan[%d] %s: Cannot get min raw value\n",
+ i, chan->name);
+
+ if (chan->min > chan->max) {
+ dev_dbg(component->dev, "chan[%d] %s: Swap min and max\n",
+ i, chan->name);
+ swap(chan->min, chan->max);
+ }
+
+ /* Set initial value */
+ ret = iio_write_channel_raw(chan->iio_chan,
+ chan->is_invert_range ? chan->max : chan->min);
+ if (ret)
+ return dev_err_probe(component->dev, ret,
+ "chan[%d] %s: Cannot set initial value\n",
+ i, chan->name);
+
+ 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,
+ str_on_off(chan->is_invert_range));
+ }
+
+ 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 audio_iio_aux_chan *iio_aux_chan;
+ struct audio_iio_aux *iio_aux;
+ const char **names;
+ u32 *invert_ranges;
+ int count;
+ 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 = device_property_string_array_count(iio_aux->dev, "io-channel-names");
+ if (count < 0)
+ return dev_err_probe(iio_aux->dev, count, "failed to count io-channel-names\n");
+
+ iio_aux->num_chans = count;
+
+ iio_aux->chans = devm_kmalloc_array(iio_aux->dev, iio_aux->num_chans,
+ sizeof(*iio_aux->chans), GFP_KERNEL);
+ if (!iio_aux->chans)
+ return -ENOMEM;
+
+ names = kcalloc(iio_aux->num_chans, sizeof(*names), GFP_KERNEL);
+ if (!names)
+ return -ENOMEM;
+
+ invert_ranges = kcalloc(iio_aux->num_chans, sizeof(*invert_ranges), GFP_KERNEL);
+ if (!invert_ranges) {
+ ret = -ENOMEM;
+ goto out_free_names;
+ }
+
+ ret = device_property_read_string_array(iio_aux->dev, "io-channel-names",
+ names, iio_aux->num_chans);
+ if (ret < 0) {
+ dev_err_probe(iio_aux->dev, ret, "failed to read io-channel-names\n");
+ goto out_free_invert_ranges;
+ }
+
+ /*
+ * snd-control-invert-range is optional and can contain fewer items
+ * than the number of channel. Unset values default to 0.
+ */
+ count = device_property_count_u32(iio_aux->dev, "snd-control-invert-range");
+ if (count > 0) {
+ count = min_t(unsigned int, count, iio_aux->num_chans);
+ device_property_read_u32_array(iio_aux->dev, "snd-control-invert-range",
+ invert_ranges, count);
+ }
+
+ for (i = 0; i < iio_aux->num_chans; i++) {
+ iio_aux_chan = iio_aux->chans + i;
+ iio_aux_chan->name = names[i];
+ iio_aux_chan->is_invert_range = invert_ranges[i];
+
+ iio_aux_chan->iio_chan = devm_iio_channel_get(iio_aux->dev, iio_aux_chan->name);
+ if (IS_ERR(iio_aux_chan->iio_chan)) {
+ ret = PTR_ERR(iio_aux_chan->iio_chan);
+ dev_err_probe(iio_aux->dev, ret, "get IIO channel '%s' failed\n",
+ iio_aux_chan->name);
+ goto out_free_invert_ranges;
+ }
+ }
+
+ platform_set_drvdata(pdev, iio_aux);
+
+ ret = devm_snd_soc_register_component(iio_aux->dev, &audio_iio_aux_component_driver,
+ NULL, 0);
+out_free_invert_ranges:
+ kfree(invert_ranges);
+out_free_names:
+ kfree(names);
+ return ret;
+}
+
+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");