| Message ID | 20241117182651.115056-8-l.rubusch@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: accel: adxl345: add FIFO operating with IRQ triggered watermark events | expand |
On Sun, 17 Nov 2024 18:26:36 +0000 Lothar Rubusch <l.rubusch@gmail.com> 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() > > Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> > --- > 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 = {
On Sun, Nov 24, 2024 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > On Sun, 17 Nov 2024 18:26:36 +0000 > Lothar Rubusch <l.rubusch@gmail.com> 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@gmail.com> > > --- > > 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 = { >
On Tue, 26 Nov 2024 17:16:07 +0100 Lothar Rubusch <l.rubusch@gmail.com> wrote: > On Sun, Nov 24, 2024 at 7:14 PM Jonathan Cameron <jic23@kernel.org> wrote: > > > > On Sun, 17 Nov 2024 18:26:36 +0000 > > Lothar Rubusch <l.rubusch@gmail.com> 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? Yes - this function does exactly what you need here (it is a very common situation! Try grepping for INT2 and you should see examples) To fill in the information you need: 1. Try getting INT1 by name. If succeeds use that and set config on basis INT1 is wired. 2. If not try getting INT2. If that succeeds then use INT2. 3. If that fails no interrupts. The binding should 'require' interrupt-names so that you always know which one is which in the interrupt properties. If the binding just had one interrupt before, then we add some text to say that it is only valid if the provided interrupt is INT1. Jonathan
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 * @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); 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 = {
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. Signed-off-by: Lothar Rubusch <l.rubusch@gmail.com> --- 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(-)