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]: 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?
>> > >> +
>> > >> +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
>> ______________________________________________________
>> Linux MTD discussion mailing list
>> http://lists.infradead.org/mailman/listinfo/linux-mtd/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
