Re: [PATCH v6 2/9] irqchip: add Amlogic Meson GPIO irqchip driver

[Kevin Hilman <khilman@xxxxxxxxxxxx>]

Heiner Kallweit <hkallweit1@xxxxxxxxx> writes:

> Am 09.06.2017 um 23:15 schrieb Kevin Hilman:
>> Jerome Brunet <jbrunet@xxxxxxxxxxxx> 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 <hkallweit1@xxxxxxxxx>
>> 
>> [...]
>> 
>>>> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html