diff mbox series

[v4,1/3] dt-bindings: iio: adf4371: add refin mode

Message ID 20250109133707.3845-2-antoniu.miclaus@analog.com (mailing list archive)
State Changes Requested
Headers show
Series ADF4371 refin mode and doubler support | expand

Commit Message

Antoniu Miclaus Jan. 9, 2025, 1:37 p.m. UTC
Add support for selecting between single-ended and differential
reference input.

By default the single-ended input is enabled.

Input frequency boundaries are change based on the mode selected
(single-ended/differential).

Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
---
changes in v4:
 - add enum in clock-names to support both se and diff input.
 .../devicetree/bindings/iio/frequency/adf4371.yaml         | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Jonathan Cameron Jan. 12, 2025, 3:01 p.m. UTC | #1
On Thu, 9 Jan 2025 15:37:05 +0200
Antoniu Miclaus <antoniu.miclaus@analog.com> wrote:

> Add support for selecting between single-ended and differential
> reference input.
> 
> By default the single-ended input is enabled.
> 
> Input frequency boundaries are change based on the mode selected
> (single-ended/differential).
> 
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> ---
> changes in v4:
>  - add enum in clock-names to support both se and diff input.
>  .../devicetree/bindings/iio/frequency/adf4371.yaml         | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/frequency/adf4371.yaml b/Documentation/devicetree/bindings/iio/frequency/adf4371.yaml
> index 1cb2adaf66f9..9ad5b22c59d7 100644
> --- a/Documentation/devicetree/bindings/iio/frequency/adf4371.yaml
> +++ b/Documentation/devicetree/bindings/iio/frequency/adf4371.yaml
> @@ -30,8 +30,11 @@ properties:
>  
>    clock-names:
>      description:
> -      Must be "clkin"
> -    maxItems: 1
> +      Must be "clkin" if the input reference is single ended or "clkin-diff"
> +      if the input reference is differential. By default single ended input is
> +      applied.
> +    enum: [clkin, clkin-diff]
> +    default: clkin
Given the property is required, what does a default mean?

I don't think you want to apply a default, or mention it in the description.
Otherwise LGTM.

>  
>    adi,mute-till-lock-en:
>      type: boolean
Krzysztof Kozlowski Jan. 13, 2025, 8:37 a.m. UTC | #2
On Thu, Jan 09, 2025 at 03:37:05PM +0200, Antoniu Miclaus wrote:
>    clock-names:
>      description:
> -      Must be "clkin"
> -    maxItems: 1
> +      Must be "clkin" if the input reference is single ended or "clkin-diff"
> +      if the input reference is differential. By default single ended input is
> +      applied.
> +    enum: [clkin, clkin-diff]
> +    default: clkin

Which pins are these? I went through adf4371 datasheet and no reference
on clock inputs like clkin or clkin-diff.

Best regards,
Krzysztof
Nuno Sá Jan. 13, 2025, 9:55 a.m. UTC | #3
On Mon, 2025-01-13 at 09:37 +0100, Krzysztof Kozlowski wrote:
> On Thu, Jan 09, 2025 at 03:37:05PM +0200, Antoniu Miclaus wrote:
> >    clock-names:
> >      description:
> > -      Must be "clkin"
> > -    maxItems: 1
> > +      Must be "clkin" if the input reference is single ended or "clkin-
> > diff"
> > +      if the input reference is differential. By default single ended input
> > is
> > +      applied.
> > +    enum: [clkin, clkin-diff]
> > +    default: clkin
> 
> Which pins are these? I went through adf4371 datasheet and no reference
> on clock inputs like clkin or clkin-diff.
> 
> 

Hmm, I guess we should call this 'refp' and 'refp-n' then (the latter seems a
bit more odd)? Or just 'ref' and 'ref-diff'?

- Nuno Sá
Antoniu Miclaus Jan. 13, 2025, 10:08 a.m. UTC | #4
--
Antoniu Miclăuş

> -----Original Message-----
> From: Nuno Sá <noname.nuno@gmail.com>
> Sent: Monday, January 13, 2025 11:56 AM
> To: Krzysztof Kozlowski <krzk@kernel.org>; Miclaus, Antoniu
> <Antoniu.Miclaus@analog.com>
> Cc: jic23@kernel.org; robh@kernel.org; conor+dt@kernel.org; linux-
> iio@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-pwm@vger.kernel.org
> Subject: Re: [PATCH v4 1/3] dt-bindings: iio: adf4371: add refin mode
> 
> [External]
> 
> On Mon, 2025-01-13 at 09:37 +0100, Krzysztof Kozlowski wrote:
> > On Thu, Jan 09, 2025 at 03:37:05PM +0200, Antoniu Miclaus wrote:
> > >    clock-names:
> > >      description:
> > > -      Must be "clkin"
> > > -    maxItems: 1
> > > +      Must be "clkin" if the input reference is single ended or "clkin-
> > > diff"
> > > +      if the input reference is differential. By default single ended input
> > > is
> > > +      applied.
> > > +    enum: [clkin, clkin-diff]
> > > +    default: clkin
> >
> > Which pins are these? I went through adf4371 datasheet and no reference
> > on clock inputs like clkin or clkin-diff.
> >
> >
> 
> Hmm, I guess we should call this 'refp' and 'refp-n' then (the latter seems a
> bit more odd)? Or just 'ref' and 'ref-diff'?

Regarding both naming and approach, I followed Jonathan's suggestion in v3.
Initially it was a separate property for refin mode (see previous versions).
If he is fine with changing again the name/approach I'll do it, but I'd wait for
his comment.

> - Nuno Sá
Krzysztof Kozlowski Jan. 13, 2025, 10:21 a.m. UTC | #5
On 13/01/2025 10:55, Nuno Sá wrote:
> On Mon, 2025-01-13 at 09:37 +0100, Krzysztof Kozlowski wrote:
>> On Thu, Jan 09, 2025 at 03:37:05PM +0200, Antoniu Miclaus wrote:
>>>    clock-names:
>>>      description:
>>> -      Must be "clkin"
>>> -    maxItems: 1
>>> +      Must be "clkin" if the input reference is single ended or "clkin-
>>> diff"
>>> +      if the input reference is differential. By default single ended input
>>> is
>>> +      applied.
>>> +    enum: [clkin, clkin-diff]
>>> +    default: clkin
>>
>> Which pins are these? I went through adf4371 datasheet and no reference
>> on clock inputs like clkin or clkin-diff.
>>
>>
> 
> Hmm, I guess we should call this 'refp' and 'refp-n' then (the latter seems a
> bit more odd)? Or just 'ref' and 'ref-diff'?

That mistake was done at the beginning - the "clkin" is just useless
name. It cannot be "clkout" and it cannot be anything else than clk, so
it is 100% redundant.

But looking for pins brought second point - here you claim these are
mutually exclusive while datasheet suggests that both inputs can be
connected. Unless they come from the same source always?

Best regards,
Krzysztof
Nuno Sá Jan. 13, 2025, 11:17 a.m. UTC | #6
On Mon, 2025-01-13 at 11:21 +0100, Krzysztof Kozlowski wrote:
> On 13/01/2025 10:55, Nuno Sá wrote:
> > On Mon, 2025-01-13 at 09:37 +0100, Krzysztof Kozlowski wrote:
> > > On Thu, Jan 09, 2025 at 03:37:05PM +0200, Antoniu Miclaus wrote:
> > > >    clock-names:
> > > >      description:
> > > > -      Must be "clkin"
> > > > -    maxItems: 1
> > > > +      Must be "clkin" if the input reference is single ended or "clkin-
> > > > diff"
> > > > +      if the input reference is differential. By default single ended
> > > > input
> > > > is
> > > > +      applied.
> > > > +    enum: [clkin, clkin-diff]
> > > > +    default: clkin
> > > 
> > > Which pins are these? I went through adf4371 datasheet and no reference
> > > on clock inputs like clkin or clkin-diff.
> > > 
> > > 
> > 
> > Hmm, I guess we should call this 'refp' and 'refp-n' then (the latter seems
> > a
> > bit more odd)? Or just 'ref' and 'ref-diff'?
> 
> That mistake was done at the beginning - the "clkin" is just useless
> name. It cannot be "clkout" and it cannot be anything else than clk, so
> it is 100% redundant.
> 

Oh sure... Makes sense and I forgot that the property is not new...

> But looking for pins brought second point - here you claim these are
> mutually exclusive while datasheet suggests that both inputs can be
> connected. Unless they come from the same source always?
> 

If you have a single ended input then only one pin (the positive one) will be
used. If the input signal is differential, then both pins will be used. So they
are mutually exclusive... You either have single ended or a differential input. 
And depending on the input type, the limit of the input frequency varies.

- Nuno Sá
Krzysztof Kozlowski Jan. 13, 2025, 11:24 a.m. UTC | #7
On 13/01/2025 12:17, Nuno Sá wrote:
> On Mon, 2025-01-13 at 11:21 +0100, Krzysztof Kozlowski wrote:
>> On 13/01/2025 10:55, Nuno Sá wrote:
>>> On Mon, 2025-01-13 at 09:37 +0100, Krzysztof Kozlowski wrote:
>>>> On Thu, Jan 09, 2025 at 03:37:05PM +0200, Antoniu Miclaus wrote:
>>>>>    clock-names:
>>>>>      description:
>>>>> -      Must be "clkin"
>>>>> -    maxItems: 1
>>>>> +      Must be "clkin" if the input reference is single ended or "clkin-
>>>>> diff"
>>>>> +      if the input reference is differential. By default single ended
>>>>> input
>>>>> is
>>>>> +      applied.
>>>>> +    enum: [clkin, clkin-diff]
>>>>> +    default: clkin
>>>>
>>>> Which pins are these? I went through adf4371 datasheet and no reference
>>>> on clock inputs like clkin or clkin-diff.
>>>>
>>>>
>>>
>>> Hmm, I guess we should call this 'refp' and 'refp-n' then (the latter seems
>>> a
>>> bit more odd)? Or just 'ref' and 'ref-diff'?
>>
>> That mistake was done at the beginning - the "clkin" is just useless
>> name. It cannot be "clkout" and it cannot be anything else than clk, so
>> it is 100% redundant.
>>
> 
> Oh sure... Makes sense and I forgot that the property is not new...
> 
>> But looking for pins brought second point - here you claim these are
>> mutually exclusive while datasheet suggests that both inputs can be
>> connected. Unless they come from the same source always?
>>
> 
> If you have a single ended input then only one pin (the positive one) will be
> used. If the input signal is differential, then both pins will be used. So they

But the clocks describe input pins, at least in typical case, so that's
my question: how many clock sources do you have here? One or two?

> are mutually exclusive... You either have single ended or a differential input. 
> And depending on the input type, the limit of the input frequency varies.

Based on this, this is the same clock, so using "diff" is not a property
of "clocks". Look at other bindings how they encode differential choice
for some signals - usually bool property, but not always - see other
adi/admv devices.

Best regards,
Krzysztof
Antoniu Miclaus Jan. 13, 2025, 11:50 a.m. UTC | #8
--
Antoniu Miclăuş

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: Monday, January 13, 2025 1:24 PM
> To: Nuno Sá <noname.nuno@gmail.com>; Miclaus, Antoniu
> <Antoniu.Miclaus@analog.com>
> Cc: jic23@kernel.org; robh@kernel.org; conor+dt@kernel.org; linux-
> iio@vger.kernel.org; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-pwm@vger.kernel.org
> Subject: Re: [PATCH v4 1/3] dt-bindings: iio: adf4371: add refin mode
> 
> [External]
> 
> On 13/01/2025 12:17, Nuno Sá wrote:
> > On Mon, 2025-01-13 at 11:21 +0100, Krzysztof Kozlowski wrote:
> >> On 13/01/2025 10:55, Nuno Sá wrote:
> >>> On Mon, 2025-01-13 at 09:37 +0100, Krzysztof Kozlowski wrote:
> >>>> On Thu, Jan 09, 2025 at 03:37:05PM +0200, Antoniu Miclaus wrote:
> >>>>>    clock-names:
> >>>>>      description:
> >>>>> -      Must be "clkin"
> >>>>> -    maxItems: 1
> >>>>> +      Must be "clkin" if the input reference is single ended or "clkin-
> >>>>> diff"
> >>>>> +      if the input reference is differential. By default single ended
> >>>>> input
> >>>>> is
> >>>>> +      applied.
> >>>>> +    enum: [clkin, clkin-diff]
> >>>>> +    default: clkin
> >>>>
> >>>> Which pins are these? I went through adf4371 datasheet and no
> reference
> >>>> on clock inputs like clkin or clkin-diff.
> >>>>
> >>>>
> >>>
> >>> Hmm, I guess we should call this 'refp' and 'refp-n' then (the latter seems
> >>> a
> >>> bit more odd)? Or just 'ref' and 'ref-diff'?
> >>
> >> That mistake was done at the beginning - the "clkin" is just useless
> >> name. It cannot be "clkout" and it cannot be anything else than clk, so
> >> it is 100% redundant.
> >>
> >
> > Oh sure... Makes sense and I forgot that the property is not new...
> >
> >> But looking for pins brought second point - here you claim these are
> >> mutually exclusive while datasheet suggests that both inputs can be
> >> connected. Unless they come from the same source always?
> >>
> >
> > If you have a single ended input then only one pin (the positive one) will be
> > used. If the input signal is differential, then both pins will be used. So they
> 
> But the clocks describe input pins, at least in typical case, so that's
> my question: how many clock sources do you have here? One or two?
> 
> > are mutually exclusive... You either have single ended or a differential input.
> > And depending on the input type, the limit of the input frequency varies.
> 
> Based on this, this is the same clock, so using "diff" is not a property
> of "clocks". Look at other bindings how they encode differential choice
> for some signals - usually bool property, but not always - see other
> adi/admv devices.

This approach that you suggest was implemented in patch series v1 (as boolean) / v3 (as enum).
Based on feedback in v3 received from Jonathan I switched to this. Should I revert it?

> Best regards,
> Krzysztof
Krzysztof Kozlowski Jan. 13, 2025, 12:05 p.m. UTC | #9
On 13/01/2025 12:50, Miclaus, Antoniu wrote:
>>>
>>> Oh sure... Makes sense and I forgot that the property is not new...
>>>
>>>> But looking for pins brought second point - here you claim these are
>>>> mutually exclusive while datasheet suggests that both inputs can be
>>>> connected. Unless they come from the same source always?
>>>>
>>>
>>> If you have a single ended input then only one pin (the positive one) will be
>>> used. If the input signal is differential, then both pins will be used. So they
>>
>> But the clocks describe input pins, at least in typical case, so that's
>> my question: how many clock sources do you have here? One or two?
>>
>>> are mutually exclusive... You either have single ended or a differential input.
>>> And depending on the input type, the limit of the input frequency varies.
>>
>> Based on this, this is the same clock, so using "diff" is not a property
>> of "clocks". Look at other bindings how they encode differential choice
>> for some signals - usually bool property, but not always - see other
>> adi/admv devices.
> 
> This approach that you suggest was implemented in patch series v1 (as boolean) / v3 (as enum).
> Based on feedback in v3 received from Jonathan I switched to this. Should I revert it?

I see:
https://lore.kernel.org/lkml/20241220195220.1e1e1d6f@jic23-huawei/

I think that v3 was preferred with arguments above. You have one clock
input and you want to configure the device differently, based on how
this clock is wired. But it is still one clock.

Wait for Jonathan before reverting to v3.

Best regards,
Krzysztof
Jonathan Cameron Jan. 13, 2025, 8:18 p.m. UTC | #10
On Mon, 13 Jan 2025 13:05:30 +0100
Krzysztof Kozlowski <krzk@kernel.org> wrote:

> On 13/01/2025 12:50, Miclaus, Antoniu wrote:
> >>>
> >>> Oh sure... Makes sense and I forgot that the property is not new...
> >>>  
> >>>> But looking for pins brought second point - here you claim these are
> >>>> mutually exclusive while datasheet suggests that both inputs can be
> >>>> connected. Unless they come from the same source always?
> >>>>  
> >>>
> >>> If you have a single ended input then only one pin (the positive one) will be
> >>> used. If the input signal is differential, then both pins will be used. So they  
> >>
> >> But the clocks describe input pins, at least in typical case, so that's
> >> my question: how many clock sources do you have here? One or two?
> >>  
> >>> are mutually exclusive... You either have single ended or a differential input.
> >>> And depending on the input type, the limit of the input frequency varies.  
> >>
> >> Based on this, this is the same clock, so using "diff" is not a property
> >> of "clocks". Look at other bindings how they encode differential choice
> >> for some signals - usually bool property, but not always - see other
> >> adi/admv devices.  
> > 
> > This approach that you suggest was implemented in patch series v1 (as boolean) / v3 (as enum).
> > Based on feedback in v3 received from Jonathan I switched to this. Should I revert it?  
> 
> I see:
> https://lore.kernel.org/lkml/20241220195220.1e1e1d6f@jic23-huawei/
> 
> I think that v3 was preferred with arguments above. You have one clock
> input and you want to configure the device differently, based on how
> this clock is wired. But it is still one clock.
> 
> Wait for Jonathan before reverting to v3.

This is following the approach we've previously used for crystal (xtal) vs clock
(typically one pin shared in those cases as well).

They are effectively different types of clock that might be connected.

e.g. adi,ad7173.yaml or adi,ad7192.yaml

I'm struggling to find the original discussion of that case.
Easy enough to find where they were added, but they mysteriously
just appear and I'm sure we had a detailed discussion prior to that.

I don't mind that much if we go to a boolean, the clock names just seems
cleaner to me as they are effectively different possible clock inputs
(with one shared pin). Whereas one of the pins is not shared as only applies
in the differential case.

Jonathan

> 
> Best regards,
> Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/iio/frequency/adf4371.yaml b/Documentation/devicetree/bindings/iio/frequency/adf4371.yaml
index 1cb2adaf66f9..9ad5b22c59d7 100644
--- a/Documentation/devicetree/bindings/iio/frequency/adf4371.yaml
+++ b/Documentation/devicetree/bindings/iio/frequency/adf4371.yaml
@@ -30,8 +30,11 @@  properties:
 
   clock-names:
     description:
-      Must be "clkin"
-    maxItems: 1
+      Must be "clkin" if the input reference is single ended or "clkin-diff"
+      if the input reference is differential. By default single ended input is
+      applied.
+    enum: [clkin, clkin-diff]
+    default: clkin
 
   adi,mute-till-lock-en:
     type: boolean