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

[Tomasz Figa <tomasz.figa@gmail.com>]

Hi Arend,

On 13.03.2014 11:16, Arend van Spriel wrote:
> 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.

That's correct. A DT node represents a component of a system and its 
contents should contain all resources and other device-specific data 
required for this device to operate or optional.

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

It may be, but it is usually not. The typical use case for such scheme 
is a bus-like topology, where devices on the bus are sub-nodes of the 
bus controller node and may contain some bus-specific information, such 
as chip select (e.g. SPI), address (e.g. I2C) or maximum bus speed.

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

Not really. The driver should care about any resources needed for the 
device to operate. In this case, a 32 kHz clock even if wired to the 
chip, sometimes is not operational until it gets ungated. This is not an 
artificial example, as on many boards I used to work with the 32 kHz 
clock was driven by a PMIC with clock gating control through I2C, gated 
by default.

Moreover, (well, 32 kHz might not be the best example) from power saving 
reasons, it might be a good idea to let the driver control the clock and 
gate it whenever it is not necessary.

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

You are confusing here provider with consumer. The bcm43xx chip is 
clearly a consumer of a 32 kHz clock and so its DT node should specify this.

A DT node for a clock, would be a clock provider node and that would be 
handled by common clock framework in case of Linux indeed. A clock 
provider node doesn't have to be limited to a single clock, though. In 
the case I mentioned above, PMIC node would be a clock provider and PMIC 
driver would register necessary clocks in common clock framework.

Best regards,
Tomasz