| 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: <4c62baf4-fcb0-474f-87cf-9689aa41966a@baylibre.com>
Date: Fri, 9 Aug 2024 11:01:31 -0500
From: David Lechner <dlechner@...libre.com>
To: Nuno Sá <noname.nuno@...il.com>,
Jonathan Cameron <jic23@...nel.org>
Cc: Michael Hennerich <Michael.Hennerich@...log.com>,
Nuno Sá <nuno.sa@...log.com>,
Jonathan Corbet <corbet@....net>, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-doc@...r.kernel.org
Subject: Re: [PATCH 1/2] iio: adc: ad4695: implement triggered buffer
On 8/9/24 9:24 AM, Nuno Sá wrote:
> On Wed, 2024-08-07 at 15:02 -0500, David Lechner wrote:
>> This implements buffered reads for the ad4695 driver using the typical
>> triggered buffer implementation, including adding a soft timestamp
>> channel.
>>
>> The chip has 4 different modes for doing conversions. The driver is
>> using the advanced sequencer mode since that is the only mode that
>> allows individual configuration of all aspects each channel (e.g.
>> bipolar config currently and oversampling to be added in the future).
>>
>> Signed-off-by: David Lechner <dlechner@...libre.com>
>> ---
>
> Hi David,
>
> Just two nit comments...
>
> Reviewed-by: Nuno Sa <nuno.sa@...log.com>
>
>> drivers/iio/adc/ad4695.c | 233 ++++++++++++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 230 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/iio/adc/ad4695.c b/drivers/iio/adc/ad4695.c
>> index 007ecb951bc3..a3bd5be36134 100644
>> --- a/drivers/iio/adc/ad4695.c
>> +++ b/drivers/iio/adc/ad4695.c
>
> ...
>
>>
>>
>> +static int ad4695_buffer_preenable(struct iio_dev *indio_dev)
>> +{
>> + struct ad4695_state *st = iio_priv(indio_dev);
>> + struct spi_transfer *xfer;
>> + u8 temp_chan_bit = st->chip_info->num_voltage_inputs;
>> + bool temp_chan_en = false;
>> + u32 reg, mask, val, bit, num_xfer, num_slots;
>> + int ret;
>> +
>> + /*
>> + * We are using the advanced sequencer since it is the only way to read
>> + * multiple channels that allows individual configuration of each
>> + * voltage input channel. Slot 0 in the advanced sequencer is used to
>> + * account for the gap between trigger polls - we don't read data from
>> + * this slot. Each enabled voltage channel is assigned a slot starting
>> + * with slot 1.
>> + */
>> + num_slots = 1;
>> +
>> + memset(st->buf_read_xfer, 0, sizeof(st->buf_read_xfer));
>> +
>> + /* First xfer is only to trigger conversion of slot 1, so no rx. */
>> + xfer = &st->buf_read_xfer[0];
>> + xfer->cs_change = 1;
>> + xfer->delay.value = AD4695_T_CNVL_NS;
>> + xfer->delay.unit = SPI_DELAY_UNIT_NSECS;
>> + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
>> + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>> + num_xfer = 1;
>> +
>> + iio_for_each_active_channel(indio_dev, bit) {
>> + xfer = &st->buf_read_xfer[num_xfer];
>> + xfer->bits_per_word = 16;
>> + xfer->rx_buf = &st->buf[(num_xfer - 1) * 2];
>> + xfer->len = 2;
>> + xfer->cs_change = 1;
>> + xfer->cs_change_delay.value = AD4695_T_CONVERT_NS;
>> + xfer->cs_change_delay.unit = SPI_DELAY_UNIT_NSECS;
>> +
>> + if (bit == temp_chan_bit) {
>> + temp_chan_en = true;
>> + } else {
>> + reg = AD4695_REG_AS_SLOT(num_slots);
>> + val = FIELD_PREP(AD4695_REG_AS_SLOT_INX, bit);
>> +
>> + ret = regmap_write(st->regmap, reg, val);
>> + if (ret)
>> + return ret;
>> +
>> + num_slots++;
>> + }
>> +
>> + num_xfer++;
>> + }
>> +
>> + /*
>> + * Don't keep CS asserted after last xfer. Also triggers conversion of
>> + * slot 0.
>> + */
>> + xfer->cs_change = 0;
>> +
>> + /**
>> + * The advanced sequencer requires that at least 2 slots are enabled.
>> + * Since slot 0 is always used for other purposes, we need only 1
>> + * enabled voltage channel to meet this requirement. This error will
>> + * only happen if only the temperature channel is enabled.
>> + */
>> + if (num_slots < 2) {
>> + dev_err_ratelimited(&indio_dev->dev,
>> + "Buffered read requires at least 1 voltage channel
>> enabled\n");
>
> This one is intriguing... Why the ratelimited variant? Normally you'd use that in IRQ
> routines where the log could be flooded.
IIO Oscilloscope does a lot of retries of buffered reads very quickly,
so was getting a minor flood (10-20 repeats). I'm not sure that
ratelimited actually helped in this case though.
I suppose we could just drop this and expect people to read the docs
if they get an EINVAL when attempting to enable the buffer. Or just
make it dev_err() since it isn't 100s of repeats.
>> + return -EINVAL;
>> + }
>> +
>> + /*
>> + * Temperature channel isn't included in the sequence, but rather
>> + * controlled by setting a bit in the TEMP_CTRL register.
>> + */
>> +
>> + reg = AD4695_REG_TEMP_CTRL;
>> + mask = AD4695_REG_TEMP_CTRL_TEMP_EN;
>> + val = FIELD_PREP(mask, temp_chan_en ? 1 : 0);
>> +
>> + ret = regmap_update_bits(st->regmap, reg, mask, val);
>> + if (ret)
>> + return ret;
>> +
>> + spi_message_init_with_transfers(&st->buf_read_msg, st->buf_read_xfer,
>> + num_xfer);
>> +
>> + ret = spi_optimize_message(st->spi, &st->buf_read_msg);
>> + if (ret)
>> + return ret;
>> +
>> + /* This triggers conversion of slot 0. */
>> + ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
>> + if (ret) {
>> + spi_unoptimize_message(&st->buf_read_msg);
>> + return ret;
>> + }
>
> Could save one line with (unless ad4695_enter_advanced_sequencer_mode() does not
> return 0 on success)
sure
>
> ret = ad4695_enter_advanced_sequencer_mode(st, num_slots);
> if (ret)
> spi_unoptimize_message(&st->buf_read_msg);
>
> return ret;
>
> - Nuno Sá
>
Powered by blists - more mailing lists