Re: [PATCH 03/11] Staging: iio: accel: Remove unnecessary comments
From: Shreeya Patel
Date: Tue Mar 06 2018 - 16:27:16 EST
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 <himanshujha199640@xxxxxxxxx>
> ---
> Â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 <linux/iio/buffer.h>
> Â#include <linux/iio/imu/adis.h>
> Â
> -#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_ADCÂÂÂÂÂÂÂÂ0x08
> -
> -/* 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_SCALEÂÂÂÂ0x14
> -
> -/* y-axis acceleration scale factor */
> Â#define ADIS16201_YACCL_SCALEÂÂÂÂ0x16
> -
> -/* 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_SCALEÂÂÂÂ0x1C
> -
> -/* y-axis inclination scale factor */
> Â#define ADIS16201_YINCL_SCALEÂÂÂÂ0x1E
> Â
> -/* 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_DACÂÂÂÂÂÂÂÂ0x30
> -
> -/* 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_CNTÂÂÂÂÂÂÂÂ0x38
> -
> -/* Operation, sleep mode control */
> Â#define ADIS16201_SLP_CNTÂÂÂÂÂÂÂÂ0x3A
> Â
> Â/* 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_ALARM2ÂÂÂÂÂÂÂÂBIT(9)
> Â
> -/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> Â#define ADIS16201_DIAG_STAT_ALARM1ÂÂÂÂÂÂÂÂBIT(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 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
Thanks,
Shreeya
> Â
> @@ -187,30 +128,30 @@ static int adis16201_read_raw(struct iio_dev
> *indio_dev,
> Â case IIO_VOLTAGE:
> Â if (chan->channel == 0) {
> Â *val = 1;
> - *val2 = 220000; /* 1.22 mV */
> + *val2 = 220000;
> Â } else {
> Â *val = 0;
> - *val2 = 610000; /* 0.610 mV */
> + *val2 = 610000;
> Â }
> Â 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:
> Â *val = 0;
> - *val2 = IIO_G_TO_M_S_2(462400); /* 0.4624 mg
> */
> + *val2 = IIO_G_TO_M_S_2(462400);
> Â return IIO_VAL_INT_PLUS_NANO;
> Â case IIO_INCLI:
> Â *val = 0;
> - *val2 = 100000; /* 0.1 degree */
> + *val2 = 100000;
> Â return IIO_VAL_INT_PLUS_MICRO;
> Â default:
> Â return -EINVAL;
> Â }
> Â break;
> Â case IIO_CHAN_INFO_OFFSET:
> - *val = 25000 / -470 - 1278; /* 25 C = 1278 */
> + *val = 25000 / -470 - 1278;
> Â return IIO_VAL_INT;
> Â case IIO_CHAN_INFO_CALIBBIAS:
> Â switch (chan->type) {
> @@ -318,13 +259,11 @@ static int adis16201_probe(struct spi_device
> *spi)
> Â struct adis *st;
> Â struct iio_dev *indio_dev;
> Â
> - /* setup the industrialio driver allocated elements */
> Â indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> Â if (!indio_dev)
> Â return -ENOMEM;
> Â
> Â st = iio_priv(indio_dev);
> - /* this is only used for removal purposes */
> Â spi_set_drvdata(spi, indio_dev);
> Â
> Â indio_dev->name = spi->dev.driver->name;
> @@ -343,7 +282,6 @@ static int adis16201_probe(struct spi_device
> *spi)
> Â if (ret)
> Â return ret;
> Â
> - /* Get the device into a sane initial state */
> Â ret = adis_initial_startup(st);
> Â if (ret)
> Â goto error_cleanup_buffer_trigger;