On Thu, 15 Jun 2023 17:26:30 +0200
Herve Codina <herve.codina@xxxxxxxxxxx> 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
> auxiliary audio devices and allows one to control them using mixer
> controls.
>
> Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>
A few trivial things inline.
With those tidied up, (for the IIO bits and general code - but I don't know
the snd part well enough to review that).
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> index 000000000000..b9d72cbb85f2
> --- /dev/null
> +++ b/sound/soc/codecs/audio-iio-aux.c
> @@ -0,0 +1,338 @@
...
> +static int audio_iio_aux_add_controls(struct snd_soc_component *component,
> + struct audio_iio_aux_chan *chan)
> +{
> + struct snd_kcontrol_new control = {};
Why not:
struct snd_kcontrol_new control = {
.iface = SNDRV_CTL_ELEM_IFACE_MIXER;
.name = chan->name;
.info = audio_iio_aux_info_volsw;
.get = audio_iio_aux_get_volsw;
.put = audio_iio_aux_put_volsw;
.private_value = (unsigned long)chan;
};
> +
> + 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 *output_name;
> + char *input_name;
> + char *pga_name;
> + int ret;
> +
> + input_name = kasprintf(GFP_KERNEL, "%s IN", chan->name);
> + if (!input_name)
> + return -ENOMEM;
> +
> + output_name = kasprintf(GFP_KERNEL, "%s OUT", chan->name);
> + if (!output_name) {
> + ret = -ENOMEM;
> + goto out_free_input_name;
> + }
Trivial but a blank line here would be nice.
> + 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);
> + 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);
Why? I'd like a comment here on what circumstances could cause this to happen.
> + 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;
> +}
