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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux