[PATCH 0/3] staging: iio: ad7780: pattern generation and gain update

[PATCH 0/3] staging: iio: ad7780: pattern generation and gain update

[Giuliano Belinassi]

This series of patches fixes a bug in ad717x chips where the PAT2 bit
was wrongly read as a GAIN bit. It also refactors the pattern_mask
generation with the PAT bits.

Giuliano Belinassi (3):
  staging: iio: ad7780: Add is_ad778x flag chip info
  staging: iio: ad7780: check if ad778x before gain update
  staging: iio: ad7780: generates pattern_mask from PAT bits

 drivers/staging/iio/adc/ad7780.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

-- 
2.19.1

[PATCH 1/3] staging: iio: ad7780: Add is_ad778x flag chip info

[Giuliano Belinassi]

This patch allows further checking of whatever the chip is (ad778x or
ad717x).

Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
 drivers/staging/iio/adc/ad7780.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 91e016d534ed..6e51bfdb076a 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -35,6 +35,7 @@ struct ad7780_chip_info {
 	struct iio_chan_spec	channel;
 	unsigned int		pattern_mask;
 	unsigned int		pattern;
+	u8			is_ad778x;
 };
 
 struct ad7780_state {
@@ -135,21 +136,25 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 		.channel = AD7780_CHANNEL(12, 24),
 		.pattern = 0x5,
 		.pattern_mask = 0x7,
+		.is_ad778x = false,
 	},
 	[ID_AD7171] = {
 		.channel = AD7780_CHANNEL(16, 24),
 		.pattern = 0x5,
 		.pattern_mask = 0x7,
+		.is_ad778x = false,
 	},
 	[ID_AD7780] = {
 		.channel = AD7780_CHANNEL(24, 32),
 		.pattern = 0x1,
 		.pattern_mask = 0x3,
+		.is_ad778x = true,
 	},
 	[ID_AD7781] = {
 		.channel = AD7780_CHANNEL(20, 32),
 		.pattern = 0x1,
 		.pattern_mask = 0x3,
+		.is_ad778x = true,
 	},
 };
 
-- 
2.19.1

Re: [PATCH 1/3] staging: iio: ad7780: Add is_ad778x flag chip info

[Ardelean, Alexandru]

On Wed, 2018-11-07 at 16:49 -0200, Giuliano Belinassi wrote:
> This patch allows further checking of whatever the chip is (ad778x or
> ad717x).

Hey,

The patch looks good overall.
I only have one nitpick for this patch. See inline.

And you can squash this patch with patch `[PATCH 2/3] staging: iio: ad7780:
check if ad778x before gain update`.
In fact, the title of the squashed patch can just be `staging: iio: ad7780:
check if ad778x before gain update` ; because the code in this patch
implies that it's used to check if the device is an ad778x chip.

This patch doesn't have much value on it's own without the 2nd patch, and
you can do them in a single go.

/*
 * Note: yes, I know that these subtle semantics between patch 
 * splitting & squashing can be a bit annoying ; I don't have a general
 * recommendation for them, other than just to keep sending patches
 */

Thanks
Alex

> 
> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> ---
>  drivers/staging/iio/adc/ad7780.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 91e016d534ed..6e51bfdb076a 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -35,6 +35,7 @@ struct ad7780_chip_info {
>  	struct iio_chan_spec	channel;
>  	unsigned int		pattern_mask;
>  	unsigned int		pattern;
> +	u8			is_ad778x;
You could make this `bool` type since you are assigning `true/false` values
to this field. It's generally good to be consistent between type names &
type values when using them [even though in C, these are pretty much the
same].

>  };
>  
>  struct ad7780_state {
> @@ -135,21 +136,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
>  		.channel = AD7780_CHANNEL(12, 24),
>  		.pattern = 0x5,
>  		.pattern_mask = 0x7,
> +		.is_ad778x = false,
>  	},
>  	[ID_AD7171] = {
>  		.channel = AD7780_CHANNEL(16, 24),
>  		.pattern = 0x5,
>  		.pattern_mask = 0x7,
> +		.is_ad778x = false,
>  	},
>  	[ID_AD7780] = {
>  		.channel = AD7780_CHANNEL(24, 32),
>  		.pattern = 0x1,
>  		.pattern_mask = 0x3,
> +		.is_ad778x = true,
>  	},
>  	[ID_AD7781] = {
>  		.channel = AD7780_CHANNEL(20, 32),
>  		.pattern = 0x1,
>  		.pattern_mask = 0x3,
> +		.is_ad778x = true,
>  	},
>  };
>  

[PATCH 2/3] staging: iio: ad7780: check if ad778x before gain update

[Giuliano Belinassi]

Only the ad778x have the 'gain' status bit. Check it before updating.

Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
 drivers/staging/iio/adc/ad7780.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 6e51bfdb076a..0a473aae52f2 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -114,10 +114,12 @@ static int ad7780_postprocess_sample(struct ad_sigma_delta *sigma_delta,
 	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
 		return -EIO;
 
-	if (raw_sample & AD7780_GAIN)
-		st->gain = 1;
-	else
-		st->gain = 128;
+	if (chip_info->is_ad778x) {
+		if (raw_sample & AD7780_GAIN)
+			st->gain = 1;
+		else
+			st->gain = 128;
+	}
 
 	return 0;
 }
-- 
2.19.1

Re: [PATCH 2/3] staging: iio: ad7780: check if ad778x before gain update

[Ardelean, Alexandru]

On Wed, 2018-11-07 at 16:50 -0200, Giuliano Belinassi wrote:
> Only the ad778x have the 'gain' status bit. Check it before updating.
> 

This looks good.
The only note is that it can be squashed with the 1st patch (which I noted
on the 1st patch).

> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> ---
>  drivers/staging/iio/adc/ad7780.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 6e51bfdb076a..0a473aae52f2 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -114,10 +114,12 @@ static int ad7780_postprocess_sample(struct
> ad_sigma_delta *sigma_delta,
>  	    ((raw_sample & chip_info->pattern_mask) != chip_info->pattern))
>  		return -EIO;
>  
> -	if (raw_sample & AD7780_GAIN)
> -		st->gain = 1;
> -	else
> -		st->gain = 128;
> +	if (chip_info->is_ad778x) {
> +		if (raw_sample & AD7780_GAIN)
> +			st->gain = 1;
> +		else
> +			st->gain = 128;
> +	}
>  
>  	return 0;
>  }

[PATCH 3/3] staging: iio: ad7780: generates pattern_mask from PAT bits

[Giuliano Belinassi]

Previously, all pattern_masks in the chip_info table were hardcoded. Now they
are generated using the PAT macros, as described in the datasheets.

Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
---
 drivers/staging/iio/adc/ad7780.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 0a473aae52f2..fa9e047b5191 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -31,6 +31,8 @@
 #define AD7780_PAT1	BIT(1)
 #define AD7780_PAT0	BIT(0)
 
+#define AD7170_PAT2	BIT(2)
+
 struct ad7780_chip_info {
 	struct iio_chan_spec	channel;
 	unsigned int		pattern_mask;
@@ -137,25 +139,25 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
 	[ID_AD7170] = {
 		.channel = AD7780_CHANNEL(12, 24),
 		.pattern = 0x5,
-		.pattern_mask = 0x7,
+		.pattern_mask = AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2,
 		.is_ad778x = false,
 	},
 	[ID_AD7171] = {
 		.channel = AD7780_CHANNEL(16, 24),
 		.pattern = 0x5,
-		.pattern_mask = 0x7,
+		.pattern_mask = AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2,
 		.is_ad778x = false,
 	},
 	[ID_AD7780] = {
 		.channel = AD7780_CHANNEL(24, 32),
 		.pattern = 0x1,
-		.pattern_mask = 0x3,
+		.pattern_mask = AD7780_PAT0 | AD7780_PAT1,
 		.is_ad778x = true,
 	},
 	[ID_AD7781] = {
 		.channel = AD7780_CHANNEL(20, 32),
 		.pattern = 0x1,
-		.pattern_mask = 0x3,
+		.pattern_mask = AD7780_PAT0 | AD7780_PAT1,
 		.is_ad778x = true,
 	},
 };
-- 
2.19.1

Re: [PATCH 3/3] staging: iio: ad7780: generates pattern_mask from PAT bits

[Ardelean, Alexandru]

On Wed, 2018-11-07 at 16:50 -0200, Giuliano Belinassi wrote:
> Previously, all pattern_masks in the chip_info table were hardcoded. Now
> they
> are generated using the PAT macros, as described in the datasheets.
> 

I like this change :)
I only have nitpicks.
See inline.


> Signed-off-by: Giuliano Belinassi <giuliano.belinassi@usp.br>
> ---
>  drivers/staging/iio/adc/ad7780.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 0a473aae52f2..fa9e047b5191 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -31,6 +31,8 @@
>  #define AD7780_PAT1	BIT(1)
>  #define AD7780_PAT0	BIT(0)
>  
> +#define AD7170_PAT2	BIT(2)
> +
>  struct ad7780_chip_info {
>  	struct iio_chan_spec	channel;
>  	unsigned int		pattern_mask;
> @@ -137,25 +139,25 @@ static const struct ad7780_chip_info
> ad7780_chip_info_tbl[] = {
>  	[ID_AD7170] = {
>  		.channel = AD7780_CHANNEL(12, 24),
>  		.pattern = 0x5,
> -		.pattern_mask = 0x7,
> +		.pattern_mask = AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2,

If you are updating these pattern masks, you should update the default
patterns as well with the macros to be consistent.

And to be a bit more compact, you could define:

#define AD7170_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
#d
efine AD7780_PATTERN_MASK	(AD7780_PAT0 | AD7780_PAT1)

#define AD7170_PATTERN		(AD7780_PAT1 | AD7170_PAT2)
#define AD7780_PATTERN		(AD7780_PAT0)

Then you can assign AD7170_PATTERN[_MASK] to AD7170/AD7171 & 
AD7780_PATTERN[_MASK] to AD7780/AD7781.

>  		.is_ad778x = false,
>  	},
>  	[ID_AD7171] = {
>  		.channel = AD7780_CHANNEL(16, 24),
>  		.pattern = 0x5,
> -		.pattern_mask = 0x7,
> +		.pattern_mask = AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2,
>  		.is_ad778x = false,
>  	},
>  	[ID_AD7780] = {
>  		.channel = AD7780_CHANNEL(24, 32),
>  		.pattern = 0x1,
> -		.pattern_mask = 0x3,
> +		.pattern_mask = AD7780_PAT0 | AD7780_PAT1,
>  		.is_ad778x = true,
>  	},
>  	[ID_AD7781] = {
>  		.channel = AD7780_CHANNEL(20, 32),
>  		.pattern = 0x1,
> -		.pattern_mask = 0x3,
> +		.pattern_mask = AD7780_PAT0 | AD7780_PAT1,
>  		.is_ad778x = true,
>  	},
>  };