[PATCH 2/6] mfd: mc13xxx-core: ADC conv: wait_for_completion returns a long

[Uwe Kleine-König]

Hello again,

On Tue, Jan 31, 2012 at 09:07:59AM +0100, Uwe Kleine-König wrote:
> > > > -	if (!ret)
> > > > +	if (timeout <= 0) {
> > > > +		dev_warn(mc13xxx->dev,
> > > > +				"timed out waiting for ADC completion\n");
> > > > 
> > > >  		ret = -ETIMEDOUT;
> > > > 
> > > > +	}
> > > 
> > > I think this is wrong. wait_for_completion_interruptible_timeout returns
> > > -ERESTARTSYS if it was interrupted. That's not a timeout and
> > > -ERESTARTSYS should be propagated then. !ret is the correct test for
> > > timeout.
> > 
> > It took me a little while to get your point here, and I guess I missed that in 
> > my original understanding of the code, (which may be more of a reflection on 
> > me :) )
> >  
> > I still think the way it was before is subtle, and would prefer something more 
> > explicit, perhaps:
> > 
> > if (timeout == 0)
> > 	ret = -ETIMEDOUT;
> > else if (timeout < 0)
> > 	ret = timeout;
> Yeah, that's better than the original as it propagates an eventual
> -ERESTARTSYS from wait_for_completion_interruptible_timeout. Don't know
> if/how the upper layer handle that though.
Just a small note, after starring again at the original code,
-ERESTARTSYS is propagated correctly. So it's only about style.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |