From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ajay kumar Subject: Re: [RFC 3/4] drm: exynos: add IELCD post processor Date: Fri, 21 Mar 2014 21:14:30 +0530 Message-ID: References: <1395238975-24600-1-git-send-email-ajaykumar.rs@samsung.com> <1395238975-24600-4-git-send-email-ajaykumar.rs@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0304139311==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Sachin Kamat Cc: linux-samsung-soc , Shirish S , Sean Paul , Seung-Woo Kim , sunil joshi , "dri-devel@lists.freedesktop.org" , marcheu@chromium.org, Prashanth G , Ajay Kumar , Rahul Sharma List-Id: linux-samsung-soc@vger.kernel.org --===============0304139311== Content-Type: multipart/alternative; boundary=047d7b86e832fb1c7c04f51fc2d6 --047d7b86e832fb1c7c04f51fc2d6 Content-Type: text/plain; charset=ISO-8859-1 Hi Sachin, On Fri, Mar 21, 2014 at 2:12 PM, Sachin Kamat wrote: > On 19 March 2014 19:52, Ajay Kumar 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 --047d7b86e832fb1c7c04f51fc2d6 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable
Hi Sachin,


On Fri, Mar 21, 2014 at 2:12 PM, Sachin Kamat <s= achin.kamat@linaro.org> wrote:
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) =A0 =A0 =A0 readl(ielcd->exynos_i= elcd_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.=A0
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 writel(v= al, ielcd->exynos_ielcd_base + addr)

> +#define IELCD_TIMEOUT_CNT =A0 =A0 =A01000
> +
> +struct ielcd_context {
> + =A0 =A0 =A0 void __iomem =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*exy= nos_ielcd_base;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vdisplay;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vsync_len;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vbpd;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vfpd;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hdisplay;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hsync_len;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hbpd;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0hfpd;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vidcon0;
> + =A0 =A0 =A0 unsigned =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0vidcon1;
> +};
> +
> +static int ielcd_logic_start(struct ielcd_context *ielcd)
> +{
> + =A0 =A0 =A0 exynos_ielcd_writel(IELCD_AUXCON, IELCD_MAGIC_KEY);
> +
> + =A0 =A0 =A0 return 0;
> +}

The return type could be made void.

Ok.=A0
> +
> +static int exynos_ielcd_hw_trigger_check(struct ielcd_context *ielcd)=
> +{
> + =A0 =A0 =A0 unsigned int cfg;
> + =A0 =A0 =A0 unsigned int count =3D 0;
> +
> + =A0 =A0 =A0 cfg =3D exynos_ielcd_readl(IELCD_TRIGCON);
> + =A0 =A0 =A0 cfg &=3D ~(SWTRGCMD_W0BUF | TRGMODE_W0BUF);
> + =A0 =A0 =A0 cfg |=3D (SWTRGCMD_W0BUF | TRGMODE_W0BUF);
> + =A0 =A0 =A0 exynos_ielcd_writel(IELCD_TRIGCON, cfg);
> +
> + =A0 =A0 =A0 do {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 cfg =3D exynos_ielcd_readl(IELCD_SHADOWC= ON);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 cfg |=3D IELCD_W0_SW_SHADOW_UPTRIG;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 exynos_ielcd_writel(IELCD_SHADOWCON, cfg= );
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 cfg =3D exynos_ielcd_readl(IELCD_VIDCON0= );
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 cfg |=3D IELCD_SW_SHADOW_UPTRIG;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 exynos_ielcd_writel(IELCD_VIDCON0, cfg);=
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 count++;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (count > IELCD_TIMEOUT_CNT) {

The 2 lines could be combined as:
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (++count > IELCD_TIMEOUT_CNT)= {

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 DRM_ERROR("ielcd st= art fail\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -EBUSY;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(10);
> + =A0 =A0 =A0 } while (exynos_ielcd_readl(IELCD_WINCON0) & IELCD_T= RGSTATUS);

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 loo= p.=A0
> +
> + =A0 =A0 =A0 return 0;
> +}
> +
> +static int exynos_ielcd_display_on(struct ielcd_context *ielcd)
> +{
> + =A0 =A0 =A0 unsigned int cfg;
> +
> + =A0 =A0 =A0 cfg =3D exynos_ielcd_readl(IELCD_VIDCON0);
> + =A0 =A0 =A0 cfg |=3D (VIDCON0_ENVID | VIDCON0_ENVID_F);
> + =A0 =A0 =A0 exynos_ielcd_writel(IELCD_VIDCON0, cfg);
> +
> + =A0 =A0 =A0 return exynos_ielcd_hw_trigger_check(ielcd);
> +}
> +
> +int exynos_ielcd_display_off(void *pp_ctx)
> +{
> + =A0 =A0 =A0 struct ielcd_context *ielcd =3D pp_ctx;
> + =A0 =A0 =A0 unsigned int cfg, ielcd_count =3D 0;
> + =A0 =A0 =A0 int ret =3D 0;

initialization not needed.
Ok.=A0
> +
> + =A0 =A0 =A0 cfg =3D exynos_ielcd_readl(IELCD_VIDCON0);
> + =A0 =A0 =A0 cfg &=3D ~(VIDCON0_ENVID | VIDCON0_ENVID_F);
> +
> + =A0 =A0 =A0 exynos_ielcd_writel(IELCD_VIDCON0, cfg);
> +
> + =A0 =A0 =A0 do {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (++ielcd_count > IELCD_TIMEOUT_CNT= ) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 DRM_ERROR("ielcd of= f TIMEDOUT\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ETIMEDOUT;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!(exynos_ielcd_readl(IELCD_VIDCON1) = &
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 VIDCON1_LINECNT_MASK)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D 0;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 break;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 udelay(200);
> + =A0 =A0 =A0 } while (1);
> +
> + =A0 =A0 =A0 return ret;
> +}
> +
> +static void exynos_ielcd_config_rgb(struct ielcd_context *ielcd)
> +{
> + =A0 =A0 =A0 unsigned int cfg;
> +
> + =A0 =A0 =A0 cfg =3D exynos_ielcd_readl(IELCD_VIDCON0);
> + =A0 =A0 =A0 cfg &=3D ~(VIDCON0_VIDOUT_MASK | VIDCON0_VCLK_MASK);=
> + =A0 =A0 =A0 cfg |=3D VIDCON0_VIDOUT_RGB;
> + =A0 =A0 =A0 cfg |=3D VIDCON0_VCLK_NORMAL;
> +
> + =A0 =A0 =A0 exynos_ielcd_writel(IELCD_VIDCON0, cfg);
> +}
> +
> +int exynos_ielcd_power_on(void *pp_ctx)
> +{
> + =A0 =A0 =A0 struct ielcd_context *ielcd =3D pp_ctx;
> + =A0 =A0 =A0 unsigned int cfg;
> + =A0 =A0 =A0 int ret =3D 0;
> +
> + =A0 =A0 =A0 ielcd_logic_start(ielcd);
> + =A0 =A0 =A0 ielcd_set_polarity(ielcd);
> +
> + =A0 =A0 =A0 ielcd_set_lcd_size(ielcd);
> + =A0 =A0 =A0 ielcd_set_timing(ielcd);
> +
> + =A0 =A0 =A0 /* window0 setting , fixed */
> + =A0 =A0 =A0 cfg =3D WINCONx_ENLOCAL | WINCON0_BPPMODE_24BPP_888 | WI= NCONx_ENWIN;
> + =A0 =A0 =A0 exynos_ielcd_writel(IELCD_WINCON0, cfg);
> +
> + =A0 =A0 =A0 exynos_ielcd_config_rgb(ielcd);
> +
> + =A0 =A0 =A0 ret =3D exynos_ielcd_display_on(ielcd);
> + =A0 =A0 =A0 if (ret) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 DRM_ERROR("IELCD failed to start\n&= quot;);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return ret;
> + =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 return 0;

The above block could be simplified as:
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0ret =3D exynos_ielcd_display_on(ielcd);
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (ret)
=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 DRM_ERROR(...);

=A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return ret;
Ok. I will = change it.=A0
> +}
> +
> +static void exynos_ielcd_mode_set(void *pp_ctx= ,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 const st= ruct drm_display_mode *in_mode)
> +{
> + =A0 =A0 =A0 struct ielcd_context *ielcd =3D pp_ctx;
> +
> + =A0 =A0 =A0 ielcd->vdisplay =3D in_mode->crtc_vdisplay;
> + =A0 =A0 =A0 ielcd->vsync_len =3D in_mode->crtc_vsync_end - in_= mode->crtc_vsync_start;
> + =A0 =A0 =A0 ielcd->vbpd =3D in_mode->crtc_vtotal - in_mode->= ;crtc_vsync_end;
> + =A0 =A0 =A0 ielcd->vfpd =3D in_mode->crtc_vsync_start - in_mod= e->crtc_vdisplay;
> +
> + =A0 =A0 =A0 ielcd->hdisplay =3D in_mode->crtc_hdisplay;
> + =A0 =A0 =A0 ielcd->hsync_len =3D in_mode->crtc_hsync_end - in_= mode->crtc_hsync_start;
> + =A0 =A0 =A0 ielcd->hbpd =3D in_mode->crtc_htotal - in_mode->= ;crtc_hsync_end;
> + =A0 =A0 =A0 ielcd->hfpd =3D in_mode->crtc_hsync_start - in_mod= e->crtc_hdisplay;
> +}
> +
> +static struct exynos_fimd_pp_ops ielcd_ops =3D {
> + =A0 =A0 =A0 .power_on =3D =A0 =A0 exynos_ielcd_power_on,
> + =A0 =A0 =A0 .power_off =3D =A0 =A0exynos_ielcd_display_off,
> + =A0 =A0 =A0 .mode_set =3D =A0 =A0 exynos_ielcd_mode_set,
> +};
> +
> +static struct exynos_fimd_pp ielcd_pp =3D {
> + =A0 =A0 =A0 .ops =3D =A0 =A0 =A0 =A0 =A0&ielcd_ops,
> +};
> +
> +int exynos_ielcd_init(struct device *dev, struct exynos_fimd_pp **pp)=
> +{
> + =A0 =A0 =A0 struct device_node *ielcd_np;
> + =A0 =A0 =A0 struct ielcd_context *ielcd;
> + =A0 =A0 =A0 u32 addr[2];
> + =A0 =A0 =A0 int ret =3D 0;
> +
> + =A0 =A0 =A0 ielcd =3D kzalloc(sizeof(struct ielcd_context), GFP_KERN= EL);
> + =A0 =A0 =A0 if (!ielcd) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 DRM_ERROR("failed to allocate ielcd= \n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D -ENOMEM;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto error0;

return directly from here and delete the label.

Ok.=A0
<= font color=3D"#888888"> --
With warm regards,
Sachin

Will = address all your comments in next patchset.

Thanks and regards,
Ajay kumar
--047d7b86e832fb1c7c04f51fc2d6-- --===============0304139311== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============0304139311==--