[PATCH 1/5] media: atmel-isi: correct yuv swap according to different sensor outputs

[g.liakhovetski@gmx.de (Guennadi Liakhovetski)]

Even simpler:

On Sun, 4 Oct 2015, Guennadi Liakhovetski wrote:

> Hi Josh,
> 
> On Tue, 22 Sep 2015, Josh Wu wrote:
> 
> > we need to configure the YCC_SWAP bits in ISI_CFG2 according to current
> > sensor output and Atmel ISI output format.
> > 
> > Current there are two cases Atmel ISI supported:
> >   1. Atmel ISI outputs YUYV format.
> >      This case we need to setup YCC_SWAP according to sensor output
> >      format.
> >   2. Atmel ISI output a pass-through formats, which means no swap.
> >      Just setup YCC_SWAP as default with no swap.
> > 
> > Signed-off-by: Josh Wu <josh.wu@atmel.com>
> > ---
> > 
> >  drivers/media/platform/soc_camera/atmel-isi.c | 43 ++++++++++++++++++++-------
> >  1 file changed, 33 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> > index 45e304a..df64294 100644
> > --- a/drivers/media/platform/soc_camera/atmel-isi.c
> > +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> > @@ -103,13 +103,41 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
> >  	return readl(isi->regs + reg);
> >  }
> >  
> > +static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
> > +		const struct soc_camera_format_xlate *xlate)
> > +{
> 
> This function will be called only for the four media codes from the 
> switch-case statement below, namely for
> 
> MEDIA_BUS_FMT_VYUY8_2X8
> MEDIA_BUS_FMT_UYVY8_2X8
> MEDIA_BUS_FMT_YVYU8_2X8
> MEDIA_BUS_FMT_YUYV8_2X8
> 
> > +	/* By default, no swap for the codec path of Atmel ISI. So codec
> > +	* output is same as sensor's output.
> > +	* For instance, if sensor's output is YUYV, then codec outputs YUYV.
> > +	* And if sensor's output is UYVY, then codec outputs UYVY.
> > +	*/
> > +	u32 cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_DEFAULT;
> 
> Then this ISI_CFG2_YCC_SWAP_DEFAULT will only hold for 
> MEDIA_BUS_FMT_YUYV8_2X8? Why don't you just add one more case below? Don't 
> think this initialisation is really justified.
> 
> > +
> > +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUYV) {
> > +		/* all convert to YUYV */
> > +		switch (xlate->code) {
> > +		case MEDIA_BUS_FMT_VYUY8_2X8:
> > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_3;
> > +			break;

Why don't you just return the result value here directly? Then you don't 
need the variable at all

> > +		case MEDIA_BUS_FMT_UYVY8_2X8:
> > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_2;
> > +			break;
> > +		case MEDIA_BUS_FMT_YVYU8_2X8:
> > +			cfg2_yuv_swap = ISI_CFG2_YCC_SWAP_MODE_1;
> > +			break;
> > +		}
> > +	}
> > +
> > +	return cfg2_yuv_swap;
> > +}
> > +
> >  static void configure_geometry(struct atmel_isi *isi, u32 width,
> > -			u32 height, u32 code)
> > +		u32 height, const struct soc_camera_format_xlate *xlate)
> >  {
> >  	u32 cfg2;
> >  
> >  	/* According to sensor's output format to set cfg2 */
> > -	switch (code) {
> > +	switch (xlate->code) {
> >  	default:
> >  	/* Grey */
> >  	case MEDIA_BUS_FMT_Y8_1X8:
> > @@ -117,16 +145,11 @@ static void configure_geometry(struct atmel_isi *isi, u32 width,
> >  		break;
> >  	/* YUV */
> >  	case MEDIA_BUS_FMT_VYUY8_2X8:
> > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3 | ISI_CFG2_COL_SPACE_YCbCr;
> > -		break;
> >  	case MEDIA_BUS_FMT_UYVY8_2X8:
> > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_2 | ISI_CFG2_COL_SPACE_YCbCr;
> > -		break;
> >  	case MEDIA_BUS_FMT_YVYU8_2X8:
> > -		cfg2 = ISI_CFG2_YCC_SWAP_MODE_1 | ISI_CFG2_COL_SPACE_YCbCr;
> > -		break;
> >  	case MEDIA_BUS_FMT_YUYV8_2X8:
> > -		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT | ISI_CFG2_COL_SPACE_YCbCr;
> > +		cfg2 = ISI_CFG2_COL_SPACE_YCbCr |
> > +				setup_cfg2_yuv_swap(isi, xlate);
> >  		break;
> >  	/* RGB, TODO */
> >  	}
> 
> I would move this switch-case completely inside setup_cfg2_yuv_swap(). 
> Just do
> 
> 	cfg2 = setup_cfg2_yuv_swap(isi, xlate);
> 
> and handle the
> 
>  	case MEDIA_BUS_FMT_Y8_1X8:
> 
> in the function too. These two switch-case statements really look 
> redundant.
> 
> > @@ -407,7 +430,7 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >  	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> >  
> >  	configure_geometry(isi, icd->user_width, icd->user_height,
> > -				icd->current_fmt->code);
> > +				icd->current_fmt);
> >  
> >  	spin_lock_irq(&isi->lock);
> >  	/* Clear any pending interrupt */
> > -- 
> > 1.9.1
> > 
> 
> Thanks
> Guennadi
>