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

On Fri, 21 Apr 2023 14:41:22 +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

Hi Herve,

There are some other IIO devices that might turn up in audio paths. In theory someone might put an IIO supported amplifier in there (though current ones are far to high frequency and expensive for that to make sense). For now it probably makes sense to support potentiometers as you are doing here, though I'm guessing that in many cases they would be used with some other analog components. Does the transfer function matter at all?

Been many years since I last touched anything in ASoC so questions may be silly ;)

A few comments inline.

Jonathan

+static int simple_iio_aux_get_volsw(struct snd_kcontrol *kcontrol,

• 		    struct snd_ctl_elem_value *ucontrol)
  

+{

• struct simple_iio_aux_chan *chan = (struct simple_iio_aux_chan *)kcontrol->private_value;
• int max = chan->max;
• int min = chan->min;
• unsigned int mask = (1 << fls(max)) - 1;

As below. I'm not following reason for use of mask

• unsigned int invert = chan->is_inverted;
• int ret;
• int val;
• ret = iio_read_channel_raw(chan->iio_chan, &val);
• if (ret < 0)

• return ret;
  

• ucontrol->value.integer.value[0] = (val & mask) - min;
• if (invert)

• ucontrol->value.integer.value[0] = max - ucontrol->value.integer.value[0];
  

• return 0;

+}

+static int simple_iio_aux_put_volsw(struct snd_kcontrol *kcontrol,

• 		    struct snd_ctl_elem_value *ucontrol)
  

+{

• struct simple_iio_aux_chan *chan = (struct simple_iio_aux_chan *)kcontrol->private_value;
• int max = chan->max;
• int min = chan->min;
• unsigned int mask = (1 << fls(max)) - 1;

Why is mask needed? Also seems like handling is making some strong assumptions on form of max and min. So at minimum some comments on reasoning needed.

• unsigned int invert = chan->is_inverted;
• 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) & mask;
• if (invert)

• 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 simple_iio_aux_probe(struct platform_device *pdev) +{

• struct device_node *np = pdev->dev.of_node;
• struct simple_iio_aux_chan *iio_aux_chan;
• struct simple_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);
  

Whilst today this will be tightly couple with of, if you can use generic firmware handling where possible (from linux/property.h) it will reduce what needs to be tidied up if anyone fills in the gaps for IIO consumer bindings in ACPI and then someone uses PRP0001 based ACPI bindings.

• if (ret < 0) {
  

• 	dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names[%d]\n", np, i);
  

dev_err_probe() would simplify these cases a little. Not sure on ASOC view on using that for cases that won't defer. I tend to take the view it's nicer everywhere for calls in probe() functions.

• 	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)) {
  

• 	ret = PTR_ERR(iio_aux_chan->iio_chan);
  

Put that inline instead of setting ret here.

• 	return dev_err_probe(iio_aux->dev, ret,
  

• 			     "get IIO channel '%s' failed (%d)\n",
  

• 			     iio_aux_chan->name, ret);
  

• }
  

• tmp = 0;
  

• of_property_read_u32_index(np, "invert", i, &tmp);
  

• iio_aux_chan->is_inverted = !!tmp;
  

As it's a bool this is the same as iio_aux_chan->is_inverted = tmp;

• }
• platform_set_drvdata(pdev, iio_aux);
• return devm_snd_soc_register_component(iio_aux->dev,

• 			       &simple_iio_aux_component_driver,
  

• 			       NULL, 0);
  

+}

+#ifdef CONFIG_OF +static const struct of_device_id simple_iio_aux_ids[] = {

• { .compatible = "simple-iio-aux", },
• { }

+}; +MODULE_DEVICE_TABLE(of, simple_iio_aux_ids); +#endif

+static struct platform_driver simple_iio_aux_driver = {

• .driver = {

• .name = "simple-iio-aux",
  

• .of_match_table = of_match_ptr(simple_iio_aux_ids),
  

I'd just drop the of_match_ptr() Whilst this won't work today with other firmwares, we might enable the missing parts at some stage. Also the driver is somewhat pointless without DT so I'd just assume it's always built with it. Cost is a tiny array on systems with a weird .config

• },
• .probe = simple_iio_aux_probe,

+};

+module_platform_driver(simple_iio_aux_driver);

+MODULE_AUTHOR("Herve Codina herve.codina@bootlin.com"); +MODULE_DESCRIPTION("IIO ALSA SoC aux driver"); +MODULE_LICENSE("GPL");