| Message ID | 20250115012628.1035928-5-pgwipeout@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | rockchip: add a functional usb3 phy driver for rk3328 | expand |
On 15/01/2025 02:26, Peter Geis wrote: > Add the node for the rk3328 usb3 phy. This node provides a combined usb2 > and usb3 phy which are permenantly tied to the dwc3 usb3 controller. > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > --- > > arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++ > 1 file changed, 39 insertions(+) > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > index 7d992c3c01ce..181a900d41f9 100644 > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > @@ -903,6 +903,43 @@ u2phy_host: host-port { > }; > }; > > + usb3phy: usb3-phy@ff460000 { > + compatible = "rockchip,rk3328-usb3phy"; > + reg = <0x0 0xff460000 0x0 0x10000>; > + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; Please wrap code according to coding style (checkpatch is not a coding style description, but only a tool), so at 80. > + clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe"; > + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "bvalid", "id", "linestate", "rxdet"; > + resets = <&cru SRST_USB3PHY_U2>, > + <&cru SRST_USB3PHY_U3>, > + <&cru SRST_USB3PHY_PIPE>, > + <&cru SRST_USB3OTG_UTMI>, > + <&cru SRST_USB3PHY_OTG_P>, > + <&cru SRST_USB3PHY_PIPE_P>; > + reset-names = "usb3phy-u2-por", "usb3phy-u3-por", > + "usb3phy-pipe-mac", "usb3phy-utmi-mac", > + "usb3phy-utmi-apb", "usb3phy-pipe-apb"; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + status = "disabled"; > + > + usb3phy_utmi: utmi-port@ff470000 { > + compatible = "rockchip,rk3328-usb3phy-utmi"; > + reg = <0x0 0xff470000 0x0 0x8000>; > + #phy-cells = <0>; Parent device is the phy provider, not child. This is odd... > + status = "disabled"; > + }; > + > + usb3phy_pipe: pipe-port@ff478000 { > + compatible = "rockchip,rk3328-usb3phy-pipe"; > + reg = <0x0 0xff478000 0x0 0x8000>; > + #phy-cells = <0>; > + status = "disabled"; > + }; > + }; Best regards, Krzysztof
On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: > On 15/01/2025 02:26, Peter Geis wrote: > > Add the node for the rk3328 usb3 phy. This node provides a combined usb2 > > and usb3 phy which are permenantly tied to the dwc3 usb3 controller. > > > > Signed-off-by: Peter Geis <pgwipeout@gmail.com> > > --- > > > > arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++ > > 1 file changed, 39 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > > index 7d992c3c01ce..181a900d41f9 100644 > > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi > > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > > @@ -903,6 +903,43 @@ u2phy_host: host-port { > > }; > > }; > > > > + usb3phy: usb3-phy@ff460000 { > > + compatible = "rockchip,rk3328-usb3phy"; > > + reg = <0x0 0xff460000 0x0 0x10000>; > > + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; > > Please wrap code according to coding style (checkpatch is not a coding > style description, but only a tool), so at 80. I'm confused: is it 80 or 100? I always thought it was 80, but then I saw several patches/commits by Dragan Simic which deliberately changed code to make use of 100. Being fed up with my own confusion, I submitted a PR to https://github.com/gregkh/kernel-coding-style/ which got accepted: https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a So now both the vim plugins code and README say 100. But as noted in my commit message: Note that the current upstream 'Linux kernel coding style' does NOT mention the 100 char limit, but only mentions the preferred max length of 80. Or is it 100 for code, but 80 for DeviceTree files and bindings? Cheers, Diederik
Hello Diederik, On 2025-01-16 17:53, Diederik de Haas wrote: > On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: >> On 15/01/2025 02:26, Peter Geis wrote: >> > Add the node for the rk3328 usb3 phy. This node provides a combined usb2 >> > and usb3 phy which are permenantly tied to the dwc3 usb3 controller. >> > >> > Signed-off-by: Peter Geis <pgwipeout@gmail.com> >> > --- >> > >> > arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++ >> > 1 file changed, 39 insertions(+) >> > >> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> > index 7d992c3c01ce..181a900d41f9 100644 >> > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >> > @@ -903,6 +903,43 @@ u2phy_host: host-port { >> > }; >> > }; >> > >> > + usb3phy: usb3-phy@ff460000 { >> > + compatible = "rockchip,rk3328-usb3phy"; >> > + reg = <0x0 0xff460000 0x0 0x10000>; >> > + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; >> >> Please wrap code according to coding style (checkpatch is not a coding >> style description, but only a tool), so at 80. > > I'm confused: is it 80 or 100? > > I always thought it was 80, but then I saw several patches/commits by > Dragan Simic which deliberately changed code to make use of 100. > Being fed up with my own confusion, I submitted a PR to > https://github.com/gregkh/kernel-coding-style/ which got accepted: > https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a > > So now both the vim plugins code and README say 100. > But as noted in my commit message: > > Note that the current upstream 'Linux kernel coding style' does NOT > mention the 100 char limit, but only mentions the preferred max > length > of 80. > > Or is it 100 for code, but 80 for DeviceTree files and bindings? I don't know about the DT files and bindings, but the 100-column limit for the kernel code has been in effect for years. In this day and age, 80 columns is really not much (for the record, I've been around when using 80x25 _physical_ CRT screens was the norm).
On 16/01/2025 17:53, Diederik de Haas wrote: > On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: >> On 15/01/2025 02:26, Peter Geis wrote: >>> Add the node for the rk3328 usb3 phy. This node provides a combined usb2 >>> and usb3 phy which are permenantly tied to the dwc3 usb3 controller. >>> >>> Signed-off-by: Peter Geis <pgwipeout@gmail.com> >>> --- >>> >>> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++ >>> 1 file changed, 39 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>> index 7d992c3c01ce..181a900d41f9 100644 >>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>> @@ -903,6 +903,43 @@ u2phy_host: host-port { >>> }; >>> }; >>> >>> + usb3phy: usb3-phy@ff460000 { >>> + compatible = "rockchip,rk3328-usb3phy"; >>> + reg = <0x0 0xff460000 0x0 0x10000>; >>> + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; >> >> Please wrap code according to coding style (checkpatch is not a coding >> style description, but only a tool), so at 80. > > I'm confused: is it 80 or 100? > > I always thought it was 80, but then I saw several patches/commits by Coding style is clear: it is 80. It also has caveat about code readability and several maintainers have their own preference. > Dragan Simic which deliberately changed code to make use of 100. > Being fed up with my own confusion, I submitted a PR to > https://github.com/gregkh/kernel-coding-style/ which got accepted: > https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a That's not kernel. That's Greg... > > So now both the vim plugins code and README say 100. > But as noted in my commit message: > > Note that the current upstream 'Linux kernel coding style' does NOT > mention the 100 char limit, but only mentions the preferred max length > of 80. > > Or is it 100 for code, but 80 for DeviceTree files and bindings? From where did you get 100? Checkpatch, right? Kernel coding style is clear, there is no discussion, no mentioning 100: "The preferred limit on the length of a single line is 80 columns. " So to be clear: all DTS, all DT bindings, all code maintained by me and some maintainers follows above (and further - there is caveat) instruction from coding style. Some maintainers follow other rules and that's fine. Best regards, Krzysztof
On 17/01/2025 05:10, Dragan Simic wrote: > Hello Diederik, > > On 2025-01-16 17:53, Diederik de Haas wrote: >> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: >>> On 15/01/2025 02:26, Peter Geis wrote: >>>> Add the node for the rk3328 usb3 phy. This node provides a combined usb2 >>>> and usb3 phy which are permenantly tied to the dwc3 usb3 controller. >>>> >>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com> >>>> --- >>>> >>>> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>> index 7d992c3c01ce..181a900d41f9 100644 >>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>> @@ -903,6 +903,43 @@ u2phy_host: host-port { >>>> }; >>>> }; >>>> >>>> + usb3phy: usb3-phy@ff460000 { >>>> + compatible = "rockchip,rk3328-usb3phy"; >>>> + reg = <0x0 0xff460000 0x0 0x10000>; >>>> + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; >>> >>> Please wrap code according to coding style (checkpatch is not a coding >>> style description, but only a tool), so at 80. >> >> I'm confused: is it 80 or 100? >> >> I always thought it was 80, but then I saw several patches/commits by >> Dragan Simic which deliberately changed code to make use of 100. >> Being fed up with my own confusion, I submitted a PR to >> https://github.com/gregkh/kernel-coding-style/ which got accepted: >> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a >> >> So now both the vim plugins code and README say 100. >> But as noted in my commit message: >> >> Note that the current upstream 'Linux kernel coding style' does NOT >> mention the 100 char limit, but only mentions the preferred max >> length >> of 80. >> >> Or is it 100 for code, but 80 for DeviceTree files and bindings? > > I don't know about the DT files and bindings, but the 100-column limit > for the kernel code has been in effect for years. In this day and age, That's just false. It was never in effect for years. Read kernel coding style document. > 80 columns is really not much (for the record, I've been around when > using 80x25 _physical_ CRT screens was the norm). You mistake agreement on dropping strong restriction in 2020 in checkpatch, which is "not for years" and even read that commit: "Yes, staying withing 80 columns is certainly still _preferred_." Checkpatch is not coding style. Since when it would be? It's just a tool. And there were more talks and the 80-preference got relaxed yet still "not for years" (last talk was 2022?) and sill kernel coding style is here specific. Best regards, Krzysztof
On 18/01/2025 09:41, Krzysztof Kozlowski wrote: > On 16/01/2025 17:53, Diederik de Haas wrote: >> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: >>> On 15/01/2025 02:26, Peter Geis wrote: >>>> Add the node for the rk3328 usb3 phy. This node provides a combined usb2 >>>> and usb3 phy which are permenantly tied to the dwc3 usb3 controller. >>>> >>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com> >>>> --- >>>> >>>> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++ >>>> 1 file changed, 39 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>> index 7d992c3c01ce..181a900d41f9 100644 >>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>> @@ -903,6 +903,43 @@ u2phy_host: host-port { >>>> }; >>>> }; >>>> >>>> + usb3phy: usb3-phy@ff460000 { >>>> + compatible = "rockchip,rk3328-usb3phy"; >>>> + reg = <0x0 0xff460000 0x0 0x10000>; >>>> + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; >>> >>> Please wrap code according to coding style (checkpatch is not a coding >>> style description, but only a tool), so at 80. >> >> I'm confused: is it 80 or 100? >> >> I always thought it was 80, but then I saw several patches/commits by > > Coding style is clear: it is 80. It also has caveat about code > readability and several maintainers have their own preference. > >> Dragan Simic which deliberately changed code to make use of 100. >> Being fed up with my own confusion, I submitted a PR to >> https://github.com/gregkh/kernel-coding-style/ which got accepted: >> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a > > That's not kernel. That's Greg... > >> >> So now both the vim plugins code and README say 100. >> But as noted in my commit message: >> >> Note that the current upstream 'Linux kernel coding style' does NOT >> mention the 100 char limit, but only mentions the preferred max length >> of 80. >> >> Or is it 100 for code, but 80 for DeviceTree files and bindings? > > From where did you get 100? Checkpatch, right? Kernel coding style is > clear, there is no discussion, no mentioning 100: > > "The preferred limit on the length of a single line is 80 columns. " > > So to be clear: all DTS, all DT bindings, all code maintained by me and > some maintainers follows above (and further - there is caveat) > instruction from coding style. Some maintainers follow other rules and > that's fine. Although let me add here caveat, after looking at some other code: DTS due to its nature of a lot of parent-child relationships combined with long constants ("GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>") has the strongest exception or the strongest second part of the coding style: "...unless exceeding 80 columns significantly increases readability..." And again: that's from official coding style document (so something which have been for years), no matter what other people tell you they think exists since years as coding style. Splitting line, I commented here in this patch, did not improve readability. Quite opposite: the line there was less readable in current format thus it is not even about coding style anymore, but just readability style. Any list with more than two short entries (by number of characters in list item) or any list with more than one long entry should be split for readability. However actual ITEMS in list should not be split - but again coding style is here very precise since years. 80 unless significantly increases readability. Best regards, Krzysztof
Hello Krzysztof, On 2025-01-18 09:46, Krzysztof Kozlowski wrote: > On 17/01/2025 05:10, Dragan Simic wrote: >> On 2025-01-16 17:53, Diederik de Haas wrote: >>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: >>>> On 15/01/2025 02:26, Peter Geis wrote: >>>>> Add the node for the rk3328 usb3 phy. This node provides a combined >>>>> usb2 >>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 >>>>> controller. >>>>> >>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com> >>>>> --- >>>>> >>>>> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 >>>>> ++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>> index 7d992c3c01ce..181a900d41f9 100644 >>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port { >>>>> }; >>>>> }; >>>>> >>>>> + usb3phy: usb3-phy@ff460000 { >>>>> + compatible = "rockchip,rk3328-usb3phy"; >>>>> + reg = <0x0 0xff460000 0x0 0x10000>; >>>>> + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru >>>>> PCLK_USB3PHY_PIPE>; >>>> >>>> Please wrap code according to coding style (checkpatch is not a >>>> coding >>>> style description, but only a tool), so at 80. >>> >>> I'm confused: is it 80 or 100? >>> >>> I always thought it was 80, but then I saw several patches/commits by >>> Dragan Simic which deliberately changed code to make use of 100. >>> Being fed up with my own confusion, I submitted a PR to >>> https://github.com/gregkh/kernel-coding-style/ which got accepted: >>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a >>> >>> So now both the vim plugins code and README say 100. >>> But as noted in my commit message: >>> >>> Note that the current upstream 'Linux kernel coding style' does NOT >>> mention the 100 char limit, but only mentions the preferred max >>> length >>> of 80. >>> >>> Or is it 100 for code, but 80 for DeviceTree files and bindings? >> >> I don't know about the DT files and bindings, but the 100-column limit >> for the kernel code has been in effect for years. In this day and >> age, > > That's just false. It was never in effect for years. Read kernel coding > style document. Perhaps it's about the semantics. Please see the commit bdc48fa11e46 (checkpatch/coding-style: deprecate 80-column warning, 2020-05-29), which clearly shows that the 80-column rule is still _preferred_, but no longer _mandatory_. >> 80 columns is really not much (for the record, I've been around when >> using 80x25 _physical_ CRT screens was the norm). > > You mistake agreement on dropping strong restriction in 2020 in > checkpatch, which is "not for years" and even read that commit: "Yes, > staying withing 80 columns is certainly still _preferred_." > > Checkpatch is not coding style. Since when it would be? It's just a > tool. > > And there were more talks and the 80-preference got relaxed yet still > "not for years" (last talk was 2022?) and sill kernel coding style is > here specific. It's perhaps again about the semantics, this time about the meaning of "for years". I don't think there's some strict definition of that term, so perhaps different people see it differently. To get back to the above-mentioned commit bdc48fa11e46, the 80-column limit has obviously been lifted, putting the new 100-column limit as an option for those who prefer having fewer "artificial" line breaks over adhering strictly to the rules. Thus, as a maintainer, you're obviously free to enforce the 80-column limit of you want so. If my opinion counts, I'd agree with the 80-column limit when it comes to the device trees and bindings. Keeping those files at the lower width usually makes them more readable to me. However, enforcing the 80-column limit in C and header files very often leads to having line breaks that do nothing but make the code look a bit silly.
On 18/01/2025 10:25, Dragan Simic wrote: > Hello Krzysztof, > > On 2025-01-18 09:46, Krzysztof Kozlowski wrote: >> On 17/01/2025 05:10, Dragan Simic wrote: >>> On 2025-01-16 17:53, Diederik de Haas wrote: >>>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: >>>>> On 15/01/2025 02:26, Peter Geis wrote: >>>>>> Add the node for the rk3328 usb3 phy. This node provides a combined >>>>>> usb2 >>>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 >>>>>> controller. >>>>>> >>>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com> >>>>>> --- >>>>>> >>>>>> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 >>>>>> ++++++++++++++++++++++++ >>>>>> 1 file changed, 39 insertions(+) >>>>>> >>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>>> index 7d992c3c01ce..181a900d41f9 100644 >>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port { >>>>>> }; >>>>>> }; >>>>>> >>>>>> + usb3phy: usb3-phy@ff460000 { >>>>>> + compatible = "rockchip,rk3328-usb3phy"; >>>>>> + reg = <0x0 0xff460000 0x0 0x10000>; >>>>>> + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru >>>>>> PCLK_USB3PHY_PIPE>; >>>>> >>>>> Please wrap code according to coding style (checkpatch is not a >>>>> coding >>>>> style description, but only a tool), so at 80. >>>> >>>> I'm confused: is it 80 or 100? >>>> >>>> I always thought it was 80, but then I saw several patches/commits by >>>> Dragan Simic which deliberately changed code to make use of 100. >>>> Being fed up with my own confusion, I submitted a PR to >>>> https://github.com/gregkh/kernel-coding-style/ which got accepted: >>>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a >>>> >>>> So now both the vim plugins code and README say 100. >>>> But as noted in my commit message: >>>> >>>> Note that the current upstream 'Linux kernel coding style' does NOT >>>> mention the 100 char limit, but only mentions the preferred max >>>> length >>>> of 80. >>>> >>>> Or is it 100 for code, but 80 for DeviceTree files and bindings? >>> >>> I don't know about the DT files and bindings, but the 100-column limit >>> for the kernel code has been in effect for years. In this day and >>> age, >> >> That's just false. It was never in effect for years. Read kernel coding >> style document. > > Perhaps it's about the semantics. > > Please see the commit bdc48fa11e46 (checkpatch/coding-style: deprecate > 80-column warning, 2020-05-29), which clearly shows that the 80-column > rule is still _preferred_, but no longer _mandatory_. I brought that commit, but nice that you also found it. Still: read the coding style, not checkpatch tool. > >>> 80 columns is really not much (for the record, I've been around when >>> using 80x25 _physical_ CRT screens was the norm). >> >> You mistake agreement on dropping strong restriction in 2020 in >> checkpatch, which is "not for years" and even read that commit: "Yes, >> staying withing 80 columns is certainly still _preferred_." >> >> Checkpatch is not coding style. Since when it would be? It's just a >> tool. >> >> And there were more talks and the 80-preference got relaxed yet still >> "not for years" (last talk was 2022?) and sill kernel coding style is >> here specific. > > It's perhaps again about the semantics, this time about the meaning > of "for years". I don't think there's some strict definition of that > term, so perhaps different people see it differently. > > To get back to the above-mentioned commit bdc48fa11e46, the 80-column > limit has obviously been lifted, putting the new 100-column limit as "Lifted" on *CHECKPATCH*, not on coding style. Do you see the difference? One is a helper tool which people were using blindly and wrapping lines without thinking, claiming that checkpatch told them to do so. Other is the actual coding style. You claim that coding style was changed. This never happened. And my first - really the first - comment here was also precise mentioning that difference: "Please wrap code according to coding style (*checkpatch is not* a coding style description, but only a tool), so at 80." Best regards, Krzysztof
On 2025-01-18 10:19, Krzysztof Kozlowski wrote: > On 18/01/2025 09:41, Krzysztof Kozlowski wrote: >> On 16/01/2025 17:53, Diederik de Haas wrote: >>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: >>>> On 15/01/2025 02:26, Peter Geis wrote: >>>>> Add the node for the rk3328 usb3 phy. This node provides a combined >>>>> usb2 >>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 >>>>> controller. >>>>> >>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com> >>>>> --- >>>>> >>>>> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 >>>>> ++++++++++++++++++++++++ >>>>> 1 file changed, 39 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>> index 7d992c3c01ce..181a900d41f9 100644 >>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port { >>>>> }; >>>>> }; >>>>> >>>>> + usb3phy: usb3-phy@ff460000 { >>>>> + compatible = "rockchip,rk3328-usb3phy"; >>>>> + reg = <0x0 0xff460000 0x0 0x10000>; >>>>> + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru >>>>> PCLK_USB3PHY_PIPE>; >>>> >>>> Please wrap code according to coding style (checkpatch is not a >>>> coding >>>> style description, but only a tool), so at 80. >>> >>> I'm confused: is it 80 or 100? >>> >>> I always thought it was 80, but then I saw several patches/commits by >> >> Coding style is clear: it is 80. It also has caveat about code >> readability and several maintainers have their own preference. >> >>> Dragan Simic which deliberately changed code to make use of 100. >>> Being fed up with my own confusion, I submitted a PR to >>> https://github.com/gregkh/kernel-coding-style/ which got accepted: >>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a >> >> That's not kernel. That's Greg... >> >>> >>> So now both the vim plugins code and README say 100. >>> But as noted in my commit message: >>> >>> Note that the current upstream 'Linux kernel coding style' does NOT >>> mention the 100 char limit, but only mentions the preferred max >>> length >>> of 80. >>> >>> Or is it 100 for code, but 80 for DeviceTree files and bindings? >> >> From where did you get 100? Checkpatch, right? Kernel coding style is >> clear, there is no discussion, no mentioning 100: >> >> "The preferred limit on the length of a single line is 80 columns. " >> >> So to be clear: all DTS, all DT bindings, all code maintained by me >> and >> some maintainers follows above (and further - there is caveat) >> instruction from coding style. Some maintainers follow other rules and >> that's fine. > > > Although let me add here caveat, after looking at some other code: DTS > due to its nature of a lot of parent-child relationships combined with > long constants ("GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>") has the strongest > exception or the strongest second part of the coding style: > "...unless exceeding 80 columns significantly increases readability..." > > And again: that's from official coding style document (so something > which have been for years), no matter what other people tell you they > think exists since years as coding style. > > Splitting line, I commented here in this patch, did not improve > readability. > > Quite opposite: the line there was less readable in current format thus > it is not even about coding style anymore, but just readability style. > Any list with more than two short entries (by number of characters in > list item) or any list with more than one long entry should be split > for > readability. However actual ITEMS in list should not be split - but > again coding style is here very precise since years. 80 unless > significantly increases readability. I fully agree with the readability being the most important factor when it comes to deciding on the column width. That's very well illustrated by the example above, i.e. the list items in device trees, which are much more readable when the items are placed in separate lines. Though, as I wrote in my earlier response, enforcing the 80-column limit in C and headers files rather often leads to line breaks that are obviously "artificial" and do nothing but make the code less readable. That's where the 100-column with limit often improves the readability.
On 2025-01-18 10:31, Krzysztof Kozlowski wrote: > On 18/01/2025 10:25, Dragan Simic wrote: >> On 2025-01-18 09:46, Krzysztof Kozlowski wrote: >>> On 17/01/2025 05:10, Dragan Simic wrote: >>>> On 2025-01-16 17:53, Diederik de Haas wrote: >>>>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: >>>>>> On 15/01/2025 02:26, Peter Geis wrote: >>>>>>> Add the node for the rk3328 usb3 phy. This node provides a >>>>>>> combined >>>>>>> usb2 >>>>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 >>>>>>> controller. >>>>>>> >>>>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com> >>>>>>> --- >>>>>>> >>>>>>> arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 >>>>>>> ++++++++++++++++++++++++ >>>>>>> 1 file changed, 39 insertions(+) >>>>>>> >>>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>>>> index 7d992c3c01ce..181a900d41f9 100644 >>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi >>>>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port { >>>>>>> }; >>>>>>> }; >>>>>>> >>>>>>> + usb3phy: usb3-phy@ff460000 { >>>>>>> + compatible = "rockchip,rk3328-usb3phy"; >>>>>>> + reg = <0x0 0xff460000 0x0 0x10000>; >>>>>>> + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, >>>>>>> <&cru >>>>>>> PCLK_USB3PHY_PIPE>; >>>>>> >>>>>> Please wrap code according to coding style (checkpatch is not a >>>>>> coding >>>>>> style description, but only a tool), so at 80. >>>>> >>>>> I'm confused: is it 80 or 100? >>>>> >>>>> I always thought it was 80, but then I saw several patches/commits >>>>> by >>>>> Dragan Simic which deliberately changed code to make use of 100. >>>>> Being fed up with my own confusion, I submitted a PR to >>>>> https://github.com/gregkh/kernel-coding-style/ which got accepted: >>>>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a >>>>> >>>>> So now both the vim plugins code and README say 100. >>>>> But as noted in my commit message: >>>>> >>>>> Note that the current upstream 'Linux kernel coding style' does >>>>> NOT >>>>> mention the 100 char limit, but only mentions the preferred max >>>>> length >>>>> of 80. >>>>> >>>>> Or is it 100 for code, but 80 for DeviceTree files and bindings? >>>> >>>> I don't know about the DT files and bindings, but the 100-column >>>> limit >>>> for the kernel code has been in effect for years. In this day and >>>> age, >>> >>> That's just false. It was never in effect for years. Read kernel >>> coding >>> style document. >> >> Perhaps it's about the semantics. >> >> Please see the commit bdc48fa11e46 (checkpatch/coding-style: deprecate >> 80-column warning, 2020-05-29), which clearly shows that the 80-column >> rule is still _preferred_, but no longer _mandatory_. > > I brought that commit, but nice that you also found it. > > Still: read the coding style, not checkpatch tool. > >>>> 80 columns is really not much (for the record, I've been around when >>>> using 80x25 _physical_ CRT screens was the norm). >>> >>> You mistake agreement on dropping strong restriction in 2020 in >>> checkpatch, which is "not for years" and even read that commit: "Yes, >>> staying withing 80 columns is certainly still _preferred_." >>> >>> Checkpatch is not coding style. Since when it would be? It's just a >>> tool. >>> >>> And there were more talks and the 80-preference got relaxed yet still >>> "not for years" (last talk was 2022?) and sill kernel coding style is >>> here specific. >> >> It's perhaps again about the semantics, this time about the meaning >> of "for years". I don't think there's some strict definition of that >> term, so perhaps different people see it differently. >> >> To get back to the above-mentioned commit bdc48fa11e46, the 80-column >> limit has obviously been lifted, putting the new 100-column limit as > > "Lifted" on *CHECKPATCH*, not on coding style. Do you see the > difference? One is a helper tool which people were using blindly and > wrapping lines without thinking, claiming that checkpatch told them to > do so. Other is the actual coding style. > > You claim that coding style was changed. This never happened. It was obviously changed in the commit bdc48fa11e46, by making the 80-column width preferred, instead of if being mandatory. The way I read the changes to the coding style introduced in that commit, it's now possible to go over 80 columns, up to 100 columns, _if_ that actually improves the readability of the source code. Just like enforcing the 80-column blindly can make the code much less readable, it's also that going liberally to 100 columns can make the code even less readable. To me, those rules aren't to be followed blindly, but the resulting readability of the code should be the deciding factor for pretty much each line of the code. > And my first - really the first - comment here was also precise > mentioning that difference: > > "Please wrap code according to coding style (*checkpatch is not* a > coding style description, but only a tool), so at 80." Again, I think that the readability should be the deciding factor.
On 18/01/2025 10:43, Dragan Simic wrote: >>> >>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: deprecate >>> 80-column warning, 2020-05-29), which clearly shows that the 80-column >>> rule is still _preferred_, but no longer _mandatory_. >> >> I brought that commit, but nice that you also found it. >> >> Still: read the coding style, not checkpatch tool. >> >>>>> 80 columns is really not much (for the record, I've been around when >>>>> using 80x25 _physical_ CRT screens was the norm). >>>> >>>> You mistake agreement on dropping strong restriction in 2020 in >>>> checkpatch, which is "not for years" and even read that commit: "Yes, >>>> staying withing 80 columns is certainly still _preferred_." >>>> >>>> Checkpatch is not coding style. Since when it would be? It's just a >>>> tool. >>>> >>>> And there were more talks and the 80-preference got relaxed yet still >>>> "not for years" (last talk was 2022?) and sill kernel coding style is >>>> here specific. >>> >>> It's perhaps again about the semantics, this time about the meaning >>> of "for years". I don't think there's some strict definition of that >>> term, so perhaps different people see it differently. >>> >>> To get back to the above-mentioned commit bdc48fa11e46, the 80-column >>> limit has obviously been lifted, putting the new 100-column limit as >> >> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the Repeating myself about because you are not addressing the actual difference. >> difference? One is a helper tool which people were using blindly and >> wrapping lines without thinking, claiming that checkpatch told them to >> do so. Other is the actual coding style. >> >> You claim that coding style was changed. This never happened. > > It was obviously changed in the commit bdc48fa11e46, by making the > 80-column width preferred, instead of if being mandatory. The way > I read the changes to the coding style introduced in that commit, > it's now possible to go over 80 columns, up to 100 columns, _if_ > that actually improves the readability of the source code. The commit is for checkpatch. Point to the change in coding style. You are bringing argument for checkpatch, so only a tool, as argument for coding style. Again, coding style did not change since years. Best regards, Krzysztof
On 2025-01-18 10:52, Krzysztof Kozlowski wrote: > On 18/01/2025 10:43, Dragan Simic wrote: >>>> >>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: >>>> deprecate >>>> 80-column warning, 2020-05-29), which clearly shows that the >>>> 80-column >>>> rule is still _preferred_, but no longer _mandatory_. >>> >>> I brought that commit, but nice that you also found it. >>> >>> Still: read the coding style, not checkpatch tool. >>> >>>>>> 80 columns is really not much (for the record, I've been around >>>>>> when >>>>>> using 80x25 _physical_ CRT screens was the norm). >>>>> >>>>> You mistake agreement on dropping strong restriction in 2020 in >>>>> checkpatch, which is "not for years" and even read that commit: >>>>> "Yes, >>>>> staying withing 80 columns is certainly still _preferred_." >>>>> >>>>> Checkpatch is not coding style. Since when it would be? It's just a >>>>> tool. >>>>> >>>>> And there were more talks and the 80-preference got relaxed yet >>>>> still >>>>> "not for years" (last talk was 2022?) and sill kernel coding style >>>>> is >>>>> here specific. >>>> >>>> It's perhaps again about the semantics, this time about the meaning >>>> of "for years". I don't think there's some strict definition of >>>> that >>>> term, so perhaps different people see it differently. >>>> >>>> To get back to the above-mentioned commit bdc48fa11e46, the >>>> 80-column >>>> limit has obviously been lifted, putting the new 100-column limit as >>> >>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the > > Repeating myself about because you are not addressing the actual > difference. Please see below. >>> difference? One is a helper tool which people were using blindly and >>> wrapping lines without thinking, claiming that checkpatch told them >>> to >>> do so. Other is the actual coding style. >>> >>> You claim that coding style was changed. This never happened. >> >> It was obviously changed in the commit bdc48fa11e46, by making the >> 80-column width preferred, instead of if being mandatory. The way >> I read the changes to the coding style introduced in that commit, >> it's now possible to go over 80 columns, up to 100 columns, _if_ >> that actually improves the readability of the source code. > > The commit is for checkpatch. Point to the change in coding style. You > are bringing argument for checkpatch, so only a tool, as argument for > coding style. Again, coding style did not change since years. Commit bdc48fa11e46 obviously addresses Documentation/process/coding-style.rst as well, as visible in the quotation from the commit below: -The limit on the length of lines is 80 columns and this is a strongly -preferred limit. - -Statements longer than 80 columns will be broken into sensible chunks, unless -exceeding 80 columns significantly increases readability and does not hide -information. Descendants are always substantially shorter than the parent and -are placed substantially to the right. The same applies to function headers -with a long argument list. However, never break user-visible strings such as -printk messages, because that breaks the ability to grep for them. +The preferred limit on the length of a single line is 80 columns. + +Statements longer than 80 columns should be broken into sensible chunks, +unless exceeding 80 columns significantly increases readability and does +not hide information. + +Descendants are always substantially shorter than the parent and are +are placed substantially to the right. A very commonly used style +is to align descendants to a function open parenthesis. + +These same rules are applied to function headers with a long argument list. + +However, never break user-visible strings such as printk messages because +that breaks the ability to grep for them. I think it's obvious that the 80-column width is no longer _strongly_ preferred, but has been demoted to some kind of a bit weaker preference. Also, please note that the coding style explicitly says that the 80- column rule is to be followed "unless exceeding 80 columns significantly increases readability and does not hide information". This just reinforces my opinion that the readability is what matters most when deciding on the column width, instead of following the rules blindly, both when deciding whether to wrap some lines at column 50, for example, or to wrap them at column 98.
On 18/01/2025 11:10, Dragan Simic wrote: > On 2025-01-18 10:52, Krzysztof Kozlowski wrote: >> On 18/01/2025 10:43, Dragan Simic wrote: >>>>> >>>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: >>>>> deprecate >>>>> 80-column warning, 2020-05-29), which clearly shows that the >>>>> 80-column >>>>> rule is still _preferred_, but no longer _mandatory_. >>>> >>>> I brought that commit, but nice that you also found it. >>>> >>>> Still: read the coding style, not checkpatch tool. >>>> >>>>>>> 80 columns is really not much (for the record, I've been around >>>>>>> when >>>>>>> using 80x25 _physical_ CRT screens was the norm). >>>>>> >>>>>> You mistake agreement on dropping strong restriction in 2020 in >>>>>> checkpatch, which is "not for years" and even read that commit: >>>>>> "Yes, >>>>>> staying withing 80 columns is certainly still _preferred_." >>>>>> >>>>>> Checkpatch is not coding style. Since when it would be? It's just a >>>>>> tool. >>>>>> >>>>>> And there were more talks and the 80-preference got relaxed yet >>>>>> still >>>>>> "not for years" (last talk was 2022?) and sill kernel coding style >>>>>> is >>>>>> here specific. >>>>> >>>>> It's perhaps again about the semantics, this time about the meaning >>>>> of "for years". I don't think there's some strict definition of >>>>> that >>>>> term, so perhaps different people see it differently. >>>>> >>>>> To get back to the above-mentioned commit bdc48fa11e46, the >>>>> 80-column >>>>> limit has obviously been lifted, putting the new 100-column limit as >>>> >>>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the >> >> Repeating myself about because you are not addressing the actual >> difference. > > Please see below. > >>>> difference? One is a helper tool which people were using blindly and >>>> wrapping lines without thinking, claiming that checkpatch told them >>>> to >>>> do so. Other is the actual coding style. >>>> >>>> You claim that coding style was changed. This never happened. >>> >>> It was obviously changed in the commit bdc48fa11e46, by making the >>> 80-column width preferred, instead of if being mandatory. The way >>> I read the changes to the coding style introduced in that commit, >>> it's now possible to go over 80 columns, up to 100 columns, _if_ >>> that actually improves the readability of the source code. >> >> The commit is for checkpatch. Point to the change in coding style. You >> are bringing argument for checkpatch, so only a tool, as argument for >> coding style. Again, coding style did not change since years. > > Commit bdc48fa11e46 obviously addresses > Documentation/process/coding-style.rst > as well, as visible in the quotation from the commit below: Yes. > > -The limit on the length of lines is 80 columns and this is a strongly 80 is here... > -preferred limit. > - > -Statements longer than 80 columns will be broken into sensible > chunks, unless > -exceeding 80 columns significantly increases readability and does not > hide > -information. Descendants are always substantially shorter than the > parent and > -are placed substantially to the right. The same applies to function > headers > -with a long argument list. However, never break user-visible strings > such as > -printk messages, because that breaks the ability to grep for them. > +The preferred limit on the length of a single line is 80 columns. > + > +Statements longer than 80 columns should be broken into sensible 80 is here as well. So now to your original statement: " but the 100-column limit for the kernel code has been in effect for years." Where is 100? Only in checkpatch. There is no 100 limit in kernel coding style. The change in coding style and checkpatch was partially done because of your understanding: reading checkpatch output as a rule. But this was never a correct approach and still is not. So whatever checkpatch is telling you, e.g. "100 column limit", is not coding style. It's only checkpatch, a tool trying to help you. > chunks, > +unless exceeding 80 columns significantly increases readability and > does > +not hide information. > + > +Descendants are always substantially shorter than the parent and are > +are placed substantially to the right. A very commonly used style > +is to align descendants to a function open parenthesis. > + > +These same rules are applied to function headers with a long argument > list. > + > +However, never break user-visible strings such as printk messages > because > +that breaks the ability to grep for them. > > I think it's obvious that the 80-column width is no longer _strongly_ > preferred, but has been demoted to some kind of a bit weaker preference. Yes, but this is not what you said before and this is not what I questioned. > > Also, please note that the coding style explicitly says that the 80- > column rule is to be followed "unless exceeding 80 columns significantly > increases readability and does not hide information". I already said it earlier... so yeah, we keep repeating ourselves while discussing original point claiming now something else than we actually discuss. Best regards, Krzysztof
On 2025-01-18 11:29, Krzysztof Kozlowski wrote: > On 18/01/2025 11:10, Dragan Simic wrote: >> On 2025-01-18 10:52, Krzysztof Kozlowski wrote: >>> On 18/01/2025 10:43, Dragan Simic wrote: >>>>>> >>>>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: >>>>>> deprecate >>>>>> 80-column warning, 2020-05-29), which clearly shows that the >>>>>> 80-column >>>>>> rule is still _preferred_, but no longer _mandatory_. >>>>> >>>>> I brought that commit, but nice that you also found it. >>>>> >>>>> Still: read the coding style, not checkpatch tool. >>>>> >>>>>>>> 80 columns is really not much (for the record, I've been around >>>>>>>> when >>>>>>>> using 80x25 _physical_ CRT screens was the norm). >>>>>>> >>>>>>> You mistake agreement on dropping strong restriction in 2020 in >>>>>>> checkpatch, which is "not for years" and even read that commit: >>>>>>> "Yes, >>>>>>> staying withing 80 columns is certainly still _preferred_." >>>>>>> >>>>>>> Checkpatch is not coding style. Since when it would be? It's just >>>>>>> a >>>>>>> tool. >>>>>>> >>>>>>> And there were more talks and the 80-preference got relaxed yet >>>>>>> still >>>>>>> "not for years" (last talk was 2022?) and sill kernel coding >>>>>>> style >>>>>>> is >>>>>>> here specific. >>>>>> >>>>>> It's perhaps again about the semantics, this time about the >>>>>> meaning >>>>>> of "for years". I don't think there's some strict definition of >>>>>> that >>>>>> term, so perhaps different people see it differently. >>>>>> >>>>>> To get back to the above-mentioned commit bdc48fa11e46, the >>>>>> 80-column >>>>>> limit has obviously been lifted, putting the new 100-column limit >>>>>> as >>>>> >>>>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the >>> >>> Repeating myself about because you are not addressing the actual >>> difference. >> >> Please see below. >> >>>>> difference? One is a helper tool which people were using blindly >>>>> and >>>>> wrapping lines without thinking, claiming that checkpatch told them >>>>> to >>>>> do so. Other is the actual coding style. >>>>> >>>>> You claim that coding style was changed. This never happened. >>>> >>>> It was obviously changed in the commit bdc48fa11e46, by making the >>>> 80-column width preferred, instead of if being mandatory. The way >>>> I read the changes to the coding style introduced in that commit, >>>> it's now possible to go over 80 columns, up to 100 columns, _if_ >>>> that actually improves the readability of the source code. >>> >>> The commit is for checkpatch. Point to the change in coding style. >>> You >>> are bringing argument for checkpatch, so only a tool, as argument for >>> coding style. Again, coding style did not change since years. >> >> Commit bdc48fa11e46 obviously addresses >> Documentation/process/coding-style.rst >> as well, as visible in the quotation from the commit below: > > Yes. > >> >> -The limit on the length of lines is 80 columns and this is a >> strongly > > 80 is here... > >> -preferred limit. >> - >> -Statements longer than 80 columns will be broken into sensible >> chunks, unless >> -exceeding 80 columns significantly increases readability and does >> not >> hide >> -information. Descendants are always substantially shorter than the >> parent and >> -are placed substantially to the right. The same applies to >> function >> headers >> -with a long argument list. However, never break user-visible >> strings >> such as >> -printk messages, because that breaks the ability to grep for them. >> +The preferred limit on the length of a single line is 80 columns. >> + >> +Statements longer than 80 columns should be broken into sensible > > 80 is here as well. > > So now to your original statement: > " but the 100-column limit > for the kernel code has been in effect for years." > > Where is 100? Only in checkpatch. There is no 100 limit in kernel > coding > style. Yes, "100" is in checkpatch only, but the coding style explicitly says that going over the 80-column limit it fine if it improves the readability. Thus, going over the 80 columns has been allowed "for years", whatever one finds that term to mean, or more precisely since mid-2020, and having "100" present in checkpatch establishes "100" as the effective "hard" limit. > The change in coding style and checkpatch was partially done because of > your understanding: reading checkpatch output as a rule. But this was > never a correct approach and still is not. So whatever checkpatch is > telling you, e.g. "100 column limit", is not coding style. It's only > checkpatch, a tool trying to help you. No, that isn't my understanding. I don't take checkpatch's output as some kind of mandatory rules; however, what checkpatch does and suggests should be based on the coding style, and if checkpatch advises wrongly, it should be fixed instead of being accused to be invalid and pointless. Though, in this particular case, checkpatch does it right. The coding style explicitly says that going over the 80-column limit is fine if that improves the readability, and checkpatch follows that by allowing up to 100 columns. >> chunks, >> +unless exceeding 80 columns significantly increases readability >> and >> does >> +not hide information. >> + >> +Descendants are always substantially shorter than the parent and >> are >> +are placed substantially to the right. A very commonly used style >> +is to align descendants to a function open parenthesis. >> + >> +These same rules are applied to function headers with a long >> argument >> list. >> + >> +However, never break user-visible strings such as printk messages >> because >> +that breaks the ability to grep for them. >> >> I think it's obvious that the 80-column width is no longer _strongly_ >> preferred, but has been demoted to some kind of a bit weaker >> preference. > > Yes, but this is not what you said before and this is not what I > questioned. It is, if you read what I wrote above carefully. The 100-column width limit has been in effect "for years", and has been defined by the combination of the coding style and checkpatch. The former says that going over 80 columns is fine, and the latter limits that to 100 columns, to prevent some very long lines. >> Also, please note that the coding style explicitly says that the 80- >> column rule is to be followed "unless exceeding 80 columns >> significantly >> increases readability and does not hide information". > > I already said it earlier... so yeah, we keep repeating ourselves while > discussing original point claiming now something else than we actually > discuss. I think it's again about the semantics. :) Please see above.
On Sat, Jan 18, 2025 at 5:45 AM Dragan Simic <dsimic@manjaro.org> wrote: > > On 2025-01-18 11:29, Krzysztof Kozlowski wrote: > > On 18/01/2025 11:10, Dragan Simic wrote: > >> On 2025-01-18 10:52, Krzysztof Kozlowski wrote: > >>> On 18/01/2025 10:43, Dragan Simic wrote: > >>>>>> > >>>>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: > >>>>>> deprecate > >>>>>> 80-column warning, 2020-05-29), which clearly shows that the > >>>>>> 80-column > >>>>>> rule is still _preferred_, but no longer _mandatory_. > >>>>> > >>>>> I brought that commit, but nice that you also found it. > >>>>> > >>>>> Still: read the coding style, not checkpatch tool. > >>>>> > >>>>>>>> 80 columns is really not much (for the record, I've been around > >>>>>>>> when > >>>>>>>> using 80x25 _physical_ CRT screens was the norm). > >>>>>>> > >>>>>>> You mistake agreement on dropping strong restriction in 2020 in > >>>>>>> checkpatch, which is "not for years" and even read that commit: > >>>>>>> "Yes, > >>>>>>> staying withing 80 columns is certainly still _preferred_." > >>>>>>> > >>>>>>> Checkpatch is not coding style. Since when it would be? It's just > >>>>>>> a > >>>>>>> tool. > >>>>>>> > >>>>>>> And there were more talks and the 80-preference got relaxed yet > >>>>>>> still > >>>>>>> "not for years" (last talk was 2022?) and sill kernel coding > >>>>>>> style > >>>>>>> is > >>>>>>> here specific. > >>>>>> > >>>>>> It's perhaps again about the semantics, this time about the > >>>>>> meaning > >>>>>> of "for years". I don't think there's some strict definition of > >>>>>> that > >>>>>> term, so perhaps different people see it differently. > >>>>>> > >>>>>> To get back to the above-mentioned commit bdc48fa11e46, the > >>>>>> 80-column > >>>>>> limit has obviously been lifted, putting the new 100-column limit > >>>>>> as > >>>>> > >>>>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the > >>> > >>> Repeating myself about because you are not addressing the actual > >>> difference. > >> > >> Please see below. > >> > >>>>> difference? One is a helper tool which people were using blindly > >>>>> and > >>>>> wrapping lines without thinking, claiming that checkpatch told them > >>>>> to > >>>>> do so. Other is the actual coding style. > >>>>> > >>>>> You claim that coding style was changed. This never happened. > >>>> > >>>> It was obviously changed in the commit bdc48fa11e46, by making the > >>>> 80-column width preferred, instead of if being mandatory. The way > >>>> I read the changes to the coding style introduced in that commit, > >>>> it's now possible to go over 80 columns, up to 100 columns, _if_ > >>>> that actually improves the readability of the source code. > >>> > >>> The commit is for checkpatch. Point to the change in coding style. > >>> You > >>> are bringing argument for checkpatch, so only a tool, as argument for > >>> coding style. Again, coding style did not change since years. > >> > >> Commit bdc48fa11e46 obviously addresses > >> Documentation/process/coding-style.rst > >> as well, as visible in the quotation from the commit below: > > > > Yes. > > > >> > >> -The limit on the length of lines is 80 columns and this is a > >> strongly > > > > 80 is here... > > > >> -preferred limit. > >> - > >> -Statements longer than 80 columns will be broken into sensible > >> chunks, unless > >> -exceeding 80 columns significantly increases readability and does > >> not > >> hide > >> -information. Descendants are always substantially shorter than the > >> parent and > >> -are placed substantially to the right. The same applies to > >> function > >> headers > >> -with a long argument list. However, never break user-visible > >> strings > >> such as > >> -printk messages, because that breaks the ability to grep for them. > >> +The preferred limit on the length of a single line is 80 columns. > >> + > >> +Statements longer than 80 columns should be broken into sensible > > > > 80 is here as well. > > > > So now to your original statement: > > " but the 100-column limit > > for the kernel code has been in effect for years." > > > > Where is 100? Only in checkpatch. There is no 100 limit in kernel > > coding > > style. > > Yes, "100" is in checkpatch only, but the coding style explicitly > says that going over the 80-column limit it fine if it improves > the readability. Thus, going over the 80 columns has been allowed > "for years", whatever one finds that term to mean, or more precisely > since mid-2020, and having "100" present in checkpatch establishes > "100" as the effective "hard" limit. > > > The change in coding style and checkpatch was partially done because of > > your understanding: reading checkpatch output as a rule. But this was > > never a correct approach and still is not. So whatever checkpatch is > > telling you, e.g. "100 column limit", is not coding style. It's only > > checkpatch, a tool trying to help you. > > No, that isn't my understanding. I don't take checkpatch's output > as some kind of mandatory rules; however, what checkpatch does and > suggests should be based on the coding style, and if checkpatch > advises wrongly, it should be fixed instead of being accused to be > invalid and pointless. > > Though, in this particular case, checkpatch does it right. The > coding style explicitly says that going over the 80-column limit > is fine if that improves the readability, and checkpatch follows > that by allowing up to 100 columns. > > >> chunks, > >> +unless exceeding 80 columns significantly increases readability > >> and > >> does > >> +not hide information. > >> + > >> +Descendants are always substantially shorter than the parent and > >> are > >> +are placed substantially to the right. A very commonly used style > >> +is to align descendants to a function open parenthesis. > >> + > >> +These same rules are applied to function headers with a long > >> argument > >> list. > >> + > >> +However, never break user-visible strings such as printk messages > >> because > >> +that breaks the ability to grep for them. > >> > >> I think it's obvious that the 80-column width is no longer _strongly_ > >> preferred, but has been demoted to some kind of a bit weaker > >> preference. > > > > Yes, but this is not what you said before and this is not what I > > questioned. > > It is, if you read what I wrote above carefully. The 100-column > width limit has been in effect "for years", and has been defined > by the combination of the coding style and checkpatch. The former > says that going over 80 columns is fine, and the latter limits that > to 100 columns, to prevent some very long lines. I'd like to say I appreciate the 80 / 100 limit on code, as it pushed me to separate out my driver code write operations into a separate function and significantly clean up the code. I apologize if my submissions aren't perfect the first time around. I admit I'm still a baby developer, this is not my day job (although I'd probably love it if it was). This is only the second driver I've written from scratch (first, if you consider this is the second iteration of a driver I wrote before my motorcomm driver). I depend heavily on checkpatch and the feedback from maintainers, as well as the coding style from similar drivers. The style and coding policies that employed kernel maintainers and developers have committed to heart I need to look up every time I submit something. So, thank you for your feedback. > > >> Also, please note that the coding style explicitly says that the 80- > >> column rule is to be followed "unless exceeding 80 columns > >> significantly > >> increases readability and does not hide information". > > > > I already said it earlier... so yeah, we keep repeating ourselves while > > discussing original point claiming now something else than we actually > > discuss. > > I think it's again about the semantics. :) Please see above.
On Sat Jan 18, 2025 at 9:41 AM CET, Krzysztof Kozlowski wrote: > On 16/01/2025 17:53, Diederik de Haas wrote: > > On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote: > >> On 15/01/2025 02:26, Peter Geis wrote: > >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > >>> index 7d992c3c01ce..181a900d41f9 100644 > >>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi > >>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi > >>> @@ -903,6 +903,43 @@ u2phy_host: host-port { > >>> }; > >>> }; > >>> > >>> + usb3phy: usb3-phy@ff460000 { > >>> + compatible = "rockchip,rk3328-usb3phy"; > >>> + reg = <0x0 0xff460000 0x0 0x10000>; > >>> + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; > >> > >> Please wrap code according to coding style (checkpatch is not a coding > >> style description, but only a tool), so at 80. > > > > I'm confused: is it 80 or 100? > > > > I always thought it was 80, but then I saw several patches/commits by > > Coding style is clear: it is 80. It also has caveat about code > readability and several maintainers have their own preference. > > > Dragan Simic which deliberately changed code to make use of 100. > > Being fed up with my own confusion, I submitted a PR to > > https://github.com/gregkh/kernel-coding-style/ which got accepted: > > https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a > > That's not kernel. That's Greg... FWIW: what Greg and Linus think/say is relevant to me. > > So now both the vim plugins code and README say 100. > > But as noted in my commit message: > > > > Note that the current upstream 'Linux kernel coding style' does NOT > > mention the 100 char limit, but only mentions the preferred max length > > of 80. > > > > Or is it 100 for code, but 80 for DeviceTree files and bindings? > > From where did you get 100? Checkpatch, right? Kernel coding style is > clear, there is no discussion, no mentioning 100: > > "The preferred limit on the length of a single line is 80 columns. " > > So to be clear: all DTS, all DT bindings, all code maintained by me and > some maintainers follows above (and further - there is caveat) > instruction from coding style. Some maintainers follow other rules and > that's fine. But indeed, before Greg or Linus (likely) see it, a patch submitter needs to convince the (subsystem) maintainer that it is an improvement. Thanks for the clarification :-) Cheers, Diederik
diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi index 7d992c3c01ce..181a900d41f9 100644 --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi @@ -903,6 +903,43 @@ u2phy_host: host-port { }; }; + usb3phy: usb3-phy@ff460000 { + compatible = "rockchip,rk3328-usb3phy"; + reg = <0x0 0xff460000 0x0 0x10000>; + clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>; + clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe"; + interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "bvalid", "id", "linestate", "rxdet"; + resets = <&cru SRST_USB3PHY_U2>, + <&cru SRST_USB3PHY_U3>, + <&cru SRST_USB3PHY_PIPE>, + <&cru SRST_USB3OTG_UTMI>, + <&cru SRST_USB3PHY_OTG_P>, + <&cru SRST_USB3PHY_PIPE_P>; + reset-names = "usb3phy-u2-por", "usb3phy-u3-por", + "usb3phy-pipe-mac", "usb3phy-utmi-mac", + "usb3phy-utmi-apb", "usb3phy-pipe-apb"; + #address-cells = <2>; + #size-cells = <2>; + ranges; + status = "disabled"; + + usb3phy_utmi: utmi-port@ff470000 { + compatible = "rockchip,rk3328-usb3phy-utmi"; + reg = <0x0 0xff470000 0x0 0x8000>; + #phy-cells = <0>; + status = "disabled"; + }; + + usb3phy_pipe: pipe-port@ff478000 { + compatible = "rockchip,rk3328-usb3phy-pipe"; + reg = <0x0 0xff478000 0x0 0x8000>; + #phy-cells = <0>; + status = "disabled"; + }; + }; + sdmmc: mmc@ff500000 { compatible = "rockchip,rk3328-dw-mshc", "rockchip,rk3288-dw-mshc"; reg = <0x0 0xff500000 0x0 0x4000>; @@ -1067,6 +1104,8 @@ usbdrd3: usb@ff600000 { clock-names = "ref_clk", "suspend_clk", "bus_clk"; dr_mode = "otg"; + phys = <&usb3phy_utmi>, <&usb3phy_pipe>; + phy-names = "usb2-phy", "usb3-phy"; phy_type = "utmi_wide"; snps,dis-del-phy-power-chg-quirk; snps,dis_enblslpm_quirk;
Add the node for the rk3328 usb3 phy. This node provides a combined usb2 and usb3 phy which are permenantly tied to the dwc3 usb3 controller. Signed-off-by: Peter Geis <pgwipeout@gmail.com> --- arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++ 1 file changed, 39 insertions(+)