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

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 5 Mar 2020 11:39:36 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:

> 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
> 
Agreed.   No one tends to buy an expensive IMU to just sample one channel
quickly!  Hence I doubt anyone will ever run it in a mode where burst
mode doesn't make sense.

Thanks,

Jonathan