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 for Android: free password hash cracker in your pocket

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

Message-ID: <20250412113340.6934a8e0@jic23-huawei>
Date: Sat, 12 Apr 2025 11:33:40 +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 Wed, 9 Apr 2025 01:25:52 +0530
Siddharth Menon <simeddon@...il.com> wrote:

> 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.

Hmm. That is really a documentation thing rather than anything real.
If they had been documented it as a control value of 8 bits and a data value of 8
bits then it would naturally map to an array.

> 
> 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.
Sure.  It is always a bit of a risk assessment for changes like this.
I'm less nervous about breaking staging drivers than others, but we should
still do our best to not do so.  Probably not worth spinning up some emulation
for this change!

> 
> Would it be acceptable to limit the scope of this patch to introducing
> bitfield macros and addressing the remaining feedback?
Sure.  We can perhaps revisit this suggestion in a future series.

Jonathan

> 
> Regards,
> Siddharth Menon
>