Message-ID: <Z7-SojPPx3kOVa4y@65YTFL3.secure.tethers.com>
Date: Wed, 26 Feb 2025 17:16:02 -0500
From: Sam Winchenbach <sam.winchenbach@...mepointer.org>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: linux-kernel@...r.kernel.org, lars@...afoo.de,
	Michael.Hennerich@...log.com, antoniu.miclaus@...log.com,
	jic23@...nel.org, robh@...nel.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org
Subject: Re: [PATCH v4 2/2] dt-bindings: iio: filter: Add lpf/hpf freq margins

On Wed, Feb 26, 2025 at 10:22:03PM +0100, Krzysztof Kozlowski wrote:
> On 26/02/2025 18:10, Sam Winchenbach wrote:
> > On Wed, Feb 26, 2025 at 09:03:13AM +0100, Krzysztof Kozlowski wrote:
> >> On Tue, Feb 25, 2025 at 08:46:12AM -0500, Sam Winchenbach wrote:
> >>> Adds two properties to add a margin when automatically finding the
> >>> corner frequencies.
> >>>
> >>> Signed-off-by: Sam Winchenbach <sam.winchenbach@...mepointer.org>
> >>> ---
> >>>  .../bindings/iio/filter/adi,admv8818.yaml          | 14 ++++++++++++++
> >>>  1 file changed, 14 insertions(+)
> >>
> >> Bindings are before users (see DT submitting patches), so this should be
> >> re-ordered.
> >>
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> >>> index b77e855bd594..2acdbd8d84cb 100644
> >>> --- a/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> >>> +++ b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> >>> @@ -44,6 +44,18 @@ properties:
> >>>    '#clock-cells':
> >>>      const: 0
> >>>  
> >>> +  adi,lpf-margin-hz:
> >>> +    description:
> >>> +      Sets minimum low-pass corner frequency to the frequency of rf_in plus
> >>> +      this value when in auto mode.
> >>> +    default: 0
> >>> +
> >>> +  adi,hpf-margin-hz:
> >>> +    description:
> >>> +      Sets maximum high-pass corner frequency to the frequency of rf_in minus
> >>> +      this value when in auto mode.
> >>
> >> IIUC, these are two bounds - lower and upper - in relation to something
> >> else (like rf_in frequency)? If so, make it an array (naming to be
> >> discuss, I assume you know better what's that):
> > 
> > It is true that these are both related to rf_in but both the low and high pass
> > filters can operate independently. Logically, IMO, it makes more sense to have
> 
> 
> You mean you can set only low or high pass and keep other as default?
> But what is the default then - something from reset value or "0" means
> disabled?

This value isn't setting the corner frequency of the filter, but the minimum
distance the corner must be from the fundamental frequency. So, for example,
if rf_in is 3.35 GHz and you set lpf-margin-hz to 0 then the corner frequency
will be set to 3.35 GHz because that is an exact value supported by the device.

If lpf-margin-hz is set to 30 MHz (for example), then corner frequency would be
at least 3.35 GHz + 30 MHz = 3.38 GHz.  3.49 GHz is the closest corner
frequency without going below 3.38 GHz that is supported by the device, so that
is what will be selected.

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.

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

Thank you for taking the time to go through this,
-Sam

> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists