| 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: <CAHp75Vfu-C3Hd0ZXTj4rxEgRe_O84cfo6jiRCPFxZJnYrvROWQ@mail.gmail.com>
Date: Sat, 30 Aug 2025 10:36:11 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Marcelo Schmitt <marcelo.schmitt@...log.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, devicetree@...r.kernel.org,
linux-spi@...r.kernel.org, jic23@...nel.org, Michael.Hennerich@...log.com,
nuno.sa@...log.com, eblanc@...libre.com, dlechner@...libre.com,
andy@...nel.org, corbet@....net, robh@...nel.org, krzk+dt@...nel.org,
conor+dt@...nel.org, broonie@...nel.org, Jonathan.Cameron@...wei.com,
andriy.shevchenko@...ux.intel.com, ahaslam@...libre.com,
sergiu.cuciurean@...log.com, tgamblin@...libre.com,
marcelo.schmitt1@...il.com
Subject: Re: [PATCH 07/15] iio: adc: ad4030: Add SPI offload support
On Sat, Aug 30, 2025 at 3:43 AM Marcelo Schmitt
<marcelo.schmitt@...log.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@...log.com>
> Cc: Nuno Sa <nuno.sa@...log.com>
> Cc: Trevor Gamblin <tgamblin@...libre.com>
> Cc: Axel Haslam <ahaslam@...libre.com>
> Cc: David Lechner <dlechner@...libre.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@...log.com>
> Signed-off-by: Sergiu Cuciurean <sergiu.cuciurean@...log.com>
> Co-developed-by: Nuno Sa <nuno.sa@...log.com>
> Signed-off-by: Nuno Sa <nuno.sa@...log.com>
> Co-developed-by: Trevor Gamblin <tgamblin@...libre.com>
> Signed-off-by: Trevor Gamblin <tgamblin@...libre.com>
> Co-developed-by: Axel Haslam <ahaslam@...libre.com>
> Signed-off-by: Axel Haslam <ahaslam@...libre.com>
> Signed-off-by: Marcelo Schmitt <marcelo.schmitt@...log.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
Powered by blists - more mailing lists