| 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: <20250922190113.xjqujtts7uu4cucg@antoni-VivoBook-ASUSLaptop-X512FAY-K512FA>
Date: Mon, 22 Sep 2025 21:01:13 +0200
From: Antoni Pokusinski <apokusinski01@...il.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>, jic23@...nel.org,
dlechner@...libre.com, nuno.sa@...log.com, andy@...nel.org,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
linux-iio@...r.kernel.org, linux@...ck-us.net,
rodrigo.gobbi.7@...il.com, naresh.solanki@...ements.com,
michal.simek@....com, grantpeltier93@...il.com,
farouk.bouabid@...rry.de, marcelo.schmitt1@...il.com
Subject: Re: [PATCH 2/3] iio: mpl3115: add support for DRDY interrupt
On Sun, Sep 21, 2025 at 10:29:28PM +0300, Andy Shevchenko wrote:
> On Sun, Sep 21, 2025 at 4:34 PM Antoni Pokusinski
> <apokusinski01@...il.com> wrote:
> >
> > MPL3115 sensor features a "data ready" interrupt which indicates the
> > presence of new measurements.
>
> ...
>
> > #include <linux/module.h>
>
> > #include <linux/iio/trigger_consumer.h>
> > #include <linux/iio/buffer.h>
> > #include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger.h>
> > #include <linux/delay.h>
>
> > +#include <linux/property.h>
>
> This is like delay.h is misplaced. What we do here, we group generic
> ones followed by subsystem (IIO) related ones, and this seems wrong in
> this driver.
>
> Can you rather move delay.h to be the first, and add property.h after
> i2c.h followed by a blank line, so in the result it will be like
>
> delay.h
> i2c.h
> module.h
> property.h
> ...blank.line...
> iio/*.h
>
> ...
>
Sure, will fix this in v2.
> > +#define MPL3115_CTRL_INT_SRC_DRDY BIT(7)
> > +
> > +#define MPL3115_PT_DATA_EVENT_ALL (BIT(2) | BIT(1) | BIT(0))
>
> Not sure I understand this definition in the following aspects:
> 1) why is this disrupting the _CTRL_ definitions?
> 2) why is this using BIT(x) and not respective definitions?
> 3) why can't you use GENMASK() if you just select all bits in a
> certain bitfield?
>
>
1) I placed the definitions of the bits/masks in the order of the registers
that they correspond to, i.e.
CTRL_INT_SRC_DRDY // bit in reg 0x12
PT_DATA_EVENT_ALL // bits in reg 0x13
CTRL_RESET // bit in reg 0x14
Actually, the wrong name here is the INT_SRC_DRDY definition because it is a
bit in the INT_SOURCE register, not a control register. Therefore, the
name should be MPL3115_INT_SRC_DRDY instead of MPL3115_CTRL_INT_SRC_DRDY
2) I saw that e.g. CTRL_OS_258MS is defined using BIT(x), so I thought
that this is the convention in this driver that I did not want to
disrupt by using GENMASK()
3) Sure, I'd even prefer GENMASK(), the only reason why I didn't use it
is explained in 2)
> > #define MPL3115_CTRL_RESET BIT(2) /* software reset */
> > #define MPL3115_CTRL_OST BIT(1) /* initiate measurement */
> > #define MPL3115_CTRL_ACTIVE BIT(0) /* continuous measurement */
> > #define MPL3115_CTRL_OS_258MS (BIT(5) | BIT(4)) /* 64x oversampling */
>
> ...
>
> > mutex_lock(&data->lock);
> > - ret = mpl3115_request(data);
> > - if (ret < 0) {
> > - mutex_unlock(&data->lock);
> > - goto done;
> > + if (!(data->ctrl_reg1 & MPL3115_CTRL_ACTIVE)) {
> > + ret = mpl3115_request(data);
> > + if (ret < 0) {
>
> > + mutex_unlock(&data->lock);
>
> Instead, I suggest adding a prerequisite that moves the driver to use
> cleanup.h, in particular scoped_guard(). This will reduce a churn
> here,
>
Will add in v2.
> > + goto done;
> > + }
> > }
>
> ...
>
> > +static int mpl3115_set_trigger_state(struct iio_trigger *trig, bool state)
> > +{
> > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > + struct mpl3115_data *data = iio_priv(indio_dev);
> > + int ret;
> > + u8 ctrl_reg1 = data->ctrl_reg1;
> > +
> > + if (state)
> > + ctrl_reg1 |= MPL3115_CTRL_ACTIVE;
> > + else
> > + ctrl_reg1 &= ~MPL3115_CTRL_ACTIVE;
>
> > + guard(mutex)(&data->lock);
>
> Oh, and you already use this! Definitely, it misses the prerequisite patch.
>
> > + ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > + ctrl_reg1);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG4,
> > + state ? MPL3115_CTRL_INT_EN_DRDY : 0);
> > + if (ret < 0)
> > + goto reg1_cleanup;
> > +
> > + data->ctrl_reg1 = ctrl_reg1;
> > +
> > + return 0;
> > +
> > +reg1_cleanup:
> > + i2c_smbus_write_byte_data(data->client, MPL3115_CTRL_REG1,
> > + data->ctrl_reg1);
> > + return ret;
> > +}
>
> ...
>
> > +static int mpl3115_trigger_probe(struct mpl3115_data *data,
> > + struct iio_dev *indio_dev)
> > +{
> > + struct fwnode_handle *fwnode;
> > + int ret, irq, irq_type;
> > + bool act_high, is_int2 = false;
>
> > + fwnode = dev_fwnode(&data->client->dev);
> > + if (!fwnode)
> > + return -ENODEV;
>
>
> Why is this fatal? Also, do we have a board file for users of this right now?
>
Actually it seems it does not have to be fatal. If we get rid of this
if, then we'd simply return without setting up the trigger and with no
interrupt support, which is ok I guess.
As for the board file, do you mean some PCB schematics? I don't know
about any, I've only used the following datasheet from NXP:
https://www.nxp.com/docs/en/data-sheet/MPL3115A2S.pdf
> > + irq = fwnode_irq_get_byname(fwnode, "INT1");
> > + if (irq < 0) {
> > + irq = fwnode_irq_get_byname(fwnode, "INT2");
> > + if (irq < 0)
> > + return 0;
> > +
> > + is_int2 = true;
> > + }
> > +
> > + irq_type = irq_get_trigger_type(irq);
> > + switch (irq_type) {
> > + case IRQF_TRIGGER_RISING:
> > + act_high = true;
> > + break;
> > + case IRQF_TRIGGER_FALLING:
> > + act_high = false;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = i2c_smbus_write_byte_data(data->client, MPL3115_PT_DATA_CFG,
> > + MPL3115_PT_DATA_EVENT_ALL);
> > + if (ret < 0)
> > + return ret;
>
> > + if (!is_int2) {
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + MPL3115_CTRL_REG5,
> > + MPL3115_CTRL_INT_CFG_DRDY);
> > + if (ret)
> > + return ret;
> > + }
> > + if (act_high) {
> > + ret = i2c_smbus_write_byte_data(data->client,
> > + MPL3115_CTRL_REG3,
> > + is_int2 ? MPL3115_CTRL_IPOL2 :
> > + MPL3115_CTRL_IPOL1);
> > + if (ret)
> > + return ret;
> > + }
>
> This if (!is_int2) and ternary with the same argument is kinda hard to
> read, can we refactor it somehow?
>
> For example, if these two booleans are represented by a common enum, we can do
>
> switch (cfg_flags) {
> case INT2_ACTIVE_HIGH:
> _write_byte_data(REG3);
> break;
> case INT2_ACTIVE_LOW:
> break;
> case INT1_ACTIVE_HIGH:
> _write_byte_data(REG5);
> _write_byte_data(REG3);
> break;
> case INT1_ACTIVE_LOW:
> _write_byte_data(REG5);
> break;
> default:
> return -EINVAL;
> }
>
> Yes, it's more verbose, but I find this better to read and understand.
>
> Note, you may drop the switch case for IRQ with this approach as you
> can use a few bits together (separate bits for raising and falling to
> make the default case working here).
>
Ok, your suggestion looks nice. I think to define maybe the enums like this:
#define INT2 BIT(2)
enum {
INT2_ACTIVE_LOW = INT2 | IRQF_TRIGGER_FALLING,
INT2_ACTIVE_HIGH = INT2 | IRQF_TRIGGER_RISING,
INT1_ACTIVE_LOW = !INT2 | IRQF_TRIGGER_FALLING,
INT1_ACTIVE_HIGH = !INT2 | IRQF_TRIGGER_RISING,
};
This way the cfg_flags could be first |= INT_2 (after the call to the
fwnode_irq_get_byname) and then |= irq_type (after the call to the irq_get_trigger_type)
> > + data->drdy_trig = devm_iio_trigger_alloc(&data->client->dev,
> > + "%s-dev%d",
> > + indio_dev->name,
> > + iio_device_id(indio_dev));
> > + if (!data->drdy_trig)
> > + return -ENOMEM;
> > +
> > + data->drdy_trig->ops = &mpl3115_trigger_ops;
> > + iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
> > + ret = iio_trigger_register(data->drdy_trig);
> > + if (ret)
> > + return ret;
> > +
> > + indio_dev->trig = iio_trigger_get(data->drdy_trig);
> > +
> > + return devm_request_threaded_irq(&data->client->dev, irq,
> > + NULL,
> > + mpl3115_interrupt_handler,
> > + IRQF_ONESHOT,
> > + "mpl3115_irq",
> > + indio_dev);
> > +}
>
> --
> With Best Regards,
> Andy Shevchenko
Kind regards,
Antoni Pokusinski
Powered by blists - more mailing lists