Hi Sachin,
On Fri, Mar 21, 2014 at 2:12 PM, Sachin Kamat <sachin.kamat at linaro.org>wrote: > On 19 March 2014 19:52, Ajay Kumar <ajaykumar.rs at samsung.com> wrote: > > Add post processor ops for IELCD and their support functions. > > Expose an interface for the FIMD to register IELCD PP. > [snip] > > + > > +#define exynos_ielcd_readl(addr) readl(ielcd->exynos_ielcd_base + > addr) > > +#define exynos_ielcd_writel(addr, val) \ > > nit: The order of arguments could be retained same as writel (i.e., > val, addr) for ease of > reading. > > Right. I will change it. > > + writel(val, ielcd->exynos_ielcd_base + > addr) > > > +#define IELCD_TIMEOUT_CNT 1000 > > + > > +struct ielcd_context { > > + void __iomem *exynos_ielcd_base; > > + unsigned vdisplay; > > + unsigned vsync_len; > > + unsigned vbpd; > > + unsigned vfpd; > > + unsigned hdisplay; > > + unsigned hsync_len; > > + unsigned hbpd; > > + unsigned hfpd; > > + unsigned vidcon0; > > + unsigned vidcon1; > > +}; > > + > > +static int ielcd_logic_start(struct ielcd_context *ielcd) > > +{ > > + exynos_ielcd_writel(IELCD_AUXCON, IELCD_MAGIC_KEY); > > + > > + return 0; > > +} > > The return type could be made void. > > Ok. > > + > > +static int exynos_ielcd_hw_trigger_check(struct ielcd_context *ielcd) > > +{ > > + unsigned int cfg; > > + unsigned int count = 0; > > + > > + cfg = exynos_ielcd_readl(IELCD_TRIGCON); > > + cfg &= ~(SWTRGCMD_W0BUF | TRGMODE_W0BUF); > > + cfg |= (SWTRGCMD_W0BUF | TRGMODE_W0BUF); > > + exynos_ielcd_writel(IELCD_TRIGCON, cfg); > > + > > + do { > > + cfg = exynos_ielcd_readl(IELCD_SHADOWCON); > > + cfg |= IELCD_W0_SW_SHADOW_UPTRIG; > > + exynos_ielcd_writel(IELCD_SHADOWCON, cfg); > > + > > + cfg = exynos_ielcd_readl(IELCD_VIDCON0); > > + cfg |= IELCD_SW_SHADOW_UPTRIG; > > + exynos_ielcd_writel(IELCD_VIDCON0, cfg); > > + > > + count++; > > + if (count > IELCD_TIMEOUT_CNT) { > > The 2 lines could be combined as: > if (++count > IELCD_TIMEOUT_CNT) { > > > + DRM_ERROR("ielcd start fail\n"); > > + return -EBUSY; > > + } > > + udelay(10); > > + } while (exynos_ielcd_readl(IELCD_WINCON0) & IELCD_TRGSTATUS); > > Also this check could be moved inside the loop and break if 0 whereas this > could > while (1) here. > > Ok. I will change the above loop. > > + > > + return 0; > > +} > > + > > +static int exynos_ielcd_display_on(struct ielcd_context *ielcd) > > +{ > > + unsigned int cfg; > > + > > + cfg = exynos_ielcd_readl(IELCD_VIDCON0); > > + cfg |= (VIDCON0_ENVID | VIDCON0_ENVID_F); > > + exynos_ielcd_writel(IELCD_VIDCON0, cfg); > > + > > + return exynos_ielcd_hw_trigger_check(ielcd); > > +} > > + > > +int exynos_ielcd_display_off(void *pp_ctx) > > +{ > > + struct ielcd_context *ielcd = pp_ctx; > > + unsigned int cfg, ielcd_count = 0; > > + int ret = 0; > > initialization not needed. Ok. > > + > > + cfg = exynos_ielcd_readl(IELCD_VIDCON0); > > + cfg &= ~(VIDCON0_ENVID | VIDCON0_ENVID_F); > > + > > + exynos_ielcd_writel(IELCD_VIDCON0, cfg); > > + > > + do { > > + if (++ielcd_count > IELCD_TIMEOUT_CNT) { > > + DRM_ERROR("ielcd off TIMEDOUT\n"); > > + ret = -ETIMEDOUT; > > + break; > > + } > > + > > + if (!(exynos_ielcd_readl(IELCD_VIDCON1) & > > + VIDCON1_LINECNT_MASK)) { > > + ret = 0; > > + break; > > + } > > + udelay(200); > > + } while (1); > > + > > + return ret; > > +} > > + > > +static void exynos_ielcd_config_rgb(struct ielcd_context *ielcd) > > +{ > > + unsigned int cfg; > > + > > + cfg = exynos_ielcd_readl(IELCD_VIDCON0); > > + cfg &= ~(VIDCON0_VIDOUT_MASK | VIDCON0_VCLK_MASK); > > + cfg |= VIDCON0_VIDOUT_RGB; > > + cfg |= VIDCON0_VCLK_NORMAL; > > + > > + exynos_ielcd_writel(IELCD_VIDCON0, cfg); > > +} > > + > > +int exynos_ielcd_power_on(void *pp_ctx) > > +{ > > + struct ielcd_context *ielcd = pp_ctx; > > + unsigned int cfg; > > + int ret = 0; > > + > > + ielcd_logic_start(ielcd); > > + ielcd_set_polarity(ielcd); > > + > > + ielcd_set_lcd_size(ielcd); > > + ielcd_set_timing(ielcd); > > + > > + /* window0 setting , fixed */ > > + cfg = WINCONx_ENLOCAL | WINCON0_BPPMODE_24BPP_888 | > WINCONx_ENWIN; > > + exynos_ielcd_writel(IELCD_WINCON0, cfg); > > + > > + exynos_ielcd_config_rgb(ielcd); > > + > > + ret = exynos_ielcd_display_on(ielcd); > > + if (ret) { > > + DRM_ERROR("IELCD failed to start\n"); > > + return ret; > > + } > > + > > + return 0; > > The above block could be simplified as: > ret = exynos_ielcd_display_on(ielcd); > if (ret) > DRM_ERROR(...); > > return ret; > Ok. I will change it. > > +} > > + > > +static void exynos_ielcd_mode_set(void *pp_ctx, > > + const struct drm_display_mode *in_mode) > > +{ > > + struct ielcd_context *ielcd = pp_ctx; > > + > > + ielcd->vdisplay = in_mode->crtc_vdisplay; > > + ielcd->vsync_len = in_mode->crtc_vsync_end - > in_mode->crtc_vsync_start; > > + ielcd->vbpd = in_mode->crtc_vtotal - in_mode->crtc_vsync_end; > > + ielcd->vfpd = in_mode->crtc_vsync_start - in_mode->crtc_vdisplay; > > + > > + ielcd->hdisplay = in_mode->crtc_hdisplay; > > + ielcd->hsync_len = in_mode->crtc_hsync_end - > in_mode->crtc_hsync_start; > > + ielcd->hbpd = in_mode->crtc_htotal - in_mode->crtc_hsync_end; > > + ielcd->hfpd = in_mode->crtc_hsync_start - in_mode->crtc_hdisplay; > > +} > > + > > +static struct exynos_fimd_pp_ops ielcd_ops = { > > + .power_on = exynos_ielcd_power_on, > > + .power_off = exynos_ielcd_display_off, > > + .mode_set = exynos_ielcd_mode_set, > > +}; > > + > > +static struct exynos_fimd_pp ielcd_pp = { > > + .ops = &ielcd_ops, > > +}; > > + > > +int exynos_ielcd_init(struct device *dev, struct exynos_fimd_pp **pp) > > +{ > > + struct device_node *ielcd_np; > > + struct ielcd_context *ielcd; > > + u32 addr[2]; > > + int ret = 0; > > + > > + ielcd = kzalloc(sizeof(struct ielcd_context), GFP_KERNEL); > > + if (!ielcd) { > > + DRM_ERROR("failed to allocate ielcd\n"); > > + ret = -ENOMEM; > > + goto error0; > > return directly from here and delete the label. > > Ok. > -- > With warm regards, > Sachin > Will address all your comments in next patchset. Thanks and regards, Ajay kumar -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140321/3f61d4ce/attachment.html>