diff mbox

[v2,3/3] media: atmel-isi: add sanity check for supported formats in set_fmt()

Message ID 1438745190-21020-3-git-send-email-josh.wu@atmel.com (mailing list archive)
State Superseded, archived
Delegated to: Guennadi Liakhovetski
Headers

Commit Message

Josh Wu Aug. 5, 2015, 3:26 a.m. UTC 
  After adding the format check in 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 v2:
- new added patch

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

Comments

Josh Wu Aug. 20, 2015, 9:13 a.m. UTC | #1 
Hi, Laurent

Could you have time to review this patch? so that I can send a pull 
request with your reviewed-by tags.

Best Regards,
Josh Wu

On 8/5/2015 11:26 AM, Josh Wu wrote:
> After adding the format check in 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 v2:
> - new added patch
>
>   drivers/media/platform/soc_camera/atmel-isi.c | 39 +++++++++++++++++++++------
>   1 file changed, 31 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index cb46aec..d0df518 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -103,17 +103,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;
> @@ -127,8 +129,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);
> @@ -139,8 +139,29 @@ 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 struct soc_camera_format_xlate *xlate)
> +{
> +	bool ret = true;
> +
> +	switch (xlate->code) {
> +	/* YUV, including grey */
> +	case MEDIA_BUS_FMT_Y8_1X8:
> +	case MEDIA_BUS_FMT_VYUY8_2X8:
> +	case MEDIA_BUS_FMT_UYVY8_2X8:
> +	case MEDIA_BUS_FMT_YVYU8_2X8:
> +	case MEDIA_BUS_FMT_YUYV8_2X8:
> +		break;
> +	/* RGB, TODO */
> +	default:
> +		dev_err(icd->parent, "not supported format: %d\n",
> +					xlate->code);
> +		ret = false;
> +	}
> +
> +	return ret;
>   }
>   
>   static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -391,10 +412,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 */
> @@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device *icd,
>   	if (mf->code != xlate->code)
>   		return -EINVAL;
>   
> +	/* check with atmel-isi support format */
> +	if (!is_supported(icd, xlate))
> +		return -EINVAL;
> +
>   	pix->width		= mf->width;
>   	pix->height		= mf->height;
>   	pix->field		= mf->field;

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Laurent Pinchart Aug. 20, 2015, 6:30 p.m. UTC | #2 
Hi Josh,

Thank you for the patch.

On Wednesday 05 August 2015 11:26:29 Josh Wu wrote:
> After adding the format check in 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 v2:
> - new added patch
> 
>  drivers/media/platform/soc_camera/atmel-isi.c | 39 ++++++++++++++++++------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
> b/drivers/media/platform/soc_camera/atmel-isi.c index cb46aec..d0df518
> 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -103,17 +103,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;
> @@ -127,8 +129,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);
> @@ -139,8 +139,29 @@ 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 struct soc_camera_format_xlate *xlate)
> +{
> +	bool ret = true;
> +
> +	switch (xlate->code) {
> +	/* YUV, including grey */
> +	case MEDIA_BUS_FMT_Y8_1X8:
> +	case MEDIA_BUS_FMT_VYUY8_2X8:
> +	case MEDIA_BUS_FMT_UYVY8_2X8:
> +	case MEDIA_BUS_FMT_YVYU8_2X8:
> +	case MEDIA_BUS_FMT_YUYV8_2X8:

I would just return true here and false below, and remove the ret variable.

> +		break;
> +	/* RGB, TODO */
> +	default:
> +		dev_err(icd->parent, "not supported format: %d\n",
> +					xlate->code);

If this can happen when userspace asks for an unsupported format I don't think 
you should print an error message to the kernel log.

> +		ret = false;
> +	}
> +
> +	return ret;
>  }
> 
>  static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -391,10 +412,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 */
> @@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
> *icd, if (mf->code != xlate->code)
>  		return -EINVAL;
> 
> +	/* check with atmel-isi support format */
> +	if (!is_supported(icd, xlate))
> +		return -EINVAL;
> +

S_FMT is supposed to pick a suitable default format when the requested format 
isn't supported. It shouldn't return an error.

>  	pix->width		= mf->width;
>  	pix->height		= mf->height;
>  	pix->field		= mf->field;
Josh Wu Aug. 21, 2015, 3:10 a.m. UTC | #3 
Hi, Laurent

Thanks for the review.

On 8/21/2015 2:30 AM, Laurent Pinchart wrote:
> Hi Josh,
>
> Thank you for the patch.
>
> On Wednesday 05 August 2015 11:26:29 Josh Wu wrote:
>> After adding the format check in 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 v2:
>> - new added patch
>>
>>   drivers/media/platform/soc_camera/atmel-isi.c | 39 ++++++++++++++++++------
>>   1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
>> b/drivers/media/platform/soc_camera/atmel-isi.c index cb46aec..d0df518
>> 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -103,17 +103,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;
>> @@ -127,8 +129,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);
>> @@ -139,8 +139,29 @@ 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 struct soc_camera_format_xlate *xlate)
>> +{
>> +	bool ret = true;
>> +
>> +	switch (xlate->code) {
>> +	/* YUV, including grey */
>> +	case MEDIA_BUS_FMT_Y8_1X8:
>> +	case MEDIA_BUS_FMT_VYUY8_2X8:
>> +	case MEDIA_BUS_FMT_UYVY8_2X8:
>> +	case MEDIA_BUS_FMT_YVYU8_2X8:
>> +	case MEDIA_BUS_FMT_YUYV8_2X8:
> I would just return true here and false below, and remove the ret variable.
Ok.

>
>> +		break;
>> +	/* RGB, TODO */
>> +	default:
>> +		dev_err(icd->parent, "not supported format: %d\n",
>> +					xlate->code);
> If this can happen when userspace asks for an unsupported format I don't think
> you should print an error message to the kernel log.

ok, I will remove this.

>
>> +		ret = false;
>> +	}
>> +
>> +	return ret;
>>   }
>>
>>   static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>> @@ -391,10 +412,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 */
>> @@ -515,6 +534,10 @@ static int isi_camera_set_fmt(struct soc_camera_device
>> *icd, if (mf->code != xlate->code)
>>   		return -EINVAL;
>>
>> +	/* check with atmel-isi support format */
>> +	if (!is_supported(icd, xlate))
>> +		return -EINVAL;
>> +
> S_FMT is supposed to pick a suitable default format when the requested format
> isn't supported. It shouldn't return an error.

So I will move this check to beginning of S_FMT and if format not 
support, I will select a default format for it.

Best Regards,
Josh Wu

>
>>   	pix->width		= mf->width;
>>   	pix->height		= mf->height;
>>   	pix->field		= mf->field;

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

Patch

diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index cb46aec..d0df518 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -103,17 +103,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;
@@ -127,8 +129,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);
@@ -139,8 +139,29 @@  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 struct soc_camera_format_xlate *xlate)
+{
+	bool ret = true;
+
+	switch (xlate->code) {
+	/* YUV, including grey */
+	case MEDIA_BUS_FMT_Y8_1X8:
+	case MEDIA_BUS_FMT_VYUY8_2X8:
+	case MEDIA_BUS_FMT_UYVY8_2X8:
+	case MEDIA_BUS_FMT_YVYU8_2X8:
+	case MEDIA_BUS_FMT_YUYV8_2X8:
+		break;
+	/* RGB, TODO */
+	default:
+		dev_err(icd->parent, "not supported format: %d\n",
+					xlate->code);
+		ret = false;
+	}
+
+	return ret;
 }
 
 static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -391,10 +412,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 */
@@ -515,6 +534,10 @@  static int isi_camera_set_fmt(struct soc_camera_device *icd,
 	if (mf->code != xlate->code)
 		return -EINVAL;
 
+	/* check with atmel-isi support format */
+	if (!is_supported(icd, xlate))
+		return -EINVAL;
+
 	pix->width		= mf->width;
 	pix->height		= mf->height;
 	pix->field		= mf->field;