| Message ID | cover.1642582832.git.zong.li@sifive.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Refactor the PRCI driver to reduce the complexity | expand |
On Wed, Jan 19, 2022 at 5:28 PM Zong Li <zong.li@sifive.com> wrote: > > This patch set tries to improve the PRCI driver to reduce the > complexity, we remove the SoCs C files by putting putting all stuff in > each SoCs header file, and include these SoCs-specific header files in > core of PRCI. It can also avoid the W=1 kernel build warnings about > variable defined but not used [-Wunused-const-variable=], like 'commit > 487dc7bb6a0c ("clk: sifive:fu540-prci: Declare static const variable > 'prci_clk_fu540' where it's used")' does. > > This patch set also contains the dt-bindings and dts change, because > we change the macro name for fu540 and fu740 by adding the prefix > respectively. > > Thanks all for your review and suggestions. > > Zong Li (4): > dt-bindings: change the macro name of prci in header files and example > riscv: dts: Change the macro name of prci in each device node > clk: sifive: Add SoCs prefix in each SoCs-dependent data > clk: sifive: Move all stuff into SoCs header files from C files > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 2 +- > .../bindings/pci/sifive,fu740-pcie.yaml | 2 +- > .../bindings/serial/sifive-serial.yaml | 2 +- > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 22 +-- > arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 26 ++-- > drivers/clk/sifive/Makefile | 2 +- > drivers/clk/sifive/fu540-prci.c | 89 ------------ > drivers/clk/sifive/fu540-prci.h | 91 +++++++++++- > drivers/clk/sifive/fu740-prci.c | 134 ------------------ > drivers/clk/sifive/fu740-prci.h | 130 ++++++++++++++++- > drivers/clk/sifive/sifive-prci.c | 5 - > include/dt-bindings/clock/sifive-fu540-prci.h | 8 +- > include/dt-bindings/clock/sifive-fu740-prci.h | 18 +-- > 13 files changed, 254 insertions(+), 277 deletions(-) > delete mode 100644 drivers/clk/sifive/fu540-prci.c > delete mode 100644 drivers/clk/sifive/fu740-prci.c > > -- > 2.31.1 > Hi all, thanks for your review, I'd like to know if anything else can be improved in this patch, or it might be good enough to be picked up. Thanks.
On Wed, 19 Jan 2022 01:28:37 PST (-0800), zong.li@sifive.com wrote: > This patch set tries to improve the PRCI driver to reduce the > complexity, we remove the SoCs C files by putting putting all stuff in > each SoCs header file, and include these SoCs-specific header files in > core of PRCI. It can also avoid the W=1 kernel build warnings about > variable defined but not used [-Wunused-const-variable=], like 'commit > 487dc7bb6a0c ("clk: sifive:fu540-prci: Declare static const variable > 'prci_clk_fu540' where it's used")' does. > > This patch set also contains the dt-bindings and dts change, because > we change the macro name for fu540 and fu740 by adding the prefix > respectively. > > Thanks all for your review and suggestions. > > Zong Li (4): > dt-bindings: change the macro name of prci in header files and example > riscv: dts: Change the macro name of prci in each device node > clk: sifive: Add SoCs prefix in each SoCs-dependent data IIUC these there aren't bisectable: the bindings change will break builds of the DTs and drivers. I'm not sure what's generally the way to go with these, but I always try to avoid broken builds in the middle of patch sets. Aside from that this generally looks good to me, but the DT and clock folks are probably a better bet for a proper review here. Happy to take this through the RISC-V tree, but IMO it's a better candidate for the clock tree so Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # aside from breaking bisect Thanks! > clk: sifive: Move all stuff into SoCs header files from C files > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 2 +- > .../bindings/pci/sifive,fu740-pcie.yaml | 2 +- > .../bindings/serial/sifive-serial.yaml | 2 +- > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 22 +-- > arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 26 ++-- > drivers/clk/sifive/Makefile | 2 +- > drivers/clk/sifive/fu540-prci.c | 89 ------------ > drivers/clk/sifive/fu540-prci.h | 91 +++++++++++- > drivers/clk/sifive/fu740-prci.c | 134 ------------------ > drivers/clk/sifive/fu740-prci.h | 130 ++++++++++++++++- > drivers/clk/sifive/sifive-prci.c | 5 - > include/dt-bindings/clock/sifive-fu540-prci.h | 8 +- > include/dt-bindings/clock/sifive-fu740-prci.h | 18 +-- > 13 files changed, 254 insertions(+), 277 deletions(-) > delete mode 100644 drivers/clk/sifive/fu540-prci.c > delete mode 100644 drivers/clk/sifive/fu740-prci.c
On Sat, Feb 5, 2022 at 2:56 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Wed, 19 Jan 2022 01:28:37 PST (-0800), zong.li@sifive.com wrote: > > This patch set tries to improve the PRCI driver to reduce the > > complexity, we remove the SoCs C files by putting putting all stuff in > > each SoCs header file, and include these SoCs-specific header files in > > core of PRCI. It can also avoid the W=1 kernel build warnings about > > variable defined but not used [-Wunused-const-variable=], like 'commit > > 487dc7bb6a0c ("clk: sifive:fu540-prci: Declare static const variable > > 'prci_clk_fu540' where it's used")' does. > > > > This patch set also contains the dt-bindings and dts change, because > > we change the macro name for fu540 and fu740 by adding the prefix > > respectively. > > > > Thanks all for your review and suggestions. > > > > Zong Li (4): > > dt-bindings: change the macro name of prci in header files and example > > riscv: dts: Change the macro name of prci in each device node > > clk: sifive: Add SoCs prefix in each SoCs-dependent data > > IIUC these there aren't bisectable: the bindings change will break > builds of the DTs and drivers. I'm not sure what's generally the way to > go with these, but I always try to avoid broken builds in the middle of > patch sets. > > Aside from that this generally looks good to me, but the DT and clock > folks are probably a better bet for a proper review here. Happy to take > this through the RISC-V tree, but IMO it's a better candidate for the > clock tree so > > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # aside from breaking bisect > > Thanks! > Many thanks for your review and reminding, and yes, it seems a bit hard there since the DT binding docs and includes need to be a separate patch. > > clk: sifive: Move all stuff into SoCs header files from C files > > > > .../devicetree/bindings/gpio/sifive,gpio.yaml | 2 +- > > .../bindings/pci/sifive,fu740-pcie.yaml | 2 +- > > .../bindings/serial/sifive-serial.yaml | 2 +- > > arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 22 +-- > > arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 26 ++-- > > drivers/clk/sifive/Makefile | 2 +- > > drivers/clk/sifive/fu540-prci.c | 89 ------------ > > drivers/clk/sifive/fu540-prci.h | 91 +++++++++++- > > drivers/clk/sifive/fu740-prci.c | 134 ------------------ > > drivers/clk/sifive/fu740-prci.h | 130 ++++++++++++++++- > > drivers/clk/sifive/sifive-prci.c | 5 - > > include/dt-bindings/clock/sifive-fu540-prci.h | 8 +- > > include/dt-bindings/clock/sifive-fu740-prci.h | 18 +-- > > 13 files changed, 254 insertions(+), 277 deletions(-) > > delete mode 100644 drivers/clk/sifive/fu540-prci.c > > delete mode 100644 drivers/clk/sifive/fu740-prci.c
On Sat, Feb 19, 2022 at 6:23 AM Stephen Boyd <sboyd@kernel.org> wrote: > > Quoting Zong Li (2022-02-06 21:21:50) > > On Sat, Feb 5, 2022 at 2:56 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > > > > > On Wed, 19 Jan 2022 01:28:37 PST (-0800), zong.li@sifive.com wrote: > > > > This patch set tries to improve the PRCI driver to reduce the > > > > complexity, we remove the SoCs C files by putting putting all stuff in > > > > each SoCs header file, and include these SoCs-specific header files in > > > > core of PRCI. It can also avoid the W=1 kernel build warnings about > > > > variable defined but not used [-Wunused-const-variable=], like 'commit > > > > 487dc7bb6a0c ("clk: sifive:fu540-prci: Declare static const variable > > > > 'prci_clk_fu540' where it's used")' does. > > > > > > > > This patch set also contains the dt-bindings and dts change, because > > > > we change the macro name for fu540 and fu740 by adding the prefix > > > > respectively. > > > > > > > > Thanks all for your review and suggestions. > > > > > > > > Zong Li (4): > > > > dt-bindings: change the macro name of prci in header files and example > > > > riscv: dts: Change the macro name of prci in each device node > > > > clk: sifive: Add SoCs prefix in each SoCs-dependent data > > > > > > IIUC these there aren't bisectable: the bindings change will break > > > builds of the DTs and drivers. I'm not sure what's generally the way to > > > go with these, but I always try to avoid broken builds in the middle of > > > patch sets. > > > > > > Aside from that this generally looks good to me, but the DT and clock > > > folks are probably a better bet for a proper review here. Happy to take > > > this through the RISC-V tree, but IMO it's a better candidate for the > > > clock tree so > > > > > > Acked-by: Palmer Dabbelt <palmer@rivosinc.com> # aside from breaking bisect > > > > > > Thanks! > > > > > > > Many thanks for your review and reminding, and yes, it seems a bit > > hard there since the DT binding docs and includes need to be a > > separate patch. > > > > Why not add new defines with the same numbers in a different file? Then > a cycle or two later the conflicting defines can be removed? The driver > can include the new file with the new defines while the old defines can > be changed in parallel? Hi Stephon, many thanks for your tips. I'm afraid that I don't completely understand, does it mean that I can create a new temporary file to define these numbers for the driver, and add a patch to remove this file in the same patch set. If I understand correctly, let me prepare the next version for doing that. Thanks.
This patch set tries to improve the PRCI driver to reduce the complexity, we remove the SoCs C files by putting putting all stuff in each SoCs header file, and include these SoCs-specific header files in core of PRCI. It can also avoid the W=1 kernel build warnings about variable defined but not used [-Wunused-const-variable=], like 'commit 487dc7bb6a0c ("clk: sifive:fu540-prci: Declare static const variable 'prci_clk_fu540' where it's used")' does. This patch set also contains the dt-bindings and dts change, because we change the macro name for fu540 and fu740 by adding the prefix respectively. Thanks all for your review and suggestions. Zong Li (4): dt-bindings: change the macro name of prci in header files and example riscv: dts: Change the macro name of prci in each device node clk: sifive: Add SoCs prefix in each SoCs-dependent data clk: sifive: Move all stuff into SoCs header files from C files .../devicetree/bindings/gpio/sifive,gpio.yaml | 2 +- .../bindings/pci/sifive,fu740-pcie.yaml | 2 +- .../bindings/serial/sifive-serial.yaml | 2 +- arch/riscv/boot/dts/sifive/fu540-c000.dtsi | 22 +-- arch/riscv/boot/dts/sifive/fu740-c000.dtsi | 26 ++-- drivers/clk/sifive/Makefile | 2 +- drivers/clk/sifive/fu540-prci.c | 89 ------------ drivers/clk/sifive/fu540-prci.h | 91 +++++++++++- drivers/clk/sifive/fu740-prci.c | 134 ------------------ drivers/clk/sifive/fu740-prci.h | 130 ++++++++++++++++- drivers/clk/sifive/sifive-prci.c | 5 - include/dt-bindings/clock/sifive-fu540-prci.h | 8 +- include/dt-bindings/clock/sifive-fu740-prci.h | 18 +-- 13 files changed, 254 insertions(+), 277 deletions(-) delete mode 100644 drivers/clk/sifive/fu540-prci.c delete mode 100644 drivers/clk/sifive/fu740-prci.c