| 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: <20250330152044.18cf81f6@jic23-huawei>
Date: Sun, 30 Mar 2025 15:20:44 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Siddharth Menon <simeddon@...il.com>
Cc: linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-staging@...ts.linux.dev, gregkh@...uxfoundation.org,
Michael.Hennerich@...log.com, lars@...afoo.de, marcelo.schmitt1@...il.com
Subject: Re: [PATCH v5] iio: frequency: ad9832: Use FIELD_PREP macro to set
bit fields
On Sun, 30 Mar 2025 19:19:51 +0530
Siddharth Menon <simeddon@...il.com> wrote:
> Use bitfield and bitmask macros to clearly specify AD9832 SPI
> command fields to make register write code more readable.
>
> Suggested-by: Marcelo Schmitt <marcelo.schmitt1@...il.com>
> Signed-off-by: Siddharth Menon <simeddon@...il.com>
Hi Siddharth,
I took a closer look at the original data layout handling
in here. It doesn't make much sense to me, so perhaps we should
fix that up whilst making these other changes.
Jonathan
> ---
> v1->v2:
> - remove 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
> v3->v4:
> - update commit message with a better one
> - convert AD9832_PHASE and RES_MASK to masks
> - cleanup a few if else blocks
> v4->v5
> - remove unnecessary inversion (val ? 0 : 1) used
> with AD9832_PHASE_MASK introduced in v4
> - use ARRAY_SIZE instead of fixed integers
> - use reverse xmas tree order
> - align mask macros
> drivers/staging/iio/frequency/ad9832.c | 85 +++++++++++++-------------
> 1 file changed, 44 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/staging/iio/frequency/ad9832.c b/drivers/staging/iio/frequency/ad9832.c
> index 140ee4f9c137..e74d085fb4f2 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>
Before this patch the includes were in alphabetical order.
That's preferred for IIO patches in general, but in this case
following local style makes that more important still.
>
> #include <linux/iio/iio.h>
> #include <linux/iio/sysfs.h>
> @@ -59,17 +62,18 @@
> #define AD9832_CMD_SLEEPRESCLR 0xC
>
> #define AD9832_FREQ BIT(11)
> -#define AD9832_PHASE(x) (((x) & 3) << 9)
> +#define AD9832_PHASE_MASK GENMASK(10, 9)
> #define AD9832_SYNC BIT(13)
> #define AD9832_SELSRC BIT(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 RES_MASK(bits) GENMASK((bits) - 1, 0)
Silly question. Is this used anywhere? Given FIELD_PREP
etc tend not to work with non constant values I went looking
to check this was constant and couldn't find any where it is used.
As such just drop it instead!
> +#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;
> + 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 < ARRAY_SIZE(regval_bytes); 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]));
Looking at the data layout here, this seems like an interesting dance to fill two unrelated
u8 values - it's not a be16 at all.
I'd be tempted to split the freq_data into u8s and then you will just have
st->freq_data[i][0] = FIELD_PREP(AD9832_CMD_MSK, freq_cmd) |
FIELD_PREP(AD9832_ADD_SMK, addr - i);
//with masks adjusted appropriately.
st->freq_data[i][1] = 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 < ARRAY_SIZE(phase_bytes); 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]));
Similarly.
> + }
>
> return spi_sync(st->spi, &st->phase_msg);
> }
> @@ -197,24 +205,22 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> ret = ad9832_write_phase(st, this_attr->address, val);
> break;
> case AD9832_PINCTRL_EN:
> - if (val)
> - st->ctrl_ss &= ~AD9832_SELSRC;
> - else
> - st->ctrl_ss |= AD9832_SELSRC;
> - st->data = cpu_to_be16((AD9832_CMD_SYNCSELSRC << CMD_SHIFT) |
> + st->ctrl_ss &= ~AD9832_SELSRC;
> + st->ctrl_ss |= FIELD_PREP(AD9832_SELSRC, val ? 0 : 1);
> +
> + st->data = cpu_to_be16(FIELD_PREP(AD9832_CMD_MSK, AD9832_CMD_SYNCSELSRC) |
> st->ctrl_ss);
> ret = spi_sync(st->spi, &st->msg);
> break;
> case AD9832_FREQ_SYM:
> - if (val == 1) {
> - st->ctrl_fp |= AD9832_FREQ;
> - } else if (val == 0) {
> + if (val == 1 || val == 0) {
> st->ctrl_fp &= ~AD9832_FREQ;
> + st->ctrl_fp |= FIELD_PREP(AD9832_FREQ, val ? 0 : 1);
> } else {
> 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;
> @@ -224,21 +230,18 @@ static ssize_t ad9832_write(struct device *dev, struct device_attribute *attr,
> break;
> }
>
> - st->ctrl_fp &= ~AD9832_PHASE(3);
> - st->ctrl_fp |= AD9832_PHASE(val);
> + st->ctrl_fp &= ~FIELD_PREP(AD9832_PHASE_MASK, 3);
> + st->ctrl_fp |= FIELD_PREP(AD9832_PHASE_MASK, 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;
> case AD9832_OUTPUT_EN:
> - if (val)
> - st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP |
> - AD9832_CLR);
> - else
> - st->ctrl_src |= AD9832_RESET;
> + st->ctrl_src &= ~(AD9832_RESET | AD9832_SLEEP | AD9832_CLR);
> + st->ctrl_src |= FIELD_PREP(AD9832_RESET, val ? 0 : 1);
>
> - 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) {
Powered by blists - more mailing lists