lkml.org 
[lkml]   [2015]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH (v4) 2/11] MIPS: bmips: Add bcm6345-l2-timer interrupt controller
Simon,

On Thu, 26 Nov 2015, Simon Arlott wrote:
> +static inline u32 bcm6345_timer_read_int_status(struct bcm6345_timer *timer)
> +{
> + if (timer->interrupt_bits == 32)
> + return __raw_readl(timer->base + timer->regs[TIMER_INT_STATUS]);
> + else
> + return __raw_readb(timer->base + timer->regs[TIMER_INT_STATUS]);
> +}

Instead of having that pile of conditionals you could just define two
functions and have a function pointer in struct bcm6345_timer which
you initialize at init time.

> +static inline void bcm6345_timer_write_control(struct bcm6345_timer *timer,
> + unsigned int id, u32 val)
> +{
> + if (id >= timer->nr_timers) {
> + WARN(1, "%s: %d >= %d", __func__, id, timer->nr_timers);

This is more than silly. You call that form the init function via:

for (i = 0; i < timer->nr_timers; i++)

Hmm?

> +static void bcm6345_timer_unmask(struct irq_data *d)
> +{
> + struct bcm6345_timer *timer = irq_data_get_irq_chip_data(d);
> + unsigned long flags;
> + u8 val;
> +
> + if (d->hwirq < timer->nr_timers) {

Again. You can have two different interrupt chips without that
completely undocumented and non obvious conditional.

BTW, how are those simple interrupts masked at all?

> + raw_spin_lock_irqsave(&timer->lock, flags);
> + val = bcm6345_timer_read_int_enable(timer);
> + val |= BIT(d->hwirq);
> + bcm6345_timer_write_int_enable(timer, val);
> + raw_spin_unlock_irqrestore(&timer->lock, flags);
> + }
> +}

> + raw_spin_lock_init(&timer->lock);
> + timer->regs = regs;
> + timer->interrupt_bits = interrupt_bits;
> + timer->nr_timers = nr_timers;
> + timer->nr_interrupts = nr_timers + 1;

What is that extra interrupt about? For the casual reader this looks
like a bug ... Comments exist for a reason.

Thanks,

tglx


\
 
 \ /
  Last update: 2015-11-27 10:01    [W:0.193 / U:0.196 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site