| Message ID | 20241215053639.738890-2-anarsoul@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | arm64: allwinner: a64: fix video output on Pinebook | expand |
Hello Vasily, On 2024-12-15 06:34, Vasily Khoruzhick wrote: > These will be used to explicitly select TCON0 clock parent in dts > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in > TCON0 mux") > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> Looking good to me, as a preparatory patch. Please feel free to include Reviewed-by: Dragan Simic <dsimic@manjaro.org> > --- > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 ++ > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h > b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h > index a8c11c0b4e06..dfba88a5ad0f 100644 > --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h > +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h > @@ -21,7 +21,6 @@ > > /* PLL_VIDEO0 exported for HDMI PHY */ > > -#define CLK_PLL_VIDEO0_2X 8 > #define CLK_PLL_VE 9 > #define CLK_PLL_DDR0 10 > > @@ -32,7 +31,6 @@ > #define CLK_PLL_PERIPH1_2X 14 > #define CLK_PLL_VIDEO1 15 > #define CLK_PLL_GPU 16 > -#define CLK_PLL_MIPI 17 > #define CLK_PLL_HSIC 18 > #define CLK_PLL_DE 19 > #define CLK_PLL_DDR1 20 > diff --git a/include/dt-bindings/clock/sun50i-a64-ccu.h > b/include/dt-bindings/clock/sun50i-a64-ccu.h > index 175892189e9d..4f220ea7a23c 100644 > --- a/include/dt-bindings/clock/sun50i-a64-ccu.h > +++ b/include/dt-bindings/clock/sun50i-a64-ccu.h > @@ -44,7 +44,9 @@ > #define _DT_BINDINGS_CLK_SUN50I_A64_H_ > > #define CLK_PLL_VIDEO0 7 > +#define CLK_PLL_VIDEO0_2X 8 > #define CLK_PLL_PERIPH0 11 > +#define CLK_PLL_MIPI 17 > > #define CLK_CPUX 21 > #define CLK_BUS_MIPI_DSI 28
On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote: > These will be used to explicitly select TCON0 clock parent in dts > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux") > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > --- > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 + You cannot combine these changes. Please run scripts/checkpatch.pl and fix reported warnings. Then please run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. Best regards, Krzysztof
On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote: > > These will be used to explicitly select TCON0 clock parent in dts > > > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux") > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > --- > > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 + Hi Krzysztof, > You cannot combine these changes. The patch basically moves defines out from ccu-sun50i-a64.h to sun50i-a64-ccu.h. How do I split the change without introducing compilation failure? > Please run scripts/checkpatch.pl and fix reported warnings. Then please > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. > Some warnings can be ignored, especially from --strict run, but the code > here looks like it needs a fix. Feel free to get in touch if the warning > is not clear. Yeah, it is not clear what do you want me to do, assuming the previous similar change to sun50i-a64-ccu.h did essentially the same, see 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9 Regards, Vasily > Best regards, > Krzysztof >
On Tue, 17 Dec 2024 10:00:45 -0800 Vasily Khoruzhick <anarsoul@gmail.com> wrote: Hi, > On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote: > > > These will be used to explicitly select TCON0 clock parent in dts > > > > > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux") > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > > --- > > > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > > > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 + > > Hi Krzysztof, > > > You cannot combine these changes. > > The patch basically moves defines out from ccu-sun50i-a64.h to > sun50i-a64-ccu.h. How do I split the change without introducing > compilation failure? You can just have the binding part first, adding the (same) definition to the binding headers. As long as the #define's are not conflicting, this is fine. Then remove the now redundant definitions in the kernel headers, with a subsequent patch. Cheers, Andre > > > Please run scripts/checkpatch.pl and fix reported warnings. Then please > > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. > > Some warnings can be ignored, especially from --strict run, but the code > > here looks like it needs a fix. Feel free to get in touch if the warning > > is not clear. > > Yeah, it is not clear what do you want me to do, assuming the previous > similar change to sun50i-a64-ccu.h did essentially the same, see > 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9 > > Regards, > Vasily > > > Best regards, > > Krzysztof > > >
Hello all, On 2024-12-17 22:15, Andre Przywara wrote: > On Tue, 17 Dec 2024 10:00:45 -0800 > Vasily Khoruzhick <anarsoul@gmail.com> wrote: >> On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> >> wrote: >> > >> > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote: >> > > These will be used to explicitly select TCON0 clock parent in dts >> > > >> > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux") >> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> >> > > --- >> > > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- >> > > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 + >> >> > You cannot combine these changes. >> >> The patch basically moves defines out from ccu-sun50i-a64.h to >> sun50i-a64-ccu.h. How do I split the change without introducing >> compilation failure? > > You can just have the binding part first, adding the (same) definition > to the binding headers. As long as the #define's are not conflicting, > this is fine. > Then remove the now redundant definitions in the kernel headers, with a > subsequent patch. Yes, that would be a way to make it formally correct, but also much less readable and understandable later, as part of the source code repository. FWIW, I find this to be an example of the form being more important than the actual function. >> > Please run scripts/checkpatch.pl and fix reported warnings. Then please >> > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. >> > Some warnings can be ignored, especially from --strict run, but the code >> > here looks like it needs a fix. Feel free to get in touch if the warning >> > is not clear. >> >> Yeah, it is not clear what do you want me to do, assuming the previous >> similar change to sun50i-a64-ccu.h did essentially the same, see >> 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9
On Tue, 17 Dec 2024 23:02:43 +0100 Dragan Simic <dsimic@manjaro.org> wrote: Hi Dragan, > Hello all, > > On 2024-12-17 22:15, Andre Przywara wrote: > > On Tue, 17 Dec 2024 10:00:45 -0800 > > Vasily Khoruzhick <anarsoul@gmail.com> wrote: > >> On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> > >> wrote: > >> > > >> > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote: > >> > > These will be used to explicitly select TCON0 clock parent in dts > >> > > > >> > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux") > >> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > >> > > --- > >> > > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > >> > > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 + > >> > >> > You cannot combine these changes. > >> > >> The patch basically moves defines out from ccu-sun50i-a64.h to > >> sun50i-a64-ccu.h. How do I split the change without introducing > >> compilation failure? > > > > You can just have the binding part first, adding the (same) definition > > to the binding headers. As long as the #define's are not conflicting, > > this is fine. > > Then remove the now redundant definitions in the kernel headers, with a > > subsequent patch. > > Yes, that would be a way to make it formally correct, but also much > less readable and understandable later, as part of the source code > repository. FWIW, I find this to be an example of the form being > more important than the actual function. Not sure I understand your last sentence, exactly, but what Krzysztof pointed out is that one part (the header change in include/dt-bindings) is a DT binding patch, so part of a spec, if you like, the other is Linux *code*. There is the DT rebasing repo, which cherry-picks DT patches, so they form a separate history there, and Linux code has no place in there. U-Boot for instance pull this repo now on a regular basis. So keeping those things strictly separate is really important here. Cheers, Andre. > >> > Please run scripts/checkpatch.pl and fix reported warnings. Then please > >> > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. > >> > Some warnings can be ignored, especially from --strict run, but the code > >> > here looks like it needs a fix. Feel free to get in touch if the warning > >> > is not clear. > >> > >> Yeah, it is not clear what do you want me to do, assuming the previous > >> similar change to sun50i-a64-ccu.h did essentially the same, see > >> 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9 >
Hello Andre, On 2024-12-18 02:38, Andre Przywara wrote: > On Tue, 17 Dec 2024 23:02:43 +0100 > Dragan Simic <dsimic@manjaro.org> wrote: >> On 2024-12-17 22:15, Andre Przywara wrote: >> > On Tue, 17 Dec 2024 10:00:45 -0800 >> > Vasily Khoruzhick <anarsoul@gmail.com> wrote: >> >> On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> >> >> wrote: >> >> > >> >> > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote: >> >> > > These will be used to explicitly select TCON0 clock parent in dts >> >> > > >> >> > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux") >> >> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> >> >> > > --- >> >> > > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- >> >> > > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 + >> >> >> >> > You cannot combine these changes. >> >> >> >> The patch basically moves defines out from ccu-sun50i-a64.h to >> >> sun50i-a64-ccu.h. How do I split the change without introducing >> >> compilation failure? >> > >> > You can just have the binding part first, adding the (same) definition >> > to the binding headers. As long as the #define's are not conflicting, >> > this is fine. >> > Then remove the now redundant definitions in the kernel headers, with a >> > subsequent patch. >> >> Yes, that would be a way to make it formally correct, but also much >> less readable and understandable later, as part of the source code >> repository. FWIW, I find this to be an example of the form being >> more important than the actual function. > > Not sure I understand your last sentence, exactly, Ah, sorry, I also saw the need to expand that sentence a bit, but only after I had already sent my message. :/ > but what Krzysztof pointed out is that one part (the header change in > include/dt-bindings) is a DT binding patch, so part of a spec, if you > like, the other is Linux *code*. There is the DT rebasing repo, which > cherry-picks DT patches, so they form a separate history there, and > Linux code has no place in there. U-Boot for instance pull this > repo now on a regular basis. So keeping those things strictly separate > is really important here. Thanks a lot for your detailed explanation and for reminding me about the DT rebasing repository. [*] I forgot about it for a moment, despite being active in U-Boot development when the repository was introduced, so yes, you're right that we're now in need to keep the bindings and the kernel code separate in patches. However, I can't stop myself from noticing that the way Krzysztof replied left a lot to be desired when it comes to friendliness in general. Such an approach is probably what contributed to me forgetting about the DT rebasing repository for a moment. [*] https://git.kernel.org/pub/scm/linux/kernel/git/devicetree/devicetree-rebasing.git/ >> >> > Please run scripts/checkpatch.pl and fix reported warnings. Then please >> >> > run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. >> >> > Some warnings can be ignored, especially from --strict run, but the code >> >> > here looks like it needs a fix. Feel free to get in touch if the warning >> >> > is not clear. >> >> >> >> Yeah, it is not clear what do you want me to do, assuming the previous >> >> similar change to sun50i-a64-ccu.h did essentially the same, see >> >> 71b597ef5d46a326fb0d5cbfc1c6ff1d73cdc7f9
On Tue, Dec 17, 2024 at 1:15 PM Andre Przywara <andre.przywara@arm.com> wrote: > > On Tue, 17 Dec 2024 10:00:45 -0800 > Vasily Khoruzhick <anarsoul@gmail.com> wrote: > > Hi, > > > On Mon, Dec 16, 2024 at 11:33 PM Krzysztof Kozlowski <krzk@kernel.org> wrote: > > > > > > On Sat, Dec 14, 2024 at 09:34:57PM -0800, Vasily Khoruzhick wrote: > > > > These will be used to explicitly select TCON0 clock parent in dts > > > > > > > > Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux") > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> > > > > --- > > > > drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- > > > > include/dt-bindings/clock/sun50i-a64-ccu.h | 2 + > > > > Hi Krzysztof, > > > > > You cannot combine these changes. > > > > The patch basically moves defines out from ccu-sun50i-a64.h to > > sun50i-a64-ccu.h. How do I split the change without introducing > > compilation failure? > > You can just have the binding part first, adding the (same) definition > to the binding headers. As long as the #define's are not conflicting, > this is fine. > Then remove the now redundant definitions in the kernel headers, with a > subsequent patch. Thanks for the suggestion! I'll address it in v2.
diff --git a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h index a8c11c0b4e06..dfba88a5ad0f 100644 --- a/drivers/clk/sunxi-ng/ccu-sun50i-a64.h +++ b/drivers/clk/sunxi-ng/ccu-sun50i-a64.h @@ -21,7 +21,6 @@ /* PLL_VIDEO0 exported for HDMI PHY */ -#define CLK_PLL_VIDEO0_2X 8 #define CLK_PLL_VE 9 #define CLK_PLL_DDR0 10 @@ -32,7 +31,6 @@ #define CLK_PLL_PERIPH1_2X 14 #define CLK_PLL_VIDEO1 15 #define CLK_PLL_GPU 16 -#define CLK_PLL_MIPI 17 #define CLK_PLL_HSIC 18 #define CLK_PLL_DE 19 #define CLK_PLL_DDR1 20 diff --git a/include/dt-bindings/clock/sun50i-a64-ccu.h b/include/dt-bindings/clock/sun50i-a64-ccu.h index 175892189e9d..4f220ea7a23c 100644 --- a/include/dt-bindings/clock/sun50i-a64-ccu.h +++ b/include/dt-bindings/clock/sun50i-a64-ccu.h @@ -44,7 +44,9 @@ #define _DT_BINDINGS_CLK_SUN50I_A64_H_ #define CLK_PLL_VIDEO0 7 +#define CLK_PLL_VIDEO0_2X 8 #define CLK_PLL_PERIPH0 11 +#define CLK_PLL_MIPI 17 #define CLK_CPUX 21 #define CLK_BUS_MIPI_DSI 28
These will be used to explicitly select TCON0 clock parent in dts Fixes: ca1170b69968 ("clk: sunxi-ng: a64: force select PLL_MIPI in TCON0 mux") Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com> --- drivers/clk/sunxi-ng/ccu-sun50i-a64.h | 2 -- include/dt-bindings/clock/sun50i-a64-ccu.h | 2 ++ 2 files changed, 2 insertions(+), 2 deletions(-)