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

[Jonathan Cameron <jic23@kernel.org>]

...
> > > > +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");  
>