> Subject: Re: [PATCH v2 3/3] clk: aspeed: add AST2700 clk driver > > Quoting Ryan Chen (2024-08-27 23:27:40) > > diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig index > > 983ef4f36d8c..855b65f2d6dd 100644 > > --- a/drivers/clk/Kconfig > > +++ b/drivers/clk/Kconfig > > @@ -269,6 +269,16 @@ config COMMON_CLK_ASPEED > > The G4 and G5 series, including the ast2400 and ast2500, are > supported > > by this driver. > > > > +config COMMON_CLK_AST2700 > > + bool "Clock driver for AST2700 SoC" > > + depends on ARCH_ASPEED || COMPILE_TEST > > + select MFD_SYSCON > > Why is this a syscon? I will remove it. > > > + select RESET_CONTROLLER > > + help > > + This driver provides support for clock on AST2700 SoC. > > + This driver is responsible for managing the various clocks > required > > + by the peripherals and cores within the AST2700. > > + > > config COMMON_CLK_S2MPS11 > > tristate "Clock driver for S2MPS1X/S5M8767 MFD" > > depends on MFD_SEC_CORE || COMPILE_TEST diff --git > > a/drivers/clk/clk-ast2700.c b/drivers/clk/clk-ast2700.c new file mode > > 100644 index 000000000000..7e0466e73980 > > --- /dev/null > > +++ b/drivers/clk/clk-ast2700.c > > @@ -0,0 +1,1198 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (c) 2024 ASPEED Technology Inc. > > + * Author: Ryan Chen <ryan_chen@xxxxxxxxxxxxxx> */ > > + > > +#include <linux/bits.h> > > +#include <linux/clk-provider.h> > [...] > > + > > +struct ast2700_reset { > > + void __iomem *base; > > + struct ast2700_reset_signal const *signal; > > + struct reset_controller_dev rcdev; }; > > Please move the reset controller to the drivers/reset directory by means of > using an auxiliary device. There are some existing examples in there if you > grep for auxiliary_device in drivers/reset to help guide. Do you mean to have another reset controller driver? If yes, I may need syscon for remap two drivers. > > > + > > +#define to_rc_data(p) container_of(p, struct ast2700_reset, rcdev) > > + > [...] > > + > > +static int ast2700_soc0_clk_init(struct device_node *soc0_node) { > > + struct clk_hw_onecell_data *clk_data; > > + void __iomem *clk_base; > [...] > > + 0, clk_base + > SCU0_CLK_STOP, > > + 28, 0, > > + &ast2700_clk_lock); > > + > > + of_clk_add_hw_provider(soc0_node, of_clk_hw_onecell_get, > > + clk_data); > > + > > + return 0; > > +}; > > + > > +CLK_OF_DECLARE_DRIVER(ast2700_soc0, "aspeed,ast2700-scu0", > > +ast2700_soc0_clk_init); CLK_OF_DECLARE_DRIVER(ast2700_soc1, > > +"aspeed,ast2700-scu1", ast2700_soc1_clk_init); > > Why can't this be a platform driver? Due to clk and reset will be the first driver in core. Do you think all drivers should be platform driver?