On 21-01-15 12:29:08, Adam Ford wrote:
...
> diff --git a/drivers/clk/imx/clk-imx25.c b/drivers/clk/imx/clk-imx25.c
> index a66cabfbf94f..66192fe0a898 100644
> --- a/drivers/clk/imx/clk-imx25.c
> +++ b/drivers/clk/imx/clk-imx25.c
> @@ -73,16 +73,6 @@ enum mx25_clks {
>
> static struct clk *clk[clk_max];
>
> -static struct clk ** const uart_clks[] __initconst = {
> - &clk[uart_ipg_per],
> - &clk[uart1_ipg],
> - &clk[uart2_ipg],
> - &clk[uart3_ipg],
> - &clk[uart4_ipg],
> - &clk[uart5_ipg],
> - NULL
> -};
> -
I'm assuming there is another patch that updatesthe dts files. Right ?
TBH, I'm against the idea of having to call consumer API from a clock provider
driver.
I'm still investigating a way of moving the uart clock control calls in
drivers/serial/imx,
where they belong.
> static int __init __mx25_clocks_init(void __iomem *ccm_base)
> {
> BUG_ON(!ccm_base);
> @@ -228,7 +218,7 @@ static int __init __mx25_clocks_init(void __iomem
> *ccm_base)
> */
> clk_set_parent(clk[cko_sel], clk[ipg]);
>
> - imx_register_uart_clocks(uart_clks);
> + imx_register_uart_clocks(6);
Suggestion: Maybe the number of clocks can be determined by the existing clocks
in that dts node.
Hardcoding is not a good ideea here.
...
>
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> index 47882c51cb85..158fe302a8b7 100644
> --- a/drivers/clk/imx/clk.c
> +++ b/drivers/clk/imx/clk.c
> @@ -147,8 +147,10 @@ void imx_cscmr1_fixup(u32 *val)
> }
>
> #ifndef MODULE
> -static int imx_keep_uart_clocks;
> -static struct clk ** const *imx_uart_clocks;
> +
> +static bool imx_keep_uart_clocks;
> +static int imx_enabled_uart_clocks;
> +static struct clk **imx_uart_clocks;
>
> static int __init imx_keep_uart_clocks_param(char *str)
> {
> @@ -161,24 +163,43 @@ __setup_param("earlycon", imx_keep_uart_earlycon,
> __setup_param("earlyprintk", imx_keep_uart_earlyprintk,
> imx_keep_uart_clocks_param, 0);
>
> -void imx_register_uart_clocks(struct clk ** const clks[])
> +void imx_register_uart_clocks(unsigned int clk_count)
> {
> +#ifdef CONFIG_OF
> if (imx_keep_uart_clocks) {
> int i;
>
> - imx_uart_clocks = clks;
> - for (i = 0; imx_uart_clocks[i]; i++)
> - clk_prepare_enable(*imx_uart_clocks[i]);
> + imx_uart_clocks = kcalloc(clk_count, sizeof(struct clk *),
> GFP_KERNEL);
> + imx_enabled_uart_clocks = 0;
> +
> + for (i = 0; i < clk_count; i++) {
> + imx_uart_clocks[imx_enabled_uart_clocks] =
> of_clk_get(of_stdout, i);
> +
> + /* Stop if there are no more of_stdout references */
> + if (IS_ERR(imx_uart_clocks[imx_enabled_uart_clocks]))
> + return;
> +
> + /* Only enable the clock if it's not NULL */
> + if (imx_uart_clocks[imx_enabled_uart_clocks])
> +
> clk_prepare_enable(imx_uart_clocks[imx_enabled_uart_clocks++]);
> + }
> }
> +#else
> + /* i.MX boards use device trees now. For build tests without
> CONFIG_OF, do nothing */
> + imx_enabled_uart_clocks = 0;
> +#endif
Don't really see the point of this #ifdef here. Just makes the code more messy.
> }
>
> static int __init imx_clk_disable_uart(void)
> {
> - if (imx_keep_uart_clocks && imx_uart_clocks) {
> + if (imx_keep_uart_clocks && imx_enabled_uart_clocks) {
> int i;
>
> - for (i = 0; imx_uart_clocks[i]; i++)
> - clk_disable_unprepare(*imx_uart_clocks[i]);
> + for (i = 0; i < imx_enabled_uart_clocks; i++) {
> + clk_disable_unprepare(imx_uart_clocks[i]);
> + clk_put(imx_uart_clocks[i]);
> + };
> + kfree(imx_uart_clocks);
> }
>
> return 0;
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index 4f04c8287286..7571603bee23 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -11,9 +11,9 @@ extern spinlock_t imx_ccm_lock;
> void imx_check_clocks(struct clk *clks[], unsigned int count);
> void imx_check_clk_hws(struct clk_hw *clks[], unsigned int count);
> #ifndef MODULE
> -void imx_register_uart_clocks(struct clk ** const clks[]);
> +void imx_register_uart_clocks(unsigned int clk_count);
> #else
> -static inline void imx_register_uart_clocks(struct clk ** const clks[])
> +static inline void imx_register_uart_clocks(unsigned int clk_count)
> {
> }
> #endif
> --
> 2.25.1
>
