[PATCH 1/2] spi: bcm2835: add spi-bcm2835aux driver for the auxiliar spi1 and spi2

[Stephen Warren]

On 07/14/2015 06:39 AM, Martin Sperl wrote:
> 
>> On 14.07.2015, at 14:56, Stephen Warren <swarren at wwwdotorg.org> wrote:
>>
>> I don't care so much about the internal code details; anything there can
>> be trivially changed. But please do make sure the DT correctly
>> represents the HW by:
>>
>> * Having a separate DT node for each HW block (SPI controller, UART, and
>> any shared interrupt mux/demux/controller/...)
> 
> That is what we have with the v3 patch:
> * spi1
> * spi2
> * syscon controlling the aux-enable register
>   (only used to enable/disable the hw block)

OK, having separate nodes is great.

However, I'd like to see a "semantic" driver for the shared register
region rather than a "syscon". IIUC, "syscon" simply provides a stylized
way for one driver to touch some shared registers directly without any
semantics. I'd strongly prefer to see a real driver inside Linux rather
than something that just lets drivers fiddle with the shared registers
willy nilly. Still, this aspect is an internal implementation detail
inside the kernel that we can change without external impact later if we
need.

More concerning: The bcm283x HW doesn't implement a "syscon" module, but
some semantic IP block. The DT should contain a real compatible value
that describes what the HW block really is, not just "syscon". We could
bind the syscon driver to this compatible value if we have to for now.

>> * If e.g. the SPI controller driver is going to need to manipulate the
>> "shared interrupt mux/demux/controller/...", there should be a phandle
>> from the SPI controller node to the "shared interrupt
>> mux/demux/controller/..." node, rather than listing the shared registers
>> in each "client"'s reg property.
> That is not what is in the v3 release of the patch any longer.
> 
> Each of the above dt-nodes has their own (non-overlapping) register
> ranges and interrupts are enabled/disabled in the respective ranges
> for spi1/spi2. Also the status register of spi1/2 contain the
> corresponding flags for the reason of an interrupt - so no need
> to access any of the shared aux registers for that - hence the use
> of IRQF_SHARED.