Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

[Krzysztof Kozlowski <krzk@kernel.org>]

On 03/03/2025 14:31, Sam Winchenbach wrote:
>>> This prevents the situation where your fundamental frequency falls on, or close
>>> to, a corner frequency which could result in 3dB (half power) loss in your
>>> signal.
>>>
>>> This is all completely indepent of the high-pass filter.
>>
>> Description is confusing a bit, because it suggests the value sets the
>> corner frequency. It explicitly says this - "sets ... corner frequency"
>> and such meaning for properties we usually associate with the property
>> doing this. Here however corner frequency will be always set to rf_in
>> and you just adjust the value.
>>
> 
> How about: "Sets the minimum distance (in Hz) between the fundamental
> frequency of `rf_in` and the corner frequency of the high-pass, input filter
> when operatred in 'auto' mode. The selected high-pass corner frequency will
> be less than, or equal to, `rf_in` - `hpf-margin-hz`. If not setting is found
> that satisfies this relationship the filter will be put into 'bypass'."
> 
> Perhaps that is a bit more clear on the intention of this parameter?

Yes

> 
>>>
>>>>
>>>>> them as separate controls but I am happy to put them into an array if that is
>>>>> the idiomatic approach to situations like this. That said, I am having a
>>>>> difficult time getting dt_binding_check to pass when I have an array of uint64.
>>>>>
>>>>> When listing two items, as in your example below, I get the following:
>>>>> adi,admv8818.example.dtb: admv8818@0: adi,filter-margins-hz: [[0, 30000000], [0, 30000000]] is too long
>>>>
>>>> Tricky to say without seeing your code. Magic crystal ball had
>>>> malfunction today.
>>>
>>> This is the property:
>>>
>>>   adi,filter-margins-hz:
>>>     items:
>>>       - description: |
>>>           The minimum distance, in Hz, between rf_in and the low-pass corner
>>>           frequency when the device is used in "auto" mode. If the sum of
>>>           rf_in and this value is greater than 18.85 GHz then the low-pass
>>>           filter will be put into bypass mode, otherwise the closest corner
>>>           frequency that is greater than or equal to the sum of rf_in plus this
>>>           value will be used.
>>>         minimum: 0
>>>         maximum: 0xFFFFFFFFFFFFFFFF
>>>         default: 0
>>>       - description: |
>>>           The minimum distance, in Hz, between rf_in and the high-pass corner
>>>           frequency when the device is used in "auto" mode. If the difference
>>>           between rf_in and this value is less than 1.75 GHz then the high-pass
>>>           filter will be put into bypass mode, otherwise the closest corner
>>>           frequency that is less than or equal to the difference of rf_in and
>>>           this value will be used.
>>>         minimum: 0
>>>         maximum: 0xFFFFFFFFFFFFFFFF
>>>         default: 0
>>>
>>> And this is the example:
>>>
>>> examples:
>>>   - |
>>>     spi {
>>>       #address-cells = <1>;
>>>       #size-cells = <0>;
>>>       admv8818@0 {
>>>         compatible = "adi,admv8818";
>>>         reg = <0>;
>>>         spi-max-frequency = <10000000>;
>>>         clocks = <&admv8818_rfin>;
>>>         clock-names = "rf_in";
>>>         adi,filter-margins-hz = /bits/ 64 <30000000 30000000>;
>>
>>
>> foo-hz is in 32-bit, so basically you have here 4 32-bit numbers which
>> indeed reported by dtschema - property is too long. Drop 64-bit here.
>>
> 
> I was hoping to keep this 64 bits seeing this is a 18 GHz+ filter. I suppose
> I could change this to MHz and just lose a bit of resolution. Does that sound
> like a better approach?

Does the hardware accept Hz resolution? How many bits do you have in the
registers per each value?

Anyway, the value was 32-bit even in your original patch and your DTS
example was not correct.


> 
>> Device allows multiple LPF/HPF values to be stored in LUT tables and it
>> actually has four independent filters. Shouldn't these be included here?
>> Maybe not LUT tables, but the configuration for all filters?
>>
> 
> There are two filters, the input (high-pass) filter, and the output (low-pass)
> filter. Each filter has four banks, each with a different range of frequencies.
> Only one bank can be selected at a time. Each bank has 16 different possible
> cutoff/corner frequencies. That is a total of 64 distinct values for each of
> the two filters.

Hm, datasheet says:
"four independently controlled high-
pass filters (HPFs) and four independently controlled low-pass
filters (LPFs)"

so four each, not one each, but I guess they wanted to say banks as only
one filter/bank can be active in given time?

> 
> The issue with setting the corner frequency directly is that in certain
> applications (such as software defined radios) the fundamental frequency
> is adjustable, necessitating that the corner frequencies of the filter are
> adjusted accordingly. When the filter is in "auto" mode it is notified via
> the clock system of frequency changes, so using this information it should be
> possible to select new corner frequencies if you know the minimum distance
> between your fundamental frequency and the corner.

I am not advocating to set the corner frequency directly, but just
pointing that your current binding seems incomplete.


Best regards,
Krzysztof