Messages in this thread | | | From | andy.shevchenko@gmail ... | Date | Sat, 3 Jun 2023 21:26:19 +0300 | 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
| |