* [PATCH 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support
2017-03-13 11:11 [PATCH 0/4] iio: accel: adxl345: Add support for buffered readings Eva Rachel Retuya
@ 2017-03-13 11:11 ` Eva Rachel Retuya
2017-03-20 19:34 ` Rob Herring
2017-03-13 11:11 ` [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions Eva Rachel Retuya
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Eva Rachel Retuya @ 2017-03-13 11:11 UTC (permalink / raw)
To: jic23, linux-iio
Cc: knaack.h, lars, pmeerw, dmitry.torokhov, michael.hennerich,
daniel.baluta, amsfield22, florian.vaussard, linux-kernel,
robh+dt, mark.rutland, devicetree, Eva Rachel Retuya
Add interrupt-names property in order to specify interrupt pin in use.
Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
Documentation/devicetree/bindings/iio/accel/adxl345.txt | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/accel/adxl345.txt b/Documentation/devicetree/bindings/iio/accel/adxl345.txt
index e7111b0..a7c9022 100644
--- a/Documentation/devicetree/bindings/iio/accel/adxl345.txt
+++ b/Documentation/devicetree/bindings/iio/accel/adxl345.txt
@@ -15,6 +15,8 @@ Optional properties:
in Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
- interrupts: interrupt mapping for IRQ as documented in
Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+ - interrupt-names: specify INTx pin in use. Should be "INT1" for INT1 pin or
+ "INT2" for INT2 pin.
Example for a I2C device node:
@@ -23,6 +25,7 @@ Example for a I2C device node:
reg = <0x53>;
interrupt-parent = <&gpio1>;
interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "INT1";
};
Example for a SPI device node:
@@ -35,4 +38,5 @@ Example for a SPI device node:
spi-cpha;
interrupt-parent = <&gpio1>;
interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "INT1";
};
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support
2017-03-13 11:11 ` [PATCH 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support Eva Rachel Retuya
@ 2017-03-20 19:34 ` Rob Herring
0 siblings, 0 replies; 15+ messages in thread
From: Rob Herring @ 2017-03-20 19:34 UTC (permalink / raw)
To: Eva Rachel Retuya
Cc: jic23, linux-iio, knaack.h, lars, pmeerw, dmitry.torokhov,
michael.hennerich, daniel.baluta, amsfield22, florian.vaussard,
linux-kernel, mark.rutland, devicetree
On Mon, Mar 13, 2017 at 07:11:34PM +0800, Eva Rachel Retuya wrote:
> Add interrupt-names property in order to specify interrupt pin in use.
>
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> ---
> Documentation/devicetree/bindings/iio/accel/adxl345.txt | 4 ++++
> 1 file changed, 4 insertions(+)
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions
2017-03-13 11:11 [PATCH 0/4] iio: accel: adxl345: Add support for buffered readings Eva Rachel Retuya
2017-03-13 11:11 ` [PATCH 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support Eva Rachel Retuya
@ 2017-03-13 11:11 ` Eva Rachel Retuya
2017-03-13 11:57 ` Andy Shevchenko
2017-03-15 21:41 ` Jonathan Cameron
2017-03-13 11:11 ` [PATCH 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya
2017-03-13 11:11 ` [PATCH 4/4] iio: accel: adxl345: Add support for triggered buffer Eva Rachel Retuya
3 siblings, 2 replies; 15+ messages in thread
From: Eva Rachel Retuya @ 2017-03-13 11:11 UTC (permalink / raw)
To: jic23, linux-iio
Cc: knaack.h, lars, pmeerw, dmitry.torokhov, michael.hennerich,
daniel.baluta, amsfield22, florian.vaussard, linux-kernel,
robh+dt, mark.rutland, devicetree, Eva Rachel Retuya
Move code that enables measurement/standby mode into its own function
and call that function when appropriate. Previously, we set the sensor
to measurement in probe and back to standby during remove. Change it
here to only enter measurement mode when request for data is initiated.
The DATA_READY bit is always set if the corresponding event occurs.
Introduce the drdy function that monitors the INT_SOURCE register of
this bit. This is done to ensure consistent readings.
Make use of drdy in read_raw and also refactor the read to perform an
all-axes read instead through get_triple. This function will come in
handy when buffered reading is introduced.
Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
drivers/iio/accel/adxl345_core.c | 116 +++++++++++++++++++++++++++++----------
1 file changed, 88 insertions(+), 28 deletions(-)
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 9ccb582..7eee31d 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -8,6 +8,7 @@
* directory of this archive for more details.
*/
+#include <linux/delay.h>
#include <linux/module.h>
#include <linux/regmap.h>
@@ -17,6 +18,7 @@
#define ADXL345_REG_DEVID 0x00
#define ADXL345_REG_POWER_CTL 0x2D
+#define ADXL345_REG_INT_SOURCE 0x30
#define ADXL345_REG_DATA_FORMAT 0x31
#define ADXL345_REG_DATAX0 0x32
#define ADXL345_REG_DATAY0 0x34
@@ -25,6 +27,10 @@
#define ADXL345_POWER_CTL_MEASURE BIT(3)
#define ADXL345_POWER_CTL_STANDBY 0x00
+/* INT_ENABLE/INT_MAP/INT_SOURCE bits */
+#define ADXL345_INT_DATA_READY BIT(7)
+#define ADXL345_INT_OVERRUN 0
+
#define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
#define ADXL345_DATA_FORMAT_2G 0
#define ADXL345_DATA_FORMAT_4G 1
@@ -47,6 +53,58 @@ struct adxl345_data {
u8 data_range;
};
+static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int ret;
+
+ ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
+ if (ret < 0) {
+ dev_err(dev, "Failed to set power mode, %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int adxl345_get_triple(struct adxl345_data *data, void *buf)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int ret;
+
+ ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, buf,
+ sizeof(__le16) * 3);
+ if (ret < 0) {
+ dev_err(dev, "Failed to read the data registers, %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int adxl345_drdy(struct adxl345_data *data)
+{
+ struct device *dev = regmap_get_device(data->regmap);
+ int tries = 5;
+ u32 regval;
+ int ret;
+
+ while (tries--) {
+ ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE,
+ ®val);
+ if (ret < 0)
+ return ret;
+ if ((regval & ADXL345_INT_DATA_READY) ==
+ ADXL345_INT_DATA_READY)
+ return 0;
+
+ msleep(20);
+ }
+ dev_err(dev, "Data is not yet ready\n");
+
+ return -EIO;
+}
+
#define ADXL345_CHANNEL(reg, axis) { \
.type = IIO_ACCEL, \
.modified = 1, \
@@ -67,22 +125,37 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct adxl345_data *data = iio_priv(indio_dev);
- __le16 regval;
+ s16 regval[3];
int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
- /*
- * Data is stored in adjacent registers:
- * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
- * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
- */
- ret = regmap_bulk_read(data->regmap, chan->address, ®val,
- sizeof(regval));
+ ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
+ if (ret < 0)
+ return ret;
+ ret = adxl345_drdy(data);
+ if (ret < 0)
+ return ret;
+
+ ret = adxl345_get_triple(data, ®val);
if (ret < 0)
return ret;
- *val = sign_extend32(le16_to_cpu(regval), 12);
+ switch (chan->address) {
+ case ADXL345_REG_DATAX0:
+ *val = regval[0];
+ break;
+ case ADXL345_REG_DATAY0:
+ *val = regval[1];
+ break;
+ case ADXL345_REG_DATAZ0:
+ *val = regval[2];
+ break;
+ default:
+ return -EINVAL;
+ }
+ adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+
return IIO_VAL_INT;
case IIO_CHAN_INFO_SCALE:
*val = 0;
@@ -126,9 +199,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
data = iio_priv(indio_dev);
dev_set_drvdata(dev, indio_dev);
data->regmap = regmap;
+ /* Make sure sensor is in standby mode before configuring registers */
+ ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+ if (ret < 0)
+ return ret;
/* Enable full-resolution mode */
data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
-
ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
data->data_range);
if (ret < 0) {
@@ -143,22 +219,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
indio_dev->channels = adxl345_channels;
indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
- /* Enable measurement mode */
- ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
- ADXL345_POWER_CTL_MEASURE);
- if (ret < 0) {
- dev_err(dev, "Failed to enable measurement mode: %d\n", ret);
- return ret;
- }
-
- ret = iio_device_register(indio_dev);
- if (ret < 0) {
- dev_err(dev, "iio_device_register failed: %d\n", ret);
- regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
- ADXL345_POWER_CTL_STANDBY);
- }
-
- return ret;
+ return iio_device_register(indio_dev);
}
EXPORT_SYMBOL_GPL(adxl345_core_probe);
@@ -169,8 +230,7 @@ int adxl345_core_remove(struct device *dev)
iio_device_unregister(indio_dev);
- return regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
- ADXL345_POWER_CTL_STANDBY);
+ return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
}
EXPORT_SYMBOL_GPL(adxl345_core_remove);
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions
2017-03-13 11:11 ` [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions Eva Rachel Retuya
@ 2017-03-13 11:57 ` Andy Shevchenko
2017-03-15 21:41 ` Jonathan Cameron
1 sibling, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2017-03-13 11:57 UTC (permalink / raw)
To: Eva Rachel Retuya
Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Dmitry Torokhov, Michael Hennerich,
Daniel Baluta, Alison Schofield, Florian Vaussard, linux-kernel,
Rob Herring, Mark Rutland, devicetree
On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> Move code that enables measurement/standby mode into its own function
> and call that function when appropriate. Previously, we set the sensor
> to measurement in probe and back to standby during remove. Change it
> here to only enter measurement mode when request for data is initiated.
>
> The DATA_READY bit is always set if the corresponding event occurs.
> Introduce the drdy function that monitors the INT_SOURCE register of
> this bit. This is done to ensure consistent readings.
>
> Make use of drdy in read_raw and also refactor the read to perform an
> all-axes read instead through get_triple. This function will come in
> handy when buffered reading is introduced.
> +static int adxl345_get_triple(struct adxl345_data *data, void *buf)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, buf,
> + sizeof(__le16) * 3);
Magic.
> +static int adxl345_drdy(struct adxl345_data *data)
Name is not speaking for itself.
Perhaps
_data_ready(...)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int tries = 5;
> + u32 regval;
> + int ret;
> +
> + while (tries--) {
do {} while pattern a bit better here, since it shows explicitly that
you run the code at least once.
> + ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE,
> + ®val);
If you rename regval to val (or v) it would fit one line I guess.
> + if (ret < 0)
> + return ret;
> + if ((regval & ADXL345_INT_DATA_READY) ==
> + ADXL345_INT_DATA_READY)
> + return 0;
> +
> + msleep(20);
This needs explanation. (It's too long for usual IO delays)
> + }
> + dev_err(dev, "Data is not yet ready\n");
> +
> + return -EIO;
Hmm... Not sure which error code is better here, though EIO is possible.
> @@ -67,22 +125,37 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> int *val, int *val2, long mask)
> {
> struct adxl345_data *data = iio_priv(indio_dev);
> - __le16 regval;
> + s16 regval[3];
I'm not sure this is equivalent change. Have you tried to test this
code on different endianess?
> int ret;
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - /*
> - * Data is stored in adjacent registers:
> - * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
> - * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
> - */
> - ret = regmap_bulk_read(data->regmap, chan->address, ®val,
> - sizeof(regval));
> + ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
> + if (ret < 0)
> + return ret;
+ empty line here.
> + ret = adxl345_drdy(data);
> + if (ret < 0)
> + return ret;
> +
> + ret = adxl345_get_triple(data, ®val);
> if (ret < 0)
> return ret;
>
> - *val = sign_extend32(le16_to_cpu(regval), 12);
> + switch (chan->address) {
> + case ADXL345_REG_DATAX0:
> + *val = regval[0];
> + break;
> + case ADXL345_REG_DATAY0:
> + *val = regval[1];
> + break;
> + case ADXL345_REG_DATAZ0:
> + *val = regval[2];
> + break;
Yes, I'm worrying about endianess here.
> @@ -126,9 +199,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> data = iio_priv(indio_dev);
> dev_set_drvdata(dev, indio_dev);
> data->regmap = regmap;
> + /* Make sure sensor is in standby mode before configuring registers */
> + ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> + if (ret < 0)
> + return ret;
> /* Enable full-resolution mode */
> data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
> -
This doesn't belong to the patch, and perhaps you may put _set_mode()
call here surrounded by empty lines.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions
2017-03-13 11:11 ` [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions Eva Rachel Retuya
2017-03-13 11:57 ` Andy Shevchenko
@ 2017-03-15 21:41 ` Jonathan Cameron
1 sibling, 0 replies; 15+ messages in thread
From: Jonathan Cameron @ 2017-03-15 21:41 UTC (permalink / raw)
To: Eva Rachel Retuya, linux-iio
Cc: knaack.h, lars, pmeerw, dmitry.torokhov, michael.hennerich,
daniel.baluta, amsfield22, florian.vaussard, linux-kernel,
robh+dt, mark.rutland, devicetree
On 13/03/17 11:11, Eva Rachel Retuya wrote:
> Move code that enables measurement/standby mode into its own function
> and call that function when appropriate. Previously, we set the sensor
> to measurement in probe and back to standby during remove. Change it
> here to only enter measurement mode when request for data is initiated.
>
> The DATA_READY bit is always set if the corresponding event occurs.
> Introduce the drdy function that monitors the INT_SOURCE register of
> this bit. This is done to ensure consistent readings.
>
> Make use of drdy in read_raw and also refactor the read to perform an
> all-axes read instead through get_triple. This function will come in
> handy when buffered reading is introduced.
>
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
A few bits from me to add to Andy's review.
> ---
> drivers/iio/accel/adxl345_core.c | 116 +++++++++++++++++++++++++++++----------
> 1 file changed, 88 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
> index 9ccb582..7eee31d 100644
> --- a/drivers/iio/accel/adxl345_core.c
> +++ b/drivers/iio/accel/adxl345_core.c
> @@ -8,6 +8,7 @@
> * directory of this archive for more details.
> */
>
> +#include <linux/delay.h>
> #include <linux/module.h>
> #include <linux/regmap.h>
>
> @@ -17,6 +18,7 @@
>
> #define ADXL345_REG_DEVID 0x00
> #define ADXL345_REG_POWER_CTL 0x2D
> +#define ADXL345_REG_INT_SOURCE 0x30
> #define ADXL345_REG_DATA_FORMAT 0x31
> #define ADXL345_REG_DATAX0 0x32
> #define ADXL345_REG_DATAY0 0x34
> @@ -25,6 +27,10 @@
> #define ADXL345_POWER_CTL_MEASURE BIT(3)
> #define ADXL345_POWER_CTL_STANDBY 0x00
>
> +/* INT_ENABLE/INT_MAP/INT_SOURCE bits */
> +#define ADXL345_INT_DATA_READY BIT(7)
> +#define ADXL345_INT_OVERRUN 0
> +
> #define ADXL345_DATA_FORMAT_FULL_RES BIT(3) /* Up to 13-bits resolution */
> #define ADXL345_DATA_FORMAT_2G 0
> #define ADXL345_DATA_FORMAT_4G 1
> @@ -47,6 +53,58 @@ struct adxl345_data {
> u8 data_range;
> };
>
> +static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret;
> +
> + ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL, mode);
> + if (ret < 0) {
> + dev_err(dev, "Failed to set power mode, %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int adxl345_get_triple(struct adxl345_data *data, void *buf)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, buf,
> + sizeof(__le16) * 3);
> + if (ret < 0) {
> + dev_err(dev, "Failed to read the data registers, %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int adxl345_drdy(struct adxl345_data *data)
> +{
> + struct device *dev = regmap_get_device(data->regmap);
> + int tries = 5;
> + u32 regval;
> + int ret;
> +
> + while (tries--) {
> + ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE,
> + ®val);
> + if (ret < 0)
> + return ret;
> + if ((regval & ADXL345_INT_DATA_READY) ==
> + ADXL345_INT_DATA_READY)
> + return 0;
> +
> + msleep(20);
> + }
> + dev_err(dev, "Data is not yet ready\n");
> +
Hmm. Does it definitely indicate a hardware failure? If so fair enough with
EIO. However, the message should perhaps say this. Sounds like an EBUSY
from that error message.
> + return -EIO;
> +}
> +
> #define ADXL345_CHANNEL(reg, axis) { \
> .type = IIO_ACCEL, \
> .modified = 1, \
> @@ -67,22 +125,37 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
> int *val, int *val2, long mask)
> {
> struct adxl345_data *data = iio_priv(indio_dev);
> - __le16 regval;
> + s16 regval[3];
> int ret;
>
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> - /*
> - * Data is stored in adjacent registers:
> - * ADXL345_REG_DATA(X0/Y0/Z0) contain the least significant byte
> - * and ADXL345_REG_DATA(X0/Y0/Z0) + 1 the most significant byte
> - */
> - ret = regmap_bulk_read(data->regmap, chan->address, ®val,
> - sizeof(regval));
> + ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
> + if (ret < 0)
> + return ret;
> + ret = adxl345_drdy(data);
> + if (ret < 0)
> + return ret;
> +
> + ret = adxl345_get_triple(data, ®val);
> if (ret < 0)
> return ret;
>
> - *val = sign_extend32(le16_to_cpu(regval), 12);
This function should as Andy pointed out still be returning in
cpu endianness. So you can't just drop it like this.
> + switch (chan->address) {
> + case ADXL345_REG_DATAX0:
> + *val = regval[0];
> + break;
> + case ADXL345_REG_DATAY0:
> + *val = regval[1];
> + break;
> + case ADXL345_REG_DATAZ0:
> + *val = regval[2];
> + break;
> + default:
> + return -EINVAL;
> + }
> + adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> +
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_SCALE:
> *val = 0;
> @@ -126,9 +199,12 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> data = iio_priv(indio_dev);
> dev_set_drvdata(dev, indio_dev);
> data->regmap = regmap;
> + /* Make sure sensor is in standby mode before configuring registers */
> + ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> + if (ret < 0)
> + return ret;
Is this one specific to the buffered support? If not, please break it out
as a precursor patch.
> /* Enable full-resolution mode */
> data->data_range = ADXL345_DATA_FORMAT_FULL_RES;
Should be seeing random white space changes in a patch doing other things.
> -
> ret = regmap_write(data->regmap, ADXL345_REG_DATA_FORMAT,
> data->data_range);
> if (ret < 0) {
> @@ -143,22 +219,7 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> indio_dev->channels = adxl345_channels;
> indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
>
> - /* Enable measurement mode */
> - ret = regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
> - ADXL345_POWER_CTL_MEASURE);
> - if (ret < 0) {
> - dev_err(dev, "Failed to enable measurement mode: %d\n", ret);
> - return ret;
> - }
> -
> - ret = iio_device_register(indio_dev);
> - if (ret < 0) {
> - dev_err(dev, "iio_device_register failed: %d\n", ret);
> - regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
> - ADXL345_POWER_CTL_STANDBY);
> - }
> -
> - return ret;
> + return iio_device_register(indio_dev);
> }
> EXPORT_SYMBOL_GPL(adxl345_core_probe);
>
> @@ -169,8 +230,7 @@ int adxl345_core_remove(struct device *dev)
>
> iio_device_unregister(indio_dev);
>
> - return regmap_write(data->regmap, ADXL345_REG_POWER_CTL,
> - ADXL345_POWER_CTL_STANDBY);
> + return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
> }
> EXPORT_SYMBOL_GPL(adxl345_core_remove);
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 3/4] iio: accel: adxl345: Setup DATA_READY trigger
2017-03-13 11:11 [PATCH 0/4] iio: accel: adxl345: Add support for buffered readings Eva Rachel Retuya
2017-03-13 11:11 ` [PATCH 1/4] dt-bindings: iio: accel: adxl345: Add optional interrupt-names support Eva Rachel Retuya
2017-03-13 11:11 ` [PATCH 2/4] iio: accel: adxl345_core: Introduce set_mode, drdy & get_triple functions Eva Rachel Retuya
@ 2017-03-13 11:11 ` Eva Rachel Retuya
2017-03-13 12:12 ` Andy Shevchenko
2017-03-13 11:11 ` [PATCH 4/4] iio: accel: adxl345: Add support for triggered buffer Eva Rachel Retuya
3 siblings, 1 reply; 15+ messages in thread
From: Eva Rachel Retuya @ 2017-03-13 11:11 UTC (permalink / raw)
To: jic23, linux-iio
Cc: knaack.h, lars, pmeerw, dmitry.torokhov, michael.hennerich,
daniel.baluta, amsfield22, florian.vaussard, linux-kernel,
robh+dt, mark.rutland, devicetree, Eva Rachel Retuya
Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
drivers/iio/accel/adxl345.h | 4 +-
drivers/iio/accel/adxl345_core.c | 113 +++++++++++++++++++++++++++++++++++++--
drivers/iio/accel/adxl345_i2c.c | 10 +++-
drivers/iio/accel/adxl345_spi.c | 10 +++-
4 files changed, 130 insertions(+), 7 deletions(-)
diff --git a/drivers/iio/accel/adxl345.h b/drivers/iio/accel/adxl345.h
index c1ddf39..f28a503 100644
--- a/drivers/iio/accel/adxl345.h
+++ b/drivers/iio/accel/adxl345.h
@@ -11,8 +11,8 @@
#ifndef _ADXL345_H_
#define _ADXL345_H_
-int adxl345_core_probe(struct device *dev, struct regmap *regmap,
- const char *name);
+int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
+ const char *name, bool use_int2);
int adxl345_core_remove(struct device *dev);
#endif /* _ADXL345_H_ */
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 7eee31d..4b7055c 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -9,15 +9,19 @@
*/
#include <linux/delay.h>
+#include <linux/interrupt.h>
#include <linux/module.h>
#include <linux/regmap.h>
#include <linux/iio/iio.h>
+#include <linux/iio/trigger.h>
#include "adxl345.h"
#define ADXL345_REG_DEVID 0x00
#define ADXL345_REG_POWER_CTL 0x2D
+#define ADXL345_REG_INT_ENABLE 0x2E
+#define ADXL345_REG_INT_MAP 0x2F
#define ADXL345_REG_INT_SOURCE 0x30
#define ADXL345_REG_DATA_FORMAT 0x31
#define ADXL345_REG_DATAX0 0x32
@@ -39,6 +43,8 @@
#define ADXL345_DEVID 0xE5
+#define ADXL345_IRQ_NAME "adxl345_event"
+
/*
* In full-resolution mode, scale factor is maintained at ~4 mg/LSB
* in all g ranges.
@@ -49,7 +55,9 @@
static const int adxl345_uscale = 38300;
struct adxl345_data {
+ struct iio_trigger *drdy_trig;
struct regmap *regmap;
+ bool drdy_trig_on;
u8 data_range;
};
@@ -167,13 +175,57 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
return -EINVAL;
}
+static irqreturn_t adxl345_irq(int irq, void *p)
+{
+ struct iio_dev *indio_dev = p;
+ struct adxl345_data *data = iio_priv(indio_dev);
+ int ret = IRQ_NONE;
+ u32 int_stat;
+
+ ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
+ if (ret < 0)
+ return ret;
+
+ if (int_stat & ADXL345_INT_DATA_READY) {
+ iio_trigger_poll(data->drdy_trig);
+ ret = IRQ_HANDLED;
+ }
+
+ return ret;
+}
+
+static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
+{
+ struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+ struct adxl345_data *data = iio_priv(indio_dev);
+ struct device *dev;
+ int ret;
+
+ dev = regmap_get_device(data->regmap);
+ ret = regmap_update_bits(data->regmap, ADXL345_REG_INT_ENABLE,
+ ADXL345_INT_DATA_READY, (state ?
+ ADXL345_INT_DATA_READY : 0));
+ if (ret < 0) {
+ dev_err(dev, "Failed to update INT_ENABLE bits\n");
+ return ret;
+ }
+ data->drdy_trig_on = state;
+
+ return ret;
+}
+
+static const struct iio_trigger_ops adxl345_trigger_ops = {
+ .owner = THIS_MODULE,
+ .set_trigger_state = adxl345_drdy_trigger_set_state,
+};
+
static const struct iio_info adxl345_info = {
.driver_module = THIS_MODULE,
.read_raw = adxl345_read_raw,
};
-int adxl345_core_probe(struct device *dev, struct regmap *regmap,
- const char *name)
+int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
+ const char *name, bool use_int2)
{
struct adxl345_data *data;
struct iio_dev *indio_dev;
@@ -212,6 +264,20 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
return ret;
}
+ /*
+ * Any bits set to 0 send their respective interrupts to the INT1 pin,
+ * whereas bits set to 1 send their respective interrupts to the INT2
+ * pin. Map all interrupts to the specified pin.
+ */
+ if (!use_int2)
+ ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0x00);
+ else
+ ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0xFF);
+ if (ret < 0) {
+ dev_err(dev, "Failed to set up interrupts: %d\n", ret);
+ return ret;
+ }
+
indio_dev->dev.parent = dev;
indio_dev->name = name;
indio_dev->info = &adxl345_info;
@@ -219,7 +285,46 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap,
indio_dev->channels = adxl345_channels;
indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
- return iio_device_register(indio_dev);
+ if (irq > 0) {
+ ret =
+ devm_request_threaded_irq(dev, irq, NULL, adxl345_irq,
+ IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+ ADXL345_IRQ_NAME, indio_dev);
+ if (ret < 0) {
+ dev_err(dev, "Failed to request irq: %d\n", irq);
+ return ret;
+ }
+
+ data->drdy_trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
+ indio_dev->name,
+ indio_dev->id);
+ if (!data->drdy_trig)
+ return -ENOMEM;
+
+ data->drdy_trig->dev.parent = dev;
+ data->drdy_trig->ops = &adxl345_trigger_ops;
+ iio_trigger_set_drvdata(data->drdy_trig, indio_dev);
+
+ ret = iio_trigger_register(data->drdy_trig);
+ if (ret) {
+ dev_err(dev, "Failed to register trigger: %d\n", ret);
+ return ret;
+ }
+ }
+
+ ret = iio_device_register(indio_dev);
+ if (ret < 0) {
+ dev_err(dev, "iio_device_register failed: %d\n", ret);
+ goto err_trigger_unregister;
+ }
+
+ return ret;
+
+err_trigger_unregister:
+ if (data->drdy_trig)
+ iio_trigger_unregister(data->drdy_trig);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(adxl345_core_probe);
@@ -229,6 +334,8 @@ int adxl345_core_remove(struct device *dev)
struct adxl345_data *data = iio_priv(indio_dev);
iio_device_unregister(indio_dev);
+ if (data->drdy_trig)
+ iio_trigger_unregister(data->drdy_trig);
return adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
}
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index 05e1ec4..8c791b8 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -13,6 +13,7 @@
#include <linux/i2c.h>
#include <linux/module.h>
+#include <linux/of_irq.h>
#include <linux/regmap.h>
#include "adxl345.h"
@@ -26,6 +27,8 @@ static int adxl345_i2c_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
struct regmap *regmap;
+ bool use_int2 = false;
+ int irq;
regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
if (IS_ERR(regmap)) {
@@ -34,7 +37,12 @@ static int adxl345_i2c_probe(struct i2c_client *client,
return PTR_ERR(regmap);
}
- return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
+ irq = of_irq_get_byname(client->dev.of_node, "INT2");
+ if (irq == client->irq)
+ use_int2 = true;
+
+ return adxl345_core_probe(&client->dev, regmap, client->irq,
+ id ? id->name : NULL, use_int2);
}
static int adxl345_i2c_remove(struct i2c_client *client)
diff --git a/drivers/iio/accel/adxl345_spi.c b/drivers/iio/accel/adxl345_spi.c
index 6d65819..731d675 100644
--- a/drivers/iio/accel/adxl345_spi.c
+++ b/drivers/iio/accel/adxl345_spi.c
@@ -9,6 +9,7 @@
*/
#include <linux/module.h>
+#include <linux/of_irq.h>
#include <linux/regmap.h>
#include <linux/spi/spi.h>
@@ -27,6 +28,8 @@ static int adxl345_spi_probe(struct spi_device *spi)
{
const struct spi_device_id *id = spi_get_device_id(spi);
struct regmap *regmap;
+ bool use_int2 = false;
+ int irq;
/* Bail out if max_speed_hz exceeds 5 MHz */
if (spi->max_speed_hz > ADXL345_MAX_SPI_FREQ_HZ) {
@@ -42,7 +45,12 @@ static int adxl345_spi_probe(struct spi_device *spi)
return PTR_ERR(regmap);
}
- return adxl345_core_probe(&spi->dev, regmap, id->name);
+ irq = of_irq_get_byname(spi->dev.of_node, "INT2");
+ if (irq == spi->irq)
+ use_int2 = true;
+
+ return adxl345_core_probe(&spi->dev, regmap, spi->irq, id->name,
+ use_int2);
}
static int adxl345_spi_remove(struct spi_device *spi)
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] iio: accel: adxl345: Setup DATA_READY trigger
2017-03-13 11:11 ` [PATCH 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya
@ 2017-03-13 12:12 ` Andy Shevchenko
2017-03-15 9:49 ` Eva Rachel Retuya
0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2017-03-13 12:12 UTC (permalink / raw)
To: Eva Rachel Retuya
Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Dmitry Torokhov, Michael Hennerich,
Daniel Baluta, Alison Schofield, Florian Vaussard, linux-kernel,
Rob Herring, Mark Rutland, devicetree
On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
Missed commit message is no-no!
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
> -int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> - const char *name);
> +int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
> + const char *name, bool use_int2);
Hmm... And tomorrow you will add another flag and another.
No, consider to use something like
struct adxl345_chip {
struct device *dev;
struct regmap *regmap;
const char *name;
}
Convert your probe to use it, and after extend for your needs.
> #define ADXL345_DEVID 0xE5
>
> +#define ADXL345_IRQ_NAME "adxl345_event"
> struct adxl345_data {
> + struct iio_trigger *drdy_trig;
> struct regmap *regmap;
> + bool drdy_trig_on;
> u8 data_range;
drdy -> data_ready
> +static irqreturn_t adxl345_irq(int irq, void *p)
> +{
> + struct iio_dev *indio_dev = p;
> + struct adxl345_data *data = iio_priv(indio_dev);
> + int ret = IRQ_NONE;
> + u32 int_stat;
> +
> + ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
> + if (ret < 0)
> + return ret;
It makes little sense AFAIU.
> +
> + if (int_stat & ADXL345_INT_DATA_READY) {
> + iio_trigger_poll(data->drdy_trig);
> + ret = IRQ_HANDLED;
> + }
> +
> + return ret;
Useless variable ret. You may return values directly.
> +}
> +
> +static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
> +{
> + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> + struct adxl345_data *data = iio_priv(indio_dev);
> + struct device *dev;
> + int ret;
> +
> + dev = regmap_get_device(data->regmap);
This may be moved to definition block.
> + ret = regmap_update_bits(data->regmap, ADXL345_REG_INT_ENABLE,
> + ADXL345_INT_DATA_READY, (state ?
> + ADXL345_INT_DATA_READY : 0));
No way:
Don't split lines like this.
Remove extra parens.
> +static const struct iio_trigger_ops adxl345_trigger_ops = {
> + .owner = THIS_MODULE,
I dunno if we still need this.
> static const struct iio_info adxl345_info = {
> .driver_module = THIS_MODULE,
Ditto.
> .read_raw = adxl345_read_raw,
> };
> + /*
> + * Any bits set to 0 send their respective interrupts to the INT1 pin,
> + * whereas bits set to 1 send their respective interrupts to the INT2
> + * pin. Map all interrupts to the specified pin.
> + */
> + if (!use_int2)
> + ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0x00);
> + else
> + ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0xFF);
I would create a temporary variable to hold the value and call
regmap_write() unconditionally.
> - return iio_device_register(indio_dev);
You are not supposed to ping-pong changes in your series. Make clear
your goal either you do like above or like below. If you choose
latter, don't alter it in previous patch.
> + if (irq > 0) {
> + ret =
> + devm_request_threaded_irq(dev, irq, NULL, adxl345_irq,
Don't split lines like this.
> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
Are you sure you have threaded IRQ handler?
> + ret = iio_device_register(indio_dev);
> + if (ret < 0) {
> + dev_err(dev, "iio_device_register failed: %d\n", ret);
> + goto err_trigger_unregister;
> + }
> +
> + return ret;
> +err_trigger_unregister:
> + if (data->drdy_trig)
> + iio_trigger_unregister(data->drdy_trig);
> +
> + return ret;
So, doesn't devm_iio_*() provide a facility to avoid this?
> @@ -229,6 +334,8 @@ int adxl345_core_remove(struct device *dev)
> struct adxl345_data *data = iio_priv(indio_dev);
>
> iio_device_unregister(indio_dev);
> + if (data->drdy_trig)
> + iio_trigger_unregister(data->drdy_trig);
Ditto.
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> - return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
> + irq = of_irq_get_byname(client->dev.of_node, "INT2");
> + if (irq == client->irq)
> + use_int2 = true;
Can't you use platform_get_irq() instead?
> - return adxl345_core_probe(&spi->dev, regmap, id->name);
> + irq = of_irq_get_byname(spi->dev.of_node, "INT2");
> + if (irq == spi->irq)
> + use_int2 = true;
Ditto.
P.S. Are you doing this stuff on your own or you are working for some
company? If the latter applies, please, consider to do *internal*
review first.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] iio: accel: adxl345: Setup DATA_READY trigger
2017-03-13 12:12 ` Andy Shevchenko
@ 2017-03-15 9:49 ` Eva Rachel Retuya
2017-03-15 11:06 ` Andy Shevchenko
0 siblings, 1 reply; 15+ messages in thread
From: Eva Rachel Retuya @ 2017-03-15 9:49 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Dmitry Torokhov, Michael Hennerich,
Daniel Baluta, Alison Schofield, Florian Vaussard, linux-kernel,
Rob Herring, Mark Rutland, devicetree
On Mon, Mar 13, 2017 at 02:12:54PM +0200, Andy Shevchenko wrote:
> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
>
Hello Andy,
Thanks for the review.
> Missed commit message is no-no!
>
> > Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>
> > -int adxl345_core_probe(struct device *dev, struct regmap *regmap,
> > - const char *name);
> > +int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
> > + const char *name, bool use_int2);
>
> Hmm... And tomorrow you will add another flag and another.
>
> No, consider to use something like
>
> struct adxl345_chip {
> struct device *dev;
> struct regmap *regmap;
> const char *name;
> }
>
> Convert your probe to use it, and after extend for your needs.
>
> > #define ADXL345_DEVID 0xE5
> >
> > +#define ADXL345_IRQ_NAME "adxl345_event"
>
>
> > struct adxl345_data {
> > + struct iio_trigger *drdy_trig;
> > struct regmap *regmap;
> > + bool drdy_trig_on;
> > u8 data_range;
>
> drdy -> data_ready
>
> > +static irqreturn_t adxl345_irq(int irq, void *p)
> > +{
> > + struct iio_dev *indio_dev = p;
> > + struct adxl345_data *data = iio_priv(indio_dev);
> > + int ret = IRQ_NONE;
> > + u32 int_stat;
> > +
>
> > + ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
>
> > + if (ret < 0)
> > + return ret;
>
> It makes little sense AFAIU.
>
Can you please elaborate further your comment regarding this?
> > +
> > + if (int_stat & ADXL345_INT_DATA_READY) {
> > + iio_trigger_poll(data->drdy_trig);
> > + ret = IRQ_HANDLED;
> > + }
> > +
> > + return ret;
>
> Useless variable ret. You may return values directly.
>
> > +}
> > +
> > +static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
> > +{
> > + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
> > + struct adxl345_data *data = iio_priv(indio_dev);
> > + struct device *dev;
> > + int ret;
> > +
>
> > + dev = regmap_get_device(data->regmap);
>
> This may be moved to definition block.
>
> > + ret = regmap_update_bits(data->regmap, ADXL345_REG_INT_ENABLE,
>
> > + ADXL345_INT_DATA_READY, (state ?
> > + ADXL345_INT_DATA_READY : 0));
>
> No way:
> Don't split lines like this.
> Remove extra parens.
>
> > +static const struct iio_trigger_ops adxl345_trigger_ops = {
>
> > + .owner = THIS_MODULE,
>
> I dunno if we still need this.
>
> > static const struct iio_info adxl345_info = {
>
> > .driver_module = THIS_MODULE,
>
> Ditto.
>
> > .read_raw = adxl345_read_raw,
> > };
>
> > + /*
> > + * Any bits set to 0 send their respective interrupts to the INT1 pin,
> > + * whereas bits set to 1 send their respective interrupts to the INT2
> > + * pin. Map all interrupts to the specified pin.
> > + */
>
> > + if (!use_int2)
> > + ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0x00);
> > + else
> > + ret = regmap_write(data->regmap, ADXL345_REG_INT_MAP, 0xFF);
>
> I would create a temporary variable to hold the value and call
> regmap_write() unconditionally.
>
>
> > - return iio_device_register(indio_dev);
>
> You are not supposed to ping-pong changes in your series. Make clear
> your goal either you do like above or like below. If you choose
> latter, don't alter it in previous patch.
>
Ack. Will revise carefully with consideration to your comments in this
whole series.
> > + if (irq > 0) {
>
> > + ret =
> > + devm_request_threaded_irq(dev, irq, NULL, adxl345_irq,
>
> Don't split lines like this.
>
> > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
>
> Are you sure you have threaded IRQ handler?
>
> > + ret = iio_device_register(indio_dev);
> > + if (ret < 0) {
> > + dev_err(dev, "iio_device_register failed: %d\n", ret);
> > + goto err_trigger_unregister;
> > + }
> > +
> > + return ret;
>
> > +err_trigger_unregister:
> > + if (data->drdy_trig)
> > + iio_trigger_unregister(data->drdy_trig);
> > +
> > + return ret;
>
> So, doesn't devm_iio_*() provide a facility to avoid this?
>
> > @@ -229,6 +334,8 @@ int adxl345_core_remove(struct device *dev)
> > struct adxl345_data *data = iio_priv(indio_dev);
> >
> > iio_device_unregister(indio_dev);
> > + if (data->drdy_trig)
> > + iio_trigger_unregister(data->drdy_trig);
>
> Ditto.
>
> > --- a/drivers/iio/accel/adxl345_i2c.c
> > +++ b/drivers/iio/accel/adxl345_i2c.c
>
> > - return adxl345_core_probe(&client->dev, regmap, id ? id->name : NULL);
> > + irq = of_irq_get_byname(client->dev.of_node, "INT2");
> > + if (irq == client->irq)
> > + use_int2 = true;
>
> Can't you use platform_get_irq() instead?
>
>
> > - return adxl345_core_probe(&spi->dev, regmap, id->name);
> > + irq = of_irq_get_byname(spi->dev.of_node, "INT2");
> > + if (irq == spi->irq)
> > + use_int2 = true;
>
> Ditto.
>
> P.S. Are you doing this stuff on your own or you are working for some
> company? If the latter applies, please, consider to do *internal*
> review first.
>
I'm doing it on my own as a project for Outreachy and have mentors to ask
for advice and/or questions.
Eva
> --
> With Best Regards,
> Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH 3/4] iio: accel: adxl345: Setup DATA_READY trigger
2017-03-15 9:49 ` Eva Rachel Retuya
@ 2017-03-15 11:06 ` Andy Shevchenko
0 siblings, 0 replies; 15+ messages in thread
From: Andy Shevchenko @ 2017-03-15 11:06 UTC (permalink / raw)
To: Andy Shevchenko, Jonathan Cameron, linux-iio, Hartmut Knaack,
Lars-Peter Clausen, Peter Meerwald, Dmitry Torokhov,
Michael Hennerich, Daniel Baluta, Alison Schofield,
Florian Vaussard, linux-kernel, Rob Herring, Mark Rutland,
devicetree
On Wed, Mar 15, 2017 at 11:49 AM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> On Mon, Mar 13, 2017 at 02:12:54PM +0200, Andy Shevchenko wrote:
>> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> Hello Andy,
> Thanks for the review.
You are welcome!
>> > +static irqreturn_t adxl345_irq(int irq, void *p)
>> > +{
>> > + struct iio_dev *indio_dev = p;
>> > + struct adxl345_data *data = iio_priv(indio_dev);
>> > + int ret = IRQ_NONE;
>> > + u32 int_stat;
>> > +
>>
>> > + ret = regmap_read(data->regmap, ADXL345_REG_INT_SOURCE, &int_stat);
>>
>> > + if (ret < 0)
>> > + return ret;
>>
>> It makes little sense AFAIU.
>>
>
> Can you please elaborate further your comment regarding this?
It's defined as enum and returned value is not in a range.
Also it would be useful to look at kernel/irq/manage.c for the
implementation details.
>> P.S. Are you doing this stuff on your own or you are working for some
>> company? If the latter applies, please, consider to do *internal*
>> review first.
> I'm doing it on my own as a project for Outreachy and have mentors to ask
> for advice and/or questions.
Ah, okay, that is fine then!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 4/4] iio: accel: adxl345: Add support for triggered buffer
2017-03-13 11:11 [PATCH 0/4] iio: accel: adxl345: Add support for buffered readings Eva Rachel Retuya
` (2 preceding siblings ...)
2017-03-13 11:11 ` [PATCH 3/4] iio: accel: adxl345: Setup DATA_READY trigger Eva Rachel Retuya
@ 2017-03-13 11:11 ` Eva Rachel Retuya
2017-03-13 12:16 ` Andy Shevchenko
3 siblings, 1 reply; 15+ messages in thread
From: Eva Rachel Retuya @ 2017-03-13 11:11 UTC (permalink / raw)
To: jic23, linux-iio
Cc: knaack.h, lars, pmeerw, dmitry.torokhov, michael.hennerich,
daniel.baluta, amsfield22, florian.vaussard, linux-kernel,
robh+dt, mark.rutland, devicetree, Eva Rachel Retuya
Provide an all-axes read for triggered buffering.
Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
drivers/iio/accel/Kconfig | 2 +
drivers/iio/accel/adxl345_core.c | 124 +++++++++++++++++++++++++++++++++++----
drivers/iio/accel/adxl345_i2c.c | 4 ++
3 files changed, 119 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index 15de262..45092da 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -7,6 +7,8 @@ menu "Accelerometers"
config ADXL345
tristate
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
config ADXL345_I2C
tristate "Analog Devices ADXL345 3-Axis Digital Accelerometer I2C Driver"
diff --git a/drivers/iio/accel/adxl345_core.c b/drivers/iio/accel/adxl345_core.c
index 4b7055c..5df1e73 100644
--- a/drivers/iio/accel/adxl345_core.c
+++ b/drivers/iio/accel/adxl345_core.c
@@ -13,8 +13,11 @@
#include <linux/module.h>
#include <linux/regmap.h>
+#include <linux/iio/buffer.h>
#include <linux/iio/iio.h>
#include <linux/iio/trigger.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
#include "adxl345.h"
@@ -54,11 +57,20 @@
*/
static const int adxl345_uscale = 38300;
+enum adxl345_scan_index {
+ ADXL345_IDX_X,
+ ADXL345_IDX_Y,
+ ADXL345_IDX_Z,
+ ADXL345_IDX_TSTAMP,
+};
+
struct adxl345_data {
struct iio_trigger *drdy_trig;
struct regmap *regmap;
+ struct mutex lock; /* protect this data structure */
bool drdy_trig_on;
u8 data_range;
+ s16 buffer[8]; /* 3 x 16-bit channels + padding + 64-bit timestamp */
};
static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
@@ -75,12 +87,12 @@ static int adxl345_set_mode(struct adxl345_data *data, u8 mode)
return 0;
}
-static int adxl345_get_triple(struct adxl345_data *data, void *buf)
+static int adxl345_get_triple(struct adxl345_data *data)
{
struct device *dev = regmap_get_device(data->regmap);
int ret;
- ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, buf,
+ ret = regmap_bulk_read(data->regmap, ADXL345_REG_DATAX0, data->buffer,
sizeof(__le16) * 3);
if (ret < 0) {
dev_err(dev, "Failed to read the data registers, %d\n", ret);
@@ -120,12 +132,25 @@ static int adxl345_drdy(struct adxl345_data *data)
.address = reg, \
.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .scan_index = ADXL345_IDX_##axis, \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = 13, \
+ .storagebits = 16, \
+ .endianness = IIO_LE, \
+ }, \
}
static const struct iio_chan_spec adxl345_channels[] = {
ADXL345_CHANNEL(ADXL345_REG_DATAX0, X),
ADXL345_CHANNEL(ADXL345_REG_DATAY0, Y),
ADXL345_CHANNEL(ADXL345_REG_DATAZ0, Z),
+ IIO_CHAN_SOFT_TIMESTAMP(ADXL345_IDX_TSTAMP),
+};
+
+static const unsigned long adxl345_scan_masks[] = {
+ BIT(ADXL345_IDX_X) | BIT(ADXL345_IDX_Y) | BIT(ADXL345_IDX_Z),
+ 0
};
static int adxl345_read_raw(struct iio_dev *indio_dev,
@@ -133,31 +158,44 @@ static int adxl345_read_raw(struct iio_dev *indio_dev,
int *val, int *val2, long mask)
{
struct adxl345_data *data = iio_priv(indio_dev);
- s16 regval[3];
int ret;
switch (mask) {
case IIO_CHAN_INFO_RAW:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ mutex_lock(&data->lock);
ret = adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
- if (ret < 0)
+ if (ret < 0) {
+ mutex_unlock(&data->lock);
return ret;
+ }
ret = adxl345_drdy(data);
- if (ret < 0)
+ if (ret < 0) {
+ adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+ mutex_unlock(&data->lock);
return ret;
+ }
- ret = adxl345_get_triple(data, ®val);
- if (ret < 0)
+ ret = adxl345_get_triple(data);
+ mutex_unlock(&data->lock);
+ iio_device_release_direct_mode(indio_dev);
+ if (ret < 0) {
+ adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
return ret;
+ }
switch (chan->address) {
case ADXL345_REG_DATAX0:
- *val = regval[0];
+ *val = data->buffer[ADXL345_IDX_X];
break;
case ADXL345_REG_DATAY0:
- *val = regval[1];
+ *val = data->buffer[ADXL345_IDX_Y];
break;
case ADXL345_REG_DATAZ0:
- *val = regval[2];
+ *val = data->buffer[ADXL345_IDX_Z];
break;
default:
return -EINVAL;
@@ -194,6 +232,56 @@ static irqreturn_t adxl345_irq(int irq, void *p)
return ret;
}
+static irqreturn_t adxl345_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct adxl345_data *data = iio_priv(indio_dev);
+ int ret;
+
+ mutex_lock(&data->lock);
+ ret = adxl345_get_triple(data);
+ mutex_unlock(&data->lock);
+ if (ret < 0)
+ goto error;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, data->buffer,
+ pf->timestamp);
+error:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int adxl345_triggered_buffer_postenable(struct iio_dev *indio_dev)
+{
+ struct adxl345_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = iio_triggered_buffer_postenable(indio_dev);
+ if (ret)
+ return ret;
+
+ return adxl345_set_mode(data, ADXL345_POWER_CTL_MEASURE);
+}
+
+static int adxl345_triggered_buffer_predisable(struct iio_dev *indio_dev)
+{
+ struct adxl345_data *data = iio_priv(indio_dev);
+ int ret;
+
+ ret = adxl345_set_mode(data, ADXL345_POWER_CTL_STANDBY);
+ if (ret)
+ return ret;
+
+ return iio_triggered_buffer_predisable(indio_dev);
+}
+
+static const struct iio_buffer_setup_ops adxl345_buffer_setup_ops = {
+ .postenable = adxl345_triggered_buffer_postenable,
+ .predisable = adxl345_triggered_buffer_predisable,
+};
+
static int adxl345_drdy_trigger_set_state(struct iio_trigger *trig, bool state)
{
struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
@@ -278,12 +366,15 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
return ret;
}
+ mutex_init(&data->lock);
+
indio_dev->dev.parent = dev;
indio_dev->name = name;
indio_dev->info = &adxl345_info;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = adxl345_channels;
indio_dev->num_channels = ARRAY_SIZE(adxl345_channels);
+ indio_dev->available_scan_masks = adxl345_scan_masks;
if (irq > 0) {
ret =
@@ -312,14 +403,24 @@ int adxl345_core_probe(struct device *dev, struct regmap *regmap, int irq,
}
}
+ ret = iio_triggered_buffer_setup(indio_dev, iio_pollfunc_store_time,
+ adxl345_trigger_handler,
+ &adxl345_buffer_setup_ops);
+ if (ret < 0) {
+ dev_err(dev, "iio_triggered_buffer_setup failed: %d\n", ret);
+ goto err_trigger_unregister;
+ }
+
ret = iio_device_register(indio_dev);
if (ret < 0) {
dev_err(dev, "iio_device_register failed: %d\n", ret);
- goto err_trigger_unregister;
+ goto err_buffer_cleanup;
}
return ret;
+err_buffer_cleanup:
+ iio_triggered_buffer_cleanup(indio_dev);
err_trigger_unregister:
if (data->drdy_trig)
iio_trigger_unregister(data->drdy_trig);
@@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
struct adxl345_data *data = iio_priv(indio_dev);
iio_device_unregister(indio_dev);
+ iio_triggered_buffer_cleanup(indio_dev);
if (data->drdy_trig)
iio_trigger_unregister(data->drdy_trig);
diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
index 8c791b8..1e0f071 100644
--- a/drivers/iio/accel/adxl345_i2c.c
+++ b/drivers/iio/accel/adxl345_i2c.c
@@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
bool use_int2 = false;
int irq;
+ if (!i2c_check_functionality(client->adapter,
+ I2C_FUNC_SMBUS_READ_I2C_BLOCK))
+ return -EOPNOTSUPP;
+
regmap = devm_regmap_init_i2c(client, &adxl345_i2c_regmap_config);
if (IS_ERR(regmap)) {
dev_err(&client->dev, "Error initializing i2c regmap: %ld\n",
--
2.7.4
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [PATCH 4/4] iio: accel: adxl345: Add support for triggered buffer
2017-03-13 11:11 ` [PATCH 4/4] iio: accel: adxl345: Add support for triggered buffer Eva Rachel Retuya
@ 2017-03-13 12:16 ` Andy Shevchenko
2017-03-15 21:50 ` Jonathan Cameron
0 siblings, 1 reply; 15+ messages in thread
From: Andy Shevchenko @ 2017-03-13 12:16 UTC (permalink / raw)
To: Eva Rachel Retuya
Cc: Jonathan Cameron, linux-iio, Hartmut Knaack, Lars-Peter Clausen,
Peter Meerwald, Dmitry Torokhov, Michael Hennerich,
Daniel Baluta, Alison Schofield, Florian Vaussard, linux-kernel,
Rob Herring, Mark Rutland, devicetree
On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> Provide an all-axes read for triggered buffering.
Better description is needed.
> -static int adxl345_get_triple(struct adxl345_data *data, void *buf)
> +static int adxl345_get_triple(struct adxl345_data *data)
Ping-ponging again. This should be essentially a change in the patch
where you introduce a helper.
> +err_buffer_cleanup:
> + iio_triggered_buffer_cleanup(indio_dev);
> err_trigger_unregister:
> if (data->drdy_trig)
> iio_trigger_unregister(data->drdy_trig);
devm_iio_*() ?
> @@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
> struct adxl345_data *data = iio_priv(indio_dev);
>
> iio_device_unregister(indio_dev);
> + iio_triggered_buffer_cleanup(indio_dev);
> if (data->drdy_trig)
> iio_trigger_unregister(data->drdy_trig);
Ditto.
> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> index 8c791b8..1e0f071 100644
> --- a/drivers/iio/accel/adxl345_i2c.c
> +++ b/drivers/iio/accel/adxl345_i2c.c
> @@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
> bool use_int2 = false;
> int irq;
>
> + if (!i2c_check_functionality(client->adapter,
> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> + return -EOPNOTSUPP;
And if driver works before, you make a regression here.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] iio: accel: adxl345: Add support for triggered buffer
2017-03-13 12:16 ` Andy Shevchenko
@ 2017-03-15 21:50 ` Jonathan Cameron
2017-03-20 19:46 ` Lars-Peter Clausen
0 siblings, 1 reply; 15+ messages in thread
From: Jonathan Cameron @ 2017-03-15 21:50 UTC (permalink / raw)
To: Andy Shevchenko, Eva Rachel Retuya
Cc: linux-iio, Hartmut Knaack, Lars-Peter Clausen, Peter Meerwald,
Dmitry Torokhov, Michael Hennerich, Daniel Baluta,
Alison Schofield, Florian Vaussard, linux-kernel, Rob Herring,
Mark Rutland, devicetree
On 13/03/17 12:16, Andy Shevchenko wrote:
> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
>> Provide an all-axes read for triggered buffering.
>
> Better description is needed.
>
>> -static int adxl345_get_triple(struct adxl345_data *data, void *buf)
>> +static int adxl345_get_triple(struct adxl345_data *data)
>
> Ping-ponging again. This should be essentially a change in the patch
> where you introduce a helper.
>
>> +err_buffer_cleanup:
>> + iio_triggered_buffer_cleanup(indio_dev);
>> err_trigger_unregister:
>> if (data->drdy_trig)
>> iio_trigger_unregister(data->drdy_trig);
>
> devm_iio_*() ?
>
>> @@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
>> struct adxl345_data *data = iio_priv(indio_dev);
>>
>> iio_device_unregister(indio_dev);
>> + iio_triggered_buffer_cleanup(indio_dev);
>> if (data->drdy_trig)
>> iio_trigger_unregister(data->drdy_trig);
>
> Ditto.
>
>> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
>> index 8c791b8..1e0f071 100644
>> --- a/drivers/iio/accel/adxl345_i2c.c
>> +++ b/drivers/iio/accel/adxl345_i2c.c
>> @@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
>> bool use_int2 = false;
>> int irq;
>>
>> + if (!i2c_check_functionality(client->adapter,
>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
>> + return -EOPNOTSUPP;
>
> And if driver works before, you make a regression here.
Absolutely. Need a fallback if it's not supported...
Jonathan
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] iio: accel: adxl345: Add support for triggered buffer
2017-03-15 21:50 ` Jonathan Cameron
@ 2017-03-20 19:46 ` Lars-Peter Clausen
2017-03-21 9:56 ` Eva Rachel Retuya
0 siblings, 1 reply; 15+ messages in thread
From: Lars-Peter Clausen @ 2017-03-20 19:46 UTC (permalink / raw)
To: Jonathan Cameron, Andy Shevchenko, Eva Rachel Retuya
Cc: linux-iio, Hartmut Knaack, Peter Meerwald, Dmitry Torokhov,
Michael Hennerich, Daniel Baluta, Alison Schofield,
Florian Vaussard, linux-kernel, Rob Herring, Mark Rutland,
devicetree
On 03/15/2017 10:50 PM, Jonathan Cameron wrote:
> On 13/03/17 12:16, Andy Shevchenko wrote:
>> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
>>> Provide an all-axes read for triggered buffering.
>>
>> Better description is needed.
>>
>>> -static int adxl345_get_triple(struct adxl345_data *data, void *buf)
>>> +static int adxl345_get_triple(struct adxl345_data *data)
>>
>> Ping-ponging again. This should be essentially a change in the patch
>> where you introduce a helper.
>>
>>> +err_buffer_cleanup:
>>> + iio_triggered_buffer_cleanup(indio_dev);
>>> err_trigger_unregister:
>>> if (data->drdy_trig)
>>> iio_trigger_unregister(data->drdy_trig);
>>
>> devm_iio_*() ?
>>
>>> @@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
>>> struct adxl345_data *data = iio_priv(indio_dev);
>>>
>>> iio_device_unregister(indio_dev);
>>> + iio_triggered_buffer_cleanup(indio_dev);
>>> if (data->drdy_trig)
>>> iio_trigger_unregister(data->drdy_trig);
>>
>> Ditto.
>>
>>> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
>>> index 8c791b8..1e0f071 100644
>>> --- a/drivers/iio/accel/adxl345_i2c.c
>>> +++ b/drivers/iio/accel/adxl345_i2c.c
>>> @@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
>>> bool use_int2 = false;
>>> int irq;
>>>
>>> + if (!i2c_check_functionality(client->adapter,
>>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
>>> + return -EOPNOTSUPP;
>>
>> And if driver works before, you make a regression here.
> Absolutely. Need a fallback if it's not supported...
I believe regmap has a fallback, so that check could simply be removed, I guess.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 4/4] iio: accel: adxl345: Add support for triggered buffer
2017-03-20 19:46 ` Lars-Peter Clausen
@ 2017-03-21 9:56 ` Eva Rachel Retuya
0 siblings, 0 replies; 15+ messages in thread
From: Eva Rachel Retuya @ 2017-03-21 9:56 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Jonathan Cameron, Andy Shevchenko, linux-iio, Hartmut Knaack,
Peter Meerwald, Dmitry Torokhov, Michael Hennerich,
Daniel Baluta, Alison Schofield, Florian Vaussard, linux-kernel,
Rob Herring, Mark Rutland, devicetree
On Mon, Mar 20, 2017 at 08:46:11PM +0100, Lars-Peter Clausen wrote:
> On 03/15/2017 10:50 PM, Jonathan Cameron wrote:
> > On 13/03/17 12:16, Andy Shevchenko wrote:
> >> On Mon, Mar 13, 2017 at 1:11 PM, Eva Rachel Retuya <eraretuya@gmail.com> wrote:
> >>> Provide an all-axes read for triggered buffering.
> >>
> >> Better description is needed.
> >>
> >>> -static int adxl345_get_triple(struct adxl345_data *data, void *buf)
> >>> +static int adxl345_get_triple(struct adxl345_data *data)
> >>
> >> Ping-ponging again. This should be essentially a change in the patch
> >> where you introduce a helper.
> >>
> >>> +err_buffer_cleanup:
> >>> + iio_triggered_buffer_cleanup(indio_dev);
> >>> err_trigger_unregister:
> >>> if (data->drdy_trig)
> >>> iio_trigger_unregister(data->drdy_trig);
> >>
> >> devm_iio_*() ?
> >>
> >>> @@ -334,6 +435,7 @@ int adxl345_core_remove(struct device *dev)
> >>> struct adxl345_data *data = iio_priv(indio_dev);
> >>>
> >>> iio_device_unregister(indio_dev);
> >>> + iio_triggered_buffer_cleanup(indio_dev);
> >>> if (data->drdy_trig)
> >>> iio_trigger_unregister(data->drdy_trig);
> >>
> >> Ditto.
> >>
> >>> diff --git a/drivers/iio/accel/adxl345_i2c.c b/drivers/iio/accel/adxl345_i2c.c
> >>> index 8c791b8..1e0f071 100644
> >>> --- a/drivers/iio/accel/adxl345_i2c.c
> >>> +++ b/drivers/iio/accel/adxl345_i2c.c
> >>> @@ -30,6 +30,10 @@ static int adxl345_i2c_probe(struct i2c_client *client,
> >>> bool use_int2 = false;
> >>> int irq;
> >>>
> >>> + if (!i2c_check_functionality(client->adapter,
> >>> + I2C_FUNC_SMBUS_READ_I2C_BLOCK))
> >>> + return -EOPNOTSUPP;
> >>
> >> And if driver works before, you make a regression here.
> > Absolutely. Need a fallback if it's not supported...
>
> I believe regmap has a fallback, so that check could simply be removed, I guess.
>
Yes, regmap does this check. I've seen it while working on the patchset.
I checked other drivers doing regmap and some have this check_functionality
check so I chose to include it because I'm not sure.
Will remove it on the next version.
Thanks,
Eva
^ permalink raw reply [flat|nested] 15+ messages in thread