Re: [PATCH v3 1/4] Staging: iio: adis16209: Remove and add some comments and group the definitions
From: Shreeya Patel
Date: Sun Mar 04 2018 - 08:07:19 EST
On Sun, 2018-03-04 at 18:26 +0530, Himanshu Jha wrote:
> Hi Shreeya,
>
> On Sun, Mar 04, 2018 at 06:06:22PM +0530, Shreeya Patel wrote:
> >
> > Remove some unnecessay comments and group the control
> > register and register field macros together.
> > Some of the register names does not make it's puporse
> > very clear and hence, add some comments for more
> > information.
> > Also there are certain unit based comments which are not
> > providing sufficient information, so expand those comments.
> >
> > Signed-off-by: Shreeya Patel <shreeya.patel23498@xxxxxxxxx>
> > ---
> >
> > Changes in v3
> > Â -This patch is the combination of two patches from the
> > previous series. Also add some more comments.
> >
> >
> > Âdrivers/staging/iio/accel/adis16209.c | 132 ++++++++++----------
> > --------------
> > Â1 file changed, 39 insertions(+), 93 deletions(-)
> >
> > diff --git a/drivers/staging/iio/accel/adis16209.c
> > b/drivers/staging/iio/accel/adis16209.c
> > index 151120f..d8aef9c 100644
> > --- a/drivers/staging/iio/accel/adis16209.c
> > +++ b/drivers/staging/iio/accel/adis16209.c
> > @@ -21,135 +21,70 @@
> > Â#include <linux/iio/imu/adis.h>
> > Â
> > Â#define ADIS16209_STARTUP_DELAY_MS 220
> > -
> > -/* Flash memory write count */
> > Â#define ADIS16209_FLASH_CNT_REG 0x00
> > Â
> > -/* Output, power supply */
> > +/* Data Output Register Definitions */
> > Â#define ADIS16209_SUPPLY_OUT_REG 0x02
> > -
> > -/* Output, x-axis accelerometer */
> > Â#define ADIS16209_XACCL_OUT_REG 0x04
> > -
> > -/* Output, y-axis accelerometer */
> > Â#define ADIS16209_YACCL_OUT_REG 0x06
> > -
> > Â/* Output, auxiliary ADC input */
> > Â#define ADIS16209_AUX_ADC_REG 0x08
> > -
> > Â/* Output, temperature */
> > Â#define ADIS16209_TEMP_OUT_REG 0x0A
> > -
> > -/* Output, x-axis inclination */
> > +/* Output, +/- 90 degrees X-axis inclination */
> > Â#define ADIS16209_XINCL_OUT_REG 0x0C
> > -
> > -/* Output, y-axis inclination */
> > Â#define ADIS16209_YINCL_OUT_REG 0x0E
> > -
> > Â/* Output, +/-180 vertical rotational position */
> > Â#define ADIS16209_ROT_OUT_REG 0x10
> > Â
> > -/* Calibration, x-axis acceleration offset null */
> > +/*
> > + * Calibration Register Definitions.
> > + * Acceleration, inclination or rotation offset null.
> > + */
> > Â#define ADIS16209_XACCL_NULL_REG 0x12
> > -
> > -/* Calibration, y-axis acceleration offset null */
> > Â#define ADIS16209_YACCL_NULL_REG 0x14
> > -
> > -/* Calibration, x-axis inclination offset null */
> > Â#define ADIS16209_XINCL_NULL_REG 0x16
> > -
> > -/* Calibration, y-axis inclination offset null */
> > Â#define ADIS16209_YINCL_NULL_REG 0x18
> > -
> > -/* Calibration, vertical rotation offset null */
> > Â#define ADIS16209_ROT_NULL_REG 0x1A
> > Â
> > -/* Alarm 1 amplitude threshold */
> > +/* Alarm Register Definitions */
> > Â#define ADIS16209_ALM_MAG1_REG 0x20
> > -
> > -/* Alarm 2 amplitude threshold */
> > Â#define ADIS16209_ALM_MAG2_REG 0x22
> > -
> > -/* Alarm 1, sample period */
> > Â#define ADIS16209_ALM_SMPL1_REG 0x24
> > -
> > -/* Alarm 2, sample period */
> > Â#define ADIS16209_ALM_SMPL2_REG 0x26
> > -
> > -/* Alarm control */
> > Â#define ADIS16209_ALM_CTRL_REG 0x28
> > Â
> > -/* Auxiliary DAC data */
> > Â#define ADIS16209_AUX_DAC_REG 0x30
> > -
> > -/* General-purpose digital input/output control */
> > Â#define ADIS16209_GPIO_CTRL_REG 0x32
> > -
> > -/* Miscellaneous control */
> > -#define ADIS16209_MSC_CTRL_REG 0x34
> > -
> > -/* Internal sample period (rate) control */
> > Â#define ADIS16209_SMPL_PRD_REG 0x36
> > -
> > -/* Operation, filter configuration */
> > Â#define ADIS16209_AVG_CNT_REG 0x38
> > -
> > -/* Operation, sleep mode control */
> > Â#define ADIS16209_SLP_CNT_REG 0x3A
> > Â
> > -/* Diagnostics, system status register */
> > -#define ADIS16209_DIAG_STAT_REG 0x3C
> > -
> > -/* Operation, system command register */
> > -#define ADIS16209_GLOB_CMD_REG 0x3E
> > -
> > -/* MSC_CTRL */
> > -
> > -/* Self-test at power-on: 1 = disabled, 0 = enabled */
> > -#define ADIS16209_MSC_CTRL_PWRUP_SELF_TEST BIT(10)
> > -
> > -/* Self-test enable */
> > -#define ADIS16209_MSC_CTRL_SELF_TEST_EN ÂÂÂÂÂÂÂÂBIT(8)
> > -
> > -/* Data-ready enable: 1 = enabled, 0 = disabled */
> > -#define ADIS16209_MSC_CTRL_DATA_RDY_EN ÂÂÂÂÂÂÂÂBIT(2)
> > -
> > +#define ADIS16209_MSC_CTRL_REG 0x34
> > +#defineÂÂADIS16209_MSC_CTRL_PWRUP_SELF_TEST BIT(10)
> > +#defineÂÂADIS16209_MSC_CTRL_SELF_TEST_EN BIT(8)
> > +#defineÂÂADIS16209_MSC_CTRL_DATA_RDY_EN BIT(2)
> > Â/* Data-ready polarity: 1 = active high, 0 = active low */
> > -#define ADIS16209_MSC_CTRL_ACTIVE_HIGH ÂÂÂÂÂÂÂÂBIT(1)
> > +#defineÂÂADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1)
> > +#defineÂÂADIS16209_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
> > Â
> > -/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> > -#define ADIS16209_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
> > -
> > -/* DIAG_STAT */
> > -
> > -/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> > -#define ADIS16209_DIAG_STAT_ALARM2ÂÂÂÂÂÂÂÂBIT(9)
> > -
> > -/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> > -#define ADIS16209_DIAG_STAT_ALARM1ÂÂÂÂÂÂÂÂBIT(8)
> > -
> > -/* Self-test diagnostic error flag: 1 = error condition, 0 =
> > normal operation */
> > +#define ADIS16209_DIAG_STAT_REG 0x3C
> > +#defineÂÂADIS16209_DIAG_STAT_ALARM2 BIT(9)
> > +#defineÂÂADIS16209_DIAG_STAT_ALARM1 BIT(8)
> > Â#define ADIS16209_DIAG_STAT_SELFTEST_FAIL_BIT 5
> > -
> > -/* SPI communications failure */
> > Â#define ADIS16209_DIAG_STAT_SPI_FAIL_BIT 3
> > -
> > -/* Flash update failure */
> > Â#define ADIS16209_DIAG_STAT_FLASH_UPT_BIT 2
> > -
> > Â/* Power supply above 3.625 V */
> > Â#define ADIS16209_DIAG_STAT_POWER_HIGH_BIT 1
> > -
> > Â/* Power supply below 3.15 V */
> > Â#define ADIS16209_DIAG_STAT_POWER_LOW_BIT 0
> > Â
> > -/* GLOB_CMD */
> > -
> > -#define ADIS16209_GLOB_CMD_SW_RESET BIT(7)
> > -#define ADIS16209_GLOB_CMD_CLEAR_STAT BIT(4)
> > -#define ADIS16209_GLOB_CMD_FACTORY_CAL BIT(1)
> > +#define ADIS16209_GLOB_CMD_REG 0x3E
> > +#defineÂÂADIS16209_GLOB_CMD_SW_RESET BIT(7)
> > +#defineÂÂADIS16209_GLOB_CMD_CLEAR_STAT BIT(4)
> > +#defineÂÂADIS16209_GLOB_CMD_FACTORY_CAL BIT(1)
> > Â
> > -#define ADIS16209_ERROR_ACTIVEÂÂÂÂÂÂÂÂÂÂBIT(14)
> > +#define ADIS16209_ERROR_ACTIVE BIT(14)
> > Â
> > Âenum adis16209_scan {
> > Â ADIS16209_SCAN_SUPPLY,
> > @@ -226,24 +161,38 @@ static int adis16209_read_raw(struct iio_dev
> > *indio_dev,
> > Â *val2 = 610500; /* 0.6105 mV */
> > Â return IIO_VAL_INT_PLUS_MICRO;
> > Â case IIO_TEMP:
> > - *val = -470; /* -0.47 C */
> > + *val = -470;
> > Â *val2 = 0;
> > Â return IIO_VAL_INT_PLUS_MICRO;
> > Â case IIO_ACCEL:
> > + /*
> > + Â* IIO base unit for sensitivity of
> > accelerometer
> > + Â* is milli g.
> > + Â* 1 LSB represents 0.244 mg.
> > + Â*/
> > Â *val = 0;
> > - *val2 = IIO_G_TO_M_S_2(244140); /*
> > 0.244140 mg */
> > + *val2 = IIO_G_TO_M_S_2(244140);
> > Â return IIO_VAL_INT_PLUS_NANO;
> > Â case IIO_INCLI:
> > Â case IIO_ROT:
> > + /*
> > + Â* IIO base units for rotation are
> > degrees.
> > + Â* 1 LSB represents 0.025 milli degrees.
> > + Â*/
> > Â *val = 0;
> > - *val2 = 25000; /* 0.025 degree */
> > + *val2 = 25000;
> > Â return IIO_VAL_INT_PLUS_MICRO;
> > Â default:
> > Â return -EINVAL;
> > Â }
> > Â break;
> > Â case IIO_CHAN_INFO_OFFSET:
> > - *val = 25000 / -470 - 0x4FE; /* 25 C = 0x4FE */
> > + /*
> > + Â* The raw ADC value is 0x4FE when the temperature
> > + Â* is 45 degrees and the scale factor per milli
> > + Â* degree celcius is -470.
> Are you sure it is *45 degrees* and not *25 degrees* instead ?
>
> As I can clearly see from the datasheet :
>
> "sensitivity = â0.47Â/LSB, 25ÂC = 1278 LSB = 0x04FE"
>
> Please check it :)
>
> I found this because I'm also doing a similar cleanup for adis16201.
Hi Himanshu,
Thanks for pointing this out :)
Yes it is 25 degrees.
I'll make the change after Jonathan's reviews on the other changes
Thank you
>