Hi Sachin,
On Fri, Mar 21, 2014 at 2:12 PM, Sachin Kamat sachin.kamat@linaro.orgwrote:
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.
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