diff mbox

[v1,media] atmel-isi: code cleanup

Message ID 1495188292-3113-2-git-send-email-hugues.fruchet@st.com (mailing list archive)
State New, archived
Headers show

Commit Message

Hugues FRUCHET May 19, 2017, 10:04 a.m. UTC
Ensure that ISI is clocked before starting sensor sub device.
Remove un-needed type check in try_fmt().
Use clamp() macro for hardware capabilities.
Fix wrong tabulation to space.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/platform/atmel/atmel-isi.c | 24 ++++++++++--------------
 1 file changed, 10 insertions(+), 14 deletions(-)

Comments

Hugues FRUCHET May 19, 2017, 12:08 p.m. UTC | #1
Adding Songjun and Ludovic as Atmel maintainers, sorry for inconvenience.

On 05/19/2017 12:04 PM, Hugues Fruchet wrote:
> Ensure that ISI is clocked before starting sensor sub device.

> Remove un-needed type check in try_fmt().

> Use clamp() macro for hardware capabilities.

> Fix wrong tabulation to space.

> 

> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>

> ---

>   drivers/media/platform/atmel/atmel-isi.c | 24 ++++++++++--------------

>   1 file changed, 10 insertions(+), 14 deletions(-)

> 

> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c

> index e4867f8..7bf9f7d 100644

> --- a/drivers/media/platform/atmel/atmel-isi.c

> +++ b/drivers/media/platform/atmel/atmel-isi.c

> @@ -36,8 +36,8 @@

>   

>   #include "atmel-isi.h"

>   

> -#define MAX_SUPPORT_WIDTH		2048

> -#define MAX_SUPPORT_HEIGHT		2048

> +#define MAX_SUPPORT_WIDTH		2048U

> +#define MAX_SUPPORT_HEIGHT		2048U

>   #define MIN_FRAME_RATE			15

>   #define FRAME_INTERVAL_MILLI_SEC	(1000 / MIN_FRAME_RATE)

>   

> @@ -424,6 +424,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)

>   	struct frame_buffer *buf, *node;

>   	int ret;

>   

> +	pm_runtime_get_sync(isi->dev);

> +

>   	/* Enable stream on the sub device */

>   	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);

>   	if (ret && ret != -ENOIOCTLCMD) {

> @@ -431,8 +433,6 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)

>   		goto err_start_stream;

>   	}

>   

> -	pm_runtime_get_sync(isi->dev);

> -

>   	/* Reset ISI */

>   	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);

>   	if (ret < 0) {

> @@ -455,10 +455,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)

>   	return 0;

>   

>   err_reset:

> -	pm_runtime_put(isi->dev);

>   	v4l2_subdev_call(isi->entity.subdev, video, s_stream, 0);

>   

>   err_start_stream:

> +	pm_runtime_put(isi->dev);

> +

>   	spin_lock_irq(&isi->irqlock);

>   	isi->active = NULL;

>   	/* Release all active buffers */

> @@ -566,20 +567,15 @@ static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,

>   	};

>   	int ret;

>   

> -	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)

> -		return -EINVAL;

> -

>   	isi_fmt = find_format_by_fourcc(isi, pixfmt->pixelformat);

>   	if (!isi_fmt) {

>   		isi_fmt = isi->user_formats[isi->num_user_formats - 1];

>   		pixfmt->pixelformat = isi_fmt->fourcc;

>   	}

>   

> -	/* Limit to Atmel ISC hardware capabilities */

> -	if (pixfmt->width > MAX_SUPPORT_WIDTH)

> -		pixfmt->width = MAX_SUPPORT_WIDTH;

> -	if (pixfmt->height > MAX_SUPPORT_HEIGHT)

> -		pixfmt->height = MAX_SUPPORT_HEIGHT;

> +	/* Limit to Atmel ISI hardware capabilities */

> +	pixfmt->width = clamp(pixfmt->width, 0U, MAX_SUPPORT_WIDTH);

> +	pixfmt->height = clamp(pixfmt->height, 0U, MAX_SUPPORT_HEIGHT);

>   

>   	v4l2_fill_mbus_format(&format.format, pixfmt, isi_fmt->mbus_code);

>   	ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,

> @@ -1058,7 +1054,7 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)

>   	struct atmel_isi *isi = notifier_to_isi(notifier);

>   	int ret;

>   

> -	isi->vdev->ctrl_handler	= isi->entity.subdev->ctrl_handler;

> +	isi->vdev->ctrl_handler = isi->entity.subdev->ctrl_handler;

>   	ret = isi_formats_init(isi);

>   	if (ret) {

>   		dev_err(isi->dev, "No supported mediabus format found\n");

>
Songjun Wu May 22, 2017, 5:02 a.m. UTC | #2
Hi Hugues,

Thank you for your patch.
Is it necessary to ensure ISI is clocked before starting sensor sub device?

On 5/19/2017 20:08, Hugues FRUCHET wrote:
> Adding Songjun and Ludovic as Atmel maintainers, sorry for inconvenience.
> 
> On 05/19/2017 12:04 PM, Hugues Fruchet wrote:
>> Ensure that ISI is clocked before starting sensor sub device.
>> Remove un-needed type check in try_fmt().
>> Use clamp() macro for hardware capabilities.
>> Fix wrong tabulation to space.
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>    drivers/media/platform/atmel/atmel-isi.c | 24 ++++++++++--------------
>>    1 file changed, 10 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
>> index e4867f8..7bf9f7d 100644
>> --- a/drivers/media/platform/atmel/atmel-isi.c
>> +++ b/drivers/media/platform/atmel/atmel-isi.c
>> @@ -36,8 +36,8 @@
>>    
>>    #include "atmel-isi.h"
>>    
>> -#define MAX_SUPPORT_WIDTH		2048
>> -#define MAX_SUPPORT_HEIGHT		2048
>> +#define MAX_SUPPORT_WIDTH		2048U
>> +#define MAX_SUPPORT_HEIGHT		2048U
>>    #define MIN_FRAME_RATE			15
>>    #define FRAME_INTERVAL_MILLI_SEC	(1000 / MIN_FRAME_RATE)
>>    
>> @@ -424,6 +424,8 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>    	struct frame_buffer *buf, *node;
>>    	int ret;
>>    
>> +	pm_runtime_get_sync(isi->dev);
>> +
>>    	/* Enable stream on the sub device */
>>    	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
>>    	if (ret && ret != -ENOIOCTLCMD) {
>> @@ -431,8 +433,6 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>    		goto err_start_stream;
>>    	}
>>    
>> -	pm_runtime_get_sync(isi->dev);
>> -
>>    	/* Reset ISI */
>>    	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
>>    	if (ret < 0) {
>> @@ -455,10 +455,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>    	return 0;
>>    
>>    err_reset:
>> -	pm_runtime_put(isi->dev);
>>    	v4l2_subdev_call(isi->entity.subdev, video, s_stream, 0);
>>    
>>    err_start_stream:
>> +	pm_runtime_put(isi->dev);
>> +
>>    	spin_lock_irq(&isi->irqlock);
>>    	isi->active = NULL;
>>    	/* Release all active buffers */
>> @@ -566,20 +567,15 @@ static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
>>    	};
>>    	int ret;
>>    
>> -	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>> -		return -EINVAL;
>> -
>>    	isi_fmt = find_format_by_fourcc(isi, pixfmt->pixelformat);
>>    	if (!isi_fmt) {
>>    		isi_fmt = isi->user_formats[isi->num_user_formats - 1];
>>    		pixfmt->pixelformat = isi_fmt->fourcc;
>>    	}
>>    
>> -	/* Limit to Atmel ISC hardware capabilities */
>> -	if (pixfmt->width > MAX_SUPPORT_WIDTH)
>> -		pixfmt->width = MAX_SUPPORT_WIDTH;
>> -	if (pixfmt->height > MAX_SUPPORT_HEIGHT)
>> -		pixfmt->height = MAX_SUPPORT_HEIGHT;
>> +	/* Limit to Atmel ISI hardware capabilities */
>> +	pixfmt->width = clamp(pixfmt->width, 0U, MAX_SUPPORT_WIDTH);
>> +	pixfmt->height = clamp(pixfmt->height, 0U, MAX_SUPPORT_HEIGHT);
>>    
>>    	v4l2_fill_mbus_format(&format.format, pixfmt, isi_fmt->mbus_code);
>>    	ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
>> @@ -1058,7 +1054,7 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>>    	struct atmel_isi *isi = notifier_to_isi(notifier);
>>    	int ret;
>>    
>> -	isi->vdev->ctrl_handler	= isi->entity.subdev->ctrl_handler;
>> +	isi->vdev->ctrl_handler = isi->entity.subdev->ctrl_handler;
>>    	ret = isi_formats_init(isi);
>>    	if (ret) {
>>    		dev_err(isi->dev, "No supported mediabus format found\n");
Hugues FRUCHET May 22, 2017, 7:52 a.m. UTC | #3
Hi Songjun,

It was an advice from Hans, I copy/paste the comment here:
http://www.mail-archive.com/linux-media@vger.kernel.org/msg112338.html
 >> +     /* Enable stream on the sub device */

 >> +     ret = v4l2_subdev_call(dcmi->entity.subdev, video, s_stream, 1);

 >> +     if (ret && ret != -ENOIOCTLCMD) {

 >> +             dev_err(dcmi->dev, "%s: Failed to start streaming, subdev

 >> streamon error",

 >> +                     __func__);

 >> +             goto err_release_buffers;

 >> +     }

 >> +

 >> +     if (clk_enable(dcmi->mclk)) {

 >> +             dev_err(dcmi->dev, "%s: Failed to start streaming, cannot

 >> enable clock",

 >> +                     __func__);

 >> +             goto err_subdev_streamoff;

 >> +     }

 >It feels more natural to me to first enable the clock, then call 

 >s_stream.


Please note that I have not tested code, but only reported changes done 
in ST DCMI driver to reflect the same on ISI driver, would it be 
possible that you check that it is still functional on your side ?

Best regards,
Hugues.

On 05/22/2017 07:02 AM, Wu, Songjun wrote:
> Hi Hugues,

> 

> Thank you for your patch.

> Is it necessary to ensure ISI is clocked before starting sensor sub device?

> 

> On 5/19/2017 20:08, Hugues FRUCHET wrote:

>> Adding Songjun and Ludovic as Atmel maintainers, sorry for inconvenience.

>>

>> On 05/19/2017 12:04 PM, Hugues Fruchet wrote:

>>> Ensure that ISI is clocked before starting sensor sub device.

>>> Remove un-needed type check in try_fmt().

>>> Use clamp() macro for hardware capabilities.

>>> Fix wrong tabulation to space.

>>>

>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>

>>> ---

>>>    drivers/media/platform/atmel/atmel-isi.c | 24 

>>> ++++++++++--------------

>>>    1 file changed, 10 insertions(+), 14 deletions(-)

>>>

>>> diff --git a/drivers/media/platform/atmel/atmel-isi.c 

>>> b/drivers/media/platform/atmel/atmel-isi.c

>>> index e4867f8..7bf9f7d 100644

>>> --- a/drivers/media/platform/atmel/atmel-isi.c

>>> +++ b/drivers/media/platform/atmel/atmel-isi.c

>>> @@ -36,8 +36,8 @@

>>>    #include "atmel-isi.h"

>>> -#define MAX_SUPPORT_WIDTH        2048

>>> -#define MAX_SUPPORT_HEIGHT        2048

>>> +#define MAX_SUPPORT_WIDTH        2048U

>>> +#define MAX_SUPPORT_HEIGHT        2048U

>>>    #define MIN_FRAME_RATE            15

>>>    #define FRAME_INTERVAL_MILLI_SEC    (1000 / MIN_FRAME_RATE)

>>> @@ -424,6 +424,8 @@ static int start_streaming(struct vb2_queue *vq, 

>>> unsigned int count)

>>>        struct frame_buffer *buf, *node;

>>>        int ret;

>>> +    pm_runtime_get_sync(isi->dev);

>>> +

>>>        /* Enable stream on the sub device */

>>>        ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);

>>>        if (ret && ret != -ENOIOCTLCMD) {

>>> @@ -431,8 +433,6 @@ static int start_streaming(struct vb2_queue *vq, 

>>> unsigned int count)

>>>            goto err_start_stream;

>>>        }

>>> -    pm_runtime_get_sync(isi->dev);

>>> -

>>>        /* Reset ISI */

>>>        ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);

>>>        if (ret < 0) {

>>> @@ -455,10 +455,11 @@ static int start_streaming(struct vb2_queue 

>>> *vq, unsigned int count)

>>>        return 0;

>>>    err_reset:

>>> -    pm_runtime_put(isi->dev);

>>>        v4l2_subdev_call(isi->entity.subdev, video, s_stream, 0);

>>>    err_start_stream:

>>> +    pm_runtime_put(isi->dev);

>>> +

>>>        spin_lock_irq(&isi->irqlock);

>>>        isi->active = NULL;

>>>        /* Release all active buffers */

>>> @@ -566,20 +567,15 @@ static int isi_try_fmt(struct atmel_isi *isi, 

>>> struct v4l2_format *f,

>>>        };

>>>        int ret;

>>> -    if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)

>>> -        return -EINVAL;

>>> -

>>>        isi_fmt = find_format_by_fourcc(isi, pixfmt->pixelformat);

>>>        if (!isi_fmt) {

>>>            isi_fmt = isi->user_formats[isi->num_user_formats - 1];

>>>            pixfmt->pixelformat = isi_fmt->fourcc;

>>>        }

>>> -    /* Limit to Atmel ISC hardware capabilities */

>>> -    if (pixfmt->width > MAX_SUPPORT_WIDTH)

>>> -        pixfmt->width = MAX_SUPPORT_WIDTH;

>>> -    if (pixfmt->height > MAX_SUPPORT_HEIGHT)

>>> -        pixfmt->height = MAX_SUPPORT_HEIGHT;

>>> +    /* Limit to Atmel ISI hardware capabilities */

>>> +    pixfmt->width = clamp(pixfmt->width, 0U, MAX_SUPPORT_WIDTH);

>>> +    pixfmt->height = clamp(pixfmt->height, 0U, MAX_SUPPORT_HEIGHT);

>>>        v4l2_fill_mbus_format(&format.format, pixfmt, 

>>> isi_fmt->mbus_code);

>>>        ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,

>>> @@ -1058,7 +1054,7 @@ static int isi_graph_notify_complete(struct 

>>> v4l2_async_notifier *notifier)

>>>        struct atmel_isi *isi = notifier_to_isi(notifier);

>>>        int ret;

>>> -    isi->vdev->ctrl_handler    = isi->entity.subdev->ctrl_handler;

>>> +    isi->vdev->ctrl_handler = isi->entity.subdev->ctrl_handler;

>>>        ret = isi_formats_init(isi);

>>>        if (ret) {

>>>            dev_err(isi->dev, "No supported mediabus format found\n");
Songjun Wu May 22, 2017, 8:02 a.m. UTC | #4
On 5/22/2017 15:52, Hugues FRUCHET wrote:
> Hi Songjun,
> 
> It was an advice from Hans, I copy/paste the comment here:
> http://www.mail-archive.com/linux-media@vger.kernel.org/msg112338.html
>   >> +     /* Enable stream on the sub device */
>   >> +     ret = v4l2_subdev_call(dcmi->entity.subdev, video, s_stream, 1);
>   >> +     if (ret && ret != -ENOIOCTLCMD) {
>   >> +             dev_err(dcmi->dev, "%s: Failed to start streaming, subdev
>   >> streamon error",
>   >> +                     __func__);
>   >> +             goto err_release_buffers;
>   >> +     }
>   >> +
>   >> +     if (clk_enable(dcmi->mclk)) {
>   >> +             dev_err(dcmi->dev, "%s: Failed to start streaming, cannot
>   >> enable clock",
>   >> +                     __func__);
>   >> +             goto err_subdev_streamoff;
>   >> +     }
>   >It feels more natural to me to first enable the clock, then call
>   >s_stream.
> 
> Please note that I have not tested code, but only reported changes done
> in ST DCMI driver to reflect the same on ISI driver, would it be
> possible that you check that it is still functional on your side ?
> 
Hi Hugues,

Thank you for your explanation.
It does not affect the function, but since it is more natural to first 
enable the clock, then call s_stream, I think this patch has no problem.

> Best regards,
> Hugues.
> 
> On 05/22/2017 07:02 AM, Wu, Songjun wrote:
>> Hi Hugues,
>>
>> Thank you for your patch.
>> Is it necessary to ensure ISI is clocked before starting sensor sub device?
>>
>> On 5/19/2017 20:08, Hugues FRUCHET wrote:
>>> Adding Songjun and Ludovic as Atmel maintainers, sorry for inconvenience.
>>>
>>> On 05/19/2017 12:04 PM, Hugues Fruchet wrote:
>>>> Ensure that ISI is clocked before starting sensor sub device.
>>>> Remove un-needed type check in try_fmt().
>>>> Use clamp() macro for hardware capabilities.
>>>> Fix wrong tabulation to space.
>>>>
>>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>>> ---
>>>>     drivers/media/platform/atmel/atmel-isi.c | 24
>>>> ++++++++++--------------
>>>>     1 file changed, 10 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/media/platform/atmel/atmel-isi.c
>>>> b/drivers/media/platform/atmel/atmel-isi.c
>>>> index e4867f8..7bf9f7d 100644
>>>> --- a/drivers/media/platform/atmel/atmel-isi.c
>>>> +++ b/drivers/media/platform/atmel/atmel-isi.c
>>>> @@ -36,8 +36,8 @@
>>>>     #include "atmel-isi.h"
>>>> -#define MAX_SUPPORT_WIDTH        2048
>>>> -#define MAX_SUPPORT_HEIGHT        2048
>>>> +#define MAX_SUPPORT_WIDTH        2048U
>>>> +#define MAX_SUPPORT_HEIGHT        2048U
>>>>     #define MIN_FRAME_RATE            15
>>>>     #define FRAME_INTERVAL_MILLI_SEC    (1000 / MIN_FRAME_RATE)
>>>> @@ -424,6 +424,8 @@ static int start_streaming(struct vb2_queue *vq,
>>>> unsigned int count)
>>>>         struct frame_buffer *buf, *node;
>>>>         int ret;
>>>> +    pm_runtime_get_sync(isi->dev);
>>>> +
>>>>         /* Enable stream on the sub device */
>>>>         ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
>>>>         if (ret && ret != -ENOIOCTLCMD) {
>>>> @@ -431,8 +433,6 @@ static int start_streaming(struct vb2_queue *vq,
>>>> unsigned int count)
>>>>             goto err_start_stream;
>>>>         }
>>>> -    pm_runtime_get_sync(isi->dev);
>>>> -
>>>>         /* Reset ISI */
>>>>         ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
>>>>         if (ret < 0) {
>>>> @@ -455,10 +455,11 @@ static int start_streaming(struct vb2_queue
>>>> *vq, unsigned int count)
>>>>         return 0;
>>>>     err_reset:
>>>> -    pm_runtime_put(isi->dev);
>>>>         v4l2_subdev_call(isi->entity.subdev, video, s_stream, 0);
>>>>     err_start_stream:
>>>> +    pm_runtime_put(isi->dev);
>>>> +
>>>>         spin_lock_irq(&isi->irqlock);
>>>>         isi->active = NULL;
>>>>         /* Release all active buffers */
>>>> @@ -566,20 +567,15 @@ static int isi_try_fmt(struct atmel_isi *isi,
>>>> struct v4l2_format *f,
>>>>         };
>>>>         int ret;
>>>> -    if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
>>>> -        return -EINVAL;
>>>> -
>>>>         isi_fmt = find_format_by_fourcc(isi, pixfmt->pixelformat);
>>>>         if (!isi_fmt) {
>>>>             isi_fmt = isi->user_formats[isi->num_user_formats - 1];
>>>>             pixfmt->pixelformat = isi_fmt->fourcc;
>>>>         }
>>>> -    /* Limit to Atmel ISC hardware capabilities */
>>>> -    if (pixfmt->width > MAX_SUPPORT_WIDTH)
>>>> -        pixfmt->width = MAX_SUPPORT_WIDTH;
>>>> -    if (pixfmt->height > MAX_SUPPORT_HEIGHT)
>>>> -        pixfmt->height = MAX_SUPPORT_HEIGHT;
>>>> +    /* Limit to Atmel ISI hardware capabilities */
>>>> +    pixfmt->width = clamp(pixfmt->width, 0U, MAX_SUPPORT_WIDTH);
>>>> +    pixfmt->height = clamp(pixfmt->height, 0U, MAX_SUPPORT_HEIGHT);
>>>>         v4l2_fill_mbus_format(&format.format, pixfmt,
>>>> isi_fmt->mbus_code);
>>>>         ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
>>>> @@ -1058,7 +1054,7 @@ static int isi_graph_notify_complete(struct
>>>> v4l2_async_notifier *notifier)
>>>>         struct atmel_isi *isi = notifier_to_isi(notifier);
>>>>         int ret;
>>>> -    isi->vdev->ctrl_handler    = isi->entity.subdev->ctrl_handler;
>>>> +    isi->vdev->ctrl_handler = isi->entity.subdev->ctrl_handler;
>>>>         ret = isi_formats_init(isi);
>>>>         if (ret) {
>>>>             dev_err(isi->dev, "No supported mediabus format found\n");
diff mbox

Patch

diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index e4867f8..7bf9f7d 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -36,8 +36,8 @@ 
 
 #include "atmel-isi.h"
 
-#define MAX_SUPPORT_WIDTH		2048
-#define MAX_SUPPORT_HEIGHT		2048
+#define MAX_SUPPORT_WIDTH		2048U
+#define MAX_SUPPORT_HEIGHT		2048U
 #define MIN_FRAME_RATE			15
 #define FRAME_INTERVAL_MILLI_SEC	(1000 / MIN_FRAME_RATE)
 
@@ -424,6 +424,8 @@  static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct frame_buffer *buf, *node;
 	int ret;
 
+	pm_runtime_get_sync(isi->dev);
+
 	/* Enable stream on the sub device */
 	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
 	if (ret && ret != -ENOIOCTLCMD) {
@@ -431,8 +433,6 @@  static int start_streaming(struct vb2_queue *vq, unsigned int count)
 		goto err_start_stream;
 	}
 
-	pm_runtime_get_sync(isi->dev);
-
 	/* Reset ISI */
 	ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
 	if (ret < 0) {
@@ -455,10 +455,11 @@  static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	return 0;
 
 err_reset:
-	pm_runtime_put(isi->dev);
 	v4l2_subdev_call(isi->entity.subdev, video, s_stream, 0);
 
 err_start_stream:
+	pm_runtime_put(isi->dev);
+
 	spin_lock_irq(&isi->irqlock);
 	isi->active = NULL;
 	/* Release all active buffers */
@@ -566,20 +567,15 @@  static int isi_try_fmt(struct atmel_isi *isi, struct v4l2_format *f,
 	};
 	int ret;
 
-	if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		return -EINVAL;
-
 	isi_fmt = find_format_by_fourcc(isi, pixfmt->pixelformat);
 	if (!isi_fmt) {
 		isi_fmt = isi->user_formats[isi->num_user_formats - 1];
 		pixfmt->pixelformat = isi_fmt->fourcc;
 	}
 
-	/* Limit to Atmel ISC hardware capabilities */
-	if (pixfmt->width > MAX_SUPPORT_WIDTH)
-		pixfmt->width = MAX_SUPPORT_WIDTH;
-	if (pixfmt->height > MAX_SUPPORT_HEIGHT)
-		pixfmt->height = MAX_SUPPORT_HEIGHT;
+	/* Limit to Atmel ISI hardware capabilities */
+	pixfmt->width = clamp(pixfmt->width, 0U, MAX_SUPPORT_WIDTH);
+	pixfmt->height = clamp(pixfmt->height, 0U, MAX_SUPPORT_HEIGHT);
 
 	v4l2_fill_mbus_format(&format.format, pixfmt, isi_fmt->mbus_code);
 	ret = v4l2_subdev_call(isi->entity.subdev, pad, set_fmt,
@@ -1058,7 +1054,7 @@  static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)
 	struct atmel_isi *isi = notifier_to_isi(notifier);
 	int ret;
 
-	isi->vdev->ctrl_handler	= isi->entity.subdev->ctrl_handler;
+	isi->vdev->ctrl_handler = isi->entity.subdev->ctrl_handler;
 	ret = isi_formats_init(isi);
 	if (ret) {
 		dev_err(isi->dev, "No supported mediabus format found\n");