Date:   Wed, 21 Sep 2016 14:47:50 +0200
From:   Geert Uytterhoeven <geert@...ux-m68k.org>
To:     Rob Herring <robh@...nel.org>
Cc:     Geert Uytterhoeven <geert+renesas@...der.be>,
        Mark Brown <broonie@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Magnus Damm <magnus.damm@...il.com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Hisashi Nakamura <hisashi.nakamura.ak@...esas.com>,
        Hiromitsu Yamasaki <hiromitsu.yamasaki.ym@...esas.com>,
        linux-spi <linux-spi@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        Linux-Renesas <linux-renesas-soc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH/RFC v2 1/7] spi: Document DT bindings for SPI controllers
 in slave mode

Hi Rob,

On Tue, Sep 20, 2016 at 5:00 PM, Rob Herring <robh@...nel.org> wrote:
> On Mon, Sep 12, 2016 at 10:50:40PM +0200, Geert Uytterhoeven wrote:
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@...der.be>
>> ---
>> v2:
>>   - Do not create a child node in SPI slave mode. Instead, add an
>>     "spi-slave" property, and put the mode properties in the controller
>>     node.
>> ---
>>  Documentation/devicetree/bindings/spi/spi-bus.txt | 34 ++++++++++++++---------
>>  1 file changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> index 17822860cb98c34d..1ae28d7cafb68dc5 100644
>> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> @@ -1,17 +1,23 @@
>>  SPI (Serial Peripheral Interface) busses
>>
>> -SPI busses can be described with a node for the SPI master device
>> -and a set of child nodes for each SPI slave on the bus.  For this
>> -discussion, it is assumed that the system's SPI controller is in
>> -SPI master mode.  This binding does not describe SPI controllers
>> -in slave mode.
>> +SPI busses can be described with a node for the SPI controller device
>> +and a set of child nodes for each SPI slave on the bus.  The system's SPI
>> +controller may be described for use in SPI master mode or in SPI slave mode,
>> +but not for both at the same time.
>>
>> -The SPI master node requires the following properties:
>> +The SPI controller node requires the following properties:
>> +- compatible      - name of SPI bus controller following generic names
>> +             recommended practice.
>
> We'll probably need some way to define what interface/protocol
> the slave has. Perhaps the most specific compatible should be the
> protocol the slave uses? Maybe that is how you use a child node?

That was indeed an advantage of using a child node (which you suggested
_not_ doing in your review of v1?): you can specify which protocol to use.

In v2, the protocol is specified through sysfs, like for i2c slave.

Note that SPI is different than I2C: an SPI slave is connected to a single
master, and can assume a single role only, while I2C is a shared bus, and
a slave can assume multiple roles (an I2C slave can respond to multiple
addresses, and can e.g. provide more than one software I2C EEPROM).
So you could argue the protocol is fixed by the hardware topology, cfr.
my v1.

>> +In master mode, the SPI controller node requires the following additional
>> +properties:
>>  - #address-cells  - number of cells required to define a chip select
>>               address on the SPI bus.
>>  - #size-cells     - should be zero.
>> -- compatible      - name of SPI bus controller following generic names
>> -             recommended practice.
>> +
>> +In slave mode, the SPI controller node requires one additional property:
>> +- spi-slave       - Empty property.
>> +
>>  No other properties are required in the SPI bus node.  It is assumed
>>  that a driver for an SPI bus device will understand that it is an SPI bus.
>>  However, the binding does not attempt to define the specific method for
>> @@ -21,7 +27,7 @@ assumption that board specific platform code will be used to manage
>>  chip selects.  Individual drivers can define additional properties to
>>  support describing the chip select layout.
>>
>> -Optional properties:
>> +Optional properties (master mode only):
>>  - cs-gpios     - gpios chip select.
>>  - num-cs       - total number of chipselects.
>>
>> @@ -41,12 +47,14 @@ cs1 : native
>>  cs2 : &gpio1 1 0
>>  cs3 : &gpio1 2 0
>>
>> -SPI slave nodes must be children of the SPI master node and can
>> -contain the following properties.
>> -- reg             - (required) chip select address of device.
>> +In master mode, SPI slave nodes must be children of the SPI controller node.
>> +In slave mode, the (single) slave device is represented by the controller node
>> +itself. SPI slave nodes can contain the following properties.
>
> I find this a bit confusing as you talk about master mode, then slave
> mode, then slave nodes (master mode again).

The last part is actually about both master and slave mode: in slave mode,
the properties apply to the controller node itself, instead of to child nodes.

I wanted to reuse as much of the existing text as possible.
But I agree the description could use some refactoring.

Thanks for your comments!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists