On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@xxxxxxxxxx> 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. ... > +ANALOG DEVICES INC ADIS16475 DRIVER > +M: Nuno Sa <nuno.sa@xxxxxxxxxx> > +L: linux-iio@xxxxxxxxxxxxxxx > +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 Run parse-maintainers.pl to fix this. ... > +#include <asm/unaligned.h> Usually it goes after linux/* > +#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> What this is for? > +#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/of_device.h> Do you really need this? Perhaps mod_devicetable.h is what you are looking for. > +#include <linux/spi/spi.h> ... > +#ifdef CONFIG_DEBUG_FS > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_serial_number_fops, > + adis16475_show_serial_number, NULL, "0x%.4llx\n"); DEBUGFS ATTRIBUTE? > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_product_id_fops, > + adis16475_show_product_id, NULL, "%llu\n"); > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_flash_count_fops, > + adis16475_show_flash_count, NULL, "%lld\n"); Ditto. > +#else > +static int adis16475_debugfs_init(struct iio_dev *indio_dev) > +{ > + return 0; > +} > +#endif ... > + /* > + * If decimation is used, then gyro and accel data will have meaningful > + * bits on the LSB registers. This info is used on the trigger handler. > + */ > + if (!dec) > + clear_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag); > + else > + set_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag); assign_bit() Also to the rest of same ... > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { Why those margins? size-2 and 1 ? > + if (adis16475_3db_freqs[i] >= filter) > + break; > + } ... > +#define ADIS16475_GYRO_CHANNEL(_mod) \ > + ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \ > + ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ## _mod, 32, \ > + 32) It's not obvious that this is macro inside macro. Can you indent better? Ditto for the rest similar ones. ... > +static int adis16475_enable_irq(struct adis *adis, bool enable) > +{ > + /* > + * There is no way to gate the data-ready signal internally inside the > + * ADIS16475. We can only control it's polarity... > + */ > + if (enable) > + enable_irq(adis->spi->irq); > + else > + disable_irq(adis->spi->irq); > + > + return 0; > +} It's seems this function is bigger than in-place calls for enable or disable IRQ. ... > + return (crc == 0); Too many parentheses. ... > + ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL, > + ADIS16500_BURST32_MASK, en); > + if (ret < 0) ret > 0 has any meaning? Maybe drop all these ' < 0' parts from the code (where it's appropriate)? > + return; ... > + buffer = (u16 *)adis->buffer; Why the casting is needed? > + crc = get_unaligned_be16(&buffer[offset + 2]); If your buffer is aligned in the structure, you may simple use be16_to_cpu(). Same for the rest of get_unaligned*() calls. Or do you have unaligned data there? > + valid = adis16475_validate_crc((u8 *)adis->buffer, crc, st->burst32); Why casting? > + if (!valid) { > + dev_err(&adis->spi->dev, "Invalid crc\n"); > + goto check_burst32; > + } ... > + /* keep sparse happy */ Perhaps buffer should be declared as __be16. > + data[i++] = (__force u16)__val; ... > + desc = irq_get_irq_data(spi->irq); > + if (!desc) { > + dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq); > + return -EINVAL; > + } Is this even possible? ... > + { .compatible = "adi,adis16507-3", > + .data = &adis16475_chip_info[ADIS16507_3] }, > + { }, Comma is not needed. ... > + st->info = of_device_get_match_data(&spi->dev); device_get_match_data() > + if (!st->info) > + return -EINVAL; -- With Best Regards, Andy Shevchenko
