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

[Tomasz Figa]

On 13.02.2014 13:07, Arend van Spriel wrote:
> On 02/13/2014 10:13 AM, Tomasz Figa wrote:
> > Hi Arend,
> >
> > On 10.02.2014 20:17, 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.
> > >
> > > Cc: Chen-Yu Tsai <wens@xxxxxxxx>
> > > Cc: Tomasz Figa <tomasz.figa@xxxxxxxxx>
> > > Reviewed-by: Hante Meuleman <meuleman@xxxxxxxxxxxx>
> > > Reviewed-by: Pieter-Paul Giesberts <pieterpg@xxxxxxxxxxxx>
> > > Signed-off-by: Arend van Spriel <arend@xxxxxxxxxxxx>
> > > ---
> > > This devicetree binding proposal is intended for platforms with
> > > Broadcom wireless device in MMC sdio slot. These devices may
> > > have their own interrupt and power line. Also the SDIO drive
> > > strength is often hardware dependent and expressed in this
> > > binding.
> > >
> > > Not sure if this should go in staging or not. Feel free to
> > > comment on this proposal.
> > >
> > > Regards,
> > > Arend
> > > ---
> > >    .../staging/net/wireless/brcm,bcm43xx-fmac.txt     |   37
> > > ++++++++++++++++++++
> > >    1 file changed, 37 insertions(+)
> > >    create mode 100644
> > > Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
> > >
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
> > > b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
> > >
> > > new file mode 100644
> > > index 0000000..535f343
> > > --- /dev/null
> > > +++
> > > b/Documentation/devicetree/bindings/staging/net/wireless/brcm,bcm43xx-fmac.txt
> > >
> > > @@ -0,0 +1,37 @@
> > > +Broadcom BCM43xx Fullmac wireless SDIO devices
> > > +
> > > +This node provides properties for controlling the Broadcom wireless
> > > device. The
> > > +node is expected to be specified as a child node to the MMC
> > > controller that
> > > +connects the device to the system.
> > > +
> > > +Required properties:
> > > +
> > > + - compatible : Should be "brcm,bcm43xx-fmac".
> > > + - wlan-supply : phandle for fixed regulator used to control power for
> > > +    the device/module.
> >
> > The BCM43xx WLAN chips I used to work always have been controlled by a
> > simple power enable GPIO of the chip itself. Has this changed in newer
> > chips?
> >
> > If you need to simply toggle a GPIO to control the power, you don't need
> > to use the regulator API at all, controlling the GPIO directly.
>
> Not sure I understand. Do you really mean 'chip' or 'wifi module' here.
> The chip needs to be powered and for that it is hooked up to a
> host/module provided GPIO (at least that is my understanding).

Your binding asks for a regulator, not a simple GPIO, which is all I 
believe you need for BCM43xx power handling.

Mark (added to Cc), could we get your opinion on this?

> > > +
> > > +Optional properties:
> > > + - drive-strength : drive strength used for SDIO pins on device
> > > (default = 6mA).
> >
> > This should be a part of the MMC binding, I think. Probably also moved
> > under MMC controller's node, since it's a board-specific property
> > altering the parameters of the MMC controller, not the WLAN chip.
>
> It is an electrical interfacing parameter between MMC controller and the
> device. The specified drive-strength here is used to configure the PMU
> on the chip so it is really related to the the chip.

Aha, so I misunderstood the description. Anyway, if it's a 
device-specific property, it should have vendor prefix ("brcm,").

> > > + - interrupt-parent : the phandle for the interrupt controller to
> > > which the
> > > +    device interrupt (HOST_WAKE) is connected.
> > > + - interrupts : interrupt specifier encoded according the interrupt
> > > controller
> > > +    specified by interrupt-parent property.
> >
> > I would also add a clock here, since the BCM43xx chips usually need a
> > 32k clock to operate (or at least the ones I used to work with did). It
> > can be optional, as not all systems can control this clock.
> >
> > > +
> > > +Example:
> > > +
> > > +mmc3: mmc@01c20000 {
> > > +    pinctrl-0 = <&mmc3_pins>;
> > > +    pinctrl-1 = <&wifi_host_wake>;
> >
> > WLAN_HOST_WAKE pin (aka the OOB interrupt) is specific to the WLAN chip,
> > so this should be rather configured in a pinctrl state of the WLAN chip
> > itself.
>
> You mean that pinctrl-1 should move inside the "brcm,bcm43xx-fmac" node,
> right?

Yes.

By the way, if not moved to device node, shouldn't it rather be just 
another entry of pinctrl-0? Also isn't pinctrl-names property missing here?

> > > +    vmmc-supply = <&mmc3_supply>;
> > > +    bus-width = <4>;
> > > +
> > > +    bcm4335: bcm4335@0 {
> >
> > nit: Why @0, if there is no reg property under this node?
>
> I am not fluent with devicetree specifications (yet) nor know all the
> conventions.

The "@" suffix is reserved for bus constructs, where the unit-address 
value after "@" corresponds to first address in reg property of the 
node. Here you could just simply use "bcm4335" or if you somehow manage 
to have two such chips (which I don't think is likely to happen) then 
you could use "bcm4335-1".

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/