From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751787AbaAOHqv (ORCPT ); Wed, 15 Jan 2014 02:46:51 -0500 Received: from mail-oa0-f48.google.com ([209.85.219.48]:56621 "EHLO mail-oa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751665AbaAOHqs (ORCPT ); Wed, 15 Jan 2014 02:46:48 -0500 MIME-Version: 1.0 In-Reply-To: <1389308855-32588-1-git-send-email-ynvich@gmail.com> References: <1387309071-22382-12-git-send-email-ynvich@gmail.com> <1389308855-32588-1-git-send-email-ynvich@gmail.com> Date: Wed, 15 Jan 2014 08:46:47 +0100 Message-ID: Subject: Re: [PATCH v3.1 11/21] ARM: pxa: support ICP DAS LP-8x4x FPGA irq From: Linus Walleij To: Sergei Ianovich Cc: "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Arnd Bergmann , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Rob Landley , Russell King , Thomas Gleixner , Grant Likely , "open list:OPEN FIRMWARE AND..." , "open list:DOCUMENTATION" Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is looking much better! On Fri, Jan 10, 2014 at 12:07 AM, Sergei Ianovich wrote: > +++ b/drivers/irqchip/irq-lp8x4x.c (...) You could add some kerneldoc to this following struct (OK nitpick, but still nice, especially for the last two variables). > +struct lp8x4x_irq_data { > + void *base; > + struct irq_domain *domain; > + unsigned long num_irq; > + unsigned char irq_sys_enabled; > + unsigned char irq_high_enabled; > +}; > + > +static void lp8x4x_mask_irq(struct irq_data *d) > +{ > + unsigned mask; > + unsigned long irq = d->hwirq; Name the local variable hwirq too so we know what it is. > + struct lp8x4x_irq_data *host = irq_data_get_irq_chip_data(d); > + > + if (!host) { > + pr_err("lp8x4x: missing host data for irq %i\n", d->irq); > + return; > + } > + > + if (irq >= host->num_irq) { > + pr_err("lp8x4x: wrong irq handler for irq %i\n", d->irq); > + return; > + } This is on the hotpath. Do you *really* need these two checks? (...) > +static void lp8x4x_unmask_irq(struct irq_data *d) > +{ > + unsigned mask; > + unsigned long irq = d->hwirq; Name the variable "hwirq". > + struct lp8x4x_irq_data *host = irq_data_get_irq_chip_data(d); > + > + if (!host) { > + pr_err("lp8x4x: missing host data for irq %i\n", d->irq); > + return; > + } > + > + if (irq >= host->num_irq) { > + pr_err("lp8x4x: wrong irq handler for irq %i\n", d->irq); > + return; > + } Again overzealous error checks. (...) > +static void lp8x4x_irq_handler(unsigned int irq, struct irq_desc *desc) > +{ > + int n; > + unsigned long mask; > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct lp8x4x_irq_data *host = irq_desc_get_handler_data(desc); > + > + if (!host) > + return; I don't think this happens either? > + chained_irq_enter(chip, desc); > + > + for (;;) { > + mask = ioread8(host->base + CLRHILVINT) & 0xff; > + mask |= (ioread8(host->base + SECOINT) & SECOINT_MASK) << 8; > + mask |= (ioread8(host->base + PRIMINT) & PRIMINT_MASK) << 8; > + mask &= host->irq_high_enabled | (host->irq_sys_enabled << 8); > + if (mask == 0) > + break; > + for_each_set_bit(n, &mask, BITS_PER_LONG) > + generic_handle_irq(irq_find_mapping(host->domain, n)); > + } I like the looks of this. If you fix this: Reviewed-by: Linus Walleij Yours, Linus Walleij From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Subject: Re: [PATCH v3.1 11/21] ARM: pxa: support ICP DAS LP-8x4x FPGA irq Date: Wed, 15 Jan 2014 08:46:47 +0100 Message-ID: References: <1387309071-22382-12-git-send-email-ynvich@gmail.com> <1389308855-32588-1-git-send-email-ynvich@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Return-path: In-Reply-To: <1389308855-32588-1-git-send-email-ynvich@gmail.com> Sender: linux-doc-owner@vger.kernel.org To: Sergei Ianovich Cc: "linux-kernel@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , Arnd Bergmann , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Rob Landley , Russell King , Thomas Gleixner , Grant Likely , "open list:OPEN FIRMWARE AND..." , "open list:DOCUMENTATION" List-Id: devicetree@vger.kernel.org This is looking much better! On Fri, Jan 10, 2014 at 12:07 AM, Sergei Ianovich wrote: > +++ b/drivers/irqchip/irq-lp8x4x.c (...) You could add some kerneldoc to this following struct (OK nitpick, but still nice, especially for the last two variables). > +struct lp8x4x_irq_data { > + void *base; > + struct irq_domain *domain; > + unsigned long num_irq; > + unsigned char irq_sys_enabled; > + unsigned char irq_high_enabled; > +}; > + > +static void lp8x4x_mask_irq(struct irq_data *d) > +{ > + unsigned mask; > + unsigned long irq = d->hwirq; Name the local variable hwirq too so we know what it is. > + struct lp8x4x_irq_data *host = irq_data_get_irq_chip_data(d); > + > + if (!host) { > + pr_err("lp8x4x: missing host data for irq %i\n", d->irq); > + return; > + } > + > + if (irq >= host->num_irq) { > + pr_err("lp8x4x: wrong irq handler for irq %i\n", d->irq); > + return; > + } This is on the hotpath. Do you *really* need these two checks? (...) > +static void lp8x4x_unmask_irq(struct irq_data *d) > +{ > + unsigned mask; > + unsigned long irq = d->hwirq; Name the variable "hwirq". > + struct lp8x4x_irq_data *host = irq_data_get_irq_chip_data(d); > + > + if (!host) { > + pr_err("lp8x4x: missing host data for irq %i\n", d->irq); > + return; > + } > + > + if (irq >= host->num_irq) { > + pr_err("lp8x4x: wrong irq handler for irq %i\n", d->irq); > + return; > + } Again overzealous error checks. (...) > +static void lp8x4x_irq_handler(unsigned int irq, struct irq_desc *desc) > +{ > + int n; > + unsigned long mask; > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct lp8x4x_irq_data *host = irq_desc_get_handler_data(desc); > + > + if (!host) > + return; I don't think this happens either? > + chained_irq_enter(chip, desc); > + > + for (;;) { > + mask = ioread8(host->base + CLRHILVINT) & 0xff; > + mask |= (ioread8(host->base + SECOINT) & SECOINT_MASK) << 8; > + mask |= (ioread8(host->base + PRIMINT) & PRIMINT_MASK) << 8; > + mask &= host->irq_high_enabled | (host->irq_sys_enabled << 8); > + if (mask == 0) > + break; > + for_each_set_bit(n, &mask, BITS_PER_LONG) > + generic_handle_irq(irq_find_mapping(host->domain, n)); > + } I like the looks of this. If you fix this: Reviewed-by: Linus Walleij Yours, Linus Walleij From mboxrd@z Thu Jan 1 00:00:00 1970 From: linus.walleij@linaro.org (Linus Walleij) Date: Wed, 15 Jan 2014 08:46:47 +0100 Subject: [PATCH v3.1 11/21] ARM: pxa: support ICP DAS LP-8x4x FPGA irq In-Reply-To: <1389308855-32588-1-git-send-email-ynvich@gmail.com> References: <1387309071-22382-12-git-send-email-ynvich@gmail.com> <1389308855-32588-1-git-send-email-ynvich@gmail.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org This is looking much better! On Fri, Jan 10, 2014 at 12:07 AM, Sergei Ianovich wrote: > +++ b/drivers/irqchip/irq-lp8x4x.c (...) You could add some kerneldoc to this following struct (OK nitpick, but still nice, especially for the last two variables). > +struct lp8x4x_irq_data { > + void *base; > + struct irq_domain *domain; > + unsigned long num_irq; > + unsigned char irq_sys_enabled; > + unsigned char irq_high_enabled; > +}; > + > +static void lp8x4x_mask_irq(struct irq_data *d) > +{ > + unsigned mask; > + unsigned long irq = d->hwirq; Name the local variable hwirq too so we know what it is. > + struct lp8x4x_irq_data *host = irq_data_get_irq_chip_data(d); > + > + if (!host) { > + pr_err("lp8x4x: missing host data for irq %i\n", d->irq); > + return; > + } > + > + if (irq >= host->num_irq) { > + pr_err("lp8x4x: wrong irq handler for irq %i\n", d->irq); > + return; > + } This is on the hotpath. Do you *really* need these two checks? (...) > +static void lp8x4x_unmask_irq(struct irq_data *d) > +{ > + unsigned mask; > + unsigned long irq = d->hwirq; Name the variable "hwirq". > + struct lp8x4x_irq_data *host = irq_data_get_irq_chip_data(d); > + > + if (!host) { > + pr_err("lp8x4x: missing host data for irq %i\n", d->irq); > + return; > + } > + > + if (irq >= host->num_irq) { > + pr_err("lp8x4x: wrong irq handler for irq %i\n", d->irq); > + return; > + } Again overzealous error checks. (...) > +static void lp8x4x_irq_handler(unsigned int irq, struct irq_desc *desc) > +{ > + int n; > + unsigned long mask; > + struct irq_chip *chip = irq_desc_get_chip(desc); > + struct lp8x4x_irq_data *host = irq_desc_get_handler_data(desc); > + > + if (!host) > + return; I don't think this happens either? > + chained_irq_enter(chip, desc); > + > + for (;;) { > + mask = ioread8(host->base + CLRHILVINT) & 0xff; > + mask |= (ioread8(host->base + SECOINT) & SECOINT_MASK) << 8; > + mask |= (ioread8(host->base + PRIMINT) & PRIMINT_MASK) << 8; > + mask &= host->irq_high_enabled | (host->irq_sys_enabled << 8); > + if (mask == 0) > + break; > + for_each_set_bit(n, &mask, BITS_PER_LONG) > + generic_handle_irq(irq_find_mapping(host->domain, n)); > + } I like the looks of this. If you fix this: Reviewed-by: Linus Walleij Yours, Linus Walleij