Re: [PATCH] Add TI-Nspire timer support

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, 30 May 2013, Daniel Tang wrote:
> +#define IO_MATCH(x)	(IO_MATCH_BEGIN + ((x)<<2))

space before and after "<<" please.
  
> +#define CNTL_STOP_TIMER	(1<<4)

Ditto.

> +struct zevio_timer {
> +	void __iomem *base;
> +	void __iomem *timer1, *timer2;
> +	void __iomem *interrupt_regs;
> +
> +	int irqnr;
> +
> +	struct clk *clk;
> +	struct clock_event_device clkevt;
> +	struct irqaction clkevt_irq;
> +
> +	char clocksource_name[64];
> +	char clockevent_name[64];
> +};
> +
> +static int zevio_timer_set_event(unsigned long delta,
> +		struct clock_event_device *dev)

static int zevio_timer_set_event(unsigned long delta,
			         struct clock_event_device *dev)

Is way more readable.

> +{
> +	unsigned long flags;
> +	struct zevio_timer *timer = container_of(dev,
> +			struct zevio_timer,
> +			clkevt);
> +

        struct zevio_timer *timer = container_of(dev, struct zevio_timer,
	       		   	    		 clekevt);

This random alignment of split lines is really annoying. Please fix
all over the place.

> +	local_irq_save(flags);

Pointless exercise. All the clockevent callbacks are called with
interrupts disabled.

> +	writel(delta, timer->timer1 + IO_CURRENT_VAL);
> +	writel(CNTL_RUN_TIMER | CNTL_DEC | CNTL_MATCH(TIMER_MATCH),
> +			timer->timer1 + IO_CONTROL);
> +
> +	local_irq_restore(flags);
> +
> +	return 0;
> +}
> +
> +static void zevio_timer_set_mode(enum clock_event_mode mode,
> +		struct clock_event_device *dev)
> +{
> +	unsigned long flags;
> +	struct zevio_timer *timer = container_of(dev,
> +			struct zevio_timer,
> +			clkevt);
> +
> +	local_irq_save(flags);

See above.

> +	switch (mode) {
> +	case CLOCK_EVT_MODE_RESUME:
> +	case CLOCK_EVT_MODE_ONESHOT:
> +		/* Enable timer interrupts */
> +		writel(TIMER_INTR_MSK, timer->interrupt_regs + IO_INTR_MSK);
> +		writel(TIMER_INTR_ALL, timer->interrupt_regs + IO_INTR_ACK);
> +		dev->mode = mode;

The core code sets the dev->mode. Where did you copy this from?

> +static int __init zevio_timer_add(struct device_node *node)
> +{
> +	struct zevio_timer *timer;
> +	struct resource res;
> +	int ret;
> +
> +	timer = kzalloc(sizeof(*timer), GFP_ATOMIC);

Why do you need an atomic allocation here ?

> +
> +	if (timer->interrupt_regs && timer->irqnr) {
> +		timer->clkevt.name	= timer->clockevent_name;
> +		timer->clkevt.set_next_event = zevio_timer_set_event;
> +		timer->clkevt.set_mode	= zevio_timer_set_mode;

Either you have a consistent alignment of all these assignments or you
have not.

> +		timer->clkevt.rating	= 200;
> +		timer->clkevt.cpumask	= cpu_all_mask;
> +		timer->clkevt.features	=
> +			CLOCK_EVT_FEAT_ONESHOT;

Pointless linebreak.

> +		writel(CNTL_STOP_TIMER, timer->timer1 + IO_CONTROL);
> +		writel(0, timer->timer1 + IO_DIVIDER);
> +
> +		/* Start with timer interrupts disabled */
> +		writel(0, timer->interrupt_regs + IO_INTR_MSK);
> +		writel(TIMER_INTR_ALL, timer->interrupt_regs + IO_INTR_ACK);
> +
> +		/* Interrupt to occur when timer value matches 0 */
> +		writel(0, timer->base + IO_MATCH(TIMER_MATCH));
> +
> +		timer->clkevt_irq.name	= timer->clockevent_name;
> +		timer->clkevt_irq.handler = zevio_timer_interrupt;
> +		timer->clkevt_irq.dev_id = timer;
> +		timer->clkevt_irq.flags	=
> +			IRQF_DISABLED | IRQF_TIMER | IRQF_IRQPOLL;

Please lookup what IRQF_DISABLED actually does.

Thanks,

	tglx