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

[Otavio Salvador]

Hello Stefano,

On Thu, Dec 19, 2013 at 8:36 AM, Stefano Babic <sbabic at 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 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 ?

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