lkml.org 
[lkml]   [2023]   [Jun]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v2 7/9] ASoC: codecs: Add support for the generic IIO auxiliary devices
    Hi Andy,

    On Sat, 3 Jun 2023 21:26:19 +0300
    andy.shevchenko@gmail.com wrote:

    > 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.

    Indeed, the file name will be removed in the nest iteration.

    >
    > ...
    >
    > > +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.

    I will mode is_invert_range after the int members.

    >
    > > + 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).

    Seems not applicable here.
    - IIO does not use linear_range or something similar. It just uses simple int.
    - ASoC does not use linear_range or something similar. It just uses simple long.

    So, I keep the simple int min and max.

    >
    > > +};
    >
    > ...
    >
    > > + if (val < 0)
    > > + return -EINVAL;
    > > + if (val > max - min)
    >
    > Btw, who will validate that max > min?

    By construction,
    min = 0
    max = iio_read_max_channel_raw() - iio_read_min_channel_raw()

    and iio_read_max_channel_raw() returns a value greater or equal to
    iio_read_min_channel_raw().

    But to be sure, I will check the last asumption at probe() and swap
    the minimum and maximum values if needed.

    >
    > > + return -EINVAL;
    >
    > ...
    >
    > > + return 1; /* The value changed */
    >
    > Perhaps this 1 needs a definition?

    Yes but to be coherent, in ASoC code, many places need to be changed too
    in order to use the newly defined value.
    I don't think these modifications should be part of this series.

    >
    > ...
    >
    > > +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.

    Indeed, the 0 assignment will be removed in the next iteration.

    >
    > ...
    >
    > > + char *input_name = NULL;
    > > + char *output_name = NULL;
    > > + char *pga_name = NULL;
    >
    > Redundant assignments if you properly label the freeing.

    I will rework the error paths (gotos) to avoid these assignement.

    >
    > ...
    >
    > > + BUILD_BUG_ON(ARRAY_SIZE(widgets) < 3);
    >
    > Use static_assert() at the place where the array is defined.

    Will be done in next iteration.

    >
    > ...
    >
    > > + BUILD_BUG_ON(ARRAY_SIZE(routes) < 2);
    >
    > Ditto.
    Will be done in next iteration.

    >
    > ...
    >
    > > +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?

    Will be changed in the next iteration.

    >
    > > + }
    > > +
    > > + 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.

    Will be changed in the next iteration.

    >
    > > + }
    > > +
    > > + /* 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.

    Will be changed in the next iteration.

    >
    > > + }
    >
    > ...
    >
    > > + 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()

    Indeed, I didn't know str_on_off().
    Thanks for pointing.
    Will be use in next iteration.

    >
    > > + }
    >
    > ...
    >
    > > + 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();
    Will be changed in next iteration.
    >
    > > + }
    >
    > ...
    >
    > > + 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.
    Will be changed in next iteration.
    >
    > > + }
    >
    > > + 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.

    Not sure, is_invert_range is a bool and tmp is a u32.

    In previous iteration, I wrote
    iio_aux_chan->is_invert_range = !!tmp;

    >
    > > + }
    >
    > Btw, can you avoid using OF APIs? It's better to have device property/fwnode
    > API to be used from day 1.

    Hum, this comment was raised in the previous iteration
    https://lore.kernel.org/linux-kernel/20230501162456.3448c494@jic23-huawei/

    I didn't find any equivalent to of_property_read_u32_index() in the
    device_property_read_*() function family.
    I mean I did find anything available to get a value from an array using an index.

    In the previous iteration it was concluded that keeping OF APIs in this series
    seemed "reasonable".

    >
    > ...
    >
    > > + platform_set_drvdata(pdev, iio_aux);
    >
    > Which callback is using this driver data?

    None -> I will remove platform_set_drvdata().

    >
    > ...
    >
    > > +static const struct of_device_id audio_iio_aux_ids[] = {
    > > + { .compatible = "audio-iio-aux", },
    >
    > Inner comma is not needed.

    Will be fixed.

    >
    > > + { }
    > > +};
    >
    > ...
    >
    > > +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

    Will be fixed.

    >
    > > +module_platform_driver(audio_iio_aux_driver);
    >



    --
    Hervé Codina, Bootlin
    Embedded Linux and Kernel engineering
    https://bootlin.com

    \
     
     \ /
      Last update: 2023-06-06 15:54    [W:4.423 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site