[PATCH 05/10 v2] s3c-fb: Add v4l2 subdevice to support framebuffer local fifo input path

[Sylwester Nawrocki]

Hello,

> -----Original Message-----
> From: linux-samsung-soc-owner at vger.kernel.org [mailto:linux-samsung-
> soc-owner at vger.kernel.org] On Behalf Of Maurus Cuelenaere
> Sent: Thursday, July 15, 2010 12:32 PM
> To: Sylwester Nawrocki
> Cc: linux-samsung-soc at vger.kernel.org; linux-arm-
> kernel at lists.infradead.org; p.osciak at samsung.com;
> m.szyprowski at samsung.com; kyungmin.park at samsung.com;
> kgene.kim at samsung.com; ben-linux at fluff.org; linux-fbdev at vger.kernel.org
> Subject: Re: [PATCH 05/10 v2] s3c-fb: Add v4l2 subdevice to support
> framebuffer local fifo input path
> 
>  Op 15-07-10 11:10, Sylwester Nawrocki schreef:
> > Selected multimedia devices in Samsung S3C/S5P SoC series are capable
> of
> > transferring data directly between each other, bypassing the main
> system
> > bus. Such a datapath exists between the camera interface/video
> > postprocessor and the lcd controller. To control the data flow from
> the
> > fimc driver level v4l2-subdevice driver is added to the framebuffer.
> > It enables to configure the lcd controller into FIFO or DMA input
> mode.
> >
> > Signed-off-by: Sylwester Nawrocki <s.nawrocki at samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park at samsung.com>
> > Signed-off-by: Marek Szyprowski <m.szyprowski at samsung.com>
> > ---
> >  arch/arm/plat-samsung/include/plat/fb.h |    6 +
> >  drivers/video/s3c-fb.c                  |  487
> +++++++++++++++++++++++++++++--
> >  2 files changed, 472 insertions(+), 21 deletions(-)
> 
> <snip>
> 
> >  /**
> > + * s3c_fb_enable_local() - switch window between input DMA and fifo
> modes
> > + *
> > + * @fb_sd: window subdevice for fifo input
> > + * @en:    1 - switch from input DMA to fifo mode and apply
> > + *	       window size and position set by the window's subdevice
> > + *         0 - restore from fifo to DMA mode
> > + */
> > +static int s3c_fb_enable_local_in(struct s3c_fb_win_sd *fb_sd, int
> en)
> > +{
> > +	struct s3c_fb_win *win = fb_sd->win;
> > +	struct s3c_fb *sfb = win->parent;
> > +	static u32 wincon;
> > +	u32 reg, data;
> > +	int ret = 0, bpp = 32;
> > +
> > +	/* disable video output and the window logic */
> > +	reg = readl(sfb->regs + WINCON(win->index));
> > +	writel(reg & ~WINCONx_ENWIN, sfb->regs + WINCON(win->index));
> > +
> > +	shadow_protect_win(win, 1);
> > +
> > +	if (en == 1) {
> > +		if (fb_sd->streaming)
> > +			return 0;
> 
> Shouldn't you do shadow_protect_win(win, 0) before returning here?

Yes, indeed the error path is wrong and also WINCONx_ENWIN
needs to be restored.
Thank you for pointing this out. 

> 
> > +
> > +		wincon = reg;
> > +
> > +		switch (fb_sd->fmt.fmt.pix.pixelformat) {
> > +		case V4L2_PIX_FMT_YUYV: /* YCbCr 4:4:4 */
> > +			reg |= WINCONx_YCbCr | WINCONx_ENLOCAL;
> > +			bpp = 16;
> > +			break;
> > +
> > +		case V4L2_PIX_FMT_RGB24:
> > +		default:
> > +			reg &= ~(WINCONx_YCbCr | WINCONx_WSWP |
> WINCONx_HAWSWP |
> > +				 WINCONx_BYTSWP | WINCONx_BITSWP |
> > +				 WINCON0_BPPMODE_MASK | WINCONx_BURSTLEN_MASK);
> > +
> > +			reg |=	WINCON0_BPPMODE_24BPP_888 |
> > +				WINCONx_BURSTLEN_4WORD;
> > +			bpp = 24;
> > +			break;
> > +		}
> > +
> > +		fb_sd->streaming = 1;
> > +		writel(reg, sfb->regs + WINCON(win->index));
> > +
> > +		s3c_fb_set_osd(fb_sd->win, &fb_sd->curr_osd_win, bpp);
> > +
> > +		writel(reg | WINCONx_ENLOCAL, sfb->regs + WINCON(win-
> >index));
> > +
> > +		shadow_protect_win(win, 0);
> > +
> > +		reg = readl(sfb->regs + WINCON(win->index));
> > +		writel(reg | WINCONx_ENWIN, sfb->regs + WINCON(win-
> >index));
> > +
> > +		if (sfb->variant.has_shadowcon)	{
> > +			data = readl(sfb->regs + SHADOWCON);
> > +			data |=	SHADOWCON_CHx_LOCAL_ENABLE(win->index);
> > +			writel(data, sfb->regs + SHADOWCON);
> > +		}
> > +
> > +	} else if (en == 0) {
> > +		if (!fb_sd->streaming)
> > +			return 0;
> 
> Same comment
> 
> > +
> > +		fb_sd->streaming = 0;
> > +
> > +		/* need to be aligned with VSYNC interrupt */
> > +		writel(wincon & ~WINCONx_ENLOCAL,
> > +		       sfb->regs + WINCON(win->index));
> > +
> > +		/* restore OSD values from before we enabled local mode */
> > +		bpp = win->fbinfo->var.bits_per_pixel;
> > +		s3c_fb_set_osd(fb_sd->win, &fb_sd->default_osd_win, bpp);
> > +
> > +		shadow_protect_win(win, 0);
> > +
> > +		if (sfb->variant.has_shadowcon)	{
> > +			data = readl(sfb->regs + SHADOWCON);
> > +			data &=	~SHADOWCON_CHx_LOCAL_ENABLE(win->index);
> > +			writel(data, sfb->regs + SHADOWCON);
> > +		}
> > +
> > +		reg = readl(sfb->regs + WINCON(win->index));
> > +		writel(reg | WINCONx_ENWIN, sfb->regs + WINCON(win-
> >index));
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> 
> --
> Maurus Cuelenaere

Regards,
--
Sylwester Nawrocki

> --
> To unsubscribe from this list: send the line "unsubscribe linux-
> samsung-soc" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html