| Message ID | 1393338807-6146-4-git-send-email-p.wilczek@samsung.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Minkyu Kang |
| Headers | show |
Hi Piotr, Find my comments inline. On Tue, Feb 25, 2014 at 11:33 PM, Piotr Wilczek <p.wilczek@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@samsung.com> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Minkyu Kang <mk7.kang@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. > #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@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > Regards, Ajay Kumar
Piotr, Adding more comments. On Thu, Feb 27, 2014 at 10:50 PM, Ajay kumar <ajaynumb@gmail.com> wrote: > Hi Piotr, > Find my comments inline. > > > On Tue, Feb 25, 2014 at 11:33 PM, Piotr Wilczek <p.wilczek@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@samsung.com> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >> Cc: Minkyu Kang <mk7.kang@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@lists.denx.de >> http://lists.denx.de/mailman/listinfo/u-boot >> > > > Regards, > Ajay Kumar > Regards, Ajay Kumar
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@gmail.com> wrote: > >> Hi Piotr, >> Find my comments inline. >> >> >> On Tue, Feb 25, 2014 at 11:33 PM, Piotr Wilczek <p.wilczek@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@samsung.com> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>> Cc: Minkyu Kang <mk7.kang@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@lists.denx.de >>> http://lists.denx.de/mailman/listinfo/u-boot >>> >> >> >> Regards, >> Ajay Kumar >> > > Regards, > Ajay Kumar > > > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > http://lists.denx.de/mailman/listinfo/u-boot > Best regards, Piotr Wilczek
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"))); + 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); + 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); #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;