Message-ID: <3mmzedwjwraepmhams5w3navb3cyga3wr7fvkrdgls2zkzdqwb@vogpd527ovgr>
Date: Mon, 22 Dec 2025 10:21:32 +0000
From: Rodrigo Alencar <455.rodrigo.alencar@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, 
	Rodrigo Alencar <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 v2 1/6] dt-bindings: iio: frequency: add adf41513

On 25/12/21 02:02PM, Krzysztof Kozlowski wrote:
> On 20/12/2025 19:05, 455.rodrigo.alencar@...il.com wrote:
> > Hi Krzystof,
> > 
> > thanks for taking a look into this again. It was my first patch it didn't want
> > to draw more attention or discussion to the V1 patch as it was declared not ready
> > at its very first review.
> > 
> > On 25/12/20 10:21AM, Krzysztof Kozlowski wrote:
> >> On Fri, Dec 19, 2025 at 12:34:48PM +0000, Rodrigo Alencar wrote:
> >>> 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).
> >>
> >> What is "existing PLL driver"? I know about motor drivers, but can you
> >> drive PLL?
> >>
> >> And how is ADF4350 related to this binding. I do not see ADF4350
> >> compatible here at all. Describe hardware, a real one.
> > 
> > ADF4350 is an older one, and its bindings can be found at:
> > Documentation/devicetree/bindings/iio/frequency/adi,adf4350.yaml
> > It is a similar part, but yet very different.
> > 
> >>
> >> Nothing improved.
> >>
> >> You ignored comments, did not bother to respond to them and then sent
> >> the same.
> > 
> > Sorry for not responding on the V1 thread, but the previous patch had to be reviewed internally
> > first. It is not true that nothing is improved, in fact, it has changed a lot, here are some notes:
> 
> Process is not like that. You first review internally, then you send.
> After you sent and receive comments, you respond to these comments.

ack.
 
> > * adi,power-up-frequency is not carrying the -hz postfix because it forces to be a uint32 by
> > the dt-bindings check. For that variable it needs to be uint64 as the part supports up to 26.5 GHz > 2^32
> 
> And what granularity do you need? Why mhz does not work?

~Hz granularity is needed to adjust frequency offsets spotted by calibration,
but I suppose that is a whim that can be dropped indeed, as the important
thing about this property is to populate frequency configs for the initialization
sequence, which requires all registers to be written.

> > * The properties related to the reference input signal path: reference-div-factor, reference-doubler-enable
> > reference-div2-enable are declared here because they are constraints for the PFD frequency definition,
> > which is the frequency that the output signal is updated, important for the loop-filter and VCO design.
> > * added support for all different power supply regulators.
> 
> Sorry, but I cannot respond that way. We discuss inline, so I have
> entire picture, not some parts of message semi-quoted here. I don't
> remember what was there and I am not going to keep looking for that.
> 
> You need to adjust to mailing list discussion style, not introduce the
> others. I have just way too many other patches to deal with, so
> implement the feedback or respond properly.

ack. Will adjust to the requested style. Already deviating from it again,
but those were the comments from the V1 review:
(https://lore.kernel.org/all/20251111-feathered-winged-bloodhound-b7e1a3@kuoka/)

> 
> Please organize the patch documenting compatible (DT bindings) before their user.
> See also: https://elixir.bootlin.com/linux/v6.14-rc6/source/Documentation/devicetree/bindings/submitting-patches.rst#L46

done.

> > +
> > +  chip-enable-gpios:
> 
> enable-gpios

done.

> > +  adi,power-up-frequency:
> > +    $ref: /schemas/types.yaml#/definitions/uint64
> 
> Use standard unit suffixes. Frequency is in Hz for example.

sorry, this was the problem, will use -mhz as suggested.

> > +  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.
> 
> Then why do you need this property? If driver calculates the optimal,
> why anyone would put wrong or sub-optimal value to DT?
> 
> Drop.

The description was bad so I rewrote this. The value is hardware constraint
for the output frequency of the Phase-Frequency Detector (PFD),
which is important for the external loop-filter/VCO design. The driver may
only change the R counter if the PFD frequency goes off limits. In that case,
some designs can acomodate different PFD frequencies, but that is not usual
and likely not recommended.

> > +
> > +        /* Example with advanced features enabled */
> > +        pll_advanced@0 {
> 
> pll@

done

thanks for the patience.

kind regards,

Rodrigo Alencar

Powered by blists - more mailing lists