Re: [RFC PATCH 0/3] ad9832: driver cleanup

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 8 Dec 2025 12:36:59 -0300
Tomas Borquez <tomasborquez13@gmail.com> wrote:

> On Sat, Dec 06, 2025 at 04:09:33PM +0000, Jonathan Cameron wrote:
> > On Fri,  5 Dec 2025 17:27:40 -0300
> > Tomas Borquez <tomasborquez13@gmail.com> wrote:  
> 
> Hey Jonathan, thanks for all the feedback!
Hi Tomas,

You are welcome.
> 
> > Opening question for a cleanup of a driver like this is how you plan
> > to test it. Do you have the hardware, or are you emulating / stubbing
> > functions to test it? It is very brave to take on major refactoring
> > without a good way to test.
> > 
> > I was kind of planning to drop this driver this cycle on basis of no
> > interest in sorting it out, but clearly you are interested so great
> > as long as we can be sure it works well after your work on it
> > (or indeed that it works currently!)  
> 
> I don't have the hardware, so I've been testing with a stubbed SPI layer.
> I created a platform device wrapper that registers the IIO device without
> real SPI and replaced spi_sync() with a function that dumps the TX buffer
> via print_hex_dump() which allows me verify the register protocol against
> the datasheet, I validated:
> 
> - Frequency tuning word calculation (32-bit, formula from datasheet)
> - Phase register writes (12-bit values across two transfers)
> - Control register state machine (SLEEPRESCLR, FPSELECT commands)
> - Proper preservation of ctrl_fp bits when updating freq_symbol vs
> phase_symbol independently
> - Input validation (bounds checking on phase/frequency values)
> 
> All the SPI command bytes match what the datasheet specifies. Obviously
> this can't catch electrical issues or timing problems that would only show
> up with real hardware but it does verify the driver's logic and register
> protocol are correct.
> 
> I also ran it through sparse, smatch, and checkpatch with no warnings.
> I can share the test module + logs if that's useful for review.

Fair enough.  That's a laborious solution but if you are willing to
do it I'm not complaining!


> 
> ...
> 
> > > 2) Scale Attributes
> > > 
> > >    The frequency scale is 1 Hz and phase scale is 2*PI/4096 radians.
> > >    I cannot use info_mask_shared_by_type for IIO_CHAN_INFO_SCALE because
> > >    all channels share IIO_ALTVOLTAGE.
> > > 
> > >    So instead I'm using IIO_CONST_ATTR for the scales:
> > > 
> > >      out_altvoltage_frequency_scale = "1"
> > >      out_altvoltage_phase_scale = "0.0015339808"
> > > 
> > >    Is there a better approach here? Or should I just document the units and
> > >    skip scale attributes entirely?  
> > 
> > Good question.  I think right option is to just do the maths in the driver and
> > have out_altvoltage0_frequencyN take the scaled value rather than the register
> > value.  Then do some fixed point maths to get to the required register value.  
> 
> Which is already done in write_frequency(), should I do it with phase too?

yes

> And should I accept radians or microradians?

Radians.  It's in Documntation/ABI/testing/sysfs-bus-iio

The whole odd units of a few basic channel types was a long ago
misconceived attempt to align with hwmon.  If I could go back and change that
everything would be in the basic SI units,  not millivolts etc.
> 
> ...
> 
> I've been working on v2 based on your ABI feedback. If the testing approach
> sounds acceptable I'll send it out and if someone with hardware wants to
> test it, that would be great.

Sounds good to me.  Analog Devices folk: Is anyone able to get their hands
on one of these?

Thanks,

Jonathan

> 
> ...
> 
> Tomas