[PATCH 0/4] staging: iio: ad7150: improve driver readability

[PATCH 0/4] staging: iio: ad7150: improve driver readability

[Melissa Wen]

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

[PATCH 1/4] staging: iio: ad7150: organize registers definition

[Melissa Wen]

Use the suffix REG to make the register addresses clear
and indentation to highlight field names.

Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
---
 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

Re: [PATCH 1/4] staging: iio: ad7150: organize registers definition

[Alexandru Ardelean]

On Sat, May 4, 2019 at 1:25 AM Melissa Wen <melissa.srw@gmail.com> 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 <melissa.srw@gmail.com>
> ---
>  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
>

Re: [PATCH 1/4] staging: iio: ad7150: organize registers definition

[Jonathan Cameron]

On Sat, 4 May 2019 13:13:55 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, May 4, 2019 at 1:25 AM Melissa Wen <melissa.srw@gmail.com> 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 <melissa.srw@gmail.com>
> > ---
> >  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
> >  

[PATCH 2/4] staging: iio: ad7150: use FIELD_GET and GENMASK

[Melissa Wen]

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 <melissa.srw@gmail.com>
---
 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

Re: [PATCH 2/4] staging: iio: ad7150: use FIELD_GET and GENMASK

[Alexandru Ardelean]

On Sat, May 4, 2019 at 1:25 AM Melissa Wen <melissa.srw@gmail.com> 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 <melissa.srw@gmail.com>
> ---
>  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
>

Re: [PATCH 2/4] staging: iio: ad7150: use FIELD_GET and GENMASK

[Ardelean, Alexandru]

On Sat, 2019-05-04 at 13:43 +0300, Alexandru Ardelean wrote:
> [External]
> 
> 
> On Sat, May 4, 2019 at 1:25 AM Melissa Wen <melissa.srw@gmail.com> 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 <alexandru.ardelean@analog.com>

> 
> One minor comment inline, which would be the object of a new patch.
> 
> > Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
> > ---
> >  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
> > 

Re: [PATCH 2/4] staging: iio: ad7150: use FIELD_GET and GENMASK

[Melissa Wen]

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 <melissa.srw@gmail.com> 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 <alexandru.ardelean@analog.com>
> 
> > 
> > One minor comment inline, which would be the object of a new patch.
> > 
> > > Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
> > > ---
> > >  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
> > > 

Re: [PATCH 2/4] staging: iio: ad7150: use FIELD_GET and GENMASK

[Ardelean, Alexandru]

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 <melissa.srw@gmail.com>
> > > 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 <alexandru.ardelean@analog.com>
> > 
> > > 
> > > One minor comment inline, which would be the object of a new patch.
> > > 
> > > > Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
> > > > ---
> > > >  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
> > > > 

[PATCH 3/4] staging: iio: ad7150: simplify i2c SMBus return treatment

[Melissa Wen]

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 <melissa.srw@gmail.com>
---
 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

Re: [PATCH 3/4] staging: iio: ad7150: simplify i2c SMBus return treatment

[Alexandru Ardelean]

On Sat, May 4, 2019 at 1:26 AM Melissa Wen <melissa.srw@gmail.com> 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 <melissa.srw@gmail.com>
> ---
>  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
>

Re: [PATCH 3/4] staging: iio: ad7150: simplify i2c SMBus return treatment

[Jonathan Cameron]

On Sat, 4 May 2019 13:36:43 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, May 4, 2019 at 1:26 AM Melissa Wen <melissa.srw@gmail.com> 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 <melissa.srw@gmail.com>
> > ---
> >  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
> >  

[PATCH 4/4] staging: iio: ad7150: clean up of comments

[Melissa Wen]

General cleaning of comments to remove useless information or improve
description.

Signed-off-by: Melissa Wen <melissa.srw@gmail.com>
---
 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

Re: [PATCH 0/4] staging: iio: ad7150: improve driver readability

[Alexandru Ardelean]

On Sat, May 4, 2019 at 1:24 AM Melissa Wen <melissa.srw@gmail.com> 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
>

Re: [PATCH 0/4] staging: iio: ad7150: improve driver readability

[Alexandru Ardelean]

On Sat, May 4, 2019 at 2:12 PM Alexandru Ardelean
<ardeleanalex@gmail.com> wrote:
>
> On Sat, May 4, 2019 at 1:24 AM Melissa Wen <melissa.srw@gmail.com> 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
> >

Re: [PATCH 0/4] staging: iio: ad7150: improve driver readability

[Jonathan Cameron]

On Sat, 4 May 2019 14:12:22 +0300
Alexandru Ardelean <ardeleanalex@gmail.com> wrote:

> On Sat, May 4, 2019 at 1:24 AM Melissa Wen <melissa.srw@gmail.com> 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
> >  

Re: [PATCH 0/4] staging: iio: ad7150: improve driver readability

[Melissa Wen]

On 05/05, Jonathan Cameron wrote:
> On Sat, 4 May 2019 14:12:22 +0300
> Alexandru Ardelean <ardeleanalex@gmail.com> wrote:
> 
> > On Sat, May 4, 2019 at 1:24 AM Melissa Wen <melissa.srw@gmail.com> 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
> > >  
>