From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752160AbbJKR7P (ORCPT ); Sun, 11 Oct 2015 13:59:15 -0400 Received: from www.linutronix.de ([62.245.132.108]:41938 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751839AbbJKR7O (ORCPT ); Sun, 11 Oct 2015 13:59:14 -0400 Date: Sun, 11 Oct 2015 19:58:36 +0200 (CEST) From: Thomas Gleixner To: Oleksij Rempel cc: linux-kernel@vger.kernel.org, marc.zyngier@arm.com, jason@lakedaemon.net Subject: Re: [PATCH v6 1/2] ARM: irqchip: mxs: prepare driver for HW with different offsets In-Reply-To: <1444482507-6748-2-git-send-email-linux@rempel-privat.de> Message-ID: References: <1444482507-6748-1-git-send-email-linux@rempel-privat.de> <1444482507-6748-2-git-send-email-linux@rempel-privat.de> 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 Oleksij, On Sat, 10 Oct 2015, Oleksij Rempel wrote: The proper subject line starts with: irqchip/mxs: > Some HW has similar functionality but different register offsets. > Make sure we can change offsets dynamically. The patch does way more than that. I told you in V2 already: > > You forgot to mention the other preparatory changes. There is still nothing in the changelog. > +static void __init icoll_add_domain(struct device_node *np, > + int num) > +{ > + icoll_domain = irq_domain_add_linear(np, num, > + &icoll_irq_domain_ops, NULL); > + > + if (!icoll_domain) > + panic("%s: unable add irq domain", np->full_name); This splitout should be a separate patch with an explanation why you add > + irq_set_default_host(icoll_domain); and > + set_handle_irq(icoll_handle_irq); The latter is already done via the DT_MACHINE_START magic. So you should it remove there, because otherwise that call is just pointless. See the implementation of set_handle_irq. > +} > + > +static void __iomem * __init icoll_init_iobase(struct device_node *np) > +{ > + void __iomem *icoll_base; > + > + icoll_base = of_io_request_and_map(np, 0, np->name); > + if (!icoll_base) > + panic("%s: unable to map resource", np->full_name); > + return icoll_base; > +} The panic() is actually a bug fix, because the old code had a WARN_ON() and then happily dereferenced the NULL pointer. So this wants to go into a separate patch as well. > + icoll_add_domain(np, ICOLL_NUM_IRQS); > > - icoll_domain = irq_domain_add_linear(np, ICOLL_NUM_IRQS, > - &icoll_irq_domain_ops, NULL); > return icoll_domain ? 0 : -ENODEV; In case of !icoll_domain this return is not reached as you paniced already. So why would we still check icoll_domain? Thanks, tglx