Hi Andy,
On Sat, 3 Jun 2023 21:26:19 +0300 andy.shevchenko@gmail.com 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);