Messages in this thread | | | Subject | Re: [PATCH] PCAP RTC driver (for 2.6.32). | From | Daniel Ribeiro <> | Date | Fri, 26 Jun 2009 20:57:38 -0300 |
| |
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] | |