Messages in this thread Patch in this message | | | Subject | Re: [PATCH 1/2] media: atmel-isi: setup the ISI_CFG2 register directly | From | Josh Wu <> | Date | Mon, 31 Aug 2015 18:53:29 +0800 |
| |
Hi, Guennadi
Thanks for the review.
On 8/30/2015 4:48 PM, Guennadi Liakhovetski wrote: > Hi Josh, > > On Wed, 17 Jun 2015, Josh Wu wrote: > >> In the function configure_geometry(), we will setup the ISI CFG2 >> according to the sensor output format. >> >> It make no sense to just read back the CFG2 register and just set part >> of it. >> >> So just set up this register directly makes things simpler. > Simpler doesn't necessarily mean better or more correct:) There are other > fields in that register and currently the driver preserves them, with this > patch you overwrite them with 0. 0 happens to be the reset value of that > register. So, you should at least mention that in this patch description, > just saying "simpler" doesn't convince me.
Correct, I should mention that the reset value (0) of cfg2 means the YUV mode in the commit message. To use YUV mode we need to clear COL_SPACE (bit 15 of CFG2) and since the reset value is 0, so in the code, I didn't need do anything.
> So, at least I'd modify that, I > can do that myself. But in general I'm not even sure why this patch is > needed. Yes, currently those fields of that register are unused, so, we > can assume they stay at their reset values. But firstly the hardware can > change and at some point the reset value can change, or those other fields > will get set indirectly by something. Or the driver will change at some > point to support more fields of that register and then this code will have > to be changed again.
I understand your concern. maybe a better solution is explicitly set the COL_SPACE (bit 15) to 0 for the YUV formats. like:
#define ISI_CFG2_COL_SPACE_YUV (0 << 15)
case MEDIA_BUS_FMT_YVYU8_2X8: cfg2 = ISI_CFG2_COL_SPACE_YUV | ISI_CFG2_YCC_SWAP_MODE_1; break;
And above modifications can be sent with RGB format support patch in future.
> So, I'd ask you again - do you really want this > patch?
yes, this patch is needed. And in future i will add the RGB format settings.
> If you insist - I'll take it, but I'd add the "reset value" > reasoning.
That would be great, thank you very much.
Best Regards, Josh Wu
> Otherwise maybe just drop it? > > Thanks > Guennadi > >> Currently only support YUV format from camera sensor. >> >> Signed-off-by: Josh Wu <josh.wu@atmel.com> >> --- >> >> drivers/media/platform/soc_camera/atmel-isi.c | 20 +++++++------------- >> 1 file changed, 7 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c >> index 9070172..8bc40ca 100644 >> --- a/drivers/media/platform/soc_camera/atmel-isi.c >> +++ b/drivers/media/platform/soc_camera/atmel-isi.c >> @@ -105,24 +105,25 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg) >> static int configure_geometry(struct atmel_isi *isi, u32 width, >> u32 height, u32 code) >> { >> - u32 cfg2, cr; >> + u32 cfg2; >> >> + /* According to sensor's output format to set cfg2 */ >> switch (code) { >> /* YUV, including grey */ >> case MEDIA_BUS_FMT_Y8_1X8: >> - cr = ISI_CFG2_GRAYSCALE; >> + cfg2 = ISI_CFG2_GRAYSCALE; >> break; >> case MEDIA_BUS_FMT_VYUY8_2X8: >> - cr = ISI_CFG2_YCC_SWAP_MODE_3; >> + cfg2 = ISI_CFG2_YCC_SWAP_MODE_3; >> break; >> case MEDIA_BUS_FMT_UYVY8_2X8: >> - cr = ISI_CFG2_YCC_SWAP_MODE_2; >> + cfg2 = ISI_CFG2_YCC_SWAP_MODE_2; >> break; >> case MEDIA_BUS_FMT_YVYU8_2X8: >> - cr = ISI_CFG2_YCC_SWAP_MODE_1; >> + cfg2 = ISI_CFG2_YCC_SWAP_MODE_1; >> break; >> case MEDIA_BUS_FMT_YUYV8_2X8: >> - cr = ISI_CFG2_YCC_SWAP_DEFAULT; >> + cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT; >> break; >> /* RGB, TODO */ >> default: >> @@ -130,17 +131,10 @@ static int configure_geometry(struct atmel_isi *isi, u32 width, >> } >> >> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS); >> - >> - cfg2 = isi_readl(isi, ISI_CFG2); >> - /* Set YCC swap mode */ >> - cfg2 &= ~ISI_CFG2_YCC_SWAP_MODE_MASK; >> - cfg2 |= cr; >> /* Set width */ >> - cfg2 &= ~(ISI_CFG2_IM_HSIZE_MASK); >> cfg2 |= ((width - 1) << ISI_CFG2_IM_HSIZE_OFFSET) & >> ISI_CFG2_IM_HSIZE_MASK; >> /* Set height */ >> - cfg2 &= ~(ISI_CFG2_IM_VSIZE_MASK); >> cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET) >> & ISI_CFG2_IM_VSIZE_MASK; >> isi_writel(isi, ISI_CFG2, cfg2); >> -- >> 1.9.1 >>
| |