Re: [PATCH 2/2] [MIPS] add DECstation DS1287 clockevent

[Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>]

Hi,

On Wed, 23 Apr 2008 11:55:28 +0900 (JST)
Atsushi Nemoto <anemo@mba.ocn.ne.jp> wrote:

> On Wed, 23 Apr 2008 08:52:45 +0900, Yoichi Yuasa 
> <yoichi_yuasa@tripeaks.co.jp> wrote:
> > add DECstation DS1287 clockevent
> > 
> > Signed-off-by: Yoichi Yuasa <yoichi_yuasa@tripeaks.co.jp>
> ...
> > --- linux-orig/arch/mips/kernel/Makefile    2008-04-22 19:01:43.957134178 
> > +0900
> > +++ linux/arch/mips/kernel/Makefile 2008-04-22 18:03:05.427649422 +0900
> > @@ -9,8 +9,9 @@ obj-y               += cpu-probe.o branch.o entry.o g
> >                time.o topology.o traps.o unaligned.o
> >  
> >  obj-$(CONFIG_CEVT_BCM1480) += cevt-bcm1480.o
> > -obj-$(CONFIG_CEVT_R4K)             += cevt-r4k.o
> > +obj-$(CONFIG_CEVT_DS1287)  += cevt-ds1287.o
> >  obj-$(CONFIG_CEVT_GT641XX) += cevt-gt641xx.o
> > +obj-$(CONFIG_CEVT_R4K)             += cevt-r4k.o
> >  obj-$(CONFIG_CEVT_SB1250)  += cevt-sb1250.o
> >  obj-$(CONFIG_CEVT_TXX9)            += cevt-txx9.o
> >  obj-$(CONFIG_CSRC_BCM1480) += csrc-bcm1480.o
> 
> Why CONFIG_CEVT_R4K line was moved?  The order is important?

Just sorted.
It's not important.

> 
> > --- linux-orig/arch/mips/kernel/cevt-ds1287.c       1970-01-01 
> > 09:00:00.000000000 +0900
> > +++ linux/arch/mips/kernel/cevt-ds1287.c    2008-04-22 18:03:05.455637018 
> > +0900
> ...
> > +static void ds1287_set_mode(enum clock_event_mode mode,
> > +                       struct clock_event_device *evt)
> > +{
> > +   unsigned long flags;
> > +   u8 val;
> > +
> > +   spin_lock_irqsave(&rtc_lock, flags);
> 
> You do not have to use irqsave here, while set_mode is always called
> with interrupts disabled.  And for rtc_lock ... I don't know if this
> code could be used on SMP :-)
> 
> > --- linux-orig/include/asm-mips/dec/ds1287.h        1970-01-01 
> > 09:00:00.000000000 +0900
> > +++ linux/include/asm-mips/dec/ds1287.h     2008-04-22 18:03:05.455637018 
> > +0900
> > @@ -0,0 +1,27 @@
> ...
> 
> I suppose CEVT_DS1287 is not DEC specific one.  If so,
> include/asm-mips/ would be better place.

Thank you for your comment.
I'll update this patch.

Yoichi