[PATCH v2 0/2] 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.

Changelog:
* v2
- Squashed is_add778x flag commit with the gain update fix
- Changed u8 is_add778x to bool is_add778x
- Set pattern and pattern_mask as macros
- Aligned identation of macros

Giuliano Belinassi (2):
staging: iio: ad7780: check if ad778x before gain update
staging: iio: ad7780: generates pattern_mask from PAT bits

drivers/staging/iio/adc/ad7780.c | 55 ++++++++++++++++++++------------
1 file changed, 35 insertions(+), 20 deletions(-)

--
2.19.1

[Giuliano Belinassi]

Only the ad778x have the 'gain' status bit. Check it before updating
through a new variable is_ad778x in chip_info.

Signed-off-by: Giuliano Belinassi <giuliano....@usp.br>
---
Changes in v2:
- Squashed is_ad778x declaration commit with the ad778x checkage
- Changed is_ad778x type to bool

drivers/staging/iio/adc/ad7780.c | 15 +++++++++++----
1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 91e016d534ed..9ec2b002539e 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;
+ bool is_ad778x;
};

struct ad7780_state {
@@ -113,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;
}
@@ -135,21 +138,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

[Giuliano Belinassi]

Previously, all pattern_masks and patterns 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....@usp.br>
---
Changes in v2:

- Added the PATTERN and PATTERN_MASK macros
- Update BIT macros alignment to match with PATTERN
- Generate pattern mask with PATTERN_MASK macros.

drivers/staging/iio/adc/ad7780.c | 40 +++++++++++++++++++-------------
1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 9ec2b002539e..ff8e3b2d0efc 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -22,14 +22,22 @@
#include <linux/iio/sysfs.h>
#include <linux/iio/adc/ad_sigma_delta.h>

-#define AD7780_RDY BIT(7)
-#define AD7780_FILTER BIT(6)
-#define AD7780_ERR BIT(5)
-#define AD7780_ID1 BIT(4)
-#define AD7780_ID0 BIT(3)
-#define AD7780_GAIN BIT(2)
-#define AD7780_PAT1 BIT(1)
-#define AD7780_PAT0 BIT(0)
+#define AD7780_RDY BIT(7)
+#define AD7780_FILTER BIT(6)
+#define AD7780_ERR BIT(5)
+#define AD7780_ID1 BIT(4)
+#define AD7780_ID0 BIT(3)
+#define AD7780_GAIN BIT(2)
+#define AD7780_PAT1 BIT(1)
+#define AD7780_PAT0 BIT(0)
+
+#define AD7780_PATTERN (AD7780_PAT0)
+#define AD7780_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1)
+
+#define AD7170_PAT2 BIT(2)
+
+#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
+#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)

struct ad7780_chip_info {
struct iio_chan_spec channel;

@@ -136,26 +144,26 @@ static const struct ad_sigma_delta_info ad7780_sigma_delta_info = {

static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {

[ID_AD7170] = {
.channel = AD7780_CHANNEL(12, 24),
- .pattern = 0x5,
- .pattern_mask = 0x7,
+ .pattern = AD7170_PATTERN,
+ .pattern_mask = AD7170_PATTERN_MASK,

.is_ad778x = false,
},
[ID_AD7171] = {
.channel = AD7780_CHANNEL(16, 24),

- .pattern = 0x5,
- .pattern_mask = 0x7,
+ .pattern = AD7170_PATTERN,
+ .pattern_mask = AD7170_PATTERN_MASK,

.is_ad778x = false,
},
[ID_AD7780] = {
.channel = AD7780_CHANNEL(24, 32),

- .pattern = 0x1,
- .pattern_mask = 0x3,
+ .pattern = AD7780_PATTERN,
+ .pattern_mask = AD7780_PATTERN_MASK,

.is_ad778x = true,
},
[ID_AD7781] = {
.channel = AD7780_CHANNEL(20, 32),

- .pattern = 0x1,
- .pattern_mask = 0x3,
+ .pattern = AD7780_PATTERN,
+ .pattern_mask = AD7780_PATTERN_MASK,

[Ardelean, Alexandru]

On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> Only the ad778x have the 'gain' status bit. Check it before updating
> through a new variable is_ad778x in chip_info.
>

Looks good.

Alex

[Ardelean, Alexandru]

On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:

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

One comment about indentation/whitespace.

Rest looks good.

Alex

Changing indentation here doesn't add much value; it's mostly
patch/whitespace noise.

While I agree that it looks nicer to indent all these to the same level,
you also need to think about the fact that the kernel git repo is already
pretty big as-is, so it's a good idea if a patch adds as much code/semantic
value [as possible] with as little change [as possible] and with as good of
readability [as possible].
[ Kind of sounds like a balance act between the 3 things ].

[Tomasz Duszynski]

Hi Giuliano,

Comment inline.

Just some random though. Instead of introducing extra level of indentation you
can simply check whether is_ad778x is asserted and simply return.

>
> return 0;
> }
> @@ -135,21 +138,25 @@ static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> .channel = AD7780_CHANNEL(12, 24),
> .pattern = 0x5,
> .pattern_mask = 0x7,
> + .is_ad778x = false,

Any reason for setting this explicitly? That's going to be false anyway.

[Giuliano Augusto Faulin Belinassi]

> Just some random though. Instead of introducing extra level of indentation you
> can simply check whether is_ad778x is asserted and simply return.

I agree that the patch would be smaller if I do that, but is it really
an issue? If yes, then I will update the patch with this change

> Any reason for setting this explicitly? That's going to be false anyway

It seems clearer to me :-)

> --
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+...@googlegroups.com.
> To post to this group, send email to kerne...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/55b5de74-a607-94b9-8c85-40658e38fbb5%40gmail.com.
> For more options, visit https://groups.google.com/d/optout.

[Giuliano Augusto Faulin Belinassi]

Hi

>While I agree that it looks nicer to indent all these to the same level,
>you also need to think about the fact that the kernel git repo is already
>pretty big as-is, so it's a good idea if a patch adds as much code/semantic
>value [as possible] with as little change [as possible] and with as good of
>readability [as possible].

Understood. But can't someone else submit another patch fixing these
indentation issues? That would be another commit and more stuff to the
repository.

> --
> You received this message because you are subscribed to the Google Groups "Kernel USP" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kernel-usp+...@googlegroups.com.
> To post to this group, send email to kerne...@googlegroups.com.

> To view this discussion on the web visit https://groups.google.com/d/msgid/kernel-usp/43efc182fc7ab9aa1d2f1ca798e27d85c7132e1f.camel%40analog.com.

[Jonathan Cameron]

On Thu, 8 Nov 2018 13:44:17 +0000
"Ardelean, Alexandru" <alexandru...@analog.com> wrote:

> On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > Only the ad778x have the 'gain' status bit. Check it before updating
> > through a new variable is_ad778x in chip_info.
> >
>
> Looks good.

Alex, formal tags definitely preferred! It's not as though a
looks good is any less of a review than an Ack, it's just better
hidden as people need to look at mailing list archives...

Jonathan

[Jonathan Cameron]

On Fri, 9 Nov 2018 20:15:45 -0200
Giuliano Augusto Faulin Belinassi <giuliano....@usp.br> wrote:

> > Just some random though. Instead of introducing extra level of indentation you
> > can simply check whether is_ad778x is asserted and simply return.
>
> I agree that the patch would be smaller if I do that, but is it really
> an issue? If yes, then I will update the patch with this change
>
> > Any reason for setting this explicitly? That's going to be false anyway
>
> It seems clearer to me :-)

Definitely marginal, but not a strong reason either way so I've
applied this as is. If there were lots of instances of it I would
have agreed with Tomasz (both suggestions were good but Tomasz said,
minor!)

Jonathan

[Jonathan Cameron]

On Fri, 9 Nov 2018 20:18:58 -0200

Giuliano Augusto Faulin Belinassi <giuliano....@usp.br> wrote:

> Hi
>
> >While I agree that it looks nicer to indent all these to the same level,
> >you also need to think about the fact that the kernel git repo is already
> >pretty big as-is, so it's a good idea if a patch adds as much code/semantic
> >value [as possible] with as little change [as possible] and with as good of
> >readability [as possible].
>
> Understood. But can't someone else submit another patch fixing these
> indentation issues? That would be another commit and more stuff to the
> repository.

Separating real changes from white space changes is much more important
from a reviewability point of view. This patch is small enough that it
doesn't 'really matter' but I would have preferred it as a realignment patch
and a follow up patch making the real change.

Anyhow, it doesn't matter that much for such a small case, so applied to
the togreg branch of iio.git and pushed out as testing for the autobuilders
to play with it.

Thanks,

Jonathan

[Ardelean, Alexandru]

On Sun, 2018-11-11 at 12:58 +0000, Jonathan Cameron wrote:
> On Thu, 8 Nov 2018 13:44:17 +0000
> "Ardelean, Alexandru" <alexandru...@analog.com> wrote:
>
> > On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > > Only the ad778x have the 'gain' status bit. Check it before updating
> > > through a new variable is_ad778x in chip_info.
> > >
> >
> > Looks good.
>
> Alex, formal tags definitely preferred! It's not as though a
> looks good is any less of a review than an Ack, it's just better
> hidden as people need to look at mailing list archives...
>
> Jonathan
>

Acked-by: Alexandru Ardelean <alexandru...@analog.com>

// Will remember that next time :)

Thanks
Alex

[Jonathan Cameron]

On Mon, 12 Nov 2018 07:57:58 +0000

"Ardelean, Alexandru" <alexandru...@analog.com> wrote:

> On Sun, 2018-11-11 at 12:58 +0000, Jonathan Cameron wrote:
> > On Thu, 8 Nov 2018 13:44:17 +0000
> > "Ardelean, Alexandru" <alexandru...@analog.com> wrote:
> >
> > > On Thu, 2018-11-08 at 11:03 -0200, Giuliano Belinassi wrote:
> > > > Only the ad778x have the 'gain' status bit. Check it before updating
> > > > through a new variable is_ad778x in chip_info.
> > > >
> > >
> > > Looks good.
> >
> > Alex, formal tags definitely preferred! It's not as though a
> > looks good is any less of a review than an Ack, it's just better
> > hidden as people need to look at mailing list archives...
> >
> > Jonathan
> >
>
> Acked-by: Alexandru Ardelean <alexandru...@analog.com>

I haven't pushed out togreg yet so can still rebase.

Added. Thanks,

Jonathan