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

[Stephen Warren <swarren@xxxxxxxxxxxxx>]

On 07/22/2015 10:28 AM, Martin Sperl wrote:
> 
> 
>> On 22.07.2015, at 03:55, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
>>
>> 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
> 
> So, do I understand you correctly that if we would call the node 
> bcm2835aux_enable as syscon with only the enable bit field register in it 
> and add a enable_reg (pointing to the above) and enable_reg_mask=2 or 4
> to the spi1/2 nodes then it would be acceptable? 
> 
> Would look like this:
> spi2@7e2150c0 {
> +    compatible = "brcm,bcm2835-aux-spi";
> +    reg = <0x7e2150c0 0x40>;
> +    interrupts = <1 29>;
> +    clocks = <&clk_spi>;
> +    #address-cells = <1>;
> +    #size-cells = <0>;
> +    cs-gpios = <&gpio 43>, <&gpio 44>, <&gpio 45>;
> +    enable_reg = <&bcm2835aux_enable>;
> +    enable_reg_mask = 4;
> +};
> +
> +/* the necessary syscon config referenced above */
> +bcm2835aux_enable: bcm2835aux_enable@0x7e215004 {
> +    compatible = "syscon";
> +    reg = <0x7e215004 0x04>;
> +};
> 
> 
> The uart aux driver would use:
> +    enable_reg = <&bcm2835aux_enable>;
> +    enable_reg_mask = 1;

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.
--
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