| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZHuFywIrTnEFpX6e@surfacebook>
Date: Sat, 3 Jun 2023 21:26:19 +0300
From: andy.shevchenko@...il.com
To: Herve Codina <herve.codina@...tlin.com>
Cc: Liam Girdwood <lgirdwood@...il.com>,
Mark Brown <broonie@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Jonathan Cameron <jic23@...nel.org>,
Lars-Peter Clausen <lars@...afoo.de>,
Jaroslav Kysela <perex@...ex.cz>,
Takashi Iwai <tiwai@...e.com>,
Kuninori Morimoto <kuninori.morimoto.gx@...esas.com>,
alsa-devel@...a-project.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
Christophe Leroy <christophe.leroy@...roup.eu>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>
Subject: Re: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO
auxiliary devices
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);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists