[v3,3/3] media: atmel-isi: add sanity check for supported formats in try/set_fmt()
Commit Message
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(-)
Comments
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.
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
>
--
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
@@ -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);