Re: [PATCH RFC 1/8] dt-bindings: iio: frequency: add ad9910

[Conor Dooley]

On Sun, Feb 22, 2026 at 10:47:41AM +0000, Rodrigo Alencar wrote:
> On 26/02/21 02:43PM, David Lechner wrote:
> > On 2/20/26 10:46 AM, Rodrigo Alencar via B4 Relay wrote:
> > > From: Rodrigo Alencar <rodrigo.alencar@xxxxxxxxxx>

> > > +
> > > +  reset-gpios:
> > > +    maxItems: 2
> > > +    description:
> > > +      GPIOs controlling the device reset and the I/O_RESET pins. This is only
> > > +      used if resets property is not defined.
> > > +
> > > +  powerdown-gpios:
> > > +    maxItems: 1
> > > +    description:
> > > +      GPIO controlling the EXT_PWR_DWN pin.
> > > +
> > > +  update-gpios:
> > > +    maxItems: 1
> > > +    description:
> > > +      GPIO controlling the I/O_UPDATE pin.
> > > +
> > > +  profile-gpios:
> > > +    minItems: 3
> > > +    maxItems: 3
> > > +    description:
> > > +      GPIOs controlling the PROFILE[2:0] pins for profile selection.
> > > +
> > 
> > Looks like possibly some interrupts as well: RAM_SWP_OVR and SYNC_SMP_ERR
> 
> Interrupts are not handled by the driver at this point, so they were not added
> here. The device is meant to have some features exposed through SPI, but to
> extract the most of it needs to interface with an FPGA. For that, an IIO
> backend is in the works.

The binding should be complete, if there are interrupts then document
them even if the driver doesn't use them. If this is where you say you
mentioned the io-backends, that's not what I meant - you need to do it
in the patch itself to explain why you have the odd setup with resets
and reset-gpios.
That said, if you don't know what the io-backends stuff is going to look
like for this device, you should probably exclude those resets until the
design for the io-backend IP is complete. reset-gpios are usually
optional anyway (since they are often just wired high/low), so there
shouldn't be any downside there.

> > > +  adi,pll-multiplier:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    minimum: 12
> > > +    maximum: 127
> > > +    description:
> > > +      PLL feedback divider value (N). The system clock frequency is
> > > +      REFCLK * N. When not specified, the PLL is bypassed.
> > 
> > What determines the value that should be selected here? This doesn't seem like
> > something we would normally put in the devicetree. (Smells like configuration
> > rather than describing what is wired up.)
> 
> The sysclk frequency is something we want fixed because it derives the sync_clk
> and pd_clk which can be constraints for FPGA desing or when interfacing with

btw, the datasheet for this device (rev e) says:
| SYNC_CLK is a rising edge active signal. It is derived from the
| system clock and a divide-by-4 frequency divider. SYNC_CLK,
| which is externally provided, can be used to synchronize external
| hardware to the AD9910 internal clocks.

It's not externally provided, so the third sentence here is confusing.

> the parallel port (high-speed up to 250 MHz). Also, when using the PLL path to
> derive the sysclk, the external loop filter will pretty much define the values
> to be used here, as it would be optimized for a certain frequency range.
> 
> > > +
> > > +  adi,pll-vco-select:
> > > +    $ref: /schemas/types.yaml#/definitions/uint32
> > > +    minimum: 0
> > > +    maximum: 5
> > > +    description: |
> > > +      VCO frequency range selection (0-5). When not specified and the PLL
> > > +      is enabled, the VCO range is automatically selected based on the
> > > +      computed system clock frequency. Typical VCO frequency ranges are:
> > > +      - Range 0: 370 MHz to 510 MHz (Auto-selected when <= 465 MHz)
> > > +      - Range 1: 420 MHz to 590 MHz (Auto-selected when > 465 MHz and <= 545 MHz)
> > > +      - Range 2: 500 MHz to 700 MHz (Auto-selected when > 545 MHz and <= 650 MHz)
> > > +      - Range 3: 600 MHz to 880 MHz (Auto-selected when > 650 MHz and <= 790 MHz)
> > > +      - Range 4: 700 MHz to 950 MHz (Auto-selected when > 790 MHz and <= 885 MHz)
> > > +      - Range 5: 820 MHz to 1050 MHz (Auto-selected when > 885 MHz)
> > > +
> > 
> > Same here. How does this depend on how the chip is wired up?
> 
> That applies to when the PLL path is used. FPGA interface and loop filter design.
> I can write those details as part of the description.

This property definitely needs justification given that it looks like
the device will just do the right thing without the property.

> > > +
> > > +  adi,sync-clk-disable:
> > > +    type: boolean
> > > +    description:
> > > +      Disable the SYNC_CLK output pin. SYNC_CLK runs at one quarter
> > > +      of the system clock frequency.
> > 
> > Clock outputs should be described as clock-controller and #clock-cells.
> > The actual enabling/disabling can be done at runtime.
> 
> I thought of that, but when interfacing with an FPGA, the clock consumer
> will be the IIO backend itself, which this device driver would depend on.
> It would create a cyclic dependency during the probe of the drivers:
> - This device being a clock provider and an IIO backend consumer
> - The FPGA IP being a IIO backend provider and a clock consumer.
> 
> This would be just save some power when not interfacing with an FPGA,
> there would not be a clock consumer to get the clock disabled.
> Normally, clock consumers would want to have clock enabled, which is
> already the case by default.
> 
> I would add the FPGA/IIO backend support in a separate patch series,
> as it would bring more stuff here.

Honestly, this binding design seems so tightly integrated with the
io-backend that you're shooting yourself in the foot, and making it hard
for us to review, by not including it. I think this really should be a
clock provider and the io-backend a clock consumer, even if it causes a
cyclical dependency. One of the two can probably be a post-init-provider
to the other, probably something like sync_clk doesn't need to be set at
probe, and setting the rate can be delayed until we start needing data
from the device? Since both sync_clk and pdclk are provided by the
ad9910 to the io-backend, your driver will have control over what the
rates of these clocks are and you shouldn't need to hard code the
multipliers etc. You just need to know if the parallel bus is in use or
not. I'm not sure if that's a given if there's an io-backend and you're
not on a serial bus or if would need a property to identify.

Most of the clocking related properties here seem like they go away if
you make the device a clock-controller, for example the driver should be
able to decide if the needs to enable reference-div2 to achieve the
internal frequencies that it needs. sync-clk-disable would be done
automatically if there's no consumer of sync_clk etc.

Attachment: signature.asc
Description: PGP signature