diff mbox series

[v5,1/5] iio: adc: ad7949: define and use bitfield names

Message ID 20210808015659.2955443-2-liambeguin@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series AD7949 Fixes | expand

Commit Message

Liam Beguin Aug. 8, 2021, 1:56 a.m. UTC 
From: Liam Beguin <lvb@xiphos.com>

Replace raw configuration register values by using FIELD_PREP and
defines to improve readability.

Signed-off-by: Liam Beguin <lvb@xiphos.com>
---
 drivers/iio/adc/ad7949.c | 49 ++++++++++++++++++++++++++++++++++------
 1 file changed, 42 insertions(+), 7 deletions(-)

Comments

Joe Perches Aug. 8, 2021, 4:51 p.m. UTC | #1 
On Sat, 2021-08-07 at 21:56 -0400, Liam Beguin wrote:
> Replace raw configuration register values by using FIELD_PREP and
> defines to improve readability.
[]
> diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
[]
+#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)

I think the naming is a bit confusing as it appears as if
these bitfield ranges are single bits.

> +/* REF: reference/buffer selection */
> +#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
[]
> +/* SEQ: channel sequencer. Allows for scanning channels */
> +#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
>
Liam Beguin Aug. 8, 2021, 10:46 p.m. UTC | #2 
On Sun Aug 8, 2021 at 12:51 PM EDT, Joe Perches wrote:
> On Sat, 2021-08-07 at 21:56 -0400, Liam Beguin wrote:
> > Replace raw configuration register values by using FIELD_PREP and
> > defines to improve readability.
> []
> > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> []
> +#define AD7949_CFG_BIT_INCC GENMASK(12, 10)
>

Hi Joe,

> I think the naming is a bit confusing as it appears as if
> these bitfield ranges are single bits.

That makes sense.
Would AD7949_CFG_BITS_* be good enough?

Thanks,
Liam

>
> > +/* REF: reference/buffer selection */
> > +#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
> []
> > +/* SEQ: channel sequencer. Allows for scanning channels */
> > +#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
> >
Joe Perches Aug. 9, 2021, 8:03 p.m. UTC | #3 
On Sun, 2021-08-08 at 18:46 -0400, Liam Beguin wrote:
> On Sun Aug 8, 2021 at 12:51 PM EDT, Joe Perches wrote:
> > On Sat, 2021-08-07 at 21:56 -0400, Liam Beguin wrote:
> > > Replace raw configuration register values by using FIELD_PREP and
> > > defines to improve readability.
> > []
> > > diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
> > []
> > +#define AD7949_CFG_BIT_INCC GENMASK(12, 10)
> > 
> 
> Hi Joe,
> 
> > I think the naming is a bit confusing as it appears as if
> > these bitfield ranges are single bits.
> 
> That makes sense.
> Would AD7949_CFG_BITS_* be good enough?

Sure, or add MASK or something else like AD7949_CFG_MASK_INCC.

It's pretty common to define _MASK and _SHIFT for these types
of uses.

For instance:
include/drm/drm_dp_helper.h-# define DP_DSC_MAJOR_MASK                  (0xf << 0)
include/drm/drm_dp_helper.h-# define DP_DSC_MINOR_MASK                  (0xf << 4)
include/drm/drm_dp_helper.h:# define DP_DSC_MAJOR_SHIFT                 0
include/drm/drm_dp_helper.h:# define DP_DSC_MINOR_SHIFT                 4
diff mbox series

Patch

diff --git a/drivers/iio/adc/ad7949.c b/drivers/iio/adc/ad7949.c
index 1b4b3203e428..937241ee2a22 100644
--- a/drivers/iio/adc/ad7949.c
+++ b/drivers/iio/adc/ad7949.c
@@ -11,13 +11,39 @@ 
 #include <linux/module.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
+#include <linux/bitfield.h>
 
-#define AD7949_MASK_CHANNEL_SEL		GENMASK(9, 7)
 #define AD7949_MASK_TOTAL		GENMASK(13, 0)
-#define AD7949_OFFSET_CHANNEL_SEL	7
-#define AD7949_CFG_READ_BACK		0x1
 #define AD7949_CFG_REG_SIZE_BITS	14
 
+/* CFG: Configuration Update */
+#define AD7949_CFG_BIT_OVERWRITE	BIT(13)
+
+/* INCC: Input Channel Configuration */
+#define AD7949_CFG_BIT_INCC		GENMASK(12, 10)
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_GND	7
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_COMM	6
+#define AD7949_CFG_VAL_INCC_UNIPOLAR_DIFF	4
+#define AD7949_CFG_VAL_INCC_TEMP		3
+#define AD7949_CFG_VAL_INCC_BIPOLAR		2
+#define AD7949_CFG_VAL_INCC_BIPOLAR_DIFF	0
+
+/* INX: Input channel Selection in a binary fashion */
+#define AD7949_CFG_BIT_INX		GENMASK(9, 7)
+
+/* BW: select bandwidth for low-pass filter. Full or Quarter */
+#define AD7949_CFG_BIT_BW_FULL			BIT(6)
+
+/* REF: reference/buffer selection */
+#define AD7949_CFG_BIT_REF		GENMASK(5, 3)
+#define AD7949_CFG_VAL_REF_EXT_BUF		7
+
+/* SEQ: channel sequencer. Allows for scanning channels */
+#define AD7949_CFG_BIT_SEQ		GENMASK(2, 1)
+
+/* RB: Read back the CFG register */
+#define AD7949_CFG_BIT_RBN		BIT(0)
+
 enum {
 	ID_AD7949 = 0,
 	ID_AD7682,
@@ -109,8 +135,8 @@  static int ad7949_spi_read_channel(struct ad7949_adc_chip *ad7949_adc, int *val,
 	 */
 	for (i = 0; i < 2; i++) {
 		ret = ad7949_spi_write_cfg(ad7949_adc,
-					   channel << AD7949_OFFSET_CHANNEL_SEL,
-					   AD7949_MASK_CHANNEL_SEL);
+					   FIELD_PREP(AD7949_CFG_BIT_INX, channel),
+					   AD7949_CFG_BIT_INX);
 		if (ret)
 			return ret;
 		if (channel == ad7949_adc->current_channel)
@@ -214,10 +240,19 @@  static int ad7949_spi_init(struct ad7949_adc_chip *ad7949_adc)
 {
 	int ret;
 	int val;
+	u16 cfg;
 
-	/* Sequencer disabled, CFG readback disabled, IN0 as default channel */
 	ad7949_adc->current_channel = 0;
-	ret = ad7949_spi_write_cfg(ad7949_adc, 0x3C79, AD7949_MASK_TOTAL);
+
+	cfg = FIELD_PREP(AD7949_CFG_BIT_OVERWRITE, 1) |
+		FIELD_PREP(AD7949_CFG_BIT_INCC, AD7949_CFG_VAL_INCC_UNIPOLAR_GND) |
+		FIELD_PREP(AD7949_CFG_BIT_INX, ad7949_adc->current_channel) |
+		FIELD_PREP(AD7949_CFG_BIT_BW_FULL, 1) |
+		FIELD_PREP(AD7949_CFG_BIT_REF, AD7949_CFG_VAL_REF_EXT_BUF) |
+		FIELD_PREP(AD7949_CFG_BIT_SEQ, 0x0) |
+		FIELD_PREP(AD7949_CFG_BIT_RBN, 1);
+
+	ret = ad7949_spi_write_cfg(ad7949_adc, cfg, AD7949_MASK_TOTAL);
 
 	/*
 	 * Do two dummy conversions to apply the first configuration setting.