Re: [PATCH 4/5] iio: imu: Add support for adis16475

Lars-Peter Clausen
2020-03-05T10:39:44Z

On 3/5/20 10:58 AM, Sa, Nuno wrote:
> On Wed, 2020-03-04 at 17:59 +0000, Sa, Nuno wrote:
>> [External]
>>
>> On Tue, 2020-03-03 at 21:08 +0000, Jonathan Cameron wrote:
>>> [External]
>>>
>>> On Tue, 25 Feb 2020 13:41:51 +0100
>>> Nuno Sá <nuno.sa@analog.com> wrote:
>>>
>>>> Support ADIS16475 and similar IMU devices. These devices are
>>>> a precision, miniature MEMS inertial measurement unit (IMU) that
>>>> includes a triaxial gyroscope and a triaxial accelerometer. Each
>>>> inertial sensor combines with signal conditioning that optimizes
>>>> dynamic performance.
>>>>
>>>> The driver adds support for the following devices:
>>>> * adis16470, adis16475, adis16477, adis16465, adis16467,
>>>> adis16500,
>>>> adis16505, adis16507.
>>>>
>>>> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
>>> A few bits and pieces inline.
>>>
>>> Thanks,
>>>
>>> Jonathan
>>>
>>>
>>>> ---
>>>> .../ABI/testing/sysfs-bus-iio-imu-adis16475 | 7 +
>>>> MAINTAINERS | 8 +
>>>> drivers/iio/imu/Kconfig | 13 +
>>>> drivers/iio/imu/Makefile | 1 +
>>>> drivers/iio/imu/adis16475.c | 1304
>>>> +++++++++++++++++
>>>> 5 files changed, 1333 insertions(+)
>>>> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-imu-
>>>> adis16475
>>>> create mode 100644 drivers/iio/imu/adis16475.c
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-imu-
>>>> adis16475
>>>> b/Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475
>>>> new file mode 100644
>>>> index 000000000000..e2c3776035ea
>>>> --- /dev/null
>>>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475
>>>> @@ -0,0 +1,7 @@
>>>> +What: /sys/bus/iio/devices/iio:deviceX/burst_mode_ena
>>>> ble
>>>> +KernelVersion:
>>>> +Contact: linux-iio@vger.kernel.org
>>>> +Description:
>>>> + Use the device burst read mode when reading buffered
>>>> + data. This mode provides a way to read a batch of
>>>> + output data registers, using a continuous stream of
>>>> bits.
>>> See comment on this below. I'm not keen on this being exposed to
>>> userspace
>>> because it will rarely have any idea how to set it.
>>>
>>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>>> index 8fa40c3eb72a..f11262f1f3bb 100644
>>>> --- a/MAINTAINERS
>>>> +++ b/MAINTAINERS
>>>> @@ -1008,6 +1008,14 @@ W:
>>>> http://ez.analog.com/community/linux-device-drivers
>>>> F: drivers/iio/imu/adis16460.c
>>>> F: Documentation/devicetree/bindings/iio/imu/adi,adis16460
>>>> .yaml
>>>>
>>>> +ANALOG DEVICES INC ADIS16475 DRIVER
>>>> +M: Nuno Sa <nuno.sa@analog.com>
>>>> +L: linux-iio@vger.kernel.org
>>>> +W: http://ez.analog.com/community/linux-device-drivers
>>>> +S: Supported
>>>> +F: drivers/iio/imu/adis16475.c
>>>> +F: Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475
>>>> +
>>>> ANALOG DEVICES INC ADM1177 DRIVER
>>>> M: Beniamin Bia <beniamin.bia@analog.com>
>>>> M: Michael Hennerich <Michael.Hennerich@analog.com>
>>>> diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
>>>> index 60bb1029e759..fc4123d518bc 100644
>>>> --- a/drivers/iio/imu/Kconfig
>>>> +++ b/drivers/iio/imu/Kconfig
>>>> @@ -29,6 +29,19 @@ config ADIS16460
>>>> To compile this driver as a module, choose M here: the module
>>>> will be
>>>> called adis16460.
>>>>
>>>> +config ADIS16475
>>>> + tristate "Analog Devices ADIS16475 and similar IMU driver"
>>>> + depends on SPI
>>>> + select IIO_ADIS_LIB
>>>> + select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
>>>> + help
>>>> + Say yes here to build support for Analog Devices ADIS16470,
>>>> ADIS16475,
>>>> + ADIS16477, ADIS16465, ADIS16467, ADIS16500, ADIS16505,
>>>> ADIS16507 inertial
>>>> + sensors.
>>>> +
>>>> + To compile this driver as a module, choose M here: the module
>>>> will be
>>>> + called adis16475.
>>>> +
>>>> config ADIS16480
>>>> tristate "Analog Devices ADIS16480 and similar IMU driver"
>>>> depends on SPI
>>>> diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
>>>> index 5237fd4bc384..88b2c4555230 100644
>>>> --- a/drivers/iio/imu/Makefile
>>>> +++ b/drivers/iio/imu/Makefile
>>>> @@ -6,6 +6,7 @@
>>>> # When adding new entries keep the list in alphabetical order
>>>> obj-$(CONFIG_ADIS16400) += adis16400.o
>>>> obj-$(CONFIG_ADIS16460) += adis16460.o
>>>> +obj-$(CONFIG_ADIS16475) += adis16475.o
>>>> obj-$(CONFIG_ADIS16480) += adis16480.o
>>>>
>>>> adis_lib-y += adis.o
>>>> diff --git a/drivers/iio/imu/adis16475.c
>>>> b/drivers/iio/imu/adis16475.c
>>>> new file mode 100644
>>>> index 000000000000..f7c637734ec8
>>>> --- /dev/null
>>>> +++ b/drivers/iio/imu/adis16475.c
>>>> @@ -0,0 +1,1304 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * ADIS16475 IMU driver
>>>> + *
>>>> + * Copyright 2019 Analog Devices Inc.
>>>> + */
>>>> +#include <asm/unaligned.h>
>>>> +#include <linux/bitfield.h>
>>>> +#include <linux/bitops.h>
>>>> +#include <linux/clk.h>
>>>> +#include <linux/debugfs.h>
>>>> +#include <linux/delay.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/iio/buffer.h>
>>>> +#include <linux/iio/iio.h>
>>>> +#include <linux/iio/imu/adis.h>
>>>> +#include <linux/iio/sysfs.h>
>>>> +#include <linux/iio/trigger_consumer.h>
>>>> +#include <linux/irq.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/spi/spi.h>
>>>> +
>>>> +#define ADIS16475_REG_DIAG_STAT 0x02
>>>> +#define ADIS16475_REG_X_GYRO_L 0x04
>>>> +#define ADIS16475_REG_Y_GYRO_L 0x08
>>>> +#define ADIS16475_REG_Z_GYRO_L 0x0C
>>>> +#define ADIS16475_REG_X_ACCEL_L 0x10
>>>> +#define ADIS16475_REG_Y_ACCEL_L 0x14
>>>> +#define ADIS16475_REG_Z_ACCEL_L 0x18
>>>> +#define ADIS16475_REG_TEMP_OUT 0x1c
>>>> +#define ADIS16475_REG_X_GYRO_BIAS_L 0x40
>>>> +#define ADIS16475_REG_Y_GYRO_BIAS_L 0x44
>>>> +#define ADIS16475_REG_Z_GYRO_BIAS_L 0x48
>>>> +#define ADIS16475_REG_X_ACCEL_BIAS_L 0x4c
>>>> +#define ADIS16475_REG_Y_ACCEL_BIAS_L 0x50
>>>> +#define ADIS16475_REG_Z_ACCEL_BIAS_L 0x54
>>>> +#define ADIS16475_REG_FILT_CTRL 0x5c
>>>> +#define ADIS16475_FILT_CTRL_MASK GENMASK(2, 0)
>>>> +#define ADIS16475_FILT_CTRL(x) FIELD_PREP(ADIS16475_FI
>>>> LT_CTRL_MASK, x)
>>>> +#define ADIS16475_REG_MSG_CTRL 0x60
>>>> +#define ADIS16475_MSG_CTRL_DR_POL_MASK BIT(0)
>>>> +#define ADIS16475_MSG_CTRL_DR_POL(x) \
>>>> + FIELD_PREP(ADIS16475_MSG_CTRL_DR_POL_MA
>>>> SK, x)
>>>> +#define ADIS16475_EXT_CLK_MASK GENMASK(4, 2)
>>>> +#define ADIS16475_EXT_CLK(x) FIELD_PREP(ADIS16475_EX
>>>> T_CLK_MASK, x)
>>>> +#define ADIS16475_REG_UP_SCALE 0x62
>>>> +#define ADIS16475_REG_DEC_RATE 0x64
>>>> +#define ADIS16475_REG_GLOB_CMD 0x68
>>>> +#define ADIS16475_REG_FIRM_REV 0x6c
>>>> +#define ADIS16475_REG_FIRM_DM 0x6e
>>>> +#define ADIS16475_REG_FIRM_Y 0x70
>>>> +#define ADIS16475_REG_PROD_ID 0x72
>>>> +#define ADIS16475_REG_SERIAL_NUM 0x74
>>>> +#define ADIS16475_REG_FLASH_CNT 0x7c
>>>> +#define ADIS16500_BURST32_MASK BIT(9)
>>>> +#define ADIS16500_BURST32(x) FIELD_PREP(ADIS16500_BU
>>>> RST32_MASK, x)
>>>> +/* number of data elements in burst mode */
>>>> +#define ADIS16475_BURST_MAX_DATA 10
>>>> +#define ADIS16475_MAX_SCAN_DATA 15
>>>> +
>>>> +enum clk_mode {
>>>> + ADIS16475_CLK_DIRECT = 1,
>>>> + ADIS16475_CLK_SCALED,
>>>> + ADIS16475_CLK_OUTPUT,
>>>> + ADIS16475_CLK_PULSE = 5,
>>>> +};
>>>> +
>>>> +struct adis16475_clks {
>>>> + const char *name;
>>>> + enum clk_mode clk_mode;
>>>> + u16 min_rate;
>>>> + u16 max_rate;
>>>> +};
>>>> +
>>>> +struct adis16475_chip_info {
>>>> + const struct iio_chan_spec *channels;
>>>> + const struct adis16475_clks *clks;
>>>> + const struct adis_data adis_data;
>>>> + u32 num_channels;
>>>> + u32 gyro_max_val;
>>>> + u32 gyro_max_scale;
>>>> + u32 accel_max_val;
>>>> + u32 accel_max_scale;
>>>> + u32 temp_scale;
>>>> + u32 int_clk;
>>>> + u16 max_dec;
>>>> + u8 num_clks;
>>>> + bool has_burst32;
>>>> +};
>>>> +
>>>> +struct adis16475 {
>>>> + const struct adis16475_chip_info *info;
>>>> + struct adis adis;
>>>> + u32 clk_freq;
>>>> + u32 cached_spi_speed_hz;
>>>> + bool burst32;
>>>> +};
>>>> +
>>>> +enum {
>>>> + ADIS16475_SCAN_GYRO_X,
>>>> + ADIS16475_SCAN_GYRO_Y,
>>>> + ADIS16475_SCAN_GYRO_Z,
>>>> + ADIS16475_SCAN_ACCEL_X,
>>>> + ADIS16475_SCAN_ACCEL_Y,
>>>> + ADIS16475_SCAN_ACCEL_Z,
>>>> + ADIS16475_SCAN_TEMP,
>>>> + ADIS16475_SCAN_DIAG_S_FLAGS,
>>>> + ADIS16475_SCAN_CRC_FAILURE,
>>>> +};
>>>> +
>>>> +#ifdef CONFIG_DEBUG_FS
>>>> +static ssize_t adis16475_show_firmware_revision(struct file
>>>> *file,
>>>> + char __user *userbuf,
>>>> + size_t count, loff_t
>>>> *ppos)
>>>> +{
>>>> + struct adis16475 *st = file->private_data;
>>>> + char buf[7];
>>>> + size_t len;
>>>> + u16 rev;
>>>> + int ret;
>>>> +
>>>> + ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FIRM_REV,
>>>> &rev);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + len = scnprintf(buf, sizeof(buf), "%x.%x\n", rev >> 8, rev &
>>>> 0xff);
>>>> +
>>>> + return simple_read_from_buffer(userbuf, count, ppos, buf, len);
>>>> +}
>>>> +
>>>> +static const struct file_operations
>>>> adis16475_firmware_revision_fops = {
>>>> + .open = simple_open,
>>>> + .read = adis16475_show_firmware_revision,
>>>> + .llseek = default_llseek,
>>>> + .owner = THIS_MODULE,
>>>> +};
>>>> +
>>>> +static ssize_t adis16475_show_firmware_date(struct file *file,
>>>> + char __user *userbuf,
>>>> + size_t count, loff_t *ppos)
>>>> +{
>>>> + struct adis16475 *st = file->private_data;
>>>> + u16 md, year;
>>>> + char buf[12];
>>>> + size_t len;
>>>> + int ret;
>>>> +
>>>> + ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FIRM_Y, &year);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FIRM_DM, &md);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + len = snprintf(buf, sizeof(buf), "%.2x-%.2x-%.4x\n", md >> 8,
>>>> md & 0xff,
>>>> + year);
>>>> +
>>>> + return simple_read_from_buffer(userbuf, count, ppos, buf, len);
>>>> +}
>>>> +
>>>> +static const struct file_operations adis16475_firmware_date_fops
>>>> =
>>>> {
>>>> + .open = simple_open,
>>>> + .read = adis16475_show_firmware_date,
>>>> + .llseek = default_llseek,
>>>> + .owner = THIS_MODULE,
>>>> +};
>>>> +
>>>> +static int adis16475_show_serial_number(void *arg, u64 *val)
>>>> +{
>>>> + struct adis16475 *st = arg;
>>>> + u16 serial;
>>>> + int ret;
>>>> +
>>>> + ret = adis_read_reg_16(&st->adis, ADIS16475_REG_SERIAL_NUM,
>>>> &serial);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + *val = serial;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +DEFINE_SIMPLE_ATTRIBUTE(adis16475_serial_number_fops,
>>>> + adis16475_show_serial_number, NULL,
>>>> "0x%.4llx\n");
>>>> +
>>>> +static int adis16475_show_product_id(void *arg, u64 *val)
>>>> +{
>>>> + struct adis16475 *st = arg;
>>>> + u16 prod_id;
>>>> + int ret;
>>>> +
>>>> + ret = adis_read_reg_16(&st->adis, ADIS16475_REG_PROD_ID,
>>>> &prod_id);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + *val = prod_id;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +DEFINE_SIMPLE_ATTRIBUTE(adis16475_product_id_fops,
>>>> + adis16475_show_product_id, NULL, "%llu\n");
>>>> +
>>>> +static int adis16475_show_flash_count(void *arg, u64 *val)
>>>> +{
>>>> + struct adis16475 *st = arg;
>>>> + u32 flash_count;
>>>> + int ret;
>>>> +
>>>> + ret = adis_read_reg_32(&st->adis, ADIS16475_REG_FLASH_CNT,
>>>> + &flash_count);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + *val = flash_count;
>>>> +
>>>> + return 0;
>>>> +}
>>>> +DEFINE_SIMPLE_ATTRIBUTE(adis16475_flash_count_fops,
>>>> + adis16475_show_flash_count, NULL, "%lld\n");
>>>> +
>>>> +static int adis16475_debugfs_init(struct iio_dev *indio_dev)
>>>> +{
>>>> + struct adis16475 *st = iio_priv(indio_dev);
>>>> +
>>>> + debugfs_create_file("serial_number", 0400, indio_dev-
>>>>> debugfs_dentry,
>>>> + st, &adis16475_serial_number_fops);
>>>> + debugfs_create_file("product_id", 0400, indio_dev-
>>>>> debugfs_dentry, st,
>>>> + &adis16475_product_id_fops);
>>>> + debugfs_create_file("flash_count", 0400, indio_dev-
>>>>> debugfs_dentry, st,
>>>> + &adis16475_flash_count_fops);
>>>> + debugfs_create_file("firmware_revision", 0400,
>>>> + indio_dev->debugfs_dentry, st,
>>>> + &adis16475_firmware_revision_fops);
>>>> + debugfs_create_file("firmware_date", 0400, indio_dev-
>>>>> debugfs_dentry,
>>>> + st, &adis16475_firmware_date_fops);
>>>> + return 0;
>>>> +}
>>>> +#else
>>>> +static int adis16475_debugfs_init(struct iio_dev *indio_dev)
>>>> +{
>>>> + return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> +static ssize_t adis16475_burst_mode_enable_get(struct device
>>>> *dev,
>>>> + struct device_attribute
>>>> *attr,
>>>> + char *buf)
>>>> +{
>>>> + struct adis16475 *st = iio_priv(dev_to_iio_dev(dev));
>>>> +
>>>> + return sprintf(buf, "%d\n", st->adis.burst->en);
>>>> +}
>>>> +
>>>> +static ssize_t adis16475_burst_mode_enable_set(struct device
>>>> *dev,
>>>> + struct device_attribute
>>>> *attr,
>>>> + const char *buf, size_t
>>>> len)
>>>> +{
>>>> + struct adis16475 *st = iio_priv(dev_to_iio_dev(dev));
>>>> + bool val;
>>>> + int ret;
>>>> +
>>>> + ret = kstrtobool(buf, &val);
>>>> + if (ret)
>>>> + return ret;
>>>> +
>>>> + if (val)
>>>> + /* 1MHz max in burst mode */
>>>> + st->adis.spi->max_speed_hz = 1000000;
>>>> + else
>>>> + st->adis.spi->max_speed_hz = st->cached_spi_speed_hz;
>>>> +
>>>> + st->adis.burst->en = val;
>>>> +
>>>> + return len;
>>>> +}
>>>> +
>>>> +static IIO_DEVICE_ATTR(burst_mode_enable, 0644,
>>>> + adis16475_burst_mode_enable_get,
>>>> + adis16475_burst_mode_enable_set, 0);
>>>> +
>>>> +static struct attribute *adis16475_attributes[] = {
>>>> + &iio_dev_attr_burst_mode_enable.dev_attr.attr,
>>> Hmm. Normally we try to avoid exposing this and make the decision
>>> automatically based on which channels are enabled.
>> Hmm. In that case, the decision would probably have to go to the
>> library since it is there that the "preparations" for buffered mode
>> are
>> done. Mostly, the data we are interested in the burst data is gyro,
>> accel and temp. So to make the decision based on which channels are
>> enabled is not that straight. Should we enable it only when all
>> channels are enabled? Some of them (and which)?
> I thought a little bit more about it and we could do some math in the
> lib to know when using burst mode becomes faster (depending on enabled
> scan elements). The library would need to know about the burst max spi
> frequency and also some minor changes in how burst_len is being
> calculated (drivers should explicitly set this rather than the
> library). The question is, is it worth it to go with this extra work
> :)?

You can leave it out of the initial version and always use burst mode
and then add support for this in a follow up patch if it is worth it.

- Lars