Re: [PATCH 07/15] iio: adc: ad4030: Add SPI offload support

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Sat, Aug 30, 2025 at 3:43 AM Marcelo Schmitt
<marcelo.schmitt@analog.com> wrote:
>
> AD4030 and similar ADCs can capture data at sample rates up to 2 mega
> samples per second (MSPS). Not all SPI controllers are able to achieve
> such high throughputs and even when the controller is fast enough to run
> transfers at the required speed, it may be costly to the CPU to handle
> transfer data at such high sample rates.  Add SPI offload support for
> AD4030 and similar ADCs so to enable ADC data capture at maximum sample

Either add a comma after ADCs or drop 'so' word.

> rates.

> Cc: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Cc: Nuno Sa <nuno.sa@analog.com>
> Cc: Trevor Gamblin <tgamblin@baylibre.com>
> Cc: Axel Haslam <ahaslam@baylibre.com>
> Cc: David Lechner <dlechner@baylibre.com>

First of all, please keep Cc:s just after the '---' line, which will
have the same effect for email and make the commit message less noisy.
Second, don't put Cc for the people that you already have other tags
for.
Here I found at least 3 people that are repeated in the given specific
tags below. By default the tools (git send-email) converts all tags to
the Cc automatically.

> Co-developed-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@analog.com>
> Co-developed-by: Nuno Sa <nuno.sa@analog.com>
> Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> Co-developed-by: Trevor Gamblin <tgamblin@baylibre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@baylibre.com>
> Co-developed-by: Axel Haslam <ahaslam@baylibre.com>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

...

> -enum {
> +enum ad4030_lane_mode {

Sounds like a candidate for a separate change, but I haven't checked
how big this part is, so perhaps it's fine just to do it here.

...

>  static const int ad4030_average_modes[] = {
>         1, 2, 4, 8, 16, 32, 64, 128,
>         256, 512, 1024, 2048, 4096, 8192, 16384, 32768,
>         65536,

Side note, this looks like the list of bits, and can be optimised to use BIT().

>  };

...

> +       /*
> +        * The hardware does the capture on zone 2 (when spi trigger PWM
> +        * is used). This means that the spi trigger signal should happen at
> +        * tsync + tquiet_con_delay being tsync the conversion signal period
> +        * and tquiet_con_delay 9.8ns. Hence set the PWM phase accordingly.
> +        *
> +        * The PWM waveform API only supports nanosecond resolution right now,
> +        * so round this setting to the closest available value.
> +        */
> +       offload_offset_ns = AD4030_TQUIET_CNV_DELAY_NS;
> +       do {
> +               config->periodic.offset_ns = offload_offset_ns;
> +               ret = spi_offload_trigger_validate(st->offload_trigger, config);
> +               if (ret)
> +                       return ret;
> +               offload_offset_ns += 10;

> +

Unneeded blank line.

> +       } while (config->periodic.offset_ns < AD4030_TQUIET_CNV_DELAY_NS);

...

> +static int ad4030_set_sampling_freq(struct iio_dev *indio_dev, unsigned int freq)
> +{
> +       struct ad4030_state *st = iio_priv(indio_dev);
> +       int ret;
> +
> +       if (PTR_ERR_OR_ZERO(st->offload))
> +               return -EINVAL;

Why shadow the actual error code?

> +       if (!freq || freq > st->chip->max_sample_rate_hz)
> +               return -EINVAL;

in_range() ?

> +       ret = __ad4030_set_sampling_freq(st, freq);
> +       iio_device_release_direct(indio_dev);
> +
> +       return ret;
> +}

...

> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               if (PTR_ERR_OR_ZERO(st->offload))
> +                       return -EINVAL;

Shadowing an actual error code needs a good justification.

> +               ad4030_get_sampling_freq(st, val);
> +               return IIO_VAL_INT;


...

> +       st->offload_msg.offload = st->offload;
> +       ret = spi_optimize_message(st->spi, &st->offload_msg);
> +       if (ret < 0)

Why ' < 0'? Is it capable of returning positive values? If so, what
are their meanings?

> +               goto out_reset_mode;

...

> +       /*
> +        * Preemptively disable the PWM, since we only want to enable it with
> +        * the buffer

Missing period.

> +        */

...

> +static void ad4030_prepare_offload_msg(struct ad4030_state *st)
> +{
> +       u8 data_width = st->chip->precision_bits;
> +       u8 offload_bpw;
> +
> +       if (st->lane_mode == AD4030_LANE_MD_INTERLEAVED)

> +               /*
> +                * This means all channels on 1 lane.
> +                */

This is a one line comment. Why 3 LoCs?

> +               offload_bpw = data_width * st->chip->num_voltage_inputs;
> +       else
> +               offload_bpw  = data_width;
> +
> +       st->offload_xfer.speed_hz = AD4030_SPI_MAX_REG_XFER_SPEED;
> +       st->offload_xfer.bits_per_word = offload_bpw;
> +       st->offload_xfer.len = roundup_pow_of_two(BITS_TO_BYTES(offload_bpw));
> +       st->offload_xfer.offload_flags = SPI_OFFLOAD_XFER_RX_STREAM;
> +       spi_message_init_with_transfers(&st->offload_msg, &st->offload_xfer, 1);
> +}

...

> +       /* Fall back to low speed usage when no SPI offload available. */

is available

And choose one style for one line comments and use it everywhere.

...

> +       if (ret == -ENODEV) {
> +               /*
> +                * One hardware channel is split in two software channels when
> +                * using common byte mode. Add one more channel for the timestamp.
> +                */
> +               indio_dev->num_channels = 2 * st->chip->num_voltage_inputs + 1;
> +               indio_dev->channels = st->chip->channels;
> +               indio_dev->available_scan_masks = st->chip->available_masks;
> +
> +               ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
> +                                                     iio_pollfunc_store_time,
> +                                                     ad4030_trigger_handler,
> +                                                     &ad4030_buffer_setup_ops);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                            "Failed to setup triggered buffer\n");

> +

Stray blank line.

> +       } else {
> +               /*
> +                * One hardware channel is split in two software channels when
> +                * using common byte mode. Offloaded SPI transfers can't support
> +                * software timestamp so no additional timestamp channel is added.
> +                */
> +               indio_dev->num_channels = 2 * st->chip->num_voltage_inputs;
> +               indio_dev->channels = st->chip->offload_channels;
> +               indio_dev->available_scan_masks = st->chip->available_masks;
> +               ret = ad4030_spi_offload_setup(indio_dev, st);
> +               if (ret)
> +                       return dev_err_probe(dev, ret,
> +                                            "Failed to setup SPI offload\n");
> +
> +               ret = ad4030_pwm_get(st);
> +               if (ret)
> +                       return dev_err_probe(&spi->dev, ret,
> +                                            "Failed to get PWM: %d\n", ret);
> +
> +               ret = __ad4030_set_sampling_freq(st, st->chip->max_sample_rate_hz);
> +               ad4030_prepare_offload_msg(st);
> +       }

...

> -       }
> +       },

You see, this is the point I always make about leaving trailing commas
in the non-terminator entries.
(It's just a good example I can't help comment on this just for others
to point out again on this)

...

> +       .max_sample_rate_hz = 2 * MEGA,

HZ_PER_MHZ


...

> +       .max_sample_rate_hz = 2 * MEGA,

Ditto.

...

> +       .max_sample_rate_hz = 2 * MEGA,

Ditto.

...

> +       .max_sample_rate_hz = 500 * KILO,

HZ_PER_KHZ

...

> +       .max_sample_rate_hz = 500 * KILO,

Ditto.

-- 
With Best Regards,
Andy Shevchenko