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

On 08/24/2013 10:31 AM, Tomasz Figa wrote:

On Saturday 24 of August 2013 10:25:26 Rob Herring wrote:

quoted

On Fri, Aug 23, 2013 at 6:04 PM, Tomasz Figa [off-list ref] 

wrote:

quoted

quoted

On Friday 23 of August 2013 16:11:18 Stephen Warren wrote:

quoted

On 08/22/2013 05:22 PM, Tomasz Figa wrote:

quoted

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