[U-Boot] [RESEND][PATCH 5/9] clk: Add Actions Semi OWL clock support

[Manivannan Sadhasivam]

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 at 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 at 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 at 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