linux-kernel - Re: [PATCH v5] iio: frequency: ad9832: Use FIELD

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGd6pzP470VDxGoP4e_2hVXsKrJhnhbv-WgFzCq7tMX9RjOLwg@mail.gmail.com>
Date: Wed, 9 Apr 2025 01:25:52 +0530
From: Siddharth Menon <simeddon@...il.com>
To: Jonathan Cameron <jic23@...nel.org>
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 at 19:50, Jonathan Cameron <jic23@...nel.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