From: Jonathan Cameron <jic23@kernel.org>
To: Paul Geurts <paul_geurts@live.nl>
Cc: "lars@metafoo.de" <lars@metafoo.de>,
"Michael.Hennerich@analog.com" <Michael.Hennerich@analog.com>,
"linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] iio: accel: adxl345: Added buffered read support
Date: Sun, 28 Apr 2024 18:21:52 +0100 [thread overview]
Message-ID: <20240428182152.7349129c@jic23-huawei> (raw)
In-Reply-To: <a1d0f85d-2ea7-4966-b842-c6e0028b61b2@live.nl>
On Mon, 22 Apr 2024 15:17:47 +0000
Paul Geurts <paul_geurts@live.nl> wrote:
> On 20-04-2024 14:54, Jonathan Cameron wrote:
> > On Wed, 17 Apr 2024 10:28:34 +0200
> > Paul Geurts <paul_geurts@live.nl> 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
prev parent reply other threads:[~2024-04-28 17:22 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-17 8:28 [PATCH] iio: accel: adxl345: Added buffered read support Paul Geurts
2024-04-20 12:54 ` Jonathan Cameron
2024-04-22 15:17 ` Paul Geurts
2024-04-28 17:21 ` Jonathan Cameron [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20240428182152.7349129c@jic23-huawei \
--to=jic23@kernel.org \
--cc=Michael.Hennerich@analog.com \
--cc=lars@metafoo.de \
--cc=linux-iio@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=paul_geurts@live.nl \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.