* [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
* 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 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 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 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 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
* [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
* 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 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 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
* [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 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 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 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 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 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 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
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.