On Mon, 22 Apr 2024 15:17:47 +0000
Paul Geurts <paul_geurts@xxxxxxx> wrote:
> On 20-04-2024 14:54, Jonathan Cameron wrote:
> > On Wed, 17 Apr 2024 10:28:34 +0200
> > Paul Geurts <paul_geurts@xxxxxxx> wrote:
> >
> > Hi Paul, various comments inline.
> >
> >
> >> This implements buffered reads for the accelerometer data. A buffered
> >> IIO device is created containing 3 channels. The device FIFO is used for
> >> sample buffering to reduce the IRQ load on the host system.
> >>
> >> Reading of the device is triggered by a FIFO waterlevel interrupt. The
> >> waterlevel settings are dependent on the sampling frequency to leverage
> >> IRQ load against responsiveness.
> > I'm unconvinced that trade off belongs in the driver. Until now we
> > have exposed all the relevant controls to userspace via bufferX/watermark.
> > Set that to 0 or 1 if you don't want a fifo and appropriate level for whatever
> > responsiveness is needed for a particular application.
> >
> > The need to manually disable / enable interrupts is also normally something
> > that needs a close look. Very very occasionally this is necessary but for most
> > devices IRQF_ONESHOT should provide suitable masking.
> >
> > It's also not clear that a trigger is appropriate here. For FIFO equipped devices
> > like this, the trigger abstraction often doesn't work as we don't have one interrupt
> > per 'scan' of data. In these cases it is not necessary to use a trigger at all
> > and that can simplify things considerably.
> >
> > Jonathan
>
> This was my first interaction with the IIO subsystem, So these changes were somewhat of a
> learning experience. Your review comments indicate major refactoring of my patch is in
> order. I will see when I have time to get to it and resend it at some point.
Great that I didn't put you off too much and I should have said welcome!
As someone who has a v9 patch set to send out tomorrow (for another area of the kernel),
I'm well aware there can be quite a learning curve.
A few follow up comments below, but mostly guessing what might have made
things trickier rather than anything useful.
Jonathan
> >
> >> + if (ret < 0)
> >> + goto out;
> > return ret;
> >> + while (regval & ADXL345_INT_DATA_READY) {
> >> + ret = regmap_bulk_read(map, ADXL345_REG_DATA_AXIS(0), &axis_data,
> >> + sizeof(axis_data));
> >> + if (ret < 0)
> >> + goto out;
> > The datasheet puts a timing requirement on repeat reads that you need to enforce..
> > 5 usec.
> >
> > return ret;
>
> I didn't find this requirement in the datasheet. I did however write this for ADXL343, which seems
> to be compatible with ADXL345. But maybe I missed something here?
It's a slow bus, maybe the delay is above that anyway.
>
> >> + ret = regmap_read(map, ADXL345_REG_INT_SOURCE, ®val);
> >> + if (ret < 0)
> >> + goto out;
> > return directly - going to a label that does nothing makes a reviewer
> > following code paths have to go see that nothing happens.
> >
> > return ret;
> >
> >> + }
> >> +
> >> +out:
> >> + return ret;
> >> +}
> >> +
> >> +static int adxl345_buffer_preenable(struct iio_dev *indio_dev)
> >> +{
> >> + struct adxl345_data *data = iio_priv(indio_dev);
> >> + int ret;
> >> +
> >> + mutex_lock(&data->lock);
> >> + /* Disable measurement mode to setup everything */
> >> + ret = regmap_clear_bits(data->regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_MEASURE);
> >> + if (ret < 0)
> >> + goto out;
> >> +
> >> + ret = adxl345_flush_fifo(data->regmap);
> >> + if (ret < 0)
> >> + goto out_enable;
> >> +
> >> + /*
> >> + * Set the FIFO up in streaming mode so the chip keeps sampling.
> >> + * Waterlevel is set by the sample frequency functions as it is dynamic
> > This I don't follow. Why is it dynamic? It's fixed for a given run at a given
> > frequency. I can sort of see a true dynamic adjustment might make sense, but that
> > would be complex and isn't obviously a problem for the kernel.
> >
> > I'd prefer to see this done like the majority of other fifo handling drivers:
> > Make setting the watermark vs frequency a userspace problem.
>
> So having an interrupt coming in for every sample on 3200Hz sampling rate completely overloaded my
> i.MX8M Mini CPU (1600MHz) I was testing this on. This was the main reason to be using the internal
> FIFO in the accelerometer. But when doing that, the device becomes somewhat unresponsive on the
> lower frequencies, as it first needs to fill the FIFO before actually firing an interrupt towards
> the CPU. Therefore I created this, which only uses the water level interrupt on highter frequencies
> to try to have best of both worlds. But I agree that this should be a userspace choice.
It's a neat bit of code and maybe one day worth considering if we should try to auto
tune, but definitely not something to do in a driver alongside the initial
enablement.
>
> >
> >> + */
> >> + ret = regmap_update_bits(data->regmap, ADXL345_REG_FIFO_CTL,
> >> + (int)~(ADXL345_FIFO_CTL_SAMPLES_MASK),
> >> + ADXL345_FIFO_CTL_MODE_STREAM);
> >> + if (ret < 0)
> >> + goto out_enable;
> >> +
> >> + /* Enable the Watermark and Overrun interrupt */
> >> + ret = regmap_write(data->regmap, ADXL345_REG_INT_ENABLE, (ADXL345_INT_WATERMARK |
> >> + ADXL345_INT_OVERRUN));
> >> + if (ret < 0)
> >> + goto out_enable;
> >> +
> >> + /* Re-enable measurement mode */
> >> + ret = regmap_set_bits(data->regmap, ADXL345_REG_POWER_CTL, ADXL345_POWER_CTL_MEASURE);
> >> + goto out;
> > Don't do this as it makes for messy control flow to review
> >
> > if (ret)
> > goto out_enabled;
> >
> > mutex_unlock(&data->lock)
> > return 0;
> >
> > out_enable:
> > ...
> > Can use guard(mutex)(&data->lock) to avoid need to unlock manually and allow at least
> > some paths to return directly.
>
> I am very pleased to find out scoped locking is finally possible in the Linux kernel :-). I didn't know this.
>
Pretty new + the more complex corners of what can be done are still
controversial. Thankfully mutexes are a simple case.
> >> + int ret, data_available;
> >> +
> >> + mutex_lock(&data->lock);
> >> +
> >> + /* Disable the IRQ before reading the FIFO */
> > This needs a lot more explanation. Disabling interrupts like
> > this can be very expensive and can be hard to reason about
> > + it's not necessary most of the time because of IRQF_ONESHOT.
>
> I did experience some issues with the interrupts, hence the disabling. But I might have just
> implemented it badly. I will take a look into why things go bad and try to make it so that
> disabling IRQs is not needed anymore.
>
Often it's a type issue for interrupts - particularly level vs edge.
Thankfully it's fairly rare to get interrupt controllers that only
do one or the other these days - that used to be really painful to
try and handle in a driver.
> >> +
> >> + mutex_init(&data->lock);
> >> +
> >> indio_dev->name = data->info->name;
> >> indio_dev->info = &adxl345_info;
> >> - indio_dev->modes = INDIO_DIRECT_MODE;
> >> + indio_dev->modes = INDIO_DIRECT_MODE | INDIO_BUFFER_TRIGGERED;
> > No need as INDIO_BUFFER_TRIGGERED is set when you register the buffer.
>
> Didn't know that!
>
It's relatively recent (few years ago) so if you are on an older tree
it might not have been true.
Jonathan
