diff mbox series

[v2] iio: frequency: ad9832: Use FIELD_PREP macro to set bit fields

Message ID 20250317173355.157536-1-simeddon@gmail.com (mailing list archive)
State New
Headers show
Series [v2] iio: frequency: ad9832: Use FIELD_PREP macro to set bit fields | expand

Commit Message

Siddharth Menon March 17, 2025, 5:25 p.m. UTC 
Refactor code to use the FIELD_PREP macro for setting bit fields
instead of manual bit manipulation.

Suggested-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
Signed-off-by: Siddharth Menon <simeddon@gmail.com>
---
 Based on feedback from Jonathan and Marcello, I have made the following
 changes
 
 v1->v2:
 - removed CMD_SHIFT andADD_SHIFT completely
 - use GENMASK
 - store regval into an array and iterate through it
 drivers/staging/iio/frequency/ad9832.c | 53 ++++++++++++++------------
 1 file changed, 28 insertions(+), 25 deletions(-)

Comments

Marcelo Schmitt March 18, 2025, 4:43 a.m. UTC | #1 
Hi Siddharth,

On 03/17, Siddharth Menon wrote:
> Refactor code to use the FIELD_PREP macro for setting bit fields
> instead of manual bit manipulation.
> 
> Suggested-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> Signed-off-by: Siddharth Menon <simeddon@gmail.com>
> ---
...
> +#define CMD_MASK_2   GENMASK(15, 12)
> +#define ADD_MASK_2   GENMASK(11, 8)
> +#define DATA_MASK_2  GENMASK(7, 0)

DATA_MASK_2? Did we already have a data mask?
What about adding the device prefix to the mask name (e.g. AD9832_CMD_MASK)?
Also, this patch fails to compile. Please, apply your patches and build the
kernel before sending the patches to the mailing list. Also, run checkpatch on them.
E.g. 
./scripts/checkpatch.pl --terse --codespell --color=always -strict my_patch.patch

>  
>  /**
>   * struct ad9832_state - driver instance specific data
> @@ -131,6 +134,7 @@ static int ad9832_write_frequency(struct ad9832_state *st,
>  {
>  	unsigned long clk_freq;
>  	unsigned long regval;
> +	u8 regval_bytes[4];
>  
>  	clk_freq = clk_get_rate(st->mclk);
>  
> @@ -138,19 +142,14 @@ static int ad9832_write_frequency(struct ad9832_state *st,
>  		return -EINVAL;
>  
>  	regval = ad9832_calc_freqreg(clk_freq, fout);
> +	put_unaligned_be32(regval, regval_bytes);
>  
> -	st->freq_data[0] = cpu_to_be16((AD9832_CMD_FRE8BITSW << CMD_SHIFT) |
> -					(addr << ADD_SHIFT) |
> -					((regval >> 24) & 0xFF));
> -	st->freq_data[1] = cpu_to_be16((AD9832_CMD_FRE16BITSW << CMD_SHIFT) |
> -					((addr - 1) << ADD_SHIFT) |
> -					((regval >> 16) & 0xFF));
> -	st->freq_data[2] = cpu_to_be16((AD9832_CMD_FRE8BITSW << CMD_SHIFT) |
> -					((addr - 2) << ADD_SHIFT) |
> -					((regval >> 8) & 0xFF));
> -	st->freq_data[3] = cpu_to_be16((AD9832_CMD_FRE16BITSW << CMD_SHIFT) |
> -					((addr - 3) << ADD_SHIFT) |
> -					((regval >> 0) & 0xFF));
> +	for (int i = 0; i < 4; i++) {
> +		st->freq_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK,
> +				(i % 2 == 0) ? AD9832_CMD_FRE8BITSW : AD9832_CMD_FRE16BITSW) |
Hmm, I mentioned using ternary operator and gave an example usage but wasn't
expecting that particular example to really be used. IMHO, the above doesn't
look very good.
Can you try come up with something that, (1) avoids the bit shifting we had
before, (2) uses sound macro/mask/variable naming, and (3) fits into 80 columns?
Might not be an easy task so probably not worth sending much more time on this
if unable to find a good refactoring for the above.

Regards,
Marcelo
Siddharth Menon March 18, 2025, 5:14 a.m. UTC | #2 
On Tue, 18 Mar 2025 at 10:12, Marcelo Schmitt
<marcelo.schmitt1@gmail.com> wrote:
>
> Hi Siddharth,
>
> On 03/17, Siddharth Menon wrote:
> > Refactor code to use the FIELD_PREP macro for setting bit fields
> > instead of manual bit manipulation.
> >
> > Suggested-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com>
> > Signed-off-by: Siddharth Menon <simeddon@gmail.com>
> > ---
> ...
> > +#define CMD_MASK_2   GENMASK(15, 12)
> > +#define ADD_MASK_2   GENMASK(11, 8)
> > +#define DATA_MASK_2  GENMASK(7, 0)
>
> DATA_MASK_2? Did we already have a data mask?
> What about adding the device prefix to the mask name (e.g. AD9832_CMD_MASK)?

I'm sorry, I was comparing the values in a custom driver and copy pasted
the wrong variable names.

> Also, this patch fails to compile. Please, apply your patches and build the
> kernel before sending the patches to the mailing list. Also, run checkpatch on them.
> E.g.
> ./scripts/checkpatch.pl --terse --codespell --color=always -strict my_patch.patch

I shall send in a new patch after testing.
diff mbox series

Patch

diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
index 140ee4f9c137..0c1816505495 100644
--- a/drivers/staging/iio/frequency/ad9832.c
+++ b/drivers/staging/iio/frequency/ad9832.c
@@ -16,6 +16,8 @@ 
 #include <linux/slab.h>
 #include <linux/spi/spi.h>
 #include <linux/sysfs.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -65,11 +67,12 @@ 
 #define AD9832_SLEEP		BIT(13)
 #define AD9832_RESET		BIT(12)
 #define AD9832_CLR		BIT(11)
-#define CMD_SHIFT		12
-#define ADD_SHIFT		8
 #define AD9832_FREQ_BITS	32
 #define AD9832_PHASE_BITS	12
 #define RES_MASK(bits)		((1 << (bits)) - 1)
+#define CMD_MASK_2   GENMASK(15, 12)
+#define ADD_MASK_2   GENMASK(11, 8)
+#define DATA_MASK_2  GENMASK(7, 0)
 
 /**
  * struct ad9832_state - driver instance specific data
@@ -131,6 +134,7 @@  static int ad9832_write_frequency(struct ad9832_state *st,
 {
 	unsigned long clk_freq;
 	unsigned long regval;
+	u8 regval_bytes[4];
 
 	clk_freq = clk_get_rate(st->mclk);
 
@@ -138,19 +142,14 @@  static int ad9832_write_frequency(struct ad9832_state *st,
 		return -EINVAL;
 
 	regval = ad9832_calc_freqreg(clk_freq, fout);
+	put_unaligned_be32(regval, regval_bytes);
 
-	st->freq_data[0] = cpu_to_be16((AD9832_CMD_FRE8BITSW << CMD_SHIFT) |
-					(addr << ADD_SHIFT) |
-					((regval >> 24) & 0xFF));
-	st->freq_data[1] = cpu_to_be16((AD9832_CMD_FRE16BITSW << CMD_SHIFT) |
-					((addr - 1) << ADD_SHIFT) |
-					((regval >> 16) & 0xFF));
-	st->freq_data[2] = cpu_to_be16((AD9832_CMD_FRE8BITSW << CMD_SHIFT) |
-					((addr - 2) << ADD_SHIFT) |
-					((regval >> 8) & 0xFF));
-	st->freq_data[3] = cpu_to_be16((AD9832_CMD_FRE16BITSW << CMD_SHIFT) |
-					((addr - 3) << ADD_SHIFT) |
-					((regval >> 0) & 0xFF));
+	for (int i = 0; i < 4; i++) {
+		st->freq_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK,
+				(i % 2 == 0) ? AD9832_CMD_FRE8BITSW : AD9832_CMD_FRE16BITSW) |
+			FIELD_PREP(AD9832_ADD_MSK, addr - i) |
+			FIELD_PREP(AD9832_DAT_MSK, regval_bytes[i]));
+	}
 
 	return spi_sync(st->spi, &st->freq_msg);
 }
@@ -158,15 +157,19 @@  static int ad9832_write_frequency(struct ad9832_state *st,
 static int ad9832_write_phase(struct ad9832_state *st,
 			      unsigned long addr, unsigned long phase)
 {
+	u8 phase_bytes[2];
+
 	if (phase >= BIT(AD9832_PHASE_BITS))
 		return -EINVAL;
 
-	st->phase_data[0] = cpu_to_be16((AD9832_CMD_PHA8BITSW << CMD_SHIFT) |
-					(addr << ADD_SHIFT) |
-					((phase >> 8) & 0xFF));
-	st->phase_data[1] = cpu_to_be16((AD9832_CMD_PHA16BITSW << CMD_SHIFT) |
-					((addr - 1) << ADD_SHIFT) |
-					(phase & 0xFF));
+	put_unaligned_be16(phase, phase_bytes);
+
+	for (int i = 0; i < 2; i++) {
+		st->phase_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK,
+				(i % 2 == 0) ? AD9832_CMD_PHA8BITSW : AD9832_CMD_PHA16BITSW) |
+			FIELD_PREP(AD9832_ADD_MSK, addr - i) |
+			FIELD_PREP(AD9832_DAT_MSK, phase_bytes[i]));
+	}
 
 	return spi_sync(st->spi, &st->phase_msg);
 }
@@ -201,7 +204,7 @@  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 			st->ctrl_ss &= ~AD9832_SELSRC;
 		else
 			st->ctrl_ss |= AD9832_SELSRC;
-		st->data = cpu_to_be16((AD9832_CMD_SYNCSELSRC << CMD_SHIFT) |
+		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SYNCSELSRC) |
 					st->ctrl_ss);
 		ret = spi_sync(st->spi, &st->msg);
 		break;
@@ -214,7 +217,7 @@  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 			ret = -EINVAL;
 			break;
 		}
-		st->data = cpu_to_be16((AD9832_CMD_FPSELECT << CMD_SHIFT) |
+		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
 					st->ctrl_fp);
 		ret = spi_sync(st->spi, &st->msg);
 		break;
@@ -227,7 +230,7 @@  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		st->ctrl_fp &= ~AD9832_PHASE(3);
 		st->ctrl_fp |= AD9832_PHASE(val);
 
-		st->data = cpu_to_be16((AD9832_CMD_FPSELECT << CMD_SHIFT) |
+		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_FPSELECT) |
 					st->ctrl_fp);
 		ret = spi_sync(st->spi, &st->msg);
 		break;
@@ -238,7 +241,7 @@  static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
 		else
 			st->ctrl_src |= AD9832_RESET;
 
-		st->data = cpu_to_be16((AD9832_CMD_SLEEPRESCLR << CMD_SHIFT) |
+		st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SLEEPRESCLR) |
 					st->ctrl_src);
 		ret = spi_sync(st->spi, &st->msg);
 		break;
@@ -396,7 +399,7 @@  static int ad9832_probe(struct spi_device *spi)
 	spi_message_add_tail(&st->phase_xfer[1], &st->phase_msg);
 
 	st->ctrl_src = AD9832_SLEEP | AD9832_RESET | AD9832_CLR;
-	st->data = cpu_to_be16((AD9832_CMD_SLEEPRESCLR << CMD_SHIFT) |
+	st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SLEEPRESCLR) |
 					st->ctrl_src);
 	ret = spi_sync(st->spi, &st->msg);
 	if (ret) {