<[email protected]> writes:

> From: Miguel Aguilar <[email protected]>
>
> This driver features:
>
> * Alarm support.
> * Periodic interrupt by using a timer include into the RTC module.
> * The update interrupt is not supported by this RTC module.
>
> This driver was tested on a DM365 EVM by using the rtc-test application
> from the Documentation/rtc.txt.
>
> Signed-off-by: Miguel Aguilar <[email protected]>

checkpatch shows:

  total: 30 errors, 9 warnings, 694 lines checked

Please run through checkpatch and fix errors and warnings.  

Some more comments below...

> ---
>  drivers/rtc/Kconfig       |   10 +
>  drivers/rtc/Makefile      |    1 +
>  drivers/rtc/rtc-davinci.c |  671 
> +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 682 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/rtc/rtc-davinci.c
>
> diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig
> index 3c20dae..61bd4d6 100644
> --- a/drivers/rtc/Kconfig
> +++ b/drivers/rtc/Kconfig
> @@ -576,6 +576,16 @@ config RTC_DRV_AB3100
>  
>  comment "on-CPU RTC drivers"
>  
> +config RTC_DRV_DAVINCI
> +     tristate "TI DaVinci RTC"
> +     depends on ARCH_DAVINCI_DM365
> +     help
> +       If you say yes here you get support for the RTC on the
> +       DaVinci platforms (DM365).
> +
> +       This driver can also be built as a module. If so, the module
> +       will be called rtc-davinci.
> +
>  config RTC_DRV_OMAP
>       tristate "TI OMAP1"
>       depends on ARCH_OMAP15XX || ARCH_OMAP16XX || ARCH_OMAP730
> diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile
> index aa3fbd5..d968694 100644
> --- a/drivers/rtc/Makefile
> +++ b/drivers/rtc/Makefile
> @@ -26,6 +26,7 @@ obj-$(CONFIG_RTC_DRV_BFIN)  += rtc-bfin.o
>  obj-$(CONFIG_RTC_DRV_BQ4802) += rtc-bq4802.o
>  obj-$(CONFIG_RTC_DRV_CMOS)   += rtc-cmos.o
>  obj-$(CONFIG_RTC_DRV_COH901331)      += rtc-coh901331.o
> +obj-$(CONFIG_RTC_DRV_DAVINCI)        += rtc-davinci.o
>  obj-$(CONFIG_RTC_DRV_DM355EVM)       += rtc-dm355evm.o
>  obj-$(CONFIG_RTC_DRV_DS1216) += rtc-ds1216.o
>  obj-$(CONFIG_RTC_DRV_DS1286) += rtc-ds1286.o
> diff --git a/drivers/rtc/rtc-davinci.c b/drivers/rtc/rtc-davinci.c
> new file mode 100644
> index 0000000..d691a58
> --- /dev/null
> +++ b/drivers/rtc/rtc-davinci.c
> @@ -0,0 +1,671 @@
> +/*
> + * DaVinci Power Management and Real Time Clock Driver for TI platforms
> + *
> + * Copyright (C) 2009 Texas Instruments, Inc
> + *
> + * Author: Miguel Aguilar <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +#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

You can drop this define.  It looks like you're passing this in from
platform_data like you should be, so it's not needed here.

> +/*
> + * The DaVinci 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 )
> + */
> +
> +/* PRTC interface registers */
> +#define DAVINCI_PRTCIF_PID                   0x00
> +#define DAVINCI_PRTCIF_CTLR                  0x04
> +#define DAVINCI_PRTCIF_LDATA                 0x08
> +#define DAVINCI_PRTCIF_UDATA                 0x0C
> +#define DAVINCI_PRTCIF_INTEN                 0x10
> +#define DAVINCI_PRTCIF_INTFLG                        0x14
> +
> +/* DAVINCI_PRTCIF_CTLR bit fields */
> +#define DAVINCI_PRTCIF_CTLR_BUSY             (1<<31)
> +#define DAVINCI_PRTCIF_CTLR_SIZE             (1<<25)
> +#define DAVINCI_PRTCIF_CTLR_DIR                      (1<<24)
> +#define DAVINCI_PRTCIF_CTLR_BENU_MSB         (1<<23)
> +#define DAVINCI_PRTCIF_CTLR_BENU_3RD_BYTE    (1<<22)
> +#define DAVINCI_PRTCIF_CTLR_BENU_2ND_BYTE    (1<<21)
> +#define DAVINCI_PRTCIF_CTLR_BENU_LSB         (1<<20)
> +#define DAVINCI_PRTCIF_CTLR_BENU_MASK                (0x00F00000)
> +#define DAVINCI_PRTCIF_CTLR_BENL_MSB         (1<<19)
> +#define DAVINCI_PRTCIF_CTLR_BENL_3RD_BYTE    (1<<18)
> +#define DAVINCI_PRTCIF_CTLR_BENL_2ND_BYTE    (1<<17)
> +#define DAVINCI_PRTCIF_CTLR_BENL_LSB         (1<<16)
> +#define DAVINCI_PRTCIF_CTLR_BENL_MASK                (0x000F0000)

Please use BIT(n) for the (1<<n) #defines.

> +/* DAVINCI_PRTCIF_INTEN bit fields */
> +#define DAVINCI_PRTCIF_INTEN_RTCSS           (1<<1)
> +#define DAVINCI_PRTCIF_INTEN_RTCIF           (1<<0)
> +#define DAVINCI_PRTCIF_INTEN_MASK            (DAVINCI_PRTCIF_INTEN_RTCSS \
> +                                             | DAVINCI_PRTCIF_INTEN_RTCIF)
> +
> +/* DAVINCI_PRTCIF_INTFLG bit fields */
> +#define DAVINCI_PRTCIF_INTFLG_RTCSS          (1<<1)
> +#define DAVINCI_PRTCIF_INTFLG_RTCIF          (1<<0)
> +#define DAVINCI_PRTCIF_INTFLG_MASK           (DAVINCI_PRTCIF_INTFLG_RTCSS \
> +                                             | DAVINCI_PRTCIF_INTFLG_RTCIF)
> +
> +/* PRTC subsystem registers */
> +#define DAVINCI_PRTCSS_RTC_INTC_EXTENA1              (0x0C)
> +#define DAVINCI_PRTCSS_RTC_CTRL                      (0x10)
> +#define DAVINCI_PRTCSS_RTC_WDT                       (0x11)
> +#define DAVINCI_PRTCSS_RTC_TMR0                      (0x12)
> +#define DAVINCI_PRTCSS_RTC_TMR1                      (0x13)
> +#define DAVINCI_PRTCSS_RTC_CCTRL             (0x14)
> +#define DAVINCI_PRTCSS_RTC_SEC                       (0x15)
> +#define DAVINCI_PRTCSS_RTC_MIN                       (0x16)
> +#define DAVINCI_PRTCSS_RTC_HOUR                      (0x17)
> +#define DAVINCI_PRTCSS_RTC_DAY0                      (0x18)
> +#define DAVINCI_PRTCSS_RTC_DAY1                      (0x19)
> +#define DAVINCI_PRTCSS_RTC_AMIN                      (0x1A)
> +#define DAVINCI_PRTCSS_RTC_AHOUR             (0x1B)
> +#define DAVINCI_PRTCSS_RTC_ADAY0             (0x1C)
> +#define DAVINCI_PRTCSS_RTC_ADAY1             (0x1D)
> +#define DAVINCI_PRTCSS_RTC_CLKC_CNT          (0x20)
> +
> +/* DAVINCI_PRTCSS_RTC_INTC_EXTENA1 */
> +#define DAVINCI_PRTCSS_RTC_INTC_EXTENA1_MASK (0x07)
> +
> +/* DAVINCI_PRTCSS_RTC_CTRL bit fields */
> +#define DAVINCI_PRTCSS_RTC_CTRL_WDTBUS               (1<<7)
> +#define DAVINCI_PRTCSS_RTC_CTRL_WEN          (1<<6)
> +#define DAVINCI_PRTCSS_RTC_CTRL_WDRT         (1<<5)
> +#define DAVINCI_PRTCSS_RTC_CTRL_WDTFLG               (1<<4)
> +#define DAVINCI_PRTCSS_RTC_CTRL_TE           (1<<3)
> +#define DAVINCI_PRTCSS_RTC_CTRL_TIEN         (1<<2)
> +#define DAVINCI_PRTCSS_RTC_CTRL_TMRFLG               (1<<1)
> +#define DAVINCI_PRTCSS_RTC_CTRL_TMMD         (1<<0)
> +
> +/* DAVINCI_PRTCSS_RTC_CCTRL bit fields */
> +#define DAVINCI_PRTCSS_RTC_CCTRL_CALBUSY     (1<<7)
> +#define DAVINCI_PRTCSS_RTC_CCTRL_DAEN                (1<<5)
> +#define DAVINCI_PRTCSS_RTC_CCTRL_HAEN                (1<<4)
> +#define DAVINCI_PRTCSS_RTC_CCTRL_MAEN                (1<<3)
> +#define DAVINCI_PRTCSS_RTC_CCTRL_ALMFLG              (1<<2)
> +#define DAVINCI_PRTCSS_RTC_CCTRL_AIEN                (1<<1)
> +#define DAVINCI_PRTCSS_RTC_CCTRL_CAEN                (1<<0)

More BIT(n) needed here.

> +static DEFINE_SPINLOCK(davinci_rtc_lock);
> +
> +struct davinci_rtc {
> +     struct rtc_device               *rtc;
> +     void __iomem                    *base;
> +     resource_size_t                 pbase;
> +     size_t                          base_size;
> +     int                             irq;
> +} *davinci_rtc;

This is a global variable.  At a minimum this should be static, but
better would be to allocate it in probe and pass this around via
private data (drvdata) instead of using a global.  That way, this
driver could be used in case of multiple instances.

> +static void davinci_rtcif_write(u32 val, u32 addr)
> +{
> +     __raw_writel(val, davinci_rtc->base + addr);
> +}
> +
> +static u32 davinci_rtcif_read(u32 addr)
> +{
> +     return __raw_readl(davinci_rtc->base + addr);
> +}

Should be using the normal writel/readl function for ioremap'd memory
instead of the __raw versions.

These should also be inline.

> +static void davinci_rtcss_write(unsigned long val, u8 addr)
> +{
> +     while (davinci_rtcif_read(DAVINCI_PRTCIF_CTLR) &
> +           DAVINCI_PRTCIF_CTLR_BUSY);
> +
> +     davinci_rtcif_write(DAVINCI_PRTCIF_CTLR_BENL_LSB | addr,
> +                         DAVINCI_PRTCIF_CTLR);
> +     davinci_rtcif_write(val, DAVINCI_PRTCIF_LDATA);
> +
> +     while (davinci_rtcif_read(DAVINCI_PRTCIF_CTLR) &
> +            DAVINCI_PRTCIF_CTLR_BUSY);
> +}
> +
> +static u8 davinci_rtcss_read(u8 addr)
> +{
> +     while (davinci_rtcif_read(DAVINCI_PRTCIF_CTLR) &
> +            DAVINCI_PRTCIF_CTLR_BUSY);
> +
> +     davinci_rtcif_write(DAVINCI_PRTCIF_CTLR_DIR |
> +                         DAVINCI_PRTCIF_CTLR_BENL_LSB |
> +                         addr, DAVINCI_PRTCIF_CTLR);
> +
> +     while (davinci_rtcif_read(DAVINCI_PRTCIF_CTLR) &
> +           DAVINCI_PRTCIF_CTLR_BUSY);
> +
> +     return davinci_rtcif_read(DAVINCI_PRTCIF_LDATA);
> +}
> +
> +static irqreturn_t davinci_rtc_interrupt(int irq, void *class_dev)
> +{
> +     unsigned long events = 0;
> +     u32 irq_flg;
> +     u8 alm_irq, tmr_irq;
> +     u8 rtc_ctrl, rtc_cctrl;
> +     int ret = IRQ_NONE;
> +
> +     irq_flg = davinci_rtcif_read(DAVINCI_PRTCIF_INTFLG) &
> +               DAVINCI_PRTCIF_INTFLG_RTCSS;
> +
> +     alm_irq = davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CCTRL) &
> +               DAVINCI_PRTCSS_RTC_CCTRL_ALMFLG;
> +
> +     tmr_irq = davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CTRL) &
> +               DAVINCI_PRTCSS_RTC_CTRL_TMRFLG;
> +
> +     if (irq_flg) {
> +             if (alm_irq) {
> +                     events |= RTC_IRQF | RTC_AF;
> +                     rtc_cctrl = 
> davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CCTRL);
> +                     rtc_cctrl |=  DAVINCI_PRTCSS_RTC_CCTRL_ALMFLG;
> +                     davinci_rtcss_write(rtc_cctrl, 
> DAVINCI_PRTCSS_RTC_CCTRL);
> +             } else if (tmr_irq) {
> +                     events |= RTC_IRQF | RTC_PF;
> +                     rtc_ctrl = davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CTRL);
> +                     rtc_ctrl |=  DAVINCI_PRTCSS_RTC_CTRL_TMRFLG;
> +                     davinci_rtcss_write(rtc_ctrl, DAVINCI_PRTCSS_RTC_CTRL);
> +             }
> +
> +             davinci_rtcif_write(DAVINCI_PRTCIF_INTFLG_RTCSS,
> +                                 DAVINCI_PRTCIF_INTFLG);
> +             rtc_update_irq(class_dev, 1, events);
> +             ret = IRQ_HANDLED;
> +     }
> +
> +     return ret;
> +}
> +
> +static int
> +davinci_rtc_ioctl(struct device *dev, unsigned int cmd, unsigned long arg)
> +{
> +     u8 rtc_ctrl;
> +     unsigned long flags;
> +     int ret = 0;
> +
> +     spin_lock_irqsave(&davinci_rtc_lock, flags);
> +
> +     rtc_ctrl = davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CTRL);
> +
> +     switch (cmd) {
> +             case RTC_WIE_ON:
> +                     rtc_ctrl |= (DAVINCI_PRTCSS_RTC_CTRL_WEN |
> +                                 DAVINCI_PRTCSS_RTC_CTRL_WDTFLG);
> +                     break;
> +             case RTC_WIE_OFF:
> +                     rtc_ctrl &= ~DAVINCI_PRTCSS_RTC_CTRL_WEN;
> +                     break;
> +             case RTC_UIE_OFF:
> +             case RTC_UIE_ON:
> +                     ret = -ENOTTY;
> +                     break;
> +             default:
> +                     ret = -ENOIOCTLCMD;
> +     }
> +
> +     davinci_rtcss_write(rtc_ctrl, DAVINCI_PRTCSS_RTC_CTRL);
> +
> +     spin_unlock_irqrestore(&davinci_rtc_lock, flags);
> +
> +     return ret;
> +}
> +
> +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;
> +}
> +
> +static int davinci_rtc_read_time(struct device *dev, struct rtc_time *tm)
> +{
> +     u16 days = 0;
> +     u8 day0, day1;
> +     unsigned long flags;
> +
> +     spin_lock_irqsave(&davinci_rtc_lock, flags);
> +
> +     while (davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CCTRL) &
> +                 DAVINCI_PRTCSS_RTC_CCTRL_CALBUSY);
> +     tm->tm_sec = bcd2bin(davinci_rtcss_read(DAVINCI_PRTCSS_RTC_SEC));
> +
> +     while (davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CCTRL) &
> +                 DAVINCI_PRTCSS_RTC_CCTRL_CALBUSY);
> +     tm->tm_min = bcd2bin(davinci_rtcss_read(DAVINCI_PRTCSS_RTC_MIN));
> +
> +     while (davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CCTRL) &
> +                 DAVINCI_PRTCSS_RTC_CCTRL_CALBUSY);
> +     tm->tm_hour = bcd2bin(davinci_rtcss_read(DAVINCI_PRTCSS_RTC_HOUR));
> +
> +     while (davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CCTRL) &
> +                 DAVINCI_PRTCSS_RTC_CCTRL_CALBUSY);
> +     day0 = davinci_rtcss_read(DAVINCI_PRTCSS_RTC_DAY0);
> +
> +     while (davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CCTRL) &
> +                 DAVINCI_PRTCSS_RTC_CCTRL_CALBUSY);
> +     day1 = davinci_rtcss_read(DAVINCI_PRTCSS_RTC_DAY1);
> +
> +     spin_unlock_irqrestore(&davinci_rtc_lock, flags);
> +
> +     days |= day1;
> +     days <<= 8;
> +     days |= day0;
> +
> +     if (convertfromdays(days, tm) < 0)
> +             return -EINVAL;
> +
> +     return 0;
> +}
> +
> +static int davinci_rtc_set_time(struct device *dev, struct rtc_time *tm)
> +{
> +     u16 days;
> +     u8 rtc_cctrl;
> +     unsigned long flags;
> +
> +     if (convert2days(&days, tm) < 0)
> +             return -EINVAL;
> +
> +     spin_lock_irqsave(&davinci_rtc_lock, flags);
> +
> +     while (davinci_rtcss_read(DAVINCI_PRTCSS_RTC_CCTRL) &
> +                 DAVINCI_PRTCSS_RTC_CCTRL_CALBUSY);

As reported by checkpatch, the empty body of the polling loop should
be on the next line, this helps readability tremendously.  Also, not
sure how many loops are expected here but rather than a
fast-as-possible poll, they should probably use cpu_relax().

Lots more of these in the rest of the driver.

Kevin


_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source

Reply via email to