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

[Piotr Wilczek]

Hi Ajay,

Thank you for review. Please see answers below.

On 02/27/2014 03:10 PM, Ajay kumar wrote:
> 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".
The 'init_panel_info' is used to init lcd panel from he board file. It 
is called when CONFIG_OF_CONTROL is not defined. When CONFIG_OF_CONTROL 
is defined then we init panel from DTB data in exynos_fimd_parse_dt 
function.

However, it may be necessary to do some additional initializations that 
are optional and board specific. That’s what 'exynos_lcd_panel_init' 
function is for.

>> Please check board/samsung/smdk5250.c
smdk5250.c is compiled when CONFIG_OF_CONTROL is not defined.
With CONFIG_OF_CONTROL enabled, the exynos5-dt.c is used but it does not 
implement 'init_panel_info' so I would get undefined reference to 
'init_panel_info'. Tahts another reason that I introduced the above 
function.

>>
>>>   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.
Right, rgb_mode and power_on_delay I was supposed to remove in the 
previous version but overlooked that, thanks.

>> For passing resolution, please use "samsung,vl-col" and "samsung,vl-row"
Previously HD_RESOLUTION was assigned to panel_info.resolution. It is 
defined as 0 in libtizen.h.

>>
>>>          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.
exynos_lcd_panel_init function is supposed to do only additional (to 
exynos_fimd_parse_dt) initializations like ex: get_tizen_logo_info.

>
> 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
>
>
>
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
>

Best regards,
Piotr Wilczek