Hi Simon,
On Tue, Jun 12, 2018 at 07:29:15PM -0600, Simon Glass wrote:
Hi Mani,
On 12 June 2018 at 00:14, Manivannan Sadhasivam manivannan.sadhasivam@linaro.org wrote:
Hi Simon,
On Mon, Jun 11, 2018 at 01:38:42PM -0600, Simon Glass wrote:
Hi,
On 11 June 2018 at 09:48, Manivannan Sadhasivam manivannan.sadhasivam@linaro.org wrote:
This commit adds Actions Semi OWL family base clock and S900 SoC specific clock support. For S900 peripheral clock support, only UART clock has been added for now.
Signed-off-by: Manivannan Sadhasivam manivannan.sadhasivam@linaro.org
arch/arm/include/asm/arch-owl/clk_owl.h | 61 +++++++++++++ arch/arm/include/asm/arch-owl/regs_s900.h | 64 +++++++++++++ arch/arm/mach-owl/Kconfig | 2 +- drivers/clk/Kconfig | 1 + drivers/clk/Makefile | 1 + drivers/clk/owl/Kconfig | 12 +++ drivers/clk/owl/Makefile | 4 + drivers/clk/owl/clk_owl.c | 60 +++++++++++++ drivers/clk/owl/clk_s900.c | 104 ++++++++++++++++++++++ 9 files changed, 308 insertions(+), 1 deletion(-) create mode 100644 arch/arm/include/asm/arch-owl/clk_owl.h create mode 100644 arch/arm/include/asm/arch-owl/regs_s900.h create mode 100644 drivers/clk/owl/Kconfig create mode 100644 drivers/clk/owl/Makefile create mode 100644 drivers/clk/owl/clk_owl.c create mode 100644 drivers/clk/owl/clk_s900.c
This doesn't look quite right to me. It looks like you should put all the code in clk_s900.c and delete clk_owl.c.
The intention is to separate generic OWL family and SoC specific part. I know that there isn't much in the OWL part now but since this is just a basic clk support and I will extend this in upcoming days with further peripherals, this makes sense to me.
Also, there are plans to support other OWL family SoCs like S500 and S700. So, in those cases this partition will avoid much code duplication. Moreover, the pattern followed here matches the Linux kernel common clock framework by some means.
I still don't understand this. From the look of it, clk_owl.c has almost nothing in it and all the logic is in the driver.
Agree!
It looks like you definitely don't want to have common driver that will support multiple compatible strings? Is that right?
But then you have this line in the 'generic' code:
{ .compatible = "actions,s900-cmu" },Of course it is hard to see where you are going with a start like this, but I imagine that the next driver you do would have a similar structure to the current one.
So my question is, why not just have the U_BOOT_DRIVER() and other 'common' stuff in each driver? I cannot see what you are gaining. You are losing discoverability since it will be hard for people to find the driver for their actual chip (the compatible string is in one file but all the code is in another).
Makes sense...
Since I don't have common code to share with other family SoC's for now, I agree with you on removing clk_owl.c and moving everything to clk_s900.c. In future we may decide on partitioning the code if there is enough code duplication.
Will send a v2 incorporating your suggestion.
Thanks for the review!
Regards, Mani
Regards, Simon