Re: [PATCH v8 1/7] irqchip: vic: Parse interrupt and resume masks from device tree

[Rob Herring <robherring2@gmail.com>]

On 08/24/2013 10:31 AM, Tomasz Figa wrote:
> On Saturday 24 of August 2013 10:25:26 Rob Herring wrote:
>> On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa <tomasz.figa@gmail.com> 
> wrote:
>>> On Friday 23 of August 2013 16:11:18 Stephen Warren wrote:
>>>> On 08/22/2013 05:22 PM, Tomasz Figa wrote:
>>>>> This patch extends vic_of_init to parse valid interrupt sources
>>>>> and resume sources masks from device tree.
>>>>>
>>>>> If mask values are not specified in device tree, all sources
>>>>> are assumed to be valid, as before this patch.
>>>>
>>>> Can you explain further why the VIC needs this information up-front?
>>>> Presumably it can accumulate it as devices request interrupts.
>>>
>>> It does not need this information just for operation, but this makes
>>> the hardware description more detailed and allows better sanity
>>> checking of interrupts being requested.
>>>
>>> To clarify, this is a mask of valid interrupt sources of the VIC,
>>> where
>>> set bit indicates that given signal is wired and clear bit that it is
>>> not.
>> I agree with Stephen here. The valid interrupts are the ones in the
>> DT. The reserved ones are the ones not present. If it is not needed
>> for the operation of the VIC, then remove it. The argument of sanity
>> checking could apply to all interrupt controllers.
> 
> Sorry, but I don't get what's wrong in having a more detailed description 
> than required just for operation of the hardware.
> 
> The feature of sanity checks based on interrupt_mask (here now called 
> valid-mask) has been present in the VIC driver since a long time already 
> (if not from the beginning of existence of this driver) and before we 
> started using DT, the mask was being passed from platform code as VIC init 
> function argument.

So we should base the binding on the Linux software design?

> I'd prefer this feature to be available when using DT as well, unless we 
> really want to move things backwards, just because we want to use DT...

As I mentioned all these arguments apply to ALL interrupt controllers
except ones which a mask does not work. So IF this makes sense, then
this should be a generic property and generic code to support.

You simply have the same information twice. One is distributed and one
is centralized. While it adds a way to validate things it also adds a
way to introduce errors. Suppose someone writes a dts such that
valid-mask matches the irq lines present in that dts (simply because
they were lazy or don't have documentation of all interrupt lines). Then
you go add a node with a new interrupt (because the initial dts was not
complete). Updating the valid-mask could very easily be forgotten. Yes,
this should all be found by testing, but people don't always have access
to all the h/w. This issue would also not likely be obvious in a review.

Rob