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. [...] > diff --git a/drivers/rtc/rtc-dm365.c b/drivers/rtc/rtc-dm365.c > new file mode 100644 [...] > + > +#include <linux/kernel.h> > +#include <linux/init.h> > +#include <linux/module.h> > +#include <linux/ioport.h> > +#include <linux/delay.h> > +#include <linux/spinlock.h> > +#include <linux/rtc.h> > +#include <linux/bcd.h> > +#include <linux/platform_device.h> > +#include <linux/io.h> > + > +#define DM365_RTC_BASE 0x01c69000 Probably unused. Please remove. > + > +/* > + * The DM365 RTC is a simple RTC with the following > + * Sec: 0 - 59 : BCD count > + * Min: 0 - 59 : BCD count > + * Hour: 0 - 23 : BCD count > + * Day: 0 - 0x7FFF(32767) : Binary count ( Over 89 years ) > + */ > + > +#define DM365_RTCIF_PID_REG 0x00 > +#define DM365_RTCIF_DMA_CMD_REG 0x04 > +#define DM365_RTCIF_DMA_DATA0_REG 0x08 > +#define DM365_RTCIF_DMA_DATA1_REG 0x0C > +#define DM365_RTCIF_INT_ENA_REG 0x10 > +#define DM365_RTCIF_INT_FLG_REG 0x14 > + > +/* DM365_RTCIF_DMA_CMD_REG bit fields */ > +#define DM365_RTCIF_DMA_CMD_BUSY (1<<31) > +#define DM365_RTCIF_DMA_CMD_SIZE_2WORD (1<<25) > +#define DM365_RTCIF_DMA_CMD_DIR_READ (1<<24) > +#define DM365_RTCIF_DMA_CMD_BYTEENA1_LSB (1<<20) > +#define DM365_RTCIF_DMA_CMD_BYTEENA1_2ND_LSB (1<<21) > +#define DM365_RTCIF_DMA_CMD_BYTEENA1_3RD_LSB (1<<22) > +#define DM365_RTCIF_DMA_CMD_BYTEENA1_MSB (1<<23) > +#define DM365_RTCIF_DMA_CMD_BYTEENA1_MASK (0x00F00000) > +#define DM365_RTCIF_DMA_CMD_BYTEENA0_LSB (1<<16) > +#define DM365_RTCIF_DMA_CMD_BYTEENA0_2ND_LSB (1<<17) > +#define DM365_RTCIF_DMA_CMD_BYTEENA0_3RD_LSB (1<<18) > +#define DM365_RTCIF_DMA_CMD_BYTEENA0_MSB (1<<19) Should use BIT(x) for these.. > +#define DM365_RTCIF_DMA_CMD_BYTEENA0_MASK (0x000F0000) > + > +#define DM365_RTCIF_INT_ENA_RTCSS_INTENA (1<<1) > +#define DM365_RTCIF_INT_ENA_RTCIF_INTENA (1<<0) > +#define DM365_RTCIF_INT_FLG_RTCSS_INTFLG (1<<1) > +#define DM365_RTCIF_INT_FLG_RTCIF_INTFLG (1<<0) ..and these and some below as well. > + > +#define DM365_RTCIF_INT_FLG_MASK (DM365_RTCIF_INT_FLG_RTCSS_INTFLG \ > + | DM365_RTCIF_INT_FLG_RTCIF_INTFLG) I guess these # definitions are not really DM365 specific but specific to RTC IP. In this case, you should drop the DM365_ prefix. > + > +/* RTC registers */ > +#define RTCSS_RTC_BASE 0x10 > +#define RTCSS_RTC_CTRL_REG (RTCSS_RTC_BASE + 0x00) > +#define RTCSS_RTC_WDT_REG (RTCSS_RTC_BASE + 0x01) > +#define RTCSS_RTC_TMR0_REG (RTCSS_RTC_BASE + 0x02) > +#define RTCSS_RTC_TMR1_REG (RTCSS_RTC_BASE + 0x03) > +#define RTCSS_RTC_CCTRL_REG (RTCSS_RTC_BASE + 0x04) > +#define RTCSS_RTC_SEC_REG (RTCSS_RTC_BASE + 0x05) > +#define RTCSS_RTC_MIN_REG (RTCSS_RTC_BASE + 0x06) > +#define RTCSS_RTC_HOUR_REG (RTCSS_RTC_BASE + 0x07) > +#define RTCSS_RTC_DAY0_REG (RTCSS_RTC_BASE + 0x08) > +#define RTCSS_RTC_DAY1_REG (RTCSS_RTC_BASE + 0x09) > +#define RTCSS_RTC_ALARM_MIN_REG (RTCSS_RTC_BASE + 0x0A) > +#define RTCSS_RTC_ALARM_HOUR_REG (RTCSS_RTC_BASE + 0x0B) > +#define RTCSS_RTC_ALARM_DAY0_REG (RTCSS_RTC_BASE + 0x0C) > +#define RTCSS_RTC_ALARM_DAY1_REG (RTCSS_RTC_BASE + 0x0D) > +#define RTCSS_RTC_CLKC_CNT (RTCSS_RTC_BASE + 0x10) > + > +/* RTCSS_RTC_CTRL_REG bit fields: */ > +#define RTCSS_RTC_CTRL_WDTBUS (1<<7) > +#define RTCSS_RTC_CTRL_WEN (1<<6) > +#define RTCSS_RTC_CTRL_WDRT (1<<5) > +#define RTCSS_RTC_CTRL_WDTFLG (1<<4) > +#define RTCSS_RTC_CTRL_TE (1<<3) > +#define RTCSS_RTC_CTRL_TIEN (1<<2) > +#define RTCSS_RTC_CTRL_TMRFLG (1<<1) > +#define RTCSS_RTC_CTRL_TMMD_FREERUN (1<<1) > + > +/* RTCSS_RTC_CCTRL_REG bit fields: */ > +#define RTCSS_RTC_CCTRL_CALBUSY (1<<7) > +#define RTCSS_RTC_CCTRL_DAEN (1<<5) > +#define RTCSS_RTC_CCTRL_HAEN (1<<4) > +#define RTCSS_RTC_CCTRL_MAEN (1<<3) > +#define RTCSS_RTC_CCTRL_ALMFLG (1<<2) > +#define RTCSS_RTC_CCTRL_AIEN (1<<1) > +#define RTCSS_RTC_CCTRL_CAEN (1<<0) > + > +#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. > + > +static DEFINE_SPINLOCK(dm365_rtc_lock); > + > +static void __iomem *dm365_rtc_base; > +static resource_size_t dm365_rtc_pbase; > +static size_t dm365_rtc_base_size; > +static int dm365_rtc_irq; > + > +static void rtcss_write_rtc(unsigned long val, u8 addr) > +{ > + unsigned int cmd; > + > + while > + (rtcif_read(DM365_RTCIF_DMA_CMD_REG) & > + DM365_RTCIF_DMA_CMD_BUSY); Having only 'while' on a line looks little weird :) [...] > + > +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. > +{ > + unsigned long events = 0; > + u32 irq_flg; > + > + irq_flg = rtcif_read(DM365_RTCIF_INT_FLG_REG); > + > + if ((irq_flg & DM365_RTCIF_INT_FLG_MASK)) { > + if (irq_flg & DM365_RTCIF_INT_FLG_RTCIF_INTFLG) { > + rtcif_write(DM365_RTCIF_INT_ENA_RTCSS_INTENA | > + DM365_RTCIF_INT_ENA_RTCIF_INTENA, > + DM365_RTCIF_INT_FLG_REG); > + } else > + events |= RTC_IRQF | RTC_AF; > + > + rtc_update_irq(class_dev, 1, events); > + return IRQ_HANDLED; > + } > + return IRQ_NONE; I think a single return path and using goto for the failure case handling would be preferable. > + > +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? > + > + spin_lock_irqsave(&dm365_rtc_lock, flags); > + rtc_ctrl = rtcss_read_rtc(RTCSS_RTC_CCTRL_REG); > + > + switch (cmd) { > + case RTC_AIE_OFF: > + rtc_ctrl &= ~RTCSS_RTC_CCTRL_AIEN; > + break; > + case RTC_AIE_ON: > + rtc_ctrl |= RTCSS_RTC_CCTRL_AIEN; > + break; > + case RTC_WIE_ON: > + rtc_ctrl |= (RTCSS_RTC_CTRL_WEN | RTCSS_RTC_CTRL_WDTFLG); > + break; > + case RTC_WIE_OFF: > + rtc_ctrl &= ~RTCSS_RTC_CTRL_WEN; > + break; > + case RTC_UIE_OFF: > + case RTC_UIE_ON: > + dm365_rtc_update_timer(cmd); > + } > + > + rtcss_write_rtc(rtc_ctrl, RTCSS_RTC_CTRL_REG); > + spin_unlock_irqrestore(&dm365_rtc_lock, flags); > + > + return 0; > +} > + > +static int convertfromdays(u16 days, struct rtc_time *tm) > +{ > + int tmp_days, year, mon; > + > + for (year = 2000;; year++) { > + tmp_days = rtc_year_days(1, 12, year); > + if (days >= tmp_days) > + days -= tmp_days; > + else { > + for (mon = 0;; mon++) { > + tmp_days = rtc_month_days(mon, year); > + if (days >= tmp_days) { > + days -= tmp_days; > + } else { > + tm->tm_year = year - 1900; > + tm->tm_mon = mon; > + tm->tm_mday = days + 1; > + break; > + } > + } > + break; > + } > + } > + return 0; > +} > + > +static int convert2days(u16 *days, struct rtc_time *tm) > +{ > + int i; > + > + *days = 0; > + > + /* epoch == 1900 */ > + if (tm->tm_year < 100 || tm->tm_year > 199) > + return -EINVAL; > + > + for (i = 2000; i < 1900 + tm->tm_year; i++) > + *days += rtc_year_days(1, 12, i); > + > + *days += rtc_year_days(tm->tm_mday, tm->tm_mon, 1900 + tm->tm_year); > + > + return 0; > +} Hmm. These two look like really generic functions. Wonder if the RTC subsystem does not give you this functionality. Thanks, Sekhar _______________________________________________ Davinci-linux-open-source mailing list [email protected] http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
