On Sun, Nov 24, 2024 at 7:14 PM Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
>
> On Sun, 17 Nov 2024 18:26:36 +0000
> Lothar Rubusch <l.rubusch@xxxxxxxxx> wrote:
>
> > Add the possibility to claim an interrupt and init the state structure
> > with interrupt number and interrupt line to use. The adxl345 can use
> > two different interrupt lines, mainly to signal FIFO watermark events,
> > single or double tap, activity, etc. Hence, having the interrupt line
> > available is crucial to implement such features.
>
> If there are two interrupt lines, you need to be more clever.
> Imagine only one of them is wired. How do you know which one it is?
>
> The query needs to be done by name. When there are multiple interrupts
> the ones found in spi and i2c structures could be anything, so don't use
> those.
>
> See fwnode_irq_get_by_name()
One of my bigger question marks is this here: The sensor has two
possible interrupt lines, INT1 or INT2, on which it will notify. But,
only one is connected. Hence, the irq passed e.g. by SPI will
automatically refer to the used one. Only this one is going to be
configured as irq. Or, am I getting this wrong? Alternatively, no
interrupt line is connected. This was the initial driver setup. In
this case, it's generally BYPASS-mode for the FIFO and any further
feature is not possible due to a lack of notification.
My questions now arise, when it comes to configure the IRQ line in the
sensor registers. The sensor needs to be configured, that the used
interrupt line for notifiction is INT1 or INT2. In the later patch of
this series - "set interrupt line to INT1" - I init it with "INT1".
But, this actually then can be configured. I can think of yet another
/sysfs handle (could be dynamically changed at runtime, I'm not sure
if this makes sense); by an additional devicetree binding (currently
my preferred idea, I'd default to INT1, but let it configure to INT1
or INT2 - or better default to bypass mode?), or is there an IIO way
of configuring it to INT1/2, or a better way? I rather tend to DT. But
this is still not part of this patch set.
Do I miss a point here and should still apply the
fwnode_irq_get_by_name()? This looks a bit like the DT approach, isn't
it?
Lothar
> > Signed-off-by: Lothar Rubusch <l.rubusch@xxxxxxxxx>
> > ---
> > drivers/iio/accel/adxl345.h | 1 +
> > drivers/iio/accel/adxl345_core.c | 6 ++++++
> > drivers/iio/accel/adxl345_i2c.c | 2 +-
> > drivers/iio/accel/adxl345_spi.c | 8 ++++++--
> > 4 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
> > index 3d5c8719db..cf4132715c 100644
> > --- a/drivers/iio/accel/adxl345.h
> > +++ b/drivers/iio/accel/adxl345.h
> > @@ -62,6 +62,7 @@ struct adxl345_chip_info {
> > };
> >
> > int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > + int irq,
> > int (*setup)(struct device*, struct regmap*));
> >
> > #endif /* _ADXL345_H_ */
> > diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> > index 81688a9eaf..902bd3568b 100644
> > --- a/drivers/iio/accel/adxl345_core.c
> > +++ b/drivers/iio/accel/adxl345_core.c
> > @@ -11,6 +11,7 @@
> > #include <linux/property.h>
> > #include <linux/regmap.h>
> > #include <linux/units.h>
> > +#include <linux/interrupt.h>
> >
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > @@ -18,6 +19,7 @@
> > #include "adxl345.h"
> >
> > struct adxl34x_state {
> > + int irq;
> > const struct adxl345_chip_info *info;
> > struct regmap *regmap;
> > };
> > @@ -196,12 +198,14 @@ static const struct iio_info adxl345_info = {
> > * also covers the adxl375 and adxl346 accelerometer
> > * @dev: Driver model representation of the device
> > * @regmap: Regmap instance for the device
> > + * @irq: Interrupt handling for async usage
>
> This is an integer, not a handling. See if you can come up with clearer comment.
>
> > * @setup: Setup routine to be executed right before the standard device
> > * setup
> > *
> > * Return: 0 on success, negative errno on error
> > */
> > int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > + int irq,
> > int (*setup)(struct device*, struct regmap*))
> > {
> > struct adxl34x_state *st;
> > @@ -224,6 +228,8 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> >
> > st = iio_priv(indio_dev);
> > st->regmap = regmap;
> > +
> > + st->irq = irq;
> > st->info = device_get_match_data(dev);
> > if (!st->info)
> > return -ENODEV;
> > diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> > index 4065b8f7c8..604b706c29 100644
> > --- a/drivers/iio/accel/adxl345_i2c.c
> > +++ b/drivers/iio/accel/adxl345_i2c.c
> > @@ -27,7 +27,7 @@ static int adxl345_i2c_probe(struct i2c_client *client)
> > if (IS_ERR(regmap))
> > return dev_err_probe(&client->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> >
> > - return adxl345_core_probe(&client->dev, regmap, NULL);
> > + return adxl345_core_probe(&client->dev, regmap, client->irq, NULL);
> > }
> >
> > static const struct adxl345_chip_info adxl345_i2c_info = {
> > diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
> > index 61fd9a6f5f..39e7d71e1d 100644
> > --- a/drivers/iio/accel/adxl345_spi.c
> > +++ b/drivers/iio/accel/adxl345_spi.c
> > @@ -39,9 +39,13 @@ static int adxl345_spi_probe(struct spi_device *spi)
> > return dev_err_probe(&spi->dev, PTR_ERR(regmap), "Error initializing regmap\n");
> >
> > if (spi->mode & SPI_3WIRE)
> > - return adxl345_core_probe(&spi->dev, regmap, adxl345_spi_setup);
> > + return adxl345_core_probe(&spi->dev, regmap,
> > + spi->irq,
> > + adxl345_spi_setup);
> Very early wrap. I think spi->irq fits on the line above.
>
> > else
> > - return adxl345_core_probe(&spi->dev, regmap, NULL);
> > + return adxl345_core_probe(&spi->dev, regmap,
> > + spi->irq,
> > + NULL);
> > }
> >
> > static const struct adxl345_chip_info adxl345_spi_info = {
>
