| Message ID | 9c8331166124ec2bd52794a9c9f844bfb70d1cd9.1551358569.git.renatogeh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | staging: iio: ad7780: move out of staging | expand |
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 <renatogeh@gmail.com> > --- > 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 <linux/sched.h> > #include <linux/gpio/consumer.h> > #include <linux/module.h> > +#include <linux/bits.h> > > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > @@ -28,16 +29,13 @@ > #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) 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_PAT2 BIT(2) > +#define AD7780_PATTERN_GOOD 1 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_MASK GENMASK(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_GOOD 5 > +#define AD7170_PATTERN_MASK GENMASK(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 >
On Fri, 1 Mar 2019 07:17:04 +0000 "Ardelean, Alexandru" <Alex.Ardelean@analog.com> 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 <renatogeh@gmail.com> > > --- > > 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 <linux/sched.h> > > #include <linux/gpio/consumer.h> > > #include <linux/module.h> > > +#include <linux/bits.h> > > > > #include <linux/iio/iio.h> > > #include <linux/iio/sysfs.h> > > @@ -28,16 +29,13 @@ > > #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) > > 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_PAT2 BIT(2) > > > +#define AD7780_PATTERN_GOOD 1 > > 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_MASK GENMASK(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_GOOD 5 > > +#define AD7170_PATTERN_MASK GENMASK(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 > >
On Sat, 2 Mar 2019 19:07:16 +0000 Jonathan Cameron <jic23@kernel.org> wrote: > On Fri, 1 Mar 2019 07:17:04 +0000 > "Ardelean, Alexandru" <Alex.Ardelean@analog.com> 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 <renatogeh@gmail.com> > > > --- > > > 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 <linux/sched.h> > > > #include <linux/gpio/consumer.h> > > > #include <linux/module.h> > > > +#include <linux/bits.h> > > > > > > #include <linux/iio/iio.h> > > > #include <linux/iio/sysfs.h> > > > @@ -28,16 +29,13 @@ > > > #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) > > > > 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_PAT2 BIT(2) > > > > > +#define AD7780_PATTERN_GOOD 1 > > > > 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_MASK GENMASK(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_GOOD 5 > > > +#define AD7170_PATTERN_MASK GENMASK(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 > > > >
On Sat, 2019-03-02 at 19:08 +0000, Jonathan Cameron wrote: > [External] > > > On Sat, 2 Mar 2019 19:07:16 +0000 > Jonathan Cameron <jic23@kernel.org> wrote: > > > On Fri, 1 Mar 2019 07:17:04 +0000 > > "Ardelean, Alexandru" <Alex.Ardelean@analog.com> 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 <renatogeh@gmail.com> > > > > --- > > > > 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 <linux/sched.h> > > > > #include <linux/gpio/consumer.h> > > > > #include <linux/module.h> > > > > +#include <linux/bits.h> > > > > > > > > #include <linux/iio/iio.h> > > > > #include <linux/iio/sysfs.h> > > > > @@ -28,16 +29,13 @@ > > > > #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) > > > > > > 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_PAT2 BIT(2) > > > > +#define AD7780_PATTERN_GOOD 1 > > > > > > 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_MASK GENMASK(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_GOOD 5 > > > > +#define AD7170_PATTERN_MASK GENMASK(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 > > > > > >
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 <linux/sched.h> #include <linux/gpio/consumer.h> #include <linux/module.h> +#include <linux/bits.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> @@ -28,16 +29,13 @@ #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 AD7780_PATTERN_GOOD 1 +#define AD7780_PATTERN_MASK GENMASK(1, 0) -#define AD7170_PATTERN (AD7780_PAT0 | AD7170_PAT2) -#define AD7170_PATTERN_MASK (AD7780_PAT0 | AD7780_PAT1 | AD7170_PAT2) +#define AD7170_PATTERN_GOOD 5 +#define AD7170_PATTERN_MASK GENMASK(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, },
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 <renatogeh@gmail.com> --- drivers/staging/iio/adc/ad7780.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)