Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   "Miclaus, Antoniu" <Antoniu.Miclaus@analog.com>
To:     Jonathan Cameron <jic23@kernel.org>
CC:     "linux-iio@vger.kernel.org" <linux-iio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
        Rob Herring <robh@kernel.org>,
        "Thahirally, Murtaza" <Murtaza.Thahirally@analog.com>
Subject: RE: [PATCH v6 2/2] dt-bindings: iio: frequency: add adrf6780 doc
Thread-Topic: [PATCH v6 2/2] dt-bindings: iio: frequency: add adrf6780 doc
Thread-Index: AQHXejersv6azsH4QE2O3EtGyv01VatHObeAgASqeIA=
Date:   Tue, 20 Jul 2021 14:05:37 +0000
Message-ID: <CY4PR03MB3399B0CC076ED48608B681649BE29@CY4PR03MB3399.namprd03.prod.outlook.com>
References: <20210716114210.141560-1-antoniu.miclaus@analog.com>
        <20210716114210.141560-2-antoniu.miclaus@analog.com>
 <20210717152147.01f0cae9@jic23-huawei>
In-Reply-To: <20210717152147.01f0cae9@jic23-huawei>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?SESOpZC8vbbyWyo9iIXHVggBoIx/p+N07JP7U8Vb2xA1IXV7RGyuheRPaj7H?=
 =?us-ascii?Q?MgLWmBZ5d+rmunF3voHrzU6uENkXCl47HFqoVXEJBWCMFNm/zFV8XYgntzV/?=
 =?us-ascii?Q?btoVhPhgolVWRni1a229aR6V1JcGJDdJdn+ksAiCkzpBPIA1ymeScgS55iIa?=
 =?us-ascii?Q?FeK4FhBgRWePRKxgnZ5Ne3Sg6Wpnnd9uCWcvIj7aPU6ZvYpz9o6JkEZcKkPL?=
 =?us-ascii?Q?OnpS72UXvKzkLY4BXVQnaYT52q8xokwq+lRcIadzLqa7llvDS+nyTy6r91vm?=
 =?us-ascii?Q?iHHw3S8WMKqyiH7lX5P6MhsP8NFTEKV7L82eyzeWEMLIAufax5hxBrFIcKeF?=
 =?us-ascii?Q?2HRtEhgASrHnzsKkAIUW45oeu6vMrfKqDiJ5BZVdb82OoY064c8WqY1Jd64b?=
 =?us-ascii?Q?kj+q6gE1PWaTo3wAYpK1z+zshtvhvVGxpv1N+bU51r/m1q5YJZhB82NxFuyg?=
 =?us-ascii?Q?z6U6fjyIqQu0vSJX7hcDcxHbZdvLZmCz3+9rU6RCsvduRWMp1xSGSH4sI/lb?=
 =?us-ascii?Q?/g/Q8dMpgHya+5rK6RUajQZcXDWT5DAqygDvSIB3x1hqBOwdJbiSkMjiivGR?=
 =?us-ascii?Q?yBvGUPBhoVqx6/8nqite72j4F60uppkzI0K2OIWWFBFOZ99vAZicKFBeMqhb?=
 =?us-ascii?Q?PUFfFS7n3673OlCFietZl9m+DDHWvil+KgftaURB7bp+MR5kIQC8ywqsRUov?=
 =?us-ascii?Q?MJ8HtCfRnDVFDeDwVQvXxBBNVMs8p8h2Bu1C2ETZ8/39HJuOCJ6+Ui3nL29P?=
 =?us-ascii?Q?iPRL6y9rHflBj9wwtPknillmNNcC6afSqywYrogjQUE51wrCt7VPAkZd9o7u?=
 =?us-ascii?Q?JdnZJOumRK4f4e2/UPNbh1/+mwnTUNQS8y90OpqvMRqrg6QYa2se983zQmz6?=
 =?us-ascii?Q?UX2cMX01xCtW+qrBEH9DBfJqceiBsO1KpINk6jzASsDoLAwO+c8UHWzbqJrO?=
 =?us-ascii?Q?XC2x7Bxj7Om6eH0YK6GIeyFC31u87tQiKVy2r6KKmlnDGhkqLEeaPbI1XrjG?=
 =?us-ascii?Q?rmdh8c2bOBTFhR1SsYG7PbQ3h/SU7Kl7q8oB+Lk2qL28LByyAYt4S36iGZB0?=
 =?us-ascii?Q?EhaRIlkSirGU3CVfym2RGE2Zx7CXGTiQ27KZLWBRx3qantjXPFEb0yiktE54?=
 =?us-ascii?Q?tP3Wla2f79qVHNyz0fpfHckoOzkAIeEaI+eByEQKk90nJ9qAhKckZjOyeLq1?=
 =?us-ascii?Q?xaZlLRG3nexVp84ALnpEH+TWoz3U6ctIUBKzdVrZ8l25V1SJakNhJishSH7P?=
 =?us-ascii?Q?+zySHI6f6O/2ZYXH5b6+dz/iuAG7bcul39ArvxOakLwG5cL0WaPSW4A2v3Dh?=
 =?us-ascii?Q?ZSbEgLon/c5nSviU3wKO8gbS?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: analog.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CY4PR03MB3399.namprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7d319fb4-6804-4f8e-ed4e-08d94b877363
X-MS-Exchange-CrossTenant-originalarrivaltime: 20 Jul 2021 14:05:37.6673
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: eaa689b4-8f87-40e0-9c6f-7228de4d754a
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 34w3Xqc4wDEsEw7U6++BjUJgOqkt/kSyyDSySEVeYxUsCnY1tIhtU8zwwBYq9jTEZM1sqq+ATTbc9s1oghoQIir12lkQJGNsg16mjPHj8Yw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR03MB2678
X-Proofpoint-GUID: KUvuyPe39rqsHkjWSguekMjQlPXgDTQR
X-Proofpoint-ORIG-GUID: KUvuyPe39rqsHkjWSguekMjQlPXgDTQR
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.790
 definitions=2021-07-20_07:2021-07-19,2021-07-20 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 suspectscore=0
 mlxlogscore=999 spamscore=0 clxscore=1015 lowpriorityscore=0 phishscore=0
 malwarescore=0 bulkscore=0 adultscore=0 mlxscore=0 impostorscore=0
 priorityscore=1501 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2104190000 definitions=main-2107200088
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello Jonathan,

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

Unfortunately, the current mail format does not allow me to highlight the p=
oints 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
>=20
> [External]
>=20
> On Fri, 16 Jul 2021 14:42:10 +0300
> Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:
>=20
> > Add device tree bindings for the ADRF6780 Upconverter.
> >
> > Reviewed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
>=20
> 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 eye=
s on
> the next version of this if possible!
>=20
> Jonathan
>=20
> > ---
> > 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.
>=20
> 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?
>=20
> 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 circ=
uit
> needs
> a high impedance input, but I'm only guessing.
>=20
>=20
> > +    type: boolean
> > +
> > +  adi,lo-buff-en:
> > +    description:
> > +      LO Buffer Enable.
>=20
> What is LO and why might it need a buffer? (or is it always a good idea t=
o turn
> this on
> when using the device?)
>=20
> > +    type: boolean
> > +
> > +  adi,if-mode-en:
> > +    description:
> > +      IF Mode Enable.
> IF stands for what? Intermediate Frequency...
>=20
> > +    type: boolean
> > +
> > +  adi,iq-mode-en:
> > +    description:
> > +      IQ Mode Enable.
> > +    type: boolean
>=20
> I struggled to figure this out from the datasheet, but is there a circums=
tance
> 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...
>=20
> 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.
>=20
> > +
> > +  adi,lo-x2-en:
> > +    description:
> > +      LO x2 Enable.
> > +    type: boolean
> > +
> > +  adi,lo-ppf-en:
> > +    description:
> > +      LO x1 Enable.
>=20
> This is curious. I agree that the register write documents it as x1 enabl=
e, 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!
>=20
> > +    type: boolean
> > +
> > +  adi,lo-en:
> > +    description:
> > +      LO Enable.
> > +    type: boolean
>=20
> 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.
>=20
> > +
> > +  adi,uc-bias-en:
> > +    description:
> > +      UC Bias Enable.
> > +    type: boolean
>=20
> This being completely undocumented apart from the register setting, I hav=
e
> 0 idea what
> it actually is.   Any chance we can get some more details?
>=20
> > +
> > +  adi,lo-sideband:
> > +    description:
> > +      Switch to the Other LO Sideband.
>=20
> Switch what to the other LO sideband?
>=20
> > +    type: boolean
> > +
> > +  adi,vdet-out-en:
>=20
> 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 meas=
ure
> the power
> whilst exposing the signal on an external pin?
>=20
> > +    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 =3D <1>;
> > +      #size-cells =3D <0>;
> > +      adrf6780@0 {
> > +        compatible =3D "adi,adrf6780";
> > +        reg =3D <0>;
> > +        spi-max-frequency =3D <1000000>;
> > +        clocks =3D <&adrf6780_lo>;
> > +        clock-names =3D "lo_in";
> > +      };
> > +    };
> > +...