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

[Gupta, Pekon]

> 
> 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'.

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