[U-Boot] [PATCH V2 01/14] mxs: add parameter base_addr for mxs_set_lcdclk

[Stefano Babic <sbabic@denx.de>]

Hi Peng,


On 20/10/2015 13:39, Peng Fan wrote:
> Change mxs_set_lcdclk prototype to add a new parameter
> base_addr. There are two LCD interfaces for i.MX6SX,
> we may support LCDIF1 or LCDIF2.
> 

ok - from your commit message I am waiting that both LCDIF1 and LCDIF2
can be supported, and a user (board maintainer) can switch between them.
And in later patch mxs_set_lcdclk() is avalilable for mx6sx and mx6ul,
that makes this inconsistent.

But I do not understand how the support for the second LCDIF is
implemented. I should see that the register set points to LCDIF1 or LCDIF2.

More as extending to both LCDIF1 and LCDIF2, it looks to me that this is
consistent with mxs_set_lcdclk() in mx7.


> Signed-off-by: Peng Fan <Peng.Fan@freescale.com>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Anatolij Gustschin <agust@denx.de>
> ---
> 
> V2:
>  none
> 
>  arch/arm/cpu/arm926ejs/mxs/clock.c    | 2 +-
>  arch/arm/include/asm/arch-mxs/clock.h | 2 +-
>  drivers/video/mxsfb.c                 | 2 +-
>  3 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/cpu/arm926ejs/mxs/clock.c b/arch/arm/cpu/arm926ejs/mxs/clock.c
> index e9d8800..dac8dc6 100644
> --- a/arch/arm/cpu/arm926ejs/mxs/clock.c
> +++ b/arch/arm/cpu/arm926ejs/mxs/clock.c
> @@ -309,7 +309,7 @@ void mxs_set_ssp_busclock(unsigned int bus, uint32_t freq)
>  		bus, tgtclk, freq);
>  }
>  
> -void mxs_set_lcdclk(uint32_t freq)
> +void mxs_set_lcdclk(uint32_t base_addr, uint32_t freq)
>  {
>  	struct mxs_clkctrl_regs *clkctrl_regs =
>  		(struct mxs_clkctrl_regs *)MXS_CLKCTRL_BASE;
> diff --git a/arch/arm/include/asm/arch-mxs/clock.h b/arch/arm/include/asm/arch-mxs/clock.h

This patch should generate warnings, because base_addr is not used. You
should at least add an  __maybe_unused attribute.

Anyway, I am asking myself if it makes sense to pass an address. Really,
you want to pass an index if you want to support LCDIF1 or LCDIF2. This
makes more sense as passing an address. In you code later (patch 7/14),
you check the address like an index:


	if (base_addr == LCDIF1_BASE_ADDR) {

This makes inconsistent. If you need it as address, this means you will
use it as address later, casting to a register structure. If you use as
I understand as index, that please use an enumeration for it.

> index fc9d75b..8840335 100644
> --- a/arch/arm/include/asm/arch-mxs/clock.h
> +++ b/arch/arm/include/asm/arch-mxs/clock.h
> @@ -46,7 +46,7 @@ uint32_t mxc_get_clock(enum mxc_clock clk);
>  void mxs_set_ioclk(enum mxs_ioclock io, uint32_t freq);
>  void mxs_set_sspclk(enum mxs_sspclock ssp, uint32_t freq, int xtal);
>  void mxs_set_ssp_busclock(unsigned int bus, uint32_t freq);
> -void mxs_set_lcdclk(uint32_t freq);
> +void mxs_set_lcdclk(uint32_t base_addr, uint32_t freq);
>  
>  /* Compatibility with the FEC Ethernet driver */
>  #define	imx_get_fecclk()	mxc_get_clock(MXC_AHB_CLK)
> diff --git a/drivers/video/mxsfb.c b/drivers/video/mxsfb.c
> index 03b0f88..eed57d5 100644
> --- a/drivers/video/mxsfb.c
> +++ b/drivers/video/mxsfb.c
> @@ -55,7 +55,7 @@ static void mxs_lcd_init(GraphicDevice *panel,
>  	uint8_t valid_data = 0;
>  
>  	/* Kick in the LCDIF clock */
> -	mxs_set_lcdclk(PS2KHZ(mode->pixclock));
> +	mxs_set_lcdclk(MXS_LCDIF_BASE, PS2KHZ(mode->pixclock));
>  
>  	/* Restart the LCDIF block */
>  	mxs_reset_block(&regs->hw_lcdif_ctrl_reg);
> 

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================