From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754322AbbK0Ii4 (ORCPT ); Fri, 27 Nov 2015 03:38:56 -0500 Received: from www.linutronix.de ([62.245.132.108]:41981 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754303AbbK0Iix (ORCPT ); Fri, 27 Nov 2015 03:38:53 -0500 Date: Fri, 27 Nov 2015 09:37:54 +0100 (CET) From: Thomas Gleixner To: Simon Arlott cc: Florian Fainelli , MIPS Mailing List , Jonas Gorski , "devicetree@vger.kernel.org" , Ralf Baechle , Jason Cooper , Marc Zyngier , Kevin Cernekee , Wim Van Sebroeck , Miguel Gaio , Maxime Bizon , Linux Kernel Mailing List , linux-watchdog@vger.kernel.org, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala Subject: Re: [PATCH (v4) 2/11] MIPS: bmips: Add bcm6345-l2-timer interrupt controller In-Reply-To: <5657886F.3090908@simon.arlott.org.uk> Message-ID: References: <5650BFD6.5030700@simon.arlott.org.uk> <5653612A.4050309@simon.arlott.org.uk> <565361AF.20400@simon.arlott.org.uk> <70d031ae4c3aa29888d77b64686c39e7e7eaae92@8b5064a13e22126c1b9329f0dc35b8915774b7c3.invalid> <5654E67A.9060800@gmail.com> <5657886F.3090908@simon.arlott.org.uk> User-Agent: Alpine 2.11 (DEB 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1,SHORTCIRCUIT=-0.0001 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Gleixner Subject: Re: [PATCH (v4) 2/11] MIPS: bmips: Add bcm6345-l2-timer interrupt controller Date: Fri, 27 Nov 2015 09:37:54 +0100 (CET) Message-ID: References: <5650BFD6.5030700@simon.arlott.org.uk> <5653612A.4050309@simon.arlott.org.uk> <565361AF.20400@simon.arlott.org.uk> <70d031ae4c3aa29888d77b64686c39e7e7eaae92@8b5064a13e22126c1b9329f0dc35b8915774b7c3.invalid> <5654E67A.9060800@gmail.com> <5657886F.3090908@simon.arlott.org.uk> Mime-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Return-path: In-Reply-To: <5657886F.3090908-qdVf85lJwsCyrPCCpiK2c/XRex20P6io@public.gmane.org> Sender: linux-watchdog-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Simon Arlott Cc: Florian Fainelli , MIPS Mailing List , Jonas Gorski , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Ralf Baechle , Jason Cooper , Marc Zyngier , Kevin Cernekee , Wim Van Sebroeck , Miguel Gaio , Maxime Bizon , Linux Kernel Mailing List , linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala List-Id: devicetree@vger.kernel.org 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 -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html