Re: [RFC] dt: bindings: add bindings for Broadcom bcm43xx sdio devices

[Arend van Spriel <arend@broadcom.com>]

On 02/25/2014 11:51 PM, Stephen Warren wrote:
> On 02/10/2014 12:17 PM, Arend van Spriel wrote:
>> The Broadcom bcm43xx sdio devices are fullmac devices that may be
>> integrated in ARM platforms. Currently, the brcmfmac driver for
>> these devices support use of platform data. This patch specifies
>> the bindings that allow this platform data to be expressed in the
>> devicetree.
> 
>> diff --git a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
> 
>> + - compatible : Should be "brcm,bcm43xx-fmac".
>> + - wlan-supply : phandle for fixed regulator used to control power for
>> +	the device/module.
> 
> Ignoring the fact that perhaps this should just be a GPIO instead and
> assuming it actually make sense for this to be a regulator:
> 
> Why "fixed regulator" not just "the regulator". There shouldn't be any
> requirement for the power supply to the device to be fixed; the driver
> should (a) set the voltage (which will be a no-op for a fixed regulator
> already providing that voltage), then (b) enable the regulator. That
> would allow a PMIC with programmable voltage to be feeding the device.
> 
> Now, if this property was really intended to control some enable GPIO on
> the device, as others have said, this shouldn't be a regulator property
> but rather a GPIO property. However, there is definitely some power
> supply fed to the device, so you definitely need /some/ supply property
> here.
> 
> Aren't there other enable GPIOs required? These should be specified in DT.
> 
> Doesn't the WiFi chip/module require a (32KHz?) clock? If so, that needs
> to be represented in DT. Preferably write the binding to require
> clock-names (name-based lookup) rather than just clocks (index-based
> lookup).

Hi Stephen,

Thanks for these comments. While I agree with most of them, I am having
some difficulty with the DT concept. Essentially, a DT node describes a
part of the system. My scope for this change is probably limited wearing
my brcmfmac glasses. Am I correct in assuming that a DT node may be
processed/used by multiple drivers. As an example, the 32 kHz clock is
not something brcmfmac cares about. It simple needs to be available and
hooked up to the wlan device. The DT should have another node for this
clock which a (common) clock driver picks up. So having it referenced in
this node is purely informational, right?

Regards,
Arend

>> +Optional properties:
>> + - drive-strength : drive strength used for SDIO pins on device (default = 6mA).
> 
> As mentioned elsewhere, since that's a binding-specific property, rename
> it brcm,drive-strength.
> 
>> + - interrupt-parent : the phandle for the interrupt controller to which the
>> +	device interrupt (HOST_WAKE) is connected.
> 
> That's such a common property, individual bindings don't typically
> mention it.
> 
>> + - interrupts : interrupt specifier encoded according the interrupt controller
>> +	specified by interrupt-parent property.
> 
> The description of the property should say which interrupt (name and/or
> description) it's describing, even if there's only 1.
> 
> 
>> +mmc3: mmc@01c20000 {
>> +	pinctrl-0 = <&mmc3_pins>;
>> +	pinctrl-1 = <&wifi_host_wake>;
>> +	vmmc-supply = <&mmc3_supply>;
>> +	bus-width = <4>;
> 
> None of that is really relevant to this binding, and may vary from SDIO
> controller to SDIO controller, so may end up being wrong.
> 
> I'm not sure whether it makes sense to show the example inside some
> arbitrary SDIO controller node. Perhaps /just/ put the WiFi node in the
> example? The text above should be enough to describe that the node
> should be inside an SDIO controller.
> 
>> +	bcm4335: bcm4335@0 {
>> +		compatible = "brcm,bcm43xx-fmac";
>> +		wlan-supply = <&wlan-reg>;
>> +		drive-strength = <4>;
>> +		interrupt-parent = <&gpx2>;
>> +		interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
>> +		interrupt-names = "HOST_WAKE";
> 
> interrupt-names wasn't documented in the list of properties above.
> Entries in *-names properties are usually lower-case.
>