| 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: <CAHp75Vd8Bwk8HVc3DhG4L=SgbSh3aFTQ2VRn7Tri8YhJrqaXgw@mail.gmail.com>
Date: Sun, 21 Sep 2025 22:29:28 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Antoni Pokusinski <apokusinski01@...il.com>
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
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 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
...
> +#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?
> #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,
> + 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?
> + 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).
> + 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
Powered by blists - more mailing lists