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

[Herve Codina <herve.codina@bootlin.com>]

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é