<div dir="ltr">Hi Sachin,<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Fri, Mar 21, 2014 at 2:12 PM, Sachin Kamat <span dir="ltr"><<a href="mailto:sachin.kamat@linaro.org" target="_blank">sachin.kamat@linaro.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">On 19 March 2014 19:52, Ajay Kumar <<a href="mailto:ajaykumar.rs@samsung.com">ajaykumar.rs@samsung.com</a>> wrote:<br>

> Add post processor ops for IELCD and their support functions.<br>
> Expose an interface for the FIMD to register IELCD PP.<br>
</div>[snip]<br>
<div class="">> +<br>
> +#define exynos_ielcd_readl(addr)       readl(ielcd->exynos_ielcd_base + addr)<br>
> +#define exynos_ielcd_writel(addr, val) \<br>
<br>
</div>nit: The order of arguments could be retained same as writel (i.e.,<br>
val, addr) for ease of<br>
reading.<br>
<div class=""><br></div></blockquote><div>Right. I will change it. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> +                               writel(val, ielcd->exynos_ielcd_base + addr)<br>
<br>
> +#define IELCD_TIMEOUT_CNT      1000<br>
> +<br>
> +struct ielcd_context {<br>
> +       void __iomem                    *exynos_ielcd_base;<br>
> +       unsigned                vdisplay;<br>
> +       unsigned                vsync_len;<br>
> +       unsigned                vbpd;<br>
> +       unsigned                vfpd;<br>
> +       unsigned                hdisplay;<br>
> +       unsigned                hsync_len;<br>
> +       unsigned                hbpd;<br>
> +       unsigned                hfpd;<br>
> +       unsigned                vidcon0;<br>
> +       unsigned                vidcon1;<br>
> +};<br>
> +<br>
> +static int ielcd_logic_start(struct ielcd_context *ielcd)<br>
> +{<br>
> +       exynos_ielcd_writel(IELCD_AUXCON, IELCD_MAGIC_KEY);<br>
> +<br>
> +       return 0;<br>
> +}<br>
<br>
</div>The return type could be made void.<br>
<div class=""><br></div></blockquote><div>Ok. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> +<br>
> +static int exynos_ielcd_hw_trigger_check(struct ielcd_context *ielcd)<br>
> +{<br>
> +       unsigned int cfg;<br>
> +       unsigned int count = 0;<br>
> +<br>
> +       cfg = exynos_ielcd_readl(IELCD_TRIGCON);<br>
> +       cfg &= ~(SWTRGCMD_W0BUF | TRGMODE_W0BUF);<br>
> +       cfg |= (SWTRGCMD_W0BUF | TRGMODE_W0BUF);<br>
> +       exynos_ielcd_writel(IELCD_TRIGCON, cfg);<br>
> +<br>
> +       do {<br>
> +               cfg = exynos_ielcd_readl(IELCD_SHADOWCON);<br>
> +               cfg |= IELCD_W0_SW_SHADOW_UPTRIG;<br>
> +               exynos_ielcd_writel(IELCD_SHADOWCON, cfg);<br>
> +<br>
> +               cfg = exynos_ielcd_readl(IELCD_VIDCON0);<br>
> +               cfg |= IELCD_SW_SHADOW_UPTRIG;<br>
> +               exynos_ielcd_writel(IELCD_VIDCON0, cfg);<br>
> +<br>
> +               count++;<br>
> +               if (count > IELCD_TIMEOUT_CNT) {<br>
<br>
</div>The 2 lines could be combined as:<br>
                    if (++count > IELCD_TIMEOUT_CNT) {<br>
<div class=""><br>
> +                       DRM_ERROR("ielcd start fail\n");<br>
> +                       return -EBUSY;<br>
> +               }<br>
> +               udelay(10);<br>
> +       } while (exynos_ielcd_readl(IELCD_WINCON0) & IELCD_TRGSTATUS);<br>
<br>
</div>Also this check could be moved inside the loop and break if 0 whereas this could<br>
while (1) here.<br>
<div class=""><br></div></blockquote><div>Ok. I will change the above loop. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="">
> +<br>
> +       return 0;<br>
> +}<br>
> +<br>
> +static int exynos_ielcd_display_on(struct ielcd_context *ielcd)<br>
> +{<br>
> +       unsigned int cfg;<br>
> +<br>
> +       cfg = exynos_ielcd_readl(IELCD_VIDCON0);<br>
> +       cfg |= (VIDCON0_ENVID | VIDCON0_ENVID_F);<br>
> +       exynos_ielcd_writel(IELCD_VIDCON0, cfg);<br>
> +<br>
> +       return exynos_ielcd_hw_trigger_check(ielcd);<br>
> +}<br>
> +<br>
> +int exynos_ielcd_display_off(void *pp_ctx)<br>
> +{<br>
> +       struct ielcd_context *ielcd = pp_ctx;<br>
> +       unsigned int cfg, ielcd_count = 0;<br>
> +       int ret = 0;<br>
<br>
</div>initialization not needed.</blockquote><div>Ok. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div class="h5">
> +<br>
> +       cfg = exynos_ielcd_readl(IELCD_VIDCON0);<br>
> +       cfg &= ~(VIDCON0_ENVID | VIDCON0_ENVID_F);<br>
> +<br>
> +       exynos_ielcd_writel(IELCD_VIDCON0, cfg);<br>
> +<br>
> +       do {<br>
> +               if (++ielcd_count > IELCD_TIMEOUT_CNT) {<br>
> +                       DRM_ERROR("ielcd off TIMEDOUT\n");<br>
> +                       ret = -ETIMEDOUT;<br>
> +                       break;<br>
> +               }<br>
> +<br>
> +               if (!(exynos_ielcd_readl(IELCD_VIDCON1) &<br>
> +                                               VIDCON1_LINECNT_MASK)) {<br>
> +                       ret = 0;<br>
> +                       break;<br>
> +               }<br>
> +               udelay(200);<br>
> +       } while (1);<br>
> +<br>
> +       return ret;<br>
> +}<br>
> +<br>
> +static void exynos_ielcd_config_rgb(struct ielcd_context *ielcd)<br>
> +{<br>
> +       unsigned int cfg;<br>
> +<br>
> +       cfg = exynos_ielcd_readl(IELCD_VIDCON0);<br>
> +       cfg &= ~(VIDCON0_VIDOUT_MASK | VIDCON0_VCLK_MASK);<br>
> +       cfg |= VIDCON0_VIDOUT_RGB;<br>
> +       cfg |= VIDCON0_VCLK_NORMAL;<br>
> +<br>
> +       exynos_ielcd_writel(IELCD_VIDCON0, cfg);<br>
> +}<br>
> +<br>
> +int exynos_ielcd_power_on(void *pp_ctx)<br>
> +{<br>
> +       struct ielcd_context *ielcd = pp_ctx;<br>
> +       unsigned int cfg;<br>
> +       int ret = 0;<br>
> +<br>
> +       ielcd_logic_start(ielcd);<br>
> +       ielcd_set_polarity(ielcd);<br>
> +<br>
> +       ielcd_set_lcd_size(ielcd);<br>
> +       ielcd_set_timing(ielcd);<br>
> +<br>
> +       /* window0 setting , fixed */<br>
> +       cfg = WINCONx_ENLOCAL | WINCON0_BPPMODE_24BPP_888 | WINCONx_ENWIN;<br>
> +       exynos_ielcd_writel(IELCD_WINCON0, cfg);<br>
> +<br>
> +       exynos_ielcd_config_rgb(ielcd);<br>
> +<br>
> +       ret = exynos_ielcd_display_on(ielcd);<br>
> +       if (ret) {<br>
> +               DRM_ERROR("IELCD failed to start\n");<br>
> +               return ret;<br>
> +       }<br>
> +<br>
> +       return 0;<br>
<br>
</div></div>The above block could be simplified as:<br>
               ret = exynos_ielcd_display_on(ielcd);<br>
               if (ret)<br>
                      DRM_ERROR(...);<br>
<br>
               return ret;<br></blockquote><div>Ok. I will change it. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
> +}<br>
> +<br>
<div><div class="h5">> +static void exynos_ielcd_mode_set(void *pp_ctx,<br>
> +                               const struct drm_display_mode *in_mode)<br>
> +{<br>
> +       struct ielcd_context *ielcd = pp_ctx;<br>
> +<br>
> +       ielcd->vdisplay = in_mode->crtc_vdisplay;<br>
> +       ielcd->vsync_len = in_mode->crtc_vsync_end - in_mode->crtc_vsync_start;<br>
> +       ielcd->vbpd = in_mode->crtc_vtotal - in_mode->crtc_vsync_end;<br>
> +       ielcd->vfpd = in_mode->crtc_vsync_start - in_mode->crtc_vdisplay;<br>
> +<br>
> +       ielcd->hdisplay = in_mode->crtc_hdisplay;<br>
> +       ielcd->hsync_len = in_mode->crtc_hsync_end - in_mode->crtc_hsync_start;<br>
> +       ielcd->hbpd = in_mode->crtc_htotal - in_mode->crtc_hsync_end;<br>
> +       ielcd->hfpd = in_mode->crtc_hsync_start - in_mode->crtc_hdisplay;<br>
> +}<br>
> +<br>
> +static struct exynos_fimd_pp_ops ielcd_ops = {<br>
> +       .power_on =     exynos_ielcd_power_on,<br>
> +       .power_off =    exynos_ielcd_display_off,<br>
> +       .mode_set =     exynos_ielcd_mode_set,<br>
> +};<br>
> +<br>
> +static struct exynos_fimd_pp ielcd_pp = {<br>
> +       .ops =          &ielcd_ops,<br>
> +};<br>
> +<br>
> +int exynos_ielcd_init(struct device *dev, struct exynos_fimd_pp **pp)<br>
> +{<br>
> +       struct device_node *ielcd_np;<br>
> +       struct ielcd_context *ielcd;<br>
> +       u32 addr[2];<br>
> +       int ret = 0;<br>
> +<br>
> +       ielcd = kzalloc(sizeof(struct ielcd_context), GFP_KERNEL);<br>
> +       if (!ielcd) {<br>
> +               DRM_ERROR("failed to allocate ielcd\n");<br>
> +               ret = -ENOMEM;<br>
> +               goto error0;<br>
<br>
</div></div>return directly from here and delete the label.<br>
<span class="HOEnZb"><font color="#888888"><br></font></span></blockquote><div>Ok. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="HOEnZb"><font color="#888888">
--<br>
With warm regards,<br>
Sachin<br>
</font></span></blockquote></div><br></div><div class="gmail_extra">Will address all your comments in next patchset.</div><div class="gmail_extra"><br></div><div class="gmail_extra">Thanks and regards,</div><div class="gmail_extra">
Ajay kumar</div></div>