by Nuno Sá

[permalink] [raw]

Subject: Re: [PATCH v3 7/9] iio: imu: adis_trigger: Allow level interrupts

On Fri, 2024-05-17 at 10:47 +0300, Ramona Gradinariu wrote:
> Currently, adis library allows configuration only for edge interrupts,
> needed for data ready sampling.
> This patch removes the restriction for level interrupts, which are
> needed to handle FIFO watermark interrupts.
> Furthermore, in case of level interrupts, devm_request_threaded_irq is
> used for interrupt allocation, to avoid blocking the processor while
> retrieving the FIFO samples.

Technically this not totally accurate (though ends up being true) as we do read
the FIFO samples in a thread already. The part that runs on the top halve should
be:

iio_trigger_generic_data_rdy_poll() -> iio_trigger_poll() -> iio_pollfunc_store_time()

and given the FIFO nature (as the interrupt keeps firing until FIFO_CNT drops
below the watermark), it seems this is enough for you to see some freezes.

Anyhow, I'd say the commit message should be a bit refactored. This also leads to
another minor detail/question (see below)

>
> Signed-off-by: Ramona Gradinariu <[email protected]>
> ---
> changes in v3:
> - new patch
> drivers/iio/imu/adis_trigger.c | 39 ++++++++++++++++++----------------
> 1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
> index f890bf842db8..becf1f558b4e 100644
> --- a/drivers/iio/imu/adis_trigger.c
> +++ b/drivers/iio/imu/adis_trigger.c
> @@ -34,21 +34,16 @@ static int adis_validate_irq_flag(struct adis *adis)
> if (adis->data->unmasked_drdy)
> adis->irq_flag |= IRQF_NO_AUTOEN;
> /*
> - * Typically this devices have data ready either on the rising edge
> or
> - * on the falling edge of the data ready pin. This checks enforces
> that
> - * one of those is set in the drivers... It defaults to
> - * IRQF_TRIGGER_RISING for backward compatibility with devices that
> - * don't support changing the pin polarity.
> + * Typically adis devices without fifo have data ready either on the
> + * rising edge or on the falling edge of the data ready pin.
> + * IMU devices with fifo support have the watermark pin level driven
> + * either high or low when the fifo is filled with the desired number
> + * of samples.
> + * It defaults to IRQF_TRIGGER_RISING for backward compatibility with
> + * devices that don't support changing the pin polarity.
> */
> - if (direction == IRQF_TRIGGER_NONE) {
> + if (direction == IRQF_TRIGGER_NONE)
> adis->irq_flag |= IRQF_TRIGGER_RISING;
> - return 0;
> - } else if (direction != IRQF_TRIGGER_RISING &&
> -    direction != IRQF_TRIGGER_FALLING) {
> - dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> - adis->irq_flag);
> - return -EINVAL;
> - }

I guess then we should rename the function as no actual validation is being
done, right?

>
> return 0;
> }
> @@ -77,11 +72,19 @@ int devm_adis_probe_trigger(struct adis *adis, struct
> iio_dev *indio_dev)
> if (ret)
> return ret;
>
> - ret = devm_request_irq(&adis->spi->dev, adis->spi->irq,
> -        &iio_trigger_generic_data_rdy_poll,
> -        adis->irq_flag,
> -        indio_dev->name,
> -        adis->trig);
> + if (adis->irq_flag & (IRQF_TRIGGER_HIGH | IRQF_TRIGGER_LOW))
> + ret = devm_request_threaded_irq(&adis->spi->dev, adis->spi-
> >irq,
> + NULL,
> + &iio_trigger_generic_data_rdy
> _poll,
> + adis->irq_flag |
> IRQF_ONESHOT,
> + indio_dev->name,
> + adis->trig);

So, this is not really a big deal for me but I wonder if we should actually tie
this change to the device having FIFO support (so a boolean in the adis_data
struct)? It seems to me that's the real reason for the split... With the
boolean, we could also constrain the IRQ level support for devices supporting
FIFOs. Anyhow, since this is the first supported device with a FIFO having the
boolean (while it makes sense to me) may add more overhead than needed to the
series so I'm fine with this as-is.

- Nuno Sá
>

2024-05-22 09:51:54

by Ramona Gradinariu

[permalink] [raw]

Subject: Re: [PATCH v3 9/9] drivers: iio: imu: Add support for adis1657x family

On 5/19/24 21:57, Jonathan Cameron wrote:

> On Fri, 17 May 2024 10:47:50 +0300
> Ramona Gradinariu <[email protected]> wrote:
>
>> Add support for ADIS1657X family devices in already exiting ADIS16475
>> driver.
>>
>> Signed-off-by: Ramona Gradinariu <[email protected]>
> Whilst it's not necessarily vital to support, it I'm curious about
> what happens to the hardware timestamp? I thought we had one driver
> still doing hardware timestamps directly to the buffer, but I can't
> find it so I guess we now deal with alignment in the few devices with
> this support. The st_lsm6dsx has this sort of combining of local
> and fifo timestamps for example.
>
> As it stands I think you push the same timestamp for all scans read
> from the fifo on a particular watermark interrupt? That isn't
> ideal given we should definitely be able to do better than that.

Currently the timestamp is added when the FIFO is read, which does not
equal the sample time.

ADI IMU devices do not actually offer a hardware timestamp. The
functionality is as follows:
- for internal sync / output sync / direct external sync the burst
data returns a data counter (which increments with each sample);

- for scaled external sync the burst data returns the 'timestamp',
which contains the time associated with the last sample in each data
update relative to the most recent edge of the external clock signal.
For example, when the value in the UP_SCALE register represents a scale
factor of 20, DEC_RATE = 0, and the external SYNC rate = 100 Hz, the
following time stamp sequence results: 0LSB, 10LSB, 21LSB, 31LSB,
41LSB, 51LSB, 61LSB, 72LSB, …, 194LSB for the 20th sample, which
translates to 0us, 490us, …, 9510 us which is the time from the
previous sync edge.

We can make assumptions, and try to better estimate the timestamp,
based on the sampling frequency.
We can assume that the last sample in the FIFO was sampled when the
watermark interrupt took place, and then, based on the sample frequency
we could decrease the timestamp with the according period for each
sample.
However, I am not sure this assumption is always valid, it depends
on when the IRQ is actually handled.

I can remove the timestamp for devices which use FIFO seeing how it
does not represent the actual sample time.

>> +static const struct iio_buffer_setup_ops adis16475_buffer_ops = {
>> + .postenable = adis16475_buffer_postenable,
>> + .postdisable = adis16475_buffer_postdisable,
>> +};
>> +
>> +static int adis16475_set_watermark(struct iio_dev *indio_dev, unsigned int val)
>> +{
>> + struct adis16475 *st = iio_priv(indio_dev);
>> + int ret;
>> + u16 wm_lvl;
>> +
>> + adis_dev_lock(&st->adis);
> As a follow up perhaps consider defining magic to use guard() for these as there are
> enough users that will be simplified to make it worth the effort.

I will do this in another patch series if that is alright.

>
>> +
>> + val = min_t(unsigned int, val, ADIS16575_MAX_FIFO_WM);
>> +
>> + wm_lvl = ADIS16575_WM_LVL(val - 1);
>> + ret = __adis_update_bits(&st->adis, ADIS16475_REG_FIFO_CTRL, ADIS16575_WM_LVL_MASK, wm_lvl);
>> + if (ret)
>> + goto unlock;
>> +
>> + st->fifo_watermark = val;
>> +
>> +unlock:
>> + adis_dev_unlock(&st->adis);
>> + return ret;
>> +}
>> +