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

Jonathan Cameron
2020-03-07T11:27:24Z

...
> > > > +static irqreturn_t adis16475_trigger_handler(int irq, void *p)
> > > > +{
> > > > + struct iio_poll_func *pf = p;
> > > > + struct iio_dev *indio_dev = pf->indio_dev;
> > > > + struct adis16475 *st = iio_priv(indio_dev);
> > > > + struct adis *adis = &st->adis;
> > > > + int ret, bit, i = 0;
> > > > + u16 crc, data[ADIS16475_MAX_SCAN_DATA], *buffer, crc_res;
> > > > + /* offset until the first element after gyro and accel */
> > > > + const u8 offset = st->burst32 ? 13 : 7;
> > > > +
> > > > + ret = spi_sync(adis->spi, &adis->msg);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + buffer = (u16 *)adis->buffer;
> > > > +
> > > > + if (!(adis->burst && adis->burst->en))
> > > > + goto push_to_buffers;
> > > > +
> > > > + /* We always validate the crc to at least print a message */
> > > > + crc = get_unaligned_be16(&buffer[offset + 2]);
> > > > + crc_res = adis16475_validate_crc((u8 *)adis->buffer, crc,
> > > > + st->burst32);
> > > > + if (crc_res)
> > > > + dev_err(&adis->spi->dev, "Invalid crc\n");
> > > > +
> > > > + for_each_set_bit(bit, indio_dev->active_scan_mask,
> > > > + indio_dev->masklength) {
> > > > + /*
> > > > + * When burst mode is used, system flags is the first
> > > > data
> > > > + * channel in the sequence, but the scan index is 7.
> > > > + */
> > > > + switch (bit) {
> > > > + case ADIS16475_SCAN_TEMP:
> > > > + data[i++] = get_unaligned(&buffer[offset]);
> > > > + break;
> > > > + case ADIS16475_SCAN_GYRO_X ... ADIS16475_SCAN_ACCEL_Z:
> > > > + /*
> > > > + * The first 2 bytes on the received data are
> > > > the
> > > > + * DIAG_STAT reg, hence the +1 offset here...
> > > > + */
> > > > + if (st->burst32) {
> > > > + /* upper 16 */
> > > > + data[i++] = get_unaligned(&buffer[bit *
> > > > 2 + 2]);
> > > > + /* lower 16 */
> > > > + data[i++] = get_unaligned(&buffer[bit *
> > > > 2 + 1]);
> > > > + } else {
> > > > + data[i++] = get_unaligned(&buffer[bit +
> > > > 1]);
> > > > + /* lower not used */
> > > > + data[i++] = 0;
> > > > + }
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + buffer = data;
> > > > +
> > > > +push_to_buffers:
> > > > + iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf-
> > > > > timestamp);
> > >
> > > I'm not sure data is the right size. It needs to have space to
> > > have
> > > an aligned
> > > timestamp at the end.
> >
> > Will double check this... Honestly I did not had the timestamp into
> > account so the size is probably wrong.
>
> I guess you are right. With all the channels enabled I think we need a
> 40 bytes buffer in order to have the aligned timestamp...
>

Sounds right.

> > > > +static const struct spi_device_id adis16475_ids[] = {
> > >
> > > Is it actually possible to instantiate this except by
> > > using the dt table below? If not, then move the 'data'
> > > part into that table and don't provide an spi_device_id
> > > table at all. It's not relevant to the possible ways
> > > of causing the driver to probe.
> >
> > I guess we could use the id table with devicetree even without the dt
> > table (even though it makes no sense).
> >
> > So, I can remove it but I was using the id->name to set the the
> > iio_dev
> > name which I guess is not the right way?
> >
> > What Im thinking is having a name/part number string in chip info
> > that
> > I can use to set the iio dev name. For parts that have the *-[1|2|3]
> > variations I could use the devicetree iio label property. Is this the
> > correct way of handling this?
>
> I was misunderstanding some stuff here. So, the *-[1|2|3] are valid
> part numbers so they can be in the `name`, right? So, labels come into
> play, for example, when we have multiple instances of the same part,
> right?

Absolutely fine to have the -1 etc. They are odd part numbers, but they
are the ones on the datasheet etc.

>
> > - Nuno Sá
> > > > + { "adis16470", ADIS16470 },
> > > > + { "adis16475-1", ADIS16475_1 },
> > > > + { "adis16475-2", ADIS16475_2 },
> > > > + { "adis16475-3", ADIS16475_3 },
> > > > + { "adis16477-1", ADIS16477_1 },
> > > > + { "adis16477-2", ADIS16477_2 },
> > > > + { "adis16477-3", ADIS16477_3 },
> > > > + { "adis16465-1", ADIS16465_1 },
> > > > + { "adis16465-2", ADIS16465_2 },
> > > > + { "adis16465-3", ADIS16465_3 },
> > > > + { "adis16467-1", ADIS16467_1 },
> > > > + { "adis16467-2", ADIS16467_2 },
> > > > + { "adis16467-3", ADIS16467_3 },
> > > > + { "adis16500", ADIS16500 },
> > > > + { "adis16505-1", ADIS16505_1 },
> > > > + { "adis16505-2", ADIS16505_2 },
> > > > + { "adis16505-3", ADIS16505_3 },
> > > > + { "adis16507-1", ADIS16507_1 },
> > > > + { "adis16507-2", ADIS16507_2 },
> > > > + { "adis16507-3", ADIS16507_3 },
> > > > + { }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(spi, adis16475_ids);
> > > > +
> > > > +static const struct of_device_id adis16475_of_match[] = {
> > > > + { .compatible = "adi,adis16470" },
> > > > + { .compatible = "adi,adis16475-1" },
> > > > + { .compatible = "adi,adis16475-2" },
> > > > + { .compatible = "adi,adis16475-3" },
> > > > + { .compatible = "adi,adis16477-1" },
> > > > + { .compatible = "adi,adis16477-2" },
> > > > + { .compatible = "adi,adis16477-3" },
> > > > + { .compatible = "adi,adis16465-1" },
> > > > + { .compatible = "adi,adis16465-2" },
> > > > + { .compatible = "adi,adis16465-3" },
> > > > + { .compatible = "adi,adis16467-1" },
> > > > + { .compatible = "adi,adis16467-2" },
> > > > + { .compatible = "adi,adis16467-3" },
> > > > + { .compatible = "adi,adis16500" },
> > > > + { .compatible = "adi,adis16505-1" },
> > > > + { .compatible = "adi,adis16505-2" },
> > > > + { .compatible = "adi,adis16505-3" },
> > > > + { .compatible = "adi,adis16507-1" },
> > > > + { .compatible = "adi,adis16507-2" },
> > > > + { .compatible = "adi,adis16507-3" },
> > > > + { },
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, adis16475_of_match);
> > > > +
> > > > +static struct spi_driver adis16475_driver = {
> > > > + .driver = {
> > > > + .name = "adis16475",
> > > > + .of_match_table = adis16475_of_match,
> > > > + },
> > > > + .id_table = adis16475_ids,
> > > > + .probe = adis16475_probe,
> > > > +};
> > > > +module_spi_driver(adis16475_driver);
> > > > +
> > > > +MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
> > > > +MODULE_DESCRIPTION("Analog Devices ADIS16475 IMU driver");
> > > > +MODULE_LICENSE("GPL");
>