Re: [PATCH dev-5.0 3/4] media: aspeed: remove checking of VE_INTERRUPT_CAPTURE_COMPLETE

[Andrew Jeffery]

On Fri, 26 Apr 2019, at 07:50, Jae Hyun Yoo wrote:
> VE_INTERRUPT_CAPTURE_COMPLETE and VE_INTERRUPT_COMP_COMPLETE are
> not set at the same time but the current interrupt handling
> mechanism of this driver doesn't clear the interrupt flag until
> both two are set, and this behavior causes unnecessary interrupt
> handler calls. In fact, this driver provides JPEG format only so
> taking care of the VE_INTERRUPT_COMP_COMPLETE is enough for getting
> compressed image frame so this commit gets rid of the
> VE_INTERRUPT_CAPTURE_COMPLETE checking logic to simplfy the logic.

Typo: s/simplfy/simplify/

It wouldn't have been too difficult to just split the IRQ handling and maintain
some state to track whether we've completed both pieces. That wouldn't
have hobbled supporting non-JPEG captures in the future, but at the same
time it's not like it's hard to add back and someone would have to patch the
driver to make non-JPEG modes work anyway.

I don't really have a dog in the race though, so I'm fine with the patch as it
stands.

Andrew

> 
> Signed-off-by: Jae Hyun Yoo <jae.hyun.yoo at linux.intel.com>
> ---
>  drivers/media/platform/aspeed-video.c | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/media/platform/aspeed-video.c 
> b/drivers/media/platform/aspeed-video.c
> index 429f676f9dea..77c209a472ca 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -463,8 +463,7 @@ static int aspeed_video_start_frame(struct 
> aspeed_video *video)
>  	aspeed_video_write(video, VE_COMP_ADDR, addr);
>  
>  	aspeed_video_update(video, VE_INTERRUPT_CTRL, 0,
> -			    VE_INTERRUPT_COMP_COMPLETE |
> -			    VE_INTERRUPT_CAPTURE_COMPLETE);
> +			    VE_INTERRUPT_COMP_COMPLETE);
>  
>  	aspeed_video_update(video, VE_SEQ_CTRL, 0,
>  			    VE_SEQ_CTRL_TRIG_CAPTURE | VE_SEQ_CTRL_TRIG_COMP);
> @@ -570,8 +569,7 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>  		}
>  	}
>  
> -	if ((sts & VE_INTERRUPT_COMP_COMPLETE) &&
> -	    (sts & VE_INTERRUPT_CAPTURE_COMPLETE)) {
> +	if (sts & VE_INTERRUPT_COMP_COMPLETE) {
>  		struct aspeed_video_buffer *buf;
>  		u32 frame_size = aspeed_video_read(video,
>  						   VE_OFFSET_COMP_STREAM);
> @@ -600,11 +598,9 @@ static irqreturn_t aspeed_video_irq(int irq, void *arg)
>  				    VE_SEQ_CTRL_FORCE_IDLE |
>  				    VE_SEQ_CTRL_TRIG_COMP, 0);
>  		aspeed_video_update(video, VE_INTERRUPT_CTRL,
> -				    VE_INTERRUPT_COMP_COMPLETE |
> -				    VE_INTERRUPT_CAPTURE_COMPLETE, 0);
> +				    VE_INTERRUPT_COMP_COMPLETE, 0);
>  		aspeed_video_write(video, VE_INTERRUPT_STATUS,
> -				   VE_INTERRUPT_COMP_COMPLETE |
> -				   VE_INTERRUPT_CAPTURE_COMPLETE);
> +				   VE_INTERRUPT_COMP_COMPLETE);
>  
>  		if (test_bit(VIDEO_STREAMING, &video->flags) && buf)
>  			aspeed_video_start_frame(video);
> -- 
> 2.21.0
> 
>