Re: [PATCH] IIO: IMU: ADIS16400: Fix miscellaneous issues

[Michael Hennerich <michael.hennerich@analog.com>]

On 03/07/2011 01:14 PM, Jonathan Cameron wrote:
> On 03/07/11 11:30, michael.hennerich@analog.com wrote:
>   
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> Fix up SPI messages cs_change behavior
>>     
> Is the removal of the cs_change =1 for the first 16 bytes a fix
> or just a cleanup?  Looks from the datasheet like this is a don't
> care case.
>   
Hi Jonathan,

I checked with the product line. The cs_change after the first 16 bit in
a read operation is suggested, but not required.
Let me do some more tests here. I think I can blame the Blackfin SPI bus
driver, since it doesn't work on Blackfin.
I've seen this before, during slow SPI transfers the CS de-asserting to
fast, sometimes violating slave timing requirements.  
>   
>> Add delay after self test
>> Fix product ID check, skip embedded revision number.
>> Fix addresses of GYRO and ACCEL calibration offset.
>> Make sure only enabled scan_elements are pushed into the ring
>>     
> Excellent.
>
> I'll try and spend some time rebasing the driver merge patches on
> top of this.  Will be nice to finally squish all these similar
> parts together into a unified driver.
>
> If I'm being really really fussy there are a couple of nitpicks to
> do with white space inline.
>
> This one should probably go to stable as well.
>   
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>     
> Acked-by: Jonathan Cameron <jic23@cam.ac.uk>
>   
>> ---
>>  drivers/staging/iio/imu/adis16400.h      |    5 +++--
>>  drivers/staging/iio/imu/adis16400_core.c |   23 +++++++++++------------
>>  drivers/staging/iio/imu/adis16400_ring.c |   14 +++++++++-----
>>  3 files changed, 23 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/staging/iio/imu/adis16400.h b/drivers/staging/iio/imu/adis16400.h
>> index 6ff33e1..172b09c 100644
>> --- a/drivers/staging/iio/imu/adis16400.h
>> +++ b/drivers/staging/iio/imu/adis16400.h
>> @@ -17,7 +17,8 @@
>>  #ifndef SPI_ADIS16400_H_
>>  #define SPI_ADIS16400_H_
>>
>> -#define ADIS16400_STARTUP_DELAY      220 /* ms */
>> +#define ADIS16400_STARTUP_DELAY      290 /* ms */
>> +#define ADIS16400_MTEST_DELAY 90 /* ms */
>>
>>  #define ADIS16400_READ_REG(a)    a
>>  #define ADIS16400_WRITE_REG(a) ((a) | 0x80)
>> @@ -67,7 +68,7 @@
>>  #define ADIS16400_AUX_DAC   0x4A /* Auxiliary DAC data */
>>
>>  #define ADIS16400_PRODUCT_ID 0x56 /* Product identifier */
>> -#define ADIS16400_PRODUCT_ID_DEFAULT 0x4015  /* Datasheet says 0x4105, I get 0x4015 */
>> +#define ADIS16400_PRODUCT_ID_DEFAULT 0x4000
>>
>>  #define ADIS16400_ERROR_ACTIVE                       (1<<14)
>>  #define ADIS16400_NEW_DATA                   (1<<14)
>> diff --git a/drivers/staging/iio/imu/adis16400_core.c b/drivers/staging/iio/imu/adis16400_core.c
>> index cfb108a..1f9fff6 100644
>> --- a/drivers/staging/iio/imu/adis16400_core.c
>> +++ b/drivers/staging/iio/imu/adis16400_core.c
>> @@ -6,6 +6,7 @@
>>   *
>>   * Copyright (c) 2009 Manuel Stahl <manuel.stahl@iis.fraunhofer.de>
>>   * Copyright (c) 2007 Jonathan Cameron <jic23@cam.ac.uk>
>> + * Copyright (c) 2011 Analog Devices Inc.
>>   *
>>   * This program is free software; you can redistribute it and/or modify
>>   * it under the terms of the GNU General Public License version 2 as
>> @@ -88,12 +89,10 @@ static int adis16400_spi_write_reg_16(struct device *dev,
>>                       .tx_buf = st->tx,
>>                       .bits_per_word = 8,
>>                       .len = 2,
>> -                     .cs_change = 1,
>>               }, {
>>                       .tx_buf = st->tx + 2,
>>                       .bits_per_word = 8,
>>                       .len = 2,
>> -                     .cs_change = 1,
>>               },
>>       };
>>
>> @@ -132,12 +131,10 @@ static int adis16400_spi_read_reg_16(struct device *dev,
>>                       .tx_buf = st->tx,
>>                       .bits_per_word = 8,
>>                       .len = 2,
>> -                     .cs_change = 1,
>>               }, {
>>                       .rx_buf = st->rx,
>>                       .bits_per_word = 8,
>>                       .len = 2,
>> -                     .cs_change = 1,
>>               },
>>       };
>>
>> @@ -375,7 +372,7 @@ static int adis16400_self_test(struct device *dev)
>>               dev_err(dev, "problem starting self test");
>>               goto err_ret;
>>       }
>> -
>> +     msleep(ADIS16400_MTEST_DELAY);
>>       adis16400_check_status(dev);
>>
>>  err_ret:
>> @@ -454,6 +451,7 @@ static int adis16400_initial_setup(struct adis16400_state *st)
>>               goto err_ret;
>>       }
>>
>> +
>>       /* Read status register to check the result */
>>       ret = adis16400_check_status(dev);
>>       if (ret) {
>> @@ -471,10 +469,11 @@ static int adis16400_initial_setup(struct adis16400_state *st)
>>       if (ret)
>>               goto err_ret;
>>
>> -     if (prod_id != ADIS16400_PRODUCT_ID_DEFAULT)
>> +     if ((prod_id & 0xF000) != ADIS16400_PRODUCT_ID_DEFAULT)
>>               dev_warn(dev, "unknown product id");
>>
>> -     printk(KERN_INFO DRIVER_NAME ": prod_id 0x%04x at CS%d (irq %d)\n",
>> +
>> +     dev_info(dev, ": prod_id 0x%04x at CS%d (irq %d)\n",
>>                       prod_id, st->us->chip_select, st->us->irq);
>>
>>       /* use high spi speed if possible */
>> @@ -497,12 +496,12 @@ err_ret:
>>                       _reg)
>>
>>  static ADIS16400_DEV_ATTR_CALIBBIAS(GYRO_X, ADIS16400_XGYRO_OFF);
>> -static ADIS16400_DEV_ATTR_CALIBBIAS(GYRO_Y, ADIS16400_XGYRO_OFF);
>> -static ADIS16400_DEV_ATTR_CALIBBIAS(GYRO_Z, ADIS16400_XGYRO_OFF);
>> +static ADIS16400_DEV_ATTR_CALIBBIAS(GYRO_Y, ADIS16400_YGYRO_OFF);
>> +static ADIS16400_DEV_ATTR_CALIBBIAS(GYRO_Z, ADIS16400_ZGYRO_OFF);
>>
>>  static ADIS16400_DEV_ATTR_CALIBBIAS(ACCEL_X, ADIS16400_XACCL_OFF);
>> -static ADIS16400_DEV_ATTR_CALIBBIAS(ACCEL_Y, ADIS16400_XACCL_OFF);
>> -static ADIS16400_DEV_ATTR_CALIBBIAS(ACCEL_Z, ADIS16400_XACCL_OFF);
>> +static ADIS16400_DEV_ATTR_CALIBBIAS(ACCEL_Y, ADIS16400_YACCL_OFF);
>> +static ADIS16400_DEV_ATTR_CALIBBIAS(ACCEL_Z, ADIS16400_ZACCL_OFF);
>>
>>
>>  static IIO_DEV_ATTR_IN_NAMED_RAW(0, supply, adis16400_read_14bit_signed,
>> @@ -647,7 +646,7 @@ static int __devinit adis16400_probe(struct spi_device *spi)
>>
>>       ret = iio_ring_buffer_register(st->indio_dev->ring, 0);
>>       if (ret) {
>> -             printk(KERN_ERR "failed to initialize the ring\n");
>> +             dev_err(&spi->dev, "failed to initialize the ring\n");
>>               goto error_unreg_ring_funcs;
>>       }
>>
>> diff --git a/drivers/staging/iio/imu/adis16400_ring.c b/drivers/staging/iio/imu/adis16400_ring.c
>> index 33293fb..5a04e38 100644
>> --- a/drivers/staging/iio/imu/adis16400_ring.c
>> +++ b/drivers/staging/iio/imu/adis16400_ring.c
>> @@ -122,12 +122,10 @@ static int adis16400_spi_read_burst(struct device *dev, u8 *rx)
>>                       .tx_buf = st->tx,
>>                       .bits_per_word = 8,
>>                       .len = 2,
>> -                     .cs_change = 0,
>>               }, {
>>                       .rx_buf = rx,
>>                       .bits_per_word = 8,
>>                       .len = 24,
>> -                     .cs_change = 1,
>>               },
>>       };
>>
>> @@ -162,9 +160,10 @@ static void adis16400_trigger_bh_to_ring(struct work_struct *work_s)
>>                              work_trigger_to_ring);
>>       struct iio_ring_buffer *ring = st->indio_dev->ring;
>>
>> -     int i = 0;
>> +     int i = 0, j;
>>       s16 *data;
>>       size_t datasize = ring->access.get_bytes_per_datum(ring);
>> +     unsigned long mask = ring->scan_mask;
>>
>>       data = kmalloc(datasize , GFP_KERNEL);
>>       if (data == NULL) {
>> @@ -174,9 +173,14 @@ static void adis16400_trigger_bh_to_ring(struct work_struct *work_s)
>>
>>       if (ring->scan_count)
>>               if (adis16400_spi_read_burst(&st->indio_dev->dev, st->rx) >= 0)
>> -                     for (; i < ring->scan_count; i++)
>> +                     for (; i < ring->scan_count; i++) {
>> +                             j = __ffs(mask);
>> +                             mask &= ~(1 << j);
>>                               data[i] = be16_to_cpup(
>> -                                     (__be16 *)&(st->rx[i*2]));
>> +                                     (__be16 *)&(st->rx[j*2]));
>> +
>>     
> Nitpick but this blank line is unnecessary
>   
>> +                     }
>>     
> As is this one.
>   
>> +
>>
>>       /* Guaranteed to be aligned with 8 byte boundary */
>>       if (ring->scan_timestamp)
>>     
>   


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif