Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230612122926.107333-1-herve.codina@bootlin.com> <20230612122926.107333-12-herve.codina@bootlin.com>
In-Reply-To: <20230612122926.107333-12-herve.codina@bootlin.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Mon, 12 Jun 2023 17:37:00 +0300
Message-ID: <CAHp75Vfz+vSJ7rQ4sxb0R+zRbrjipXUX3VpaHyvOWWePdPxejQ@mail.gmail.com>
Subject: Re: [PATCH v3 11/12] ASoC: codecs: Add support for the generic IIO
 auxiliary devices
To:     Herve Codina <herve.codina@bootlin.com>
Cc:     Liam Girdwood <lgirdwood@gmail.com>,
        Mark Brown <broonie@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Jaroslav Kysela <perex@perex.cz>,
        Takashi Iwai <tiwai@suse.com>,
        Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>,
        alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Jun 12, 2023 at 3:30=E2=80=AFPM 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 =3D {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 =3D snd_soc_component_get_dapm(=
component);

> +       char *input_name =3D NULL;
> +       char *output_name =3D NULL;
> +       char *pga_name =3D NULL;

Now these assignments can be dropped.

> +       int ret;
> +
> +       input_name =3D kasprintf(GFP_KERNEL, "%s IN", chan->name);
> +       if (!input_name) {
> +               ret =3D -ENOMEM;
> +               goto out;
> +       }
> +       output_name =3D kasprintf(GFP_KERNEL, "%s OUT", chan->name);
> +       if (!output_name) {
> +               ret =3D -ENOMEM;
> +               goto out_free_input_name;
> +       }
> +       pga_name =3D kasprintf(GFP_KERNEL, "%s PGA", chan->name);
> +       if (!pga_name) {
> +               ret =3D -ENOMEM;
> +               goto out_free_output_name;
> +       }
> +
> +       widgets[0] =3D SND_SOC_DAPM_INPUT(input_name);
> +       widgets[1] =3D SND_SOC_DAPM_OUTPUT(output_name);
> +       widgets[2] =3D SND_SOC_DAPM_PGA(pga_name, SND_SOC_NOPM, 0, 0, NUL=
L, 0);
> +       ret =3D snd_soc_dapm_new_controls(dapm, widgets, 3);
> +       if (ret)
> +               goto out_free_pga_name;
> +
> +       routes[0].sink =3D pga_name;
> +       routes[0].control =3D NULL;
> +       routes[0].source =3D input_name;
> +       routes[1].sink =3D output_name;
> +       routes[1].control =3D NULL;
> +       routes[1].source =3D pga_name;
> +       ret =3D snd_soc_dapm_add_routes(dapm, routes, 2);
> +
> +       /* Allocated names are no more needed (duplicated in ASoC interna=
ls) */
> +
> +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 =3D &pdev->dev;

> +       iio_aux =3D 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 =3D &pdev->dev;
> +
> +       count =3D device_property_string_array_count(iio_aux->dev, "io-ch=
annel-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 coun=
t io-channel-names\n");

Ditto. And for the rest.

...

> +       iio_aux->chans =3D 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 ite=
ms
> +        * than the number of channel. Unset values default to 0.

channels

> +        */
> +       count =3D device_property_count_u32(iio_aux->dev, "snd-control-in=
vert-range");
> +       if (count > 0) {
> +               count =3D 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.

> +       }

--=20
With Best Regards,
Andy Shevchenko