Message-ID: <2tpnfgcrr4k27ghdz4ciwkoekf3gof4l4rnp5xr7ukxi37ouls@c33ey3kytevc>
Date: Sun, 21 Dec 2025 19:45:11 +0000
From: Rodrigo Alencar <455.rodrigo.alencar@...il.com>
To: Jonathan Cameron <jic23@...nel.org>, 
	Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@...nel.org>
Cc: rodrigo.alencar@...log.com, linux-kernel@...r.kernel.org, 
	linux-iio@...r.kernel.org, devicetree@...r.kernel.org, linux-doc@...r.kernel.org, 
	David Lechner <dlechner@...libre.com>, Andy Shevchenko <andy@...nel.org>, 
	Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>, Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH v2 1/6] dt-bindings: iio: frequency: add adf41513


Hi Jonathan, thanks for your time.

On 25/12/21 04:59PM, Jonathan Cameron wrote:
> On Fri, 19 Dec 2025 12:34:48 +0000
> Rodrigo Alencar via B4 Relay <devnull+rodrigo.alencar.analog.com@...nel.org> wrote:
> 
> > From: Rodrigo Alencar <rodrigo.alencar@...log.com>
> > 
> > dt-bindings for ADF41513, an ultralow noise PLL frequency synthesizer that
> > can be used to implement local oscillators (LOs) as high as 26.5 GHz.
> > Most properties refer to existing PLL driver properties (e.g. ADF4350).
> 
> "Refer" implies a cross reference in this document.   Based upon is probably a better
> way to put this.
> 
> Otherwise I've mostly commented on properties that to me don't sound like
> they belong in the dt-binding as they are policy things that we want
> to make runtime configurable.
> 
> Thanks,
> 
> Jonathan
> 
> > 
> > Signed-off-by: Rodrigo Alencar <rodrigo.alencar@...log.com>
> > ---
> >  .../bindings/iio/frequency/adi,adf41513.yaml       | 246 +++++++++++++++++++++
> >  MAINTAINERS                                        |   7 +
> >  2 files changed, 253 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml b/Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml
> > new file mode 100644
> > index 000000000000..01ceb2a7d21b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml
> > @@ -0,0 +1,246 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/frequency/adi,adf41513.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADF41513 PLL Frequency Synthesizer
> > +
> > +maintainers:
> > +  - Rodrigo Alencar <rodrigo.alencar@...log.com>
> > +
> > +description:
> > +  The ADF41513 is an ultralow noise frequency synthesizer that can be used to
> > +  implement local oscillators (LOs) as high as 26.5 GHz in the upconversion and
> > +  downconversion sections of wireless receivers and transmitters. The ADF41510
> > +  supports frequencies up to 10 GHz.
> > +
> > +  https://www.analog.com/en/products/adf41513.html
> > +  https://www.analog.com/en/products/adf41510.html
> > +
> > +$ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adf41510
> > +      - adi,adf41513
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 25000000
> > +
> > +  clocks:
> > +    maxItems: 1
> > +    description: Clock that provides the reference input frequency.
> > +
> > +  avdd1-supply:
> > +    description: PFD and Up and Down Digital Driver Power Supply (3.3 V)
> > +
> > +  avdd2-supply:
> > +    description: RF Buffer and Prescaler Power Supply (3.3 V)
> > +
> > +  avdd3-supply:
> > +    description: N Divider Power Supply (3.3 V)
> > +
> > +  avdd4-supply:
> > +    description: R Divider and Lock Detector Power Supply (3.3 V)
> > +
> > +  avdd5-supply:
> > +    description: Sigma-Delta Modulator and SPI Power Supply (3.3 V)
> > +
> > +  vp-supply:
> > +    description: Charge Pump Power Supply (3.3 V)
> > +
> > +  enable-gpios:
> > +    description:
> > +      GPIO that controls the chip enable pin. A logic low on this pin
> > +      powers down the device and puts the charge pump output into
> > +      three-state mode.
> > +    maxItems: 1
> > +
> > +  lock-detect-gpios:
> > +    description:
> > +      GPIO for lock detect functionality. When configured for digital lock
> > +      detect, this pin will output a logic high when the PLL is locked.
> 
> This seems to be one potential use of the muxout pin.  So to me feels like
> a policy decision that belongs with the driver or userspace, not in dt.
> mux-out-gpios:
> would make more sense to me.
> Some of the potential settings probably don't make sense but then we just
> don't support those in the driver if this is connected to a gpio.
 
Actually, there is the DLD pin (separate pin from muxout) dedicated for lock detection.

> > +    maxItems: 1
> > +
> > +  adi,power-up-frequency:
> > +    $ref: /schemas/types.yaml#/definitions/uint64
> > +    minimum: 1000000000
> > +    maximum: 26500000000
> > +    default: 10000000000
> > +    description:
> > +      The PLL tunes to this frequency (in Hz) during the initialization
> > +      sequence. This property should be set to a frequency supported by the
> > +      loop filter and VCO used in the design. Range is 1 GHz to 26.5 GHz for
> > +      ADF41513, and 1 GHz to 10 GHz for ADF41510.
> 
> Why is this in DT?  Feels like this should be done by userspace control
> prior to setting an enable rather than being in DT.

The initialization sequence of the PLL requires the write of all the registers.
This property exist so that meaningful values are written at start up.
Here it is desired to have a frequency value that is within the range supported
by the loop-filter and the external VCO.  
 
> > +
> > +  adi,reference-div-factor:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 1
> > +    maximum: 32
> > +    default: 1
> > +    description:
> > +      Value for the reference division factor (R Counter). The driver will
> > +      increment R Counter as needed to achieve a PFD frequency within the
> > +      allowed range. High R counter values will reduce the PFD frequency, which
> > +      lowers the frequency resolution, and affects phase noise performance.
> 
> Why is this in DT?  Is there not a 'best' choice to be made given a particular
> input frequency and desired output frequency?

When designing the external circuitry to the PLL, customers often refers to
https://www.analog.com/en/lp/resources/adisimpll.html
which is a piece of software that assists the choice of some parts and
simulates performance metrics. After having the suggested design simulated,
it turns out that the reference signal path defines the output frequency
of the Phase-Frequency Detector (PFD), which is set as a constraint for
the loop-filter design. Therefore, those properties (reference-div-factor,
reference-doubler-enable and reference-div2-enable) are meant to reflect
a hardware constraint for a PFD frequency value expected by the loop-filter/VCO

> > +
> > +  adi,reference-doubler-enable:
> > +    description:
> > +      Enables the reference doubler. The maximum reference frequency when
> > +      the doubler is enabled is 225 MHz.
> As above.

explained above as part of a hardware constraint for external circuitry design

> > +    type: boolean
> > +
> > +  adi,reference-div2-enable:
> > +    description:
> > +      Enables the reference divide-by-2 function. This provides a 50%
> > +      duty cycle signal to the PFD.
> As above.

same

> > +    type: boolean
> > +
> > +  adi,charge-pump-resistor-ohms:
> > +    minimum: 1800
> > +    maximum: 10000
> > +    default: 2700
> > +    description:
> > +      External charge pump resistor (R_SET) value in ohms. This sets the maximum
> > +      charge pump current along with the charge pump current setting.
> > +
> > +  adi,charge-pump-current-microamp:
> > +    description:
> > +      Charge pump current (I_CP) in microamps. The value will be rounded to the
> > +      nearest supported value. Range of acceptable values depends on the
> > +      charge pump resistor value, such that 810 mV <= I_CP * R_SET <= 12960 mV.
> > +      This value depends on the loop filter design.
> > +
> > +  adi,muxout-select:
> > +    description:
> > +      On chip multiplexer output selection.
> > +      high_z - MUXOUT Pin set to high-Z. (default)
> > +      muxout_high - MUXOUT Pin set to high.
> > +      muxout_low - MUXOUT Pin set to low.
> > +      f_div_rclk - MUXOUT Pin set to R divider output
> > +      f_div_nclk - MUXOUT Pin set to N divider output
> > +      lock_detect - MUXOUT Pin set to Digital lock detect
> > +      serial_data - MUXOUT Pin set to Serial data output
> > +      readback - MUXOUT Pin set to Readback mode
> > +      f_div_clk1 - MUXOUT Pin set to CLK1 divider output
> > +      f_div_rclk_2 - MUXOUT Pin set to R divider/2 output
> > +      f_div_nclk_2 - MUXOUT Pin set to N divider/2 output
> > +    enum: [high_z, muxout_high, muxout_low, f_div_rclk, f_div_nclk, lock_detect,
> > +           serial_data, readback, f_div_clk1, f_div_rclk_2, f_div_nclk_2]
> 
> This needs explanation of 'why' it should be in DT?  To me it seems mostly
> to be a debug feature that should be controlled perhaps via a debugfs interface.

There has been a discussion on that, and I understand the concerns.
Altough it looks like a debug pin, I suppose some board designs might
expect the muxout pin to be working as a specific function.
The device driver provides register access through the debugfs interface,
so worst case, the muxout select value could be changed there.

> > +
> > +  adi,muxout-level-1v8-enable:
> > +    description:
> > +      Set MUXOUT and DLD logic levels to 1.8V. Default is 3.3V.
> > +    type: boolean
> > +
> > +  adi,phase-detector-polarity-positive-enable:
> > +    description:
> > +      Set phase detector polarity to positive. Default is negative.
> > +      Use positive polarity with non-inverting loop filter and VCO with
> > +      positive tuning slope, or with inverting loop filter and VCO with
> > +      negative tuning slope.
> > +    type: boolean
> > +
> > +  adi,lock-detector-count:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    default: 64
> > +    description:
> > +      Sets the value for Lock Detector count of the PLL, which determines the
> > +      number of consecutive phase detector cycles that must be within the lock
> > +      detector window before lock is declared. Lower values increase the lock
> > +      detection sensitivity.
> > +    enum: [2, 4, 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, 8192]
> 
> Sounds like policy.  Maybe it is related to the circuit design and there
> is a right choice for  particular board? If not we should figure out how to leave
> this to userspace control.  Probably as some form of event property.

There was also a internal discussion around that. I'd say that this might
be related to the application where the design is being used. There should
be value on making it configurable at runtime, mostly for tunning for
specific needs, but this is not a value to be changed at runtime in
field-deployed products.

kind regards,

Rodrigo Alencar

Powered by blists - more mailing lists