| Message ID | 20220423140658.145000-1-biju.das.jz@bp.renesas.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] arm64: dts: renesas: r9a07g044: Fix external clk node names | expand |
On 24/04/2022 12:22, Biju Das wrote: > Hi Krzysztof Kozlowski, > > Thanks for the feedback. > >> Subject: Re: [PATCH 1/2] arm64: dts: renesas: r9a07g044: Fix external clk >> node names >> >> On 24/04/2022 09:50, Biju Das wrote: >>>> Subject: RE: [PATCH 1/2] arm64: dts: renesas: r9a07g044: Fix external >>>> clk node names >>>> >>>> Hi Krzysztof Kozlowski, >>>> >>>> Thanks for the feedback. >>>> >>>>> Subject: Re: [PATCH 1/2] arm64: dts: renesas: r9a07g044: Fix >>>>> external clk node names >>>>> >>>>> On 23/04/2022 16:06, Biju Das wrote: >>>>>> Fix audio clk node names with "_" -> "-" and add suffix '-clk' for >>>>>> can and extal clks. >>>>>> >>>>>> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> >>>>>> --- >>>>>> arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 8 ++++---- >>>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi >>>>>> b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi >>>>>> index 19287cccb1f0..4f9a84d7af08 100644 >>>>>> --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi >>>>>> +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi >>>>>> @@ -13,14 +13,14 @@ / { >>>>>> #address-cells = <2>; >>>>>> #size-cells = <2>; >>>>>> >>>>>> - audio_clk1: audio_clk1 { >>>>>> + audio_clk1: audio-clk1 { >>>>> >>>>> How about in such case keeping suffix "clk" everywhere and moving >>>>> the index >>>>> (1/2) to the first part? This way one day maybe schema will match >>>>> the clocks. >>>> >>>> Just a question, >>>> >>>> Your suggestion is "audio_clk1: audio-clk1" -> "audio_clk1: audio-clk" >>>> >>>> In that case, If you plan to drop the label in future, how will you >>>> differentiate between >>>> audio-clk1 and audio-clk2 with just node names? >>> >>> Or >>> >>> Do you want me to do the change like this? >>> >>> "audio_clk1: audio-clk1" -> "audio_clk_1: audio-clk-1" >>> >>> And fix the phandle reference in other dtsi files?? >> >> My suggestion was to move the [12] part into the first part, so the suffix >> "clk" stays consistent: >> audio1-clk >> audio2-clk > > From HW perspective, there are 2 audio clocks, audio clock1(multiple and sub multiple of 44.1 Khz) > and audio clk 2(Multiple and submultiple of 48Khz) connected to a single audio Codec. > > Based on the sampling rate, through clock generator driver we can switch the clock source for > audio mclock along with audio clock for SSI and we can support both these rates > > Since there is a single audio codec, I am not sure, audio1-clk and audio2-clk is a good choise. The name of the clock is not "audio clock" but "audio", because you do not call a car "Ford Mustang car", but just "Ford Mustang". Therefore "clock" is not part of the name, but just description of a type. > > What about like > > audio_clk1: audio-clk-1 ? > audio_clk2: audio-clk-2 ? > > Which is consistent with naming used for cpu and opp-tables? It's not consistent with clk naming. Nodes should have generic names, so the generic part is "clk". You add specific audio/audio-X prefix or suffix - it's fine, but not both. This is exactly the trouble when you start using specific names and Devicetree spec explicitly asks for generic names. So maybe go with the spec and call of these "clk-[0-9]" and problem is gone. Best regards, Krzysztof
On 25/04/2022 17:28, Biju Das wrote: >>>> My suggestion was to move the [12] part into the first part, so the >>>> suffix "clk" stays consistent: >>>> audio1-clk >>>> audio2-clk >>> >>> From HW perspective, there are 2 audio clocks, audio clock1(multiple >>> and sub multiple of 44.1 Khz) and audio clk 2(Multiple and submultiple >> of 48Khz) connected to a single audio Codec. >>> >>> Based on the sampling rate, through clock generator driver we can >>> switch the clock source for audio mclock along with audio clock for >>> SSI and we can support both these rates >>> >>> Since there is a single audio codec, I am not sure, audio1-clk and >> audio2-clk is a good choise. >> >> The name of the clock is not "audio clock" but "audio", because you do not >> call a car "Ford Mustang car", but just "Ford Mustang". Therefore "clock" >> is not part of the name, but just description of a type. > > The hardware mention the name as AUDIO_CLK1 and AUDIO_CLK2. The hardware document might call it "AUDIO_CLK_REAL_CLK_CLK" and it won't be an argument to call device node that way in DTS. > There are 2 Clock availables for audio interface. > In that case if you term it as audio1-clk and audio-clk2, > But as you said clk-1-audio and clk-2-audio will be correct? If you change all other clocks to follow same principle - generic name followed by specific suffix - then yes. Then you should have "clk-extal", "clk-can" etc. > >> >>> >>> What about like >>> >>> audio_clk1: audio-clk-1 ? >>> audio_clk2: audio-clk-2 ? >>> >>> Which is consistent with naming used for cpu and opp-tables? >> >> >> It's not consistent with clk naming. Nodes should have generic names, so >> the generic part is "clk". You add specific audio/audio-X prefix or suffix >> - it's fine, but not both. >> >> This is exactly the trouble when you start using specific names and >> Devicetree spec explicitly asks for generic names. So maybe go with the >> spec and call of these "clk-[0-9]" and problem is gone. > > Ok Will change like > > "audio_clk1: clk-1-audio" > What do you mean "ok"? I said "clk-[0-9]", so "clk-0", "clk-1", "clk-2" and so on. No specific prefix. > Label name matches with hardware manual and node names as per Device tree spec. Best regards, Krzysztof
Hi Krzysztof Kozlowski, > Subject: Re: [PATCH 1/2] arm64: dts: renesas: r9a07g044: Fix external clk > node names > > On 25/04/2022 18:26, Biju Das wrote: > >> > >> What do you mean "ok"? I said "clk-[0-9]", so "clk-0", "clk-1", "clk-2" > >> and so on. No specific prefix. > > > > Ah ok. > > > > As you mentioned above "generic part is "clk". You add specific > > audio/audio-X prefix or suffix" > > > > So based on that, I thought "clk" is generic part and "-1-audio" as > suffix, "clk-1-audio" > > should be acceptable. > > > > For eg:- If I plan to match the label name with the hardware > > manual(audio_clk1), > > Labels are not restricted (except using [a-z0-9_], no dashes), so it is > perfectly fine. OK. > > > > > then, as per the discussion we have, the node names should be > > > > either > > > > "audio_clk1: clk-0" > > > > or > > > > "audio_clk1: clk-1" > > > > Or > > > > "audio_clk1: audio1-clk" > > > > Correct? > > Yes, correct. Thanks for the clarification. Cheers, Biju
diff --git a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi index 19287cccb1f0..4f9a84d7af08 100644 --- a/arch/arm64/boot/dts/renesas/r9a07g044.dtsi +++ b/arch/arm64/boot/dts/renesas/r9a07g044.dtsi @@ -13,14 +13,14 @@ / { #address-cells = <2>; #size-cells = <2>; - audio_clk1: audio_clk1 { + audio_clk1: audio-clk1 { compatible = "fixed-clock"; #clock-cells = <0>; /* This value must be overridden by boards that provide it */ clock-frequency = <0>; }; - audio_clk2: audio_clk2 { + audio_clk2: audio-clk2 { compatible = "fixed-clock"; #clock-cells = <0>; /* This value must be overridden by boards that provide it */ @@ -28,14 +28,14 @@ audio_clk2: audio_clk2 { }; /* External CAN clock - to be overridden by boards that provide it */ - can_clk: can { + can_clk: can-clk { compatible = "fixed-clock"; #clock-cells = <0>; clock-frequency = <0>; }; /* clock can be either from exclk or crystal oscillator (XIN/XOUT) */ - extal_clk: extal { + extal_clk: extal-clk { compatible = "fixed-clock"; #clock-cells = <0>; /* This value must be overridden by the board */
Fix audio clk node names with "_" -> "-" and add suffix '-clk' for can and extal clks. Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com> --- arch/arm64/boot/dts/renesas/r9a07g044.dtsi | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)