Re: [PATCH v3 11/12] ASoC: codecs: Add support for the generic IIO auxiliary devices

[Andy Shevchenko]

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