Re: [PATCH 4/6] Staging: iio: adis16209: Remove unnecessary comments and group the definitions
On Fri, 2 Mar 2018 18:58:55 +0530 Shreeya Patel wrote: > Remove some unnecessay comments and group the control > register and register field macros together. > > Signed-off-by: Shreeya Patel Hi Shreeya, Nice patch. As you have probably already seen comment removal (and addition) is always open to debate. I think a few of the removed comments are useful and should remain or in one or two cases be expanded to become useful. In other cases a slight tweak to the name of the define makes it self explanatory. Thanks, Jonathan > --- > drivers/staging/iio/accel/adis16209.c | 116 > ++ > 1 file changed, 19 insertions(+), 97 deletions(-) > > diff --git a/drivers/staging/iio/accel/adis16209.c > b/drivers/staging/iio/accel/adis16209.c > index 151120f..d2d1254 100644 > --- a/drivers/staging/iio/accel/adis16209.c > +++ b/drivers/staging/iio/accel/adis16209.c > @@ -21,135 +21,60 @@ > #include > > #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_REG0x08 > - > -/* Output, temperature */ > #define ADIS16209_TEMP_OUT_REG 0x0A > - > -/* Output, 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_REG0x10 This one is 'odd' enough I think it is worth keeping some sort of description... > > -/* Calibration, x-axis acceleration offset null */ > +/* Calibration Register Definitions */ > #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_REG0x30 > - > -/* 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_REG0x38 > - > -/* Operation, sleep mode control */ > #define ADIS16209_SLP_CNT_REG0x3A > > -/* 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) > - > -/* Data-ready polarity: 1 = active high, 0 = active low */ > -#define ADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1) > +#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) > +#define ADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1) This one isn't named well enough to make it obvious what it is controlling. Either leave the comment or tweak the anme to something like: ADIS16209_MSC_CTRL_DATA_RDY_ACTIVE_HIGH > +#define ADIS16209_MSC_CTRL_DATA_RDY_DIO2BIT(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 */ > -#defin
[PATCH 4/6] Staging: iio: adis16209: Remove unnecessary comments and group the definitions
Remove some unnecessay comments and group the control register and register field macros together. Signed-off-by: Shreeya Patel --- drivers/staging/iio/accel/adis16209.c | 116 ++ 1 file changed, 19 insertions(+), 97 deletions(-) diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c index 151120f..d2d1254 100644 --- a/drivers/staging/iio/accel/adis16209.c +++ b/drivers/staging/iio/accel/adis16209.c @@ -21,135 +21,60 @@ #include #define ADIS16209_STARTUP_DELAY_MS 220 - -/* Flash memory write count */ #define ADIS16209_FLASH_CNT_REG0x00 -/* Output, power supply */ +/* Data Output Register Definitions */ #define ADIS16209_SUPPLY_OUT_REG 0x02 - -/* Output, x-axis accelerometer */ #define ADIS16209_XACCL_OUT_REG0x04 - -/* Output, y-axis accelerometer */ #define ADIS16209_YACCL_OUT_REG0x06 - -/* Output, auxiliary ADC input */ #define ADIS16209_AUX_ADC_REG 0x08 - -/* Output, temperature */ #define ADIS16209_TEMP_OUT_REG 0x0A - -/* Output, x-axis inclination */ #define ADIS16209_XINCL_OUT_REG0x0C - -/* Output, y-axis inclination */ #define ADIS16209_YINCL_OUT_REG0x0E - -/* Output, +/-180 vertical rotational position */ #define ADIS16209_ROT_OUT_REG 0x10 -/* Calibration, x-axis acceleration offset null */ +/* Calibration Register Definitions */ #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_REG0x24 - -/* Alarm 2, sample period */ #define ADIS16209_ALM_SMPL2_REG0x26 - -/* 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_REG0x32 - -/* 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_REG0x3C - -/* 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_ENBIT(8) - -/* Data-ready enable: 1 = enabled, 0 = disabled */ -#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_REG 0x34 +#define ADIS16209_MSC_CTRL_PWRUP_SELF_TESTBIT(10) +#define ADIS16209_MSC_CTRL_SELF_TEST_EN BIT(8) +#define ADIS16209_MSC_CTRL_DATA_RDY_ENBIT(2) +#define ADIS16209_MSC_CTRL_ACTIVE_HIGHBIT(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_ALARM2BIT(9) - -/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */ -#define ADIS16209_DIAG_STAT_ALARM1BIT(8) - -/* Self-test diagnostic error flag: 1 = error condition, 0 = normal operation */ +#define ADIS16209_DIAG_STAT_REG0x3C +#define ADIS16209_DIAG_STAT_ALARM2BIT(9) +#define ADIS16209_DIAG_STAT_ALARM1BIT(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_RESETBIT(7) -#define ADIS16209_GLOB_CMD_CLEAR_STAT BIT(4) -#define ADIS16209_GLOB_CMD_FACTORY_CAL BIT(1) +#define
