Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752414AbdLKJpQ (ORCPT ); Mon, 11 Dec 2017 04:45:16 -0500 Received: from webbox1416.server-home.net ([77.236.96.61]:58691 "EHLO webbox1416.server-home.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750836AbdLKJpO (ORCPT ); Mon, 11 Dec 2017 04:45:14 -0500 From: Alexander Stein To: Rasmus Villemoes Cc: Marc Zyngier , Thomas Gleixner , Jason Cooper , Rob Herring , Mark Rutland , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org Subject: Re: [RFC] irqchip: add support for LS1021A external interrupt lines Date: Mon, 11 Dec 2017 10:45:09 +0100 Message-ID: <5117875.4tMaEC1223@ws-stein> In-Reply-To: <58297576-cc32-819d-c6b3-7d1355095482@prevas.dk> References: <48d2d08c-c57a-ce49-5958-0fd5ad4a2dc7@arm.com> <9eab29c0-3347-635c-af92-4e8429064ed3@arm.com> <58297576-cc32-819d-c6b3-7d1355095482@prevas.dk> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2291 Lines: 61 On Monday, December 11, 2017, 10:08:20 AM CET Rasmus Villemoes wrote: > >>> +static int > >>> +ls1021a_extirq_set_type(struct irq_data *data, unsigned int type) > >>> +{ > >>> + irq_hw_number_t hwirq = data->hwirq; > >>> + struct extirq_chip_data *chip_data = data->chip_data; > >>> + u32 value, mask; > >>> + int ret; > >>> + > >>> + mask = 1U << (31 - hwirq); > >> > >> Is this really correct? IRQ0 is still at bit position 0. Don't be mislead > >> by the left most position in the register layout. This is just strange way > >> to express bit-endian access. > > Yes, I'm sure. The 26 unused bits in the INTPCR register are marked as > reserved with a POR value of 0. Fortunately, they can still be set and > read back, and when I did 1U << hwirq it was some of those bits that got > set (the POR value of the six used bits are all 1, so the hardware still > worked on my board because all the lines happen to be of negative polarity). Which functions do reg_read and reg_write in chip_data->syscon->bus_context actually point to? bus_context is actually a struct regmap_mmio_context *. > >> Anyway, please use BIT(x) instead. > > I really prefer not to, that macro obfuscates the type, and unsigned > long is the wrong thing to use for something that must be a 32 bit > quantity. Sure, BITS_PER_LONG==32 in this case, but I don't think > BIT(foo) is any easier to read than 1U << (foo). Well, there a lots of other places where BIT(x) is used for u32 data types, or even 16 Bit types. IMHO BIT(x) is more obvious as it already says set Bit x > >>> + *type = fwspec->param[2] & IRQ_TYPE_SENSE_MASK; > >>> + return 0; > >>> +} > >>> + > >>> +static int > >>> +ls1021a_extirq_domain_alloc(struct irq_domain *domain, unsigned int virq, > >>> + unsigned int nr_irqs, void *arg) > >>> +{ > >>> + static const unsigned xlate[NIRQ] = {163,164,165,167,168,169}; > >> ^^^^^^ > >> No need for static here. > > > > Why would you store this on the stack each time you enter the function? > > Exactly, it takes a lot less .rodata to make this static than having gcc > generate .text to build this array on the stack. > > > That's the wrong construct (these values should come from DT), but > > static is perfectly fine. > > OK. Intresting. Thanks for the info. Regards, Alexander