lkml.org 
[lkml]   [2009]   [Jun]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] PCAP RTC driver (for 2.6.32).
From
Date
Hi Andrew,

Em Sex, 2009-06-26 às 13:23 -0700, Andrew Morton escreveu:
> This could be coded more simply:
>
> unsigned long rtc_events;
>
> if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_1HZ))
> rtc_events = RTC_IRQF | RTC_UF;
> else if (irq == pcap_to_irq(pcap_rtc->pcap, PCAP_IRQ_TODA))
> rtc_events = RTC_IRQF | RTC_AF;

This would make gcc complain about rtc_events being used uninitialized.

> SEC_PER_DAY is defined in include/linux/mfd/ezx-pcap.h. It should not
> be, because it is not specific to that driver and can be used elsewhere
> in the kernel.
>
> I'd suggest that we
>
> - define SECS_PER_DAY in include/linux/time.h
>
> - remove the private definitions of SECS_PER_DAY from
> arch/m68k/mac/misc, arch/parisc/include/asm/rtc.h,
> arch/ia64/hp/sim/boot/fw-emu.c, fs/udf/udftime.c, fs/fat/misc.c and
> wherever else it appears, make those files use the common definition
>
> - migrate rtc-pcap.c from SEC_PER_DAY over to the common SECS_PER_DAY.
>
> - Then do it all again for SECS_PER_MIN and SECS_PER_HOUR.
>
>
> What a mess.

I can't work on this right now, but i will be happy to reserve some
hours for this next month if nobody else does it before.

> "tmp" is a poor identifier. We had an opportunity here to use an
> identifier which would communicate useful information to the reader.
> But we blew it and used the information-free "tmp" instead.
>
> Something like
>
> u32 time_of_day; /* In seconds since midnight */
>
> would be nice. If that is indeed what the variable contains. How
> would I know? It's called 'tmp" and is undescribed!

Hum.. I used tmp because the variable is reused and it may be "days
since 1/1/1970" or "seconds since today's 00:00:00". I will add some
comments to make this clear.

> > +static inline int pcap_rtc_irq_enable(struct device *dev, int pirq,
> > + unsigned int en)
> > +{
> Inlining this function probably made the code larger and slower.

Right, i will amend this.


I see that you have already added it to the -mm tree. Do you prefer an
incremental patch to address your comments or should I send a patch to
replace the current version?

Thanks for the review! :)

--
Daniel Ribeiro
[unhandled content-type:application/pgp-signature]
\
 
 \ /
  Last update: 2009-06-27 02:01    [W:0.273 / U:0.072 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site