Re: [PATCH 4/4] wl1251: spi: add device tree support

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

On Monday 28 of October 2013 01:37:34 Kumar Gala wrote:
> On Oct 27, 2013, at 11:14 AM, Sebastian Reichel wrote:
> > Add device tree support for the spi variant of wl1251
> > and document the binding.
> > 
> > Signed-off-by: Sebastian Reichel <sre@debian.org>
> > ---
> > .../devicetree/bindings/net/wireless/ti,wl1251.txt | 36
> > ++++++++++++++++++++++ drivers/net/wireless/ti/wl1251/spi.c          
> >     | 23 ++++++++++---- 2 files changed, 53 insertions(+), 6
> > deletions(-)
> > create mode 100644
> > Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt
> > b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt new
> > file mode 100644
> > index 0000000..5f8a154
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/wireless/ti,wl1251.txt
> > @@ -0,0 +1,36 @@
> > +* Texas Instruments wl1251 controller
> > +
> > +The wl1251 chip can be connected via SPI or via SDIO. The linux
> > +kernel currently only supports device tree for the SPI variant.
> > +
> 
> From the binding I have no idea what this chip actually does, also we
> don't normally reference linux kernel support in bindings specs (so
> please remove it).
> 
> However, what would expect the SDIO binding to look like?  Or more
> specifically, how would you distinguish the SPI vs SDIO
> binding/connection?  I'm wondering if the compatible should be
> something like "ti,wl1251-spi" and than the sdio can be
> "ti,wl1251-sdio"

Well, you can easily distinguish an SDIO device from an SPI device by its 
parent node, but...

The binding for SDIO might require different set of properties (other than 
ones inherited from generic SDIO or SPI bindings) than one for SPI. So 
probably different compatible values might be justified.

Did we already have such case before? (maybe some I2C + SPI devices?)

> > +Required properties:
> > +- compatible : Should be "ti,wl1251"
> 
> reg is not listed as a required prop.

It is implied by SPI bindings, but it might be a good idea to have this 
stated here as well.

> 
> > +- interrupts : Should contain interrupt line
> > +- interrupt-parent : Should be the phandle for the interrupt
> > +  controller that services interrupts for this device
> > +- vio-supply : phandle to regulator providing VIO
> > +- power-gpio : GPIO connected to chip's PMEN pin
> 
> should be vendor prefixed: ti,power-gpio

Hmm, out of curiosity, is it a rule for this kind of properties? I can see 
both cases with and without prefixes when grepping for "-gpios" in 
Documentation/devicetree/bindings. We should really have such things 
written down somewhere.

> 
> > +- For additional required properties on SPI, please consult
> > +  Documentation/devicetree/bindings/spi/spi-bus.txt
> > +
> > +Optional properties:
> > +- ti,use-eeprom : If found, configuration will be loaded from eeprom.
> 
> can you be a bit more specific on what cfg will be loaded.  Also, is
> this property a boolean, if so how do I know which eeprom the cfg is
> loaded from (is it one that is directly connected to the wl1251?

Maybe one from ti,has-eeprom or ti,config-eeprom would be better name for 
this property?

Best regards,
Tomasz