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

[Rodrigo Alencar]

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>

...

> > +  resets:
> > +    minItems: 1
> > +    maxItems: 2
> > +
> > +  reset-names:
> > +    oneOf:
> > +      - items:
> > +          - const: dev
> > +      - items:
> > +          - const: dev
> > +          - const: io
> 
> This could be a bit more clear. Does dev == MASTER_RESET pin?

Correct.

> And what about DAC_RSET?

DAC_RSET is not a reset, it is a pin for a 10K omh resistor.
 
> > +
> > +  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.

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

> > +  adi,charge-pump-current-microamp:
> > +    minimum: 212
> > +    maximum: 387
> > +    default: 387
> > +    description:
> > +      PLL charge pump current in microamps. Only applicable when the PLL
> > +      is enabled. The value is rounded to the nearest supported step.
> > +
> > +  adi,refclk-out-drive-strength:
> > +    $ref: /schemas/types.yaml#/definitions/string
> > +    enum: [ disabled, low, medium, high ]
> > +    default: disabled
> > +    description:
> > +      Reference clock output (DRV0) drive strength. Only applicable when
> > +      the PLL is enabled.
> > +
> > +  adi,reference-div2-enable:
> > +    type: boolean
> > +    description:
> > +      Enable the reference clock input divider. When enabled, the input
> > +      reference frequency is halved before deriving the system clock.
> > +      This is only applicable when the PLL is bypassed.
> > +
> > +  adi,inverse-sinc-enable:
> > +    type: boolean
> > +    description:
> > +      Enable the inverse sinc filter that compensates for the sinc roll-off
> > +      of the DAC output. When it is enabled, the filter introduces up to 3 dB
> > +      of insertion loss.
> > +
> > +  adi,sine-output-enable:
> > +    type: boolean
> > +    description:
> > +      Select sine wave output from the DDS core. When not set, the
> > +      output is a cosine wave.
> 
> And these last few are really getting into things that are typically
> controlled by the IIO driver and not hard-coded.

indeed, adi,sine-output-enable could be removed from here.

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

> > +
> > +  adi,pdclk-disable:
> > +    type: boolean
> > +    description:
> > +      Disable the parallel data clock (PDCLK) output. PDCLK runs at
> > +      one quarter of the system clock frequency.
> > +
> > +  adi,pdclk-invert:
> > +    type: boolean
> > +    description:
> > +      Invert the polarity of the PDCLK output.
> > +
> > +  adi,tx-enable-invert:
> > +    type: boolean
> > +    description:
> > +      Invert the polarity of the TX_ENABLE input pin.
> > +
> > +  adi,dac-output-current-microamp:
> > +    minimum: 8640
> > +    maximum: 31590
> > +    default: 20070
> > +    description:
> > +      DAC full-scale output current in microamps.
> > +
> > +dependencies:
> > +  adi,pll-vco-select: [ 'adi,pll-multiplier' ]
> > +  adi,charge-pump-current-microamp: [ 'adi,pll-multiplier' ]
> > +  adi,refclk-out-drive-strength: [ 'adi,pll-multiplier' ]
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - dvdd-io33-supply
> > +  - avdd33-supply
> > +  - dvdd18-supply
> > +  - avdd18-supply
> > +
> > +dependentSchemas:
> > +  resets:
> > +    properties:
> > +      reset-gpios: false
> > +  reset-gpios:
> > +    properties:
> > +      resets: false
> 
> This seems too strict. Couldn't we have some resets from a reset
> controller and others from gpios?

I suppose that would be confusing. using reset-gpios is the case where
we are NOT interfacing with an FPGA. Otherwise, using resets would be the
case when the IIO backend also implements the reset controller interface.

> Or maybe the reset bindings are enough and we don't need the gpio
> bindings for the same pin?

normally reset-gpios are enough as it can be used to instantiate a
reset controller too. I could introduce resets when adding the IIO backend
support.

-- 
Kind regards,

Rodrigo Alencar