| 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: <20230716161107.75dc183a@jic23-huawei>
Date: Sun, 16 Jul 2023 16:11:07 +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 v2 3/3] iio: Add driver for Murata IRS-D200
On Wed, 12 Jul 2023 17:12: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,
Some additional comments inline.
Jonathan
> ---
> drivers/iio/proximity/Kconfig | 12 +
> drivers/iio/proximity/Makefile | 1 +
> drivers/iio/proximity/irsd200.c | 982 ++++++++++++++++++++++++++++++++
> 3 files changed, 995 insertions(+)
> create mode 100644 drivers/iio/proximity/irsd200.c
> diff --git a/drivers/iio/proximity/irsd200.c b/drivers/iio/proximity/irsd200.c
> new file mode 100644
> index 000000000000..e6b9eca1bf8e
> --- /dev/null
> +++ b/drivers/iio/proximity/irsd200.c
> @@ -0,0 +1,982 @@
> +
> +static int irsd200_read_data(struct irsd200_data *data, s16 *val)
> +{
> + u8 buf[2];
__le16
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, IRS_REG_DATA_LO, buf,
> + ARRAY_SIZE(buf));
> + if (ret) {
> + dev_err(data->dev, "Could not bulk read data (%d)\n", ret);
> + return ret;
> + }
> +
> + *val = (buf[1] << 8) | buf[0];
If you make buf a __le16 then you can use aligned conversions otherwise
get_unaligned_le16(buf)
> +
> + return 0;
> +}
> +
...
> +
> +static int irsd200_read_timer(struct irsd200_data *data, int *val, int *val2)
> +{
> + u8 buf[2];
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, IRS_REG_TIMER_LO, buf,
> + ARRAY_SIZE(buf));
> + if (ret) {
> + dev_err(data->dev, "Could not bulk read timer (%d)\n", ret);
> + return ret;
> + }
> +
> + ret = irsd200_read_data_rate(data, val2);
> + if (ret)
> + return ret;
> +
> + /* Value is 10 bits. IRS_REG_TIMER_HI is the two MSBs. */
> + *val = (buf[1] << 8) | buf[0];
Another one where type should be __le16 and appropriate conversion functions
used.
> +
> + return 0;
> +}
> +
> +static int irsd200_write_timer(struct irsd200_data *data, int val, int val2)
> +{
> + unsigned int regval;
> + int data_rate;
> + u8 buf[2];
__le16 might be more appropriate.
> + int ret;
> +
> + if (val < 0 || val2 < 0)
> + return -ERANGE;
> +
> + ret = irsd200_read_data_rate(data, &data_rate);
> + if (ret)
> + return ret;
> +
> + /* Quantize from seconds. */
> + regval = val * data_rate + (val2 * data_rate) / 1000000;
> +
> + /* Value is 10 bits. */
> + if (regval >= BIT(10))
> + return -ERANGE;
> +
> + /* IRS_REG_TIMER_LO is the 8 LSBs and IRS_REG_TIMER_HI is the 2 MSBs. */
> + buf[0] = FIELD_GET(GENMASK(7, 0), regval);
> + buf[1] = FIELD_GET(GENMASK(9, 8), regval);
I think I'd rather see this as appropriate put_unaligned_le16() or similar with
the input masked.
> +
> + ret = regmap_bulk_write(data->regmap, IRS_REG_TIMER_LO, buf,
> + ARRAY_SIZE(buf));
> + if (ret) {
> + dev_err(data->dev, "Could not bulk write timer (%d)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
0;
> +}
> +
> +static int irsd200_write_nr_count(struct irsd200_data *data, int val)
> +{
> + unsigned int regval;
> + int ret;
> +
> + /* A value of zero means that IRS_REG_STATUS is never set. */
> + if (val <= 0 || val >= BIT(3))
BIT(3) is an unusual representation... Here, 8 is probably clearer unless
it really has something to do with that bit being set.
> + return -ERANGE;
> +
> + regval = val;
> +
> + if (regval >= 2) {
> + /*
> + * According to the data sheet, timer must be also set in this
> + * case (i.e. be non-zero). Check and enforce that.
> + */
> + ret = irsd200_read_timer(data, &val, &val);
> + if (ret)
> + return ret;
> +
> + if (val == 0) {
> + dev_err(data->dev,
> + "Timer must be non-zero when nr count is %u\n",
> + regval);
> + return -EPERM;
> + }
> + }
> +
> + ret = regmap_write(data->regmap, IRS_REG_NR_COUNT, regval);
> + if (ret) {
> + dev_err(data->dev, "Could not write nr count (%d)\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +
> +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 val;
> + 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, &val);
I'd use a different variable than val, preferably named to indicate
we don't care about the contents. Using val here and below is a little
confusing.
> + if (ret)
> + return ret;
> +
> + val = !!state;
> + ret = regmap_field_write(
> + data->regfields[IRS_REGF_INTR_COUNT_THR_OR], val);
> + if (ret)
> + return ret;
> +
> + return val;
Why return val? Should probably be 0 if this write succeeded.
> + 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_NONE;
As below. IRQ_NONE does not normally mean error, it means we are sure it is not
our interrupt. Even in error cases, return IRQ_HANDLED.
If you confirm via status bits that it isn't out interrupt, that's when you
return IRQ_NONE.
> + }
> +
> + ret = regmap_read(data->regmap, IRS_REG_STATUS, &status);
> + if (ret) {
> + dev_err(data->dev, "Could not acknowledge interrupt (%d)\n",
> + ret);
> + return IRQ_NONE;
> + }
> +
> + 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) {
if (!clear) then it's not our interrupt (I think) and you should return IRQ_NONE.
> + ret = regmap_write(data->regmap, IRS_REG_STATUS, clear);
> + if (ret)
> + dev_err(data->dev,
> + "Could not clear interrupt status (%d)\n", ret);
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t irsd200_trigger_handler(int irq, void *pollf)
> +{
> + struct iio_dev *indio_dev = ((struct iio_poll_func *)pollf)->indio_dev;
> + struct irsd200_data *data = iio_priv(indio_dev);
> + s16 buf = 0;
> + int ret;
> +
> + ret = irsd200_read_data(data, &buf);
> + if (ret)
> + goto end;
> +
> + iio_push_to_buffers_with_timestamp(indio_dev, &buf,
> + iio_get_time_ns(indio_dev));
> +
> +end:
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return ret ? IRQ_NONE : IRQ_HANDLED;
An error in a read is not reported as IRQ_NONE. The only time we should report
that is if we know it definitely wasn't our interrupt. Otherwise we will
fire the spurious interrupt handling disable logic for the wrong reason.
> +}
> +
> +
> +static int irsd200_probe(struct i2c_client *client)
> +{
> + struct iio_trigger *trigger;
> + struct irsd200_data *data;
> + struct iio_dev *indio_dev;
> + size_t i;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return dev_err_probe(&client->dev, -ENOMEM,
> + "Could not allocate iio device\n");
> +
> + data = iio_priv(indio_dev);
> + data->dev = &client->dev;
> +
> + data->regmap = devm_regmap_init_i2c(client, &irsd200_regmap_config);
> + if (IS_ERR(data->regmap))
> + return dev_err_probe(data->dev, PTR_ERR(data->regmap),
> + "Could not initialize regmap\n");
> +
> + for (i = 0; i < IRS_REGF_MAX; ++i) {
> + data->regfields[i] = devm_regmap_field_alloc(
> + data->dev, data->regmap, irsd200_regfields[i]);
> + if (IS_ERR(data->regfields[i]))
> + return dev_err_probe(
> + data->dev, PTR_ERR(data->regfields[i]),
> + "Could not allocate register field %zu\n", i);
> + }
> +
> + data->regulator = devm_regulator_get(data->dev, "vdd");
devm_regulator_get_enable()
> + if (IS_ERR(data->regulator))
> + return dev_err_probe(data->dev, PTR_ERR(data->regulator),
> + "Could not get regulator\n");
> +
> + ret = regulator_enable(data->regulator);
> + if (ret)
> + return dev_err_probe(data->dev, ret,
> + "Could not enable regulator (%d)\n", ret);
> +
> + ret = devm_add_action_or_reset(data->dev, irsd200_regulator_disable,
> + data->regulator);
> + if (ret)
> + return dev_err_probe(
> + data->dev, ret,
> + "Could not add regulator disable devres action (%d)\n",
> + ret);
...
Powered by blists - more mailing lists