On Sunday 21 June 2009, Nori, Sekhar wrote:
> On Sun, Jun 21, 2009 at 20:16:19, Paulraj, Sandeep wrote:
> > From: Sandeep Paulraj <[email protected]>
> >
> > Patch adds support for the RTC Driver in DM365.
>
> The MAINTAINERS file mentions the RTC Subsystem ML as
> [email protected]. This probably will have to get posted there as
> well.
After another round or two, sure. I'd make sure
it passes "rtctest" first.
> > +#define DM365_RTCIF_PID_REG 0x00
That comment about these registers being IP-specfic would
seem to apply to all the registers and the driver name, yes?
So maybe the name should be "davinci_rtc" and the
driver-internal register and function names can be less
painfully long ... just RTC_PID_REG or somesuch.
> > +#define rtcif_read(addr) __raw_readl( \
> > + (unsigned int *)((u32)dm365_rtc_base + (u32)(addr)))
> > +#define rtcif_write(val, addr) __raw_writel(val, \
> > + (unsigned int *)((u32)dm365_rtc_base + (u32)(addr)))
>
> I have seen maintainers asking to use static inline functions instead of
> macros for register accessors.
Absolutely. Macros with params -- avoid.
> > +static irqreturn_t dm365_rtc_isr(int irq, void *class_dev)
>
> Can we drop the dm365_ prefix for the static functions? These functions
> should be applicable on the next platform that uses this IP and being
> static, its easy to drop the prefix. The comment also applies to some of the
> static variables declared earlier in the file.
Keep *some* prefix though, like davinci_rtc_isr() or somesuch.
The "rtc_" prefix is very generic, and will sometimes show up
in stack backtraces. Plus, it's not clear there won't be more
than one RTC on a system. (It's likely that a discrete RTC
that's optimized for low power could use less power than the
PRTSS module ... that tradeoff could matter.)
>
> > +
> > +static int
> > +dm365_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> > +{
> > + u8 rtc_ctrl;
> > + unsigned long flags;
> > +
> > + switch (cmd) {
> > + case RTC_AIE_OFF:
> > + case RTC_AIE_ON:
> > + case RTC_UIE_OFF:
> > + case RTC_UIE_ON:
> > + case RTC_IRQP_READ:
> > + case RTC_IRQP_SET:
> > + case RTC_PIE_OFF:
> > + case RTC_PIE_ON:
> > + case RTC_WIE_ON:
> > + case RTC_WIE_OFF:
> > + break;
> > + default:
> > + return -ENOIOCTLCMD;
> > + }
>
> Do you really require this switch? Why not just use the one below?
Almost all of them should be handled by RTC methods,
instead of through ioctl().
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
