| Message ID | 20250319045212.72650-1-simeddon@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] iio: frequency: ad9832: Use FIELD_PREP macro to set bit fields | expand |
Hello Siddharth, I think the proposed changes look better now. Still, some additional comments inline. On 03/19, Siddharth Menon wrote: > Refactor code to use the FIELD_PREP macro for setting bit fields > instead of manual bit manipulation. The word 'refactor' by itself isn't very appealing. Instead, maybe promote how the patch improves code readability saying something like 'Use bitfield macros to clearly specify AD9832 SPI command fields and to make register write code more readable.' Use that exact text if you want. > > Suggested-by: Marcelo Schmitt <marcelo.schmitt1@gmail.com> > Signed-off-by: Siddharth Menon <simeddon@gmail.com> > --- > This one compiles and loads without issue > Should I eliminate the use of FIELD_PREP to avoid bit shifting > alltogether? Not sure I'm getting your question here but some bitshift will be inevitable due to the structure of AD9832 SPI command. The difference is now the shifting is done by macros declared in bitfield.h. That makes it easier for us to focus on aspects that are more specific to the chip in the IIO driver. By the way, I think AD9832_PHASE and RES_MASK could also be declared as a mask, but maybe that change can be made on a separate patch. > v1->v2: > - removed CMD_SHIFT and ADD_SHIFT > - use GENMASK > - store regval in an array and iterate through it > v2->v3: > - add missing header > - refactor code in the previously introduced loops > drivers/staging/iio/frequency/ad9832.c | 58 +++++++++++++++----------- > 1 file changed, 33 insertions(+), 25 deletions(-) > > diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c > index 140ee4f9c137..9a27acd88926 100644 > --- a/drivers/staging/iio/frequency/ad9832.c > +++ b/drivers/staging/iio/frequency/ad9832.c > @@ -16,6 +16,9 @@ > #include <linux/slab.h> > #include <linux/spi/spi.h> > #include <linux/sysfs.h> > +#include <linux/bitfield.h> > +#include <linux/bits.h> > +#include <linux/unaligned.h> > > #include <linux/iio/iio.h> > #include <linux/iio/sysfs.h> > @@ -65,11 +68,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 AD9832_CMD_MSK GENMASK(15, 12) > +#define AD9832_ADD_MSK GENMASK(11, 8) > +#define AD9832_DAT_MSK GENMASK(7, 0) Use tabs to separate mask name and definiiton? #define AD9832_CMD_MSK GENMASK(15, 12) #define AD9832_ADD_MSK GENMASK(11, 8) #define AD9832_DAT_MSK GENMASK(7, 0) > > /** > * struct ad9832_state - driver instance specific data > @@ -131,6 +135,8 @@ static int ad9832_write_frequency(struct ad9832_state *st, > { > unsigned long clk_freq; > unsigned long regval; > + u16 freq_cmd; > + u8 regval_bytes[4]; Prefer reverse xmas tree order. i.e. u8 regval_bytes[4]; u16 freq_cmd; > > clk_freq = clk_get_rate(st->mclk); > > @@ -138,19 +144,15 @@ 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++) { Use ARRAY_SIZE() when possible. for (int i = 0; i < ARRAY_SIZE(st->freq_data); i++) { > + freq_cmd = (i % 2 == 0) ? AD9832_CMD_FRE8BITSW : AD9832_CMD_FRE16BITSW; > + > + st->freq_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, freq_cmd) | > + FIELD_PREP(AD9832_ADD_MSK, addr - i) | > + FIELD_PREP(AD9832_DAT_MSK, regval_bytes[i])); Strange checkpatch didn't complain about that. Would expect the arguments be aligned st->freq_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, freq_cmd) | FIELD_PREP(AD9832_ADD_MSK, addr - i) | FIELD_PREP(AD9832_DAT_MSK, regval_bytes[i])); although code now exceeds 80 columns by a few characters. Not sure which version would look better to Jonathan. > + } > > return spi_sync(st->spi, &st->freq_msg); > } > @@ -158,15 +160,21 @@ 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]; > + u16 phase_cmd; > + > 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++) { Use ARRAY_SIZE() here too. > + phase_cmd = (i % 2 == 0) ? AD9832_CMD_PHA8BITSW : AD9832_CMD_PHA16BITSW; > + > + st->phase_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, phase_cmd) | > + FIELD_PREP(AD9832_ADD_MSK, addr - i) | > + FIELD_PREP(AD9832_DAT_MSK, phase_bytes[i])); Would suggest to align with the first cpu_to_be16 argument here too, but better wait for Jonathan's opinion about this. > + } > > return spi_sync(st->spi, &st->phase_msg); > } > @@ -201,7 +209,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); Not sure about this assignment with mix of FIELD_PREP and non-FIELD_PREP value. Maybe have FIELD_PREP(AD9832_DAT_MSK, st->ctrl_ss)); ? > ret = spi_sync(st->spi, &st->msg); > break; > @@ -214,7 +222,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); Same would apply to these other cases. > ret = spi_sync(st->spi, &st->msg); > break; > @@ -227,7 +235,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 +246,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 +404,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) { > -- > 2.48.1 >
On Sat, 22 Mar 2025 at 18:50, Marcelo Schmitt <marcelo.schmitt1@gmail.com> wrote: > > On 03/19, Siddharth Menon wrote: > > Refactor code to use the FIELD_PREP macro for setting bit fields > > instead of manual bit manipulation. > > The word 'refactor' by itself isn't very appealing. > Instead, maybe promote how the patch improves code readability saying something > like 'Use bitfield macros to clearly specify AD9832 SPI command fields and to > make register write code more readable.' Use that exact text if you want. > Thanks for the feedback, I will update the commit message. > I think AD9832_PHASE and RES_MASK could also be declared as a mask, but maybe > that change can be made on a separate patch. I shall send in another patch addressing this. > > - 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); > Not sure about this assignment with mix of FIELD_PREP and non-FIELD_PREP value. > Maybe have > FIELD_PREP(AD9832_DAT_MSK, st->ctrl_ss)); ? From what I understood, I don't think that would work out. AD9832_SELSRC = BIT(12) but AD9832_DAT_MSK only covers bits 7 through 0 GENMASK(7, 0). It could exceed the maximum value allowed by the mask. Thanks, Siddharth Menon
> > + phase_cmd = (i % 2 == 0) ? AD9832_CMD_PHA8BITSW : AD9832_CMD_PHA16BITSW; > > + > > + st->phase_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, phase_cmd) | > > + FIELD_PREP(AD9832_ADD_MSK, addr - i) | > > + FIELD_PREP(AD9832_DAT_MSK, phase_bytes[i])); > Would suggest to align with the first cpu_to_be16 argument here too, but better > wait for Jonathan's opinion about this. I'd typically go for one tab as done here.
Hi Siddharth, On 03/25, Siddharth Menon wrote: > On Sat, 22 Mar 2025 at 18:50, Marcelo Schmitt > <marcelo.schmitt1@gmail.com> wrote: > > > > On 03/19, Siddharth Menon wrote: > > > Refactor code to use the FIELD_PREP macro for setting bit fields > > > instead of manual bit manipulation. > > ... > I shall send in another patch addressing this. My understanding of Jonathan's reply to this patch is that it would actually be ok to do all MASK/FIELD_PREP/FIELD_GET cleanup in a single patch. > > > > - 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); > > Not sure about this assignment with mix of FIELD_PREP and non-FIELD_PREP value. > > Maybe have > > FIELD_PREP(AD9832_DAT_MSK, st->ctrl_ss)); ? > > From what I understood, I don't think that would work out. AD9832_SELSRC > = BIT(12) but AD9832_DAT_MSK only covers bits 7 through 0 GENMASK(7, 0). > It could exceed the maximum value allowed by the mask. Ah yes, that's correct. I didn't look very carefully at the assignment surroundings when replying. Sure, it would use the BIT(12) mask. So, something like FIELD_PREP(AD9832_SELSRC, st->ctrl_ss) > > Thanks, > Siddharth Menon Regards, Marcelo
diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c index 140ee4f9c137..9a27acd88926 100644 --- a/drivers/staging/iio/frequency/ad9832.c +++ b/drivers/staging/iio/frequency/ad9832.c @@ -16,6 +16,9 @@ #include <linux/slab.h> #include <linux/spi/spi.h> #include <linux/sysfs.h> +#include <linux/bitfield.h> +#include <linux/bits.h> +#include <linux/unaligned.h> #include <linux/iio/iio.h> #include <linux/iio/sysfs.h> @@ -65,11 +68,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 AD9832_CMD_MSK GENMASK(15, 12) +#define AD9832_ADD_MSK GENMASK(11, 8) +#define AD9832_DAT_MSK GENMASK(7, 0) /** * struct ad9832_state - driver instance specific data @@ -131,6 +135,8 @@ static int ad9832_write_frequency(struct ad9832_state *st, { unsigned long clk_freq; unsigned long regval; + u16 freq_cmd; + u8 regval_bytes[4]; clk_freq = clk_get_rate(st->mclk); @@ -138,19 +144,15 @@ 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++) { + freq_cmd = (i % 2 == 0) ? AD9832_CMD_FRE8BITSW : AD9832_CMD_FRE16BITSW; + + st->freq_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, freq_cmd) | + 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 +160,21 @@ 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]; + u16 phase_cmd; + 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++) { + phase_cmd = (i % 2 == 0) ? AD9832_CMD_PHA8BITSW : AD9832_CMD_PHA16BITSW; + + st->phase_data[i] = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, phase_cmd) | + 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 +209,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 +222,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 +235,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 +246,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 +404,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) {
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> --- This one compiles and loads without issue Should I eliminate the use of FIELD_PREP to avoid bit shifting alltogether? v1->v2: - removed CMD_SHIFT and ADD_SHIFT - use GENMASK - store regval in an array and iterate through it v2->v3: - add missing header - refactor code in the previously introduced loops drivers/staging/iio/frequency/ad9832.c | 58 +++++++++++++++----------- 1 file changed, 33 insertions(+), 25 deletions(-)