| 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: <20240821215622.GA478206@vamoiridPC>
Date: Wed, 21 Aug 2024 23:56:22 +0200
From: Vasileios Amoiridis <vassilisamir@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Vasileios Amoiridis <vassilisamir@...il.com>, lars@...afoo.de,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
andriy.shevchenko@...ux.intel.com, ang.iglesiasg@...il.com,
linus.walleij@...aro.org, biju.das.jz@...renesas.com,
javier.carrasco.cruz@...il.com, semen.protsenko@...aro.org,
579lpy@...il.com, ak@...klinger.de, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 6/7] iio: pressure: bmp280: Add data ready trigger
support
On Sun, Jul 28, 2024 at 05:06:50PM +0100, Jonathan Cameron wrote:
> On Fri, 26 Jul 2024 01:10:38 +0200
> Vasileios Amoiridis <vassilisamir@...il.com> wrote:
>
> > The BMP3xx and BMP5xx sensors have an interrupt pin which can be used as
> > a trigger for when there are data ready in the sensor for pick up.
> >
> > This use case is used along with NORMAL_MODE in the sensor, which allows
> > the sensor to do consecutive measurements depending on the ODR rate value.
> >
> > The trigger pin can be configured to be open-drain or push-pull and either
> > rising or falling edge.
> >
> > No support is added yet for interrupts for FIFO, WATERMARK and out of range
> > values.
> >
> > Signed-off-by: Vasileios Amoiridis <vassilisamir@...il.com>
> Hi Vasileios,
>
> A few minor things inline, including a suggestion that perhaps the trigger_probe()
> functions can be combined to reduce duplication. That would use a
> __bmp280_trigger_probe(struct iio_dev *, struct iio_trigger_ops *,
> + some function pointers).
>
> Perhaps it's not worth it - I didn't try writing the actual code!
>
> Jonathan
>
Hi Jonathan,
Initially, in the v1 of the series, I had designed differently the handling of the
interrupts, and having split functions was making more sense in case you had
sensors with extra interrupts.
Since you explained to me how to do it properly, I think that now, what you are
proposing makes sense.
> > ---
> > drivers/iio/pressure/bmp280-core.c | 309 ++++++++++++++++++++++++++-
> > drivers/iio/pressure/bmp280-regmap.c | 2 +-
> > drivers/iio/pressure/bmp280.h | 23 +-
> > 3 files changed, 328 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> > index 4a8d2ed4a9c4..4238f37b7805 100644
> > --- a/drivers/iio/pressure/bmp280-core.c
> > +++ b/drivers/iio/pressure/bmp280-core.c
> > @@ -37,12 +37,14 @@
>
>
>
> > +static irqreturn_t bmp380_irq_thread_handler(int irq, void *p)
> > +{
> > + struct iio_dev *indio_dev = p;
> > + struct bmp280_data *data = iio_priv(indio_dev);
> > + unsigned int int_ctrl;
> > + int ret;
> > +
> > + scoped_guard(mutex, &data->lock) {
> > + ret = regmap_read(data->regmap, BMP380_REG_INT_STATUS, &int_ctrl);
> > + if (ret)
> > + return IRQ_NONE;
> > + }
> > +
> > + if (FIELD_GET(BMP380_INT_STATUS_DRDY, int_ctrl))
> > + iio_trigger_poll_nested(data->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int bmp380_trigger_probe(struct iio_dev *indio_dev)
>
> Two of these functions are very similar. Perhaps define a common
> function that takes a function call for int config, the ops, and
> interrupt handler as arguments then add device specific
> calls that use that.
>
True, I will think if it could generate any issues in the future with newer
sensors adding more interrupts or with the current ones (bmp580 supports much
more interrupts than the bmp380) and I will act accordingly.
>
>
> > +{
> > + struct bmp280_data *data = iio_priv(indio_dev);
> > + struct fwnode_handle *fwnode;
> > + int ret, irq, irq_type;
> > + struct irq_data *desc;
> > +
> > + fwnode = dev_fwnode(data->dev);
> > + if (!fwnode)
> > + return -ENODEV;
> > +
> > + irq = fwnode_irq_get(fwnode, 0);
> > + if (!irq) {
> > + dev_err(data->dev, "No interrupt found\n");
> > + return -ENODEV;
> > + }
> > +
> > + desc = irq_get_irq_data(irq);
> > + if (!desc)
> > + return -EINVAL;
> > +
> > + irq_type = irqd_get_trigger_type(desc);
> > + switch (irq_type) {
> > + case IRQF_TRIGGER_RISING:
> > + data->trig_active_high = true;
> > + break;
> > + case IRQF_TRIGGER_FALLING:
> > + data->trig_active_high = false;
> > + break;
> > + default:
> > + dev_err(data->dev, "Invalid interrupt type specified\n");
> > + return -EINVAL;
> > + }
> > +
> > + data->trig_open_drain = fwnode_property_read_bool(fwnode,
> > + "int-open-drain");
> > +
> > + ret = bmp380_int_config(data);
> > + if (ret)
> > + return ret;
> > +
> > + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> > + indio_dev->name,
> > + iio_device_id(indio_dev));
> > + if (!data->trig)
> > + return -ENOMEM;
> > +
> > + data->trig->ops = &bmp380_trigger_ops;
> > + iio_trigger_set_drvdata(data->trig, data);
> > +
> > + ret = devm_request_threaded_irq(data->dev, irq, NULL,
> > + bmp380_irq_thread_handler, IRQF_ONESHOT,
> > + indio_dev->name, indio_dev);
> > + if (ret) {
> > + dev_err(data->dev, "request irq failed\n");
> > + return ret;
> > + }
> > +
> > + ret = devm_iio_trigger_register(data->dev, data->trig);
> > + if (ret) {
> > + dev_err(data->dev, "iio trigger register failed\n");
> > + return ret;
> > + }
> > +
> > + indio_dev->trig = iio_trigger_get(data->trig);
> > +
> > + return 0;
> > +}
> > +
> > +
>
> one blank line only.
>
Ack.
> > static irqreturn_t bmp380_trigger_handler(int irq, void *p)
> > {
> > struct iio_poll_func *pf = p;
> > @@ -1854,6 +1998,7 @@ const struct bmp280_chip_info bmp380_chip_info = {
> > .wait_conv = bmp380_wait_conv,
> > .preinit = bmp380_preinit,
> >
> > + .trigger_probe = bmp380_trigger_probe,
> > .trigger_handler = bmp380_trigger_handler,
> > };
> > EXPORT_SYMBOL_NS(bmp380_chip_info, IIO_BMP280);
> > @@ -2390,6 +2535,154 @@ static int bmp580_chip_config(struct bmp280_data *data)
> > return 0;
> > }
> >
>
> ...
>
> > +static irqreturn_t bmp580_irq_thread_handler(int irq, void *p)
> > +{
> > + struct iio_dev *indio_dev = p;
> > + struct bmp280_data *data = iio_priv(indio_dev);
> > + unsigned int int_ctrl;
> > + int ret;
> > +
> > + scoped_guard(mutex, &data->lock) {
>
> Indent wrong.
>
Ack.
> > + ret = regmap_read(data->regmap, BMP580_REG_INT_STATUS, &int_ctrl);
> > + if (ret)
> > + return IRQ_NONE;
> > + }
> > +
> > + if (FIELD_GET(BMP580_INT_STATUS_DRDY_MASK, int_ctrl))
> > + iio_trigger_poll_nested(data->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static int bmp580_trigger_probe(struct iio_dev *indio_dev)
> > +{
> ...
>
> > +
> > + data->trig = devm_iio_trigger_alloc(data->dev, "%s-dev%d",
> > + indio_dev->name,
> > + iio_device_id(indio_dev));
> > + if (!data->trig)
> > + return -ENOMEM;
> > +
> > + data->trig->ops = &bmp580_trigger_ops;
> > + iio_trigger_set_drvdata(data->trig, data);
> > +
> > + ret = devm_request_threaded_irq(data->dev, irq, NULL,
> > + bmp580_irq_thread_handler, IRQF_ONESHOT,
> > + indio_dev->name, indio_dev);
> > + if (ret) {
> > + dev_err(data->dev, "request irq failed\n");
>
> Only in probe paths I think, so return dev_err_probe() thoughout these
> trigger setup callbacks.
>
Ack.
>
>
> > +}
>
> > diff --git a/drivers/iio/pressure/bmp280-regmap.c b/drivers/iio/pressure/bmp280-regmap.c
> > index d27d68edd906..cccdf8fc6c09 100644
> > --- a/drivers/iio/pressure/bmp280-regmap.c
> > +++ b/drivers/iio/pressure/bmp280-regmap.c
> > @@ -109,7 +109,7 @@ static bool bmp380_is_writeable_reg(struct device *dev, unsigned int reg)
> > case BMP380_REG_FIFO_WATERMARK_LSB:
> > case BMP380_REG_FIFO_WATERMARK_MSB:
> > case BMP380_REG_POWER_CONTROL:
> > - case BMP380_REG_INT_CONTROL:
> > + case BMP380_REG_INT_CTRL:
>
> Unrelated change. I'm also not sure it's worth making.
>
I did it because this is tha name in the datasheet and it was also helping
with the 80 char limit. But I can leave it as it is, there is no problem.
> > case BMP380_REG_IF_CONFIG:
> > case BMP380_REG_ODR:
> > case BMP380_REG_OSR:
> > diff --git a/drivers/iio/pressure/bmp280.h b/drivers/iio/pressure/bmp280.h
> > index f5d192509d61..754eda367941 100644
> > --- a/drivers/iio/pressure/bmp280.h
> > +++ b/drivers/iio/pressure/bmp280.h
> > @@ -55,8 +55,17 @@
> > #define BMP580_CMD_NVM_WRITE_SEQ_1 0xA0
> > #define BMP580_CMD_SOFT_RESET 0xB6
> >
> > +#define BMP580_INT_STATUS_DRDY_MASK BIT(0)
> > #define BMP580_INT_STATUS_POR_MASK BIT(4)
> >
> > +#define BMP580_INT_SOURCE_DRDY BIT(0)
> > +
> > +#define BMP580_INT_CONFIG_MASK GENMASK(3, 0)
> > +#define BMP580_INT_CONFIG_LATCH BIT(0)
> > +#define BMP580_INT_CONFIG_LEVEL BIT(1)
> > +#define BMP580_INT_CONFIG_OPEN_DRAIN BIT(2)
> > +#define BMP580_INT_CONFIG_INT_EN BIT(3)
> > +
> > #define BMP580_STATUS_CORE_RDY_MASK BIT(0)
> > #define BMP580_STATUS_NVM_RDY_MASK BIT(1)
> > #define BMP580_STATUS_NVM_ERR_MASK BIT(2)
> > @@ -117,7 +126,7 @@
> > #define BMP380_REG_OSR 0x1C
> > #define BMP380_REG_POWER_CONTROL 0x1B
> > #define BMP380_REG_IF_CONFIG 0x1A
> > -#define BMP380_REG_INT_CONTROL 0x19
> > +#define BMP380_REG_INT_CTRL 0x19
> As above.
>
> Jonathan
>
Cheers,
Vasilis
Powered by blists - more mailing lists