diff mbox

Re: rtc-ds1307: Remove rtc_valid_tm() from ds1307_get_time()

Message ID 49708FE6.2000903@visionsystems.de
State Rejected, archived
Headers show

Commit Message

Yegor Yefremov Jan. 16, 2009, 1:47 p.m. UTC 
Alessandro Zummo schrieb:
> On Fri, 16 Jan 2009 11:16:39 +0100
> Yegor Yefremov <yegor_sub1@visionsystems.de> wrote:
>
>   
>> The script will set the date to default value. But the problem with 
>> rtc_hctosys() still remains. If the value returned from the RTC driver 
>> (those returning rtc_valid_tm() and not 0 if there were no communication 
>> errors) is invalid you'll see the following error message:
>>
>> "hctosys: unable to read the hardware clock\n"
>>     
>
>  I will probably change this message to something different.
>  
>   
>> because err != 0 and you don't reach the rtc_valid_tm() in rtc_hctosys() 
>> itself. This is only functioning if read call returns 0, then 
>> rtc_valid_tm() will be called and provided the value is invalid you get 
>> the right message:
>>
>> "hctosys: invalid date/time\n"
>>     
>
>  that's a fallback to cope with badly written drivers.
>  
>   
>> So in the case of ds1307 you cannot rely on the error message. And if it 
>> is the desired behavior, that the value has to be checked by all 
>> drivers, why not doing so on the central place in 
>> drivers/rtc/interface.c->rtc_read_time()? (see interface.diff)
>>     
>
>  It's done that way in order to make driver authors conscious
>  on what they are doing.
>
>   
Summary of this discussion:

1 .the rtc_hctosys() should give the correct information concerning what 
was wrong while getting the time. I've attached another patch. It just 
parses the return code, so the old behavior for drivers not returning 
rtc_valid_tm() is still the same. Please check it.

2. rtc_valid_tm() should be some day placed on some central place 
(drivers/rtc/interface.c) and removed from the drivers. This means a lot 
of work and should be confirmed/voted.

Jüri, I hope I could answer your question too.

Best regards,
Yegor

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---

Comments

Alessandro Zummo Jan. 16, 2009, 2:08 p.m. UTC | #1 
On Fri, 16 Jan 2009 14:47:18 +0100
Yegor Yefremov <yegor_sub1@visionsystems.de> wrote:

> 
> 1 .the rtc_hctosys() should give the correct information concerning what 
> was wrong while getting the time. I've attached another patch. It just 
> parses the return code, so the old behavior for drivers not returning 
> rtc_valid_tm() is still the same. Please check it.

 Thanks, but I already have something to that effect in my tree. It will
 be pushed to -mm in the weekend.
 
> 2. rtc_valid_tm() should be some day placed on some central place 
> (drivers/rtc/interface.c) and removed from the drivers. This means a lot 
> of work and should be confirmed/voted.
> 
> Jüri, I hope I could answer your question too.

 rtc_valid_tm is perfect where's now. It's done this way to remember
 the driver authors to act responsibly . In the past we had a lot of
 problems with drivers altering/fixing the date on read, which is plain
 wrong. rtc_valid_tm means "yes, I have read the date from the clock and
 I trust it's correct. If it's not, I'm giving you -EINVAL."

--

 Your Faithfully,
   The RTC Subsystem Maintainer


--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---
David Brownell Jan. 17, 2009, 10:48 p.m. UTC | #2 
On Friday 16 January 2009, Alessandro Zummo wrote:
> > 2. rtc_valid_tm() should be some day placed on some central place 
> > (drivers/rtc/interface.c) and removed from the drivers. This means a lot 
> > of work and should be confirmed/voted.
> > 
> > Jüri, I hope I could answer your question too.
> 
>  rtc_valid_tm is perfect where's now. It's done this way to remember
>  the driver authors to act responsibly . In the past we had a lot of
>  problems with drivers altering/fixing the date on read, which is plain
>  wrong. rtc_valid_tm means "yes, I have read the date from the clock and
>  I trust it's correct. If it's not, I'm giving you -EINVAL."

My two cents:  I'd be happy to see this more centrally located.
Less work for the drivers; and more consistency.

We've had a lot of driver bugs in the past, which have been
fixed.  If some RTC driver hacks the value returned by the
hardwdare for any reason other than handling a well defined
silicon erratum, we can just keep such code out of mainline.

- Dave

--~--~---------~--~----~------------~-------~--~----~
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
-~----------~----~----~----~------~----~------~--~---
diff mbox

Patch

diff --git a/drivers/rtc/hctosys.c b/drivers/rtc/hctosys.c
index 33c0e98..3e4c98d 100644
--- a/drivers/rtc/hctosys.c
+++ b/drivers/rtc/hctosys.c
@@ -57,9 +57,14 @@  static int __init rtc_hctosys(void)
 			dev_err(rtc->dev.parent,
 				"hctosys: invalid date/time\n");
 	}
-	else
+	else if(err == -EINVAL){
+		dev_err(rtc->dev.parent,
+				"hctosys: invalid date/time\n");
+	}
+	else{
 		dev_err(rtc->dev.parent,
 			"hctosys: unable to read the hardware clock\n");
+	}
 
 	rtc_class_close(rtc);