On 19 March 2014 19:52, Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> 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. > + 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. > + > +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. > + > + 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. > + > + 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; > +} > + > +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. -- With warm regards, Sachin -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html