On Mon, Jun 12, 2023 at 3:30 PM Herve Codina <herve.codina@xxxxxxxxxxx> 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.
> + }
--
With Best Regards,
Andy Shevchenko
