| Message ID | 1518436499-8584-3-git-send-email-himanshujha199640@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
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(-)