[PATCH 1/2] spi: dual and quad support(device tree)

[yuhang wang]

Hi, Pekon

2013/8/26 Gupta, Pekon <pekon at ti.com>:
>>
>> Hi, Pekon
>>
>> 2013/8/26 Gupta, Pekon <pekon at ti.com>:
>> >> Hi, Pekon
>> >>
>> >> 2013/8/26 Gupta, Pekon <pekon at ti.com>:
>> >> >>
>> >> >> Signed-off-by: wangyuhang <wangyuhang2014 at gmail.com>
>> >> >> ---
>> >> >>  Documentation/devicetree/bindings/spi/spi-bus.txt |   14
>> >> ++++++++++++++
>> >> >>  1 file changed, 14 insertions(+)
>> >> >>
>> >> >> diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> >> >> b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> >> >> index 296015e..145ba96 100644
>> >> >> --- a/Documentation/devicetree/bindings/spi/spi-bus.txt
>> >> >> +++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
>> >> >> @@ -55,6 +55,20 @@ contain the following properties.
>> >> >>               chip select active high
>> >> >>  - spi-3wire       - (optional) Empty property indicating device requires
>> >> >>                   3-wire mode.
>> >> >> +- spi-tx-nbits    - (optional) Number of bits used for MOSI(writting)
>> >> >> +- spi-rx-nbits    - (optional) Number of bits used for MISO(reading)
>> >> >> +
>> >> >> +So if for example the slave has 4 wires for writting and 2 wires for
>> >> reading,
>> >> >> +and the spi-tx/rx-nbits property should be set as follows:
>> >> >> +
>> >> >> +spi-tx-nbits = <4>;
>> >> >> +spi-rx-nbits = <2>;
>> >> >
>> >> > [Pekon]: there is a problem here...
>> >> > spi-tx-nbit = <4> suggests that SPI device support QUAD writes, but it
>> does
>> >> > not indicate whether DUAL writes are supported by device or not.
>> >> > So, In my view having either of the following implementation could help
>> >> > in specifying capabilities independently and clearly.
>> >> > *Implementation-1 Boolean*
>> >> > spi-tx-quad = <true | false>
>> >> > spi-tx-dual = <true | false>
>> >> > spi-tx-single = <true | false>
>> >> > Same way for Rx..
>> >> > spi-rx-quad = <true | false>
>> >> > spi-rx-dual = <true | false>
>> >> > spi-rx-single = <true | false>
>> >> >
>> >> > *Implementation-2 Multi-option*
>> >> > spi-quad = <tx-only | rx-only | duplex>
>> >> > spi-dual = <tx-only | rx-only | duplex>
>> >> > spi-single = <tx-only | rx-only | full-duplex | half-duplex>
>> >> >
>> >> Not exactly,  spi-tx-nbit = <4> suggests that SPI device will use QUAD
>> >> writes, not support QUAD writes. There is no need to set what mode
>> >> slave supports, user just set the certain mode slave will work in.
>> >>
>> > [Pekon]: Wait.. I think there is some mis-match in our understandings..
>> > (a) struct spi_device->mode determines capabilities spi_device can support
>> > (b) And, when user (here m25p80 like flash driver) puts a spi_transfer, then
>> > it specifies whether to use spi_transfer->tx_nbits = 4 or not..
>> > And you are using DT binding to specify (a) correct ?
>> >
>> > Now, how would you specify following configuration, using DT ?
>> >         spi_device->mode = SPI_TX_SINGLE | SPI_TX_QUAD. (but not dual)
>> > so that driver
>> > - driver give error,
>> >         when m25p80 driver uses spi_transfer->tx-nbits = 2
>> > - But allow,
>> >         when m25p80 driver uses spi_transfer->tx-nbits = 1 or
>> >         when m25p80 driver uses spi_transfer->tx-nbits = 4
>> >
>> >> >> +
>> >> >> +Now the value that spi-tx-nbits and spi-rx-nbits can receive is only
>> >> >> +1(single), 2(dual) and 4(quad). If you don't set spi-tx-nbits or spi-rx-
>> nbits,
>> >> >> +spi_device mode will be set in single(1 wire) as default. Another
>> point, if
>> >> >> +property:spi-3wire is set, spi-tx/rx-nbits is forbidden to set to <2 or
>> 4>,
>> >> >> +otherwise, an errro will return.
>> >> >>
>> >> > [Pekon]: Also, instead of having separate binding for 'spi-3wire', it can
>> be
>> >> > moved under as spi-single = <half-duplex>.
>> >> > Full-duplex = Tx and Rx operate on independent channels and
>> >> concurrently.
>> >> > Half-duplex = Tx and Rx use same bi-directional channel for transmission
>> >> >         one by one
>> >> >
>> >> Actually, spi-3wire can be regarded as a part of spi-single, but
>> >> corrected as what you said, there will be some inconvenient.
>> >> 1,the driver that has already used spi-3wire need a big change.
>> >> 2,there have to be a complexed check in spi framework if set like:
>> >>  spi-quad = <tx-only | rx-only | duplex>
>> >>  spi-dual = <tx-only | rx-only | duplex>
>> >>  spi-single = <tx-only | rx-only | full-duplex | half-duplex>
>> >>
>> > [Pekon]: No, I'm not asking you to update logic in all drivers,
>> > just the DT bindings. Something like this..
>> > @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct
>> spi_master *master)
>> >                 /* Device DUAL/QUAD mode */
>> >                 prop = of_get_property(nc, "spi-single", &len);
>> >                 if (prop == HALF_DUPLEX)
>> >                         spi->mode |= SPI_3WIRE;
>> >
>> > So, you set the same spi->mode[SPI_3WIRE] bit, thus other drivers are not
>> > impacted. And you can deprecate the older "spi-3wire" binding.
>> >
>>
>> OK, I got it. Logically it is better. But there are some difficulties
>> in Implementation.
>> 1. The property (spi-quad spi-dual spi-single) should be optional,
>> user can don't set any of these three properties. Then spi-3wire can
>> not be specified. Or if you want to set these properties as required,
>> all the spi slave (at least DT) should be changed.
>
> [Pekon]: Nope you don't need to change anything.
> Even today, there is an explicitly DT binding called 'spi-3wire' to enable
> this mode. Instead of it I'm proposing to use 'spi-tx-max-nbit = 0'
> or 'spi-rx-max-nbit = 0', and deprecate 'spi-3wire'.
>
I also thought of that way. But logically spi-tx-max-nbit = 0 and
spi-rx-max-nbit = 0 looks like use neither tx wires nor rx wires. So I
did not use that way to specify spi-3wire.

> So only those DTS need to be updated which use 'spi-3wire'
> And, I havn't found any DTS with spi-3wire :-)
>         grep -i 3wire arch/*/boot/dts/* -r
>
> Refer: Documentation/devicetree/bindings/spi/spi-bus.txt
> - spi-3wire       - (optional) Empty property indicating device requires
>                     3-wire mode.
>
>> 2. Add property like (spi-quad spi-dual spi-single) will make the
>> check in spi framework a little complicated.
>>   example:
>>   1)if one of these three is duplex, the other can not be set.
>>   2)there can not be the same value in either of the three.
>>   3)all three can not be set at the same time.
>>   .....
>> If in the future, more wires transfer is supported, you have to add
>> properties, then the check will be more complex.
>> So I still need some good aproaches to specify spi-3wire.
>>
> [Pekon]: Anyways this proposal is invalid, bcoz I mis-understood
>  ur patch earlier. So no issues here..
>> >
>> >> >>  If a gpio chipselect is used for the SPI slave the gpio number will be
>> >> passed
>> >> >>  via the cs_gpio
>> >> >> --
>> >> >> 1.7.9.5
>> >> >
>
> with regards, pekon