Hi Sachin,
On Fri, Mar 21, 2014 at 2:12 PM, Sachin Kamat <sachin.kamat@xxxxxxxxxx> wrote:
On 19 March 2014 19:52, Ajay Kumar <ajaykumar.rs@xxxxxxxxxxx> wrote:[snip]
> Add post processor ops for IELCD and their support functions.
> Expose an interface for the FIMD to register IELCD PP.
> +nit: The order of arguments could be retained same as writel (i.e.,
> +#define exynos_ielcd_readl(addr) readl(ielcd->exynos_ielcd_base + addr)
> +#define exynos_ielcd_writel(addr, val) \
val, addr) for ease of
reading.
Right. I will change it.
> + writel(val, ielcd->exynos_ielcd_base + addr)The return type could be made void.
> +#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;
> +}
Ok.
> +The 2 lines could be combined as:
> +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) {
if (++count > IELCD_TIMEOUT_CNT) {
Also this check could be moved inside the loop and break if 0 whereas this could
> + DRM_ERROR("ielcd start fail\n");
> + return -EBUSY;
> + }
> + udelay(10);
> + } while (exynos_ielcd_readl(IELCD_WINCON0) & IELCD_TRGSTATUS);
while (1) here.
Ok. I will change the above loop.
> +initialization not needed.
> + 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;
Ok.
The above block could be simplified as:> +
> + 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;
ret = exynos_ielcd_display_on(ielcd);
if (ret)
DRM_ERROR(...);
return ret;
Ok. I will change it.
> +}
> +
return directly from here and delete the label.> +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;
Ok.
--
With warm regards,
Sachin
Will address all your comments in next patchset.
Thanks and regards,
Ajay kumar
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
