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

[Jae Hyun Yoo]

Hi Andrew,

On 4/29/2019 10:50 PM, Andrew Jeffery wrote:
> 
> 
> 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/

Will fix it.

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

Yes, I agree with you. The CAPTURE_COMPLETE event could be easily added
back when non-JPEG support is implemented in the future so for now that
event will be blocked for better performance and stability. If this
driver supports multiple formats, CAPTURE_COMPLETE and COMP_COMPLETE
should be selectively enabled in accordance with the format setting
to avoid unnecessary interrupt handling.

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

Thanks for your review!

Jae

> 
> 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
>>
>>