[U-Boot] [PATCH v3 1/8] wandboard: add Future Eletronics 7" WVGA LCD extension board

[Stefano Babic <sbabic@denx.de>]

Hi Otavio,

one minor point.

On 16/12/2013 23:43, Otavio Salvador wrote:

> +int board_video_skip(void)
> +{
> +	int i;
> +	int ret;
> +	int detected = 0;
> +	char const *panel = getenv("panel");
> +	if (!panel) {
> +		for (i = 0; i < ARRAY_SIZE(displays); i++) {
> +			struct display_info_t const *dev = displays+i;
> +			if (dev->detect && dev->detect(dev)) {
> +				panel = dev->mode.name;
> +				detected = 1;
> +				break;
> +			}
> +		}
> +		if (!panel) {
> +			panel = displays[0].mode.name;
> +			printf("No panel detected: default to %s\n", panel);
> +			i = 0;
> +		}
> +	} else {
> +		for (i = 0; i < ARRAY_SIZE(displays); i++) {
> +			if (!strcmp(panel, displays[i].mode.name))
> +				break;
> +		}
> +	}
> +	if (i < ARRAY_SIZE(displays)) {
> +		ret = ipuv3_fb_init(&displays[i].mode, 0,
> +				    displays[i].pixfmt);
> +
> +		if (!ret) {
> +			displays[i].enable(displays+i);
> +			printf("Display: %s (%ux%u) %s\n",
> +			       displays[i].mode.name,
> +			       displays[i].mode.xres,
> +			       displays[i].mode.yres,
> +			       (detected ? "[auto]" : ""));
> +		} else
> +			printf("LCD %s cannot be configured: %d\n",
> +			       displays[i].mode.name, ret);
> +	} else {
> +		printf("unsupported panel %s\n", panel);
> +		return -EINVAL;
> +	}
> +
> +	return 0;
>  }
>  

This is identical to the nitrogen one, and quite identical to the same
function in sabresd. Or better, function in sabresd is older and has not
received the fixes as nitrogen (line with dev->detect).

IMHO the function is generic to be factorized. The display_info_t
structure lets us to specialize the behavior for each board, and the
generic part can be put in a common part.



>  static void setup_display(void)
>  {
>  	struct mxc_ccm_reg *mxc_ccm = (struct mxc_ccm_reg *)CCM_BASE_ADDR;
> +	struct iomuxc *iomux = (struct iomuxc *)IOMUXC_BASE_ADDR;
>  	int reg;
>  
>  	enable_ipu_clock();
>  	imx_setup_hdmi();
>  
> +	/* Turn on LDB0, LDB1, IPU,IPU DI0 clocks */
> +	reg = __raw_readl(&mxc_ccm->CCGR3);
> +	reg |=  MXC_CCM_CCGR3_LDB_DI0_MASK | MXC_CCM_CCGR3_LDB_DI1_MASK;
> +	writel(reg, &mxc_ccm->CCGR3);
> +
> +	/* set LDB0, LDB1 clk select to 011/011 */
> +	reg = readl(&mxc_ccm->cs2cdr);
> +	reg &= ~(MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_MASK
> +		 | MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_MASK);
> +	reg |= (3 << MXC_CCM_CS2CDR_LDB_DI0_CLK_SEL_OFFSET)
> +	      | (3 << MXC_CCM_CS2CDR_LDB_DI1_CLK_SEL_OFFSET);
> +	writel(reg, &mxc_ccm->cs2cdr);
> +
> +	reg = readl(&mxc_ccm->cscmr2);
> +	reg |= MXC_CCM_CSCMR2_LDB_DI0_IPU_DIV | MXC_CCM_CSCMR2_LDB_DI1_IPU_DIV;
> +	writel(reg, &mxc_ccm->cscmr2);
> +
>  	reg = readl(&mxc_ccm->chsccdr);
>  	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
>  		<< MXC_CCM_CHSCCDR_IPU1_DI0_CLK_SEL_OFFSET);
> +	reg |= (CHSCCDR_CLK_SEL_LDB_DI0
> +		<< MXC_CCM_CHSCCDR_IPU1_DI1_CLK_SEL_OFFSET);
>  	writel(reg, &mxc_ccm->chsccdr);
> +
> +	reg = IOMUXC_GPR2_BGREF_RRMODE_EXTERNAL_RES
> +	     | IOMUXC_GPR2_DI1_VS_POLARITY_ACTIVE_LOW
> +	     | IOMUXC_GPR2_DI0_VS_POLARITY_ACTIVE_LOW
> +	     | IOMUXC_GPR2_BIT_MAPPING_CH1_SPWG
> +	     | IOMUXC_GPR2_DATA_WIDTH_CH1_18BIT
> +	     | IOMUXC_GPR2_BIT_MAPPING_CH0_SPWG
> +	     | IOMUXC_GPR2_DATA_WIDTH_CH0_18BIT
> +	     | IOMUXC_GPR2_LVDS_CH0_MODE_DISABLED
> +	     | IOMUXC_GPR2_LVDS_CH1_MODE_ENABLED_DI0;
> +	writel(reg, &iomux->gpr[2]);
> +
> +	reg = readl(&iomux->gpr[3]);
> +	reg = (reg & ~(IOMUXC_GPR3_LVDS1_MUX_CTL_MASK
> +			| IOMUXC_GPR3_HDMI_MUX_CTL_MASK))
> +	    | (IOMUXC_GPR3_MUX_SRC_IPU1_DI0
> +	       << IOMUXC_GPR3_LVDS1_MUX_CTL_OFFSET);
> +	writel(reg, &iomux->gpr[3]);
> +
> +	/* Disable LCD backlight */
> +	imx_iomux_v3_setup_pad(MX6_PAD_DI0_PIN4__GPIO4_IO20);
> +	gpio_direction_input(IMX_GPIO_NR(4, 20));
>  }
>  #endif /* CONFIG_VIDEO_IPUV3 */
>  

This function is also quite identical to all boards.You add here only
the disable of the LCD backlight. I can recognize some parts, that are
surely common to all implementations (enabling clocks). Even if the
setup of gpr[2]/[3] is identical, I can imagine that it could be board
specific. Anyway, any chance to factorize the code ?


>  
> diff --git a/include/configs/wandboard.h b/include/configs/wandboard.h
> index e9c7e64..85f3c16 100644
> --- a/include/configs/wandboard.h
> +++ b/include/configs/wandboard.h
> @@ -55,6 +55,12 @@
>  #define CONFIG_LOADADDR			0x12000000
>  #define CONFIG_SYS_TEXT_BASE		0x17800000
>  
> +/* I2C Configs */
> +#define CONFIG_CMD_I2C
> +#define CONFIG_SYS_I2C
> +#define CONFIG_SYS_I2C_MXC
> +#define CONFIG_SYS_I2C_SPEED		100000
> +
>  /* MMC Configuration */
>  #define CONFIG_FSL_ESDHC
>  #define CONFIG_FSL_USDHC
> @@ -97,6 +103,7 @@
>  #define CONFIG_VIDEO_LOGO
>  #define CONFIG_VIDEO_BMP_LOGO
>  #define CONFIG_IPUV3_CLK 260000000
> +#define CONFIG_CMD_HDMIDETECT
>  #define CONFIG_IMX_HDMI
>  
>  #if defined(CONFIG_MX6DL) || defined(CONFIG_MX6S)
> @@ -134,7 +141,33 @@
>  			"fi; "	\
>  		"fi\0" \
>  	"mmcargs=setenv bootargs console=${console},${baudrate} " \
> -		"root=${mmcroot}\0" \
> +		"root=${mmcroot}; run videoargs\0" \
> +	"videoargs=" \
> +		"setenv nextcon 0; " \
> +		"if hdmidet; then " \
> +			"setenv bootargs ${bootargs} " \
> +				"video=mxcfb${nextcon}:dev=hdmi,1280x720M at 60," \
> +					"if=RGB24; " \
> +			"setenv fbmen fbmem=28M; " \
> +			"setexpr nextcon ${nextcon} + 1; " \
> +		"else " \
> +			"echo - no HDMI monitor;" \
> +		"fi; " \
> +		"i2c dev 0; " \
> +		"if i2c probe 0x10; then " \
> +			"setenv bootargs ${bootargs} " \
> +				"video=mxcfb${nextcon}:dev=lcd,800x480 at 60," \
> +					"if=RGB666; " \

Why do we need this ? The kernel should get the setup for display from
DTS (display-timings node). Do you need to short-circuit DTS setup ?

> +			"if test 0 -eq ${nextcon}; then " \
> +				"setenv fbmem fbmem=10M; " \
> +			"else " \
> +				"setenv fbmem ${fbmem},10M; " \
> +			"fi; " \
> +			"setexpr nextcon ${nextcon} + 1; " \
> +		"else " \
> +			"echo '- no FWBADAPT-7WVGA-LCD-F07A-0102 display';" \
> +		"fi; " \
> +		"setenv bootargs ${bootargs} ${fbmem}\0" \
>  	"loadbootscript=" \
>  		"fatload mmc ${mmcdev}:${mmcpart} ${loadaddr} ${script};\0" \
>  	"bootscript=echo Running bootscript from mmc ...; " \

I was against adding so many scripts in CONFIG_ENV_EXTRA, but as Simon's
patches to move the default environment outside u-boot are not yet in
mainline, I will not stop patches for this.

Best regards,
Stefano Babic


-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
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
=====================================================================