> From: Andy Shevchenko <andy.shevchenko@xxxxxxxxx> > Sent: Dienstag, 31. März 2020 20:16 > To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx> > Cc: linux-iio <linux-iio@xxxxxxxxxxxxxxx>; devicetree > <devicetree@xxxxxxxxxxxxxxx>; Jonathan Cameron <jic23@xxxxxxxxxx>; > Hartmut Knaack <knaack.h@xxxxxx>; Lars-Peter Clausen <lars@xxxxxxxxxx>; > Peter Meerwald-Stadler <pmeerw@xxxxxxxxxx>; Rob Herring > <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Ardelean, > Alexandru <alexandru.Ardelean@xxxxxxxxxx>; Hennerich, Michael > <Michael.Hennerich@xxxxxxxxxx> > Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475 > > [External] > > 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. > Ups. Leftover from v1. Thanks! > ... > > > +#include <asm/unaligned.h> > I thought we wanted alphabetic order... > 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? > Yeps. Not really needed... > > +#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. > Yes. For ` of_device_get_match_data ``. If changed by `device_get_match_data`, then I guess I can drop it.. > > +#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() > Will do that... > Also to the rest of same > > ... > > > + for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) { > > Why those margins? size-2 and 1 ? > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember why I did it like this. Using > 0 is the same and more "common"... > > + 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. > Honestly here I don't see any problems with indentation and it goes in conformity with other IMU drivers already in tree. So here, as long as anyone else has a problem with this, I prefer to keep it this way... > ... > > > +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. > This api is callbacked from the ADIS library... > ... > > > + 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? This is a nice point. So, honestly I made it like this to keep conformity with other drivers we have in our internal tree (in queue for upstream) and I also wondered about this. The only justification I can find to use unligned calls is to keep this independent from the ADIS lib (not sure if it makes sense) since we get the pointer from the library (allocated there). Now, if Im not missing nothing obvious we can access the buffer normally since it's being allocated with kmalloc which means we have ARCH_KMALLOC_MINALIGN (which is at least 8 if Im not mistaken). On top of this, the device sends the data as n 16 bits segments. So in theory, I guess we can ditch the overhead of the *unaligned calls if any objections? > > + valid = adis16475_validate_crc((u8 *)adis->buffer, crc, st->burst32); > > Why casting? > Not really needed... > > + if (!valid) { > > + dev_err(&adis->spi->dev, "Invalid crc\n"); > > + goto check_burst32; > > + } > > ... > > > + /* keep sparse happy */ > > Perhaps buffer should be declared as __be16. > Will do it if removing the *unaligned calls... > > + 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? > I guess. If someone does not include it in device tree... > ... > > > + { .compatible = "adi,adis16507-3", > > + .data = &adis16475_chip_info[ADIS16507_3] }, > > + { }, > > Comma is not needed. > Will remove it > ... > > > + st->info = of_device_get_match_data(&spi->dev); > > device_get_match_data() Will use it... > > > + if (!st->info) > > + return -EINVAL; > Thanks for the review - Nuno Sá > -- > With Best Regards, > Andy Shevchenko
