| Message ID | 1375208791-15781-11-git-send-email-detheridge@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 30/07/13 21:26, Darren Etheridge wrote: > From: Afzal Mohammed <afzal@ti.com> > > fb_set_par helps in runtime configuration of lcd controller like > changing resolution, pixel clock etc. (eg. using fbset utility) > > Reconfigure lcd controller based on information passed by framework. > Enable raster back if it was already enabled. > > As fb_set_par would get invoked indirectly from probe via fb_set_var, > remove existing lcdc initialization in probe and do lcdc reset in > probe so that reset happens only at the begining. > > Signed-off-by: Afzal Mohammed <afzal@ti.com> > Signed-off-by: Darren Etheridge <detheridge@ti.com> > --- > drivers/video/da8xx-fb.c | 60 +++++++++++++++++++++++++++++++++++++-------- > 1 files changed, 49 insertions(+), 11 deletions(-) > > diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c > index 8d73730..b25b810 100644 > --- a/drivers/video/da8xx-fb.c > +++ b/drivers/video/da8xx-fb.c > @@ -243,6 +243,11 @@ static struct fb_videomode known_lcd_panels[] = { > }, > }; > > +static inline bool da8xx_fb_is_raster_enabled(void) > +{ > + return !!(lcdc_read(LCD_RASTER_CTRL_REG) & LCD_RASTER_ENABLE); > +} See Documentation/CodingStyle about inline. I think, generally, it's better not to use inline at all in normal functions. Let the compiler decide. Even more so with funcs like da8xx_fb_is_raster_enabled(), which I guess is only used rarely. There are some inlines added in other patches in the series also. > /* Enable the Raster Engine of the LCD Controller */ > static inline void lcd_enable_raster(void) > { > @@ -665,9 +670,6 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green, > > static void da8xx_fb_lcd_reset(void) > { > - /* Disable the Raster if previously Enabled */ > - lcd_disable_raster(false); > - > /* DMA has to be disabled */ > lcdc_write(0, LCD_DMA_CTRL_REG); > lcdc_write(0, LCD_RASTER_CTRL_REG); > @@ -720,8 +722,6 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg, > u32 bpp; > int ret = 0; > > - da8xx_fb_lcd_reset(); > - > da8xx_fb_calc_config_clk_divider(par, panel); > > if (panel->sync & FB_SYNC_CLK_INVERT) > @@ -1201,9 +1201,52 @@ static int da8xx_pan_display(struct fb_var_screeninfo *var, > return ret; > } > > +static int da8xxfb_set_par(struct fb_info *info) > +{ > + struct da8xx_fb_par *par = info->par; > + int ret; > + bool raster = da8xx_fb_is_raster_enabled(); > + > + if (raster) > + lcd_disable_raster(true); > + else > + lcd_disable_raster(false); This looks odd. If raster is enabled, you disable it. And if raster is disabled, you disable it. Tomi
> > > > +static int da8xxfb_set_par(struct fb_info *info) { > > + struct da8xx_fb_par *par = info->par; > > + int ret; > > + bool raster = da8xx_fb_is_raster_enabled(); > > + > > + if (raster) > > + lcd_disable_raster(true); > > + else > > + lcd_disable_raster(false); > > This looks odd. If raster is enabled, you disable it. And if raster is disabled, > you disable it. I corrected this one in patch 0011 - I agree this code is very confusing. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 31/07/13 21:56, Etheridge, Darren wrote: >>> >>> +static int da8xxfb_set_par(struct fb_info *info) { >>> + struct da8xx_fb_par *par = info->par; >>> + int ret; >>> + bool raster = da8xx_fb_is_raster_enabled(); >>> + >>> + if (raster) >>> + lcd_disable_raster(true); >>> + else >>> + lcd_disable_raster(false); >> >> This looks odd. If raster is enabled, you disable it. And if raster is disabled, >> you disable it. > > I corrected this one in patch 0011 - I agree this code is very confusing. In patch 11 you add the enum. I wasn't referring to that. My point was that even if raster is already disabled, lcd_disable_raster(dont-wait-for-framedone) is called. Tomi
>>> +static int da8xxfb_set_par(struct fb_info *info) { >>> + struct da8xx_fb_par *par = info->par; >>> + int ret; >>> + bool raster = da8xx_fb_is_raster_enabled(); >>> + >>> + if (raster) >>> + lcd_disable_raster(true); >>> + else >>> + lcd_disable_raster(false); >> >> This looks odd. If raster is enabled, you disable it. And if raster is disabled, >> you disable it. > > I corrected this one in patch 0011 - I agree this code is very confusing. > >In patch 11 you add the enum. I wasn't referring to that. My point was >that even if raster is already disabled, >lcd_disable_raster(dont-wait-for-framedone) is called. Oh I see, yes you are absolutely right - I will check into what the intent of this code was and see if it is even necessary. -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Wed [2013-Jul-31 14:38:52 +0300]: > > > > +static inline bool da8xx_fb_is_raster_enabled(void) > > +{ > > + return !!(lcdc_read(LCD_RASTER_CTRL_REG) & LCD_RASTER_ENABLE); > > +} > > See Documentation/CodingStyle about inline. > > I think, generally, it's better not to use inline at all in normal > functions. Let the compiler decide. Even more so with funcs like > da8xx_fb_is_raster_enabled(), which I guess is only used rarely. > > There are some inlines added in other patches in the series also. > I have added a new patch to the update series that removes the use of inline from all offending places. Darren -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote on Thu [2013-Aug-01 08:44:00 +0300]: > On 31/07/13 21:56, Etheridge, Darren wrote: > >>> > >>> +static int da8xxfb_set_par(struct fb_info *info) { > >>> + struct da8xx_fb_par *par = info->par; > >>> + int ret; > >>> + bool raster = da8xx_fb_is_raster_enabled(); > >>> + > >>> + if (raster) > >>> + lcd_disable_raster(true); > >>> + else > >>> + lcd_disable_raster(false); > >> > >> This looks odd. If raster is enabled, you disable it. And if raster is disabled, > >> you disable it. > > > > I corrected this one in patch 0011 - I agree this code is very confusing. > > In patch 11 you add the enum. I wasn't referring to that. My point was > that even if raster is already disabled, > lcd_disable_raster(dont-wait-for-framedone) is called. Agreed and removed, the lcd_disable_raster function does the same check as da8xx_fb_is_raster_enabled() and the immediately exits if not enabled so this path of the conditional appears completely redundant. Darren -- To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c index 8d73730..b25b810 100644 --- a/drivers/video/da8xx-fb.c +++ b/drivers/video/da8xx-fb.c @@ -243,6 +243,11 @@ static struct fb_videomode known_lcd_panels[] = { }, }; +static inline bool da8xx_fb_is_raster_enabled(void) +{ + return !!(lcdc_read(LCD_RASTER_CTRL_REG) & LCD_RASTER_ENABLE); +} + /* Enable the Raster Engine of the LCD Controller */ static inline void lcd_enable_raster(void) { @@ -665,9 +670,6 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green, static void da8xx_fb_lcd_reset(void) { - /* Disable the Raster if previously Enabled */ - lcd_disable_raster(false); - /* DMA has to be disabled */ lcdc_write(0, LCD_DMA_CTRL_REG); lcdc_write(0, LCD_RASTER_CTRL_REG); @@ -720,8 +722,6 @@ static int lcd_init(struct da8xx_fb_par *par, const struct lcd_ctrl_config *cfg, u32 bpp; int ret = 0; - da8xx_fb_lcd_reset(); - da8xx_fb_calc_config_clk_divider(par, panel); if (panel->sync & FB_SYNC_CLK_INVERT) @@ -1201,9 +1201,52 @@ static int da8xx_pan_display(struct fb_var_screeninfo *var, return ret; } +static int da8xxfb_set_par(struct fb_info *info) +{ + struct da8xx_fb_par *par = info->par; + int ret; + bool raster = da8xx_fb_is_raster_enabled(); + + if (raster) + lcd_disable_raster(true); + else + lcd_disable_raster(false); + + fb_var_to_videomode(&par->mode, &info->var); + + par->cfg.bpp = info->var.bits_per_pixel; + + info->fix.visual = (par->cfg.bpp <= 8) ? + FB_VISUAL_PSEUDOCOLOR : FB_VISUAL_TRUECOLOR; + info->fix.line_length = (par->mode.xres * par->cfg.bpp) / 8; + + ret = lcd_init(par, &par->cfg, &par->mode); + if (ret < 0) { + dev_err(par->dev, "lcd init failed\n"); + return ret; + } + + par->dma_start = info->fix.smem_start + + info->var.yoffset * info->fix.line_length + + info->var.xoffset * info->var.bits_per_pixel / 8; + par->dma_end = par->dma_start + + info->var.yres * info->fix.line_length - 1; + + lcdc_write(par->dma_start, LCD_DMA_FRM_BUF_BASE_ADDR_0_REG); + lcdc_write(par->dma_end, LCD_DMA_FRM_BUF_CEILING_ADDR_0_REG); + lcdc_write(par->dma_start, LCD_DMA_FRM_BUF_BASE_ADDR_1_REG); + lcdc_write(par->dma_end, LCD_DMA_FRM_BUF_CEILING_ADDR_1_REG); + + if (raster) + lcd_enable_raster(); + + return 0; +} + static struct fb_ops da8xx_fb_ops = { .owner = THIS_MODULE, .fb_check_var = fb_check_var, + .fb_set_par = da8xxfb_set_par, .fb_setcolreg = fb_setcolreg, .fb_pan_display = da8xx_pan_display, .fb_ioctl = fb_ioctl, @@ -1312,14 +1355,9 @@ static int fb_probe(struct platform_device *device) } fb_videomode_to_var(&da8xx_fb_var, lcdc_info); - fb_var_to_videomode(&par->mode, &da8xx_fb_var); par->cfg = *lcd_cfg; - if (lcd_init(par, lcd_cfg, lcdc_info) < 0) { - dev_err(&device->dev, "lcd_init failed\n"); - ret = -EFAULT; - goto err_release_fb; - } + da8xx_fb_lcd_reset(); /* allocate frame buffer */ par->vram_size = lcdc_info->xres * lcdc_info->yres * lcd_cfg->bpp;