Re: [RFC 3/4] drm: exynos: add IELCD post processor

On 19 March 2014 19:52, Ajay Kumar ajaykumar.rs@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.

•                           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.