Re: [PATCH V9 2/7] clk: imx: add scu clock common part

["Lothar Waßmann" <LW@KARO-electronics.de>]

Hi,

On Tue, 11 Dec 2018 03:35:48 +0000 Aisheng Dong wrote:
> > -----Original Message-----
> > From: Stephen Boyd [mailto:sboyd@kernel.org]
> > Quoting Aisheng DONG (2018-12-04 06:39:25)  
> > > diff --git a/drivers/clk/imx/Kconfig b/drivers/clk/imx/Kconfig index
> > > 43a3ecc..63e7b01 100644
> > > --- a/drivers/clk/imx/Kconfig
> > > +++ b/drivers/clk/imx/Kconfig
> > > @@ -3,3 +3,7 @@
> > >  config MXC_CLK
> > >         bool
> > >         depends on ARCH_MXC
> > > +
> > > +config MXC_CLK_SCU  
> > 
> > Is there any reason to make this a hidden option instead of making it a
> > selectable option? It can still depend on ARCH_MXC and ARM64, but otherwise
> > it should be compilable as long as CONFIG_IMX_SCU is defined (this should
> > also be a config we depend on here).
> >   
> 
> This is mostly following the exist using that CLK is selected by SoC config option.
> https://patchwork.kernel.org/patch/10677309/
> 
> As CLK usually is required for platform to run well, so we did not make it selectable.
> 
> > > +       bool
> > > +       depends on ARCH_MXC && ARM64
> > > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> > > f850424..4abed37 100644
> > > --- a/drivers/clk/imx/Makefile
> > > +++ b/drivers/clk/imx/Makefile
> > > @@ -20,6 +20,9 @@ obj-$(CONFIG_MXC_CLK) += \
> > >         clk-pllv4.o \
> > >         clk-sccg-pll.o
> > >
> > > +obj-$(CONFIG_MXC_CLK_SCU) += \
> > > +       clk-scu.o
> > > +
> > >  obj-$(CONFIG_SOC_IMX1)   += clk-imx1.o
> > >  obj-$(CONFIG_SOC_IMX21)  += clk-imx21.o
> > >  obj-$(CONFIG_SOC_IMX25)  += clk-imx25.o diff --git
> > > a/drivers/clk/imx/clk-scu.c b/drivers/clk/imx/clk-scu.c new file mode
> > > 100644 index 0000000..ec8a471
> > > --- /dev/null
> > > +++ b/drivers/clk/imx/clk-scu.c
> > > @@ -0,0 +1,265 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2018 NXP
> > > + *   Dong Aisheng <aisheng.dong@nxp.com>
> > > + */
> > > +
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/err.h>
> > > +#include <linux/slab.h>
> > > +
> > > +#include "clk-scu.h"
> > > +
> > > +struct imx_sc_ipc *ccm_ipc_handle;  
> > 
> > Why does this need to be a global? Can it be in each clk_scu instance instead?
> >   
> 
> No, no need to be in each clk_scu instance.
> There's only one handler.
> 
Shouldn't it be 'static'?


Lothar Waßmann