* [PATCH 0/3] adis16209 driver cleanup
@ 2018-02-17 16:02 Shreeya Patel
2018-02-17 16:04 ` [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier Shreeya Patel
` (2 more replies)
0 siblings, 3 replies; 13+ messages in thread
From: Shreeya Patel @ 2018-02-17 16:02 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, linux-iio,
devel, daniel.baluta
Cc: Shreeya Patel
This patchset has been introduced for the cleanup of
adis16209 driver.
This patchset cleans up miscellaneous code fragments
and improves the code readability.
Shreeya Patel (3):
Staging: iio: adis16209: Use SPDX identifier
Staging: iio: adis16209: Remove unnecessary comments and add suitable
suffix
Staging: iio: adis16209: Use sign_extend32 and adjust a switch
statement
drivers/staging/iio/accel/adis16209.c | 231 ++++++++++++----------------------
1 file changed, 82 insertions(+), 149 deletions(-)
--
2.7.4
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
2018-02-17 16:02 [PATCH 0/3] adis16209 driver cleanup Shreeya Patel
@ 2018-02-17 16:04 ` Shreeya Patel
2018-02-18 11:31 ` Himanshu Jha
2018-02-18 12:04 ` Jonathan Cameron
2018-02-17 16:07 ` [PATCH 2/3] Staging: iio: adis16209: Remove unnecessary comments and add suitable suffix Shreeya Patel
2018-02-17 16:21 ` [PATCH 3/3] Staging: iio: adis16209: Use sign_extend32 and adjust a switch statement Shreeya Patel
2 siblings, 2 replies; 13+ messages in thread
From: Shreeya Patel @ 2018-02-17 16:04 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, linux-iio,
devel, daniel.baluta
Cc: Shreeya Patel
Use SPDX identifier format instead of GPLv2. Also rearrange the
headers in alphabetical order.
Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
drivers/staging/iio/accel/adis16209.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c
index 7fcef9a..e3d9f80 100644
--- a/drivers/staging/iio/accel/adis16209.c
+++ b/drivers/staging/iio/accel/adis16209.c
@@ -1,19 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0+
/*
* ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
*
* Copyright 2010 Analog Devices Inc.
- *
- * Licensed under the GPL-2 or later.
*/
#include <linux/delay.h>
#include <linux/device.h>
#include <linux/kernel.h>
+#include <linux/list.h>
+#include <linux/module.h>
#include <linux/spi/spi.h>
#include <linux/slab.h>
#include <linux/sysfs.h>
-#include <linux/list.h>
-#include <linux/module.h>
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 2/3] Staging: iio: adis16209: Remove unnecessary comments and add suitable suffix
2018-02-17 16:02 [PATCH 0/3] adis16209 driver cleanup Shreeya Patel
2018-02-17 16:04 ` [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier Shreeya Patel
@ 2018-02-17 16:07 ` Shreeya Patel
2018-02-18 12:44 ` Jonathan Cameron
2018-02-17 16:21 ` [PATCH 3/3] Staging: iio: adis16209: Use sign_extend32 and adjust a switch statement Shreeya Patel
2 siblings, 1 reply; 13+ messages in thread
From: Shreeya Patel @ 2018-02-17 16:07 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, linux-iio,
devel, daniel.baluta
Cc: Shreeya Patel
Remove some unnecessary comments and therefore, add _REG suffix
for registers. Group the control register and register field
macros together.
Also, align function arguments to match open parentheses using
tabs.
Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
---
drivers/staging/iio/accel/adis16209.c | 209 +++++++++++-----------------------
1 file changed, 69 insertions(+), 140 deletions(-)
diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c
index e3d9f80..6101212 100644
--- a/drivers/staging/iio/accel/adis16209.c
+++ b/drivers/staging/iio/accel/adis16209.c
@@ -19,136 +19,67 @@
#include <linux/iio/buffer.h>
#include <linux/iio/imu/adis.h>
-#define ADIS16209_STARTUP_DELAY 220 /* ms */
-
-/* Flash memory write count */
-#define ADIS16209_FLASH_CNT 0x00
-
-/* Output, power supply */
-#define ADIS16209_SUPPLY_OUT 0x02
-
-/* Output, x-axis accelerometer */
-#define ADIS16209_XACCL_OUT 0x04
-
-/* Output, y-axis accelerometer */
-#define ADIS16209_YACCL_OUT 0x06
-
-/* Output, auxiliary ADC input */
-#define ADIS16209_AUX_ADC 0x08
-
-/* Output, temperature */
-#define ADIS16209_TEMP_OUT 0x0A
-
-/* Output, x-axis inclination */
-#define ADIS16209_XINCL_OUT 0x0C
-
-/* Output, y-axis inclination */
-#define ADIS16209_YINCL_OUT 0x0E
-
-/* Output, +/-180 vertical rotational position */
-#define ADIS16209_ROT_OUT 0x10
-
-/* Calibration, x-axis acceleration offset null */
-#define ADIS16209_XACCL_NULL 0x12
-
-/* Calibration, y-axis acceleration offset null */
-#define ADIS16209_YACCL_NULL 0x14
-
-/* Calibration, x-axis inclination offset null */
-#define ADIS16209_XINCL_NULL 0x16
-
-/* Calibration, y-axis inclination offset null */
-#define ADIS16209_YINCL_NULL 0x18
-
-/* Calibration, vertical rotation offset null */
-#define ADIS16209_ROT_NULL 0x1A
-
-/* Alarm 1 amplitude threshold */
-#define ADIS16209_ALM_MAG1 0x20
-
-/* Alarm 2 amplitude threshold */
-#define ADIS16209_ALM_MAG2 0x22
-
-/* Alarm 1, sample period */
-#define ADIS16209_ALM_SMPL1 0x24
-
-/* Alarm 2, sample period */
-#define ADIS16209_ALM_SMPL2 0x26
-
-/* Alarm control */
-#define ADIS16209_ALM_CTRL 0x28
-
-/* Auxiliary DAC data */
-#define ADIS16209_AUX_DAC 0x30
-
-/* General-purpose digital input/output control */
-#define ADIS16209_GPIO_CTRL 0x32
-
-/* Miscellaneous control */
-#define ADIS16209_MSC_CTRL 0x34
-
-/* Internal sample period (rate) control */
-#define ADIS16209_SMPL_PRD 0x36
-
-/* Operation, filter configuration */
-#define ADIS16209_AVG_CNT 0x38
-
-/* Operation, sleep mode control */
-#define ADIS16209_SLP_CNT 0x3A
-
-/* Diagnostics, system status register */
-#define ADIS16209_DIAG_STAT 0x3C
-
-/* Operation, system command register */
-#define ADIS16209_GLOB_CMD 0x3E
+#define ADIS16209_STARTUP_DELAY_MS 220
+#define ADIS16209_FLASH_CNT_REG 0x00
+
+/* Data Output Register Definitions */
+#define ADIS16209_SUPPLY_OUT_REG 0x02
+#define ADIS16209_XACCL_OUT_REG 0x04
+#define ADIS16209_YACCL_OUT_REG 0x06
+#define ADIS16209_AUX_ADC_REG 0x08
+#define ADIS16209_TEMP_OUT_REG 0x0A
+#define ADIS16209_XINCL_OUT_REG 0x0C
+#define ADIS16209_YINCL_OUT_REG 0x0E
+#define ADIS16209_ROT_OUT_REG 0x10
+
+/* Calibration Register Definitions */
+#define ADIS16209_XACCL_NULL_REG 0x12
+#define ADIS16209_YACCL_NULL_REG 0x14
+#define ADIS16209_XINCL_NULL_REG 0x16
+#define ADIS16209_YINCL_NULL_REG 0x18
+#define ADIS16209_ROT_NULL_REG 0x1A
+
+/* Alarm Register Definitions */
+#define ADIS16209_ALM_MAG1_REG 0x20
+#define ADIS16209_ALM_MAG2_REG 0x22
+#define ADIS16209_ALM_SMPL1_REG 0x24
+#define ADIS16209_ALM_SMPL2_REG 0x26
+#define ADIS16209_ALM_CTRL_REG 0x28
+
+#define ADIS16209_AUX_DAC_REG 0x30
+#define ADIS16209_GPIO_CTRL_REG 0x32
+#define ADIS16209_SMPL_PRD_REG 0x36
+#define ADIS16209_AVG_CNT_REG 0x38
+#define ADIS16209_SLP_CNT_REG 0x3A
/* MSC_CTRL */
-/* Self-test at power-on: 1 = disabled, 0 = enabled */
+#define ADIS16209_MSC_CTRL_REG 0x34
#define ADIS16209_MSC_CTRL_PWRUP_SELF_TEST BIT(10)
-
-/* Self-test enable */
-#define ADIS16209_MSC_CTRL_SELF_TEST_EN BIT(8)
-
-/* Data-ready enable: 1 = enabled, 0 = disabled */
-#define ADIS16209_MSC_CTRL_DATA_RDY_EN BIT(2)
-
-/* Data-ready polarity: 1 = active high, 0 = active low */
-#define ADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1)
-
-/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
-#define ADIS16209_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
+#define ADIS16209_MSC_CTRL_SELF_TEST_EN BIT(8)
+#define ADIS16209_MSC_CTRL_DATA_RDY_EN BIT(2)
+#define ADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1)
+#define ADIS16209_MSC_CTRL_DATA_RDY_DIO1 BIT(0)
/* DIAG_STAT */
-/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
-#define ADIS16209_DIAG_STAT_ALARM2 BIT(9)
-
-/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
-#define ADIS16209_DIAG_STAT_ALARM1 BIT(8)
-
-/* Self-test diagnostic error flag: 1 = error condition, 0 = normal operation */
+#define ADIS16209_DIAG_STAT_REG 0x3C
+#define ADIS16209_DIAG_STAT_ALARM2 BIT(9)
+#define ADIS16209_DIAG_STAT_ALARM1 BIT(8)
#define ADIS16209_DIAG_STAT_SELFTEST_FAIL_BIT 5
-
-/* SPI communications failure */
#define ADIS16209_DIAG_STAT_SPI_FAIL_BIT 3
-
-/* Flash update failure */
#define ADIS16209_DIAG_STAT_FLASH_UPT_BIT 2
-
-/* Power supply above 3.625 V */
#define ADIS16209_DIAG_STAT_POWER_HIGH_BIT 1
-
-/* Power supply below 3.15 V */
#define ADIS16209_DIAG_STAT_POWER_LOW_BIT 0
/* GLOB_CMD */
-#define ADIS16209_GLOB_CMD_SW_RESET BIT(7)
-#define ADIS16209_GLOB_CMD_CLEAR_STAT BIT(4)
-#define ADIS16209_GLOB_CMD_FACTORY_CAL BIT(1)
+#define ADIS16209_GLOB_CMD_REG 0x3E
+#define ADIS16209_GLOB_CMD_SW_RESET BIT(7)
+#define ADIS16209_GLOB_CMD_CLEAR_STAT BIT(4)
+#define ADIS16209_GLOB_CMD_FACTORY_CAL BIT(1)
-#define ADIS16209_ERROR_ACTIVE BIT(14)
+#define ADIS16209_ERROR_ACTIVE BIT(14)
enum adis16209_scan {
ADIS16209_SCAN_SUPPLY,
@@ -164,10 +95,10 @@ enum adis16209_scan {
static const u8 adis16209_addresses[8][1] = {
[ADIS16209_SCAN_SUPPLY] = { },
[ADIS16209_SCAN_AUX_ADC] = { },
- [ADIS16209_SCAN_ACC_X] = { ADIS16209_XACCL_NULL },
- [ADIS16209_SCAN_ACC_Y] = { ADIS16209_YACCL_NULL },
- [ADIS16209_SCAN_INCLI_X] = { ADIS16209_XINCL_NULL },
- [ADIS16209_SCAN_INCLI_Y] = { ADIS16209_YINCL_NULL },
+ [ADIS16209_SCAN_ACC_X] = { ADIS16209_XACCL_NULL_REG },
+ [ADIS16209_SCAN_ACC_Y] = { ADIS16209_YACCL_NULL_REG },
+ [ADIS16209_SCAN_INCLI_X] = { ADIS16209_XINCL_NULL_REG },
+ [ADIS16209_SCAN_INCLI_Y] = { ADIS16209_YINCL_NULL_REG },
[ADIS16209_SCAN_ROT] = { },
[ADIS16209_SCAN_TEMP] = { },
};
@@ -214,35 +145,35 @@ static int adis16209_read_raw(struct iio_dev *indio_dev,
switch (mask) {
case IIO_CHAN_INFO_RAW:
return adis_single_conversion(indio_dev, chan,
- ADIS16209_ERROR_ACTIVE, val);
+ ADIS16209_ERROR_ACTIVE, val);
case IIO_CHAN_INFO_SCALE:
switch (chan->type) {
case IIO_VOLTAGE:
*val = 0;
if (chan->channel == 0)
- *val2 = 305180; /* 0.30518 mV */
+ *val2 = 305180;
else
- *val2 = 610500; /* 0.6105 mV */
+ *val2 = 610500;
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(244140); /* 0.244140 mg */
+ *val2 = IIO_G_TO_M_S_2(244140);
return IIO_VAL_INT_PLUS_NANO;
case IIO_INCLI:
case IIO_ROT:
*val = 0;
- *val2 = 25000; /* 0.025 degree */
+ *val2 = 25000;
return IIO_VAL_INT_PLUS_MICRO;
default:
return -EINVAL;
}
break;
case IIO_CHAN_INFO_OFFSET:
- *val = 25000 / -470 - 0x4FE; /* 25 C = 0x4FE */
+ *val = 25000 / -470 - 0x4FE;
return IIO_VAL_INT;
case IIO_CHAN_INFO_CALIBBIAS:
switch (chan->type) {
@@ -265,18 +196,19 @@ static int adis16209_read_raw(struct iio_dev *indio_dev,
}
static const struct iio_chan_spec adis16209_channels[] = {
- ADIS_SUPPLY_CHAN(ADIS16209_SUPPLY_OUT, ADIS16209_SCAN_SUPPLY, 0, 14),
- ADIS_TEMP_CHAN(ADIS16209_TEMP_OUT, ADIS16209_SCAN_TEMP, 0, 12),
- ADIS_ACCEL_CHAN(X, ADIS16209_XACCL_OUT, ADIS16209_SCAN_ACC_X,
+ ADIS_SUPPLY_CHAN(ADIS16209_SUPPLY_OUT_REG, ADIS16209_SCAN_SUPPLY,
+ 0, 14),
+ ADIS_TEMP_CHAN(ADIS16209_TEMP_OUT_REG, ADIS16209_SCAN_TEMP, 0, 12),
+ ADIS_ACCEL_CHAN(X, ADIS16209_XACCL_OUT_REG, ADIS16209_SCAN_ACC_X,
BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
- ADIS_ACCEL_CHAN(Y, ADIS16209_YACCL_OUT, ADIS16209_SCAN_ACC_Y,
+ ADIS_ACCEL_CHAN(Y, ADIS16209_YACCL_OUT_REG, ADIS16209_SCAN_ACC_Y,
BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
- ADIS_AUX_ADC_CHAN(ADIS16209_AUX_ADC, ADIS16209_SCAN_AUX_ADC, 0, 12),
- ADIS_INCLI_CHAN(X, ADIS16209_XINCL_OUT, ADIS16209_SCAN_INCLI_X,
+ ADIS_AUX_ADC_CHAN(ADIS16209_AUX_ADC_REG, ADIS16209_SCAN_AUX_ADC, 0, 12),
+ ADIS_INCLI_CHAN(X, ADIS16209_XINCL_OUT_REG, ADIS16209_SCAN_INCLI_X,
0, 0, 14),
- ADIS_INCLI_CHAN(Y, ADIS16209_YINCL_OUT, ADIS16209_SCAN_INCLI_Y,
+ ADIS_INCLI_CHAN(Y, ADIS16209_YINCL_OUT_REG, ADIS16209_SCAN_INCLI_Y,
0, 0, 14),
- ADIS_ROT_CHAN(X, ADIS16209_ROT_OUT, ADIS16209_SCAN_ROT, 0, 0, 14),
+ ADIS_ROT_CHAN(X, ADIS16209_ROT_OUT_REG, ADIS16209_SCAN_ROT, 0, 0, 14),
IIO_CHAN_SOFT_TIMESTAMP(8)
};
@@ -296,13 +228,13 @@ static const char * const adis16209_status_error_msgs[] = {
static const struct adis_data adis16209_data = {
.read_delay = 30,
- .msc_ctrl_reg = ADIS16209_MSC_CTRL,
- .glob_cmd_reg = ADIS16209_GLOB_CMD,
- .diag_stat_reg = ADIS16209_DIAG_STAT,
+ .msc_ctrl_reg = ADIS16209_MSC_CTRL_REG,
+ .glob_cmd_reg = ADIS16209_GLOB_CMD_REG,
+ .diag_stat_reg = ADIS16209_DIAG_STAT_REG,
.self_test_mask = ADIS16209_MSC_CTRL_SELF_TEST_EN,
.self_test_no_autoclear = true,
- .startup_delay = ADIS16209_STARTUP_DELAY,
+ .startup_delay = ADIS16209_STARTUP_DELAY_MS,
.status_error_msgs = adis16209_status_error_msgs,
.status_error_mask = BIT(ADIS16209_DIAG_STAT_SELFTEST_FAIL_BIT) |
@@ -318,12 +250,10 @@ static int adis16209_probe(struct spi_device *spi)
struct adis *st;
struct iio_dev *indio_dev;
- /* setup the industrialio driver allocated elements */
indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
if (!indio_dev)
return -ENOMEM;
st = iio_priv(indio_dev);
- /* this is only used for removal purposes */
spi_set_drvdata(spi, indio_dev);
indio_dev->name = spi->dev.driver->name;
@@ -340,7 +270,6 @@ static int adis16209_probe(struct spi_device *spi)
if (ret)
return ret;
- /* Get the device into a sane initial state */
ret = adis_initial_startup(st);
if (ret)
goto error_cleanup_buffer_trigger;
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 3/3] Staging: iio: adis16209: Use sign_extend32 and adjust a switch statement
2018-02-17 16:02 [PATCH 0/3] adis16209 driver cleanup Shreeya Patel
2018-02-17 16:04 ` [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier Shreeya Patel
2018-02-17 16:07 ` [PATCH 2/3] Staging: iio: adis16209: Remove unnecessary comments and add suitable suffix Shreeya Patel
@ 2018-02-17 16:21 ` Shreeya Patel
2018-02-18 12:47 ` Jonathan Cameron
2 siblings, 1 reply; 13+ messages in thread
From: Shreeya Patel @ 2018-02-17 16:21 UTC (permalink / raw)
To: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, linux-iio,
devel, daniel.baluta
Cc: Shreeya Patel
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: Shreeya Patel <shreeya.patel23498@gmail.com>
---
drivers/staging/iio/accel/adis16209.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c
index 6101212..f7c228b 100644
--- a/drivers/staging/iio/accel/adis16209.c
+++ b/drivers/staging/iio/accel/adis16209.c
@@ -150,10 +150,16 @@ static int adis16209_read_raw(struct iio_dev *indio_dev,
switch (chan->type) {
case IIO_VOLTAGE:
*val = 0;
- if (chan->channel == 0)
+ switch (chan->channel) {
+ case 0:
*val2 = 305180;
- else
+ break;
+ case 1:
*val2 = 610500;
+ break;
+ default:
+ return -EINVAL;
+ }
return IIO_VAL_INT_PLUS_MICRO;
case IIO_TEMP:
*val = -470;
@@ -187,9 +193,8 @@ static int adis16209_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;
+
+ *val = sign_extend32(val16, bits - 1);
return IIO_VAL_INT;
}
return -EINVAL;
--
2.7.4
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
2018-02-17 16:04 ` [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier Shreeya Patel
@ 2018-02-18 11:31 ` Himanshu Jha
2018-02-18 11:37 ` Shreeya Patel
2018-02-18 12:04 ` Jonathan Cameron
1 sibling, 1 reply; 13+ messages in thread
From: Himanshu Jha @ 2018-02-18 11:31 UTC (permalink / raw)
To: Shreeya Patel
Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, linux-iio,
devel, daniel.baluta
Hi Shreeya,
On Sat, Feb 17, 2018 at 09:34:56PM +0530, Shreeya Patel wrote:
> Use SPDX identifier format instead of GPLv2. Also rearrange the
> headers in alphabetical order.
>
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
> drivers/staging/iio/accel/adis16209.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c
> index 7fcef9a..e3d9f80 100644
> --- a/drivers/staging/iio/accel/adis16209.c
> +++ b/drivers/staging/iio/accel/adis16209.c
> @@ -1,19 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0+
> /*
> * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
> *
> * Copyright 2010 Analog Devices Inc.
> - *
> - * Licensed under the GPL-2 or later.
I see that you too are doing similar cleanup which I did a while ago
https://lkml.org/lkml/2018/2/12/255
where I got some update suggestions for the patch series. It would be
great if you could update this patch series consistent with the
reviewers.
For eg: in this patch you changed
+// SPDX-License-Identifier: GPL-2.0+
and therefore
MODULE_LICENSE("GPL v2");
should also be changed to
MODULE_LICENSE("GPL");
as explained by Philippe Ombredanne to me in my patch series.
I am not sure if you're subscribed to the IIO mailing list but you can find
all the update suggestions from the above link. :)
--
Thanks
Himanshu Jha
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
2018-02-18 11:31 ` Himanshu Jha
@ 2018-02-18 11:37 ` Shreeya Patel
2018-02-18 12:02 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: Shreeya Patel @ 2018-02-18 11:37 UTC (permalink / raw)
To: Himanshu Jha
Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, linux-iio,
devel, daniel.baluta
On Sun, 2018-02-18 at 17:01 +0530, Himanshu Jha wrote:
> Hi Shreeya,
>
Hi Himanshu,
> On Sat, Feb 17, 2018 at 09:34:56PM +0530, Shreeya Patel wrote:
> >
> > Use SPDX identifier format instead of GPLv2. Also rearrange the
> > headers in alphabetical order.
> >
> > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> > ---
> > drivers/staging/iio/accel/adis16209.c | 7 +++----
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/iio/accel/adis16209.c
> > b/drivers/staging/iio/accel/adis16209.c
> > index 7fcef9a..e3d9f80 100644
> > --- a/drivers/staging/iio/accel/adis16209.c
> > +++ b/drivers/staging/iio/accel/adis16209.c
> > @@ -1,19 +1,18 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > /*
> > * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
> > *
> > * Copyright 2010 Analog Devices Inc.
> > - *
> > - * Licensed under the GPL-2 or later.
> I see that you too are doing similar cleanup which I did a while ago
> https://lkml.org/lkml/2018/2/12/255
Yes, Jonathan suggested me to work on adis16209.
Your patches were quite useful for me :)
> where I got some update suggestions for the patch series. It would be
> great if you could update this patch series consistent with the
> reviewers.
>
> For eg: in this patch you changed
>
> +// SPDX-License-Identifier: GPL-2.0+
>
> and therefore
>
> MODULE_LICENSE("GPL v2");
>
> should also be changed to
>
> MODULE_LICENSE("GPL");
>
> as explained by Philippe Ombredanne to me in my patch series.
Great.
I'll make the necessary changes.
>
> I am not sure if you're subscribed to the IIO mailing list but you
> can find
> all the update suggestions from the above link. :)
Thanks for your suggestions
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
2018-02-18 11:37 ` Shreeya Patel
@ 2018-02-18 12:02 ` Jonathan Cameron
2018-02-18 12:08 ` Lars-Peter Clausen
0 siblings, 1 reply; 13+ messages in thread
From: Jonathan Cameron @ 2018-02-18 12:02 UTC (permalink / raw)
To: Shreeya Patel
Cc: Himanshu Jha, lars, Michael.Hennerich, knaack.h, pmeerw,
linux-iio, devel, daniel.baluta
On Sun, 18 Feb 2018 17:07:57 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> On Sun, 2018-02-18 at 17:01 +0530, Himanshu Jha wrote:
> > Hi Shreeya,
> >
> Hi Himanshu,
>
> > On Sat, Feb 17, 2018 at 09:34:56PM +0530, Shreeya Patel wrote:
> > >
> > > Use SPDX identifier format instead of GPLv2. Also rearrange the
> > > headers in alphabetical order.
> > >
> > > Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> > > ---
> > > drivers/staging/iio/accel/adis16209.c | 7 +++----
> > > 1 file changed, 3 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/accel/adis16209.c
> > > b/drivers/staging/iio/accel/adis16209.c
> > > index 7fcef9a..e3d9f80 100644
> > > --- a/drivers/staging/iio/accel/adis16209.c
> > > +++ b/drivers/staging/iio/accel/adis16209.c
> > > @@ -1,19 +1,18 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > /*
> > > * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
> > > *
> > > * Copyright 2010 Analog Devices Inc.
> > > - *
> > > - * Licensed under the GPL-2 or later.
> > I see that you too are doing similar cleanup which I did a while ago
> > https://lkml.org/lkml/2018/2/12/255
>
> Yes, Jonathan suggested me to work on adis16209.
> Your patches were quite useful for me :)
>
> > where I got some update suggestions for the patch series. It would be
> > great if you could update this patch series consistent with the
> > reviewers.
> >
> > For eg: in this patch you changed
> >
> > +// SPDX-License-Identifier: GPL-2.0+
> >
> > and therefore
> >
> > MODULE_LICENSE("GPL v2");
> >
> > should also be changed to
> >
> > MODULE_LICENSE("GPL");
> >
> > as explained by Philippe Ombredanne to me in my patch series.
>
I'm not sure that was exactly what Philippe was suggesting.
He was making the point that the licensing was inconsistent without
saying which option should be chosen.
We will need to seek clarification from Analog Devices
on what their opinion on this is.
Lars / Michael, any clarification on the right way to resolve this
inconsistency?
> Great.
> I'll make the necessary changes.
>
> >
> > I am not sure if you're subscribed to the IIO mailing list but you
> > can find
> > all the update suggestions from the above link. :)
>
> Thanks for your suggestions
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
2018-02-17 16:04 ` [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier Shreeya Patel
2018-02-18 11:31 ` Himanshu Jha
@ 2018-02-18 12:04 ` Jonathan Cameron
1 sibling, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2018-02-18 12:04 UTC (permalink / raw)
To: Shreeya Patel
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, linux-iio, devel,
daniel.baluta
On Sat, 17 Feb 2018 21:34:56 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> Use SPDX identifier format instead of GPLv2. Also rearrange the
> headers in alphabetical order.
As Dan pointed out for the Himanshu Jha's patches, the moment
we see the word 'also' in a patch it pretty much makes it obvious
that the author knows it is doing two things.
Two patches please. This is all about making the changes
trivial to review. Sometimes it may seem silly where we have
two small changes like here, but when you are reviewing hundreds
of patches it really does help.
Jonathan
>
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
> drivers/staging/iio/accel/adis16209.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c
> index 7fcef9a..e3d9f80 100644
> --- a/drivers/staging/iio/accel/adis16209.c
> +++ b/drivers/staging/iio/accel/adis16209.c
> @@ -1,19 +1,18 @@
> +// SPDX-License-Identifier: GPL-2.0+
> /*
> * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
> *
> * Copyright 2010 Analog Devices Inc.
> - *
> - * Licensed under the GPL-2 or later.
> */
>
> #include <linux/delay.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> +#include <linux/list.h>
> +#include <linux/module.h>
> #include <linux/spi/spi.h>
> #include <linux/slab.h>
> #include <linux/sysfs.h>
> -#include <linux/list.h>
> -#include <linux/module.h>
>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
2018-02-18 12:02 ` Jonathan Cameron
@ 2018-02-18 12:08 ` Lars-Peter Clausen
2018-02-18 12:14 ` Lars-Peter Clausen
0 siblings, 1 reply; 13+ messages in thread
From: Lars-Peter Clausen @ 2018-02-18 12:08 UTC (permalink / raw)
To: Jonathan Cameron, Shreeya Patel
Cc: Himanshu Jha, Michael.Hennerich, knaack.h, pmeerw, linux-iio,
devel, daniel.baluta
On 02/18/2018 01:02 PM, Jonathan Cameron wrote:
> On Sun, 18 Feb 2018 17:07:57 +0530
> Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
>
>> On Sun, 2018-02-18 at 17:01 +0530, Himanshu Jha wrote:
>>> Hi Shreeya,
>>>
>> Hi Himanshu,
>>
>>> On Sat, Feb 17, 2018 at 09:34:56PM +0530, Shreeya Patel wrote:
>>>>
>>>> Use SPDX identifier format instead of GPLv2. Also rearrange the
>>>> headers in alphabetical order.
>>>>
>>>> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
>>>> ---
>>>> drivers/staging/iio/accel/adis16209.c | 7 +++----
>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/iio/accel/adis16209.c
>>>> b/drivers/staging/iio/accel/adis16209.c
>>>> index 7fcef9a..e3d9f80 100644
>>>> --- a/drivers/staging/iio/accel/adis16209.c
>>>> +++ b/drivers/staging/iio/accel/adis16209.c
>>>> @@ -1,19 +1,18 @@
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>> /*
>>>> * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
>>>> *
>>>> * Copyright 2010 Analog Devices Inc.
>>>> - *
>>>> - * Licensed under the GPL-2 or later.
>>> I see that you too are doing similar cleanup which I did a while ago
>>> https://lkml.org/lkml/2018/2/12/255
>>
>> Yes, Jonathan suggested me to work on adis16209.
>> Your patches were quite useful for me :)
>>
>>> where I got some update suggestions for the patch series. It would be
>>> great if you could update this patch series consistent with the
>>> reviewers.
>>>
>>> For eg: in this patch you changed
>>>
>>> +// SPDX-License-Identifier: GPL-2.0+
>>>
>>> and therefore
>>>
>>> MODULE_LICENSE("GPL v2");
>>>
>>> should also be changed to
>>>
>>> MODULE_LICENSE("GPL");
>>>
>>> as explained by Philippe Ombredanne to me in my patch series.
>>
> I'm not sure that was exactly what Philippe was suggesting.
> He was making the point that the licensing was inconsistent without
> saying which option should be chosen.
>
> We will need to seek clarification from Analog Devices
> on what their opinion on this is.
>
> Lars / Michael, any clarification on the right way to resolve this
> inconsistency?
I can't speak for the intended license for code I wasn't involved in.
But I'd in general if there are conflicting licensing information and you
want to be on the safe side choose the more restrictive license. E.g. GPL2+
is compatible with GPL2, but GPL2 is not compatible with GPL2+. So to be
compatible with both choose GPL2.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
2018-02-18 12:08 ` Lars-Peter Clausen
@ 2018-02-18 12:14 ` Lars-Peter Clausen
2018-02-18 12:51 ` Jonathan Cameron
0 siblings, 1 reply; 13+ messages in thread
From: Lars-Peter Clausen @ 2018-02-18 12:14 UTC (permalink / raw)
To: Jonathan Cameron, Shreeya Patel
Cc: Himanshu Jha, Michael.Hennerich, knaack.h, pmeerw, linux-iio,
devel, daniel.baluta
On 02/18/2018 01:08 PM, Lars-Peter Clausen wrote:
> On 02/18/2018 01:02 PM, Jonathan Cameron wrote:
>> On Sun, 18 Feb 2018 17:07:57 +0530
>> Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
>>
>>> On Sun, 2018-02-18 at 17:01 +0530, Himanshu Jha wrote:
>>>> Hi Shreeya,
>>>>
>>> Hi Himanshu,
>>>
>>>> On Sat, Feb 17, 2018 at 09:34:56PM +0530, Shreeya Patel wrote:
>>>>>
>>>>> Use SPDX identifier format instead of GPLv2. Also rearrange the
>>>>> headers in alphabetical order.
>>>>>
>>>>> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
>>>>> ---
>>>>> drivers/staging/iio/accel/adis16209.c | 7 +++----
>>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/drivers/staging/iio/accel/adis16209.c
>>>>> b/drivers/staging/iio/accel/adis16209.c
>>>>> index 7fcef9a..e3d9f80 100644
>>>>> --- a/drivers/staging/iio/accel/adis16209.c
>>>>> +++ b/drivers/staging/iio/accel/adis16209.c
>>>>> @@ -1,19 +1,18 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>> /*
>>>>> * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
>>>>> *
>>>>> * Copyright 2010 Analog Devices Inc.
>>>>> - *
>>>>> - * Licensed under the GPL-2 or later.
>>>> I see that you too are doing similar cleanup which I did a while ago
>>>> https://lkml.org/lkml/2018/2/12/255
>>>
>>> Yes, Jonathan suggested me to work on adis16209.
>>> Your patches were quite useful for me :)
>>>
>>>> where I got some update suggestions for the patch series. It would be
>>>> great if you could update this patch series consistent with the
>>>> reviewers.
>>>>
>>>> For eg: in this patch you changed
>>>>
>>>> +// SPDX-License-Identifier: GPL-2.0+
>>>>
>>>> and therefore
>>>>
>>>> MODULE_LICENSE("GPL v2");
>>>>
>>>> should also be changed to
>>>>
>>>> MODULE_LICENSE("GPL");
>>>>
>>>> as explained by Philippe Ombredanne to me in my patch series.
>>>
>> I'm not sure that was exactly what Philippe was suggesting.
>> He was making the point that the licensing was inconsistent without
>> saying which option should be chosen.
>>
>> We will need to seek clarification from Analog Devices
>> on what their opinion on this is.
>>
>> Lars / Michael, any clarification on the right way to resolve this
>> inconsistency?
>
> I can't speak for the intended license for code I wasn't involved in.
>
> But I'd in general if there are conflicting licensing information and you
> want to be on the safe side choose the more restrictive license. E.g. GPL2+
> is compatible with GPL2, but GPL2 is not compatible with GPL2+. So to be
> compatible with both choose GPL2.
This is not legal advice btw.
I personally would stay away from messing with the licenses of code I do not
own. Not everybody seems to agree yet that a SPDX tag is equivalent to a
explicit licensing statement.
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] Staging: iio: adis16209: Remove unnecessary comments and add suitable suffix
2018-02-17 16:07 ` [PATCH 2/3] Staging: iio: adis16209: Remove unnecessary comments and add suitable suffix Shreeya Patel
@ 2018-02-18 12:44 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2018-02-18 12:44 UTC (permalink / raw)
To: Shreeya Patel
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, linux-iio, devel,
daniel.baluta
On Sat, 17 Feb 2018 21:37:37 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> Remove some unnecessary comments and therefore, add _REG suffix
> for registers. Group the control register and register field
> macros together.
> Also, align function arguments to match open parentheses using
> tabs.
Lots of different changes in here - one patch per change type please.
(so probably 3 here)
A few comments inline.
Jonathan
>
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
> drivers/staging/iio/accel/adis16209.c | 209 +++++++++++-----------------------
> 1 file changed, 69 insertions(+), 140 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c
> index e3d9f80..6101212 100644
> --- a/drivers/staging/iio/accel/adis16209.c
> +++ b/drivers/staging/iio/accel/adis16209.c
> @@ -19,136 +19,67 @@
> #include <linux/iio/buffer.h>
> #include <linux/iio/imu/adis.h>
>
> -#define ADIS16209_STARTUP_DELAY 220 /* ms */
> -
> -/* Flash memory write count */
> -#define ADIS16209_FLASH_CNT 0x00
> -
> -/* Output, power supply */
> -#define ADIS16209_SUPPLY_OUT 0x02
> -
> -/* Output, x-axis accelerometer */
> -#define ADIS16209_XACCL_OUT 0x04
> -
> -/* Output, y-axis accelerometer */
> -#define ADIS16209_YACCL_OUT 0x06
> -
> -/* Output, auxiliary ADC input */
> -#define ADIS16209_AUX_ADC 0x08
> -
> -/* Output, temperature */
> -#define ADIS16209_TEMP_OUT 0x0A
> -
> -/* Output, x-axis inclination */
> -#define ADIS16209_XINCL_OUT 0x0C
> -
> -/* Output, y-axis inclination */
> -#define ADIS16209_YINCL_OUT 0x0E
> -
> -/* Output, +/-180 vertical rotational position */
> -#define ADIS16209_ROT_OUT 0x10
> -
> -/* Calibration, x-axis acceleration offset null */
> -#define ADIS16209_XACCL_NULL 0x12
> -
> -/* Calibration, y-axis acceleration offset null */
> -#define ADIS16209_YACCL_NULL 0x14
> -
> -/* Calibration, x-axis inclination offset null */
> -#define ADIS16209_XINCL_NULL 0x16
> -
> -/* Calibration, y-axis inclination offset null */
> -#define ADIS16209_YINCL_NULL 0x18
> -
> -/* Calibration, vertical rotation offset null */
> -#define ADIS16209_ROT_NULL 0x1A
> -
> -/* Alarm 1 amplitude threshold */
> -#define ADIS16209_ALM_MAG1 0x20
> -
> -/* Alarm 2 amplitude threshold */
> -#define ADIS16209_ALM_MAG2 0x22
> -
> -/* Alarm 1, sample period */
> -#define ADIS16209_ALM_SMPL1 0x24
> -
> -/* Alarm 2, sample period */
> -#define ADIS16209_ALM_SMPL2 0x26
> -
> -/* Alarm control */
> -#define ADIS16209_ALM_CTRL 0x28
> -
> -/* Auxiliary DAC data */
> -#define ADIS16209_AUX_DAC 0x30
> -
> -/* General-purpose digital input/output control */
> -#define ADIS16209_GPIO_CTRL 0x32
> -
> -/* Miscellaneous control */
> -#define ADIS16209_MSC_CTRL 0x34
> -
> -/* Internal sample period (rate) control */
> -#define ADIS16209_SMPL_PRD 0x36
> -
> -/* Operation, filter configuration */
> -#define ADIS16209_AVG_CNT 0x38
> -
> -/* Operation, sleep mode control */
> -#define ADIS16209_SLP_CNT 0x3A
> -
> -/* Diagnostics, system status register */
> -#define ADIS16209_DIAG_STAT 0x3C
> -
> -/* Operation, system command register */
> -#define ADIS16209_GLOB_CMD 0x3E
> +#define ADIS16209_STARTUP_DELAY_MS 220
This one is a different type of change to the others,
I would break it out in another patch.
> +#define ADIS16209_FLASH_CNT_REG 0x00
> +
> +/* Data Output Register Definitions */
> +#define ADIS16209_SUPPLY_OUT_REG 0x02
> +#define ADIS16209_XACCL_OUT_REG 0x04
> +#define ADIS16209_YACCL_OUT_REG 0x06
> +#define ADIS16209_AUX_ADC_REG 0x08
> +#define ADIS16209_TEMP_OUT_REG 0x0A
> +#define ADIS16209_XINCL_OUT_REG 0x0C
For these last 3 I'm not sure that it is obvious what they are - I had
to look at the datasheet. Perhaps a comment block explaining how
these relate to the orientation of the device would be a useful addition.
This may be a little 'controversial' so would be good to have
this clearly laid out.
> +#define ADIS16209_YINCL_OUT_REG 0x0E
> +#define ADIS16209_ROT_OUT_REG 0x10
> +
> +/* Calibration Register Definitions */
> +#define ADIS16209_XACCL_NULL_REG 0x12
> +#define ADIS16209_YACCL_NULL_REG 0x14
> +#define ADIS16209_XINCL_NULL_REG 0x16
> +#define ADIS16209_YINCL_NULL_REG 0x18
> +#define ADIS16209_ROT_NULL_REG 0x1A
This group are not necessarily clear in meaning. I would explain
in the comment what is meant by NULL in this case.
> +
> +/* Alarm Register Definitions */
> +#define ADIS16209_ALM_MAG1_REG 0x20
> +#define ADIS16209_ALM_MAG2_REG 0x22
> +#define ADIS16209_ALM_SMPL1_REG 0x24
> +#define ADIS16209_ALM_SMPL2_REG 0x26
> +#define ADIS16209_ALM_CTRL_REG 0x28
Hmm. This is an interesting trade off. There isn't actually enough defined
here to allow implementation of the ALM functionality (and I wouldn't suggest
trying to do so without hardware access) - so does having this set of defines
provide any useful information? I'm not sure I care either way!
> +
> +#define ADIS16209_AUX_DAC_REG 0x30
> +#define ADIS16209_GPIO_CTRL_REG 0x32
> +#define ADIS16209_SMPL_PRD_REG 0x36
> +#define ADIS16209_AVG_CNT_REG 0x38
> +#define ADIS16209_SLP_CNT_REG 0x3A
>
> /* MSC_CTRL */
This one is obvious given the register name.
>
> -/* Self-test at power-on: 1 = disabled, 0 = enabled */
> +#define ADIS16209_MSC_CTRL_REG 0x34
> #define ADIS16209_MSC_CTRL_PWRUP_SELF_TEST BIT(10)
> -
> -/* Self-test enable */
> -#define ADIS16209_MSC_CTRL_SELF_TEST_EN BIT(8)
> -
> -/* Data-ready enable: 1 = enabled, 0 = disabled */
> -#define ADIS16209_MSC_CTRL_DATA_RDY_EN BIT(2)
> -
> -/* Data-ready polarity: 1 = active high, 0 = active low */
> -#define ADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1)
> -
> -/* Data-ready line selection: 1 = DIO2, 0 = DIO1 */
> -#define ADIS16209_MSC_CTRL_DATA_RDY_DIO2 BIT(0)
> +#define ADIS16209_MSC_CTRL_SELF_TEST_EN BIT(8)
One comment trick that I think improves readability is to
slightly indent the field definitions to make them clearly
different from the register addresses.
#define ADIS16209_MSC_CTRL_REG 0x34
#define ADIS16209_MSC_CTRL_PWRUP_SELF_TEST BIT(10)
#define ADIS16209_MSC_CTRL_SELF_TEST_EN ...
There are other ways of doing this but I think this one is subtle
but effective.
> +#define ADIS16209_MSC_CTRL_DATA_RDY_EN BIT(2)
> +#define ADIS16209_MSC_CTRL_ACTIVE_HIGH BIT(1)
> +#define ADIS16209_MSC_CTRL_DATA_RDY_DIO1 BIT(0)
>
> /* DIAG_STAT */
Again, the register name makes this clear so no need for the comment.
Just use a blank line to separate this block from previous.
>
> -/* Alarm 2 status: 1 = alarm active, 0 = alarm inactive */
> -#define ADIS16209_DIAG_STAT_ALARM2 BIT(9)
> -
> -/* Alarm 1 status: 1 = alarm active, 0 = alarm inactive */
> -#define ADIS16209_DIAG_STAT_ALARM1 BIT(8)
> -
> -/* Self-test diagnostic error flag: 1 = error condition, 0 = normal operation */
> +#define ADIS16209_DIAG_STAT_REG 0x3C
> +#define ADIS16209_DIAG_STAT_ALARM2 BIT(9)
> +#define ADIS16209_DIAG_STAT_ALARM1 BIT(8)
> #define ADIS16209_DIAG_STAT_SELFTEST_FAIL_BIT 5
> -
> -/* SPI communications failure */
> #define ADIS16209_DIAG_STAT_SPI_FAIL_BIT 3
> -
> -/* Flash update failure */
> #define ADIS16209_DIAG_STAT_FLASH_UPT_BIT 2
> -
> -/* Power supply above 3.625 V */
> #define ADIS16209_DIAG_STAT_POWER_HIGH_BIT 1
> -
> -/* Power supply below 3.15 V */
> #define ADIS16209_DIAG_STAT_POWER_LOW_BIT 0
>
> /* GLOB_CMD */
Again, the register name makes this clear. It's not
'special' compared to the other registers so doesn't need
an additional comment.
>
> -#define ADIS16209_GLOB_CMD_SW_RESET BIT(7)
> -#define ADIS16209_GLOB_CMD_CLEAR_STAT BIT(4)
> -#define ADIS16209_GLOB_CMD_FACTORY_CAL BIT(1)
> +#define ADIS16209_GLOB_CMD_REG 0x3E
> +#define ADIS16209_GLOB_CMD_SW_RESET BIT(7)
> +#define ADIS16209_GLOB_CMD_CLEAR_STAT BIT(4)
> +#define ADIS16209_GLOB_CMD_FACTORY_CAL BIT(1)
>
> -#define ADIS16209_ERROR_ACTIVE BIT(14)
> +#define ADIS16209_ERROR_ACTIVE BIT(14)
>
> enum adis16209_scan {
> ADIS16209_SCAN_SUPPLY,
> @@ -164,10 +95,10 @@ enum adis16209_scan {
> static const u8 adis16209_addresses[8][1] = {
> [ADIS16209_SCAN_SUPPLY] = { },
> [ADIS16209_SCAN_AUX_ADC] = { },
> - [ADIS16209_SCAN_ACC_X] = { ADIS16209_XACCL_NULL },
> - [ADIS16209_SCAN_ACC_Y] = { ADIS16209_YACCL_NULL },
> - [ADIS16209_SCAN_INCLI_X] = { ADIS16209_XINCL_NULL },
> - [ADIS16209_SCAN_INCLI_Y] = { ADIS16209_YINCL_NULL },
> + [ADIS16209_SCAN_ACC_X] = { ADIS16209_XACCL_NULL_REG },
> + [ADIS16209_SCAN_ACC_Y] = { ADIS16209_YACCL_NULL_REG },
> + [ADIS16209_SCAN_INCLI_X] = { ADIS16209_XINCL_NULL_REG },
> + [ADIS16209_SCAN_INCLI_Y] = { ADIS16209_YINCL_NULL_REG },
> [ADIS16209_SCAN_ROT] = { },
> [ADIS16209_SCAN_TEMP] = { },
> };
> @@ -214,35 +145,35 @@ static int adis16209_read_raw(struct iio_dev *indio_dev,
> switch (mask) {
> case IIO_CHAN_INFO_RAW:
> return adis_single_conversion(indio_dev, chan,
> - ADIS16209_ERROR_ACTIVE, val);
> + ADIS16209_ERROR_ACTIVE, val);
This is a different type of change. Should be in a different patch.
> case IIO_CHAN_INFO_SCALE:
> switch (chan->type) {
> case IIO_VOLTAGE:
> *val = 0;
> if (chan->channel == 0)
> - *val2 = 305180; /* 0.30518 mV */
Again, I think Dan was right in his earlier review in that these
do provide some value, but I think it can be improved by
adding fuller comments.
> + *val2 = 305180;
> else
> - *val2 = 610500; /* 0.6105 mV */
> + *val2 = 610500;
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_TEMP:
> - *val = -470; /* -0.47 C */
> + *val = -470;
> *val2 = 0;
This one is interesting...
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_ACCEL:
> *val = 0;
> - *val2 = IIO_G_TO_M_S_2(244140); /* 0.244140 mg */
> + *val2 = IIO_G_TO_M_S_2(244140);
As below, perhaps what we really want is a useful comment rather than
no comment.
> return IIO_VAL_INT_PLUS_NANO;
> case IIO_INCLI:
> case IIO_ROT:
> *val = 0;
> - *val2 = 25000; /* 0.025 degree */
Dan commented on these and I think he is correct in that it
is useful to have some description here of what is going on.
However, I'd like something more along the lines of:
/*
* IIO base units for rotation are degrees.
* 1 LSB represents 0.025 milli degrees.
*/
Or more interestingly does it given that I just checked
our ABI docs
Documentation/ABI/testing/sysfs-bus-iio and
rot_raw is not defined?
Would be great to:
1) Check what the various drivers with rotation channels
are actually using.
2) Document it properly.
> + *val2 = 25000;
> return IIO_VAL_INT_PLUS_MICRO;
> default:
> return -EINVAL;
> }
> break;
> case IIO_CHAN_INFO_OFFSET:
> - *val = 25000 / -470 - 0x4FE; /* 25 C = 0x4FE */
> + *val = 25000 / -470 - 0x4FE;
On this one I think it is sufficiently non obvious that we want
to go the other way and explain 'why' the offset is as described.
The comment is not that much help as it stands.
It is all very well knowing it should be 0x45e at 25 degrees but
where did that scale factor of -470 come from?
> return IIO_VAL_INT;
> case IIO_CHAN_INFO_CALIBBIAS:
> switch (chan->type) {
> @@ -265,18 +196,19 @@ static int adis16209_read_raw(struct iio_dev *indio_dev,
> }
>
> static const struct iio_chan_spec adis16209_channels[] = {
> - ADIS_SUPPLY_CHAN(ADIS16209_SUPPLY_OUT, ADIS16209_SCAN_SUPPLY, 0, 14),
> - ADIS_TEMP_CHAN(ADIS16209_TEMP_OUT, ADIS16209_SCAN_TEMP, 0, 12),
> - ADIS_ACCEL_CHAN(X, ADIS16209_XACCL_OUT, ADIS16209_SCAN_ACC_X,
> + ADIS_SUPPLY_CHAN(ADIS16209_SUPPLY_OUT_REG, ADIS16209_SCAN_SUPPLY,
> + 0, 14),
> + ADIS_TEMP_CHAN(ADIS16209_TEMP_OUT_REG, ADIS16209_SCAN_TEMP, 0, 12),
> + ADIS_ACCEL_CHAN(X, ADIS16209_XACCL_OUT_REG, ADIS16209_SCAN_ACC_X,
> BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
> - ADIS_ACCEL_CHAN(Y, ADIS16209_YACCL_OUT, ADIS16209_SCAN_ACC_Y,
> + ADIS_ACCEL_CHAN(Y, ADIS16209_YACCL_OUT_REG, ADIS16209_SCAN_ACC_Y,
> BIT(IIO_CHAN_INFO_CALIBBIAS), 0, 14),
> - ADIS_AUX_ADC_CHAN(ADIS16209_AUX_ADC, ADIS16209_SCAN_AUX_ADC, 0, 12),
> - ADIS_INCLI_CHAN(X, ADIS16209_XINCL_OUT, ADIS16209_SCAN_INCLI_X,
> + ADIS_AUX_ADC_CHAN(ADIS16209_AUX_ADC_REG, ADIS16209_SCAN_AUX_ADC, 0, 12),
> + ADIS_INCLI_CHAN(X, ADIS16209_XINCL_OUT_REG, ADIS16209_SCAN_INCLI_X,
> 0, 0, 14),
> - ADIS_INCLI_CHAN(Y, ADIS16209_YINCL_OUT, ADIS16209_SCAN_INCLI_Y,
> + ADIS_INCLI_CHAN(Y, ADIS16209_YINCL_OUT_REG, ADIS16209_SCAN_INCLI_Y,
> 0, 0, 14),
> - ADIS_ROT_CHAN(X, ADIS16209_ROT_OUT, ADIS16209_SCAN_ROT, 0, 0, 14),
> + ADIS_ROT_CHAN(X, ADIS16209_ROT_OUT_REG, ADIS16209_SCAN_ROT, 0, 0, 14),
> IIO_CHAN_SOFT_TIMESTAMP(8)
> };
>
> @@ -296,13 +228,13 @@ static const char * const adis16209_status_error_msgs[] = {
>
> static const struct adis_data adis16209_data = {
> .read_delay = 30,
> - .msc_ctrl_reg = ADIS16209_MSC_CTRL,
> - .glob_cmd_reg = ADIS16209_GLOB_CMD,
> - .diag_stat_reg = ADIS16209_DIAG_STAT,
> + .msc_ctrl_reg = ADIS16209_MSC_CTRL_REG,
> + .glob_cmd_reg = ADIS16209_GLOB_CMD_REG,
> + .diag_stat_reg = ADIS16209_DIAG_STAT_REG,
>
> .self_test_mask = ADIS16209_MSC_CTRL_SELF_TEST_EN,
> .self_test_no_autoclear = true,
> - .startup_delay = ADIS16209_STARTUP_DELAY,
> + .startup_delay = ADIS16209_STARTUP_DELAY_MS,
>
> .status_error_msgs = adis16209_status_error_msgs,
> .status_error_mask = BIT(ADIS16209_DIAG_STAT_SELFTEST_FAIL_BIT) |
> @@ -318,12 +250,10 @@ static int adis16209_probe(struct spi_device *spi)
> struct adis *st;
> struct iio_dev *indio_dev;
>
> - /* setup the industrialio driver allocated elements */
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> if (!indio_dev)
> return -ENOMEM;
> st = iio_priv(indio_dev);
> - /* this is only used for removal purposes */
> spi_set_drvdata(spi, indio_dev);
>
> indio_dev->name = spi->dev.driver->name;
> @@ -340,7 +270,6 @@ static int adis16209_probe(struct spi_device *spi)
> if (ret)
> return ret;
>
> - /* Get the device into a sane initial state */
> ret = adis_initial_startup(st);
> if (ret)
> goto error_cleanup_buffer_trigger;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 3/3] Staging: iio: adis16209: Use sign_extend32 and adjust a switch statement
2018-02-17 16:21 ` [PATCH 3/3] Staging: iio: adis16209: Use sign_extend32 and adjust a switch statement Shreeya Patel
@ 2018-02-18 12:47 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2018-02-18 12:47 UTC (permalink / raw)
To: Shreeya Patel
Cc: lars, Michael.Hennerich, knaack.h, pmeerw, linux-iio, devel,
daniel.baluta
On Sat, 17 Feb 2018 21:51:35 +0530
Shreeya Patel <shreeya.patel23498@gmail.com> 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.
Two items, two patches please.
The -EINVAL is a bit of an odd one. We know it can never happen and
in reality we only add it to suppress the static checker warnings
that may otherwise ensue. I would definitely not say that it helps
readability. The change to the switch statement makes it clear there
are only two channels so is semantically clearer to my mind and it is
that which I argued improved readability.
Jonathan
>
> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> ---
> drivers/staging/iio/accel/adis16209.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/staging/iio/accel/adis16209.c b/drivers/staging/iio/accel/adis16209.c
> index 6101212..f7c228b 100644
> --- a/drivers/staging/iio/accel/adis16209.c
> +++ b/drivers/staging/iio/accel/adis16209.c
> @@ -150,10 +150,16 @@ static int adis16209_read_raw(struct iio_dev *indio_dev,
> switch (chan->type) {
> case IIO_VOLTAGE:
> *val = 0;
> - if (chan->channel == 0)
> + switch (chan->channel) {
> + case 0:
> *val2 = 305180;
> - else
> + break;
> + case 1:
> *val2 = 610500;
> + break;
> + default:
> + return -EINVAL;
> + }
> return IIO_VAL_INT_PLUS_MICRO;
> case IIO_TEMP:
> *val = -470;
> @@ -187,9 +193,8 @@ static int adis16209_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;
> +
> + *val = sign_extend32(val16, bits - 1);
> return IIO_VAL_INT;
> }
> return -EINVAL;
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier
2018-02-18 12:14 ` Lars-Peter Clausen
@ 2018-02-18 12:51 ` Jonathan Cameron
0 siblings, 0 replies; 13+ messages in thread
From: Jonathan Cameron @ 2018-02-18 12:51 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Shreeya Patel, Himanshu Jha, Michael.Hennerich, knaack.h, pmeerw,
linux-iio, devel, daniel.baluta, Himanshu Jha
On Sun, 18 Feb 2018 13:14:27 +0100
Lars-Peter Clausen <lars@metafoo.de> wrote:
> On 02/18/2018 01:08 PM, Lars-Peter Clausen wrote:
> > On 02/18/2018 01:02 PM, Jonathan Cameron wrote:
> >> On Sun, 18 Feb 2018 17:07:57 +0530
> >> Shreeya Patel <shreeya.patel23498@gmail.com> wrote:
> >>
> >>> On Sun, 2018-02-18 at 17:01 +0530, Himanshu Jha wrote:
> >>>> Hi Shreeya,
> >>>>
> >>> Hi Himanshu,
> >>>
> >>>> On Sat, Feb 17, 2018 at 09:34:56PM +0530, Shreeya Patel wrote:
> >>>>>
> >>>>> Use SPDX identifier format instead of GPLv2. Also rearrange the
> >>>>> headers in alphabetical order.
> >>>>>
> >>>>> Signed-off-by: Shreeya Patel <shreeya.patel23498@gmail.com>
> >>>>> ---
> >>>>> drivers/staging/iio/accel/adis16209.c | 7 +++----
> >>>>> 1 file changed, 3 insertions(+), 4 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/staging/iio/accel/adis16209.c
> >>>>> b/drivers/staging/iio/accel/adis16209.c
> >>>>> index 7fcef9a..e3d9f80 100644
> >>>>> --- a/drivers/staging/iio/accel/adis16209.c
> >>>>> +++ b/drivers/staging/iio/accel/adis16209.c
> >>>>> @@ -1,19 +1,18 @@
> >>>>> +// SPDX-License-Identifier: GPL-2.0+
> >>>>> /*
> >>>>> * ADIS16209 Dual-Axis Digital Inclinometer and Accelerometer
> >>>>> *
> >>>>> * Copyright 2010 Analog Devices Inc.
> >>>>> - *
> >>>>> - * Licensed under the GPL-2 or later.
> >>>> I see that you too are doing similar cleanup which I did a while ago
> >>>> https://lkml.org/lkml/2018/2/12/255
> >>>
> >>> Yes, Jonathan suggested me to work on adis16209.
> >>> Your patches were quite useful for me :)
> >>>
> >>>> where I got some update suggestions for the patch series. It would be
> >>>> great if you could update this patch series consistent with the
> >>>> reviewers.
> >>>>
> >>>> For eg: in this patch you changed
> >>>>
> >>>> +// SPDX-License-Identifier: GPL-2.0+
> >>>>
> >>>> and therefore
> >>>>
> >>>> MODULE_LICENSE("GPL v2");
> >>>>
> >>>> should also be changed to
> >>>>
> >>>> MODULE_LICENSE("GPL");
> >>>>
> >>>> as explained by Philippe Ombredanne to me in my patch series.
> >>>
> >> I'm not sure that was exactly what Philippe was suggesting.
> >> He was making the point that the licensing was inconsistent without
> >> saying which option should be chosen.
> >>
> >> We will need to seek clarification from Analog Devices
> >> on what their opinion on this is.
> >>
> >> Lars / Michael, any clarification on the right way to resolve this
> >> inconsistency?
> >
> > I can't speak for the intended license for code I wasn't involved in.
> >
> > But I'd in general if there are conflicting licensing information and you
> > want to be on the safe side choose the more restrictive license. E.g. GPL2+
> > is compatible with GPL2, but GPL2 is not compatible with GPL2+. So to be
> > compatible with both choose GPL2.
>
> This is not legal advice btw.
>
> I personally would stay away from messing with the licenses of code I do not
> own. Not everybody seems to agree yet that a SPDX tag is equivalent to a
> explicit licensing statement.
>
OK, in that case let's not do the spdx conversions for these drivers.
We probably need to fix the inconsistency however between the text and the
module license tag. Doesn't need to be part of moving it out of staging however.
Jonathan
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-02-18 12:51 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-17 16:02 [PATCH 0/3] adis16209 driver cleanup Shreeya Patel
2018-02-17 16:04 ` [PATCH 1/3] Staging: iio: adis16209: Use SPDX identifier Shreeya Patel
2018-02-18 11:31 ` Himanshu Jha
2018-02-18 11:37 ` Shreeya Patel
2018-02-18 12:02 ` Jonathan Cameron
2018-02-18 12:08 ` Lars-Peter Clausen
2018-02-18 12:14 ` Lars-Peter Clausen
2018-02-18 12:51 ` Jonathan Cameron
2018-02-18 12:04 ` Jonathan Cameron
2018-02-17 16:07 ` [PATCH 2/3] Staging: iio: adis16209: Remove unnecessary comments and add suitable suffix Shreeya Patel
2018-02-18 12:44 ` Jonathan Cameron
2018-02-17 16:21 ` [PATCH 3/3] Staging: iio: adis16209: Use sign_extend32 and adjust a switch statement Shreeya Patel
2018-02-18 12:47 ` Jonathan Cameron
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.