Hello Jonathan,

Adding @Thahirally, Murtaza, maybe he can help us with some more details hardware-wise.

Unfortunately, the current mail format does not allow me to highlight the points of interest in the conversation.

-----Original Message-----
From: Jonathan Cameron <jic23@kernel.org>
Sent: Saturday, July 17, 2021 5:22 PM
To: Miclaus, Antoniu <Antoniu.Miclaus@analog.com>
Cc: linux-iio@vger.kernel.org; linux-kernel@vger.kernel.org;
devicetree@vger.kernel.org; Rob Herring [off-list ref]
Subject: Re: [PATCH v6 2/2] dt-bindings: iio: frequency: add adrf6780 doc

[External]

On Fri, 16 Jul 2021 14:42:10 +0300
Antoniu Miclaus [off-list ref] wrote:

quoted

Add device tree bindings for the ADRF6780 Upconverter.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>

oops. Would have ideally gotten to taking a close look at this before v6 :(
Sorry about that!  I don't suppose we have any other reviewers whose
knowledge of
this sort of hardware is fresher and deeper than mine?  I'd like more eyes on
the next version of this if possible!

Jonathan

quoted

---
no changes in v6
 .../bindings/iio/frequency/adi,adrf6780.yaml  | 119 ++++++++++++++++++
 1 file changed, 119 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml

quoted

diff --git

a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml
b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml

quoted

new file mode 100644
index 000000000000..65cb3bee4aca

--- /dev/null
+++

b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml

quoted

@@ -0,0 +1,119 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id:

https://urldefense.com/v3/__http://devicetree.org/schemas/iio/frequency
/adi,adrf6780.yaml*__;Iw!!A3Ni8CS0y2Y!tq2rTJBBpvfHI71YPxIn96552nFJvLYy
U6rbHeP_e5sxgwDvLPHhZ_9NMjP_lD2kj1lJ$

quoted

+$schema: https://urldefense.com/v3/__http://devicetree.org/meta-

schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!tq2rTJBBpvfHI71YPxIn96552nFJvLY
yU6rbHeP_e5sxgwDvLPHhZ_9NMjP_lJHL0noG$

quoted

+
+title: ADRF6780 Microwave Upconverter
+
+maintainers:
+  - Antoniu Miclaus [off-list ref]
+
+description: |
+   Wideband, microwave upconverter optimized for point to point

microwave

quoted

+   radio designs operating in the 5.9 GHz to 23.6 GHz frequency range.
+
+   https://www.analog.com/en/products/adrf6780.html
+
+properties:
+  compatible:
+    enum:
+      - adi,adrf6780
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 1000000
+
+  clocks:
+    description:
+      Definition of the external clock.
+    minItems: 1
+
+  clock-names:
+    items:
+      - const: lo_in
+
+  clock-output-names:
+    maxItems: 1
+
+  adi,vga-buff-en:
+    description:
+      VGA Buffer Enable.

Ideally I'd like any acronyms spelt out in the descriptions.
(I assume this is variable gain amplifier?) The fun question from looking at
this in the datasheet is where is it in the functional diagram?  I'm not actually
sure where it might be.  Perhaps in the VVA, so on the VAAT input?

If you have a convenient path to point out to your datasheet people that it is
undocumented, that might be good to clean up :)
My guess is this is needed if the precision reference on the example circuit
needs
a high impedance input, but I'm only guessing.

quoted

+    type: boolean
+
+  adi,lo-buff-en:
+    description:
+      LO Buffer Enable.

What is LO and why might it need a buffer? (or is it always a good idea to turn
this on
when using the device?)

quoted

+    type: boolean
+
+  adi,if-mode-en:
+    description:
+      IF Mode Enable.

IF stands for what? Intermediate Frequency...

quoted

+    type: boolean
+
+  adi,iq-mode-en:
+    description:
+      IQ Mode Enable.
+    type: boolean

I struggled to figure this out from the datasheet, but is there a circumstance
under which
if and iq mode might both be enabled? Nothing stops you setting the
registers that way, but
it seems to be documented as one or the other...

For that matter, why probably want to describe this as "baseband IQ mode"
given datasheet
refers to as baseband in most places other than the register definition.

quoted

+
+  adi,lo-x2-en:
+    description:
+      LO x2 Enable.
+    type: boolean
+
+  adi,lo-ppf-en:
+    description:
+      LO x1 Enable.

This is curious. I agree that the register write documents it as x1 enable, but it
seems
to be be enabling polyphase filters - what exactly their relationship is to x1 is
not that
clear to me.  Perhaps one to query with the hardware people if possible!

quoted

+    type: boolean
+
+  adi,lo-en:
+    description:
+      LO Enable.
+    type: boolean

Would you ever have this off whilst running?  It's possible I'm missing
something, but
I think you need that frequency path to be enabled to get anything at all to
happen.

quoted

+
+  adi,uc-bias-en:
+    description:
+      UC Bias Enable.
+    type: boolean

This being completely undocumented apart from the register setting, I have
0 idea what
it actually is.   Any chance we can get some more details?

quoted

+
+  adi,lo-sideband:
+    description:
+      Switch to the Other LO Sideband.

Switch what to the other LO sideband?

quoted

+    type: boolean
+
+  adi,vdet-out-en:

So my reading of the datasheet on this one didn't lead me to a completely
clear answer.
Does turning this one effectively stop you using the internal ADC to measure
the power
whilst exposing the signal on an external pin?

quoted

+    description:
+      VDET Output Select Enable.
+    type: boolean
+
+  '#clock-cells':
+    const: 0
+
+dependencies:
+  adi,lo-x2-en: [ "adi,lo-en" ]
+  adi,lo-ppf-en: [ "adi,lo-en" ]
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+
+additionalProperties: false
+
+examples:
+  - |
+    spi {
+      #address-cells = <1>;
+      #size-cells = <0>;
+      adrf6780@0 {
+        compatible = "adi,adrf6780";
+        reg = <0>;
+        spi-max-frequency = <1000000>;
+        clocks = <&adrf6780_lo>;
+        clock-names = "lo_in";
+      };
+    };
+...