lkml.org 
[lkml]   [2023]   [Apr]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 4/4] ASoC: codecs: Add support for the generic IIO auxiliary devices
    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é

    \
     
     \ /
      Last update: 2023-04-24 12:53    [W:6.372 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site