On Wed, 5 Mar 2025 07:58:23 -0500
Sam Winchenbach <sam.winchenbach@xxxxxxxxxxxxxxxx> wrote:
> On Tue, Mar 04, 2025 at 11:55:02PM +0000, Jonathan Cameron wrote:
> > On Tue, 25 Feb 2025 08:46:11 -0500
> > Sam Winchenbach <sam.winchenbach@xxxxxxxxxxxxxxxx> wrote:
> >
> > Hi Sam,
> >
> > Various comments inline.
> >
> > Jonathan
> >
> >
> > > Corrects the upper range of LPF Band 4 from 18.5 GHz to 18.85 GHz per
> > > the ADMV8818 datasheet
> > This feels like a first fix...
>
> Agreed. This should be broken out. For discussion let's call this PATCH_1.
>
> > >
> > > Search for the minimum error while ensuring that the LPF corner
> > > frequency is greater than the target, and the HPF corner frequency
> > > is lower than the target
> > >
> > > This fixes issues where the range calculations were suboptimal.
> > This feels like a 2nd one. Maybe two patches appropriate.
> >
>
> Agreed. For discussion let's call this PATCH_2.
>
> > >
> > > Add two new DTS properties to set the margin between the input frequency
> > > and the calculated corner frequency
> > And this feels like a feature. So 3rd patch that we don't necessarily
> > backport. For earlier stages we just use the default values that
> > you have in the binding.
> >
>
> Hmm. This is interesting. I propose that PATCH_1 is a fix, and both PATCH_2 and this DTS change are treated as a feature. The reason I am suggesting this is that PATCH_2 changes the behavior of the corner frequency and if we backport that then some users may find that their devices no longer function as they used to. Another way of saying this is that PATCH_2 really should include the DTS changes for those users that depended on the old corner calculation algorithm. Does this sound reasonable?
Sure. I'm fine going with your judgement on what is a fix and what isn't.
Jonathan
>
> > >
> > > Below is a generated table of the differences between the old algorithm
> > > and the new. This is a sweep from 0 to 20 GHz in 10 MHz steps.
> >
> > So, these are just the entries where the 3db point changes?
> > All the others are same?
> >
>
> With a 10 MHz resolution, yes. It was an attempt to show that if the user selected a corner frequency that in many cases there was a better option. The code, as it exists, uses the same algortihm for finding the corner frequency when in either manual or auto mode - There are two problems with this approach:
> 1. If you are using manual mode, you can't select a specific corner frequency without subtracting 1 from your target frequency. This highlights problem number 2.
> 2. If you are in automatic mode and your fundamental frequency is 1 Hz below a corner - that corner will be selected. This will efectively put the corner frequency/3db point at the fundamental frequency. This will cut your signal power in half.
>
> Perhaps there is a better way to show this? Conveying this without images is challenging.
Agreed. Ascii art is always an option but obviously limited.
>
> Here is an example of where both algorithms agree (1 Hz resolution):
> freq = 1750000001 Hz, 3db: 1750000000 (old algorithm) => 1750000000 (new algorithm) Hz
>
> Note that if the user is in `auto` mode then the corner frequency will be put almost exactly on their fundamental frequency.
> The same will happen with the new algorithm, but the user has the ability to select a minimum margin using DTS.
>
> > Multiline comment in IIO (and most of kernel for that matter) is
> > /*
> > * This...
> >
>
> Shoot, I figured check_patch would have caught that. Noted for v5
Unfortunately it's not a consistent requirement across the kernel.
networking still likes the style you used I think.
> > > @@ -242,16 +280,28 @@ static int admv8818_lpf_select(struct admv8818_state *st, u64 freq)
> > > static int admv8818_rfin_band_select(struct admv8818_state *st)
> > > {
> > > int ret;
> > > + u64 hpf_corner_target, lpf_corner_target;
> > >
> > > st->cf_hz = clk_get_rate(st->clkin);
> > >
> > > + // Check for underflow
> >
> > No C++ style comments in IIO code. This is just a consistency thing rather than
> > really matter. We have lots of code that predates those being at all acceptable
> > in the kernel and a mixture of the two styles is messy!
> >
>
> Bugger. check_patch failed me again :)
> Noted. I will go through and address all comments to make sure they fit the style.
Likewise, not a hard rule in all of the kernel.
>
> > > + if (st->cf_hz > st->hpf_margin_hz)
> > st->lpf_margin_hz = 0;
> > device_property_read_u64(...)
> >
> > and no explicit error checking.
> >
> > If you really want to retain the protection against wrong formats etc, then fair enough.
>
> My only concern that the user will have no feedback that his or her filter settings are not being used which could lead to subtle, hard to track down frequency responses. Would it be more appropriate here to print a warning instead of returning an error?
Just stick to an error. I'd hope they test enough to know their
DT is broken even without the error messages but i guess maybe not.
Jonathan
