On Tue, Jul 19, 2016 at 03:15:49PM -0700, Florian Fainelli wrote:
> On 07/13/2016 03:36 PM, Mark Brown wrote:

> > If it was unconditionally in the perhipheral it'd be fairly normal and
> > standard but it's not, it's a completely separate and optional register
> > block with a bunch of separate code in a driver which already has above
> > average complexity even for the bits that belong in it.

> It is not separate nor optional, the fact that the register range seems
> discontiguous is just an integration choice here, but it contains bits
> that are relevant exclusively to the SPI controller.

Some versions of the block have it, some versions of the block don't
have it.  That suggests that it's optional.  It is a completely separate
register map, that suggests that it is an external block.

> It is not exclusively an interrupt controller, there are other bits in
> there are do not functionally belong into an interrupt controller driver
> per-se, things like a clock enable, arbiter control and a random bit to
> control MSPI flush behavior.

> Oh, and please don't tell me regmap is the solution here if we need to
> control both the interrupt-related bits and the other bits within this
> register.

That does sound system controllerish if those bits do need to be
managed.  Are we sure future designs won't just integrate this into the
main system controller?

> Considering that this block aggregates interrupt enable bits, but not
> just, there is no reason for this block to change over time, and clearly
> this won't be handled via a generic interrupt controller driver either.

There must be *some* logic behind the way the hardware has been
structured, and right now the code just isn't making any of this
apparent which makes it look like it needs refactoring.