[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]: Oh.. Sorry.. I mis-understood your patch here..
>> > So here 'spi-tx-nbits' and 'spi-rx-nbits' specify how many data-channels
>> > are actually connected on board to a spi_device.. correct ?
>> >
>> > And, m25p30 driver will determine what to put in spi_transfer->tx_nbits
>> > based on different flash commands .. Correct ?
>> >
>> Yes, that's it.
>>
>> > In that sense.. its your approach is correct..
>> > But then please use different binding names, something like below..
>> >         s/spi-tx-nbits/spi-tx-max-width
>> >         s/spi-rx-nbits/spi-rx-max-width
>> >
>> > -----------------------------------------------
>> Sorry, what do you mean by using spi-tx/rx-max-width, I did not get it clearly.
>>
> [Pekon]: Just a change in name of binding..
> Like using spi-max-rx-nbits and spi-max-tx-nbits instead.
>
So do you mean that the name in DT should look different from
spi_transfer->tx_nbits/rx_nbits. OK, I will correct it. Thanks.

>> >> > > [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.
>> >> >
>> > -----------------------------------------------
>> > My above comment is more for DT binding for spi_master (master DT
>> node)
>> > probed by controller driver, which can have multiple capabilities.
>> >
>> > I think you havn't added anything for that .. neither checks for that..
>> > Do you plan to have a patch for that too ?
>> >
>> >
>> Well, I am still not sure whether to add DT binding for spi_master.
>> Now the multiple capabilities are set directly in probe of spi
>> controller. But I think there is no need to check the capabilities
>> from DT. Because no matter it is in probe or DT, it's all controller
>> driver designers' job.
>> Take a example:
>> 1. I set the property in master node as follows:
>>     spi-tx-support = <single | dual | quad>;
>>     spi-rx-support = <single | dual>;
>> 2. I set the master mode in probe of spi controller driver
>>     master->mode_bits = SPI_TX_DUAL | SPI_TX_QUAD | SPI_RX_DUAL;
>>
>> So do you think that there's any need to check the supported mode in 1
>> and 2? However, using 1 to set 2 seems OK.
>> What do you think of that?
>
> [Pekon]: Yes, (1) should be used to set (2).
> Ok fine.. if some spi_master needs DT, they can add it later..
> But by checking I meant following (same comments as on you V1 patch set)
> -------------------------


>> [Pekon]: Yes, for controller the controller_driver should parse or fill in
>> this info in the spi_master->mode.
>> But, still you need to add check while probing spi_device DT that
>> only common supported compatibilities get into spi_device.
>> Example: if spi_master->mode == SPI_TX_DUAL only
>> and of_spi_device() returns SPI_TX_QUAD, then you should return
>> back with error showing mis-match in controller and device capabilities.
>> So, spi_device capabilites should always be sub-set of spi_master capabilities.
>>
> -------------------------
>
Yes, I have got your comment and added that check in my previous
applied V3 patch. just as below

+ if ((xfer->tx_nbits == SPI_NBITS_DUAL) &&
+ !(spi->mode & (SPI_TX_DUAL | SPI_TX_QUAD)))
+ return -EINVAL;
+ if ((xfer->tx_nbits == SPI_NBITS_QUAD) &&
+ !(spi->mode & SPI_TX_QUAD))
+ return -EINVAL;


>> >> > >> +
>> >> > >> +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.
>> >>
>> >> with regards, pekon
>> >>
>> >> > >>  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