Re: [PATCH v5] iio: frequency: ad9832: Use FIELD_PREP macro to set bit fields

Siddharth Menon
2025-04-08T19:56:29Z

On Sun, 30 Mar 2025 at 19:50, Jonathan Cameron <jic23@kernel.org> wrote:
> > + 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];
>

Hello Jonathan,

I briefly went through the datasheet for the device.
From what I understand, the device is expecting 16 bit write operations where:
- First 4 bits: Operation type (frequency/phase)
- Next 4 bits: Destination register address
- Last 8 bits: Data
so these fields would need to be combined into a single 16-bit value regardless.

As I am unable to procure a testing unit at this time, I’m hesitant to make
changes that could unintentionally break the existing driver.

Would it be acceptable to limit the scope of this patch to introducing
bitfield macros and addressing the remaining feedback?

Regards,
Siddharth Menon