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

[Simon Glass]

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.

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).

Regards,
Simon