Re: [PATCH 000/100] rtc: remove cargo culted code

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 02/03/2018 at 08:46:48 +0000, Steve Twiss wrote:
> On Wed, Feb 21, 2018 at 8:54 PM, Alexandre Belloni wrote:
>  
> > subject:	[PATCH 000/100] rtc: remove cargo culted code
> > mailing list:	linux-kernel@vger.kernel.org Filter messages from this mailing list
> >
> > Hello,
> >
> > This series:
> >  - removes useless calls to rtc_valid_tm in .read_time, .set_time and
> >    .set_alarm
> >  - removes code setting default values for RTCs (and lets the core
> >    handle it)
> >  - removes useless "time is invalid" messages at probe time
> >  - removes useless indirect calls
> >
> > Those were mostly copy pasted from other drivers
> 
> Hi Alexandre,
> 
> Acked for:
>   rtc: da9063: stop validating rtc_time in .read_time
>   rtc: da9052: stop validating rtc_time in .read_time
>   rtc: da9055: stop validating rtc_time in .read_time
> 
> Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>
> 
> Agreed -- rtc_valid_tm() call is cargo cult for the above.
> 
> (By definition) for DA9063 I was trying to be rigorous.
> The .read_time function is slightly different here because I can make a copy the alarm time into the RTC time
> structure to solve an RTC synchronisation problem internally to the DA9063.
> https://elixir.bootlin.com/linux/v4.5.6/source/drivers/rtc/rtc-da9063.c#L253
> 
> But after some further looking, I have not got any explicit case of how the time read directly from the DA9063
> registers can be incorrectly represented. So there should be no need to check this.
> 

My point is that it is checked later in the core anyway so you end up
doing:

da9063_rtc_read_time()
	return rtc_valid_tm(tm);

__rtc_read_time()
	if (err < 0)
		return err;
	err = rtc_valid_tm(tm);

	return err;

So the check in da9063_rtc_read_time is always pointless.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com