| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z965Rz8NuXhbHrgy@debian-BULLSEYE-live-builder-AMD64>
Date: Sat, 22 Mar 2025 10:21:11 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: Siddharth Menon <simeddon@...il.com>
Cc: linux-iio@...r.kernel.org, lars@...afoo.de,
Michael.Hennerich@...log.com, jic23@...nel.org,
gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
linux-staging@...ts.linux.dev
Subject: Re: [PATCH v3] iio: frequency: ad9832: Use FIELD_PREP macro to set
bit fields
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@...il.com>
> Signed-off-by: Siddharth Menon <simeddon@...il.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
>
Powered by blists - more mailing lists