Re: [PATCH V8 1/2] clocksource: add gx6605s SOC system timer

[Guo Ren]

On Thu, Sep 20, 2018 at 06:06:19PM +0200, Daniel Lezcano wrote:
> On 20/09/2018 10:03, Guo Ren wrote:
> > Changelog:
> >  - Add License and Copyright
> >  - Use timer-of framework
> >  - Change name with upstream feedback
> >  - Use clksource_mmio framework
> > 
> > Signed-off-by: Guo Ren <ren_guo@xxxxxxxxx>
> > ---
> >  drivers/clocksource/Kconfig         |   8 ++
> >  drivers/clocksource/Makefile        |   1 +
> >  drivers/clocksource/timer-gx6605s.c | 150 ++++++++++++++++++++++++++++++++++++
> >  3 files changed, 159 insertions(+)
> >  create mode 100644 drivers/clocksource/timer-gx6605s.c
> > 
> > diff --git a/drivers/clocksource/Kconfig b/drivers/clocksource/Kconfig
> > index a11f4ba..6d0f18d 100644
> > --- a/drivers/clocksource/Kconfig
> > +++ b/drivers/clocksource/Kconfig
> > @@ -620,4 +620,12 @@ config RISCV_TIMER
> >  	  is accessed via both the SBI and the rdcycle instruction.  This is
> >  	  required for all RISC-V systems.
> >  
> > +config GX6605S_TIMER
> > +	bool "Gx6605s SOC system timer driver"
> > +	depends on CSKY
> > +	select CLKSRC_MMIO
> > +	select TIMER_OF
> > +	help
> > +	  This option enables support for gx6605s SOC's timer.
> > +
> >  endmenu
> 
> Please make the option not visible as default. There are currently two
> approaches look at MTK_TIMER and SPRD_TIMER.
Em ... (ot sure why COMPILE_TEST?):
	bool "Gx6605s SOC system timer driver" if COMPILE_TEST
Hmm?

> > +static irqreturn_t gx6605s_timer_interrupt(int irq, void *dev)
> > +{
> > +	struct clock_event_device *ce = (struct clock_event_device *) dev;
> 
> nit: no cast is needed.
Yes, change to:
	struct clock_event_device *ce = dev;


> > +static u64 notrace gx6605s_sched_clock_read(void)
> > +{
> > +	void __iomem *base;
> > +
> > +	base = timer_of_base(&to) + CLKSRC_OFFSET;
> > +
> > +	return (u64) readl_relaxed(base + TIMER_VALUE);
> 
> nit: extra space after '(u64)'
Ok
	return (u64)readl_relaxed(base + TIMER_VALUE);

Best Regards
 Guo Ren