| 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: <20230723132216.3c2f4924@jic23-huawei>
Date: Sun, 23 Jul 2023 13:22:16 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Waqar Hameed <waqar.hameed@...s.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, <kernel@...s.com>,
<linux-kernel@...r.kernel.org>, <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v4 3/3] iio: Add driver for Murata IRS-D200
On Wed, 19 Jul 2023 09:51:17 +0200
Waqar Hameed <waqar.hameed@...s.com> wrote:
> Murata IRS-D200 is a PIR sensor for human detection. It has support for
> raw data measurements and detection event notification.
>
> Add a driver with support for triggered buffer and events. Map the
> various settings to the `iio` framework, e.g. threshold values, sampling
> frequency, filter frequencies etc.
>
> Signed-off-by: Waqar Hameed <waqar.hameed@...s.com>
Hi Waqar.
I had some more feedback (see inline) but as it was all very minor, I've
made the changes whilst applying. Let me know if I messed anything up!
Applied to the togreg branch of iio.git and pushed out as testing for 0-day
to see if it can find anything we missed.
Thanks,
Jonathan
> +
> +/*
> + * Index corresponds to the (field) value of IRS_REG_FILTER register. Note that
> + * this represents a fractional value (e.g the first value corresponds to 3 / 10
> + * = 0.3 Hz).
> + */
> +static const unsigned int irsd200_hp_filter_freq[][2] = {
> + {3, 10},
> + {5, 10},
> +};
Prefer space after / before {}
...
> +static int irsd200_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state)
> +{
> + struct irsd200_data *data = iio_priv(indio_dev);
> + unsigned int tmp;
> + int ret;
> +
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + /* Clear the count register (by reading from it). */
> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &tmp);
> + if (ret)
> + return ret;
> +
> + tmp = !!state;
Don't use tmp again here as it suggests some use of previous value.
I'd just put !!state in where the parameter is used.
> + ret = regmap_field_write(
> + data->regfields[IRS_REGF_INTR_COUNT_THR_OR], tmp);
> + if (ret)
> + return ret;
> +
> + return 0;
return regmap_field_write(
data->regfields[IRS_REGF_INTR_COUNT_THR_OR], !!state);
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static irqreturn_t irsd200_irq_thread(int irq, void *dev_id)
> +{
> + struct iio_dev *indio_dev = dev_id;
> + struct irsd200_data *data = iio_priv(indio_dev);
> + enum iio_event_direction dir;
> + unsigned int lower_count;
> + unsigned int upper_count;
> + unsigned int status = 0;
> + unsigned int source = 0;
> + unsigned int clear = 0;
> + unsigned int count = 0;
> + int ret;
> +
> + ret = regmap_read(data->regmap, IRS_REG_INTR, &source);
> + if (ret) {
> + dev_err(data->dev, "Could not read interrupt source (%d)\n",
> + ret);
> + return IRQ_HANDLED;
> + }
> +
> + ret = regmap_read(data->regmap, IRS_REG_STATUS, &status);
> + if (ret) {
> + dev_err(data->dev, "Could not acknowledge interrupt (%d)\n",
> + ret);
> + return IRQ_HANDLED;
> + }
> +
> + if (status & BIT(IRS_INTR_DATA) && iio_buffer_enabled(indio_dev)) {
> + iio_trigger_poll_nested(indio_dev->trig);
> + clear |= BIT(IRS_INTR_DATA);
> + }
> +
> + if (status & BIT(IRS_INTR_COUNT_THR_OR) &&
> + source & BIT(IRS_INTR_COUNT_THR_OR)) {
> + /*
> + * The register value resets to zero after reading. We therefore
> + * need to read once and manually extract the lower and upper
> + * count register fields.
> + */
> + ret = regmap_read(data->regmap, IRS_REG_COUNT, &count);
> + if (ret)
> + dev_err(data->dev, "Could not read count (%d)\n", ret);
> +
> + upper_count = IRS_UPPER_COUNT(count);
> + lower_count = IRS_LOWER_COUNT(count);
> +
> + /*
> + * We only check the OR mode to be able to push events for
> + * rising and falling thresholds. AND mode is covered when both
> + * upper and lower count is non-zero, and is signaled with
> + * IIO_EV_DIR_EITHER.
> + */
> + if (upper_count && !lower_count)
> + dir = IIO_EV_DIR_RISING;
> + else if (!upper_count && lower_count)
> + dir = IIO_EV_DIR_FALLING;
> + else
> + dir = IIO_EV_DIR_EITHER;
> +
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_PROXIMITY, 0,
> + IIO_EV_TYPE_THRESH, dir),
> + iio_get_time_ns(indio_dev));
> +
> + /*
> + * The OR mode will always trigger when the AND mode does, but
> + * not vice versa. However, it seems like the AND bit needs to
> + * be cleared if data capture _and_ threshold count interrupts
> + * are desirable, even though it hasn't explicitly been selected
> + * (with IRS_REG_INTR). Either way, it doesn't hurt...
> + */
> + clear |= BIT(IRS_INTR_COUNT_THR_OR) |
> + BIT(IRS_INTR_COUNT_THR_AND);
> + }
> +
> + if (clear) {
> + ret = regmap_write(data->regmap, IRS_REG_STATUS, clear);
> + if (ret)
> + dev_err(data->dev,
> + "Could not clear interrupt status (%d)\n", ret);
> + }
> +
> + return clear ? IRQ_HANDLED : IRQ_NONE;
if (!clear)
return IRQ_NONE
ret = regmap_write(data->regmap ...
if (ret)
...
return IRQ_HANDLED;
Avoids complexity of checking clear multiple times and keeps the 'error' paths as
the ones that are indented. (here it might just be a shared interrupt rather than
an error).
> +}
> +
> +static const struct iio_chan_spec irsd200_channels[] = {
> + {
> + .type = IIO_PROXIMITY,
> + .info_mask_separate =
> + BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |
> + BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),
> + .info_mask_separate_available =
> + BIT(IIO_CHAN_INFO_SAMP_FREQ) |
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |
> + BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),
> + .event_spec = irsd200_event_spec,
> + .num_event_specs = ARRAY_SIZE(irsd200_event_spec),
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 16,
> + .endianness = IIO_CPU,
As a general rule, we don't adjust the byte ordering of data going out via
the buffered interface, instead leaving that work for userspace.
Given the handling here is very simple, I don't feel that strongly about
keeping to that, but thought I'd mention it.
Jonathan
> + },
> + },
> +};
Powered by blists - more mailing lists