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 <[email protected]> > --- > > 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; > + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
