[U-Boot] [PATCH V3 03/12] video:exynos_fb:fdt: add additional fdt data

[Ajay kumar]

Piotr,

Adding more comments.

On Thu, Feb 27, 2014 at 10:50 PM, Ajay kumar <ajaynumb at gmail.com> wrote:

> Hi Piotr,
> Find my comments inline.
>
>
> On Tue, Feb 25, 2014 at 11:33 PM, Piotr Wilczek <p.wilczek at samsung.com>wrote:
>
>> This patch adds additional data parsing from DTB and adds the new
>> exynos_lcd_panel_init() function for panel specific initialisation
>> from the board file.
>>
>> Signed-off-by: Piotr Wilczek <p.wilczek at samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
>> Cc: Minkyu Kang <mk7.kang at samsung.com>
>> ---
>> Changes for v3:
>>  - none
>>
>> Changes for v2:
>>  - removed duplicate DTB node parsing for panel_info.logo_on
>>  - added (weak) exynos_lcd_panel_init function for panel specific
>>         initialisation from board file
>>
>>  drivers/video/exynos_fb.c | 21 +++++++++++++++++++++
>>  1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/video/exynos_fb.c b/drivers/video/exynos_fb.c
>> index 00a0a11..88d9037 100644
>> --- a/drivers/video/exynos_fb.c
>> +++ b/drivers/video/exynos_fb.c
>> @@ -104,6 +104,13 @@ void __exynos_backlight_reset(void)
>>  void exynos_backlight_reset(void)
>>         __attribute__((weak, alias("__exynos_backlight_reset")));
>>
>> +int __exynos_lcd_panel_init(vidinfo_t *vid)
>> +{
>> +       return 0;
>> +}
>> +int exynos_lcd_panel_init(vidinfo_t *vid)
>> +       __attribute__((weak, alias("__exynos_lcd_panel_init")));
>> +
>>
> This is redundant! We already have exynos_cfg_lcd_gpio, exynos_lcd_power_on
> and other similar functions to support "panel init".
> Please check board/samsung/smdk5250.c
>
>>  static void lcd_panel_on(vidinfo_t *vid)
>>  {
>>         udelay(vid->init_delay);
>> @@ -269,6 +276,15 @@ int exynos_fimd_parse_dt(const void *blob)
>>         panel_info.dual_lcd_enabled = fdtdec_get_int(blob, node,
>>
>> "samsung,dual-lcd-enabled", 0);
>>
>> +       panel_info.resolution = fdtdec_get_int(blob, node,
>> +                                               "samsung,resolution", 0);
>> +
>> +       panel_info.rgb_mode = fdtdec_get_int(blob, node,
>> +                                               "samsung,rgb-mode", 0);
>> +
>> +       panel_info.power_on_delay = fdtdec_get_int(blob, node,
>> +                                               "samsung,power-on-delay",
>> 0);
>> +
>>
> All the above DT properties are already present in the same file!
> This are definitely duplicate entries.
> For passing resolution, please use "samsung,vl-col" and "samsung,vl-row"
>
>>         return 0;
>>  }
>>  #endif
>> @@ -281,10 +297,15 @@ void lcd_ctrl_init(void *lcdbase)
>>  #ifdef CONFIG_OF_CONTROL
>>         if (exynos_fimd_parse_dt(gd->fdt_blob))
>>                 debug("Can't get proper panel info\n");
>> +#ifdef CONFIG_EXYNOS_MIPI_DSIM
>> +       exynos_init_dsim_platform_data(&panel_info);
>> +#endif
>> +       exynos_lcd_panel_init(&panel_info);
>>
> This is already present as part of lcd_enable in same file!
> Please check it.
>
Ok. I just heard from MIPI-DSI engineer that MIPI-DSI should
usually be initialized before FIMD video output starts.

Is that the reason why you are trying to do panel_init here?
That seems ok, but definitely you should not be using a new
function for that.

Use something like below snippet:
====================
/* exynos_fb.c */
.
lcd_ctrl_init()
{.
.
.
if(CONFIG_EXYNOS_MIPI...)
call lcd_panel_on  /* MIPI-DSI to be initialized before FIMD init */
.
do exynos_lcd_init() /* FIMD init */
.
}
.
.
.
.
lcd_enable()
{
.
.
if(CONFIG_EXYNOS_DP...)
call lcd_panel_on /* DP to be initialized after FIMD init */
.
.
}

>   #else
>>         /* initialize parameters which is specific to panel. */
>>         init_panel_info(&panel_info);
>>  #endif
>> +
>>         panel_width = panel_info.vl_width;
>>         panel_height = panel_info.vl_height;
>>
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
>
> Regards,
> Ajay Kumar
>

Regards,
Ajay Kumar