On Mon, Jun 12, 2023 at 3:30 PM 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
auxiliary
audio devices and allows to control them using mixer controls.
allows one to control?
...
+#include <linux/iio/consumer.h> +#include <linux/minmax.h> +#include <linux/mod_devicetable.h> +#include <linux/platform_device.h> +#include <linux/slab.h>
+#include <sound/soc.h> +#include <linux/string_helpers.h>
Perhaps a bit of order? And maybe a blank line between linux/* and sound/*?
+#include <sound/tlv.h>
...
struct snd_kcontrol_new control = {0};
0 is not needed.
...
+static int audio_iio_aux_add_dapms(struct snd_soc_component *component,
struct audio_iio_aux_chan *chan)+{
struct snd_soc_dapm_context *dapm = snd_soc_component_get_dapm(component);
char *input_name = NULL;char *output_name = NULL;char *pga_name = NULL;
Now these assignments can be dropped.
int ret;input_name = kasprintf(GFP_KERNEL, "%s IN", chan->name);if (!input_name) {ret = -ENOMEM;goto out;}output_name = kasprintf(GFP_KERNEL, "%s OUT", chan->name);if (!output_name) {ret = -ENOMEM;goto out_free_input_name;}pga_name = kasprintf(GFP_KERNEL, "%s PGA", chan->name);if (!pga_name) {ret = -ENOMEM;goto out_free_output_name;}widgets[0] = SND_SOC_DAPM_INPUT(input_name);widgets[1] = SND_SOC_DAPM_OUTPUT(output_name);widgets[2] = SND_SOC_DAPM_PGA(pga_name, SND_SOC_NOPM, 0, 0, NULL, 0);ret = snd_soc_dapm_new_controls(dapm, widgets, 3);if (ret)goto out_free_pga_name;routes[0].sink = pga_name;routes[0].control = NULL;routes[0].source = input_name;routes[1].sink = output_name;routes[1].control = NULL;routes[1].source = pga_name;ret = snd_soc_dapm_add_routes(dapm, routes, 2);/* Allocated names are no more needed (duplicated in ASoC internals) */+out_free_pga_name:
kfree(pga_name);+out_free_output_name:
kfree(output_name);+out_free_input_name:
kfree(input_name);
+out:
Seems redundant label, you can return directly.
return ret;+}
...
With
struct device *dev = &pdev->dev;
iio_aux = devm_kzalloc(&pdev->dev, sizeof(*iio_aux), GFP_KERNEL);if (!iio_aux)return -ENOMEM;
You can make this kind of call neater.
...
iio_aux->dev = &pdev->dev;count = device_property_string_array_count(iio_aux->dev, "io-channel-names");
It's not recommended to switch over inside one function to a new pointer even if they are the same. With dev here as a parameters it's much easier to understand where the property is taken from.
if (count < 0)return dev_err_probe(iio_aux->dev, count, "failed to count io-channel-names\n");
Ditto. And for the rest.
...
iio_aux->chans = devm_kmalloc_array(iio_aux->dev, iio_aux->num_chans,
Esp. in this case, it will add confusion, because we have been having the object lifetime issues with devm_*() APIs from the past and then...
sizeof(*iio_aux->chans), GFP_KERNEL);if (!iio_aux->chans)return -ENOMEM;
...
/** snd-control-invert-range is optional and can contain fewer items* than the number of channel. Unset values default to 0.
channels
*/count = device_property_count_u32(iio_aux->dev, "snd-control-invert-range");if (count > 0) {count = min_t(unsigned int, count, iio_aux->num_chans);device_property_read_u32_array(iio_aux->dev, "snd-control-invert-range",invert_ranges, count);
Probably good to check an error, while it might be almost a dead code. If something happens during this we will at least know.
}