RE: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc

[Miclaus, Antoniu]

Hello Jonathan,

--
Antoniu Miclăuş

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Friday, November 12, 2021 7:46 PM
> To: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>
> Cc: robh+dt@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH 3/4] dt-bindings:iio:filter: add admv8818 doc
> 
> [External]
> 
> On Tue, 9 Nov 2021 14:31:26 +0200
> Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx> wrote:
> 
> > Add device tree bindings for the ADMV8818 Filter.
> >
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > ---
> >  .../bindings/iio/filter/adi,admv8818.yaml     | 78 +++++++++++++++++++
> >  1 file changed, 78 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> >
> > diff --git
> a/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> > new file mode 100644
> > index 000000000000..d581e236dbdc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/filter/adi,admv8818.yaml
> > @@ -0,0 +1,78 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id:
> https://urldefense.com/v3/__http://devicetree.org/schemas/iio/filter/adi,a
> dmv8818.yaml*__;Iw!!A3Ni8CS0y2Y!qkKokhmcgS0YEIy3uC6OfOOF7Bq3yE_r
> Ny91yIkDRTXFe54x-cHq_BtsyzDOedLohB5D$
> > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta-
> schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!qkKokhmcgS0YEIy3uC6OfOOF7Bq3
> yE_rNy91yIkDRTXFe54x-cHq_BtsyzDOeYdHtx0a$
> > +
> > +title: ADMV8818 Digitally Tunable, High-Pass and Low-Pass Filter
> > +
> > +maintainers:
> > +  - Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> > +
> > +description: |
> > +    Fully monolithic microwave integrated circuit (MMIC) that
> > +    features a digitally selectable frequency of operation.
> > +    The device features four independently controlled high-pass
> > +    filters (HPFs) and four independently controlled low-pass filters
> > +    (LPFs) that span the 2 GHz to 18 GHz frequency range.
> > +
> > +    https://www.analog.com/en/products/admv8818.html
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,admv8818
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 10000000
> > +
> > +  clocks:
> > +    description:
> > +      Definition of the external clock.
> > +    minItems: 1
> > +
> > +  clock-names:
> > +    items:
> > +      - const: "rf_in"
> 
> Is this what we'd normally think of as a clock signal?  I'd not expect
> a nice squarewave on that pin for example so this seems an odd way to
> define it.
> 
The only actual use of this part, until now, was to filter the output of the following part:
https://www.analog.com/en/products/adf5610.html
This is the reason of using the clock framework in the driver. Moreover, the clock input is
optional inside the driver.
> > +
> > +  clock-output-names:
> > +    maxItems: 1
> > +
> > +  adi,bw-hz:
> > +    description:
> > +      Allows the user to increase the Bandpass Filter (BPF) bandwidth
> > +      in Hz. Normally when invoked by the clk notifier, the driver
> > +      sets the HPF cutoff close below the frequency and the LPF cutoff
> > +      close above the frequency, and thus creating a BPF.
> 
> I don't understand this item at all.  Why do we need a control to
> basically change how the other filter parameters are expressed?
> 

Indeed, this property was requested by the users of the application in which this part was involved.
Same goes for the filter modes and the bandwidth in the ABI documentation.

If you think these attributes/properties are way too custom, I can drop them.

Let me know your thoughts.
> > +    $ref: /schemas/types.yaml#/definitions/uint64
> > +
> > +  '#clock-cells':
> > +    const: 0
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +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,bw-hz = /bits/ 64 <600000000>;
> > +      };
> > +    };
> > +...
> > +