This patchset solves readability issues in AD7150 code, such as clarify
register and mask definition, fashion improvement of mask uses, reduce
tedious operation and useless comments.
Melissa Wen (4):
staging: iio: ad7150: organize registers definition
staging: iio: ad7150: use FIELD_GET and GENMASK
staging: iio: ad7150: simplify i2c SMBus return treatment
staging: iio: ad7150: clean up of comments
drivers/staging/iio/cdc/ad7150.c | 102 ++++++++++++++-----------------
1 file changed, 47 insertions(+), 55 deletions(-)
--
2.20.1
Use the suffix REG to make the register addresses clear
and indentation to highlight field names.
Signed-off-by: Melissa Wen <[email protected]>
---
drivers/staging/iio/cdc/ad7150.c | 75 ++++++++++++++++----------------
1 file changed, 37 insertions(+), 38 deletions(-)
diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index dd7fcab8e19e..24601ba7db88 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -15,35 +15,34 @@
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.h>
#include <linux/iio/events.h>
-/*
- * AD7150 registers definition
- */
-#define AD7150_STATUS 0
-#define AD7150_STATUS_OUT1 BIT(3)
-#define AD7150_STATUS_OUT2 BIT(5)
-#define AD7150_CH1_DATA_HIGH 1
-#define AD7150_CH2_DATA_HIGH 3
-#define AD7150_CH1_AVG_HIGH 5
-#define AD7150_CH2_AVG_HIGH 7
-#define AD7150_CH1_SENSITIVITY 9
-#define AD7150_CH1_THR_HOLD_H 9
-#define AD7150_CH1_TIMEOUT 10
-#define AD7150_CH1_SETUP 11
-#define AD7150_CH2_SENSITIVITY 12
-#define AD7150_CH2_THR_HOLD_H 12
-#define AD7150_CH2_TIMEOUT 13
-#define AD7150_CH2_SETUP 14
-#define AD7150_CFG 15
-#define AD7150_CFG_FIX BIT(7)
-#define AD7150_PD_TIMER 16
-#define AD7150_CH1_CAPDAC 17
-#define AD7150_CH2_CAPDAC 18
-#define AD7150_SN3 19
-#define AD7150_SN2 20
-#define AD7150_SN1 21
-#define AD7150_SN0 22
-#define AD7150_ID 23
+/* AD7150 registers */
+
+#define AD7150_STATUS_REG 0x00
+#define AD7150_STATUS_OUT1 BIT(3)
+#define AD7150_STATUS_OUT2 BIT(5)
+#define AD7150_CH1_DATA_HIGH_REG 0x01
+#define AD7150_CH2_DATA_HIGH_REG 0x03
+#define AD7150_CH1_AVG_HIGH_REG 0x05
+#define AD7150_CH2_AVG_HIGH_REG 0x07
+#define AD7150_CH1_SENSITIVITY_REG 0x09
+#define AD7150_CH1_THR_HOLD_H_REG 0x09
+#define AD7150_CH2_SENSITIVITY_REG 0x0C
+#define AD7150_CH1_TIMEOUT_REG 0x0A
+#define AD7150_CH1_SETUP_REG 0x0B
+#define AD7150_CH2_THR_HOLD_H_REG 0x0C
+#define AD7150_CH2_TIMEOUT_REG 0x0D
+#define AD7150_CH2_SETUP_REG 0x0E
+#define AD7150_CFG_REG 0x0F
+#define AD7150_CFG_FIX BIT(7)
+#define AD7150_PD_TIMER_REG 0x10
+#define AD7150_CH1_CAPDAC_REG 0x11
+#define AD7150_CH2_CAPDAC_REG 0x12
+#define AD7150_SN3_REG 0x13
+#define AD7150_SN2_REG 0x14
+#define AD7150_SN1_REG 0x15
+#define AD7150_SN0_REG 0x16
+#define AD7150_ID_REG 0x17
/**
* struct ad7150_chip_info - instance specific chip data
@@ -85,12 +84,12 @@ struct ad7150_chip_info {
*/
static const u8 ad7150_addresses[][6] = {
- { AD7150_CH1_DATA_HIGH, AD7150_CH1_AVG_HIGH,
- AD7150_CH1_SETUP, AD7150_CH1_THR_HOLD_H,
- AD7150_CH1_SENSITIVITY, AD7150_CH1_TIMEOUT },
- { AD7150_CH2_DATA_HIGH, AD7150_CH2_AVG_HIGH,
- AD7150_CH2_SETUP, AD7150_CH2_THR_HOLD_H,
- AD7150_CH2_SENSITIVITY, AD7150_CH2_TIMEOUT },
+ { AD7150_CH1_DATA_HIGH_REG, AD7150_CH1_AVG_HIGH_REG,
+ AD7150_CH1_SETUP_REG, AD7150_CH1_THR_HOLD_H_REG,
+ AD7150_CH1_SENSITIVITY_REG, AD7150_CH1_TIMEOUT_REG },
+ { AD7150_CH2_DATA_HIGH_REG, AD7150_CH2_AVG_HIGH_REG,
+ AD7150_CH2_SETUP_REG, AD7150_CH2_THR_HOLD_H_REG,
+ AD7150_CH2_SENSITIVITY_REG, AD7150_CH2_TIMEOUT_REG },
};
static int ad7150_read_raw(struct iio_dev *indio_dev,
@@ -133,7 +132,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
bool adaptive;
struct ad7150_chip_info *chip = iio_priv(indio_dev);
- ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
+ ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
if (ret < 0)
return ret;
@@ -229,7 +228,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
if (event_code == chip->current_event)
return 0;
mutex_lock(&chip->state_lock);
- ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
+ ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
if (ret < 0)
goto error_ret;
@@ -264,7 +263,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
cfg |= (!adaptive << 7) | (thresh_type << 5);
- ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
+ ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG_REG, cfg);
if (ret < 0)
goto error_ret;
@@ -497,7 +496,7 @@ static irqreturn_t ad7150_event_handler(int irq, void *private)
s64 timestamp = iio_get_time_ns(indio_dev);
int ret;
- ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
+ ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS_REG);
if (ret < 0)
return IRQ_HANDLED;
--
2.20.1
Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask in
one go. This makes the code more readable than explicit masking followed
by a shift.
Signed-off-by: Melissa Wen <[email protected]>
---
drivers/staging/iio/cdc/ad7150.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 24601ba7db88..4ba46fb6ac02 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -5,6 +5,7 @@
* Copyright 2010-2011 Analog Devices Inc.
*/
+#include <linux/bitfield.h>
#include <linux/interrupt.h>
#include <linux/device.h>
#include <linux/kernel.h>
@@ -44,6 +45,9 @@
#define AD7150_SN0_REG 0x16
#define AD7150_ID_REG 0x17
+/* AD7150 masks */
+#define AD7150_THRESHTYPE_MSK GENMASK(6, 5)
+
/**
* struct ad7150_chip_info - instance specific chip data
* @client: i2c client for this device
@@ -136,7 +140,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
if (ret < 0)
return ret;
- threshtype = (ret >> 5) & 0x03;
+ threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
adaptive = !!(ret & 0x80);
switch (type) {
--
2.20.1
Since i2c_smbus_write_byte_data returns no-positive value, this commit
making the treatment of its return value less verbose.
Signed-off-by: Melissa Wen <[email protected]>
---
drivers/staging/iio/cdc/ad7150.c | 10 +++-------
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 4ba46fb6ac02..3a4572a9e5ec 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -201,16 +201,12 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
ret = i2c_smbus_write_byte_data(chip->client,
ad7150_addresses[chan][4],
sens);
- if (ret < 0)
+ if (ret)
return ret;
-
- ret = i2c_smbus_write_byte_data(chip->client,
+ else
+ return i2c_smbus_write_byte_data(chip->client,
ad7150_addresses[chan][5],
timeout);
- if (ret < 0)
- return ret;
-
- return 0;
}
static int ad7150_write_event_config(struct iio_dev *indio_dev,
--
2.20.1
General cleaning of comments to remove useless information or improve
description.
Signed-off-by: Melissa Wen <[email protected]>
---
drivers/staging/iio/cdc/ad7150.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
index 3a4572a9e5ec..775818b0761e 100644
--- a/drivers/staging/iio/cdc/ad7150.c
+++ b/drivers/staging/iio/cdc/ad7150.c
@@ -162,7 +162,8 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
return -EINVAL;
}
-/* lock should be held */
+/* state_lock should be held to ensure consistent state*/
+
static int ad7150_write_event_params(struct iio_dev *indio_dev,
unsigned int chan,
enum iio_event_type type,
@@ -484,10 +485,6 @@ static const struct iio_chan_spec ad7150_channels[] = {
},
};
-/*
- * threshold events
- */
-
static irqreturn_t ad7150_event_handler(int irq, void *private)
{
struct iio_dev *indio_dev = private;
@@ -576,10 +573,6 @@ static const struct iio_info ad7150_info = {
.write_event_value = &ad7150_write_event_value,
};
-/*
- * device probe and remove
- */
-
static int ad7150_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{
--
2.20.1
On Sat, May 4, 2019 at 1:25 AM Melissa Wen <[email protected]> wrote:
>
> Use the suffix REG to make the register addresses clear
> and indentation to highlight field names.
>
I'm inclined to say that this change is a bit too much noise versus added value.
While the REG suffix does make sense (generally), since it hasn't been
added from the beginning, it doesn't make much sense to add it now.
It is sufficiently clear (as-is) that these macros refer to registers.
They should be easy to match with the datasheet as well.
> Signed-off-by: Melissa Wen <[email protected]>
> ---
> drivers/staging/iio/cdc/ad7150.c | 75 ++++++++++++++++----------------
> 1 file changed, 37 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index dd7fcab8e19e..24601ba7db88 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -15,35 +15,34 @@
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> #include <linux/iio/events.h>
> -/*
> - * AD7150 registers definition
> - */
>
> -#define AD7150_STATUS 0
> -#define AD7150_STATUS_OUT1 BIT(3)
> -#define AD7150_STATUS_OUT2 BIT(5)
> -#define AD7150_CH1_DATA_HIGH 1
> -#define AD7150_CH2_DATA_HIGH 3
> -#define AD7150_CH1_AVG_HIGH 5
> -#define AD7150_CH2_AVG_HIGH 7
> -#define AD7150_CH1_SENSITIVITY 9
> -#define AD7150_CH1_THR_HOLD_H 9
> -#define AD7150_CH1_TIMEOUT 10
> -#define AD7150_CH1_SETUP 11
> -#define AD7150_CH2_SENSITIVITY 12
> -#define AD7150_CH2_THR_HOLD_H 12
> -#define AD7150_CH2_TIMEOUT 13
> -#define AD7150_CH2_SETUP 14
> -#define AD7150_CFG 15
> -#define AD7150_CFG_FIX BIT(7)
> -#define AD7150_PD_TIMER 16
> -#define AD7150_CH1_CAPDAC 17
> -#define AD7150_CH2_CAPDAC 18
> -#define AD7150_SN3 19
> -#define AD7150_SN2 20
> -#define AD7150_SN1 21
> -#define AD7150_SN0 22
> -#define AD7150_ID 23
> +/* AD7150 registers */
> +
> +#define AD7150_STATUS_REG 0x00
> +#define AD7150_STATUS_OUT1 BIT(3)
> +#define AD7150_STATUS_OUT2 BIT(5)
> +#define AD7150_CH1_DATA_HIGH_REG 0x01
> +#define AD7150_CH2_DATA_HIGH_REG 0x03
> +#define AD7150_CH1_AVG_HIGH_REG 0x05
> +#define AD7150_CH2_AVG_HIGH_REG 0x07
> +#define AD7150_CH1_SENSITIVITY_REG 0x09
> +#define AD7150_CH1_THR_HOLD_H_REG 0x09
> +#define AD7150_CH2_SENSITIVITY_REG 0x0C
> +#define AD7150_CH1_TIMEOUT_REG 0x0A
> +#define AD7150_CH1_SETUP_REG 0x0B
> +#define AD7150_CH2_THR_HOLD_H_REG 0x0C
> +#define AD7150_CH2_TIMEOUT_REG 0x0D
> +#define AD7150_CH2_SETUP_REG 0x0E
> +#define AD7150_CFG_REG 0x0F
> +#define AD7150_CFG_FIX BIT(7)
> +#define AD7150_PD_TIMER_REG 0x10
> +#define AD7150_CH1_CAPDAC_REG 0x11
> +#define AD7150_CH2_CAPDAC_REG 0x12
> +#define AD7150_SN3_REG 0x13
> +#define AD7150_SN2_REG 0x14
> +#define AD7150_SN1_REG 0x15
> +#define AD7150_SN0_REG 0x16
> +#define AD7150_ID_REG 0x17
>
> /**
> * struct ad7150_chip_info - instance specific chip data
> @@ -85,12 +84,12 @@ struct ad7150_chip_info {
> */
>
> static const u8 ad7150_addresses[][6] = {
> - { AD7150_CH1_DATA_HIGH, AD7150_CH1_AVG_HIGH,
> - AD7150_CH1_SETUP, AD7150_CH1_THR_HOLD_H,
> - AD7150_CH1_SENSITIVITY, AD7150_CH1_TIMEOUT },
> - { AD7150_CH2_DATA_HIGH, AD7150_CH2_AVG_HIGH,
> - AD7150_CH2_SETUP, AD7150_CH2_THR_HOLD_H,
> - AD7150_CH2_SENSITIVITY, AD7150_CH2_TIMEOUT },
> + { AD7150_CH1_DATA_HIGH_REG, AD7150_CH1_AVG_HIGH_REG,
> + AD7150_CH1_SETUP_REG, AD7150_CH1_THR_HOLD_H_REG,
> + AD7150_CH1_SENSITIVITY_REG, AD7150_CH1_TIMEOUT_REG },
> + { AD7150_CH2_DATA_HIGH_REG, AD7150_CH2_AVG_HIGH_REG,
> + AD7150_CH2_SETUP_REG, AD7150_CH2_THR_HOLD_H_REG,
> + AD7150_CH2_SENSITIVITY_REG, AD7150_CH2_TIMEOUT_REG },
> };
>
> static int ad7150_read_raw(struct iio_dev *indio_dev,
> @@ -133,7 +132,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
> bool adaptive;
> struct ad7150_chip_info *chip = iio_priv(indio_dev);
>
> - ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> + ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
> if (ret < 0)
> return ret;
>
> @@ -229,7 +228,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
> if (event_code == chip->current_event)
> return 0;
> mutex_lock(&chip->state_lock);
> - ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> + ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
> if (ret < 0)
> goto error_ret;
>
> @@ -264,7 +263,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
>
> cfg |= (!adaptive << 7) | (thresh_type << 5);
>
> - ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
> + ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG_REG, cfg);
> if (ret < 0)
> goto error_ret;
>
> @@ -497,7 +496,7 @@ static irqreturn_t ad7150_event_handler(int irq, void *private)
> s64 timestamp = iio_get_time_ns(indio_dev);
> int ret;
>
> - ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
> + ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS_REG);
> if (ret < 0)
> return IRQ_HANDLED;
>
> --
> 2.20.1
>
On Sat, May 4, 2019 at 1:24 AM Melissa Wen <[email protected]> wrote:
>
> This patchset solves readability issues in AD7150 code, such as clarify
> register and mask definition, fashion improvement of mask uses, reduce
> tedious operation and useless comments.
>
Hey,
Two patches seem a bit noisy/un-needed.
The other 2 are fine from me.
This driver does need some work to move it out of staging.
I am not sure what would be a big blocker for it, other than maybe it
needs a device-tree binding doc (in YAML format).
Maybe Jonathan remembers.
Some other low-hanging-fruit ideas would be:
1) remove the code for platform_data ; that one seems forgotten from
some other time; the interrupts should be coming from device-tree,
from the i2c bindings
2) you could do a AD7150_EVENT_SPEC() macro (similar to
AD7150_TIMEOUT() macro) and use it in the ad7150_events[] list; that
would reduce a few lines
3) similar to 2), you could do a AD7150_CHANNEL(x) macro ;
4) in ad7150_event_handler() the checks could be wrapped into a macro,
or maybe some function ; i am referring to "(int_status &
AD7150_STATUS_OUT1) && (chip->old_state & AD7150_STATUS_OUT1)" checks
; those seem to be repeated
5) add of_match_table to the driver
I (now) suspect that the reason this driver is still in staging is this comment:
/* Timeouts not currently handled by core */
I wonder if things changed since then ?
If not, it would be interesting to implement it in core.
Thanks
Alex
> Melissa Wen (4):
> staging: iio: ad7150: organize registers definition
> staging: iio: ad7150: use FIELD_GET and GENMASK
> staging: iio: ad7150: simplify i2c SMBus return treatment
> staging: iio: ad7150: clean up of comments
>
> drivers/staging/iio/cdc/ad7150.c | 102 ++++++++++++++-----------------
> 1 file changed, 47 insertions(+), 55 deletions(-)
>
> --
> 2.20.1
>
On Sat, May 4, 2019 at 1:25 AM Melissa Wen <[email protected]> wrote:
>
> Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask in
> one go. This makes the code more readable than explicit masking followed
> by a shift.
>
This looks neat.
I'd have to remember to ack it from my work email.
One minor comment inline, which would be the object of a new patch.
> Signed-off-by: Melissa Wen <[email protected]>
> ---
> drivers/staging/iio/cdc/ad7150.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index 24601ba7db88..4ba46fb6ac02 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -5,6 +5,7 @@
> * Copyright 2010-2011 Analog Devices Inc.
> */
>
> +#include <linux/bitfield.h>
> #include <linux/interrupt.h>
> #include <linux/device.h>
> #include <linux/kernel.h>
> @@ -44,6 +45,9 @@
> #define AD7150_SN0_REG 0x16
> #define AD7150_ID_REG 0x17
>
> +/* AD7150 masks */
> +#define AD7150_THRESHTYPE_MSK GENMASK(6, 5)
> +
> /**
> * struct ad7150_chip_info - instance specific chip data
> * @client: i2c client for this device
> @@ -136,7 +140,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
> if (ret < 0)
> return ret;
>
> - threshtype = (ret >> 5) & 0x03;
> + threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
> adaptive = !!(ret & 0x80);
Why not also do something similar for the `adaptive` value ?
>
> switch (type) {
> --
> 2.20.1
>
On Sat, May 4, 2019 at 1:26 AM Melissa Wen <[email protected]> wrote:
>
> Since i2c_smbus_write_byte_data returns no-positive value, this commit
> making the treatment of its return value less verbose.
>
> Signed-off-by: Melissa Wen <[email protected]>
> ---
> drivers/staging/iio/cdc/ad7150.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> index 4ba46fb6ac02..3a4572a9e5ec 100644
> --- a/drivers/staging/iio/cdc/ad7150.c
> +++ b/drivers/staging/iio/cdc/ad7150.c
> @@ -201,16 +201,12 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
> ret = i2c_smbus_write_byte_data(chip->client,
> ad7150_addresses[chan][4],
> sens);
> - if (ret < 0)
> + if (ret)
For i2c_smbus_write_byte_data(), checking "ret < 0" or non-zero, is the same.
Changing this doesn't have any added value.
> return ret;
> -
> - ret = i2c_smbus_write_byte_data(chip->client,
> + else
> + return i2c_smbus_write_byte_data(chip->client,
> ad7150_addresses[chan][5],
> timeout);
The introduction of the "else" branch is a bit noisy.
The code was a bit neater (and readable) before the else branch, and
functionally identical.
Well, when I say neater before, you have to understand, that I (and I
assume that some other people who write drivers) have a slight
fixation for this pattern:
example1:
ret = fn1();
if (ret < 0) // could also be just "if (ret)"
return ret;
ret = fn2();
if (ret < 0) // could also be just "if (ret)"
return ret;
example1a:
+ret = fn3();
+if (ret < 0) // could also be just "if (ret)"
+ return ret;
Various higher-level programming languages, will discourage this
pattern in favor of neater patterns.
I personally, have a few arguments in favor of this pattern:
1) it is closer to how the machine code ; so, closer to how a
low-level instruction looks like
2) if (ever) this needs to be patched, the patch could be neat (see
example1a) ; the examle assumes that it's been added via a patch at a
later point in time
3) it keeps indentation level to a minimum ; this also aligns with
kernel-coding guidelines
(https://www.kernel.org/doc/html/v4.10/process/coding-style.html )
(indentation seems a bit OCD-like when someone points it out at a
review, but it has it's value over time)
> - if (ret < 0)
> - return ret;
> -
> - return 0;
> }
>
> static int ad7150_write_event_config(struct iio_dev *indio_dev,
> --
> 2.20.1
>
On Sat, May 4, 2019 at 2:12 PM Alexandru Ardelean
<[email protected]> wrote:
>
> On Sat, May 4, 2019 at 1:24 AM Melissa Wen <[email protected]> wrote:
> >
> > This patchset solves readability issues in AD7150 code, such as clarify
> > register and mask definition, fashion improvement of mask uses, reduce
> > tedious operation and useless comments.
> >
>
> Hey,
>
> Two patches seem a bit noisy/un-needed.
> The other 2 are fine from me.
>
> This driver does need some work to move it out of staging.
> I am not sure what would be a big blocker for it, other than maybe it
> needs a device-tree binding doc (in YAML format).
> Maybe Jonathan remembers.
>
> Some other low-hanging-fruit ideas would be:
> 1) remove the code for platform_data ; that one seems forgotten from
> some other time; the interrupts should be coming from device-tree,
> from the i2c bindings
> 2) you could do a AD7150_EVENT_SPEC() macro (similar to
> AD7150_TIMEOUT() macro) and use it in the ad7150_events[] list; that
> would reduce a few lines
> 3) similar to 2), you could do a AD7150_CHANNEL(x) macro ;
> 4) in ad7150_event_handler() the checks could be wrapped into a macro,
> or maybe some function ; i am referring to "(int_status &
> AD7150_STATUS_OUT1) && (chip->old_state & AD7150_STATUS_OUT1)" checks
> ; those seem to be repeated
> 5) add of_match_table to the driver
>
> I (now) suspect that the reason this driver is still in staging is this comment:
> /* Timeouts not currently handled by core */
>
> I wonder if things changed since then ?
> If not, it would be interesting to implement it in core.
>
I forgot to mention the wiki page for the driver:
https://wiki.analog.com/resources/tools-software/linux-drivers/iio-cdc/ad7150
it may help with a few things
> Thanks
> Alex
>
>
> > Melissa Wen (4):
> > staging: iio: ad7150: organize registers definition
> > staging: iio: ad7150: use FIELD_GET and GENMASK
> > staging: iio: ad7150: simplify i2c SMBus return treatment
> > staging: iio: ad7150: clean up of comments
> >
> > drivers/staging/iio/cdc/ad7150.c | 102 ++++++++++++++-----------------
> > 1 file changed, 47 insertions(+), 55 deletions(-)
> >
> > --
> > 2.20.1
> >
On Sat, 4 May 2019 13:36:43 +0300
Alexandru Ardelean <[email protected]> wrote:
> On Sat, May 4, 2019 at 1:26 AM Melissa Wen <[email protected]> wrote:
> >
> > Since i2c_smbus_write_byte_data returns no-positive value, this commit
> > making the treatment of its return value less verbose.
> >
> > Signed-off-by: Melissa Wen <[email protected]>
> > ---
> > drivers/staging/iio/cdc/ad7150.c | 10 +++-------
> > 1 file changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > index 4ba46fb6ac02..3a4572a9e5ec 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -201,16 +201,12 @@ static int ad7150_write_event_params(struct iio_dev *indio_dev,
> > ret = i2c_smbus_write_byte_data(chip->client,
> > ad7150_addresses[chan][4],
> > sens);
> > - if (ret < 0)
> > + if (ret)
>
> For i2c_smbus_write_byte_data(), checking "ret < 0" or non-zero, is the same.
> Changing this doesn't have any added value.
The slight note I'd add to that is that if you are (and I think you should)
just doing
return i2c_smbus_write_byte_data()
for the last call then that effectively means we are assuming ret is never positive
in some paths and not others. I'd encourage consistency so would would prefer
this is changed to if (ret).
As in the earlier patch the line between what is noise in a staging (or new
driver) and what is noise in a driver that has been outside staging for years
is different. Not so good for Alex perhaps if there is a chance Analog will
backport fixes for their drivers, but tough luck :)
>
> > return ret;
> > -
> > - ret = i2c_smbus_write_byte_data(chip->client,
> > + else
> > + return i2c_smbus_write_byte_data(chip->client,
> > ad7150_addresses[chan][5],
> > timeout);
>
> The introduction of the "else" branch is a bit noisy.
> The code was a bit neater (and readable) before the else branch, and
> functionally identical.
>
> Well, when I say neater before, you have to understand, that I (and I
> assume that some other people who write drivers) have a slight
> fixation for this pattern:
>
> example1:
> ret = fn1();
>
> if (ret < 0) // could also be just "if (ret)"
> return ret;
>
> ret = fn2();
> if (ret < 0) // could also be just "if (ret)"
> return ret;
>
> example1a:
> +ret = fn3();
> +if (ret < 0) // could also be just "if (ret)"
> + return ret;
>
>
> Various higher-level programming languages, will discourage this
> pattern in favor of neater patterns.
>
> I personally, have a few arguments in favor of this pattern:
> 1) it is closer to how the machine code ; so, closer to how a
> low-level instruction looks like
> 2) if (ever) this needs to be patched, the patch could be neat (see
> example1a) ; the examle assumes that it's been added via a patch at a
> later point in time
> 3) it keeps indentation level to a minimum ; this also aligns with
> kernel-coding guidelines
> (https://www.kernel.org/doc/html/v4.10/process/coding-style.html )
> (indentation seems a bit OCD-like when someone points it out at a
> review, but it has it's value over time)
Nicely laid out argument. Strongest one is the maintainability and
reviewability aspect of it being how kernel code is done and hence
takes every so slightly less thought ;)
Errors paths are indented, good paths not (in general).
Jonathan
>
> > - if (ret < 0)
> > - return ret;
> > -
> > - return 0;
> > }
> >
> > static int ad7150_write_event_config(struct iio_dev *indio_dev,
> > --
> > 2.20.1
> >
On Sat, 4 May 2019 14:12:22 +0300
Alexandru Ardelean <[email protected]> wrote:
> On Sat, May 4, 2019 at 1:24 AM Melissa Wen <[email protected]> wrote:
> >
> > This patchset solves readability issues in AD7150 code, such as clarify
> > register and mask definition, fashion improvement of mask uses, reduce
> > tedious operation and useless comments.
> >
>
> Hey,
>
> Two patches seem a bit noisy/un-needed.
> The other 2 are fine from me.
>
> This driver does need some work to move it out of staging.
> I am not sure what would be a big blocker for it, other than maybe it
> needs a device-tree binding doc (in YAML format).
> Maybe Jonathan remembers.
>
> Some other low-hanging-fruit ideas would be:
> 1) remove the code for platform_data ; that one seems forgotten from
> some other time; the interrupts should be coming from device-tree,
> from the i2c bindings
> 2) you could do a AD7150_EVENT_SPEC() macro (similar to
> AD7150_TIMEOUT() macro) and use it in the ad7150_events[] list; that
> would reduce a few lines
> 3) similar to 2), you could do a AD7150_CHANNEL(x) macro ;
> 4) in ad7150_event_handler() the checks could be wrapped into a macro,
> or maybe some function ; i am referring to "(int_status &
> AD7150_STATUS_OUT1) && (chip->old_state & AD7150_STATUS_OUT1)" checks
> ; those seem to be repeated
> 5) add of_match_table to the driver
>
> I (now) suspect that the reason this driver is still in staging is this comment:
> /* Timeouts not currently handled by core */
>
> I wonder if things changed since then ?
> If not, it would be interesting to implement it in core.
Hmm. Timeouts are 'unusual' to put it lightly.
I'm thinking the ABI needs to perhaps be more specific but not sure what
a good naming is.
Otherwise, I just took a quick look and can't see anything much else
that needs doing. Obviously something might come up in a thorough
review prior to moving it though!
Jonathan
>
> Thanks
> Alex
>
>
> > Melissa Wen (4):
> > staging: iio: ad7150: organize registers definition
> > staging: iio: ad7150: use FIELD_GET and GENMASK
> > staging: iio: ad7150: simplify i2c SMBus return treatment
> > staging: iio: ad7150: clean up of comments
> >
> > drivers/staging/iio/cdc/ad7150.c | 102 ++++++++++++++-----------------
> > 1 file changed, 47 insertions(+), 55 deletions(-)
> >
> > --
> > 2.20.1
> >
On Sat, 4 May 2019 13:13:55 +0300
Alexandru Ardelean <[email protected]> wrote:
> On Sat, May 4, 2019 at 1:25 AM Melissa Wen <[email protected]> wrote:
> >
> > Use the suffix REG to make the register addresses clear
> > and indentation to highlight field names.
> >
>
> I'm inclined to say that this change is a bit too much noise versus added value.
> While the REG suffix does make sense (generally), since it hasn't been
> added from the beginning, it doesn't make much sense to add it now.
>
> It is sufficiently clear (as-is) that these macros refer to registers.
> They should be easy to match with the datasheet as well.
I'd agree for a driver that was outside staging that this is more noise than
it is worth, but one of the aims of moving drivers out is to produce
very clean and nice code. In this particular case there are very few fields
so it's not nearly as much of a mess as some other drivers where it really
hasn't been clear and the _REG suffix added much greater benefits.
Some of the arguments for minimizing noise typically don't apply on
the basis we 'probably' won't backport fixes across the move out of staging.
So I'm a bit on the fence on this one, but probably fall slightly
more in favour of the change.
Hence I would say up to you Melissa on whether you want to keep this
one or not given the arguments in favour and against that Alex
has laid out.
Given there are a few bits needed in later patches, I'll not pick this
one up for now either way!
Jonathan
>
> > Signed-off-by: Melissa Wen <[email protected]>
> > ---
> > drivers/staging/iio/cdc/ad7150.c | 75 ++++++++++++++++----------------
> > 1 file changed, 37 insertions(+), 38 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c b/drivers/staging/iio/cdc/ad7150.c
> > index dd7fcab8e19e..24601ba7db88 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -15,35 +15,34 @@
> > #include <linux/iio/iio.h>
> > #include <linux/iio/sysfs.h>
> > #include <linux/iio/events.h>
> > -/*
> > - * AD7150 registers definition
> > - */
> >
> > -#define AD7150_STATUS 0
> > -#define AD7150_STATUS_OUT1 BIT(3)
> > -#define AD7150_STATUS_OUT2 BIT(5)
> > -#define AD7150_CH1_DATA_HIGH 1
> > -#define AD7150_CH2_DATA_HIGH 3
> > -#define AD7150_CH1_AVG_HIGH 5
> > -#define AD7150_CH2_AVG_HIGH 7
> > -#define AD7150_CH1_SENSITIVITY 9
> > -#define AD7150_CH1_THR_HOLD_H 9
> > -#define AD7150_CH1_TIMEOUT 10
> > -#define AD7150_CH1_SETUP 11
> > -#define AD7150_CH2_SENSITIVITY 12
> > -#define AD7150_CH2_THR_HOLD_H 12
> > -#define AD7150_CH2_TIMEOUT 13
> > -#define AD7150_CH2_SETUP 14
> > -#define AD7150_CFG 15
> > -#define AD7150_CFG_FIX BIT(7)
> > -#define AD7150_PD_TIMER 16
> > -#define AD7150_CH1_CAPDAC 17
> > -#define AD7150_CH2_CAPDAC 18
> > -#define AD7150_SN3 19
> > -#define AD7150_SN2 20
> > -#define AD7150_SN1 21
> > -#define AD7150_SN0 22
> > -#define AD7150_ID 23
> > +/* AD7150 registers */
> > +
> > +#define AD7150_STATUS_REG 0x00
> > +#define AD7150_STATUS_OUT1 BIT(3)
> > +#define AD7150_STATUS_OUT2 BIT(5)
> > +#define AD7150_CH1_DATA_HIGH_REG 0x01
> > +#define AD7150_CH2_DATA_HIGH_REG 0x03
> > +#define AD7150_CH1_AVG_HIGH_REG 0x05
> > +#define AD7150_CH2_AVG_HIGH_REG 0x07
> > +#define AD7150_CH1_SENSITIVITY_REG 0x09
> > +#define AD7150_CH1_THR_HOLD_H_REG 0x09
> > +#define AD7150_CH2_SENSITIVITY_REG 0x0C
> > +#define AD7150_CH1_TIMEOUT_REG 0x0A
> > +#define AD7150_CH1_SETUP_REG 0x0B
> > +#define AD7150_CH2_THR_HOLD_H_REG 0x0C
> > +#define AD7150_CH2_TIMEOUT_REG 0x0D
> > +#define AD7150_CH2_SETUP_REG 0x0E
> > +#define AD7150_CFG_REG 0x0F
> > +#define AD7150_CFG_FIX BIT(7)
> > +#define AD7150_PD_TIMER_REG 0x10
> > +#define AD7150_CH1_CAPDAC_REG 0x11
> > +#define AD7150_CH2_CAPDAC_REG 0x12
> > +#define AD7150_SN3_REG 0x13
> > +#define AD7150_SN2_REG 0x14
> > +#define AD7150_SN1_REG 0x15
> > +#define AD7150_SN0_REG 0x16
> > +#define AD7150_ID_REG 0x17
> >
> > /**
> > * struct ad7150_chip_info - instance specific chip data
> > @@ -85,12 +84,12 @@ struct ad7150_chip_info {
> > */
> >
> > static const u8 ad7150_addresses[][6] = {
> > - { AD7150_CH1_DATA_HIGH, AD7150_CH1_AVG_HIGH,
> > - AD7150_CH1_SETUP, AD7150_CH1_THR_HOLD_H,
> > - AD7150_CH1_SENSITIVITY, AD7150_CH1_TIMEOUT },
> > - { AD7150_CH2_DATA_HIGH, AD7150_CH2_AVG_HIGH,
> > - AD7150_CH2_SETUP, AD7150_CH2_THR_HOLD_H,
> > - AD7150_CH2_SENSITIVITY, AD7150_CH2_TIMEOUT },
> > + { AD7150_CH1_DATA_HIGH_REG, AD7150_CH1_AVG_HIGH_REG,
> > + AD7150_CH1_SETUP_REG, AD7150_CH1_THR_HOLD_H_REG,
> > + AD7150_CH1_SENSITIVITY_REG, AD7150_CH1_TIMEOUT_REG },
> > + { AD7150_CH2_DATA_HIGH_REG, AD7150_CH2_AVG_HIGH_REG,
> > + AD7150_CH2_SETUP_REG, AD7150_CH2_THR_HOLD_H_REG,
> > + AD7150_CH2_SENSITIVITY_REG, AD7150_CH2_TIMEOUT_REG },
> > };
> >
> > static int ad7150_read_raw(struct iio_dev *indio_dev,
> > @@ -133,7 +132,7 @@ static int ad7150_read_event_config(struct iio_dev *indio_dev,
> > bool adaptive;
> > struct ad7150_chip_info *chip = iio_priv(indio_dev);
> >
> > - ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> > + ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
> > if (ret < 0)
> > return ret;
> >
> > @@ -229,7 +228,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
> > if (event_code == chip->current_event)
> > return 0;
> > mutex_lock(&chip->state_lock);
> > - ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG);
> > + ret = i2c_smbus_read_byte_data(chip->client, AD7150_CFG_REG);
> > if (ret < 0)
> > goto error_ret;
> >
> > @@ -264,7 +263,7 @@ static int ad7150_write_event_config(struct iio_dev *indio_dev,
> >
> > cfg |= (!adaptive << 7) | (thresh_type << 5);
> >
> > - ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG, cfg);
> > + ret = i2c_smbus_write_byte_data(chip->client, AD7150_CFG_REG, cfg);
> > if (ret < 0)
> > goto error_ret;
> >
> > @@ -497,7 +496,7 @@ static irqreturn_t ad7150_event_handler(int irq, void *private)
> > s64 timestamp = iio_get_time_ns(indio_dev);
> > int ret;
> >
> > - ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS);
> > + ret = i2c_smbus_read_byte_data(chip->client, AD7150_STATUS_REG);
> > if (ret < 0)
> > return IRQ_HANDLED;
> >
> > --
> > 2.20.1
> >
On Sat, 2019-05-04 at 13:43 +0300, Alexandru Ardelean wrote:
> [External]
>
>
> On Sat, May 4, 2019 at 1:25 AM Melissa Wen <[email protected]> wrote:
> >
> > Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask
> > in
> > one go. This makes the code more readable than explicit masking
> > followed
> > by a shift.
> >
>
> This looks neat.
> I'd have to remember to ack it from my work email.
Acked-by: Alexandru Ardelean <[email protected]>
>
> One minor comment inline, which would be the object of a new patch.
>
> > Signed-off-by: Melissa Wen <[email protected]>
> > ---
> > drivers/staging/iio/cdc/ad7150.c | 6 +++++-
> > 1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > b/drivers/staging/iio/cdc/ad7150.c
> > index 24601ba7db88..4ba46fb6ac02 100644
> > --- a/drivers/staging/iio/cdc/ad7150.c
> > +++ b/drivers/staging/iio/cdc/ad7150.c
> > @@ -5,6 +5,7 @@
> > * Copyright 2010-2011 Analog Devices Inc.
> > */
> >
> > +#include <linux/bitfield.h>
> > #include <linux/interrupt.h>
> > #include <linux/device.h>
> > #include <linux/kernel.h>
> > @@ -44,6 +45,9 @@
> > #define AD7150_SN0_REG 0x16
> > #define AD7150_ID_REG 0x17
> >
> > +/* AD7150 masks */
> > +#define AD7150_THRESHTYPE_MSK GENMASK(6, 5)
> > +
> > /**
> > * struct ad7150_chip_info - instance specific chip data
> > * @client: i2c client for this device
> > @@ -136,7 +140,7 @@ static int ad7150_read_event_config(struct iio_dev
> > *indio_dev,
> > if (ret < 0)
> > return ret;
> >
> > - threshtype = (ret >> 5) & 0x03;
> > + threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
> > adaptive = !!(ret & 0x80);
>
> Why not also do something similar for the `adaptive` value ?
>
> >
> > switch (type) {
> > --
> > 2.20.1
> >
On 05/05, Jonathan Cameron wrote:
> On Sat, 4 May 2019 14:12:22 +0300
> Alexandru Ardelean <[email protected]> wrote:
>
> > On Sat, May 4, 2019 at 1:24 AM Melissa Wen <[email protected]> wrote:
> > >
> > > This patchset solves readability issues in AD7150 code, such as clarify
> > > register and mask definition, fashion improvement of mask uses, reduce
> > > tedious operation and useless comments.
> > >
> >
> > Hey,
> >
> > Two patches seem a bit noisy/un-needed.
> > The other 2 are fine from me.
> >
> > This driver does need some work to move it out of staging.
> > I am not sure what would be a big blocker for it, other than maybe it
> > needs a device-tree binding doc (in YAML format).
> > Maybe Jonathan remembers.
> >
> > Some other low-hanging-fruit ideas would be:
> > 1) remove the code for platform_data ; that one seems forgotten from
> > some other time; the interrupts should be coming from device-tree,
> > from the i2c bindings
> > 2) you could do a AD7150_EVENT_SPEC() macro (similar to
> > AD7150_TIMEOUT() macro) and use it in the ad7150_events[] list; that
> > would reduce a few lines
> > 3) similar to 2), you could do a AD7150_CHANNEL(x) macro ;
> > 4) in ad7150_event_handler() the checks could be wrapped into a macro,
> > or maybe some function ; i am referring to "(int_status &
> > AD7150_STATUS_OUT1) && (chip->old_state & AD7150_STATUS_OUT1)" checks
> > ; those seem to be repeated
> > 5) add of_match_table to the driver
> >
> > I (now) suspect that the reason this driver is still in staging is this comment:
> > /* Timeouts not currently handled by core */
> >
> > I wonder if things changed since then ?
> > If not, it would be interesting to implement it in core.
> Hmm. Timeouts are 'unusual' to put it lightly.
> I'm thinking the ABI needs to perhaps be more specific but not sure what
> a good naming is.
>
> Otherwise, I just took a quick look and can't see anything much else
> that needs doing. Obviously something might come up in a thorough
> review prior to moving it though!
>
> Jonathan
> >
> > Thanks
> > Alex
> >
Hi Alexandru and Jonathan,
Thank you for your help! Soon I will send a v2 with the fixes pointed out.
I'm also including the ideas above in the work plan for this driver.
P.s.: Sorry for having previously sent an email with HTML.
Melissa
> >
> > > Melissa Wen (4):
> > > staging: iio: ad7150: organize registers definition
> > > staging: iio: ad7150: use FIELD_GET and GENMASK
> > > staging: iio: ad7150: simplify i2c SMBus return treatment
> > > staging: iio: ad7150: clean up of comments
> > >
> > > drivers/staging/iio/cdc/ad7150.c | 102 ++++++++++++++-----------------
> > > 1 file changed, 47 insertions(+), 55 deletions(-)
> > >
> > > --
> > > 2.20.1
> > >
>
On 05/06, Ardelean, Alexandru wrote:
> On Sat, 2019-05-04 at 13:43 +0300, Alexandru Ardelean wrote:
> > [External]
> >
> >
> > On Sat, May 4, 2019 at 1:25 AM Melissa Wen <[email protected]> wrote:
> > >
> > > Use the bitfield macro FIELD_GET, and GENMASK to do the shift and mask
> > > in
> > > one go. This makes the code more readable than explicit masking
> > > followed
> > > by a shift.
> > >
> >
> > This looks neat.
> > I'd have to remember to ack it from my work email.
>
> Acked-by: Alexandru Ardelean <[email protected]>
>
> >
> > One minor comment inline, which would be the object of a new patch.
> >
> > > Signed-off-by: Melissa Wen <[email protected]>
> > > ---
> > > drivers/staging/iio/cdc/ad7150.c | 6 +++++-
> > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > > b/drivers/staging/iio/cdc/ad7150.c
> > > index 24601ba7db88..4ba46fb6ac02 100644
> > > --- a/drivers/staging/iio/cdc/ad7150.c
> > > +++ b/drivers/staging/iio/cdc/ad7150.c
> > > @@ -5,6 +5,7 @@
> > > * Copyright 2010-2011 Analog Devices Inc.
> > > */
> > >
> > > +#include <linux/bitfield.h>
> > > #include <linux/interrupt.h>
> > > #include <linux/device.h>
> > > #include <linux/kernel.h>
> > > @@ -44,6 +45,9 @@
> > > #define AD7150_SN0_REG 0x16
> > > #define AD7150_ID_REG 0x17
> > >
> > > +/* AD7150 masks */
> > > +#define AD7150_THRESHTYPE_MSK GENMASK(6, 5)
> > > +
> > > /**
> > > * struct ad7150_chip_info - instance specific chip data
> > > * @client: i2c client for this device
> > > @@ -136,7 +140,7 @@ static int ad7150_read_event_config(struct iio_dev
> > > *indio_dev,
> > > if (ret < 0)
> > > return ret;
> > >
> > > - threshtype = (ret >> 5) & 0x03;
> > > + threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
> > > adaptive = !!(ret & 0x80);
> >
> > Why not also do something similar for the `adaptive` value ?
Hi Alexandru,
Yes, I'm working on it! However, taking a look at the driver datasheet (Table
13, page 19), there seems to be a little mistake in choosing the variable name
and its meaning, since when bit(7) is 1 (true) the threshold is fixed, and not
adaptive. For this reason, I removed it from this patchset to mature the
solution. I will send it as a bug fix if I prove it's necessary.
Do you have any advice or feeling about it to share?
P.s.: Sorry for having previously sent an email with HTML.
Melissa
> >
> > >
> > > switch (type) {
> > > --
> > > 2.20.1
> > >
On Tue, 2019-05-07 at 17:44 -0300, Melissa Wen wrote:
> [External]
>
>
> On 05/06, Ardelean, Alexandru wrote:
> > On Sat, 2019-05-04 at 13:43 +0300, Alexandru Ardelean wrote:
> > > [External]
> > >
> > >
> > > On Sat, May 4, 2019 at 1:25 AM Melissa Wen <[email protected]>
> > > wrote:
> > > >
> > > > Use the bitfield macro FIELD_GET, and GENMASK to do the shift and
> > > > mask
> > > > in
> > > > one go. This makes the code more readable than explicit masking
> > > > followed
> > > > by a shift.
> > > >
> > >
> > > This looks neat.
> > > I'd have to remember to ack it from my work email.
> >
> > Acked-by: Alexandru Ardelean <[email protected]>
> >
> > >
> > > One minor comment inline, which would be the object of a new patch.
> > >
> > > > Signed-off-by: Melissa Wen <[email protected]>
> > > > ---
> > > > drivers/staging/iio/cdc/ad7150.c | 6 +++++-
> > > > 1 file changed, 5 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/staging/iio/cdc/ad7150.c
> > > > b/drivers/staging/iio/cdc/ad7150.c
> > > > index 24601ba7db88..4ba46fb6ac02 100644
> > > > --- a/drivers/staging/iio/cdc/ad7150.c
> > > > +++ b/drivers/staging/iio/cdc/ad7150.c
> > > > @@ -5,6 +5,7 @@
> > > > * Copyright 2010-2011 Analog Devices Inc.
> > > > */
> > > >
> > > > +#include <linux/bitfield.h>
> > > > #include <linux/interrupt.h>
> > > > #include <linux/device.h>
> > > > #include <linux/kernel.h>
> > > > @@ -44,6 +45,9 @@
> > > > #define AD7150_SN0_REG 0x16
> > > > #define AD7150_ID_REG 0x17
> > > >
> > > > +/* AD7150 masks */
> > > > +#define AD7150_THRESHTYPE_MSK GENMASK(6, 5)
> > > > +
> > > > /**
> > > > * struct ad7150_chip_info - instance specific chip data
> > > > * @client: i2c client for this device
> > > > @@ -136,7 +140,7 @@ static int ad7150_read_event_config(struct
> > > > iio_dev
> > > > *indio_dev,
> > > > if (ret < 0)
> > > > return ret;
> > > >
> > > > - threshtype = (ret >> 5) & 0x03;
> > > > + threshtype = FIELD_GET(AD7150_THRESHTYPE_MSK, ret);
> > > > adaptive = !!(ret & 0x80);
> > >
> > > Why not also do something similar for the `adaptive` value ?
>
> Hi Alexandru,
>
> Yes, I'm working on it! However, taking a look at the driver datasheet
> (Table
> 13, page 19), there seems to be a little mistake in choosing the variable
> name
> and its meaning, since when bit(7) is 1 (true) the threshold is fixed,
> and not
> adaptive. For this reason, I removed it from this patchset to mature the
> solution. I will send it as a bug fix if I prove it's necessary.
> Do you have any advice or feeling about it to share?
>
Good find.
Since this is a bug-fix, typically it's good to fix the code as-is now
[even if it isn't neat code], and then do the conversions to neat code.
Bug-fixes always take priority over cosmetic changes.
So, I would send the bug-fix as-soon-as-possible, and then update things.
> P.s.: Sorry for having previously sent an email with HTML.
>
> Melissa
>
> > >
> > > >
> > > > switch (type) {
> > > > --
> > > > 2.20.1
> > > >