* [PATCH 0/4] staging: iio: accel: adis16201 driver cleanup
@ 2018-02-12 11:54 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, linux-kernel,
Himanshu Jha, pmeerw, knaack.h
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 0/4] staging: iio: accel: adis16201 driver cleanup
@ 2018-02-12 11:54 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio,
devel, linux-kernel, 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
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH 1/4] staging: iio: accel: adis16201: Use SPDX identifier
2018-02-12 11:54 ` Himanshu Jha
@ 2018-02-12 11:54 ` Himanshu Jha
-1 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, linux-kernel,
Himanshu Jha, pmeerw, knaack.h
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 1/4] staging: iio: accel: adis16201: Use SPDX identifier
@ 2018-02-12 11:54 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio,
devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
2018-02-12 11:54 ` Himanshu Jha
@ 2018-02-12 11:54 ` Himanshu Jha
-1 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, linux-kernel,
Himanshu Jha, pmeerw, knaack.h
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
@ 2018-02-12 11:54 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio,
devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
2018-02-12 11:54 ` Himanshu Jha
@ 2018-02-12 11:54 ` Himanshu Jha
-1 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, linux-kernel,
Himanshu Jha, pmeerw, knaack.h
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
@ 2018-02-12 11:54 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio,
devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
2018-02-12 11:54 ` Himanshu Jha
@ 2018-02-12 11:54 ` Himanshu Jha
-1 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, linux-kernel,
Himanshu Jha, pmeerw, knaack.h
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
@ 2018-02-12 11:54 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 11:54 UTC (permalink / raw)
To: 21cnbao, jic23
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, gregkh, linux-iio,
devel, linux-kernel, 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
^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
2018-02-12 11:54 ` Himanshu Jha
@ 2018-02-12 12:53 ` Dan Carpenter
-1 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 12:53 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
@ 2018-02-12 12:53 ` Dan Carpenter
0 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 12:53 UTC (permalink / raw)
To: Himanshu Jha
Cc: 21cnbao, jic23, devel, lars, Michael.Hennerich, linux-iio,
gregkh, linux-kernel, pmeerw, knaack.h
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
2018-02-12 11:54 ` Himanshu Jha
@ 2018-02-12 13:10 ` Dan Carpenter
-1 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 13:10 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
@ 2018-02-12 13:10 ` Dan Carpenter
0 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 13:10 UTC (permalink / raw)
To: Himanshu Jha
Cc: 21cnbao, jic23, devel, lars, Michael.Hennerich, linux-iio,
gregkh, linux-kernel, pmeerw, knaack.h
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
2018-02-12 11:54 ` Himanshu Jha
@ 2018-02-12 13:18 ` Dan Carpenter
-1 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 13:18 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
@ 2018-02-12 13:18 ` Dan Carpenter
0 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 13:18 UTC (permalink / raw)
To: Himanshu Jha
Cc: 21cnbao, jic23, devel, lars, Michael.Hennerich, linux-iio,
gregkh, linux-kernel, pmeerw, knaack.h
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
2018-02-12 11:54 ` Himanshu Jha
@ 2018-02-12 14:10 ` Philippe Ombredanne
-1 siblings, 0 replies; 41+ messages in thread
From: Philippe Ombredanne @ 2018-02-12 14:10 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, Lars-Peter Clausen, Michael.Hennerich, linux-iio,
Greg Kroah-Hartman, 21cnbao, LKML, Peter Meerwald-Stadler,
Hartmut Knaack, Jonathan Cameron
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
@ 2018-02-12 14:10 ` Philippe Ombredanne
0 siblings, 0 replies; 41+ messages in thread
From: Philippe Ombredanne @ 2018-02-12 14:10 UTC (permalink / raw)
To: Himanshu Jha
Cc: 21cnbao, Jonathan Cameron, Lars-Peter Clausen, Michael.Hennerich,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
linux-iio, devel, LKML
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
2018-02-12 12:53 ` Dan Carpenter
@ 2018-02-12 14:35 ` Himanshu Jha
-1 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 14:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
@ 2018-02-12 14:35 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 14:35 UTC (permalink / raw)
To: Dan Carpenter
Cc: 21cnbao, jic23, devel, lars, Michael.Hennerich, linux-iio,
gregkh, linux-kernel, pmeerw, knaack.h
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
2018-02-12 14:10 ` Philippe Ombredanne
@ 2018-02-12 14:37 ` Himanshu Jha
-1 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 14:37 UTC (permalink / raw)
To: Philippe Ombredanne
Cc: devel, Lars-Peter Clausen, Michael.Hennerich, linux-iio,
Greg Kroah-Hartman, 21cnbao, LKML, Peter Meerwald-Stadler,
Hartmut Knaack, Jonathan Cameron
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
@ 2018-02-12 14:37 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 14:37 UTC (permalink / raw)
To: Philippe Ombredanne
Cc: 21cnbao, Jonathan Cameron, Lars-Peter Clausen, Michael.Hennerich,
Hartmut Knaack, Peter Meerwald-Stadler, Greg Kroah-Hartman,
linux-iio, devel, LKML
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
2018-02-12 13:18 ` Dan Carpenter
@ 2018-02-12 14:41 ` Himanshu Jha
-1 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 14:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
@ 2018-02-12 14:41 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 14:41 UTC (permalink / raw)
To: Dan Carpenter
Cc: 21cnbao, jic23, devel, lars, Michael.Hennerich, linux-iio,
gregkh, linux-kernel, pmeerw, knaack.h
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
2018-02-12 14:41 ` Himanshu Jha
@ 2018-02-12 14:45 ` Dan Carpenter
-1 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 14:45 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
@ 2018-02-12 14:45 ` Dan Carpenter
0 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 14:45 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
2018-02-12 14:35 ` Himanshu Jha
@ 2018-02-12 14:57 ` Dan Carpenter
-1 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 14:57 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
@ 2018-02-12 14:57 ` Dan Carpenter
0 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-12 14:57 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
2018-02-12 14:57 ` Dan Carpenter
@ 2018-02-12 19:46 ` Himanshu Jha
-1 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 19:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
@ 2018-02-12 19:46 ` Himanshu Jha
0 siblings, 0 replies; 41+ messages in thread
From: Himanshu Jha @ 2018-02-12 19:46 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, jic23
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
2018-02-12 14:37 ` Himanshu Jha
(?)
@ 2018-02-12 22:18 ` Philippe Ombredanne
-1 siblings, 0 replies; 41+ messages in thread
From: Philippe Ombredanne @ 2018-02-12 22:18 UTC (permalink / raw)
To: Himanshu Jha
Cc: Barry Song, Jonathan Cameron, Lars-Peter Clausen,
Michael.Hennerich, Hartmut Knaack, Peter Meerwald-Stadler,
Greg Kroah-Hartman, linux-iio, devel, LKML
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
2018-02-12 14:57 ` Dan Carpenter
@ 2018-02-17 12:16 ` Jonathan Cameron
-1 siblings, 0 replies; 41+ messages in thread
From: Jonathan Cameron @ 2018-02-17 12:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, Himanshu Jha, pmeerw, knaack.h
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
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
@ 2018-02-17 12:16 ` Jonathan Cameron
0 siblings, 0 replies; 41+ messages in thread
From: Jonathan Cameron @ 2018-02-17 12:16 UTC (permalink / raw)
To: Dan Carpenter
Cc: Himanshu Jha, devel, lars, Michael.Hennerich, linux-iio, gregkh,
21cnbao, linux-kernel, pmeerw, knaack.h
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
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
2018-02-12 19:46 ` Himanshu Jha
@ 2018-02-17 12:19 ` Jonathan Cameron
-1 siblings, 0 replies; 41+ messages in thread
From: Jonathan Cameron @ 2018-02-17 12:19 UTC (permalink / raw)
To: Himanshu Jha
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, pmeerw, knaack.h, Dan Carpenter
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix
@ 2018-02-17 12:19 ` Jonathan Cameron
0 siblings, 0 replies; 41+ messages in thread
From: Jonathan Cameron @ 2018-02-17 12:19 UTC (permalink / raw)
To: Himanshu Jha
Cc: Dan Carpenter, devel, lars, Michael.Hennerich, linux-iio, gregkh,
21cnbao, linux-kernel, pmeerw, knaack.h
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
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
2018-02-12 13:10 ` Dan Carpenter
@ 2018-02-17 12:23 ` Jonathan Cameron
-1 siblings, 0 replies; 41+ messages in thread
From: Jonathan Cameron @ 2018-02-17 12:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, Himanshu Jha, pmeerw, knaack.h
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
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
@ 2018-02-17 12:23 ` Jonathan Cameron
0 siblings, 0 replies; 41+ messages in thread
From: Jonathan Cameron @ 2018-02-17 12:23 UTC (permalink / raw)
To: Dan Carpenter
Cc: Himanshu Jha, 21cnbao, devel, lars, Michael.Hennerich, linux-iio,
gregkh, linux-kernel, pmeerw, knaack.h
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
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
2018-02-12 14:45 ` Dan Carpenter
@ 2018-02-17 12:26 ` Jonathan Cameron
-1 siblings, 0 replies; 41+ messages in thread
From: Jonathan Cameron @ 2018-02-17 12:26 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, Himanshu Jha, pmeerw, knaack.h
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
>
>
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging
@ 2018-02-17 12:26 ` Jonathan Cameron
0 siblings, 0 replies; 41+ messages in thread
From: Jonathan Cameron @ 2018-02-17 12:26 UTC (permalink / raw)
To: Dan Carpenter
Cc: Himanshu Jha, devel, lars, Michael.Hennerich, linux-iio, gregkh,
21cnbao, linux-kernel, pmeerw, knaack.h
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
>
>
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
2018-02-17 12:23 ` Jonathan Cameron
@ 2018-02-17 17:24 ` Dan Carpenter
-1 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-17 17:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: devel, lars, Michael.Hennerich, linux-iio, gregkh, 21cnbao,
linux-kernel, Himanshu Jha, pmeerw, knaack.h
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
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement
@ 2018-02-17 17:24 ` Dan Carpenter
0 siblings, 0 replies; 41+ messages in thread
From: Dan Carpenter @ 2018-02-17 17:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Himanshu Jha, 21cnbao, devel, lars, Michael.Hennerich, linux-iio,
gregkh, linux-kernel, pmeerw, knaack.h
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
^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2018-02-17 17:24 UTC | newest]
Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-12 11:54 [PATCH 0/4] staging: iio: accel: adis16201 driver cleanup Himanshu Jha
2018-02-12 11:54 ` Himanshu Jha
2018-02-12 11:54 ` [PATCH 1/4] staging: iio: accel: adis16201: Use SPDX identifier Himanshu Jha
2018-02-12 11:54 ` Himanshu Jha
2018-02-12 11:54 ` [PATCH 2/4] staging: iio: accel: Remove unnecessary comments and add suitable suffix Himanshu Jha
2018-02-12 11:54 ` Himanshu Jha
2018-02-12 12:53 ` Dan Carpenter
2018-02-12 12:53 ` Dan Carpenter
2018-02-12 14:35 ` Himanshu Jha
2018-02-12 14:35 ` Himanshu Jha
2018-02-12 14:57 ` Dan Carpenter
2018-02-12 14:57 ` Dan Carpenter
2018-02-12 19:46 ` Himanshu Jha
2018-02-12 19:46 ` Himanshu Jha
2018-02-17 12:19 ` Jonathan Cameron
2018-02-17 12:19 ` Jonathan Cameron
2018-02-17 12:16 ` Jonathan Cameron
2018-02-17 12:16 ` Jonathan Cameron
2018-02-12 11:54 ` [PATCH 3/4] staging: iio: accel: Use sign_extend32 and adjust a switch statement Himanshu Jha
2018-02-12 11:54 ` Himanshu Jha
2018-02-12 13:10 ` Dan Carpenter
2018-02-12 13:10 ` Dan Carpenter
2018-02-17 12:23 ` Jonathan Cameron
2018-02-17 12:23 ` Jonathan Cameron
2018-02-17 17:24 ` Dan Carpenter
2018-02-17 17:24 ` Dan Carpenter
2018-02-12 11:54 ` [PATCH 4/4] staging: iio: accel: Move adis16201 driver out of staging Himanshu Jha
2018-02-12 11:54 ` Himanshu Jha
2018-02-12 13:18 ` Dan Carpenter
2018-02-12 13:18 ` Dan Carpenter
2018-02-12 14:41 ` Himanshu Jha
2018-02-12 14:41 ` Himanshu Jha
2018-02-12 14:45 ` Dan Carpenter
2018-02-12 14:45 ` Dan Carpenter
2018-02-17 12:26 ` Jonathan Cameron
2018-02-17 12:26 ` Jonathan Cameron
2018-02-12 14:10 ` Philippe Ombredanne
2018-02-12 14:10 ` Philippe Ombredanne
2018-02-12 14:37 ` Himanshu Jha
2018-02-12 14:37 ` Himanshu Jha
2018-02-12 22:18 ` Philippe Ombredanne
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.