[PATCH v3 1/3] media: atmel-isi: setup the ISI_CFG2 register directly

[PATCH v3 1/3] media: atmel-isi: setup the ISI_CFG2 register directly

[Josh Wu]

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.
Currently only support YUV format from camera sensor.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Changes in v3: None
Changes in v2:
- add Laurent's reviewed-by tag.

 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 f39132c..b67da70 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

[PATCH v3 2/3] media: atmel-isi: move configure_geometry() to start_streaming()

[Josh Wu]

As in set_fmt() function we only need to know which format is been set,
we don't need to access the ISI hardware in this moment.

So move the configure_geometry(), which access the ISI hardware, to
start_streaming() will make code more consistent and simpler.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---

Changes in v3: None
Changes in v2:
- Add Laurent's reviewed-by tag.

 drivers/media/platform/soc_camera/atmel-isi.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index b67da70..fe9247a 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -390,6 +390,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	/* Disable all interrupts */
 	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
 
+	ret = configure_geometry(isi, icd->user_width, icd->user_height,
+				icd->current_fmt->code);
+	if (ret < 0)
+		return ret;
+
 	spin_lock_irq(&isi->lock);
 	/* Clear any pending interrupt */
 	isi_readl(isi, ISI_STATUS);
@@ -477,8 +482,6 @@ static int isi_camera_init_videobuf(struct vb2_queue *q,
 static int isi_camera_set_fmt(struct soc_camera_device *icd,
 			      struct v4l2_format *f)
 {
-	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
-	struct atmel_isi *isi = ici->priv;
 	struct v4l2_subdev *sd = soc_camera_to_subdev(icd);
 	const struct soc_camera_format_xlate *xlate;
 	struct v4l2_pix_format *pix = &f->fmt.pix;
@@ -511,16 +514,6 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
 	if (mf->code != xlate->code)
 		return -EINVAL;
 
-	/* Enable PM and peripheral clock before operate isi registers */
-	pm_runtime_get_sync(ici->v4l2_dev.dev);
-
-	ret = configure_geometry(isi, pix->width, pix->height, xlate->code);
-
-	pm_runtime_put(ici->v4l2_dev.dev);
-
-	if (ret < 0)
-		return ret;
-
 	pix->width		= mf->width;
 	pix->height		= mf->height;
 	pix->field		= mf->field;
-- 
1.9.1

[PATCH v3 3/3] media: atmel-isi: add sanity check for supported formats in try/set_fmt()

[Josh Wu]

After adding the format check in try_fmt()/set_fmt(), we don't need any
format check in configure_geometry(). So make configure_geometry() as
void type.

Signed-off-by: Josh Wu <josh.wu@atmel.com>
---

Changes in v3:
- check the whether format is supported, if no then return a default
  format.
- misc changes according to Laurent's feedback.

Changes in v2:
- new added patch

 drivers/media/platform/soc_camera/atmel-isi.c | 37 +++++++++++++++++++++------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index fe9247a..84c91d3 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -102,17 +102,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
 	return readl(isi->regs + reg);
 }
 
-static int configure_geometry(struct atmel_isi *isi, u32 width,
+static void configure_geometry(struct atmel_isi *isi, u32 width,
 			u32 height, u32 code)
 {
 	u32 cfg2;
 
 	/* According to sensor's output format to set cfg2 */
 	switch (code) {
-	/* YUV, including grey */
+	default:
+	/* Grey */
 	case MEDIA_BUS_FMT_Y8_1X8:
 		cfg2 = ISI_CFG2_GRAYSCALE;
 		break;
+	/* YUV */
 	case MEDIA_BUS_FMT_VYUY8_2X8:
 		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
 		break;
@@ -126,8 +128,6 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
 		cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
 		break;
 	/* RGB, TODO */
-	default:
-		return -EINVAL;
 	}
 
 	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
@@ -138,8 +138,23 @@ static int configure_geometry(struct atmel_isi *isi, u32 width,
 	cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
 			& ISI_CFG2_IM_VSIZE_MASK;
 	isi_writel(isi, ISI_CFG2, cfg2);
+}
 
-	return 0;
+static bool is_supported(struct soc_camera_device *icd,
+		const u32 pixformat)
+{
+	switch (pixformat) {
+	/* YUV, including grey */
+	case V4L2_PIX_FMT_GREY:
+	case V4L2_PIX_FMT_YUYV:
+	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_YVYU:
+	case V4L2_PIX_FMT_VYUY:
+		return true;
+	/* RGB, TODO */
+	default:
+		return false;
+	}
 }
 
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -390,10 +405,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	/* Disable all interrupts */
 	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
 
-	ret = configure_geometry(isi, icd->user_width, icd->user_height,
+	configure_geometry(isi, icd->user_width, icd->user_height,
 				icd->current_fmt->code);
-	if (ret < 0)
-		return ret;
 
 	spin_lock_irq(&isi->lock);
 	/* Clear any pending interrupt */
@@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
 	struct v4l2_mbus_framefmt *mf = &format.format;
 	int ret;
 
+	/* check with atmel-isi support format, if not support use UYVY */
+	if (!is_supported(icd, pix->pixelformat))
+		pix->pixelformat = V4L2_PIX_FMT_YUYV;
+
 	xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
 	if (!xlate) {
 		dev_warn(icd->parent, "Format %x not found\n",
@@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device *icd,
 	u32 pixfmt = pix->pixelformat;
 	int ret;
 
+	/* check with atmel-isi support format, if not support use UYVY */
+	if (!is_supported(icd, pix->pixelformat))
+		pix->pixelformat = V4L2_PIX_FMT_YUYV;
+
 	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
 	if (pixfmt && !xlate) {
 		dev_warn(icd->parent, "Format %x not found\n", pixfmt);
-- 
1.9.1

Re: [PATCH v3 3/3] media: atmel-isi: add sanity check for supported formats in try/set_fmt()

[Laurent Pinchart]

Hi Josh,

Thank you for the patch.

On Friday 21 August 2015 16:08:14 Josh Wu wrote:
> After adding the format check in try_fmt()/set_fmt(), we don't need any
> format check in configure_geometry(). So make configure_geometry() as
> void type.
> 
> Signed-off-by: Josh Wu <josh.wu@atmel.com>
> ---
> 
> Changes in v3:
> - check the whether format is supported, if no then return a default
>   format.
> - misc changes according to Laurent's feedback.
> 
> Changes in v2:
> - new added patch
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 37 ++++++++++++++++++------
>  1 file changed, 29 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index fe9247a..84c91d3
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -102,17 +102,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>  	return readl(isi->regs + reg);
>  }
> 
> -static int configure_geometry(struct atmel_isi *isi, u32 width,
> +static void configure_geometry(struct atmel_isi *isi, u32 width,
>  			u32 height, u32 code)
>  {
>  	u32 cfg2;
> 
>  	/* According to sensor's output format to set cfg2 */
>  	switch (code) {
> -	/* YUV, including grey */
> +	default:
> +	/* Grey */
>  	case MEDIA_BUS_FMT_Y8_1X8:
>  		cfg2 = ISI_CFG2_GRAYSCALE;
>  		break;
> +	/* YUV */
>  	case MEDIA_BUS_FMT_VYUY8_2X8:
>  		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
>  		break;
> @@ -126,8 +128,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
> width, cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
>  		break;
>  	/* RGB, TODO */
> -	default:
> -		return -EINVAL;
>  	}
> 
>  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> @@ -138,8 +138,23 @@ static int configure_geometry(struct atmel_isi *isi,
> u32 width, cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>  			& ISI_CFG2_IM_VSIZE_MASK;
>  	isi_writel(isi, ISI_CFG2, cfg2);
> +}
> 
> -	return 0;
> +static bool is_supported(struct soc_camera_device *icd,
> +		const u32 pixformat)
> +{
> +	switch (pixformat) {
> +	/* YUV, including grey */
> +	case V4L2_PIX_FMT_GREY:
> +	case V4L2_PIX_FMT_YUYV:
> +	case V4L2_PIX_FMT_UYVY:
> +	case V4L2_PIX_FMT_YVYU:
> +	case V4L2_PIX_FMT_VYUY:
> +		return true;
> +	/* RGB, TODO */
> +	default:
> +		return false;
> +	}
>  }
> 
>  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -390,10 +405,8 @@ static int start_streaming(struct vb2_queue *vq,
> unsigned int count) /* Disable all interrupts */
>  	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> 
> -	ret = configure_geometry(isi, icd->user_width, icd->user_height,
> +	configure_geometry(isi, icd->user_width, icd->user_height,
>  				icd->current_fmt->code);
> -	if (ret < 0)
> -		return ret;
> 
>  	spin_lock_irq(&isi->lock);
>  	/* Clear any pending interrupt */
> @@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
> *icd, struct v4l2_mbus_framefmt *mf = &format.format;
>  	int ret;
> 
> +	/* check with atmel-isi support format, if not support use UYVY */
> +	if (!is_supported(icd, pix->pixelformat))
> +		pix->pixelformat = V4L2_PIX_FMT_YUYV;

The comment mentions UYVY and the code uses YUYV.

> +
>  	xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
>  	if (!xlate) {
>  		dev_warn(icd->parent, "Format %x not found\n",

Can this still happen ?

> @@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device
> *icd, u32 pixfmt = pix->pixelformat;
>  	int ret;
> 
> +	/* check with atmel-isi support format, if not support use UYVY */
> +	if (!is_supported(icd, pix->pixelformat))
> +		pix->pixelformat = V4L2_PIX_FMT_YUYV;
> +
>  	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
>  	if (pixfmt && !xlate) {
>  		dev_warn(icd->parent, "Format %x not found\n", pixfmt);

Same comment here.

I wonder whether most of the content of isi_camera_set_fmt() and 
isi_camera_try_fmt() could be factorized out into a shared function.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v3 3/3] media: atmel-isi: add sanity check for supported formats in try/set_fmt()

[Josh Wu]

Hi, Laurent

On 8/22/2015 2:22 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Friday 21 August 2015 16:08:14 Josh Wu wrote:
>> After adding the format check in try_fmt()/set_fmt(), we don't need any
>> format check in configure_geometry(). So make configure_geometry() as
>> void type.
>>
>> Signed-off-by: Josh Wu <josh.wu@atmel.com>
>> ---
>>
>> Changes in v3:
>> - check the whether format is supported, if no then return a default
>>    format.
>> - misc changes according to Laurent's feedback.
>>
>> Changes in v2:
>> - new added patch
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 37 ++++++++++++++++++------
>>   1 file changed, 29 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>> b/drivers/media/platform/soc_camera/atmel-isi.c index fe9247a..84c91d3
>> 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -102,17 +102,19 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
>>   	return readl(isi->regs + reg);
>>   }
>>
>> -static int configure_geometry(struct atmel_isi *isi, u32 width,
>> +static void configure_geometry(struct atmel_isi *isi, u32 width,
>>   			u32 height, u32 code)
>>   {
>>   	u32 cfg2;
>>
>>   	/* According to sensor's output format to set cfg2 */
>>   	switch (code) {
>> -	/* YUV, including grey */
>> +	default:
>> +	/* Grey */
>>   	case MEDIA_BUS_FMT_Y8_1X8:
>>   		cfg2 = ISI_CFG2_GRAYSCALE;
>>   		break;
>> +	/* YUV */
>>   	case MEDIA_BUS_FMT_VYUY8_2X8:
>>   		cfg2 = ISI_CFG2_YCC_SWAP_MODE_3;
>>   		break;
>> @@ -126,8 +128,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
>> width, cfg2 = ISI_CFG2_YCC_SWAP_DEFAULT;
>>   		break;
>>   	/* RGB, TODO */
>> -	default:
>> -		return -EINVAL;
>>   	}
>>
>>   	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> @@ -138,8 +138,23 @@ static int configure_geometry(struct atmel_isi *isi,
>> u32 width, cfg2 |= ((height - 1) << ISI_CFG2_IM_VSIZE_OFFSET)
>>   			& ISI_CFG2_IM_VSIZE_MASK;
>>   	isi_writel(isi, ISI_CFG2, cfg2);
>> +}
>>
>> -	return 0;
>> +static bool is_supported(struct soc_camera_device *icd,
>> +		const u32 pixformat)
>> +{
>> +	switch (pixformat) {
>> +	/* YUV, including grey */
>> +	case V4L2_PIX_FMT_GREY:
>> +	case V4L2_PIX_FMT_YUYV:
>> +	case V4L2_PIX_FMT_UYVY:
>> +	case V4L2_PIX_FMT_YVYU:
>> +	case V4L2_PIX_FMT_VYUY:
>> +		return true;
>> +	/* RGB, TODO */
>> +	default:
>> +		return false;
>> +	}
>>   }
>>
>>   static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>> @@ -390,10 +405,8 @@ static int start_streaming(struct vb2_queue *vq,
>> unsigned int count) /* Disable all interrupts */
>>   	isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>>
>> -	ret = configure_geometry(isi, icd->user_width, icd->user_height,
>> +	configure_geometry(isi, icd->user_width, icd->user_height,
>>   				icd->current_fmt->code);
>> -	if (ret < 0)
>> -		return ret;
>>
>>   	spin_lock_irq(&isi->lock);
>>   	/* Clear any pending interrupt */
>> @@ -491,6 +504,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
>> *icd, struct v4l2_mbus_framefmt *mf = &format.format;
>>   	int ret;
>>
>> +	/* check with atmel-isi support format, if not support use UYVY */
>> +	if (!is_supported(icd, pix->pixelformat))
>> +		pix->pixelformat = V4L2_PIX_FMT_YUYV;
> The comment mentions UYVY and the code uses YUYV.

Oops, forgotten to change the comments. I'll fix it.
>
>> +
>>   	xlate = soc_camera_xlate_by_fourcc(icd, pix->pixelformat);
>>   	if (!xlate) {
>>   		dev_warn(icd->parent, "Format %x not found\n",
> Can this still happen ?

I think this warning should not happen if user call set_fmt() with the 
format that get from by get_formats().
But it is a common pattern in soc_camera host code to handle this error.

>
>> @@ -540,6 +557,10 @@ static int isi_camera_try_fmt(struct soc_camera_device
>> *icd, u32 pixfmt = pix->pixelformat;
>>   	int ret;
>>
>> +	/* check with atmel-isi support format, if not support use UYVY */
>> +	if (!is_supported(icd, pix->pixelformat))
>> +		pix->pixelformat = V4L2_PIX_FMT_YUYV;
>> +
>>   	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
>>   	if (pixfmt && !xlate) {
>>   		dev_warn(icd->parent, "Format %x not found\n", pixfmt);
> Same comment here.
>
> I wonder whether most of the content of isi_camera_set_fmt() and
> isi_camera_try_fmt() could be factorized out into a shared function.

I agree. the current set_fmt() doesn't touch any hardware, so it's 
almost same as try_fmt().

Best Regards,
Josh Wu
>