2019-02-19 17:13:38

by Stefan Popa

[permalink] [raw]
Subject: [PATCH 0/6] iio: imu: adis16480: Add support for ADIS1649x family of devices

This series has as main goal to add support for ADIS1649x family of devices as
part of the already existing adis16480, but on the way it also deals with some
outstanding items:

* Make drdy pin configurable
* Add OF device ID table
* Deal with the temperature max scale in a generic way
* Add missing docs

Stefan Popa (6):
iio: imu: adis16480: Use the default data ready pin configuration
iio: imu: adis16480: Add support for configurable drdy indicator
iio: imu: adis16480: Add OF device ID table
iio: imu: adis16480: Treat temperature scale in a generic way
iio: imu: adis16480: Add support for ADIS1649x family of devices
iio: imu: adis16480: Add docs for ADIS16480 IMU

.../devicetree/bindings/iio/imu/adi,adis16480.txt | 49 +++++
MAINTAINERS | 1 +
drivers/iio/imu/adis16480.c | 198 ++++++++++++++++++++-
3 files changed, 243 insertions(+), 5 deletions(-)
create mode 100644 Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt

--
2.7.4



2019-02-19 17:14:20

by Stefan Popa

[permalink] [raw]
Subject: [PATCH 2/6] iio: imu: adis16480: Add support for configurable drdy indicator

The FNCTIO_CTRL register provides configuration control for each I/O pin
(DIO1, DIO2, DIO3 and DIO4).

This patch adds the option to configure each DIOx pin as data ready
indicator with positive or negative polarity by reading the 'interrupts'
and 'interrupt-names' properties from the devicetree. The
'interrupt-names' property is optional, if it is not specified, then the
factory default DIO2 data ready signal is used.

Signed-off-by: Stefan Popa <[email protected]>
---
drivers/iio/imu/adis16480.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 76 insertions(+)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index d222188..38ba0c1 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -10,6 +10,7 @@
*/

#include <linux/bitfield.h>
+#include <linux/of_irq.h>
#include <linux/interrupt.h>
#include <linux/delay.h>
#include <linux/mutex.h>
@@ -109,6 +110,10 @@
#define ADIS16480_FIR_COEF_D(x) ADIS16480_FIR_COEF(0x0B, (x))

/* ADIS16480_REG_FNCTIO_CTRL */
+#define ADIS16480_DRDY_SEL_MSK GENMASK(1, 0)
+#define ADIS16480_DRDY_SEL(x) FIELD_PREP(ADIS16480_DRDY_SEL_MSK, x)
+#define ADIS16480_DRDY_POL_MSK BIT(2)
+#define ADIS16480_DRDY_POL(x) FIELD_PREP(ADIS16480_DRDY_POL_MSK, x)
#define ADIS16480_DRDY_EN_MSK BIT(3)
#define ADIS16480_DRDY_EN(x) FIELD_PREP(ADIS16480_DRDY_EN_MSK, x)

@@ -121,12 +126,26 @@ struct adis16480_chip_info {
unsigned int accel_max_scale;
};

+enum adis16480_int_pin {
+ ADIS16480_PIN_DIO1,
+ ADIS16480_PIN_DIO2,
+ ADIS16480_PIN_DIO3,
+ ADIS16480_PIN_DIO4
+};
+
struct adis16480 {
const struct adis16480_chip_info *chip_info;

struct adis adis;
};

+static const char * const adis16480_int_pin_names[4] = {
+ [ADIS16480_PIN_DIO1] = "DIO1",
+ [ADIS16480_PIN_DIO2] = "DIO2",
+ [ADIS16480_PIN_DIO3] = "DIO3",
+ [ADIS16480_PIN_DIO4] = "DIO4",
+};
+
#ifdef CONFIG_DEBUG_FS

static ssize_t adis16480_show_firmware_revision(struct file *file,
@@ -840,6 +859,59 @@ static const struct adis_data adis16480_data = {
.enable_irq = adis16480_enable_irq,
};

+static int adis16480_config_irq_pin(struct device_node *of_node,
+ struct adis16480 *st)
+{
+ struct irq_data *desc;
+ enum adis16480_int_pin pin;
+ unsigned int irq_type;
+ uint16_t val;
+ int i, irq = 0;
+
+ desc = irq_get_irq_data(st->adis.spi->irq);
+ if (!desc) {
+ dev_err(&st->adis.spi->dev, "Could not find IRQ %d\n", irq);
+ return -EINVAL;
+ }
+
+ /* Disable data ready */
+ val = ADIS16480_DRDY_EN(0);
+
+ /*
+ * Get the interrupt from the devicetre by reading the
+ * interrupt-names property. If it is not specified, use
+ * the default interrupt on DIO2 pin.
+ */
+ pin = ADIS16480_PIN_DIO2;
+ for (i = 0; i < ARRAY_SIZE(adis16480_int_pin_names); i++) {
+ irq = of_irq_get_byname(of_node, adis16480_int_pin_names[i]);
+ if (irq > 0) {
+ pin = i;
+ break;
+ }
+ }
+
+ val |= ADIS16480_DRDY_SEL(pin);
+
+ /*
+ * Get the interrupt line behaviour. The data ready polarity can be
+ * configured as positive or negative, corresponding to
+ * IRQF_TRIGGER_RISING or IRQF_TRIGGER_FALLING respectively.
+ */
+ irq_type = irqd_get_trigger_type(desc);
+ if (irq_type == IRQF_TRIGGER_RISING) { /* Default */
+ val |= ADIS16480_DRDY_POL(1);
+ } else if (irq_type == IRQF_TRIGGER_FALLING) {
+ val |= ADIS16480_DRDY_POL(0);
+ } else {
+ dev_err(&st->adis.spi->dev,
+ "Invalid interrupt type 0x%x specified\n", irq_type);
+ return -EINVAL;
+ }
+ /* Write the data ready configuration to the FNCTIO_CTRL register */
+ return adis_write_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL, val);
+}
+
static int adis16480_probe(struct spi_device *spi)
{
const struct spi_device_id *id = spi_get_device_id(spi);
@@ -867,6 +939,10 @@ static int adis16480_probe(struct spi_device *spi)
if (ret)
return ret;

+ ret = adis16480_config_irq_pin(spi->dev.of_node, st);
+ if (ret)
+ return ret;
+
ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
if (ret)
return ret;
--
2.7.4


2019-02-19 17:14:41

by Stefan Popa

[permalink] [raw]
Subject: [PATCH 1/6] iio: imu: adis16480: Use the default data ready pin configuration

The FNCTIO_CTRL register, Bits[3:0] provide three configuration options
for the data ready function: on/off, polarity, and DIOx line. The
factory default assigns DIO2 as a positive polarity, data ready signal.

The adis16480_enable_irq() function, overwrites this configuration when
it enables/disables the data ready pin by only setting BIT[3].
As a result, the data ready signal becomes DIO1 pin which is assigned as
negative polarity.

This patch reads the FNCTIO_CTRL register and creates a mask, such that
only data ready enable (BIT[3]) will be modified when
adis16480_enable_irq function is called.

Signed-off-by: Stefan Popa <[email protected]>
---
drivers/iio/imu/adis16480.c | 18 ++++++++++++++++--
1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index a27fe20..d222188 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -9,6 +9,7 @@
*
*/

+#include <linux/bitfield.h>
#include <linux/interrupt.h>
#include <linux/delay.h>
#include <linux/mutex.h>
@@ -107,6 +108,10 @@
#define ADIS16480_FIR_COEF_C(x) ADIS16480_FIR_COEF(0x09, (x))
#define ADIS16480_FIR_COEF_D(x) ADIS16480_FIR_COEF(0x0B, (x))

+/* ADIS16480_REG_FNCTIO_CTRL */
+#define ADIS16480_DRDY_EN_MSK BIT(3)
+#define ADIS16480_DRDY_EN(x) FIELD_PREP(ADIS16480_DRDY_EN_MSK, x)
+
struct adis16480_chip_info {
unsigned int num_channels;
const struct iio_chan_spec *channels;
@@ -741,8 +746,17 @@ static int adis16480_stop_device(struct iio_dev *indio_dev)

static int adis16480_enable_irq(struct adis *adis, bool enable)
{
- return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL,
- enable ? BIT(3) : 0);
+ uint16_t val;
+ int ret;
+
+ ret = adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
+ if (ret < 0)
+ return ret;
+
+ val &= ~ADIS16480_DRDY_EN_MSK;
+ val |= ADIS16480_DRDY_EN(enable);
+
+ return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, val);
}

static int adis16480_initial_setup(struct iio_dev *indio_dev)
--
2.7.4


2019-02-19 17:16:06

by Stefan Popa

[permalink] [raw]
Subject: [PATCH 3/6] iio: imu: adis16480: Add OF device ID table

The driver does not have a struct of_device_id table, but supported
devices are registered via Device Trees. This patch adds OF device ID
table.

Signed-off-by: Stefan Popa <[email protected]>
---
drivers/iio/imu/adis16480.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 38ba0c1..7ae71f4 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -988,9 +988,19 @@ static const struct spi_device_id adis16480_ids[] = {
};
MODULE_DEVICE_TABLE(spi, adis16480_ids);

+static const struct of_device_id adis16480_of_match[] = {
+ { .compatible = "adi,adis16375" },
+ { .compatible = "adi,adis16480" },
+ { .compatible = "adi,adis16485" },
+ { .compatible = "adi,adis16488" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, adis16480_of_match);
+
static struct spi_driver adis16480_driver = {
.driver = {
.name = "adis16480",
+ .of_match_table = adis16480_of_match,
},
.id_table = adis16480_ids,
.probe = adis16480_probe,
--
2.7.4


2019-02-19 17:16:28

by Stefan Popa

[permalink] [raw]
Subject: [PATCH 4/6] iio: imu: adis16480: Treat temperature scale in a generic way

All supported devices provide internal temperature measurement from -40 C
to +85 C, with +25 C representing value 0x00.

This patch treats the temperature scale in a generic way, similar to the
accelerometer and gyroscope scales. So far, there are no temperature max
scale differences between the supported devices. However, devices that
will make use of this feature will be added in the future.

Signed-off-by: Stefan Popa <[email protected]>
---
drivers/iio/imu/adis16480.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index 7ae71f4..cc53825 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -124,6 +124,7 @@ struct adis16480_chip_info {
unsigned int gyro_max_scale;
unsigned int accel_max_val;
unsigned int accel_max_scale;
+ unsigned int temp_max_scale;
};

enum adis16480_int_pin {
@@ -530,6 +531,7 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
const struct iio_chan_spec *chan, int *val, int *val2, long info)
{
struct adis16480 *st = iio_priv(indio_dev);
+ unsigned int temp, scale;

switch (info) {
case IIO_CHAN_INFO_RAW:
@@ -549,8 +551,15 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
*val2 = 100; /* 0.0001 gauss */
return IIO_VAL_INT_PLUS_MICRO;
case IIO_TEMP:
- *val = 5;
- *val2 = 650000; /* 5.65 milli degree Celsius */
+ /*
+ * +85 degrees Celsius = temp_max_scale
+ * +25 degrees Celsius = 0
+ * LSB, 25 degrees Celsius = 60 / temp_max_scale
+ */
+ scale = DIV_ROUND_CLOSEST_ULL(60 * 1000000LL,
+ st->chip_info->temp_max_scale);
+ *val = scale / 1000;
+ *val2 = (scale % 1000) * 1000;
return IIO_VAL_INT_PLUS_MICRO;
case IIO_PRESSURE:
*val = 0;
@@ -561,7 +570,10 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
}
case IIO_CHAN_INFO_OFFSET:
/* Only the temperature channel has a offset */
- *val = 4425; /* 25 degree Celsius = 0x0000 */
+ temp = 25 * 1000000LL; /* 25 degree Celsius = 0x0000 */
+ scale = DIV_ROUND_CLOSEST_ULL(60 * 1000000LL,
+ st->chip_info->temp_max_scale);
+ *val = DIV_ROUND_CLOSEST_ULL(temp, scale);
return IIO_VAL_INT;
case IIO_CHAN_INFO_CALIBBIAS:
return adis16480_get_calibbias(indio_dev, chan, val);
@@ -717,6 +729,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
.gyro_max_scale = 300,
.accel_max_val = IIO_M_S_2_TO_G(21973),
.accel_max_scale = 18,
+ .temp_max_scale = 10619,
},
[ADIS16480] = {
.channels = adis16480_channels,
@@ -725,6 +738,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
.gyro_max_scale = 450,
.accel_max_val = IIO_M_S_2_TO_G(12500),
.accel_max_scale = 10,
+ .temp_max_scale = 10619,
},
[ADIS16485] = {
.channels = adis16485_channels,
@@ -733,6 +747,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
.gyro_max_scale = 450,
.accel_max_val = IIO_M_S_2_TO_G(20000),
.accel_max_scale = 5,
+ .temp_max_scale = 10619,
},
[ADIS16488] = {
.channels = adis16480_channels,
@@ -741,6 +756,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
.gyro_max_scale = 450,
.accel_max_val = IIO_M_S_2_TO_G(22500),
.accel_max_scale = 18,
+ .temp_max_scale = 10619,
},
};

--
2.7.4


2019-02-19 17:16:56

by Stefan Popa

[permalink] [raw]
Subject: [PATCH 5/6] iio: imu: adis16480: Add support for ADIS1649x family of devices

The ADIS16495 and ADIS16497 are inertial systems that include a triaxis
gyroscope and a triaxis accelerometer. The serial peripheral interface
(SPI) provide a simple interface for data collection and configuration
control. The devices are similar to ADIS16475, ADIS16480, ADIS16485 and
ADIS16488, the main differences are related to range and scale factors.

The temperature data scale is 0.00565 C/LSB for ADIS16475 and ADIS1648x
devices, while for ADIS1649x 0.0125 C/LSB.

Another difference is that ADIS1649x devices support different gyroscope
measurement ranges which are dependent on the dash number (-1, -2, -3),
see Table 24 in the ADIS16495 datasheet. However, the ADIS16497
gyroscopes have the same scale as ADIS16495.

Furthermore, ADIS16495 devices support the acceleration maximum range of
8g, while ADIS16497 devices go up to 40g.

Datasheets:
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/adis16495.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/adis16497.pdf

Signed-off-by: Stefan Popa <[email protected]>
---
drivers/iio/imu/adis16480.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 72 insertions(+)

diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
index cc53825..c30acfdb 100644
--- a/drivers/iio/imu/adis16480.c
+++ b/drivers/iio/imu/adis16480.c
@@ -714,6 +714,12 @@ enum adis16480_variant {
ADIS16480,
ADIS16485,
ADIS16488,
+ ADIS16495_1,
+ ADIS16495_2,
+ ADIS16495_3,
+ ADIS16497_1,
+ ADIS16497_2,
+ ADIS16497_3,
};

static const struct adis16480_chip_info adis16480_chip_info[] = {
@@ -758,6 +764,60 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
.accel_max_scale = 18,
.temp_max_scale = 10619,
},
+ [ADIS16495_1] = {
+ .channels = adis16485_channels,
+ .num_channels = ARRAY_SIZE(adis16485_channels),
+ .gyro_max_val = IIO_RAD_TO_DEGREE(20000),
+ .gyro_max_scale = 125,
+ .accel_max_val = IIO_M_S_2_TO_G(32000),
+ .accel_max_scale = 8,
+ .temp_max_scale = 4800,
+ },
+ [ADIS16495_2] = {
+ .channels = adis16485_channels,
+ .num_channels = ARRAY_SIZE(adis16485_channels),
+ .gyro_max_val = IIO_RAD_TO_DEGREE(18000),
+ .gyro_max_scale = 450,
+ .accel_max_val = IIO_M_S_2_TO_G(32000),
+ .accel_max_scale = 8,
+ .temp_max_scale = 4800,
+ },
+ [ADIS16495_3] = {
+ .channels = adis16485_channels,
+ .num_channels = ARRAY_SIZE(adis16485_channels),
+ .gyro_max_val = IIO_RAD_TO_DEGREE(20000),
+ .gyro_max_scale = 2000,
+ .accel_max_val = IIO_M_S_2_TO_G(32000),
+ .accel_max_scale = 8,
+ .temp_max_scale = 4800,
+ },
+ [ADIS16497_1] = {
+ .channels = adis16485_channels,
+ .num_channels = ARRAY_SIZE(adis16485_channels),
+ .gyro_max_val = IIO_RAD_TO_DEGREE(20000),
+ .gyro_max_scale = 125,
+ .accel_max_val = IIO_M_S_2_TO_G(32000),
+ .accel_max_scale = 40,
+ .temp_max_scale = 4800,
+ },
+ [ADIS16497_2] = {
+ .channels = adis16485_channels,
+ .num_channels = ARRAY_SIZE(adis16485_channels),
+ .gyro_max_val = IIO_RAD_TO_DEGREE(18000),
+ .gyro_max_scale = 450,
+ .accel_max_val = IIO_M_S_2_TO_G(32000),
+ .accel_max_scale = 40,
+ .temp_max_scale = 4800,
+ },
+ [ADIS16497_3] = {
+ .channels = adis16485_channels,
+ .num_channels = ARRAY_SIZE(adis16485_channels),
+ .gyro_max_val = IIO_RAD_TO_DEGREE(20000),
+ .gyro_max_scale = 2000,
+ .accel_max_val = IIO_M_S_2_TO_G(32000),
+ .accel_max_scale = 40,
+ .temp_max_scale = 4800,
+ },
};

static const struct iio_info adis16480_info = {
@@ -1000,6 +1060,12 @@ static const struct spi_device_id adis16480_ids[] = {
{ "adis16480", ADIS16480 },
{ "adis16485", ADIS16485 },
{ "adis16488", ADIS16488 },
+ { "adis16495-1", ADIS16495_1 },
+ { "adis16495-2", ADIS16495_2 },
+ { "adis16495-3", ADIS16495_3 },
+ { "adis16497-1", ADIS16497_1 },
+ { "adis16497-2", ADIS16497_2 },
+ { "adis16497-3", ADIS16497_3 },
{ }
};
MODULE_DEVICE_TABLE(spi, adis16480_ids);
@@ -1009,6 +1075,12 @@ static const struct of_device_id adis16480_of_match[] = {
{ .compatible = "adi,adis16480" },
{ .compatible = "adi,adis16485" },
{ .compatible = "adi,adis16488" },
+ { .compatible = "adi,adis16495-1" },
+ { .compatible = "adi,adis16495-2" },
+ { .compatible = "adi,adis16495-3" },
+ { .compatible = "adi,adis16497-1" },
+ { .compatible = "adi,adis16497-2" },
+ { .compatible = "adi,adis16497-3" },
{ },
};
MODULE_DEVICE_TABLE(of, adis16480_of_match);
--
2.7.4


2019-02-19 17:17:26

by Stefan Popa

[permalink] [raw]
Subject: [PATCH 6/6] iio: imu: adis16480: Add docs for ADIS16480 IMU

Document support for ADIS16480 Inertial Measurement Unit.

Signed-off-by: Stefan Popa <[email protected]>
---
.../devicetree/bindings/iio/imu/adi,adis16480.txt | 49 ++++++++++++++++++++++
MAINTAINERS | 1 +
2 files changed, 50 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt

diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt b/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt
new file mode 100644
index 0000000..dacf5f7
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt
@@ -0,0 +1,49 @@
+
+Analog Devices ADIS16480 and similar IMUs
+
+Required properties for the ADIS16480:
+
+- compatible: Must be one of
+ * "adi,adis16375"
+ * "adi,adis16480"
+ * "adi,adis16485"
+ * "adi,adis16488"
+ * "adi,adis16495-1"
+ * "adi,adis16495-2"
+ * "adi,adis16495-3"
+ * "adi,adis16497-1"
+ * "adi,adis16497-2"
+ * "adi,adis16497-3"
+- reg: SPI chip select number for the device
+- spi-max-frequency: Max SPI frequency to use
+ see: Documentation/devicetree/bindings/spi/spi-bus.txt
+- spi-cpha: See Documentation/devicetree/bindings/spi/spi-bus.txt
+- spi-cpol: See Documentation/devicetree/bindings/spi/spi-bus.txt
+- interrupts: interrupt mapping for IRQ, accepted values are:
+ * IRQF_TRIGGER_RISING
+ * IRQF_TRIGGER_FALLING
+
+Optional properties:
+
+- interrupt-names: Data ready line selection. Valid values are:
+ * DIO1
+ * DIO2
+ * DIO3
+ * DIO4
+ If this field is left empty, the factory default assigns DIO2 as data
+ ready signal.
+- reset-gpios: must be the device tree identifier of the RESET pin. As the line
+ is active low, it should be marked GPIO_ACTIVE_LOW.
+
+Example:
+
+ imu@0 {
+ compatible = "adi,adis16495-1";
+ reg = <0>;
+ spi-max-frequency = <3200000>;
+ spi-cpol;
+ spi-cpha;
+ interrupts = <25 IRQF_TRIGGER_FALLING>;
+ interrupt-parent = <&gpio>;
+ interrupt-names = "DIO2";
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index e4091ac..beecd1e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -942,6 +942,7 @@ F: drivers/dma/dma-axi-dmac.c
ANALOG DEVICES INC IIO DRIVERS
M: Lars-Peter Clausen <[email protected]>
M: Michael Hennerich <[email protected]>
+M: Stefan Popa <[email protected]>
W: http://wiki.analog.com/
W: http://ez.analog.com/community/linux-device-drivers
S: Supported
--
2.7.4


2019-02-20 10:21:15

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 1/6] iio: imu: adis16480: Use the default data ready pin configuration

On Tue, 19 Feb 2019 19:12:13 +0200
Stefan Popa <[email protected]> wrote:

> The FNCTIO_CTRL register, Bits[3:0] provide three configuration options
> for the data ready function: on/off, polarity, and DIOx line. The
> factory default assigns DIO2 as a positive polarity, data ready signal.
>
> The adis16480_enable_irq() function, overwrites this configuration when
> it enables/disables the data ready pin by only setting BIT[3].
> As a result, the data ready signal becomes DIO1 pin which is assigned as
> negative polarity.
>
> This patch reads the FNCTIO_CTRL register and creates a mask, such that
> only data ready enable (BIT[3]) will be modified when
> adis16480_enable_irq function is called.

So this is potentially a problem. As I read this, we just changed the default.
So a device that has been relying on this 'bug' for a long time will now
not work as it will be expecting the interrupt on the wrong physical pin.

So, whilst it might seem logical to let the device stay with it's default
we can't do it because the defacto Linux default is the other choice.

The delights of having to support old 'bugs' :)

Jonathan
>
> Signed-off-by: Stefan Popa <[email protected]>
> ---
> drivers/iio/imu/adis16480.c | 18 ++++++++++++++++--
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index a27fe20..d222188 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -9,6 +9,7 @@
> *
> */
>
> +#include <linux/bitfield.h>
> #include <linux/interrupt.h>
> #include <linux/delay.h>
> #include <linux/mutex.h>
> @@ -107,6 +108,10 @@
> #define ADIS16480_FIR_COEF_C(x) ADIS16480_FIR_COEF(0x09, (x))
> #define ADIS16480_FIR_COEF_D(x) ADIS16480_FIR_COEF(0x0B, (x))
>
> +/* ADIS16480_REG_FNCTIO_CTRL */
> +#define ADIS16480_DRDY_EN_MSK BIT(3)
> +#define ADIS16480_DRDY_EN(x) FIELD_PREP(ADIS16480_DRDY_EN_MSK, x)
> +
> struct adis16480_chip_info {
> unsigned int num_channels;
> const struct iio_chan_spec *channels;
> @@ -741,8 +746,17 @@ static int adis16480_stop_device(struct iio_dev *indio_dev)
>
> static int adis16480_enable_irq(struct adis *adis, bool enable)
> {
> - return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL,
> - enable ? BIT(3) : 0);
> + uint16_t val;
> + int ret;
> +
> + ret = adis_read_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, &val);
> + if (ret < 0)
> + return ret;
> +
> + val &= ~ADIS16480_DRDY_EN_MSK;
> + val |= ADIS16480_DRDY_EN(enable);
> +
> + return adis_write_reg_16(adis, ADIS16480_REG_FNCTIO_CTRL, val);
> }
>
> static int adis16480_initial_setup(struct iio_dev *indio_dev)


2019-02-20 10:31:01

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 2/6] iio: imu: adis16480: Add support for configurable drdy indicator

On Tue, 19 Feb 2019 19:12:14 +0200
Stefan Popa <[email protected]> wrote:

> The FNCTIO_CTRL register provides configuration control for each I/O pin
> (DIO1, DIO2, DIO3 and DIO4).
>
> This patch adds the option to configure each DIOx pin as data ready
> indicator with positive or negative polarity by reading the 'interrupts'
> and 'interrupt-names' properties from the devicetree. The
> 'interrupt-names' property is optional, if it is not specified, then the
> factory default DIO2 data ready signal is used.
>
> Signed-off-by: Stefan Popa <[email protected]>
Other than follow on from the previous patch change of the default, this
looks fine to me.

One question inline.

Jonathan
> ---
> drivers/iio/imu/adis16480.c | 76 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 76 insertions(+)
>
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index d222188..38ba0c1 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -10,6 +10,7 @@
> */
>
> #include <linux/bitfield.h>
> +#include <linux/of_irq.h>
> #include <linux/interrupt.h>
> #include <linux/delay.h>
> #include <linux/mutex.h>
> @@ -109,6 +110,10 @@
> #define ADIS16480_FIR_COEF_D(x) ADIS16480_FIR_COEF(0x0B, (x))
>
> /* ADIS16480_REG_FNCTIO_CTRL */
> +#define ADIS16480_DRDY_SEL_MSK GENMASK(1, 0)
> +#define ADIS16480_DRDY_SEL(x) FIELD_PREP(ADIS16480_DRDY_SEL_MSK, x)
> +#define ADIS16480_DRDY_POL_MSK BIT(2)
> +#define ADIS16480_DRDY_POL(x) FIELD_PREP(ADIS16480_DRDY_POL_MSK, x)
> #define ADIS16480_DRDY_EN_MSK BIT(3)
> #define ADIS16480_DRDY_EN(x) FIELD_PREP(ADIS16480_DRDY_EN_MSK, x)
>
> @@ -121,12 +126,26 @@ struct adis16480_chip_info {
> unsigned int accel_max_scale;
> };
>
> +enum adis16480_int_pin {
> + ADIS16480_PIN_DIO1,
> + ADIS16480_PIN_DIO2,
> + ADIS16480_PIN_DIO3,
> + ADIS16480_PIN_DIO4
> +};
> +
> struct adis16480 {
> const struct adis16480_chip_info *chip_info;
>
> struct adis adis;
> };
>
> +static const char * const adis16480_int_pin_names[4] = {
> + [ADIS16480_PIN_DIO1] = "DIO1",
> + [ADIS16480_PIN_DIO2] = "DIO2",
> + [ADIS16480_PIN_DIO3] = "DIO3",
> + [ADIS16480_PIN_DIO4] = "DIO4",
> +};
> +
> #ifdef CONFIG_DEBUG_FS
>
> static ssize_t adis16480_show_firmware_revision(struct file *file,
> @@ -840,6 +859,59 @@ static const struct adis_data adis16480_data = {
> .enable_irq = adis16480_enable_irq,
> };
>
> +static int adis16480_config_irq_pin(struct device_node *of_node,
> + struct adis16480 *st)
> +{
> + struct irq_data *desc;
> + enum adis16480_int_pin pin;
> + unsigned int irq_type;
> + uint16_t val;
> + int i, irq = 0;
> +
> + desc = irq_get_irq_data(st->adis.spi->irq);
> + if (!desc) {
> + dev_err(&st->adis.spi->dev, "Could not find IRQ %d\n", irq);
> + return -EINVAL;
> + }
> +
> + /* Disable data ready */
> + val = ADIS16480_DRDY_EN(0);
Does it default to on after reset?
That's a little unusual and nasty. If not, why are you disabling here?
> +
> + /*
> + * Get the interrupt from the devicetre by reading the
> + * interrupt-names property. If it is not specified, use
> + * the default interrupt on DIO2 pin.
> + */
> + pin = ADIS16480_PIN_DIO2;
> + for (i = 0; i < ARRAY_SIZE(adis16480_int_pin_names); i++) {
> + irq = of_irq_get_byname(of_node, adis16480_int_pin_names[i]);
> + if (irq > 0) {
> + pin = i;
> + break;
> + }
> + }
> +
> + val |= ADIS16480_DRDY_SEL(pin);
> +
> + /*
> + * Get the interrupt line behaviour. The data ready polarity can be
> + * configured as positive or negative, corresponding to
> + * IRQF_TRIGGER_RISING or IRQF_TRIGGER_FALLING respectively.
> + */
> + irq_type = irqd_get_trigger_type(desc);
> + if (irq_type == IRQF_TRIGGER_RISING) { /* Default */
> + val |= ADIS16480_DRDY_POL(1);
> + } else if (irq_type == IRQF_TRIGGER_FALLING) {
> + val |= ADIS16480_DRDY_POL(0);
> + } else {
> + dev_err(&st->adis.spi->dev,
> + "Invalid interrupt type 0x%x specified\n", irq_type);
> + return -EINVAL;
> + }
> + /* Write the data ready configuration to the FNCTIO_CTRL register */
> + return adis_write_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL, val);
> +}
> +
> static int adis16480_probe(struct spi_device *spi)
> {
> const struct spi_device_id *id = spi_get_device_id(spi);
> @@ -867,6 +939,10 @@ static int adis16480_probe(struct spi_device *spi)
> if (ret)
> return ret;
>
> + ret = adis16480_config_irq_pin(spi->dev.of_node, st);
> + if (ret)
> + return ret;
> +
> ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
> if (ret)
> return ret;


2019-02-20 10:32:44

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 3/6] iio: imu: adis16480: Add OF device ID table

On Tue, 19 Feb 2019 19:12:15 +0200
Stefan Popa <[email protected]> wrote:

> The driver does not have a struct of_device_id table, but supported
> devices are registered via Device Trees. This patch adds OF device ID
> table.
>
> Signed-off-by: Stefan Popa <[email protected]>
This is fine, I'll pick it up in v2.
> ---
> drivers/iio/imu/adis16480.c | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index 38ba0c1..7ae71f4 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -988,9 +988,19 @@ static const struct spi_device_id adis16480_ids[] = {
> };
> MODULE_DEVICE_TABLE(spi, adis16480_ids);
>
> +static const struct of_device_id adis16480_of_match[] = {
> + { .compatible = "adi,adis16375" },
> + { .compatible = "adi,adis16480" },
> + { .compatible = "adi,adis16485" },
> + { .compatible = "adi,adis16488" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, adis16480_of_match);
> +
> static struct spi_driver adis16480_driver = {
> .driver = {
> .name = "adis16480",
> + .of_match_table = adis16480_of_match,
> },
> .id_table = adis16480_ids,
> .probe = adis16480_probe,


2019-02-20 10:38:44

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 4/6] iio: imu: adis16480: Treat temperature scale in a generic way

On Tue, 19 Feb 2019 19:12:16 +0200
Stefan Popa <[email protected]> wrote:

> All supported devices provide internal temperature measurement from -40 C
> to +85 C, with +25 C representing value 0x00.
>
> This patch treats the temperature scale in a generic way, similar to the
> accelerometer and gyroscope scales. So far, there are no temperature max
> scale differences between the supported devices. However, devices that
> will make use of this feature will be added in the future.
>
> Signed-off-by: Stefan Popa <[email protected]>


Given the datasheet (well 16480 anyway as I'm lazy and only checked that
one) give the scale directly in deg C / LSB, why not just provide directly?
For 16480 it is 0.00565.

For the other channel types it also gives them this way so we could change
them all over, but probably best not to touch something that is 'working' :)

Jonathan

> ---
> drivers/iio/imu/adis16480.c | 22 +++++++++++++++++++---
> 1 file changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index 7ae71f4..cc53825 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -124,6 +124,7 @@ struct adis16480_chip_info {
> unsigned int gyro_max_scale;
> unsigned int accel_max_val;
> unsigned int accel_max_scale;
> + unsigned int temp_max_scale;
> };
>
> enum adis16480_int_pin {
> @@ -530,6 +531,7 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
> const struct iio_chan_spec *chan, int *val, int *val2, long info)
> {
> struct adis16480 *st = iio_priv(indio_dev);
> + unsigned int temp, scale;
>
> switch (info) {
> case IIO_CHAN_INFO_RAW:
> @@ -549,8 +551,15 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
> *val2 = 100; /* 0.0001 gauss */
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_TEMP:
> - *val = 5;
> - *val2 = 650000; /* 5.65 milli degree Celsius */
> + /*
> + * +85 degrees Celsius = temp_max_scale
> + * +25 degrees Celsius = 0
> + * LSB, 25 degrees Celsius = 60 / temp_max_scale
> + */
> + scale = DIV_ROUND_CLOSEST_ULL(60 * 1000000LL,
> + st->chip_info->temp_max_scale);
> + *val = scale / 1000;
> + *val2 = (scale % 1000) * 1000;
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_PRESSURE:
> *val = 0;
> @@ -561,7 +570,10 @@ static int adis16480_read_raw(struct iio_dev *indio_dev,
> }
> case IIO_CHAN_INFO_OFFSET:
> /* Only the temperature channel has a offset */
> - *val = 4425; /* 25 degree Celsius = 0x0000 */
> + temp = 25 * 1000000LL; /* 25 degree Celsius = 0x0000 */
> + scale = DIV_ROUND_CLOSEST_ULL(60 * 1000000LL,
> + st->chip_info->temp_max_scale);
> + *val = DIV_ROUND_CLOSEST_ULL(temp, scale);
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_CALIBBIAS:
> return adis16480_get_calibbias(indio_dev, chan, val);
> @@ -717,6 +729,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
> .gyro_max_scale = 300,
> .accel_max_val = IIO_M_S_2_TO_G(21973),
> .accel_max_scale = 18,
> + .temp_max_scale = 10619,
> },
> [ADIS16480] = {
> .channels = adis16480_channels,
> @@ -725,6 +738,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
> .gyro_max_scale = 450,
> .accel_max_val = IIO_M_S_2_TO_G(12500),
> .accel_max_scale = 10,
> + .temp_max_scale = 10619,
> },
> [ADIS16485] = {
> .channels = adis16485_channels,
> @@ -733,6 +747,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
> .gyro_max_scale = 450,
> .accel_max_val = IIO_M_S_2_TO_G(20000),
> .accel_max_scale = 5,
> + .temp_max_scale = 10619,
> },
> [ADIS16488] = {
> .channels = adis16480_channels,
> @@ -741,6 +756,7 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
> .gyro_max_scale = 450,
> .accel_max_val = IIO_M_S_2_TO_G(22500),
> .accel_max_scale = 18,
> + .temp_max_scale = 10619,
> },
> };
>


2019-02-20 10:41:32

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 5/6] iio: imu: adis16480: Add support for ADIS1649x family of devices

On Tue, 19 Feb 2019 19:12:17 +0200
Stefan Popa <[email protected]> wrote:

> The ADIS16495 and ADIS16497 are inertial systems that include a triaxis
> gyroscope and a triaxis accelerometer. The serial peripheral interface
> (SPI) provide a simple interface for data collection and configuration
> control. The devices are similar to ADIS16475, ADIS16480, ADIS16485 and
> ADIS16488, the main differences are related to range and scale factors.
>
> The temperature data scale is 0.00565 C/LSB for ADIS16475 and ADIS1648x
> devices, while for ADIS1649x 0.0125 C/LSB.
>
> Another difference is that ADIS1649x devices support different gyroscope
> measurement ranges which are dependent on the dash number (-1, -2, -3),
> see Table 24 in the ADIS16495 datasheet. However, the ADIS16497
> gyroscopes have the same scale as ADIS16495.
>
> Furthermore, ADIS16495 devices support the acceleration maximum range of
> 8g, while ADIS16497 devices go up to 40g.
>
> Datasheets:
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/adis16495.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/adis16497.pdf
>
> Signed-off-by: Stefan Popa <[email protected]>
Looks fine, but will probably change with the tweaks suggested for earlier
patches.

Thanks,

Jonathan

> ---
> drivers/iio/imu/adis16480.c | 72 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 72 insertions(+)
>
> diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> index cc53825..c30acfdb 100644
> --- a/drivers/iio/imu/adis16480.c
> +++ b/drivers/iio/imu/adis16480.c
> @@ -714,6 +714,12 @@ enum adis16480_variant {
> ADIS16480,
> ADIS16485,
> ADIS16488,
> + ADIS16495_1,
> + ADIS16495_2,
> + ADIS16495_3,
> + ADIS16497_1,
> + ADIS16497_2,
> + ADIS16497_3,
> };
>
> static const struct adis16480_chip_info adis16480_chip_info[] = {
> @@ -758,6 +764,60 @@ static const struct adis16480_chip_info adis16480_chip_info[] = {
> .accel_max_scale = 18,
> .temp_max_scale = 10619,
> },
> + [ADIS16495_1] = {
> + .channels = adis16485_channels,
> + .num_channels = ARRAY_SIZE(adis16485_channels),
> + .gyro_max_val = IIO_RAD_TO_DEGREE(20000),
> + .gyro_max_scale = 125,
> + .accel_max_val = IIO_M_S_2_TO_G(32000),
> + .accel_max_scale = 8,
> + .temp_max_scale = 4800,
> + },
> + [ADIS16495_2] = {
> + .channels = adis16485_channels,
> + .num_channels = ARRAY_SIZE(adis16485_channels),
> + .gyro_max_val = IIO_RAD_TO_DEGREE(18000),
> + .gyro_max_scale = 450,
> + .accel_max_val = IIO_M_S_2_TO_G(32000),
> + .accel_max_scale = 8,
> + .temp_max_scale = 4800,
> + },
> + [ADIS16495_3] = {
> + .channels = adis16485_channels,
> + .num_channels = ARRAY_SIZE(adis16485_channels),
> + .gyro_max_val = IIO_RAD_TO_DEGREE(20000),
> + .gyro_max_scale = 2000,
> + .accel_max_val = IIO_M_S_2_TO_G(32000),
> + .accel_max_scale = 8,
> + .temp_max_scale = 4800,
> + },
> + [ADIS16497_1] = {
> + .channels = adis16485_channels,
> + .num_channels = ARRAY_SIZE(adis16485_channels),
> + .gyro_max_val = IIO_RAD_TO_DEGREE(20000),
> + .gyro_max_scale = 125,
> + .accel_max_val = IIO_M_S_2_TO_G(32000),
> + .accel_max_scale = 40,
> + .temp_max_scale = 4800,
> + },
> + [ADIS16497_2] = {
> + .channels = adis16485_channels,
> + .num_channels = ARRAY_SIZE(adis16485_channels),
> + .gyro_max_val = IIO_RAD_TO_DEGREE(18000),
> + .gyro_max_scale = 450,
> + .accel_max_val = IIO_M_S_2_TO_G(32000),
> + .accel_max_scale = 40,
> + .temp_max_scale = 4800,
> + },
> + [ADIS16497_3] = {
> + .channels = adis16485_channels,
> + .num_channels = ARRAY_SIZE(adis16485_channels),
> + .gyro_max_val = IIO_RAD_TO_DEGREE(20000),
> + .gyro_max_scale = 2000,
> + .accel_max_val = IIO_M_S_2_TO_G(32000),
> + .accel_max_scale = 40,
> + .temp_max_scale = 4800,
> + },
> };
>
> static const struct iio_info adis16480_info = {
> @@ -1000,6 +1060,12 @@ static const struct spi_device_id adis16480_ids[] = {
> { "adis16480", ADIS16480 },
> { "adis16485", ADIS16485 },
> { "adis16488", ADIS16488 },
> + { "adis16495-1", ADIS16495_1 },
> + { "adis16495-2", ADIS16495_2 },
> + { "adis16495-3", ADIS16495_3 },
> + { "adis16497-1", ADIS16497_1 },
> + { "adis16497-2", ADIS16497_2 },
> + { "adis16497-3", ADIS16497_3 },
> { }
> };
> MODULE_DEVICE_TABLE(spi, adis16480_ids);
> @@ -1009,6 +1075,12 @@ static const struct of_device_id adis16480_of_match[] = {
> { .compatible = "adi,adis16480" },
> { .compatible = "adi,adis16485" },
> { .compatible = "adi,adis16488" },
> + { .compatible = "adi,adis16495-1" },
> + { .compatible = "adi,adis16495-2" },
> + { .compatible = "adi,adis16495-3" },
> + { .compatible = "adi,adis16497-1" },
> + { .compatible = "adi,adis16497-2" },
> + { .compatible = "adi,adis16497-3" },
> { },
> };
> MODULE_DEVICE_TABLE(of, adis16480_of_match);


2019-02-20 10:44:07

by Popa, Stefan Serban

[permalink] [raw]
Subject: Re: [PATCH 2/6] iio: imu: adis16480: Add support for configurable drdy indicator

On Mi, 2019-02-20 at 10:29 +0000, Jonathan Cameron wrote:
> [External]
>
>
> On Tue, 19 Feb 2019 19:12:14 +0200
> Stefan Popa <[email protected]> wrote:
>
> >
> > The FNCTIO_CTRL register provides configuration control for each I/O
> > pin
> > (DIO1, DIO2, DIO3 and DIO4).
> >
> > This patch adds the option to configure each DIOx pin as data ready
> > indicator with positive or negative polarity by reading the
> > 'interrupts'
> > and 'interrupt-names' properties from the devicetree. The
> > 'interrupt-names' property is optional, if it is not specified, then
> > the
> > factory default DIO2 data ready signal is used.
> >
> > Signed-off-by: Stefan Popa <[email protected]>
> Other than follow on from the previous patch change of the default, this
> looks fine to me.
>
> One question inline.
>
> Jonathan
Hi Jonathan,

Thank you for the review!
I will fall back on the 'wrong' default in the previous patch.
Answer inline.

-Stefan

> >
> > ---
> >  drivers/iio/imu/adis16480.c | 76
> > +++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >
> > diff --git a/drivers/iio/imu/adis16480.c b/drivers/iio/imu/adis16480.c
> > index d222188..38ba0c1 100644
> > --- a/drivers/iio/imu/adis16480.c
> > +++ b/drivers/iio/imu/adis16480.c
> > @@ -10,6 +10,7 @@
> >   */
> >
> >  #include <linux/bitfield.h>
> > +#include <linux/of_irq.h>
> >  #include <linux/interrupt.h>
> >  #include <linux/delay.h>
> >  #include <linux/mutex.h>
> > @@ -109,6 +110,10 @@
> >  #define
> > ADIS16480_FIR_COEF_D(x)                      ADIS16480_FIR_COEF(0x0B,
> > (x))
> >
> >  /* ADIS16480_REG_FNCTIO_CTRL */
> > +#define ADIS16480_DRDY_SEL_MSK               GENMASK(1, 0)
> > +#define
> > ADIS16480_DRDY_SEL(x)                FIELD_PREP(ADIS16480_DRDY_SEL_MSK,
> > x)
> > +#define ADIS16480_DRDY_POL_MSK               BIT(2)
> > +#define
> > ADIS16480_DRDY_POL(x)                FIELD_PREP(ADIS16480_DRDY_POL_MSK,
> > x)
> >  #define ADIS16480_DRDY_EN_MSK                BIT(3)
> >  #define ADIS16480_DRDY_EN(x)         FIELD_PREP(ADIS16480_DRDY_EN_MSK,
> > x)
> >
> > @@ -121,12 +126,26 @@ struct adis16480_chip_info {
> >       unsigned int accel_max_scale;
> >  };
> >
> > +enum adis16480_int_pin {
> > +     ADIS16480_PIN_DIO1,
> > +     ADIS16480_PIN_DIO2,
> > +     ADIS16480_PIN_DIO3,
> > +     ADIS16480_PIN_DIO4
> > +};
> > +
> >  struct adis16480 {
> >       const struct adis16480_chip_info *chip_info;
> >
> >       struct adis adis;
> >  };
> >
> > +static const char * const adis16480_int_pin_names[4] = {
> > +     [ADIS16480_PIN_DIO1] = "DIO1",
> > +     [ADIS16480_PIN_DIO2] = "DIO2",
> > +     [ADIS16480_PIN_DIO3] = "DIO3",
> > +     [ADIS16480_PIN_DIO4] = "DIO4",
> > +};
> > +
> >  #ifdef CONFIG_DEBUG_FS
> >
> >  static ssize_t adis16480_show_firmware_revision(struct file *file,
> > @@ -840,6 +859,59 @@ static const struct adis_data adis16480_data = {
> >       .enable_irq = adis16480_enable_irq,
> >  };
> >
> > +static int adis16480_config_irq_pin(struct device_node *of_node,
> > +                                 struct adis16480 *st)
> > +{
> > +     struct irq_data *desc;
> > +     enum adis16480_int_pin pin;
> > +     unsigned int irq_type;
> > +     uint16_t val;
> > +     int i, irq = 0;
> > +
> > +     desc = irq_get_irq_data(st->adis.spi->irq);
> > +     if (!desc) {
> > +             dev_err(&st->adis.spi->dev, "Could not find IRQ %d\n",
> > irq);
> > +             return -EINVAL;
> > +     }
> > +
> > +     /* Disable data ready */
> > +     val = ADIS16480_DRDY_EN(0);
> Does it default to on after reset?
> That's a little unusual and nasty.  If not, why are you disabling here?
Yes, the default after reset is on. 
> >
> > +
> > +     /*
> > +      * Get the interrupt from the devicetre by reading the
> > +      * interrupt-names property. If it is not specified, use
> > +      * the default interrupt on DIO2 pin.
> > +      */
> > +     pin = ADIS16480_PIN_DIO2;
> > +     for (i = 0; i < ARRAY_SIZE(adis16480_int_pin_names); i++) {
> > +             irq = of_irq_get_byname(of_node,
> > adis16480_int_pin_names[i]);
> > +             if (irq > 0) {
> > +                     pin = i;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     val |= ADIS16480_DRDY_SEL(pin);
> > +
> > +     /*
> > +      * Get the interrupt line behaviour. The data ready polarity can
> > be
> > +      * configured as positive or negative, corresponding to
> > +      * IRQF_TRIGGER_RISING or IRQF_TRIGGER_FALLING respectively.
> > +      */
> > +     irq_type = irqd_get_trigger_type(desc);
> > +     if (irq_type == IRQF_TRIGGER_RISING) { /* Default */
> > +             val |= ADIS16480_DRDY_POL(1);
> > +     } else if (irq_type == IRQF_TRIGGER_FALLING) {
> > +             val |= ADIS16480_DRDY_POL(0);
> > +     } else {
> > +             dev_err(&st->adis.spi->dev,
> > +                     "Invalid interrupt type 0x%x specified\n",
> > irq_type);
> > +             return -EINVAL;
> > +     }
> > +     /* Write the data ready configuration to the FNCTIO_CTRL register
> > */
> > +     return adis_write_reg_16(&st->adis, ADIS16480_REG_FNCTIO_CTRL,
> > val);
> > +}
> > +
> >  static int adis16480_probe(struct spi_device *spi)
> >  {
> >       const struct spi_device_id *id = spi_get_device_id(spi);
> > @@ -867,6 +939,10 @@ static int adis16480_probe(struct spi_device *spi)
> >       if (ret)
> >               return ret;
> >
> > +     ret =  adis16480_config_irq_pin(spi->dev.of_node, st);
> > +     if (ret)
> > +             return ret;
> > +
> >       ret = adis_setup_buffer_and_trigger(&st->adis, indio_dev, NULL);
> >       if (ret)
> >               return ret;

2019-02-20 10:46:47

by Jonathan Cameron

[permalink] [raw]
Subject: Re: [PATCH 6/6] iio: imu: adis16480: Add docs for ADIS16480 IMU

On Tue, 19 Feb 2019 19:12:18 +0200
Stefan Popa <[email protected]> wrote:

> Document support for ADIS16480 Inertial Measurement Unit.
>
> Signed-off-by: Stefan Popa <[email protected]>
Make sure to CC the dt list as well as Rob. Just because it's normally
Rob who is kind enough to review our patches, doesn't mean we shouldn't
bombard any other interested parties with them as well ;)

Looks good to me, with the exception of the DIO1 vs 2 default
discussed in the earlier patch.

Thanks,

Jonathan

> ---
> .../devicetree/bindings/iio/imu/adi,adis16480.txt | 49 ++++++++++++++++++++++
> MAINTAINERS | 1 +
> 2 files changed, 50 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt
>
> diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt b/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt
> new file mode 100644
> index 0000000..dacf5f7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16480.txt
> @@ -0,0 +1,49 @@
> +
> +Analog Devices ADIS16480 and similar IMUs
> +
> +Required properties for the ADIS16480:
> +
> +- compatible: Must be one of
> + * "adi,adis16375"
> + * "adi,adis16480"
> + * "adi,adis16485"
> + * "adi,adis16488"
> + * "adi,adis16495-1"
> + * "adi,adis16495-2"
> + * "adi,adis16495-3"
> + * "adi,adis16497-1"
> + * "adi,adis16497-2"
> + * "adi,adis16497-3"
> +- reg: SPI chip select number for the device
> +- spi-max-frequency: Max SPI frequency to use
> + see: Documentation/devicetree/bindings/spi/spi-bus.txt
> +- spi-cpha: See Documentation/devicetree/bindings/spi/spi-bus.txt
> +- spi-cpol: See Documentation/devicetree/bindings/spi/spi-bus.txt
> +- interrupts: interrupt mapping for IRQ, accepted values are:
> + * IRQF_TRIGGER_RISING
> + * IRQF_TRIGGER_FALLING
> +
> +Optional properties:
> +
> +- interrupt-names: Data ready line selection. Valid values are:
> + * DIO1
> + * DIO2
> + * DIO3
> + * DIO4
> + If this field is left empty, the factory default assigns DIO2 as data
> + ready signal.
> +- reset-gpios: must be the device tree identifier of the RESET pin. As the line
> + is active low, it should be marked GPIO_ACTIVE_LOW.
> +
> +Example:
> +
> + imu@0 {
> + compatible = "adi,adis16495-1";
> + reg = <0>;
> + spi-max-frequency = <3200000>;
> + spi-cpol;
> + spi-cpha;
> + interrupts = <25 IRQF_TRIGGER_FALLING>;
> + interrupt-parent = <&gpio>;
> + interrupt-names = "DIO2";
> + };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e4091ac..beecd1e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -942,6 +942,7 @@ F: drivers/dma/dma-axi-dmac.c
> ANALOG DEVICES INC IIO DRIVERS
> M: Lars-Peter Clausen <[email protected]>
> M: Michael Hennerich <[email protected]>
> +M: Stefan Popa <[email protected]>
> W: http://wiki.analog.com/
> W: http://ez.analog.com/community/linux-device-drivers
> S: Supported