Hello Stefano,
On Thu, Dec 19, 2013 at 8:36 AM, Stefano Babic <sba...@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]" : ""));
>> + } 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.
...
>
> 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).
--
Otavio Salvador O.S. Systems
http://www.ossystems.com.br http://code.ossystems.com.br
Mobile: +55 (53) 9981-7854 Mobile: +1 (347) 903-9750
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
