Hi Andy,
On Sat, 3 Jun 2023 21:26:19 +0300
andy.shevchenko@xxxxxxxxx wrote:
> Tue, May 23, 2023 at 05:12:21PM +0200, Herve Codina kirjoitti:
> > 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.
>
> ...
>
> > +// audio-iio-aux.c -- ALSA SoC glue to use IIO devices as audio components
>
> Putting file name into file is not a good idea in case the file will be renamed
> in the future.
Indeed, the file name will be removed in the nest iteration.
>
> ...
>
> > +struct audio_iio_aux_chan {
> > + struct iio_channel *iio_chan;
> > + const char *name;
> > + bool is_invert_range;
>
> If you put bool after int:s it may save a few bytes in some cases.
I will mode is_invert_range after the int members.
>
> > + int max;
> > + int min;
>
> Wondering if there is already a data type for the ranges (like linear_range.h,
> but not sure it's applicable here).
Seems not applicable here.
- IIO does not use linear_range or something similar. It just uses simple int.
- ASoC does not use linear_range or something similar. It just uses simple long.
So, I keep the simple int min and max.
>
> > +};
>
> ...
>
> > + if (val < 0)
> > + return -EINVAL;
> > + if (val > max - min)
>
> Btw, who will validate that max > min?
By construction,
min = 0
max = iio_read_max_channel_raw() - iio_read_min_channel_raw()
and iio_read_max_channel_raw() returns a value greater or equal to
iio_read_min_channel_raw().
But to be sure, I will check the last asumption at probe() and swap
the minimum and maximum values if needed.
>
> > + return -EINVAL;
>
> ...
>
> > + return 1; /* The value changed */
>
> Perhaps this 1 needs a definition?
Yes but to be coherent, in ASoC code, many places need to be changed too
in order to use the newly defined value.
I don't think these modifications should be part of this series.
>
> ...
>
> > +static struct snd_soc_dapm_widget widgets[3] = {0};
> > +static struct snd_soc_dapm_route routes[2] = {0};
>
> 0:s are not needed. Moreover, the entire assingments are redundant
> as this is guaranteed by the C standard.
Indeed, the 0 assignment will be removed in the next iteration.
>
> ...
>
> > + char *input_name = NULL;
> > + char *output_name = NULL;
> > + char *pga_name = NULL;
>
> Redundant assignments if you properly label the freeing.
I will rework the error paths (gotos) to avoid these assignement.
>
> ...
>
> > + BUILD_BUG_ON(ARRAY_SIZE(widgets) < 3);
>
> Use static_assert() at the place where the array is defined.
Will be done in next iteration.
>
> ...
>
> > + BUILD_BUG_ON(ARRAY_SIZE(routes) < 2);
>
> Ditto.
Will be done in next iteration.
>
> ...
>
> > +end:
>
> out_free:
>
> > + /* Allocated names are no more needed (duplicated in ASoC internals) */
> > + kfree(pga_name);
> > + kfree(output_name);
> > + kfree(input_name);
> > +
> > + return ret;
>
> ...
>
> > + for (i = 0; i < iio_aux->num_chans; i++) {
> > + chan = iio_aux->chans + i;
> > +
> > + ret = iio_read_max_channel_raw(chan->iio_chan, &chan->max);
> > + if (ret) {
> > + dev_err(component->dev, "chan[%d] %s: Cannot get max raw value (%d)\n",
> > + i, chan->name, ret);
> > + return ret;
>
> It sounds like a part of ->probe() flow, correct?
> Can dev_err_probe() be used here?
Will be changed in the next iteration.
>
> > + }
> > +
> > + ret = iio_read_min_channel_raw(chan->iio_chan, &chan->min);
> > + if (ret) {
> > + dev_err(component->dev, "chan[%d] %s: Cannot get min raw value (%d)\n",
> > + i, chan->name, ret);
> > + return ret;
>
> Ditto.
Will be changed in the next iteration.
>
> > + }
> > +
> > + /* Set initial value */
> > + ret = iio_write_channel_raw(chan->iio_chan,
> > + chan->is_invert_range ? chan->max : chan->min);
> > + if (ret) {
> > + dev_err(component->dev, "chan[%d] %s: Cannot set initial value (%d)\n",
> > + i, chan->name, ret);
> > + return ret;
>
> Ditto.
Will be changed in the next iteration.
>
> > + }
>
> ...
>
> > + dev_dbg(component->dev, "chan[%d]: Added %s (min=%d, max=%d, invert=%s)\n",
> > + i, chan->name, chan->min, chan->max,
> > + chan->is_invert_range ? "on" : "off");
>
> str_on_off()
Indeed, I didn't know str_on_off().
Thanks for pointing.
Will be use in next iteration.
>
> > + }
>
> ...
>
> > + 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;
>
> return dev_err_probe();
Will be changed in next iteration.
>
> > + }
>
> ...
>
> > + 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);
> > + if (ret < 0) {
> > + dev_err(iio_aux->dev, "%pOF: failed to read io-channel-names[%d]\n", np, i);
> > + return ret;
>
> Ditto.
Will be changed in next iteration.
>
> > + }
>
> > + tmp = 0;
> > + of_property_read_u32_index(np, "snd-control-invert-range", i, &tmp);
>
> > + iio_aux_chan->is_invert_range = tmp;
>
> You can use this variable directly.
Not sure, is_invert_range is a bool and tmp is a u32.
In previous iteration, I wrote
iio_aux_chan->is_invert_range = !!tmp;
>
> > + }
>
> Btw, can you avoid using OF APIs? It's better to have device property/fwnode
> API to be used from day 1.
Hum, this comment was raised in the previous iteration
https://lore.kernel.org/linux-kernel/20230501162456.3448c494@jic23-huawei/
I didn't find any equivalent to of_property_read_u32_index() in the
device_property_read_*() function family.
I mean I did find anything available to get a value from an array using an index.
In the previous iteration it was concluded that keeping OF APIs in this series
seemed "reasonable".
>
> ...
>
> > + platform_set_drvdata(pdev, iio_aux);
>
> Which callback is using this driver data?
None -> I will remove platform_set_drvdata().
>
> ...
>
> > +static const struct of_device_id audio_iio_aux_ids[] = {
> > + { .compatible = "audio-iio-aux", },
>
> Inner comma is not needed.
Will be fixed.
>
> > + { }
> > +};
>
> ...
>
> > +static struct platform_driver audio_iio_aux_driver = {
> > + .driver = {
> > + .name = "audio-iio-aux",
> > + .of_match_table = audio_iio_aux_ids,
> > + },
> > + .probe = audio_iio_aux_probe,
> > +};
>
> > +
>
> Redundant blank line
Will be fixed.
>
> > +module_platform_driver(audio_iio_aux_driver);
>
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
