| Message ID | 20210716114210.141560-2-antoniu.miclaus@analog.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Jonathan Cameron |
| Headers | show |
| Series | [v6,1/2] iio: frequency: adrf6780: add support for ADRF6780 | expand |
On Fri, 16 Jul 2021 14:42:10 +0300 Antoniu Miclaus <antoniu.miclaus@analog.com> wrote: > 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 > --- > 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 > > diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml > new file mode 100644 > index 000000000000..65cb3bee4aca > --- /dev/null > +++ b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml > @@ -0,0 +1,119 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/iio/frequency/adi,adrf6780.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: ADRF6780 Microwave Upconverter > + > +maintainers: > + - Antoniu Miclaus <antoniu.miclaus@analog.com> > + > +description: | > + Wideband, microwave upconverter optimized for point to point microwave > + 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. > + 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?) > + type: boolean > + > + adi,if-mode-en: > + description: > + IF Mode Enable. IF stands for what? Intermediate Frequency... > + 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. > + > + 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! > + 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. > + > + 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? > + > + adi,lo-sideband: > + description: > + Switch to the Other LO Sideband. Switch what to the other LO sideband? > + 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? > + 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"; > + }; > + }; > +...
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 <robh@kernel.org> > 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 <antoniu.miclaus@analog.com> wrote: > > > 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 > > > --- > > 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 > > > > diff --git > a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml > b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml > > new file mode 100644 > > index 000000000000..65cb3bee4aca > > --- /dev/null > > +++ > b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml > > @@ -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$ > > +$schema: https://urldefense.com/v3/__http://devicetree.org/meta- > schemas/core.yaml*__;Iw!!A3Ni8CS0y2Y!tq2rTJBBpvfHI71YPxIn96552nFJvLY > yU6rbHeP_e5sxgwDvLPHhZ_9NMjP_lJHL0noG$ > > + > > +title: ADRF6780 Microwave Upconverter > > + > > +maintainers: > > + - Antoniu Miclaus <antoniu.miclaus@analog.com> > > + > > +description: | > > + Wideband, microwave upconverter optimized for point to point > microwave > > + 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. > > > > + 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?) > > > + type: boolean > > + > > + adi,if-mode-en: > > + description: > > + IF Mode Enable. > IF stands for what? Intermediate Frequency... > > > + 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. > > > + > > + 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! > > > + 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. > > > + > > + 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? > > > + > > + adi,lo-sideband: > > + description: > > + Switch to the Other LO Sideband. > > Switch what to the other LO sideband? > > > + 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? > > > + 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"; > > + }; > > + }; > > +...
diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml new file mode 100644 index 000000000000..65cb3bee4aca --- /dev/null +++ b/Documentation/devicetree/bindings/iio/frequency/adi,adrf6780.yaml @@ -0,0 +1,119 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/frequency/adi,adrf6780.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: ADRF6780 Microwave Upconverter + +maintainers: + - Antoniu Miclaus <antoniu.miclaus@analog.com> + +description: | + Wideband, microwave upconverter optimized for point to point microwave + 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. + type: boolean + + adi,lo-buff-en: + description: + LO Buffer Enable. + type: boolean + + adi,if-mode-en: + description: + IF Mode Enable. + type: boolean + + adi,iq-mode-en: + description: + IQ Mode Enable. + type: boolean + + adi,lo-x2-en: + description: + LO x2 Enable. + type: boolean + + adi,lo-ppf-en: + description: + LO x1 Enable. + type: boolean + + adi,lo-en: + description: + LO Enable. + type: boolean + + adi,uc-bias-en: + description: + UC Bias Enable. + type: boolean + + adi,lo-sideband: + description: + Switch to the Other LO Sideband. + type: boolean + + adi,vdet-out-en: + 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"; + }; + }; +...