[PATCH 0/4] staging: iio: accel: adis16201 driver cleanup

[PATCH 0/4] staging: iio: accel: adis16201 driver cleanup

[Himanshu Jha]

The following patch series cleans up miscellaneous code fragments and
then the driver is moved from staging to mainline IIO subsytem directory.
Note that the last patch to move driver is *not* generated using '-M' flag,
which is used for detecting renames, since it may help reviewers to
suggest more cleanups/fix while pointing inline to the patch the
necessary changes.

All the patches have been tested using 0-day test service with success.

For all the patches:
Suggested-by: Jonathan Cameron <jic23@kernel.org>

Himanshu Jha (4):
  staging: iio: accel: adis16201: Use SPDX identifier
  staging: iio: accel: Remove unnecessary comments and add suitable
    suffix
  staging: iio: accel: Use sign_extend32 and adjust a switch statement
  staging: iio: accel: Move adis16201 driver out of staging

 drivers/iio/accel/Kconfig             |  12 ++
 drivers/iio/accel/Makefile            |   1 +
 drivers/iio/accel/adis16201.c         | 315 ++++++++++++++++++++++++++++
 drivers/staging/iio/accel/Kconfig     |  12 --
 drivers/staging/iio/accel/Makefile    |   1 -
 drivers/staging/iio/accel/adis16201.c | 381 ----------------------------------
 6 files changed, 328 insertions(+), 394 deletions(-)
 create mode 100644 drivers/iio/accel/adis16201.c
 delete mode 100644 drivers/staging/iio/accel/adis16201.c

-- 
2.7.4

[PATCH 1/4] staging: iio: accel: adis16201: Use SPDX identifier

[Himanshu Jha]

Use SPDX identifier format instead of GPLv2. Also, rearrange the headers
in the alphabetical order.

Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
---
 drivers/staging/iio/accel/adis16201.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
index 2ebd275..b2145c9 100644
--- a/drivers/staging/iio/accel/adis16201.c
+++ b/drivers/staging/iio/accel/adis16201.c
@@ -1,19 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0+
 /*
  * ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer
  *
  * Copyright 2010 Analog Devices Inc.
- *
- * Licensed under the GPL-2 or later.
  */
 
 #include <linux/delay.h>
-#include <linux/mutex.h>
 #include <linux/device.h>
 #include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
 #include <linux/spi/spi.h>
 #include <linux/slab.h>
 #include <linux/sysfs.h>
-#include <linux/module.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
-- 
2.7.4

[PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Himanshu Jha]

Remove some unnecessary comments by giving appropriate name to macros.
Therefore, add _REG suffix for control registers. Also, align function
arguments to match open parentheses using tabs.
Group the control register and register field macros together.

Blank line before some returns improves code readability.

Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
---
 drivers/staging/iio/accel/adis16201.c | 226 ++++++++++++----------------------
 1 file changed, 79 insertions(+), 147 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
index b2145c9..011d2c5 100644
--- a/drivers/staging/iio/accel/adis16201.c
+++ b/drivers/staging/iio/accel/adis16201.c
@@ -19,135 +19,63 @@
 #include <linux/iio/buffer.h>
 #include <linux/iio/imu/adis.h>
 
-#define ADIS16201_STARTUP_DELAY	220 /* ms */
-
-/* Flash memory write count */
-#define ADIS16201_FLASH_CNT      0x00
-
-/* Output, power supply */
-#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 */
-#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 */
-#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 */
-#define ADIS16201_DIAG_STAT      0x3C
-
-/* Operation, system command register */
-#define ADIS16201_GLOB_CMD       0x3E
+#define ADIS16201_STARTUP_DELAY_MS		220
+#define ADIS16201_FLASH_CNT_REG			0x00
+
+/* Data Output Register Definitions */
+#define ADIS16201_SUPPLY_OUT_REG		0x02
+#define ADIS16201_XACCL_OUT_REG			0x04
+#define ADIS16201_YACCL_OUT_REG			0x06
+#define ADIS16201_AUX_ADC_REG			0x08
+#define ADIS16201_TEMP_OUT_REG			0x0A
+#define ADIS16201_XINCL_OUT_REG			0x0C
+#define ADIS16201_YINCL_OUT_REG			0x0E
+
+/* Calibration Register Definitions */
+#define ADIS16201_XACCL_OFFS_REG		0x10
+#define ADIS16201_YACCL_OFFS_REG		0x12
+#define ADIS16201_XACCL_SCALE_REG		0x14
+#define ADIS16201_YACCL_SCALE_REG		0x16
+#define ADIS16201_XINCL_OFFS_REG		0x18
+#define ADIS16201_YINCL_OFFS_REG		0x1A
+#define ADIS16201_XINCL_SCALE_REG		0x1C
+#define ADIS16201_YINCL_SCALE_REG		0x1E
+
+/* Alarm Register Definitions */
+#define ADIS16201_ALM_MAG1_REG			0x20
+#define ADIS16201_ALM_MAG2_REG			0x22
+#define ADIS16201_ALM_SMPL1_REG			0x24
+#define ADIS16201_ALM_SMPL2_REG			0x26
+#define ADIS16201_ALM_CTRL_REG			0x28
+
+#define ADIS16201_AUX_DAC_REG			0x30
+#define ADIS16201_GPIO_CTRL_REG			0x32
+#define ADIS16201_SMPL_PRD_REG			0x36
+#define ADIS16201_AVG_CNT_REG			0x38
+#define ADIS16201_SLP_CNT_REG			0x3A
 
 /* 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_REG			0x34
+#define ADIS16201_MSC_CTRL_SELF_TEST_EN		BIT(8)
+#define ADIS16201_MSC_CTRL_DATA_RDY_EN		BIT(2)
+#define ADIS16201_MSC_CTRL_ACTIVE_HIGH		BIT(1)
 #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
+#define ADIS16201_DIAG_STAT_REG			0x3C
+#define ADIS16201_DIAG_STAT_ALARM2		BIT(9)
+#define ADIS16201_DIAG_STAT_ALARM1		BIT(8)
+#define ADIS16201_DIAG_STAT_SPI_FAIL_BIT	3
+#define ADIS16201_DIAG_STAT_FLASH_UPT_BIT	2
+#define ADIS16201_DIAG_STAT_POWER_HIGH_BIT	1
+#define ADIS16201_DIAG_STAT_POWER_LOW_BIT	0
 
 /* GLOB_CMD */
+#define ADIS16201_GLOB_CMD_REG			0x3E
+#define ADIS16201_GLOB_CMD_SW_RESET		BIT(7)
+#define ADIS16201_GLOB_CMD_FACTORY_CAL		BIT(1)
 
-#define ADIS16201_GLOB_CMD_SW_RESET	BIT(7)
-#define ADIS16201_GLOB_CMD_FACTORY_CAL	BIT(1)
-
-#define ADIS16201_ERROR_ACTIVE          BIT(14)
+#define ADIS16201_ERROR_ACTIVE			BIT(14)
 
 enum adis16201_scan {
 	ADIS16201_SCAN_ACC_X,
@@ -160,10 +88,10 @@ enum adis16201_scan {
 };
 
 static const u8 adis16201_addresses[] = {
-	[ADIS16201_SCAN_ACC_X] = ADIS16201_XACCL_OFFS,
-	[ADIS16201_SCAN_ACC_Y] = ADIS16201_YACCL_OFFS,
-	[ADIS16201_SCAN_INCLI_X] = ADIS16201_XINCL_OFFS,
-	[ADIS16201_SCAN_INCLI_Y] = ADIS16201_YINCL_OFFS,
+	[ADIS16201_SCAN_ACC_X] = ADIS16201_XACCL_OFFS_REG,
+	[ADIS16201_SCAN_ACC_Y] = ADIS16201_YACCL_OFFS_REG,
+	[ADIS16201_SCAN_INCLI_X] = ADIS16201_XINCL_OFFS_REG,
+	[ADIS16201_SCAN_INCLI_Y] = ADIS16201_YINCL_OFFS_REG,
 };
 
 static int adis16201_read_raw(struct iio_dev *indio_dev,
@@ -180,36 +108,36 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_RAW:
 		return adis_single_conversion(indio_dev, chan,
-				ADIS16201_ERROR_ACTIVE, val);
+						ADIS16201_ERROR_ACTIVE, val);
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		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) {
@@ -226,11 +154,13 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 		ret = adis_read_reg_16(st, addr, &val16);
 		if (ret)
 			return ret;
+
 		val16 &= (1 << bits) - 1;
 		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
 		*val = val16;
 		return IIO_VAL_INT;
 	}
+
 	return -EINVAL;
 }
 
@@ -261,20 +191,21 @@ static int adis16201_write_raw(struct iio_dev *indio_dev,
 		addr = adis16201_addresses[chan->scan_index];
 		return adis_write_reg_16(st, addr, val16);
 	}
+
 	return -EINVAL;
 }
 
 static const struct iio_chan_spec adis16201_channels[] = {
-	ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT, ADIS16201_SCAN_SUPPLY, 0, 12),
-	ADIS_TEMP_CHAN(ADIS16201_TEMP_OUT, ADIS16201_SCAN_TEMP, 0, 12),
-	ADIS_ACCEL_CHAN(X, ADIS16201_XACCL_OUT, ADIS16201_SCAN_ACC_X,
+	ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT_REG, ADIS16201_SCAN_SUPPLY, 0, 12),
+	ADIS_TEMP_CHAN(ADIS16201_TEMP_OUT_REG, ADIS16201_SCAN_TEMP, 0, 12),
+	ADIS_ACCEL_CHAN(X, ADIS16201_XACCL_OUT_REG, ADIS16201_SCAN_ACC_X,
 			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
-	ADIS_ACCEL_CHAN(Y, ADIS16201_YACCL_OUT, ADIS16201_SCAN_ACC_Y,
+	ADIS_ACCEL_CHAN(Y, ADIS16201_YACCL_OUT_REG, ADIS16201_SCAN_ACC_Y,
 			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
-	ADIS_AUX_ADC_CHAN(ADIS16201_AUX_ADC, ADIS16201_SCAN_AUX_ADC, 0, 12),
-	ADIS_INCLI_CHAN(X, ADIS16201_XINCL_OUT, ADIS16201_SCAN_INCLI_X,
+	ADIS_AUX_ADC_CHAN(ADIS16201_AUX_ADC_REG, ADIS16201_SCAN_AUX_ADC, 0, 12),
+	ADIS_INCLI_CHAN(X, ADIS16201_XINCL_OUT_REG, ADIS16201_SCAN_INCLI_X,
 			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
-	ADIS_INCLI_CHAN(X, ADIS16201_YINCL_OUT, ADIS16201_SCAN_INCLI_Y,
+	ADIS_INCLI_CHAN(X, ADIS16201_YINCL_OUT_REG, ADIS16201_SCAN_INCLI_Y,
 			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
 	IIO_CHAN_SOFT_TIMESTAMP(7)
 };
@@ -294,13 +225,13 @@ static const char * const adis16201_status_error_msgs[] = {
 
 static const struct adis_data adis16201_data = {
 	.read_delay = 20,
-	.msc_ctrl_reg = ADIS16201_MSC_CTRL,
-	.glob_cmd_reg = ADIS16201_GLOB_CMD,
-	.diag_stat_reg = ADIS16201_DIAG_STAT,
+	.msc_ctrl_reg = ADIS16201_MSC_CTRL_REG,
+	.glob_cmd_reg = ADIS16201_GLOB_CMD_REG,
+	.diag_stat_reg = ADIS16201_DIAG_STAT_REG,
 
 	.self_test_mask = ADIS16201_MSC_CTRL_SELF_TEST_EN,
 	.self_test_no_autoclear = true,
-	.startup_delay = ADIS16201_STARTUP_DELAY,
+	.startup_delay = ADIS16201_STARTUP_DELAY_MS,
 
 	.status_error_msgs = adis16201_status_error_msgs,
 	.status_error_mask = BIT(ADIS16201_DIAG_STAT_SPI_FAIL_BIT) |
@@ -311,17 +242,15 @@ static const struct adis_data adis16201_data = {
 
 static int adis16201_probe(struct spi_device *spi)
 {
-	int ret;
-	struct adis *st;
 	struct iio_dev *indio_dev;
+	struct adis *st;
+	int ret;
 
-	/* 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;
@@ -335,6 +264,7 @@ static int adis16201_probe(struct spi_device *spi)
 	ret = adis_init(st, indio_dev, spi, &adis16201_data);
 	if (ret)
 		return ret;
+
 	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
 	if (ret)
 		return ret;
@@ -347,10 +277,12 @@ static int adis16201_probe(struct spi_device *spi)
 	ret = iio_device_register(indio_dev);
 	if (ret < 0)
 		goto error_cleanup_buffer_trigger;
+
 	return 0;
 
 error_cleanup_buffer_trigger:
 	adis_cleanup_buffer_and_trigger(st, indio_dev);
+
 	return ret;
 }
 
-- 
2.7.4

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Dan Carpenter]

On Mon, Feb 12, 2018 at 05:24:57PM +0530, Himanshu Jha wrote:
> Remove some unnecessary comments by giving appropriate name to macros.
> Therefore, add _REG suffix for control registers. Also, align function
> arguments to match open parentheses using tabs.
> Group the control register and register field macros together.
> 
> Blank line before some returns improves code readability.
> 
> Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>

The most obvious response is that this needs to be broken up into
multiple patches.

I think I liked almost all the comments...

> -/* Output, power supply */
> -#define ADIS16201_SUPPLY_OUT     0x02
> +#define ADIS16201_SUPPLY_OUT_REG		0x02

To me it seems useful to know that we're talking about the power supply.

Adding _REG to the name seems not terribly useful and it makes the name
longer so we're going to run into the 80 character limit.  I just
checked and this patch does add some checkpatch warnings.

But aligning the arguments is fine, deleting unused macros is fine,
adding blank lines is fine.

>  static int adis16201_probe(struct spi_device *spi)
>  {
> -	int ret;
> -	struct adis *st;
>  	struct iio_dev *indio_dev;
> +	struct adis *st;
> +	int ret;

Making this reverse Christmas tree is fine.  But these things should all
be done in separate patches.

regards,
dan carpenter

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Himanshu Jha]

Hi Dan,

On Mon, Feb 12, 2018 at 03:53:12PM +0300, Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 05:24:57PM +0530, Himanshu Jha wrote:
> > Remove some unnecessary comments by giving appropriate name to macros.
> > Therefore, add _REG suffix for control registers. Also, align function
> > arguments to match open parentheses using tabs.
> > Group the control register and register field macros together.
> > 
> > Blank line before some returns improves code readability.
> > 
> > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> The most obvious response is that this needs to be broken up into
> multiple patches.
> 
> I think I liked almost all the comments...

Please note that all the changes are suggested by Joanathan in his TODO
https://marc.info/?l=linux-iio&m=151775804702998&w=2
I think it far more commenting as compared to other drivers in the
mainline IIO drivers. I grouped all the relevant control registers
together at one place with suitable comment.
For eg:

+/* Data Output Register Definitions */

The macro names are identical to the names in the datasheet if you can
look and one could easily refer to the datasheet easily. Also, I
grouped the control registers and their fields together make it more readable.
For eg:

+#define ADIS16201_MSC_CTRL_REG                 0x34
+#define ADIS16201_MSC_CTRL_SELF_TEST_EN                BIT(8)
+#define ADIS16201_MSC_CTRL_DATA_RDY_EN         BIT(2)


> > -/* Output, power supply */
> > -#define ADIS16201_SUPPLY_OUT     0x02
> > +#define ADIS16201_SUPPLY_OUT_REG		0x02
> 
> To me it seems useful to know that we're talking about the power supply.

For that purpose I already grouped data output register definitions.

> Adding _REG to the name seems not terribly useful and it makes the name
> longer so we're going to run into the 80 character limit.  I just
> checked and this patch does add some checkpatch warnings.

_REG prefix is used to differentiate between registers addresses and the
fields in the register as pointed in the above MSC_CTRL_REG and
MSC_CTRL_SELF_TEST_EN.

Yes by doing this it triggered 2 I think 80 character warning. For eg:

	ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT_REG, ADIS16201_SCAN_SUPPLY, 0, 12),

But you know with 81 characters so I neglected it because it looked more
readable without breaking into lines IMHO.

> But aligning the arguments is fine, deleting unused macros is fine,
> adding blank lines is fine.
> 
> >  static int adis16201_probe(struct spi_device *spi)
> >  {
> > -	int ret;
> > -	struct adis *st;
> >  	struct iio_dev *indio_dev;
> > +	struct adis *st;
> > +	int ret;
> 
> Making this reverse Christmas tree is fine.  But these things should all
> be done in separate patches.

I am sometimes confused in dividing the patches. As per your advice I
should make separate patch for :

1. remove unnecessary comments.
2. add suitable suffix.
3. add tabs instead of space.
4. reverse christmas tree.

But these should be done when we have *more* instances.

For eg: 
I added a tab space in function static int adis16201_read_raw() argument
to match open parentheses in this patch. But I also added tabs while
removing and adding suitable suffix to the macros. So, should it also be
done in a separate patch ? Since there was only one instance of adding tabs
therefore I did it here without framing another patch for that purpose
while saving my time to plan 'what to include/exclude in the patch ?'

Also, given the above queries I was clueless as what to do first! Since
sometimes it happens that the patch series doesn't apply cleanly because
of incorrect ordering for framing the patches.

Thanks for taking your time to review the patch series.

-- 
Thanks
Himanshu Jha

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Dan Carpenter]

On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote:
> But these should be done when we have *more* instances.
> 
> For eg: 
> I added a tab space in function static int adis16201_read_raw() argument
> to match open parentheses in this patch. But I also added tabs while
> removing and adding suitable suffix to the macros. So, should it also be
> done in a separate patch ?

If you're changing a line of code and you fix a white space issue on
that same line, then that's fine.  If it's just in the same function,
then do it in a separate patch.  In other words, adding tabs when you're
moving around macros is fine, but adding it to the arguments is
unrelated.

This patch was honestly pretty tricky to review.

Jonathan assumes reviewers have the datasheet in front of them and I
assume that that they don't.  He's probably right...  But especially
comments like this:

		*val2 = 220000; /* 1.22 mV */

They seem really helpful to me.

regards,
dan carpenter

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Himanshu Jha]

On Mon, Feb 12, 2018 at 05:57:31PM +0300, Dan Carpenter wrote:
> On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote:
> > But these should be done when we have *more* instances.
> > 
> > For eg: 
> > I added a tab space in function static int adis16201_read_raw() argument
> > to match open parentheses in this patch. But I also added tabs while
> > removing and adding suitable suffix to the macros. So, should it also be
> > done in a separate patch ?
> 
> If you're changing a line of code and you fix a white space issue on
> that same line, then that's fine.  If it's just in the same function,
> then do it in a separate patch.  In other words, adding tabs when you're
> moving around macros is fine, but adding it to the arguments is
> unrelated.

I will try and divide my patches next time :)

> This patch was honestly pretty tricky to review.

I am sorry for that. Might be easy for IIO reviewers ;)

> Jonathan assumes reviewers have the datasheet in front of them and I
> assume that that they don't.  He's probably right...  But especially
> comments like this:
> 
> 		*val2 = 220000; /* 1.22 mV */

They are pretty obvious as you can see from the return statements
just below that which is :

		return IIO_VAL_INT_PLUS_MICRO;

These comments are obvious because we know 'val1' will be responsible
for Integer part(1.0) and 'val2' for the Micro part(220000 * 10^-6 = 0.22).
Isn't it ?

Although I am new to IIO please correct if I'm wrong.

-- 
Thanks
Himanshu Jha

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Jonathan Cameron]

On Tue, 13 Feb 2018 01:16:05 +0530
Himanshu Jha <himanshujha199640@gmail.com> wrote:

> On Mon, Feb 12, 2018 at 05:57:31PM +0300, Dan Carpenter wrote:
> > On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote:  
> > > But these should be done when we have *more* instances.
> > > 
> > > For eg: 
> > > I added a tab space in function static int adis16201_read_raw() argument
> > > to match open parentheses in this patch. But I also added tabs while
> > > removing and adding suitable suffix to the macros. So, should it also be
> > > done in a separate patch ?  
> > 
> > If you're changing a line of code and you fix a white space issue on
> > that same line, then that's fine.  If it's just in the same function,
> > then do it in a separate patch.  In other words, adding tabs when you're
> > moving around macros is fine, but adding it to the arguments is
> > unrelated.  
> 
> I will try and divide my patches next time :)
> 
> > This patch was honestly pretty tricky to review.  
> 
> I am sorry for that. Might be easy for IIO reviewers ;)
> 
> > Jonathan assumes reviewers have the datasheet in front of them and I
> > assume that that they don't.  He's probably right...  But especially
> > comments like this:
> > 
> > 		*val2 = 220000; /* 1.22 mV */  
> 
> They are pretty obvious as you can see from the return statements
> just below that which is :
> 
> 		return IIO_VAL_INT_PLUS_MICRO;
> 
> These comments are obvious because we know 'val1' will be responsible
> for Integer part(1.0) and 'val2' for the Micro part(220000 * 10^-6 = 0.22).
> Isn't it ?
> 
> Although I am new to IIO please correct if I'm wrong.
> 
The oddity here is that the base units (here mV) are inconsistent due
to some ancient attempts to align with other subsystems.

Dan is perhaps correct in that the comment might be helpful for that reason.
If so I would prefer to see it made clear what is going on.

/* Voltage base units are mV hence 1.22 mV */
Also should definitely have it's own line rather than being associated
with just the val2 line like it currently is.

Jonathan

Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix

[Jonathan Cameron]

On Mon, 12 Feb 2018 17:57:31 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Mon, Feb 12, 2018 at 08:05:22PM +0530, Himanshu Jha wrote:
> > But these should be done when we have *more* instances.
> > 
> > For eg: 
> > I added a tab space in function static int adis16201_read_raw() argument
> > to match open parentheses in this patch. But I also added tabs while
> > removing and adding suitable suffix to the macros. So, should it also be
> > done in a separate patch ?  
> 
> If you're changing a line of code and you fix a white space issue on
> that same line, then that's fine.  If it's just in the same function,
> then do it in a separate patch.  In other words, adding tabs when you're
> moving around macros is fine, but adding it to the arguments is
> unrelated.
> 
> This patch was honestly pretty tricky to review.
> 
> Jonathan assumes reviewers have the datasheet in front of them and I
> assume that that they don't.  He's probably right...  But especially
> comments like this:

Actually I don't.  I like the code to be very clear without the datasheet.

That is one of the reasons I always advocate making it very clear what
is a register and what is a field. The _REG postfix is useful to my mind
for that reason.

What I really don't like is needing comments to tell you what a register
is for when the name of the define should make it clear.  Obviously
there are sometimes places you can't do this because the meaning cannot
be explained in a short enough name but they are fairly rare.

I agree it is a trade off on whether the naming is sufficiently clear
or not and your example of the power supply one is a classic.
That register has a stupid name on the datasheet given how easy
it would have been to make it clear in the name choice that it was
measuring the power supply.

The naming things _OUT on this datasheet is particularly nasty as
it adds nothing other than confusion.  However, the question arises
on whether it makes sense to get rid of that in the driver and
make it harder to read with the datasheet.


> 
> 		*val2 = 220000; /* 1.22 mV */
> 
> They seem really helpful to me.
This isn't about the data sheet, it is about knowledge of IIO.

That one is perhaps debatable as the base units for voltage are
less than ideal (I really wish I had been a stickler for SI units
throughout from the first - this came about through trying to maintain
compatibility with hwmon which with hind sight was a bad idea).

> 
> regards,
> dan carpenter
> 

[PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement

[Himanshu Jha]

Use sign_extend32 function instead of manually coding it. Also, adjust a
switch block to explicitly match channels and return -EINVAL as default
case which improves code readability.

Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
---
 drivers/staging/iio/accel/adis16201.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
index 011d2c5..6800347 100644
--- a/drivers/staging/iio/accel/adis16201.c
+++ b/drivers/staging/iio/accel/adis16201.c
@@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
 		case IIO_VOLTAGE:
-			if (chan->channel == 0) {
+			switch (chan->channel) {
+			case 0:
 				*val = 1;
 				*val2 = 220000;
-			} else {
+				break;
+			case 1:
 				*val = 0;
 				*val2 = 610000;
+				break;
+			default:
+				return -EINVAL;
 			}
 			return IIO_VAL_INT_PLUS_MICRO;
 		case IIO_TEMP:
@@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
 		if (ret)
 			return ret;
 
-		val16 &= (1 << bits) - 1;
-		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
-		*val = val16;
+		*val = sign_extend32(val16, bits - 1);
 		return IIO_VAL_INT;
 	}
 
-- 
2.7.4

Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement

[Dan Carpenter]

On Mon, Feb 12, 2018 at 05:24:58PM +0530, Himanshu Jha wrote:
> Use sign_extend32 function instead of manually coding it. Also, adjust a
                                                            ^^^^^
> switch block to explicitly match channels and return -EINVAL as default
> case which improves code readability.

Greg likes to say something along the lines of "when you start your
sentence with "Also, " that could be a clue that it should be a separate
patch.".

> 
> Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
> ---
>  drivers/staging/iio/accel/adis16201.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
> index 011d2c5..6800347 100644
> --- a/drivers/staging/iio/accel/adis16201.c
> +++ b/drivers/staging/iio/accel/adis16201.c
> @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
>  		case IIO_VOLTAGE:
> -			if (chan->channel == 0) {
> +			switch (chan->channel) {
> +			case 0:
>  				*val = 1;
>  				*val2 = 220000;
> -			} else {
> +				break;
> +			case 1:
>  				*val = 0;
>  				*val2 = 610000;
> +				break;
> +			default:
> +				return -EINVAL;
>  			}

I don't think this improves readability.  The -EINVAL is to handle a
driver bug which we haven't introduced yet.  Probably we would be better
off printing a warning or something?  But it feels weird to introduce so
much code to handle a bug which would actually be pretty difficult to
write.  The original code is fine.

>  			return IIO_VAL_INT_PLUS_MICRO;
>  		case IIO_TEMP:
> @@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
>  		if (ret)
>  			return ret;
>  
> -		val16 &= (1 << bits) - 1;
> -		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
> -		*val = val16;
> +		*val = sign_extend32(val16, bits - 1);

Yeah.  This is a nice clean up.

regards,
dan carpenter

Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement

[Jonathan Cameron]

On Mon, 12 Feb 2018 16:10:01 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Mon, Feb 12, 2018 at 05:24:58PM +0530, Himanshu Jha wrote:
> > Use sign_extend32 function instead of manually coding it. Also, adjust a  
>                                                             ^^^^^
> > switch block to explicitly match channels and return -EINVAL as default
> > case which improves code readability.  
> 
> Greg likes to say something along the lines of "when you start your
> sentence with "Also, " that could be a clue that it should be a separate
> patch.".
> 
> > 
> > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
> > ---
> >  drivers/staging/iio/accel/adis16201.c | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
> > index 011d2c5..6800347 100644
> > --- a/drivers/staging/iio/accel/adis16201.c
> > +++ b/drivers/staging/iio/accel/adis16201.c
> > @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> >  	case IIO_CHAN_INFO_SCALE:
> >  		switch (chan->type) {
> >  		case IIO_VOLTAGE:
> > -			if (chan->channel == 0) {
> > +			switch (chan->channel) {
> > +			case 0:
> >  				*val = 1;
> >  				*val2 = 220000;
> > -			} else {
> > +				break;
> > +			case 1:
> >  				*val = 0;
> >  				*val2 = 610000;
> > +				break;
> > +			default:
> > +				return -EINVAL;
> >  			}  
> 
> I don't think this improves readability.  The -EINVAL is to handle a
> driver bug which we haven't introduced yet.  Probably we would be better
> off printing a warning or something?  But it feels weird to introduce so
> much code to handle a bug which would actually be pretty difficult to
> write.  The original code is fine.

Hmm. My thought here was that it is not obvious from the code
that we only have channel 0 and channel 1.  The if statement
kind of implies that channel 0 is special compared to 'all the other'
elements where as what we are actually doing is picking from
a set of options. So semantically it's a switch statement being
implemented as an if else pair ;)

Perhaps we can compromise on the addition of a comment on the else
case to say it only applies to channel 1?

Dan, what do you think?

It isn't particularly important either way though so feel free to
just drop this one.

> 
> >  			return IIO_VAL_INT_PLUS_MICRO;
> >  		case IIO_TEMP:
> > @@ -155,9 +160,7 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> >  		if (ret)
> >  			return ret;
> >  
> > -		val16 &= (1 << bits) - 1;
> > -		val16 = (s16)(val16 << (16 - bits)) >> (16 - bits);
> > -		*val = val16;
> > +		*val = sign_extend32(val16, bits - 1);  
> 
> Yeah.  This is a nice clean up.
> 
> regards,
> dan carpenter
> 

Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement

[Dan Carpenter]

On Sat, Feb 17, 2018 at 12:23:53PM +0000, Jonathan Cameron wrote:
> On Mon, 12 Feb 2018 16:10:01 +0300
> Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> > On Mon, Feb 12, 2018 at 05:24:58PM +0530, Himanshu Jha wrote:
> > > Use sign_extend32 function instead of manually coding it. Also, adjust a  
> >                                                             ^^^^^
> > > switch block to explicitly match channels and return -EINVAL as default
> > > case which improves code readability.  
> > 
> > Greg likes to say something along the lines of "when you start your
> > sentence with "Also, " that could be a clue that it should be a separate
> > patch.".
> > 
> > > 
> > > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
> > > ---
> > >  drivers/staging/iio/accel/adis16201.c | 13 ++++++++-----
> > >  1 file changed, 8 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
> > > index 011d2c5..6800347 100644
> > > --- a/drivers/staging/iio/accel/adis16201.c
> > > +++ b/drivers/staging/iio/accel/adis16201.c
> > > @@ -112,12 +112,17 @@ static int adis16201_read_raw(struct iio_dev *indio_dev,
> > >  	case IIO_CHAN_INFO_SCALE:
> > >  		switch (chan->type) {
> > >  		case IIO_VOLTAGE:
> > > -			if (chan->channel == 0) {
> > > +			switch (chan->channel) {
> > > +			case 0:
> > >  				*val = 1;
> > >  				*val2 = 220000;
> > > -			} else {
> > > +				break;
> > > +			case 1:
> > >  				*val = 0;
> > >  				*val2 = 610000;
> > > +				break;
> > > +			default:
> > > +				return -EINVAL;
> > >  			}  
> > 
> > I don't think this improves readability.  The -EINVAL is to handle a
> > driver bug which we haven't introduced yet.  Probably we would be better
> > off printing a warning or something?  But it feels weird to introduce so
> > much code to handle a bug which would actually be pretty difficult to
> > write.  The original code is fine.
> 
> Hmm. My thought here was that it is not obvious from the code
> that we only have channel 0 and channel 1.  The if statement
> kind of implies that channel 0 is special compared to 'all the other'
> elements where as what we are actually doing is picking from
> a set of options. So semantically it's a switch statement being
> implemented as an if else pair ;)
> 
> Perhaps we can compromise on the addition of a comment on the else
> case to say it only applies to channel 1?
> 
> Dan, what do you think?
> 
> It isn't particularly important either way though so feel free to
> just drop this one.
> 

To be honest, I dont care either way...  The original and the new code
are equivalently clean to me so I have a "leave the code as-is bias" but
if someone else is invested in this code then I like to let the person
who cares the most be the one to decide.

This patch is actually fine but the patch description makes it sound
like it's doing two things.  If the subject was
"cleanup adis16201_read_raw()" then that would sound like one thing.
I obviously review thousands of staging patches so some of my responses
are pretty mechanical at this point.  If it's a two random things from
the same file then split it into to two patches, but if it's from the
same function that's acceptable.

regards,
dan carpenter

[PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Himanshu Jha]

Move the adis16201 driver out of staging directory and merge to the
mainline IIO directory.

Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
---
 drivers/iio/accel/Kconfig             |  12 ++
 drivers/iio/accel/Makefile            |   1 +
 drivers/iio/accel/adis16201.c         | 315 ++++++++++++++++++++++++++++++++++
 drivers/staging/iio/accel/Kconfig     |  12 --
 drivers/staging/iio/accel/Makefile    |   1 -
 drivers/staging/iio/accel/adis16201.c | 315 ----------------------------------
 6 files changed, 328 insertions(+), 328 deletions(-)
 create mode 100644 drivers/iio/accel/adis16201.c
 delete mode 100644 drivers/staging/iio/accel/adis16201.c

diff --git a/drivers/iio/accel/Kconfig b/drivers/iio/accel/Kconfig
index c6d9517..9416c6f 100644
--- a/drivers/iio/accel/Kconfig
+++ b/drivers/iio/accel/Kconfig
@@ -5,6 +5,18 @@
 
 menu "Accelerometers"
 
+config ADIS16201
+        tristate "Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer"
+        depends on SPI
+        select IIO_ADIS_LIB
+        select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
+        help
+          Say Y here to build support for Analog Devices adis16201 dual-axis
+          digital inclinometer and accelerometer.
+
+          To compile this driver as a module, say M here: the module will
+          be called adis16201.
+
 config ADXL345
 	tristate
 
diff --git a/drivers/iio/accel/Makefile b/drivers/iio/accel/Makefile
index 368aedb..7832ec9 100644
--- a/drivers/iio/accel/Makefile
+++ b/drivers/iio/accel/Makefile
@@ -4,6 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_ADIS16201) += adis16201.o
 obj-$(CONFIG_ADXL345) += adxl345_core.o
 obj-$(CONFIG_ADXL345_I2C) += adxl345_i2c.o
 obj-$(CONFIG_ADXL345_SPI) += adxl345_spi.o
diff --git a/drivers/iio/accel/adis16201.c b/drivers/iio/accel/adis16201.c
new file mode 100644
index 0000000..6800347
--- /dev/null
+++ b/drivers/iio/accel/adis16201.c
@@ -0,0 +1,315 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer
+ *
+ * Copyright 2010 Analog Devices Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/spi/spi.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/imu/adis.h>
+
+#define ADIS16201_STARTUP_DELAY_MS		220
+#define ADIS16201_FLASH_CNT_REG			0x00
+
+/* Data Output Register Definitions */
+#define ADIS16201_SUPPLY_OUT_REG		0x02
+#define ADIS16201_XACCL_OUT_REG			0x04
+#define ADIS16201_YACCL_OUT_REG			0x06
+#define ADIS16201_AUX_ADC_REG			0x08
+#define ADIS16201_TEMP_OUT_REG			0x0A
+#define ADIS16201_XINCL_OUT_REG			0x0C
+#define ADIS16201_YINCL_OUT_REG			0x0E
+
+/* Calibration Register Definitions */
+#define ADIS16201_XACCL_OFFS_REG		0x10
+#define ADIS16201_YACCL_OFFS_REG		0x12
+#define ADIS16201_XACCL_SCALE_REG		0x14
+#define ADIS16201_YACCL_SCALE_REG		0x16
+#define ADIS16201_XINCL_OFFS_REG		0x18
+#define ADIS16201_YINCL_OFFS_REG		0x1A
+#define ADIS16201_XINCL_SCALE_REG		0x1C
+#define ADIS16201_YINCL_SCALE_REG		0x1E
+
+/* Alarm Register Definitions */
+#define ADIS16201_ALM_MAG1_REG			0x20
+#define ADIS16201_ALM_MAG2_REG			0x22
+#define ADIS16201_ALM_SMPL1_REG			0x24
+#define ADIS16201_ALM_SMPL2_REG			0x26
+#define ADIS16201_ALM_CTRL_REG			0x28
+
+#define ADIS16201_AUX_DAC_REG			0x30
+#define ADIS16201_GPIO_CTRL_REG			0x32
+#define ADIS16201_SMPL_PRD_REG			0x36
+#define ADIS16201_AVG_CNT_REG			0x38
+#define ADIS16201_SLP_CNT_REG			0x3A
+
+/* MSC_CTRL */
+#define ADIS16201_MSC_CTRL_REG			0x34
+#define ADIS16201_MSC_CTRL_SELF_TEST_EN		BIT(8)
+#define ADIS16201_MSC_CTRL_DATA_RDY_EN		BIT(2)
+#define ADIS16201_MSC_CTRL_ACTIVE_HIGH		BIT(1)
+#define ADIS16201_MSC_CTRL_DATA_RDY_DIO1	BIT(0)
+
+/* DIAG_STAT */
+#define ADIS16201_DIAG_STAT_REG			0x3C
+#define ADIS16201_DIAG_STAT_ALARM2		BIT(9)
+#define ADIS16201_DIAG_STAT_ALARM1		BIT(8)
+#define ADIS16201_DIAG_STAT_SPI_FAIL_BIT	3
+#define ADIS16201_DIAG_STAT_FLASH_UPT_BIT	2
+#define ADIS16201_DIAG_STAT_POWER_HIGH_BIT	1
+#define ADIS16201_DIAG_STAT_POWER_LOW_BIT	0
+
+/* GLOB_CMD */
+#define ADIS16201_GLOB_CMD_REG			0x3E
+#define ADIS16201_GLOB_CMD_SW_RESET		BIT(7)
+#define ADIS16201_GLOB_CMD_FACTORY_CAL		BIT(1)
+
+#define ADIS16201_ERROR_ACTIVE			BIT(14)
+
+enum adis16201_scan {
+	ADIS16201_SCAN_ACC_X,
+	ADIS16201_SCAN_ACC_Y,
+	ADIS16201_SCAN_INCLI_X,
+	ADIS16201_SCAN_INCLI_Y,
+	ADIS16201_SCAN_SUPPLY,
+	ADIS16201_SCAN_AUX_ADC,
+	ADIS16201_SCAN_TEMP,
+};
+
+static const u8 adis16201_addresses[] = {
+	[ADIS16201_SCAN_ACC_X] = ADIS16201_XACCL_OFFS_REG,
+	[ADIS16201_SCAN_ACC_Y] = ADIS16201_YACCL_OFFS_REG,
+	[ADIS16201_SCAN_INCLI_X] = ADIS16201_XINCL_OFFS_REG,
+	[ADIS16201_SCAN_INCLI_Y] = ADIS16201_YINCL_OFFS_REG,
+};
+
+static int adis16201_read_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int *val, int *val2,
+			      long mask)
+{
+	struct adis *st = iio_priv(indio_dev);
+	int ret;
+	int bits;
+	u8 addr;
+	s16 val16;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return adis_single_conversion(indio_dev, chan,
+						ADIS16201_ERROR_ACTIVE, val);
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_VOLTAGE:
+			switch (chan->channel) {
+			case 0:
+				*val = 1;
+				*val2 = 220000;
+				break;
+			case 1:
+				*val = 0;
+				*val2 = 610000;
+				break;
+			default:
+				return -EINVAL;
+			}
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_TEMP:
+			*val = -470;
+			*val2 = 0;
+			return IIO_VAL_INT_PLUS_MICRO;
+		case IIO_ACCEL:
+			*val = 0;
+			*val2 = IIO_G_TO_M_S_2(462400);
+			return IIO_VAL_INT_PLUS_NANO;
+		case IIO_INCLI:
+			*val = 0;
+			*val2 = 100000;
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+		break;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = 25000 / -470 - 1278;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_CALIBBIAS:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			bits = 12;
+			break;
+		case IIO_INCLI:
+			bits = 9;
+			break;
+		default:
+			return -EINVAL;
+		}
+		addr = adis16201_addresses[chan->scan_index];
+		ret = adis_read_reg_16(st, addr, &val16);
+		if (ret)
+			return ret;
+
+		*val = sign_extend32(val16, bits - 1);
+		return IIO_VAL_INT;
+	}
+
+	return -EINVAL;
+}
+
+static int adis16201_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val,
+			       int val2,
+			       long mask)
+{
+	struct adis *st = iio_priv(indio_dev);
+	int bits;
+	s16 val16;
+	u8 addr;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_CALIBBIAS:
+		switch (chan->type) {
+		case IIO_ACCEL:
+			bits = 12;
+			break;
+		case IIO_INCLI:
+			bits = 9;
+			break;
+		default:
+			return -EINVAL;
+		}
+		val16 = val & ((1 << bits) - 1);
+		addr = adis16201_addresses[chan->scan_index];
+		return adis_write_reg_16(st, addr, val16);
+	}
+
+	return -EINVAL;
+}
+
+static const struct iio_chan_spec adis16201_channels[] = {
+	ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT_REG, ADIS16201_SCAN_SUPPLY, 0, 12),
+	ADIS_TEMP_CHAN(ADIS16201_TEMP_OUT_REG, ADIS16201_SCAN_TEMP, 0, 12),
+	ADIS_ACCEL_CHAN(X, ADIS16201_XACCL_OUT_REG, ADIS16201_SCAN_ACC_X,
+			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
+	ADIS_ACCEL_CHAN(Y, ADIS16201_YACCL_OUT_REG, ADIS16201_SCAN_ACC_Y,
+			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
+	ADIS_AUX_ADC_CHAN(ADIS16201_AUX_ADC_REG, ADIS16201_SCAN_AUX_ADC, 0, 12),
+	ADIS_INCLI_CHAN(X, ADIS16201_XINCL_OUT_REG, ADIS16201_SCAN_INCLI_X,
+			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
+	ADIS_INCLI_CHAN(X, ADIS16201_YINCL_OUT_REG, ADIS16201_SCAN_INCLI_Y,
+			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
+	IIO_CHAN_SOFT_TIMESTAMP(7)
+};
+
+static const struct iio_info adis16201_info = {
+	.read_raw = adis16201_read_raw,
+	.write_raw = adis16201_write_raw,
+	.update_scan_mode = adis_update_scan_mode,
+};
+
+static const char * const adis16201_status_error_msgs[] = {
+	[ADIS16201_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
+	[ADIS16201_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
+	[ADIS16201_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
+	[ADIS16201_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 3.15V",
+};
+
+static const struct adis_data adis16201_data = {
+	.read_delay = 20,
+	.msc_ctrl_reg = ADIS16201_MSC_CTRL_REG,
+	.glob_cmd_reg = ADIS16201_GLOB_CMD_REG,
+	.diag_stat_reg = ADIS16201_DIAG_STAT_REG,
+
+	.self_test_mask = ADIS16201_MSC_CTRL_SELF_TEST_EN,
+	.self_test_no_autoclear = true,
+	.startup_delay = ADIS16201_STARTUP_DELAY_MS,
+
+	.status_error_msgs = adis16201_status_error_msgs,
+	.status_error_mask = BIT(ADIS16201_DIAG_STAT_SPI_FAIL_BIT) |
+		BIT(ADIS16201_DIAG_STAT_FLASH_UPT_BIT) |
+		BIT(ADIS16201_DIAG_STAT_POWER_HIGH_BIT) |
+		BIT(ADIS16201_DIAG_STAT_POWER_LOW_BIT),
+};
+
+static int adis16201_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct adis *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->name = spi->dev.driver->name;
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &adis16201_info;
+
+	indio_dev->channels = adis16201_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = adis_init(st, indio_dev, spi, &adis16201_data);
+	if (ret)
+		return ret;
+
+	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
+	if (ret)
+		return ret;
+
+	/* Get the device into a sane initial state */
+	ret = adis_initial_startup(st);
+	if (ret)
+		goto error_cleanup_buffer_trigger;
+
+	ret = iio_device_register(indio_dev);
+	if (ret < 0)
+		goto error_cleanup_buffer_trigger;
+
+	return 0;
+
+error_cleanup_buffer_trigger:
+	adis_cleanup_buffer_and_trigger(st, indio_dev);
+
+	return ret;
+}
+
+static int adis16201_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct adis *st = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	adis_cleanup_buffer_and_trigger(st, indio_dev);
+
+	return 0;
+}
+
+static struct spi_driver adis16201_driver = {
+	.driver = {
+		.name = "adis16201",
+	},
+	.probe = adis16201_probe,
+	.remove = adis16201_remove,
+};
+module_spi_driver(adis16201_driver);
+
+MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("spi:adis16201");
diff --git a/drivers/staging/iio/accel/Kconfig b/drivers/staging/iio/accel/Kconfig
index c6b0f5e..2f61e21 100644
--- a/drivers/staging/iio/accel/Kconfig
+++ b/drivers/staging/iio/accel/Kconfig
@@ -3,18 +3,6 @@
 #
 menu "Accelerometers"
 
-config ADIS16201
-	tristate "Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer"
-	depends on SPI
-	select IIO_ADIS_LIB
-	select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
-	help
-	  Say Y here to build support for Analog Devices adis16201 dual-axis
-	  digital inclinometer and accelerometer.
-
-	  To compile this driver as a module, say M here: the module will
-	  be called adis16201.
-
 config ADIS16203
 	tristate "Analog Devices ADIS16203 Programmable 360 Degrees Inclinometer"
 	depends on SPI
diff --git a/drivers/staging/iio/accel/Makefile b/drivers/staging/iio/accel/Makefile
index 5d8ad21..7dd55410 100644
--- a/drivers/staging/iio/accel/Makefile
+++ b/drivers/staging/iio/accel/Makefile
@@ -2,7 +2,6 @@
 # Makefile for industrial I/O accelerometer drivers
 #
 
-obj-$(CONFIG_ADIS16201) += adis16201.o
 obj-$(CONFIG_ADIS16203) += adis16203.o
 obj-$(CONFIG_ADIS16209) += adis16209.o
 obj-$(CONFIG_ADIS16240) += adis16240.o
diff --git a/drivers/staging/iio/accel/adis16201.c b/drivers/staging/iio/accel/adis16201.c
deleted file mode 100644
index 6800347..0000000
--- a/drivers/staging/iio/accel/adis16201.c
+++ /dev/null
@@ -1,315 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0+
-/*
- * ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer
- *
- * Copyright 2010 Analog Devices Inc.
- */
-
-#include <linux/delay.h>
-#include <linux/device.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/mutex.h>
-#include <linux/spi/spi.h>
-#include <linux/slab.h>
-#include <linux/sysfs.h>
-
-#include <linux/iio/iio.h>
-#include <linux/iio/sysfs.h>
-#include <linux/iio/buffer.h>
-#include <linux/iio/imu/adis.h>
-
-#define ADIS16201_STARTUP_DELAY_MS		220
-#define ADIS16201_FLASH_CNT_REG			0x00
-
-/* Data Output Register Definitions */
-#define ADIS16201_SUPPLY_OUT_REG		0x02
-#define ADIS16201_XACCL_OUT_REG			0x04
-#define ADIS16201_YACCL_OUT_REG			0x06
-#define ADIS16201_AUX_ADC_REG			0x08
-#define ADIS16201_TEMP_OUT_REG			0x0A
-#define ADIS16201_XINCL_OUT_REG			0x0C
-#define ADIS16201_YINCL_OUT_REG			0x0E
-
-/* Calibration Register Definitions */
-#define ADIS16201_XACCL_OFFS_REG		0x10
-#define ADIS16201_YACCL_OFFS_REG		0x12
-#define ADIS16201_XACCL_SCALE_REG		0x14
-#define ADIS16201_YACCL_SCALE_REG		0x16
-#define ADIS16201_XINCL_OFFS_REG		0x18
-#define ADIS16201_YINCL_OFFS_REG		0x1A
-#define ADIS16201_XINCL_SCALE_REG		0x1C
-#define ADIS16201_YINCL_SCALE_REG		0x1E
-
-/* Alarm Register Definitions */
-#define ADIS16201_ALM_MAG1_REG			0x20
-#define ADIS16201_ALM_MAG2_REG			0x22
-#define ADIS16201_ALM_SMPL1_REG			0x24
-#define ADIS16201_ALM_SMPL2_REG			0x26
-#define ADIS16201_ALM_CTRL_REG			0x28
-
-#define ADIS16201_AUX_DAC_REG			0x30
-#define ADIS16201_GPIO_CTRL_REG			0x32
-#define ADIS16201_SMPL_PRD_REG			0x36
-#define ADIS16201_AVG_CNT_REG			0x38
-#define ADIS16201_SLP_CNT_REG			0x3A
-
-/* MSC_CTRL */
-#define ADIS16201_MSC_CTRL_REG			0x34
-#define ADIS16201_MSC_CTRL_SELF_TEST_EN		BIT(8)
-#define ADIS16201_MSC_CTRL_DATA_RDY_EN		BIT(2)
-#define ADIS16201_MSC_CTRL_ACTIVE_HIGH		BIT(1)
-#define ADIS16201_MSC_CTRL_DATA_RDY_DIO1	BIT(0)
-
-/* DIAG_STAT */
-#define ADIS16201_DIAG_STAT_REG			0x3C
-#define ADIS16201_DIAG_STAT_ALARM2		BIT(9)
-#define ADIS16201_DIAG_STAT_ALARM1		BIT(8)
-#define ADIS16201_DIAG_STAT_SPI_FAIL_BIT	3
-#define ADIS16201_DIAG_STAT_FLASH_UPT_BIT	2
-#define ADIS16201_DIAG_STAT_POWER_HIGH_BIT	1
-#define ADIS16201_DIAG_STAT_POWER_LOW_BIT	0
-
-/* GLOB_CMD */
-#define ADIS16201_GLOB_CMD_REG			0x3E
-#define ADIS16201_GLOB_CMD_SW_RESET		BIT(7)
-#define ADIS16201_GLOB_CMD_FACTORY_CAL		BIT(1)
-
-#define ADIS16201_ERROR_ACTIVE			BIT(14)
-
-enum adis16201_scan {
-	ADIS16201_SCAN_ACC_X,
-	ADIS16201_SCAN_ACC_Y,
-	ADIS16201_SCAN_INCLI_X,
-	ADIS16201_SCAN_INCLI_Y,
-	ADIS16201_SCAN_SUPPLY,
-	ADIS16201_SCAN_AUX_ADC,
-	ADIS16201_SCAN_TEMP,
-};
-
-static const u8 adis16201_addresses[] = {
-	[ADIS16201_SCAN_ACC_X] = ADIS16201_XACCL_OFFS_REG,
-	[ADIS16201_SCAN_ACC_Y] = ADIS16201_YACCL_OFFS_REG,
-	[ADIS16201_SCAN_INCLI_X] = ADIS16201_XINCL_OFFS_REG,
-	[ADIS16201_SCAN_INCLI_Y] = ADIS16201_YINCL_OFFS_REG,
-};
-
-static int adis16201_read_raw(struct iio_dev *indio_dev,
-			      struct iio_chan_spec const *chan,
-			      int *val, int *val2,
-			      long mask)
-{
-	struct adis *st = iio_priv(indio_dev);
-	int ret;
-	int bits;
-	u8 addr;
-	s16 val16;
-
-	switch (mask) {
-	case IIO_CHAN_INFO_RAW:
-		return adis_single_conversion(indio_dev, chan,
-						ADIS16201_ERROR_ACTIVE, val);
-	case IIO_CHAN_INFO_SCALE:
-		switch (chan->type) {
-		case IIO_VOLTAGE:
-			switch (chan->channel) {
-			case 0:
-				*val = 1;
-				*val2 = 220000;
-				break;
-			case 1:
-				*val = 0;
-				*val2 = 610000;
-				break;
-			default:
-				return -EINVAL;
-			}
-			return IIO_VAL_INT_PLUS_MICRO;
-		case IIO_TEMP:
-			*val = -470;
-			*val2 = 0;
-			return IIO_VAL_INT_PLUS_MICRO;
-		case IIO_ACCEL:
-			*val = 0;
-			*val2 = IIO_G_TO_M_S_2(462400);
-			return IIO_VAL_INT_PLUS_NANO;
-		case IIO_INCLI:
-			*val = 0;
-			*val2 = 100000;
-			return IIO_VAL_INT_PLUS_MICRO;
-		default:
-			return -EINVAL;
-		}
-		break;
-	case IIO_CHAN_INFO_OFFSET:
-		*val = 25000 / -470 - 1278;
-		return IIO_VAL_INT;
-	case IIO_CHAN_INFO_CALIBBIAS:
-		switch (chan->type) {
-		case IIO_ACCEL:
-			bits = 12;
-			break;
-		case IIO_INCLI:
-			bits = 9;
-			break;
-		default:
-			return -EINVAL;
-		}
-		addr = adis16201_addresses[chan->scan_index];
-		ret = adis_read_reg_16(st, addr, &val16);
-		if (ret)
-			return ret;
-
-		*val = sign_extend32(val16, bits - 1);
-		return IIO_VAL_INT;
-	}
-
-	return -EINVAL;
-}
-
-static int adis16201_write_raw(struct iio_dev *indio_dev,
-			       struct iio_chan_spec const *chan,
-			       int val,
-			       int val2,
-			       long mask)
-{
-	struct adis *st = iio_priv(indio_dev);
-	int bits;
-	s16 val16;
-	u8 addr;
-
-	switch (mask) {
-	case IIO_CHAN_INFO_CALIBBIAS:
-		switch (chan->type) {
-		case IIO_ACCEL:
-			bits = 12;
-			break;
-		case IIO_INCLI:
-			bits = 9;
-			break;
-		default:
-			return -EINVAL;
-		}
-		val16 = val & ((1 << bits) - 1);
-		addr = adis16201_addresses[chan->scan_index];
-		return adis_write_reg_16(st, addr, val16);
-	}
-
-	return -EINVAL;
-}
-
-static const struct iio_chan_spec adis16201_channels[] = {
-	ADIS_SUPPLY_CHAN(ADIS16201_SUPPLY_OUT_REG, ADIS16201_SCAN_SUPPLY, 0, 12),
-	ADIS_TEMP_CHAN(ADIS16201_TEMP_OUT_REG, ADIS16201_SCAN_TEMP, 0, 12),
-	ADIS_ACCEL_CHAN(X, ADIS16201_XACCL_OUT_REG, ADIS16201_SCAN_ACC_X,
-			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
-	ADIS_ACCEL_CHAN(Y, ADIS16201_YACCL_OUT_REG, ADIS16201_SCAN_ACC_Y,
-			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
-	ADIS_AUX_ADC_CHAN(ADIS16201_AUX_ADC_REG, ADIS16201_SCAN_AUX_ADC, 0, 12),
-	ADIS_INCLI_CHAN(X, ADIS16201_XINCL_OUT_REG, ADIS16201_SCAN_INCLI_X,
-			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
-	ADIS_INCLI_CHAN(X, ADIS16201_YINCL_OUT_REG, ADIS16201_SCAN_INCLI_Y,
-			BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
-	IIO_CHAN_SOFT_TIMESTAMP(7)
-};
-
-static const struct iio_info adis16201_info = {
-	.read_raw = adis16201_read_raw,
-	.write_raw = adis16201_write_raw,
-	.update_scan_mode = adis_update_scan_mode,
-};
-
-static const char * const adis16201_status_error_msgs[] = {
-	[ADIS16201_DIAG_STAT_SPI_FAIL_BIT] = "SPI failure",
-	[ADIS16201_DIAG_STAT_FLASH_UPT_BIT] = "Flash update failed",
-	[ADIS16201_DIAG_STAT_POWER_HIGH_BIT] = "Power supply above 3.625V",
-	[ADIS16201_DIAG_STAT_POWER_LOW_BIT] = "Power supply below 3.15V",
-};
-
-static const struct adis_data adis16201_data = {
-	.read_delay = 20,
-	.msc_ctrl_reg = ADIS16201_MSC_CTRL_REG,
-	.glob_cmd_reg = ADIS16201_GLOB_CMD_REG,
-	.diag_stat_reg = ADIS16201_DIAG_STAT_REG,
-
-	.self_test_mask = ADIS16201_MSC_CTRL_SELF_TEST_EN,
-	.self_test_no_autoclear = true,
-	.startup_delay = ADIS16201_STARTUP_DELAY_MS,
-
-	.status_error_msgs = adis16201_status_error_msgs,
-	.status_error_mask = BIT(ADIS16201_DIAG_STAT_SPI_FAIL_BIT) |
-		BIT(ADIS16201_DIAG_STAT_FLASH_UPT_BIT) |
-		BIT(ADIS16201_DIAG_STAT_POWER_HIGH_BIT) |
-		BIT(ADIS16201_DIAG_STAT_POWER_LOW_BIT),
-};
-
-static int adis16201_probe(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev;
-	struct adis *st;
-	int ret;
-
-	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
-	if (!indio_dev)
-		return -ENOMEM;
-
-	st = iio_priv(indio_dev);
-	spi_set_drvdata(spi, indio_dev);
-
-	indio_dev->name = spi->dev.driver->name;
-	indio_dev->dev.parent = &spi->dev;
-	indio_dev->info = &adis16201_info;
-
-	indio_dev->channels = adis16201_channels;
-	indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
-	indio_dev->modes = INDIO_DIRECT_MODE;
-
-	ret = adis_init(st, indio_dev, spi, &adis16201_data);
-	if (ret)
-		return ret;
-
-	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
-	if (ret)
-		return ret;
-
-	/* Get the device into a sane initial state */
-	ret = adis_initial_startup(st);
-	if (ret)
-		goto error_cleanup_buffer_trigger;
-
-	ret = iio_device_register(indio_dev);
-	if (ret < 0)
-		goto error_cleanup_buffer_trigger;
-
-	return 0;
-
-error_cleanup_buffer_trigger:
-	adis_cleanup_buffer_and_trigger(st, indio_dev);
-
-	return ret;
-}
-
-static int adis16201_remove(struct spi_device *spi)
-{
-	struct iio_dev *indio_dev = spi_get_drvdata(spi);
-	struct adis *st = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	adis_cleanup_buffer_and_trigger(st, indio_dev);
-
-	return 0;
-}
-
-static struct spi_driver adis16201_driver = {
-	.driver = {
-		.name = "adis16201",
-	},
-	.probe = adis16201_probe,
-	.remove = adis16201_remove,
-};
-module_spi_driver(adis16201_driver);
-
-MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
-MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer");
-MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("spi:adis16201");
-- 
2.7.4

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Dan Carpenter]

I think -M is prefered for these types of diffs?  Not sure.

On Mon, Feb 12, 2018 at 05:24:59PM +0530, Himanshu Jha wrote:
> +static int adis16201_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct adis *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	indio_dev->name = spi->dev.driver->name;
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &adis16201_info;
> +
> +	indio_dev->channels = adis16201_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = adis_init(st, indio_dev, spi, &adis16201_data);
> +	if (ret)
> +		return ret;
> +
> +	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> +	if (ret)
> +		return ret;

We should clean up the IRQ which we enabled in adis_init() instead of
returning directly.

> +
> +	/* Get the device into a sane initial state */
> +	ret = adis_initial_startup(st);
> +	if (ret)
> +		goto error_cleanup_buffer_trigger;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret < 0)
> +		goto error_cleanup_buffer_trigger;
> +
> +	return 0;
> +
> +error_cleanup_buffer_trigger:
> +	adis_cleanup_buffer_and_trigger(st, indio_dev);
> +
> +	return ret;
> +}

Otherwise it looks fine to my not-an-iio-expert eye.

regards,
dan carpenter

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Himanshu Jha]

On Mon, Feb 12, 2018 at 04:18:26PM +0300, Dan Carpenter wrote:
> I think -M is prefered for these types of diffs?  Not sure.

I wrote about that in the cover letter if you missed. :)

> On Mon, Feb 12, 2018 at 05:24:59PM +0530, Himanshu Jha wrote:
> > +static int adis16201_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct adis *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +	spi_set_drvdata(spi, indio_dev);
> > +
> > +	indio_dev->name = spi->dev.driver->name;
> > +	indio_dev->dev.parent = &spi->dev;
> > +	indio_dev->info = &adis16201_info;
> > +
> > +	indio_dev->channels = adis16201_channels;
> > +	indio_dev->num_channels = ARRAY_SIZE(adis16201_channels);
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +
> > +	ret = adis_init(st, indio_dev, spi, &adis16201_data);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > +	if (ret)
> > +		return ret;
> 
> We should clean up the IRQ which we enabled in adis_init() instead of
> returning directly.

I'm not sure about how to do that.

-- 
Thanks
Himanshu Jha

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Dan Carpenter]

On Mon, Feb 12, 2018 at 08:11:57PM +0530, Himanshu Jha wrote:
> On Mon, Feb 12, 2018 at 04:18:26PM +0300, Dan Carpenter wrote:
> > I think -M is prefered for these types of diffs?  Not sure.
> 
> I wrote about that in the cover letter if you missed. :)
> 

Yeah.  I seldom read cover letters.

> > > +	ret = adis_init(st, indio_dev, spi, &adis16201_data);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > > +	if (ret)
> > > +		return ret;
> > 
> > We should clean up the IRQ which we enabled in adis_init() instead of
> > returning directly.
> 
> I'm not sure about how to do that.
> 

I believe in you that you can figure it out.  :)

regards,
dan carpenter

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Jonathan Cameron]

On Mon, 12 Feb 2018 17:45:42 +0300
Dan Carpenter <dan.carpenter@oracle.com> wrote:

> On Mon, Feb 12, 2018 at 08:11:57PM +0530, Himanshu Jha wrote:
> > On Mon, Feb 12, 2018 at 04:18:26PM +0300, Dan Carpenter wrote:  
> > > I think -M is prefered for these types of diffs?  Not sure.  
> > 
> > I wrote about that in the cover letter if you missed. :)
> >   
> 
> Yeah.  I seldom read cover letters.

For anyone else (like me) who also doesn't often read them...

This was a specific request from me to not use move detection.

The reason is that it presents an opportunity for staging graduation patches
for people to have the whole code in front of them allowing them to do a full
review as if it were a new driver.

I personally find this very helpful in this one case. For any other
code move I'm as in favour of short emails as the next person ;)

Jonathan
> 
> > > > +	ret = adis_init(st, indio_dev, spi, &adis16201_data);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	ret = adis_setup_buffer_and_trigger(st, indio_dev, NULL);
> > > > +	if (ret)
> > > > +		return ret;  
> > > 
> > > We should clean up the IRQ which we enabled in adis_init() instead of
> > > returning directly.  
> > 
> > I'm not sure about how to do that.
> >   
> 
> I believe in you that you can figure it out.  :)
> 
> regards,
> dan carpenter
> 
> 

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Philippe Ombredanne]

On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha
<himanshujha199640@gmail.com> wrote:
> Move the adis16201 driver out of staging directory and merge to the
> mainline IIO directory.
>
> Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>

<snip>
> --- /dev/null
> +++ b/drivers/iio/accel/adis16201.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0+

<snip>

> +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("spi:adis16201");

Your MODULE_LICENSE does not match your SPDX license id.
MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0
MODULE_LICENSE("GPL"); means SPDX GPL-2.0+


-- 
Cordially
Philippe Ombredanne

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Himanshu Jha]

On Mon, Feb 12, 2018 at 03:10:56PM +0100, Philippe Ombredanne wrote:
> On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha
> <himanshujha199640@gmail.com> wrote:
> > Move the adis16201 driver out of staging directory and merge to the
> > mainline IIO directory.
> >
> > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
> 
> <snip>
> > --- /dev/null
> > +++ b/drivers/iio/accel/adis16201.c
> > @@ -0,0 +1,315 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> 
> <snip>
> 
> > +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
> > +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("spi:adis16201");
> 
> Your MODULE_LICENSE does not match your SPDX license id.
> MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0
> MODULE_LICENSE("GPL"); means SPDX GPL-2.0+
> 

I didn't knew about that! Thanks for pointing.

-- 
Thanks
Himanshu Jha

Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging

[Philippe Ombredanne]

On Mon, Feb 12, 2018 at 3:37 PM, Himanshu Jha
<himanshujha199640@gmail.com> wrote:
> On Mon, Feb 12, 2018 at 03:10:56PM +0100, Philippe Ombredanne wrote:
>> On Mon, Feb 12, 2018 at 12:54 PM, Himanshu Jha
>> <himanshujha199640@gmail.com> wrote:
>> > Move the adis16201 driver out of staging directory and merge to the
>> > mainline IIO directory.
>> >
>> > Signed-off-by: Himanshu Jha <himanshujha199640@gmail.com>
>>
>> <snip>
>> > --- /dev/null
>> > +++ b/drivers/iio/accel/adis16201.c
>> > @@ -0,0 +1,315 @@
>> > +// SPDX-License-Identifier: GPL-2.0+
>>
>> <snip>
>>
>> > +MODULE_AUTHOR("Barry Song <21cnbao@gmail.com>");
>> > +MODULE_DESCRIPTION("Analog Devices ADIS16201 Dual-Axis Digital Inclinometer and Accelerometer");
>> > +MODULE_LICENSE("GPL v2");
>> > +MODULE_ALIAS("spi:adis16201");
>>
>> Your MODULE_LICENSE does not match your SPDX license id.
>> MODULE_LICENSE("GPL v2"); means SPDX GPL-2.0
>> MODULE_LICENSE("GPL"); means SPDX GPL-2.0+
>>
>
> I didn't knew about that! Thanks for pointing.

Sure thing. I reckon this can be a tad surprising.
FWIW, the reference for the SPDX tag is in
Documentation/process/license-rules.rst and for the MODULE_LICENSE
macro this is in module.h
It is unlikely module.h will ever evolve there as this would break all
third-party module loaders/tools that rely on the documented data
values and conventions

-- 
Cordially
Philippe Ombredanne