From mboxrd@z Thu Jan 1 00:00:00 1970 From: khilman@baylibre.com (Kevin Hilman) Date: Fri, 09 Jun 2017 16:30:07 -0700 Subject: [PATCH v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver In-Reply-To: <702e4608-8592-41fe-b79c-fef985c49be7@gmail.com> (Heiner Kallweit's message of "Sat, 10 Jun 2017 00:30:14 +0200") References: <04378047-4194-bb0f-3da3-e1d62345a86b@gmail.com> <89d02a38-7386-fcdc-4dce-ea7e531c90b4@gmail.com> <1496999266.3552.61.camel@baylibre.com> <702e4608-8592-41fe-b79c-fef985c49be7@gmail.com> Message-ID: To: linus-amlogic@lists.infradead.org List-Id: linus-amlogic.lists.infradead.org Heiner Kallweit writes: > Am 09.06.2017 um 23:15 schrieb Kevin Hilman: >> Jerome Brunet writes: >> >>> On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote: >>>> Add a driver supporting the GPIO interrupt controller on certain >>>> Amlogic meson SoC's. >>>> >>>> Signed-off-by: Heiner Kallweit >> >> [...] >> >>>> +static unsigned int meson_irq_startup(struct irq_data *data) >>>> +{ >>>> + irq_chip_unmask_parent(data); >>>> + /* >>>> + * An extra bit was added to allow having the same gpio hwirq twice >>>> + * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the >>>> + * gpio hwirq. >>>> + */ >>>> + meson_irq_set_hwirq(data, data->hwirq >> 1); >>> >>> Please keep in mind that any device can use this controller as irq parent. >>> It has to make sense, even when not serving the gpio driver. >>> >>> This hack means that, in DT, we'd have to multiply by 2 the values given under >>> section "22.3 GPIO interrupts" of the datasheet. This is an example Linux >>> specific stuff in DT. >>> It also means that the controller declares a lot more lines that it really has >>> ... >>> >>> This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the >>> mapping from the irq_set_type callback of the GPIO driver, which is still think >>> should be dropped at this point. >> >> +1 >> >> Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the >> basic design and functionality. The gymnastics required to support this >> hack (due to broken hardware) are getting in the way of getting basic >> functionality merged. >> > I haven't heard any feedback on the other proposal yet: > Always reserve two parent irq's in the request_resources callback, > then set_type is easy. How about this one? The feedback is the same: let's consider that after getting the basics reviewed, accepted and merged. > Having max. 4 gpio irq's should be a fair price for a cleaner design. Yuck, but better than no support for both _BOTH I guess. But again, discussion of hacks for the hardware limitation is getting in the way discussing and merging the basics. Let's get the basics right first, then add functionality. I understand it can be rather frustrating to give up features and/or functionality for having something "clean", but unfortunately, that's how the upstream linux kernel community has always worked. Most maintainers prefer clean, well-designed and maintainable code that comes in small, easily reviewable chunks over something that's difficult to understand with hacks and workarounds. Even if that means a limited feature set initially. Additional functionality (even if hacky) is much easier to review, discuss and maintain *later* when it's on top of a starting point that is clean. Kevin From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kevin Hilman Subject: Re: [PATCH v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver Date: Fri, 09 Jun 2017 16:30:07 -0700 Message-ID: References: <04378047-4194-bb0f-3da3-e1d62345a86b@gmail.com> <89d02a38-7386-fcdc-4dce-ea7e531c90b4@gmail.com> <1496999266.3552.61.camel@baylibre.com> <702e4608-8592-41fe-b79c-fef985c49be7@gmail.com> Mime-Version: 1.0 Content-Type: text/plain Return-path: Received: from mail-pg0-f51.google.com ([74.125.83.51]:33962 "EHLO mail-pg0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751676AbdFIXaM (ORCPT ); Fri, 9 Jun 2017 19:30:12 -0400 Received: by mail-pg0-f51.google.com with SMTP id v18so30747618pgb.1 for ; Fri, 09 Jun 2017 16:30:12 -0700 (PDT) In-Reply-To: <702e4608-8592-41fe-b79c-fef985c49be7@gmail.com> (Heiner Kallweit's message of "Sat, 10 Jun 2017 00:30:14 +0200") Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Heiner Kallweit Cc: Jerome Brunet , Mark Rutland , Marc Zyngier , Linus Walleij , Thomas Gleixner , Rob Herring , Neil Armstrong , devicetree@vger.kernel.org, linux-amlogic@lists.infradead.org, linux-gpio@vger.kernel.org, "thierry.reding@gmail.com" , Thierry Reding Heiner Kallweit writes: > Am 09.06.2017 um 23:15 schrieb Kevin Hilman: >> Jerome Brunet writes: >> >>> On Thu, 2017-06-08 at 21:38 +0200, Heiner Kallweit wrote: >>>> Add a driver supporting the GPIO interrupt controller on certain >>>> Amlogic meson SoC's. >>>> >>>> Signed-off-by: Heiner Kallweit >> >> [...] >> >>>> +static unsigned int meson_irq_startup(struct irq_data *data) >>>> +{ >>>> + irq_chip_unmask_parent(data); >>>> + /* >>>> + * An extra bit was added to allow having the same gpio hwirq twice >>>> + * for handling IRQ_TYPE_EDGE_BOTH. Remove this bit to get the >>>> + * gpio hwirq. >>>> + */ >>>> + meson_irq_set_hwirq(data, data->hwirq >> 1); >>> >>> Please keep in mind that any device can use this controller as irq parent. >>> It has to make sense, even when not serving the gpio driver. >>> >>> This hack means that, in DT, we'd have to multiply by 2 the values given under >>> section "22.3 GPIO interrupts" of the datasheet. This is an example Linux >>> specific stuff in DT. >>> It also means that the controller declares a lot more lines that it really has >>> ... >>> >>> This is all to accommodate your hack around IRQ_TYPE_BOTH and creating the >>> mapping from the irq_set_type callback of the GPIO driver, which is still think >>> should be dropped at this point. >> >> +1 >> >> Please drop the hack for IRQ_TYPE_BOTH so we can reach agreement on the >> basic design and functionality. The gymnastics required to support this >> hack (due to broken hardware) are getting in the way of getting basic >> functionality merged. >> > I haven't heard any feedback on the other proposal yet: > Always reserve two parent irq's in the request_resources callback, > then set_type is easy. How about this one? The feedback is the same: let's consider that after getting the basics reviewed, accepted and merged. > Having max. 4 gpio irq's should be a fair price for a cleaner design. Yuck, but better than no support for both _BOTH I guess. But again, discussion of hacks for the hardware limitation is getting in the way discussing and merging the basics. Let's get the basics right first, then add functionality. I understand it can be rather frustrating to give up features and/or functionality for having something "clean", but unfortunately, that's how the upstream linux kernel community has always worked. Most maintainers prefer clean, well-designed and maintainable code that comes in small, easily reviewable chunks over something that's difficult to understand with hacks and workarounds. Even if that means a limited feature set initially. Additional functionality (even if hacky) is much easier to review, discuss and maintain *later* when it's on top of a starting point that is clean. Kevin