>
> Hi, Pekon
>
> 2013/8/26 Gupta, Pekon <pekon@xxxxxx>:
> >>
> >> > Hi, Pekon
> >> >
> >> > 2013/8/26 Gupta, Pekon <pekon@xxxxxx>:
> >> > >>
> >> > >> Signed-off-by: wangyuhang <wangyuhang2014@xxxxxxxxx>
> >> > >> ---
> >> > >> 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.
> >> > > [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.
>
-------------------------
> >> > >> +
> >> > >> +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
��.n��������+%������w��{.n����z�{��ܨ}���Ơz�j:+v�����w����ޙ��&�)ߡ�a����z�ޗ���ݢj��w�f
