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

[Jonathan Cameron]

On Wed, 9 Apr 2025 01:25:52 +0530
Siddharth Menon <simeddon@xxxxxxxxx> wrote:

> On Sun, 30 Mar 2025 at 19:50, Jonathan Cameron <jic23@xxxxxxxxxx> 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
>