Re: [PATCH] fix RTC_CLASS regression with PARISC

[David Brownell <david-b@pacbell.net>]

On Monday 08 September 2008, David Miller wrote:
> > > > >     struct rtc_device *rtc = rtc_class_open("rtc0");
> > 
> > One more point:  that should probably use CONFIG_RTC_HCTOSYS_DEVICE
> > instead of hard-wiring to "rtc0".  Yeah, I'm sure your SPARCs have
> > lots of RTCs to choose from -- not! -- but I'd like to see you end
> > up with code that many folk can reuse/recycle/pirate.  ;)
> 
> Can you be more specific?  Oh, you want me to use the string defined
> by that config option.  Ok :-)
> 
> But as far as I can tell this will only be set of RTC_HCTOSYS and
> users currently are allowed to not set that.
> 
> If this code goes somewhere generic you would need to ifdef test on
> that, depending upon where you'd want to put it and how it would
> be provided generically.

OK.


> > > > I'd be tempted to cache that ... notice how you never
> > > > close it, too.  That will goof lots of refcounts...
> > > 
> > > Well if I cache it then we'll hold it forever and that's not
> > > so nice right?
> > 
> > Why wouldn't it be, so long as it's eventually closed
> > to prevent leakage?  Other code can rtc_class_open() too;
> > unlike a userspace open("/dev/rtc0", ...) this isn't an
> > exclusive operation.
> 
> When would be "eventually closed" if I open it here and remember
> the pointer in a static local variable, and don't close it?
> 
> I guess you need to be more specific about what you mean by
> caching :)

I'll translate that as "-ENOPATCH".  :)

It'd suffice to fire a timer every 15 minutes or so, and
close it if the NTP logic hasn't refreshed the clock since
last time.  You're right that the simplest scheme is to just
open/close on each call.  The extra work is so infrequent it
may not be measurable.


> > If you're concerned about stuff like "rmmod my-i2c-rtc-driver"
> > losing (or "rmmod my-i2c-rtc-driver's-i2c-adapter") ... what's
> > supposed to happen is that you start getting an -ENODEV return
> > from your rtc_set_mmss() call, and then you close and null your
> > cached handle to free up its memory.
> 
> I see... god that's ugly.  If you want to do this in the generic
> RTC layer helper routines,

... when they get created ...


> 	that's fine, but I don't feel like 
> adding all sorts of stuff like that to the sparc specific routine
> at the moment.
> 
> I'm trying to do things that are practical and that I can check
> into sparc-next-2.6 right now.

OK by me.