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

[Tomasz Figa]

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/