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.
...
+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.
- 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).
+};
...
- if (val < 0)
return -EINVAL;- if (val > max - min)
Btw, who will validate that max > min?
return -EINVAL;
...
- return 1; /* The value changed */
Perhaps this 1 needs a definition?
...
+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.
...
- char *input_name = NULL;
- char *output_name = NULL;
- char *pga_name = NULL;
Redundant assignments if you properly label the freeing.
...
- BUILD_BUG_ON(ARRAY_SIZE(widgets) < 3);
Use static_assert() at the place where the array is defined.
...
- BUILD_BUG_ON(ARRAY_SIZE(routes) < 2);
Ditto.
...
+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?
}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.
}/* 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.
}
...
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()
- }
...
- 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();
- }
...
- 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.
}
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.
- }
Btw, can you avoid using OF APIs? It's better to have device property/fwnode API to be used from day 1.
...
- platform_set_drvdata(pdev, iio_aux);
Which callback is using this driver data?
...
+static const struct of_device_id audio_iio_aux_ids[] = {
- { .compatible = "audio-iio-aux", },
Inner comma is not needed.
- { }
+};
...
+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
+module_platform_driver(audio_iio_aux_driver);