Re: [PATCH v2] rtc: Add NXP PCF8523 support

[Thierry Reding]

On Wed, Nov 28, 2012 at 03:31:58PM -0800, Andrew Morton wrote:
> On Wed, 28 Nov 2012 20:21:26 +0100
> Thierry Reding <thierry.reding@xxxxxxxxxxxxxxxxx> wrote:
> 
> > +	err = i2c_transfer(client->adapter, &msg, 1);
> > +	if (err < 0) {
> > +		/*
> > +		 * If the time cannot be set, restart the RTC anyway. Note
> > +		 * that errors are ignored if the RTC cannot be started so
> > +		 * that we have a chance to propagate the original error.
> > +		 */
> > +		pcf8523_start_rtc(client);
> > +		return err;
> > +	}
> > +
> > +	return pcf8523_start_rtc(client);
> 
> hm, well, that is of course equivalent to
> 
> 	err = i2c_transfer(client->adapter, &msg, 1);
> 	pcf8523_start_rtc(client);
> 	return err;

At the risk of sounding pedantic, it isn't quite the same thing. There
is a possibility that i2c_transfer() succeeds but pcf8523_start_rtc()
still fails. So in fact we catch errors in i2c_transfer() and from
starting the RTC in case the time was properly set. The only error we
might loose if if the RTC fails to start after the time cannot be set
but in that case there's probably not a whole lot we can do.

> but I suppose the code as proposed is clear, extensible, and not our
> worst-ever sin ;)

Yes, this is one of the cases where the gain in clarity justifies the
extra verbosity. =)

Thierry

Attachment: pgp00000.pgp
Description: PGP signature