From mboxrd@z Thu Jan 1 00:00:00 1970 From: Piotr Wilczek Date: Fri, 28 Feb 2014 09:54:05 +0100 Subject: [U-Boot] [PATCH V3 03/12] video:exynos_fb:fdt: add additional fdt data In-Reply-To: References: <1390832143-372-1-git-send-email-p.wilczek@samsung.com> <1393338807-6146-1-git-send-email-p.wilczek@samsung.com> <1393338807-6146-4-git-send-email-p.wilczek@samsung.com> Message-ID: <53104EAD.7040006@samsung.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 wrote: > >> Hi Piotr, >> Find my comments inline. >> >> >> On Tue, Feb 25, 2014 at 11:33 PM, Piotr Wilczek 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 >>> Signed-off-by: Kyungmin Park >>> Cc: Minkyu Kang >>> --- >>> 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