Re: [PATCH] sh_mobile_ceu_camera: add VBP error cure operation

Dear Guennadi

Thank you for advice.

> I think, it would be good to add a coment here, explaining, that the 
> return code doesn't reflex the success or failure to queue the new buffer, 
> but rather the status of the previous buffer, if any.

OK. I add explaining

> > +	/* VBP error */
> > +	if (status & CEU_CEIER_VBP) {
> > +		/* soft reset */
> > +		ceu_write(pcdev, CAPSR, 1 << 16);
> > +		while ((1 << 16) & ceu_read(pcdev, CAPSR))
> > +			cpu_relax();
> > +		while (ceu_read(pcdev, CSTSR) & 1)
> > +			cpu_relax();
> 
> Let's put some security in these loops - please, add some counter to them. 
> Even more importantly, since these loops are also run in IRQ.

OK.
But there are part where run soft reset.
So, I will send another patch which add/use soft_reset function.

> > +		ret = -1;
> 
> I see, that this ret value is only used internally, still, I think it 
> would look better with some -E... errno code.

OK. I could understand.

> The IRQ handler will exit immediately, if there is no active video buffer, 
> without entering sh_mobile_ceu_capture(). Are we sure, that this VBP 
> condition cannot happen then?

VBP error will happen 
when VD has been input while the CEU holds data.

And my patch check VBP error even if there is no active video buffer.

or am I misunderstanding ?

> sh_mobile_ceu_capture() is also called from 
> sh_mobile_ceu_videobuf_queue(), maybe just add a small comment there 
> explaining, that we're not interested in the return code here, since there 
> were no active buffer at that moment.

OK

> Last question - do you agree to queue this patch (after you address my 
> comments) for 2.6.32 or would you prefer to get it in for 2.6.31 as a 
> bug-fix?

I can agree to queue 2.6.32.
Thank you

Best regards
--
Kuninori Morimoto

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request <at> redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list