Hi Jonathan, Mark,
On Sat, 22 Apr 2023 18:08:14 +0100 Jonathan Cameron jic23@kernel.org wrote:
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.
This mask was present in the internal ASoC helpers used when devices can be accessed using regmap. The IIO accesses done by simple_iio_aux_get_volsw() and simple_iio_aux_put_volsw() were based on these internal helpers. Not sure about the exact reason to this mask. Maybe Mark can answer.
For these particular use-cases using an IIO channel, the mask present in simple_iio_aux_get_volsw() and simple_iio_aux_put_volsw() can be removed.
I will remove in the next iteration except if Mark tell me to keep them.
- 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.
No device_property_read_*() function family are available to get a value from an array using an index.
I would prefer to keep the of_property_read_*() function family I use for this first IIO auxiliary device support.
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.
I have the feeling that ASoC uses dev_err_probe() for cases that can defer. Mark, can you confirm ?
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.
Will be done in the next iteration.
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;
I will remove the '!!' construction.
- }
- 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
of_match_ptr will be removed (and the #ifdef CONFIG_OF also).
- },
- .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");
Thanks for the review.
Best regards, Hervé