Hi Jonathan, Mark,
On Sat, 22 Apr 2023 18:08:14 +0100
Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On Fri, 21 Apr 2023 14:41:22 +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 auxliary
> > audio devices and allows to control them using mixer controls.
> >
> > Signed-off-by: Herve Codina <herve.codina@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>");
> > +MODULE_DESCRIPTION("IIO ALSA SoC aux driver");
> > +MODULE_LICENSE("GPL");
>
Thanks for the review.
Best regards,
Hervé
