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 |
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
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
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 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á
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
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á
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 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
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
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 --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
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(-)