Message-ID: <aRXbaMGbxdbwgUhi@debian-BULLSEYE-live-builder-AMD64>
Date: Thu, 13 Nov 2025 10:21:44 -0300
From: Marcelo Schmitt <marcelo.schmitt1@...il.com>
To: rodrigo.alencar@...log.com
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-doc@...r.kernel.org,
	Jonathan Cameron <jic23@...nel.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 2/3] dt-bindings: iio: frequency: add adf41513

Hi Rodrigo,

Many comments inline in addition to what Krzysztof has already mentioned.

On 11/10, Rodrigo Alencar via B4 Relay wrote:
> From: Rodrigo Alencar <rodrigo.alencar@...log.com>
> 
> ultralow noise PLL frequency synthesizer that can be used to
> implement local oscillators (LOs) as high as 26.5 GHz
> 
> Signed-off-by: Rodrigo Alencar <rodrigo.alencar@...log.com>
> ---
>  .../bindings/iio/frequency/adi,adf41513.yaml       | 268 +++++++++++++++++++++
>  MAINTAINERS                                        |   1 +
>  2 files changed, 269 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..7e1ad80d68af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml
> @@ -0,0 +1,268 @@
> +# 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
> +
> +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.
> +
> +  '#clock-cells':
> +    const: 0
> +
> +  clock-output-names:
> +    maxItems: 1
> +
> +  vcc-supply:
> +    description: Power supply for the device (3.3V)
I see we can have AVDD1 == ... == AVDD5 == VP == 3.3V.
But we should document them all here in case somebody wants to use a separate
supply for any of those.

> +
> +  chip-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.
> +    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) on driver probe.
> +      Range is 1 GHz to 26.5 GHz for ADF41513, and 1 GHz to 10 GHz for ADF41510.
The PLL settings are also controllable at runtime and independent of hw
connections, right? Can't this be just a sw/driver default instead of a dt
property?

> +
> +  adi,reference-div-factor:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 1
> +    maximum: 32
> +    description:
> +      Reference division factor (R Counter). If not specified, the driver
> +      will calculate the optimal value automatically.
> +
> +  adi,reference-doubler-enable:
> +    description:
> +      Enables the reference doubler. The maximum reference frequency when
> +      the doubler is enabled is 225 MHz.
> +    type: boolean
This also seems like runtime configuration. At first glance, my feeling is that
it impacts the output frequency and thus should probably be somehow handled
in adf41513_write() IIO_CHAN_INFO_FREQUENCY case. By the way, if IIO_VAL_INT_64
format allows setting the frequency resolution in sub-Hz (see comment in driver file),
then I also suggest to use usual IIO _raw interfaces, e.g.

static const struct iio_info adf41513_info = {
  .write_raw = adf41513_write(),
	.write_raw_get_fmt = adf41513_write_raw_get_fmt(),
	.debugfs_reg_access = &adf41513_reg_access,
};

> +
> +  adi,reference-div2-enable:
> +    description:
> +      Enables the reference divide-by-2 function. This provides a 50%
> +      duty cycle signal to the PFD.
> +    type: boolean
This one also seems to be associated with output frequency and runtime
configurable, no?

> +
> +  adi,charge-pump-current-microamp:
> +    minimum: 450
> +    maximum: 7200
> +    default: 2400
> +    description:
> +      Charge pump current in microamps. The value will be rounded to the
> +      nearest supported value.
> +
> +  adi,charge-pump-resistor-ohms:
> +    minimum: 1800
> +    maximum: 10000
> +    default: 2700
> +    description:
> +      External charge pump resistor value in ohms. This sets the maximum
> +      charge pump current along with the charge pump current setting.

hmm these charge-pump props seem a bit tricky. IIUC, the achievable charge pump
output currents depend on the charge pump resistor value and the cp output
current would be something that a user would latter want to fine tune at
runtime. I'd keep adi,charge-pump-resistor-ohms only, pick a default for the
pump current, then provide an IIO attribute to allow tweaking the charge pump
current for fine tuning the loop filter frequency response. This might need
a new IIO ABI.

> +
> +  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]

I don't think this should be a dt property. The mux output can be controlled at
runtime by updating register 12. Also, if somebody sets, for example,
'muxout_high' here, then the user would not be able to set the mux output to
something else latter? Would not be able to output the configured RF output
frequency for the example?

> +
> +  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-detect-precision:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 3
> +    description:
> +      Lock detector precision setting. Controls the sensitivity of the
> +      lock detector. Lower values of precision increases the lock detector
> +      window size.
This sounds more like something that could be supported through an IIO device
attribute.

> +
> +  adi,lock-detect-count:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 7
> +    description: |
> +      Lock detector count setting (3-bit value). Determines the number of
> +      consecutive phase detector cycles that must be within the lock detector
> +      window before lock is declared. The count grows in powers of two of the
> +      programmed value:
> +      - if adi,fast-lock-enable is set count = 2 * 2^value
> +      - if adi,fast-lock-enable is not set count = 64 * 2^value
> +
> +  adi,lock-detect-bias-microamp:
> +    description:
> +      Lock detector bias current. Controls the lock detector window size
> +      along with the lock detector precision setting. Lower bias current
> +      increases the window size.
> +    enum: [10, 20, 30, 40]
> +
> +  adi,fast-lock-enable:
> +    description:
> +      Enable fast lock mode. This changes the lock detector clock selection
> +      for faster lock indication.
> +    type: boolean
adi,lock-detect-count, adi,lock-detect-bias-microamp, and adi,fast-lock-enable
also sound like they could be IIO device properties.

> +
> +  adi,phase-resync-enable:
> +    description:
> +      Enable phase resync functionality. This produces a consistent output
> +      phase offset with respect to the input reference.
> +    type: boolean
IIUC, this would work similarly to the description of out_altvoltageY_phase ABI,
except the phase would be relative to the reference clock instead of a second
output channel. New IIO ABI? Or, maybe, provide out_altvoltage0_frequency and
out_altvoltage1_frequency? One of the altvoltageY would be the frequency of
muxout while the other would be the frequency of the reference clock. Then phase
resync would be supported through out_altvoltageY_phase. Not sure if having
a "virtual" output channel would be misleading, though. Extending the
out_altvoltageY_phase ABI might be an alternative.

> +
> +  adi,12bit-clk-divider:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 4095
> +    description:
> +      CLK1 divider value used when adi,phase-resync-enable is set
> +
> +  adi,12bit-clk2-divider:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    minimum: 0
> +    maximum: 4095
> +    description:
> +      CLK2 divider value used when adi,phase-resync-enable is set
CLK1 and CLK2 would be calculated by the driver according to the value set
through out_altvoltageY_phase (if the idea above makes sense). Then
adi,12bit-clk-divider and adi,12bit-clk2-divider would also not be needed.

> +
> +  adi,le-sync-enable:
> +    description:
> +      Synchronize the rising edge of LE on an SPI write with the falling
> +      edge of the reference signal to prevent glitches.
> +    type: boolean
> +
> +  adi,freq-resolution:
> +    $ref: /schemas/types.yaml#/definitions/uint64
> +    minimum: 1
> +    default: 1000000
> +    description:
> +      Initial frequency resolution in micro-Hz (µHz) for the algorithm to achieve.
> +      This influences the choice between fixed and variable modulus modes.
> +      Default is 1000000 µHz (1 Hz).
This one also looks like it could be a driver default instead of a dt prop.


Best regards,
Marcelo

Powered by blists - more mailing lists