* [PATCH] staging: iio: cdc: ad7746: add additional config defines
@ 2016-10-17 3:21 Eva Rachel Retuya
2016-10-17 7:51 ` Lars-Peter Clausen
0 siblings, 1 reply; 3+ messages in thread
From: Eva Rachel Retuya @ 2016-10-17 3:21 UTC (permalink / raw)
To: linux-iio, outreachy-kernel
Cc: lars, Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh,
Eva Rachel Retuya
Introduce defines for shifting and mask under the config register for
better readability.
Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
---
drivers/staging/iio/cdc/ad7746.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
index f41251c..751f0d1 100644
--- a/drivers/staging/iio/cdc/ad7746.c
+++ b/drivers/staging/iio/cdc/ad7746.c
@@ -70,8 +70,12 @@
#define AD7746_EXCSETUP_EXCLVL(x) (((x) & 0x3) << 0)
/* Config Register Bit Designations (AD7746_REG_CFG) */
-#define AD7746_CONF_VTFS(x) ((x) << 6)
-#define AD7746_CONF_CAPFS(x) ((x) << 3)
+#define AD7746_CONF_VTFS_SHIFT 6
+#define AD7746_CONF_CAPFS_SHIFT 3
+#define AD7746_CONF_VTFS_MASK 0x3
+#define AD7746_CONF_CAPFS_MASK 0x7
+#define AD7746_CONF_VTFS(x) ((x) << AD7746_CONF_VTFS_SHIFT)
+#define AD7746_CONF_CAPFS(x) ((x) << AD7746_CONF_CAPFS_SHIFT)
#define AD7746_CONF_MODE_IDLE (0 << 0)
#define AD7746_CONF_MODE_CONT_CONV (1 << 0)
#define AD7746_CONF_MODE_SINGLE_CONV (2 << 0)
@@ -224,8 +228,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
case IIO_CAPACITANCE:
cap_setup = (chan->address & 0xFF) | AD7746_CAPSETUP_CAPEN;
vt_setup = chip->vt_setup & ~AD7746_VTSETUP_VTEN;
- delay = ad7746_cap_filter_rate_table[(chip->config >> 3) &
- 0x7][1];
+ delay = ad7746_cap_filter_rate_table[(chip->config &
+ AD7746_CONF_CAPFS_MASK) >> AD7746_CONF_CAPFS_SHIFT][1];
if (chip->capdac_set != chan->channel) {
ret = i2c_smbus_write_byte_data(chip->client,
@@ -246,8 +250,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
case IIO_TEMP:
vt_setup = (chan->address & 0xFF) | AD7746_VTSETUP_VTEN;
cap_setup = chip->cap_setup & ~AD7746_CAPSETUP_CAPEN;
- delay = ad7746_cap_filter_rate_table[(chip->config >> 6) &
- 0x3][1];
+ delay = ad7746_cap_filter_rate_table[(chip->config &
+ AD7746_CONF_VTFS_MASK) >> AD7746_CONF_VTFS_SHIFT][1];
break;
default:
return -EINVAL;
@@ -369,7 +373,7 @@ static int ad7746_store_cap_filter_rate_setup(struct ad7746_chip_info *chip,
if (i >= ARRAY_SIZE(ad7746_cap_filter_rate_table))
i = ARRAY_SIZE(ad7746_cap_filter_rate_table) - 1;
- chip->config &= ~AD7746_CONF_CAPFS(0x7);
+ chip->config &= ~AD7746_CONF_CAPFS(AD7746_CONF_CAPFS_MASK);
chip->config |= AD7746_CONF_CAPFS(i);
return 0;
@@ -387,7 +391,7 @@ static int ad7746_store_vt_filter_rate_setup(struct ad7746_chip_info *chip,
if (i >= ARRAY_SIZE(ad7746_vt_filter_rate_table))
i = ARRAY_SIZE(ad7746_vt_filter_rate_table) - 1;
- chip->config &= ~AD7746_CONF_VTFS(0x3);
+ chip->config &= ~AD7746_CONF_VTFS(AD7746_CONF_VTFS_MASK);
chip->config |= AD7746_CONF_VTFS(i);
return 0;
@@ -636,12 +640,14 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
switch (chan->type) {
case IIO_CAPACITANCE:
*val = ad7746_cap_filter_rate_table[
- (chip->config >> 3) & 0x7][0];
+ (chip->config & AD7746_CONF_CAPFS_MASK) >>
+ AD7746_CONF_CAPFS_SHIFT][0];
ret = IIO_VAL_INT;
break;
case IIO_VOLTAGE:
*val = ad7746_vt_filter_rate_table[
- (chip->config >> 6) & 0x3][0];
+ (chip->config & AD7746_CONF_VTFS_MASK) >>
+ AD7746_CONF_VTFS_SHIFT][0];
break;
default:
ret = -EINVAL;
--
2.7.4
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] staging: iio: cdc: ad7746: add additional config defines
2016-10-17 3:21 [PATCH] staging: iio: cdc: ad7746: add additional config defines Eva Rachel Retuya
@ 2016-10-17 7:51 ` Lars-Peter Clausen
2016-10-17 9:45 ` Eva Rachel Retuya
0 siblings, 1 reply; 3+ messages in thread
From: Lars-Peter Clausen @ 2016-10-17 7:51 UTC (permalink / raw)
To: Eva Rachel Retuya, linux-iio, outreachy-kernel
Cc: Michael.Hennerich, jic23, knaack.h, pmeerw, gregkh
On 10/17/2016 05:21 AM, Eva Rachel Retuya wrote:
> Introduce defines for shifting and mask under the config register for
> better readability.
>
> Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
Hi,
Thanks for the patch. Comments inline.
> ---
> drivers/staging/iio/cdc/ad7746.c | 26 ++++++++++++++++----------
> 1 file changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> index f41251c..751f0d1 100644
> --- a/drivers/staging/iio/cdc/ad7746.c
> +++ b/drivers/staging/iio/cdc/ad7746.c
> @@ -70,8 +70,12 @@
> #define AD7746_EXCSETUP_EXCLVL(x) (((x) & 0x3) << 0)
>
> /* Config Register Bit Designations (AD7746_REG_CFG) */
> -#define AD7746_CONF_VTFS(x) ((x) << 6)
> -#define AD7746_CONF_CAPFS(x) ((x) << 3)
> +#define AD7746_CONF_VTFS_SHIFT 6
> +#define AD7746_CONF_CAPFS_SHIFT 3
> +#define AD7746_CONF_VTFS_MASK 0x3
> +#define AD7746_CONF_CAPFS_MASK 0x7
> +#define AD7746_CONF_VTFS(x) ((x) << AD7746_CONF_VTFS_SHIFT)
> +#define AD7746_CONF_CAPFS(x) ((x) << AD7746_CONF_CAPFS_SHIFT)
> #define AD7746_CONF_MODE_IDLE (0 << 0)
> #define AD7746_CONF_MODE_CONT_CONV (1 << 0)
> #define AD7746_CONF_MODE_SINGLE_CONV (2 << 0)
> @@ -224,8 +228,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
> case IIO_CAPACITANCE:
> cap_setup = (chan->address & 0xFF) | AD7746_CAPSETUP_CAPEN;
> vt_setup = chip->vt_setup & ~AD7746_VTSETUP_VTEN;
> - delay = ad7746_cap_filter_rate_table[(chip->config >> 3) &
> - 0x7][1];
> + delay = ad7746_cap_filter_rate_table[(chip->config &
> + AD7746_CONF_CAPFS_MASK) >> AD7746_CONF_CAPFS_SHIFT][1];
This flips the order in which the shift and the mask are applied, that will
break things. But maybe we should change things so that the mask includes
the offset and use the GENMASK() macro to create them. This would also allow
to simplify
chip->config &= ~AD7746_CONF_CAPFS(AD7746_CONF_CAPFS_MASK);
to
chip->config &= ~AD7746_CONF_CAPFS_MASK;
I'd also introduce a helper variable to move the index calculation outside
of the array brackets. It is a bit convoluted the way it is right now.
Same comment for the similar changes done below.
>
> if (chip->capdac_set != chan->channel) {
> ret = i2c_smbus_write_byte_data(chip->client,
> @@ -246,8 +250,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
> case IIO_TEMP:
> vt_setup = (chan->address & 0xFF) | AD7746_VTSETUP_VTEN;
> cap_setup = chip->cap_setup & ~AD7746_CAPSETUP_CAPEN;
> - delay = ad7746_cap_filter_rate_table[(chip->config >> 6) &
> - 0x3][1];
> + delay = ad7746_cap_filter_rate_table[(chip->config &
> + AD7746_CONF_VTFS_MASK) >> AD7746_CONF_VTFS_SHIFT][1];
> break;
> default:
> return -EINVAL;
[...]
> @@ -636,12 +640,14 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
> switch (chan->type) {
> case IIO_CAPACITANCE:
> *val = ad7746_cap_filter_rate_table[
> - (chip->config >> 3) & 0x7][0];
> + (chip->config & AD7746_CONF_CAPFS_MASK) >>
> + AD7746_CONF_CAPFS_SHIFT][0];
> ret = IIO_VAL_INT;
> break;
> case IIO_VOLTAGE:
> *val = ad7746_vt_filter_rate_table[
> - (chip->config >> 6) & 0x3][0];
> + (chip->config & AD7746_CONF_VTFS_MASK) >>
> + AD7746_CONF_VTFS_SHIFT][0];
> break;
> default:
> ret = -EINVAL;
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH] staging: iio: cdc: ad7746: add additional config defines
2016-10-17 7:51 ` Lars-Peter Clausen
@ 2016-10-17 9:45 ` Eva Rachel Retuya
0 siblings, 0 replies; 3+ messages in thread
From: Eva Rachel Retuya @ 2016-10-17 9:45 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: linux-iio, outreachy-kernel, Michael.Hennerich, jic23, knaack.h,
pmeerw, gregkh
On Mon, Oct 17, 2016 at 09:51:07AM +0200, Lars-Peter Clausen wrote:
> On 10/17/2016 05:21 AM, Eva Rachel Retuya wrote:
> > Introduce defines for shifting and mask under the config register for
> > better readability.
> >
> > Signed-off-by: Eva Rachel Retuya <eraretuya@gmail.com>
>
> Hi,
>
> Thanks for the patch. Comments inline.
>
> > ---
> > drivers/staging/iio/cdc/ad7746.c | 26 ++++++++++++++++----------
> > 1 file changed, 16 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/staging/iio/cdc/ad7746.c b/drivers/staging/iio/cdc/ad7746.c
> > index f41251c..751f0d1 100644
> > --- a/drivers/staging/iio/cdc/ad7746.c
> > +++ b/drivers/staging/iio/cdc/ad7746.c
> > @@ -70,8 +70,12 @@
> > #define AD7746_EXCSETUP_EXCLVL(x) (((x) & 0x3) << 0)
> >
> > /* Config Register Bit Designations (AD7746_REG_CFG) */
> > -#define AD7746_CONF_VTFS(x) ((x) << 6)
> > -#define AD7746_CONF_CAPFS(x) ((x) << 3)
> > +#define AD7746_CONF_VTFS_SHIFT 6
> > +#define AD7746_CONF_CAPFS_SHIFT 3
> > +#define AD7746_CONF_VTFS_MASK 0x3
> > +#define AD7746_CONF_CAPFS_MASK 0x7
> > +#define AD7746_CONF_VTFS(x) ((x) << AD7746_CONF_VTFS_SHIFT)
> > +#define AD7746_CONF_CAPFS(x) ((x) << AD7746_CONF_CAPFS_SHIFT)
> > #define AD7746_CONF_MODE_IDLE (0 << 0)
> > #define AD7746_CONF_MODE_CONT_CONV (1 << 0)
> > #define AD7746_CONF_MODE_SINGLE_CONV (2 << 0)
> > @@ -224,8 +228,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
> > case IIO_CAPACITANCE:
> > cap_setup = (chan->address & 0xFF) | AD7746_CAPSETUP_CAPEN;
> > vt_setup = chip->vt_setup & ~AD7746_VTSETUP_VTEN;
> > - delay = ad7746_cap_filter_rate_table[(chip->config >> 3) &
> > - 0x7][1];
> > + delay = ad7746_cap_filter_rate_table[(chip->config &
> > + AD7746_CONF_CAPFS_MASK) >> AD7746_CONF_CAPFS_SHIFT][1];
>
> This flips the order in which the shift and the mask are applied, that will
> break things. But maybe we should change things so that the mask includes
> the offset and use the GENMASK() macro to create them. This would also allow
> to simplify
>
My bad, I blindly followed the feedback from a recent patch I made on
this driver. I'm aware and wondering why the order got reversed,
should've replied back to that thread.
Anyways, thank you for the feedback. Will send v2 with your proposed
changes.
Eva
> chip->config &= ~AD7746_CONF_CAPFS(AD7746_CONF_CAPFS_MASK);
>
> to
>
> chip->config &= ~AD7746_CONF_CAPFS_MASK;
>
> I'd also introduce a helper variable to move the index calculation outside
> of the array brackets. It is a bit convoluted the way it is right now.
>
> Same comment for the similar changes done below.
>
> >
> > if (chip->capdac_set != chan->channel) {
> > ret = i2c_smbus_write_byte_data(chip->client,
> > @@ -246,8 +250,8 @@ static int ad7746_select_channel(struct iio_dev *indio_dev,
> > case IIO_TEMP:
> > vt_setup = (chan->address & 0xFF) | AD7746_VTSETUP_VTEN;
> > cap_setup = chip->cap_setup & ~AD7746_CAPSETUP_CAPEN;
> > - delay = ad7746_cap_filter_rate_table[(chip->config >> 6) &
> > - 0x3][1];
> > + delay = ad7746_cap_filter_rate_table[(chip->config &
> > + AD7746_CONF_VTFS_MASK) >> AD7746_CONF_VTFS_SHIFT][1];
> > break;
> > default:
> > return -EINVAL;
> [...]
> > @@ -636,12 +640,14 @@ static int ad7746_read_raw(struct iio_dev *indio_dev,
> > switch (chan->type) {
> > case IIO_CAPACITANCE:
> > *val = ad7746_cap_filter_rate_table[
> > - (chip->config >> 3) & 0x7][0];
> > + (chip->config & AD7746_CONF_CAPFS_MASK) >>
> > + AD7746_CONF_CAPFS_SHIFT][0];
> > ret = IIO_VAL_INT;
> > break;
> > case IIO_VOLTAGE:
> > *val = ad7746_vt_filter_rate_table[
> > - (chip->config >> 6) & 0x3][0];
> > + (chip->config & AD7746_CONF_VTFS_MASK) >>
> > + AD7746_CONF_VTFS_SHIFT][0];
> > break;
> > default:
> > ret = -EINVAL;
> >
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-10-17 9:45 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-10-17 3:21 [PATCH] staging: iio: cdc: ad7746: add additional config defines Eva Rachel Retuya
2016-10-17 7:51 ` Lars-Peter Clausen
2016-10-17 9:45 ` Eva Rachel Retuya
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox