Re: [PATCH 03/11] Staging: iio: accel: Remove unnecessary comments

[Jonathan Cameron]

On Mon,  5 Mar 2018 13:19:22 +0530
Himanshu Jha  wrote:

> Remove unnecessary comments since the definitions are pretty clear
> with their macro names.
> 
> Signed-off-by: Himanshu Jha 

Hi,

The art of commenting (and indeed removing comments) is never a clear one.
I agree with the vast majority of what you have here, but as with other
drivers in this set, I think there are a couple of places comments are useful
and even in one case where the comment should be expanded to become useful.

I'd also like to see the patches reordered so the renames occur before this
one.  That will make some of the changes you have made in here a lot more
obviously good!

Thanks,

Jonathan.
> ---
>  drivers/staging/iio/accel/adis16201.c | 82 
> +--
>  1 file changed, 10 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16201.c 
> b/drivers/staging/iio/accel/adis16201.c
> index 0fae8aa..59c1166 100644
> --- a/drivers/staging/iio/accel/adis16201.c
> +++ b/drivers/staging/iio/accel/adis16201.c
> @@ -20,87 +20,42 @@
>  #include 
>  #include 
>  
> -#define ADIS16201_STARTUP_DELAY  220 /* ms */
> +#define ADIS16201_STARTUP_DELAY  220
As below, this would have made a lot more sense if you had
the rename of the macro patch (I assume the unit will be added
to the name) before this one.

>  
> -/* Flash memory write count */
>  #define ADIS16201_FLASH_CNT  0x00
>  
> -/* Output, power supply */
> +/* Data Output Register Information */
>  #define ADIS16201_SUPPLY_OUT 0x02
> -
> -/* Output, x-axis accelerometer */
>  #define ADIS16201_XACCL_OUT  0x04
> -
> -/* Output, y-axis accelerometer */
>  #define ADIS16201_YACCL_OUT  0x06
> -
> -/* Output, auxiliary ADC input */
>  #define ADIS16201_AUX_ADC0x08
> -
> -/* Output, temperature */
>  #define ADIS16201_TEMP_OUT   0x0A
> -
> -/* Output, x-axis inclination */
>  #define ADIS16201_XINCL_OUT  0x0C
> -
> -/* Output, y-axis inclination */
>  #define ADIS16201_YINCL_OUT  0x0E
>  
> -/* Calibration, x-axis acceleration offset */
> +/* Calibration Register Definition */
>  #define ADIS16201_XACCL_OFFS 0x10
> -
> -/* Calibration, y-axis acceleration offset */
>  #define ADIS16201_YACCL_OFFS 0x12
> -
> -/* x-axis acceleration scale factor */
>  #define ADIS16201_XACCL_SCALE0x14
> -
> -/* y-axis acceleration scale factor */
>  #define ADIS16201_YACCL_SCALE0x16
> -
> -/* Calibration, x-axis inclination offset */
>  #define ADIS16201_XINCL_OFFS 0x18
> -
> -/* Calibration, y-axis inclination offset */
>  #define ADIS16201_YINCL_OFFS 0x1A
> -
> -/* x-axis inclination scale factor */
>  #define ADIS16201_XINCL_SCALE0x1C
> -
> -/* y-axis inclination scale factor */
>  #define ADIS16201_YINCL_SCALE0x1E
>  
> -/* Alarm 1 amplitude threshold */
> +/* Alarm Register Definition */
>  #define ADIS16201_ALM_MAG1   0x20
> -
> -/* Alarm 2 amplitude threshold */
>  #define ADIS16201_ALM_MAG2   0x22
> -
> -/* Alarm 1, sample period */
>  #define ADIS16201_ALM_SMPL1  0x24
> -
> -/* Alarm 2, sample period */
>  #define ADIS16201_ALM_SMPL2  0x26
> -
> -/* Alarm control */
>  #define ADIS16201_ALM_CTRL   0x28
>  
> -/* Auxiliary DAC data */
>  #define ADIS16201_AUX_DAC0x30
> -
> -/* General-purpose digital input/output control */
>  #define ADIS16201_GPIO_CTRL  0x32
> -
> -/* Miscellaneous control */
>  #define ADIS16201_MSC_CTRL   0x34
>  
> -/* Internal sample period (rate) control */
>  #define ADIS16201_SMPL_PRD   0x36
> -
> -/* Operation, filter configuration */
>  #define ADIS16201_AVG_CNT0x38
> -
> -/* Operation, sleep mode control */
>  #define ADIS16201_SLP_CNT0x3A
>  
>  /* Diagnostics, system status register */
> @@ -109,42 +64,28 @@
>  /* Operation, system command register */
>  #define ADIS16201_GLOB_CMD   0x3E
>  
> -/* MSC_CTRL */
>  
> -/* Self-test enable */
>  #define ADIS16201_MSC_CTRL_SELF_TEST_EN  BIT(8)
>  
> -/* Data-ready enable: 1 = enabled, 0 = disabled */
>  #define ADIS16201_MSC_CTRL_DATA_RDY_EN   BIT(2)
>  
> -/* Data-ready polarity: 1 = active high, 0 = active low */
>  #define ADIS16201_MSC_CTRL_ACTIVE_HIGH   BIT(1)
>  
> -/* Data-ready line selection: 1 = DIO1, 0 = DIO0 */
>  #define ADIS16201_MSC_CTRL_DATA_RDY_DIO1 BIT(0)
>  
> -/* DIAG_STAT */
>  
> -/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
>  #define ADIS16201_DIAG_STAT_ALARM2BIT(9)
>  
> -/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
>  #define ADIS16201_DIAG_STAT_ALARM1BIT(8)
>  
> -/* SPI communications failure */
>  #define ADIS16201_DIAG_STAT_SPI_FAIL_BIT   3
>  
> -/* Flash update failure */
>  #define ADIS16201_DIAG_STAT_FLASH_UPT_BIT  2
I would have preferred this and the next patch to be
in the opposite order as then it would have been obvious when
reading this patch on why the 

Re: [PATCH 03/11] Staging: iio: accel: Remove unnecessary comments

[Himanshu Jha]

Hi Shreeya,

> I was just going through your patch for giving myself 
> a habit of reading patches.

Great!

> I see here that there are many comments which are necessary
> have also been removed.
> Jonathan told that some of the names do not explain
> much about the how registers are related to the orientation.
> So it is necessary for some comments to be there here.
> 
> I saw your next patch too, in which you are changing some of the
> names for betterment, but again, that doesn't cover everything which
> was told by Jonathan.
> 
> Here is the link to the patch where Jonathan had given detailed 
> description
> 
> Just sharing this information so in case if Jonathan agrees with
> this then he will not have to explain it all again :)
> 
> https://lkml.org/lkml/2018/3/3/104

Well, the naming of macros is debatable as Jonathan pointed out[1] and
the unusual/odd naming pointed to you was for rotation registers

#define ADIS16209_ROT_OUT_REG   0x10

I renamed the unusual/odd naming macros for eg
#define ADIS16209_DIAG_STAT_FLASH_UPT_BIT

to

#define ADIS16201_DIAG_STAT_FLASH_UPT_FAIL_BIT

as it didn't look like a failure bit address for which I consulted Jonathan to 
which
he agreed. Also, *DIAG_STAT* seems like a good name since these status registers
are serving the purpose of diagnosing the device behavior. Again, GLOB_CMD 
stands
for Global Command register for controlling the deivce operation such as
Fatory Reset, Software Reset, etc.

See, it is difficult to point the perfect names than the suitable ones!
And let's just leave these *bikeshedding* ;-)

-- 
Thanks
Himanshu Jha
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Re: [PATCH 03/11] Staging: iio: accel: Remove unnecessary comments

[Shreeya Patel]

On Mon, 2018-03-05 at 13:19 +0530, Himanshu Jha wrote:
> Remove unnecessary comments since the definitions are pretty clear
> with their macro names.
> 
> Signed-off-by: Himanshu Jha 
> ---
>  drivers/staging/iio/accel/adis16201.c | 82 +--
> 
>  1 file changed, 10 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16201.c
> b/drivers/staging/iio/accel/adis16201.c
> index 0fae8aa..59c1166 100644
> --- a/drivers/staging/iio/accel/adis16201.c
> +++ b/drivers/staging/iio/accel/adis16201.c
> @@ -20,87 +20,42 @@
>  #include 
>  #include 
>  
> -#define ADIS16201_STARTUP_DELAY  220 /* ms */
> +#define ADIS16201_STARTUP_DELAY  220
>  
> -/* Flash memory write count */
>  #define ADIS16201_FLASH_CNT  0x00
>  
> -/* Output, power supply */
> +/* Data Output Register Information */
>  #define ADIS16201_SUPPLY_OUT 0x02
> -
> -/* Output, x-axis accelerometer */
>  #define ADIS16201_XACCL_OUT  0x04
> -
> -/* Output, y-axis accelerometer */
>  #define ADIS16201_YACCL_OUT  0x06
> -
> -/* Output, auxiliary ADC input */
>  #define ADIS16201_AUX_ADC0x08
> -
> -/* Output, temperature */
>  #define ADIS16201_TEMP_OUT   0x0A
> -
> -/* Output, x-axis inclination */
>  #define ADIS16201_XINCL_OUT  0x0C
> -
> -/* Output, y-axis inclination */
>  #define ADIS16201_YINCL_OUT  0x0E
>  
> -/* Calibration, x-axis acceleration offset */
> +/* Calibration Register Definition */
>  #define ADIS16201_XACCL_OFFS 0x10
> -
> -/* Calibration, y-axis acceleration offset */
>  #define ADIS16201_YACCL_OFFS 0x12
> -
> -/* x-axis acceleration scale factor */
>  #define ADIS16201_XACCL_SCALE0x14
> -
> -/* y-axis acceleration scale factor */
>  #define ADIS16201_YACCL_SCALE0x16
> -
> -/* Calibration, x-axis inclination offset */
>  #define ADIS16201_XINCL_OFFS 0x18
> -
> -/* Calibration, y-axis inclination offset */
>  #define ADIS16201_YINCL_OFFS 0x1A
> -
> -/* x-axis inclination scale factor */
>  #define ADIS16201_XINCL_SCALE0x1C
> -
> -/* y-axis inclination scale factor */
>  #define ADIS16201_YINCL_SCALE0x1E
>  
> -/* Alarm 1 amplitude threshold */
> +/* Alarm Register Definition */
>  #define ADIS16201_ALM_MAG1   0x20
> -
> -/* Alarm 2 amplitude threshold */
>  #define ADIS16201_ALM_MAG2   0x22
> -
> -/* Alarm 1, sample period */
>  #define ADIS16201_ALM_SMPL1  0x24
> -
> -/* Alarm 2, sample period */
>  #define ADIS16201_ALM_SMPL2  0x26
> -
> -/* Alarm control */
>  #define ADIS16201_ALM_CTRL   0x28
>  
> -/* Auxiliary DAC data */
>  #define ADIS16201_AUX_DAC0x30
> -
> -/* General-purpose digital input/output control */
>  #define ADIS16201_GPIO_CTRL  0x32
> -
> -/* Miscellaneous control */
>  #define ADIS16201_MSC_CTRL   0x34
>  
> -/* Internal sample period (rate) control */
>  #define ADIS16201_SMPL_PRD   0x36
> -
> -/* Operation, filter configuration */
>  #define ADIS16201_AVG_CNT0x38
> -
> -/* Operation, sleep mode control */
>  #define ADIS16201_SLP_CNT0x3A
>  
>  /* Diagnostics, system status register */
> @@ -109,42 +64,28 @@
>  /* Operation, system command register */
>  #define ADIS16201_GLOB_CMD   0x3E
>  
> -/* MSC_CTRL */
>  
> -/* Self-test enable */
>  #define ADIS16201_MSC_CTRL_SELF_TEST_EN  BIT(8)
>  
> -/* Data-ready enable: 1 = enabled, 0 = disabled */
>  #define ADIS16201_MSC_CTRL_DATA_RDY_EN   BIT(2)
>  
> -/* Data-ready polarity: 1 = active high, 0 = active low */
>  #define ADIS16201_MSC_CTRL_ACTIVE_HIGH   BIT(1)
>  
> -/* Data-ready line selection: 1 = DIO1, 0 = DIO0 */
>  #define ADIS16201_MSC_CTRL_DATA_RDY_DIO1 BIT(0)
>  
> -/* DIAG_STAT */
>  
> -/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
>  #define ADIS16201_DIAG_STAT_ALARM2BIT(9)
>  
> -/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
>  #define ADIS16201_DIAG_STAT_ALARM1BIT(8)
>  
> -/* SPI communications failure */
>  #define ADIS16201_DIAG_STAT_SPI_FAIL_BIT   3
>  
> -/* Flash update failure */
>  #define ADIS16201_DIAG_STAT_FLASH_UPT_BIT  2
>  
> -/* Power supply above 3.625 V */
>  #define ADIS16201_DIAG_STAT_POWER_HIGH_BIT 1
>  
> -/* Power supply below 3.15 V */
>  #define ADIS16201_DIAG_STAT_POWER_LOW_BIT  0
>  
> -/* GLOB_CMD */
> -
>  #define ADIS16201_GLOB_CMD_SW_RESET  BIT(7)
>  #define ADIS16201_GLOB_CMD_FACTORY_CAL   BIT(1)

I was just going through your patch for giving myself 
a habit of reading patches.

I see here that there are many comments which are necessary
have also been removed.
Jonathan told that some of the names do not explain
much about the how registers are related to the orientation.
So it is necessary for some comments to be there here.

I saw your next patch too, in which you are changing some of the
names for betterment, but again, that doesn't cover everything which
was told by Jonathan.

Here is the link to the