Hello Stefano,
On Thu, Dec 19, 2013 at 8:36 AM, Stefano Babic sbabic@denx.de wrote:
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]" : ""));} elseprintf("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.
...
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 ?
I agree with both remarks but I think we're late in the release cycle for this kind of refactoring; I prefer to merge this patch as is for now and send a patch, on top of this one, to rework this.
Can we go further this way? I am awaiting for this patch to be merged/reviewed for quite a while already.
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@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@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 ?
Current kernel still does not use DTS (it is based on 3.0.35 FSL fork).