Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly
On Sat, 2019-03-02 at 19:08 +, Jonathan Cameron wrote:
> [External]
>
>
> On Sat, 2 Mar 2019 19:07:16 +
> Jonathan Cameron wrote:
>
> > On Fri, 1 Mar 2019 07:17:04 +
> > "Ardelean, Alexandru" wrote:
> >
> > > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> > > >
> > > >
> > > > The AD7780 driver contains status pattern bits designed for
> > > > checking
> > > > whether serial transfers have been correctly performed. Pattern
> > > > macros
> > > > were previously generated through bit fields. This patch sets good
> > > > pattern values directly and masks through GENMASK.
> > > >
> > > > Signed-off-by: Renato Lui Geh
> > > > ---
> > > > drivers/staging/iio/adc/ad7780.c | 20 +---
> > > > 1 file changed, 9 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > > b/drivers/staging/iio/adc/ad7780.c
> > > > index 7a68e90ddf14..56c49e28f432 100644
> > > > --- a/drivers/staging/iio/adc/ad7780.c
> > > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > > @@ -17,6 +17,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include
> > > >
> > > > #include
> > > > #include
> > > > @@ -28,16 +29,13 @@
> > > > #define AD7780_ID1 BIT(4)
> > > > #define AD7780_ID0 BIT(3)
> > > > #define AD7780_GAINBIT(2)
> > > > -#define AD7780_PAT1BIT(1)
> > > > -#define AD7780_PAT0BIT(0)
> > >
> > > I don't see a problem to leave the bitfields; they can be read &
> > > matched
> > > easier with the datasheet.
> > >
> > > >
> > > > -#define AD7780_PATTERN (AD7780_PAT0)
> > > > -#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
> > > >
> > > > -#define AD7170_PAT2BIT(2)
> > > > +#define AD7780_PATTERN_GOOD1
> > >
> > > It was also nice before that the PAT0..PAT2 bitfields were used to
> > > define a
> > > good pattern, since it's easier to match with the datasheet.
> >
> > This one was much suggestion. Not particularly important one.
>
> Not enough sleep this week clearly :)
> This one was _my_ suggestion.
Ok.
I assumed a bit I missed a discussion somewhere.
Let's have this as-is here.
I don't mind it much either.
My [personal] feeling is that [in the context of a move-out-of-staging-of-
this-driver] this patch is a bit of noise.
If the series were more tidy-up, then I probably would not have replied.
>
> >
> > Personally if a datasheet is pointlessly confusing I tend to ignore it.
> > This is a two bit field as the bits don't have independent meaning!
> >
> > I'm not strongly tied to it though and as it's an Analog driver and
> > you all do a good job maintaining the set I'll go with your preference!
> > I do prefer the naming of PATTERN_GOOD though if nothing else!
> > >
> > >
> > > > +#define AD7780_PATTERN_MASKGENMASK(1, 0)
> > >
> > > I like the general usage of GENMASK, but I'm not sure in this case
> > > it's
> > > worth doing. Maybe I missed a discussion somewhere, about doing this
> > > change, but it is mostly a cosmetic without any functional change.
> > >
> > >
> > > >
> > > > -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > > > -#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 |
> > > > AD7170_PAT2)
> > > > +#define AD7170_PATTERN_GOOD5
> > > > +#define AD7170_PATTERN_MASKGENMASK(2, 0)
> > > >
> > > > #define AD7780_GAIN_MIDPOINT 64
> > > > #define AD7780_FILTER_MIDPOINT 13350
> > > > @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> > > > ad7780_sigma_delta_info = {
> > > > static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > > > [ID_AD7170] = {
> > > > .channel = AD7170_CHANNEL(12, 24),
> > > > - .pattern = AD7170_PATTERN,
> > > > + .pattern = AD7170_PATTERN_GOOD,
> > > > .pattern_mask = AD7170_PATTERN_MASK,
> > > > .is_ad778x = false,
> > > > },
> > > > [ID_AD7171] = {
> > > > .channel = AD7170_CHANNEL(16, 24),
> > > > - .pattern = AD7170_PATTERN,
> > > > + .pattern = AD7170_PATTERN_GOOD,
> > > > .pattern_mask = AD7170_PATTERN_MASK,
> > > > .is_ad778x = false,
> > > > },
> > > > [ID_AD7780] = {
> > > > .channel = AD7780_CHANNEL(24, 32),
> > > > - .pattern = AD7780_PATTERN,
> > > > + .pattern = AD7780_PATTERN_GOOD,
> > > > .pattern_mask = AD7780_PATTERN_MASK,
> > > > .is_ad778x = true,
> > > > },
> > > > [ID_AD7781] = {
> > > > .channel = AD7780_CHANNEL(20, 32),
> > > > - .pattern = AD7780_PATTERN,
> > > > + .pattern = AD7780_PATTERN_GOOD,
> > > > .pattern_mask = AD7780_PATTERN_MASK,
> > > > .is_ad778x = true,
> > > > },
> > > > --
> > > > 2.21.0
> > > >
>
>
Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly
On Sat, 2 Mar 2019 19:07:16 +
Jonathan Cameron wrote:
> On Fri, 1 Mar 2019 07:17:04 +
> "Ardelean, Alexandru" wrote:
>
> > On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> > >
> > >
> > > The AD7780 driver contains status pattern bits designed for checking
> > > whether serial transfers have been correctly performed. Pattern macros
> > > were previously generated through bit fields. This patch sets good
> > > pattern values directly and masks through GENMASK.
> > >
> > > Signed-off-by: Renato Lui Geh
> > > ---
> > > drivers/staging/iio/adc/ad7780.c | 20 +---
> > > 1 file changed, 9 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/staging/iio/adc/ad7780.c
> > > b/drivers/staging/iio/adc/ad7780.c
> > > index 7a68e90ddf14..56c49e28f432 100644
> > > --- a/drivers/staging/iio/adc/ad7780.c
> > > +++ b/drivers/staging/iio/adc/ad7780.c
> > > @@ -17,6 +17,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > >
> > > #include
> > > #include
> > > @@ -28,16 +29,13 @@
> > > #define AD7780_ID1 BIT(4)
> > > #define AD7780_ID0 BIT(3)
> > > #define AD7780_GAINBIT(2)
> > > -#define AD7780_PAT1BIT(1)
> > > -#define AD7780_PAT0BIT(0)
> >
> > I don't see a problem to leave the bitfields; they can be read & matched
> > easier with the datasheet.
> >
> > >
> > > -#define AD7780_PATTERN (AD7780_PAT0)
> > > -#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
> > >
> > > -#define AD7170_PAT2BIT(2)
> >
> > > +#define AD7780_PATTERN_GOOD1
> >
> > It was also nice before that the PAT0..PAT2 bitfields were used to define a
> > good pattern, since it's easier to match with the datasheet.
> This one was much suggestion. Not particularly important one.
Not enough sleep this week clearly :)
This one was _my_ suggestion.
>
> Personally if a datasheet is pointlessly confusing I tend to ignore it.
> This is a two bit field as the bits don't have independent meaning!
>
> I'm not strongly tied to it though and as it's an Analog driver and
> you all do a good job maintaining the set I'll go with your preference!
> I do prefer the naming of PATTERN_GOOD though if nothing else!
> >
> >
> > > +#define AD7780_PATTERN_MASKGENMASK(1, 0)
> >
> > I like the general usage of GENMASK, but I'm not sure in this case it's
> > worth doing. Maybe I missed a discussion somewhere, about doing this
> > change, but it is mostly a cosmetic without any functional change.
> >
> >
> > >
> > > -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > > -#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> > > +#define AD7170_PATTERN_GOOD5
> > > +#define AD7170_PATTERN_MASKGENMASK(2, 0)
> > >
> > > #define AD7780_GAIN_MIDPOINT 64
> > > #define AD7780_FILTER_MIDPOINT 13350
> > > @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> > > ad7780_sigma_delta_info = {
> > > static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > > [ID_AD7170] = {
> > > .channel = AD7170_CHANNEL(12, 24),
> > > - .pattern = AD7170_PATTERN,
> > > + .pattern = AD7170_PATTERN_GOOD,
> > > .pattern_mask = AD7170_PATTERN_MASK,
> > > .is_ad778x = false,
> > > },
> > > [ID_AD7171] = {
> > > .channel = AD7170_CHANNEL(16, 24),
> > > - .pattern = AD7170_PATTERN,
> > > + .pattern = AD7170_PATTERN_GOOD,
> > > .pattern_mask = AD7170_PATTERN_MASK,
> > > .is_ad778x = false,
> > > },
> > > [ID_AD7780] = {
> > > .channel = AD7780_CHANNEL(24, 32),
> > > - .pattern = AD7780_PATTERN,
> > > + .pattern = AD7780_PATTERN_GOOD,
> > > .pattern_mask = AD7780_PATTERN_MASK,
> > > .is_ad778x = true,
> > > },
> > > [ID_AD7781] = {
> > > .channel = AD7780_CHANNEL(20, 32),
> > > - .pattern = AD7780_PATTERN,
> > > + .pattern = AD7780_PATTERN_GOOD,
> > > .pattern_mask = AD7780_PATTERN_MASK,
> > > .is_ad778x = true,
> > > },
> > > --
> > > 2.21.0
> > >
>
Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly
On Fri, 1 Mar 2019 07:17:04 +
"Ardelean, Alexandru" wrote:
> On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
> >
> >
> > The AD7780 driver contains status pattern bits designed for checking
> > whether serial transfers have been correctly performed. Pattern macros
> > were previously generated through bit fields. This patch sets good
> > pattern values directly and masks through GENMASK.
> >
> > Signed-off-by: Renato Lui Geh
> > ---
> > drivers/staging/iio/adc/ad7780.c | 20 +---
> > 1 file changed, 9 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/staging/iio/adc/ad7780.c
> > b/drivers/staging/iio/adc/ad7780.c
> > index 7a68e90ddf14..56c49e28f432 100644
> > --- a/drivers/staging/iio/adc/ad7780.c
> > +++ b/drivers/staging/iio/adc/ad7780.c
> > @@ -17,6 +17,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include
> > #include
> > @@ -28,16 +29,13 @@
> > #define AD7780_ID1 BIT(4)
> > #define AD7780_ID0 BIT(3)
> > #define AD7780_GAINBIT(2)
> > -#define AD7780_PAT1BIT(1)
> > -#define AD7780_PAT0BIT(0)
>
> I don't see a problem to leave the bitfields; they can be read & matched
> easier with the datasheet.
>
> >
> > -#define AD7780_PATTERN (AD7780_PAT0)
> > -#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
> >
> > -#define AD7170_PAT2BIT(2)
>
> > +#define AD7780_PATTERN_GOOD1
>
> It was also nice before that the PAT0..PAT2 bitfields were used to define a
> good pattern, since it's easier to match with the datasheet.
This one was much suggestion. Not particularly important one.
Personally if a datasheet is pointlessly confusing I tend to ignore it.
This is a two bit field as the bits don't have independent meaning!
I'm not strongly tied to it though and as it's an Analog driver and
you all do a good job maintaining the set I'll go with your preference!
I do prefer the naming of PATTERN_GOOD though if nothing else!
>
>
> > +#define AD7780_PATTERN_MASKGENMASK(1, 0)
>
> I like the general usage of GENMASK, but I'm not sure in this case it's
> worth doing. Maybe I missed a discussion somewhere, about doing this
> change, but it is mostly a cosmetic without any functional change.
>
>
> >
> > -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> > -#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> > +#define AD7170_PATTERN_GOOD5
> > +#define AD7170_PATTERN_MASKGENMASK(2, 0)
> >
> > #define AD7780_GAIN_MIDPOINT 64
> > #define AD7780_FILTER_MIDPOINT 13350
> > @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> > ad7780_sigma_delta_info = {
> > static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> > [ID_AD7170] = {
> > .channel = AD7170_CHANNEL(12, 24),
> > - .pattern = AD7170_PATTERN,
> > + .pattern = AD7170_PATTERN_GOOD,
> > .pattern_mask = AD7170_PATTERN_MASK,
> > .is_ad778x = false,
> > },
> > [ID_AD7171] = {
> > .channel = AD7170_CHANNEL(16, 24),
> > - .pattern = AD7170_PATTERN,
> > + .pattern = AD7170_PATTERN_GOOD,
> > .pattern_mask = AD7170_PATTERN_MASK,
> > .is_ad778x = false,
> > },
> > [ID_AD7780] = {
> > .channel = AD7780_CHANNEL(24, 32),
> > - .pattern = AD7780_PATTERN,
> > + .pattern = AD7780_PATTERN_GOOD,
> > .pattern_mask = AD7780_PATTERN_MASK,
> > .is_ad778x = true,
> > },
> > [ID_AD7781] = {
> > .channel = AD7780_CHANNEL(20, 32),
> > - .pattern = AD7780_PATTERN,
> > + .pattern = AD7780_PATTERN_GOOD,
> > .pattern_mask = AD7780_PATTERN_MASK,
> > .is_ad778x = true,
> > },
> > --
> > 2.21.0
> >
Re: [PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly
On Thu, 2019-02-28 at 11:24 -0300, Renato Lui Geh wrote:
>
>
> The AD7780 driver contains status pattern bits designed for checking
> whether serial transfers have been correctly performed. Pattern macros
> were previously generated through bit fields. This patch sets good
> pattern values directly and masks through GENMASK.
>
> Signed-off-by: Renato Lui Geh
> ---
> drivers/staging/iio/adc/ad7780.c | 20 +---
> 1 file changed, 9 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/staging/iio/adc/ad7780.c
> b/drivers/staging/iio/adc/ad7780.c
> index 7a68e90ddf14..56c49e28f432 100644
> --- a/drivers/staging/iio/adc/ad7780.c
> +++ b/drivers/staging/iio/adc/ad7780.c
> @@ -17,6 +17,7 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> @@ -28,16 +29,13 @@
> #define AD7780_ID1 BIT(4)
> #define AD7780_ID0 BIT(3)
> #define AD7780_GAINBIT(2)
> -#define AD7780_PAT1BIT(1)
> -#define AD7780_PAT0BIT(0)
I don't see a problem to leave the bitfields; they can be read & matched
easier with the datasheet.
>
> -#define AD7780_PATTERN (AD7780_PAT0)
> -#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
>
> -#define AD7170_PAT2BIT(2)
> +#define AD7780_PATTERN_GOOD1
It was also nice before that the PAT0..PAT2 bitfields were used to define a
good pattern, since it's easier to match with the datasheet.
> +#define AD7780_PATTERN_MASKGENMASK(1, 0)
I like the general usage of GENMASK, but I'm not sure in this case it's
worth doing. Maybe I missed a discussion somewhere, about doing this
change, but it is mostly a cosmetic without any functional change.
>
> -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
> -#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
> +#define AD7170_PATTERN_GOOD5
> +#define AD7170_PATTERN_MASKGENMASK(2, 0)
>
> #define AD7780_GAIN_MIDPOINT 64
> #define AD7780_FILTER_MIDPOINT 13350
> @@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
> ad7780_sigma_delta_info = {
> static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
> [ID_AD7170] = {
> .channel = AD7170_CHANNEL(12, 24),
> - .pattern = AD7170_PATTERN,
> + .pattern = AD7170_PATTERN_GOOD,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7171] = {
> .channel = AD7170_CHANNEL(16, 24),
> - .pattern = AD7170_PATTERN,
> + .pattern = AD7170_PATTERN_GOOD,
> .pattern_mask = AD7170_PATTERN_MASK,
> .is_ad778x = false,
> },
> [ID_AD7780] = {
> .channel = AD7780_CHANNEL(24, 32),
> - .pattern = AD7780_PATTERN,
> + .pattern = AD7780_PATTERN_GOOD,
> .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> [ID_AD7781] = {
> .channel = AD7780_CHANNEL(20, 32),
> - .pattern = AD7780_PATTERN,
> + .pattern = AD7780_PATTERN_GOOD,
> .pattern_mask = AD7780_PATTERN_MASK,
> .is_ad778x = true,
> },
> --
> 2.21.0
>
[PATCH v4 3/9] staging: iio: ad7780: set pattern values and masks directly
The AD7780 driver contains status pattern bits designed for checking
whether serial transfers have been correctly performed. Pattern macros
were previously generated through bit fields. This patch sets good
pattern values directly and masks through GENMASK.
Signed-off-by: Renato Lui Geh
---
drivers/staging/iio/adc/ad7780.c | 20 +---
1 file changed, 9 insertions(+), 11 deletions(-)
diff --git a/drivers/staging/iio/adc/ad7780.c b/drivers/staging/iio/adc/ad7780.c
index 7a68e90ddf14..56c49e28f432 100644
--- a/drivers/staging/iio/adc/ad7780.c
+++ b/drivers/staging/iio/adc/ad7780.c
@@ -17,6 +17,7 @@
#include
#include
#include
+#include
#include
#include
@@ -28,16 +29,13 @@
#define AD7780_ID1 BIT(4)
#define AD7780_ID0 BIT(3)
#define AD7780_GAIN BIT(2)
-#define AD7780_PAT1BIT(1)
-#define AD7780_PAT0BIT(0)
-#define AD7780_PATTERN (AD7780_PAT0)
-#define AD7780_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1)
-#define AD7170_PAT2BIT(2)
+#define AD7780_PATTERN_GOOD1
+#define AD7780_PATTERN_MASKGENMASK(1, 0)
-#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2)
-#define AD7170_PATTERN_MASK(AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2)
+#define AD7170_PATTERN_GOOD5
+#define AD7170_PATTERN_MASKGENMASK(2, 0)
#define AD7780_GAIN_MIDPOINT64
#define AD7780_FILTER_MIDPOINT 13350
@@ -209,25 +207,25 @@ static const struct ad_sigma_delta_info
ad7780_sigma_delta_info = {
static const struct ad7780_chip_info ad7780_chip_info_tbl[] = {
[ID_AD7170] = {
.channel = AD7170_CHANNEL(12, 24),
- .pattern = AD7170_PATTERN,
+ .pattern = AD7170_PATTERN_GOOD,
.pattern_mask = AD7170_PATTERN_MASK,
.is_ad778x = false,
},
[ID_AD7171] = {
.channel = AD7170_CHANNEL(16, 24),
- .pattern = AD7170_PATTERN,
+ .pattern = AD7170_PATTERN_GOOD,
.pattern_mask = AD7170_PATTERN_MASK,
.is_ad778x = false,
},
[ID_AD7780] = {
.channel = AD7780_CHANNEL(24, 32),
- .pattern = AD7780_PATTERN,
+ .pattern = AD7780_PATTERN_GOOD,
.pattern_mask = AD7780_PATTERN_MASK,
.is_ad778x = true,
},
[ID_AD7781] = {
.channel = AD7780_CHANNEL(20, 32),
- .pattern = AD7780_PATTERN,
+ .pattern = AD7780_PATTERN_GOOD,
.pattern_mask = AD7780_PATTERN_MASK,
.is_ad778x = true,
},
--
2.21.0
