| 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: <20250608164416.7a2f3ac0@jic23-huawei>
Date: Sun, 8 Jun 2025 16:44:16 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lothar Rubusch <l.rubusch@...il.com>
Cc: dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
corbet@....net, lucas.p.stankus@...il.com, lars@...afoo.de,
Michael.Hennerich@...log.com, bagasdotme@...il.com,
linux-iio@...r.kernel.org, linux-doc@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 06/11] iio: accel: adxl313: add basic interrupt
handling for FIFO watermark
On Sun, 1 Jun 2025 17:21:34 +0000
Lothar Rubusch <l.rubusch@...il.com> wrote:
> Prepare the interrupt handler. Add register entries to evaluate the
> incoming interrupt. Add functions to clear status registers and reset the
> FIFO.
>
> Add FIFO watermark configuration and evaluation. Let a watermark to be
> configured. Evaluate the interrupt accordingly. Read out the FIFO content
> and push the values to the IIO channel.
>
> Signed-off-by: Lothar Rubusch <l.rubusch@...il.com>
> ---
> drivers/iio/accel/adxl313.h | 10 ++
> drivers/iio/accel/adxl313_core.c | 190 +++++++++++++++++++++++++++++++
> 2 files changed, 200 insertions(+)
>
> diff --git a/drivers/iio/accel/adxl313.h b/drivers/iio/accel/adxl313.h
> index ab6b9e11fde4..4f4a9fd39f6d 100644
> --- a/drivers/iio/accel/adxl313.h
> +++ b/drivers/iio/accel/adxl313.h
> @@ -53,11 +53,19 @@
> #define ADXL313_INT_ACTIVITY BIT(4)
> #define ADXL313_INT_DREADY BIT(7)
>
> +/* FIFO entries: how many values are stored in the FIFO */
> +#define ADXL313_REG_FIFO_STATUS_ENTRIES_MSK GENMASK(5, 0)
> +/* FIFO samples: number of samples needed for watermark (FIFO mode) */
> +#define ADXL313_REG_FIFO_CTL_SAMPLES_MSK GENMASK(4, 0)
> #define ADXL313_REG_FIFO_CTL_MODE_MSK GENMASK(7, 6)
>
> #define ADXL313_FIFO_BYPASS 0
> #define ADXL313_FIFO_STREAM 2
>
> +#define ADXL313_FIFO_SIZE 32
> +
> +#define ADXL313_NUM_AXIS 3
> +
> extern const struct regmap_access_table adxl312_readable_regs_table;
> extern const struct regmap_access_table adxl313_readable_regs_table;
> extern const struct regmap_access_table adxl314_readable_regs_table;
> @@ -78,7 +86,9 @@ struct adxl313_data {
> struct regmap *regmap;
> const struct adxl313_chip_info *chip_info;
> struct mutex lock; /* lock to protect transf_buf */
> + u8 watermark;
> __le16 transf_buf __aligned(IIO_DMA_MINALIGN);
> + __le16 fifo_buf[ADXL313_NUM_AXIS * ADXL313_FIFO_SIZE + 1];
> };
>
> struct adxl313_chip_info {
> diff --git a/drivers/iio/accel/adxl313_core.c b/drivers/iio/accel/adxl313_core.c
> index 31ce1b218488..8a0b5542fb40 100644
> --- a/drivers/iio/accel/adxl313_core.c
> +++ b/drivers/iio/accel/adxl313_core.c
> @@ -10,15 +10,21 @@
> #include <linux/bitfield.h>
> #include <linux/interrupt.h>
> #include <linux/module.h>
> +#include <linux/overflow.h>
> #include <linux/property.h>
> #include <linux/regmap.h>
>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/kfifo_buf.h>
> +
> #include "adxl313.h"
>
> #define ADXL313_INT_NONE U8_MAX
> #define ADXL313_INT1 1
> #define ADXL313_INT2 2
>
> +#define ADXL313_REG_XYZ_BASE ADXL313_REG_DATA_AXIS(0)
> +
> static const struct regmap_range adxl312_readable_reg_range[] = {
> regmap_reg_range(ADXL313_REG_DEVID0, ADXL313_REG_DEVID0),
> regmap_reg_range(ADXL313_REG_OFS_AXIS(0), ADXL313_REG_OFS_AXIS(2)),
> @@ -62,6 +68,7 @@ bool adxl313_is_volatile_reg(struct device *dev, unsigned int reg)
> case ADXL313_REG_DATA_AXIS(4):
> case ADXL313_REG_DATA_AXIS(5):
> case ADXL313_REG_FIFO_STATUS:
> + case ADXL313_REG_INT_SOURCE:
It's always been volatile, whether or not we were writing it.
Hmm. Given this I'm dropping the regcache patch as I'd missed that was a partial
list when reviewing that one.
> return true;
> default:
> return false;
> @@ -363,6 +370,176 @@ static int adxl313_write_raw(struct iio_dev *indio_dev,
> }
> }
>
> +static int adxl313_set_watermark(struct iio_dev *indio_dev, unsigned int value)
> +{
> + struct adxl313_data *data = iio_priv(indio_dev);
> + const unsigned int fifo_mask = 0x1f, interrupt_mask = 0x02;
Why not just use defines for the fields? The second one is particularly
confusing as that is just the mask for the watermark interrupt not of
a general 'interrupt' field as the name suggests.
> + int ret;
> +
> + value = min(value, ADXL313_FIFO_SIZE - 1);
> +
> + ret = regmap_update_bits(data->regmap, ADXL313_REG_FIFO_CTL,
> + fifo_mask, value);
> + if (ret)
> + return ret;
> +
> + data->watermark = value;
> +
> + return regmap_update_bits(data->regmap, ADXL313_REG_INT_ENABLE,
> + interrupt_mask, ADXL313_INT_WATERMARK);
return regmap_set_bits(data->regmap, ADXL313_REG_INT_ENABLED,
ADXL313_INT_WATERMARK);
> +}
> +
> +static int adxl313_buffer_postenable(struct iio_dev *indio_dev)
> +{
> + struct adxl313_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + ret = adxl313_set_measure_en(data, false);
I'd like a comment on why we need to disable measurements
here. Some reference to the datasheet probably and that fifo mode
can only be changed with measurements disabled.
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(data->regmap, ADXL313_REG_FIFO_CTL,
> + FIELD_PREP(ADXL313_REG_FIFO_CTL_SAMPLES_MSK, data->watermark) |
> + FIELD_PREP(ADXL313_REG_FIFO_CTL_MODE_MSK, ADXL313_FIFO_STREAM));
> + if (ret)
> + return ret;
> +
> + return adxl313_set_measure_en(data, true);
> +}
> +static int adxl313_push_event(struct iio_dev *indio_dev, int int_stat)
Can we avoid 'event' naming here. Events in IIO terms would not include
watermarks used to drain a fifo.
> +{
> + struct adxl313_data *data = iio_priv(indio_dev);
> + int samples;
> +
> + if (FIELD_GET(ADXL313_INT_WATERMARK, int_stat)) {
> + samples = adxl313_get_samples(data);
> + if (samples < 0)
> + return samples;
> +
> + return adxl313_fifo_push(indio_dev, samples);
> + }
> +
> + /* Return error if no event data was pushed to the IIO channel. */
> + return -ENOENT;
> +}
> +
> +static irqreturn_t adxl313_irq_handler(int irq, void *p)
> +{
> + struct iio_dev *indio_dev = p;
> + struct adxl313_data *data = iio_priv(indio_dev);
> + int int_stat;
> +
> + if (regmap_read(data->regmap, ADXL313_REG_INT_SOURCE, &int_stat))
> + return IRQ_NONE;
> +
> + if (adxl313_push_event(indio_dev, int_stat))
> + goto err;
> +
> + if (FIELD_GET(ADXL313_INT_OVERRUN, int_stat))
I suspect that we can have watermark and overrun set. Whether it is appropriate
to drain the data out and push it to userspace isn't clear to me. Maybe
add a comment on that so we can refer to it when considering the logic.
> + goto err;
> +
> + return IRQ_HANDLED;
> +
> +err:
> + adxl313_fifo_reset(data);
> +
> + return IRQ_HANDLED;
> +}
> +
> static int adxl313_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> unsigned int writeval, unsigned int *readval)
> {
> @@ -377,6 +554,7 @@ static const struct iio_info adxl313_info = {
> .read_raw = adxl313_read_raw,
> .write_raw = adxl313_write_raw,
> .read_avail = adxl313_read_freq_avail,
> + .hwfifo_set_watermark = adxl313_set_watermark,
> .debugfs_reg_access = &adxl313_reg_access,
> };
>
> @@ -487,6 +665,18 @@ int adxl313_core_probe(struct device *dev,
> int_map_msk, int_line == ADXL313_INT2);
> if (ret)
> return ret;
> +
> + ret = devm_iio_kfifo_buffer_setup(dev, indio_dev,
> + &adxl313_buffer_ops);
> + if (ret)
> + return ret;
> +
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + &adxl313_irq_handler,
> + IRQF_SHARED | IRQF_ONESHOT,
> + indio_dev->name, indio_dev);
> + if (ret)
> + return ret;
> } else {
> /*
> * FIFO_BYPASSED mode
Powered by blists - more mailing lists