Device tree review #2

[Stephen Warren]

On 06/20/2012 02:24 PM, Simon Arlott wrote:
> On 20/06/12 04:48, Stephen Warren wrote:

>>> 		vc_bell: bell {
>>> 			compatible = "broadcom,bcm2835-bell", "broadcom,bcm2708-bell";
>>> 			reg = <0xB840 0x10>, <0xB8E4 0x4>;
>>> 			interrupts = <0 2>, <0 3>;
...
>>> 			interrupt-controller;
>>> 			#interrupt-cells = <1>;
>>
>> The bell object is a GPIO and IRQ controller too? IRQ perhaps I could
>> understand, but GPIO typicaly implies an external pin on the SoC
>> package, which seems slightly unlikely here. Could you explain more
>> about this HW; it's not in BCM2835-ARM-Peripherals.pdf right?
> 
> I'll have to remove the bell driver as I don't have a user for it.
> 
> When you write to the bell register it generates an interrupt. There
> are 4 bells, with 2 interrupts on the ARM and 2 on the VC.

Aren't those two interrupts the two that are described in the interrupts
property? I'd expect those two interrupts to be requested by the bell
driver itself, and handled within the bell driver. The
interrupt-contrller/#interrupt-cells properties are only required if the
bell driver doesn't directly handle the interrupts, but simply passes
them on to some other driver directly, in which case, I'd expect that
other driver just to hook the interrupt directly from the main interrupt
controller.

>>> 	power: regulator {
>>> 		compatible = "broadcom,bcm2835-power-mgr", "broadcom,bcm2708-power-mgr", "simple-bus";
>>> 		#address-cells = <1>;
>>> 		#size-cells = <0>;
>>
>> Given that the power manager is a core part of the SoC, I'd expect most
>> of this node to be in bcm2835.dtsi, not the board-specific file. The
>> board file can always add or override any properties that need to be set
>> in a board-specific way, perhaps such as the voltages.
> 
> The power manager is not part of the SoC, it's part of the Raspberry Pi
> version of the VideoCore,

"version of the VideoCore" /firmware/ I suppose.

> so it should be in the rpi board file. If we
> get direct access to the power management then that situation will
> change.

By that argument, isn't the display controller also something that
should be in bcm2835-rpi-b.dts rather than bcm2835.dtsi, since it's also
purely firmware-based?

>>> 		broadcom,vc-mailbox = <&vc_mbox>;
>>> 		broadcom,vc-channel = <0>;
>>>
>>> 		regulator-name = "VideoCore";
>>> 		regulator-min-microvolt = <5000000>;
>>> 		regulator-max-microvolt = <5000000>;
>>> 		regulator-always-on = <1>;
>>
>> It's not clear to me why one regulator is defined at the "top" level
>> directly as part of the power manager device, whereas the other two are
>> defined as child nodes; I would expect to see all 3 regulators defined
>> as child nodes.
> 
> I need a parent device as there's currently only a single bit map for
> all powered devices, which the child regulators need to share.

You can still have a parent device that exists solely to host 3
regulator child devices, rather than having the top-level node be one of
the regulators, with (a presumably arbitrarily selected) couple of child
nodes.