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

[Stephen Warren <swarren@xxxxxxxxxxxxx>]

On 07/24/2015 12:47 AM, Martin Sperl wrote:
> 
>> On 24.07.2015, at 06:09, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
>>
>> I think I'd expect the shared registers to be:
>>
>> bcm2835aux: misc@0x7e215000 {
>>    compatible = "brcm,bcm2835-aux";
>>    reg = <0x7e215000 0x08>;
>> };
>>
>> There are two 4-byte registers outside the UART/SPI blocks, and the
>> compatible value should reflect what the block is, not that Linux may
>> use a syscon driver to implement the driver for it.
>>
>> In the client, I'd expect a more semantic naming for the reference;
>> something like:
>>
>> 	brcm,aux = <&bcm2835aux 4>;
>>
>> brcm, since it's a custom/vendor-specific property. aux is the name of
>> the object that's referenced. Packing the phandle and data together into
>> a single property reduces the number of separate properties, and is a
>> typical thing to do in a client of a service in DT.
> 
> So in the end you are saying we need a separate driver to get written
> (because of ‘compatible = "brcm,bcm2835-aux”;’)

It's fine if some existing driver matches that compatible value.

> That is unless you would allow:
> compatible = compatible = "brcm,bcm2835-aux”, “syscon”;

"syscon" definitely shouldn't be in a DT.

> If this is not acceptable, then where should such a driver go in the
> kernel tree?
> 
> It would essentially implement the following:
> bcm2835aux_enable(dev, device-id);
> bcm2835aux_disable(dev, device-id);
> 
> Which would just set/clean the bits in the register while holding a lock.

That sounds reasonable. I'd also expect a function that the client
drivers could call during probe() to look up the device (and implement
deferred probe) and another to release the reference during the client's
remove().

> As an alternative: maybe we could implement it as an IRQ driver
> and when the irq is requested for that device, then the HW-block gets
> enabled automatically (and disabled when released).

That seems a bit too implicit; there's no strict requirement that a
driver for e.g. the SPI block has to use an interrupt; it's merely
extremely likely.

> That way we may not need to have a separate driver that would enable
> the uart1, but it would be sufficient to configure it as follows:

Surely the IRQ driver would be a separate driver either way? The only
difference is whether it exposes custom APIs, or does things as a
side-effect of the existing IRQ API.
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html