| Message ID | 1377496080-14356-1-git-send-email-wangyuhang2014@gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
> > Signed-off-by: wangyuhang <wangyuhang2014@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> > + > +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 > 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
Hi, Pekon 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> Signed-off-by: wangyuhang <wangyuhang2014@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. >> + >> +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> >> 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
> Hi, Pekon > > 2013/8/26 Gupta, Pekon <pekon@ti.com>: > >> > >> Signed-off-by: wangyuhang <wangyuhang2014@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. 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
> > > Hi, Pekon > > > > 2013/8/26 Gupta, Pekon <pekon@ti.com>: > > >> > > >> Signed-off-by: wangyuhang <wangyuhang2014@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 ? 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 ----------------------------------------------- > > > [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 ? > > >> + > > >> +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/
Hi, Pekon 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> > Hi, Pekon >> > >> > 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> > >> >> > >> Signed-off-by: wangyuhang <wangyuhang2014@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]: 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/
Hi, Pekon 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> Hi, Pekon >> >> 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> >> >> Signed-off-by: wangyuhang <wangyuhang2014@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. 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. > 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
> > Hi, Pekon > > 2013/8/26 Gupta, Pekon <pekon@ti.com>: > >> > >> > Hi, Pekon > >> > > >> > 2013/8/26 Gupta, Pekon <pekon@ti.com>: > >> > >> > >> > >> Signed-off-by: wangyuhang <wangyuhang2014@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. > >> > > [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
> > Hi, Pekon > > 2013/8/26 Gupta, Pekon <pekon@ti.com>: > >> Hi, Pekon > >> > >> 2013/8/26 Gupta, Pekon <pekon@ti.com>: > >> >> > >> >> Signed-off-by: wangyuhang <wangyuhang2014@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
Hi, Pekon 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> Hi, Pekon >> >> 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> >> >> > Hi, Pekon >> >> > >> >> > 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> > >> >> >> > >> Signed-off-by: wangyuhang <wangyuhang2014@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
Hi, Pekon 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> Hi, Pekon >> >> 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> Hi, Pekon >> >> >> >> 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> >> >> >> >> Signed-off-by: wangyuhang <wangyuhang2014@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
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>; + +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. If a gpio chipselect is used for the SPI slave the gpio number will be passed via the cs_gpio
Add spi-tx-nbits and spi-rx-nbits for spi slave node. spi-tx-nbits:Number of bits used for MOSI(writting) spi-rx-nbits:Number of bits used for MISO(reading) Support for spi-tx/rx-nbits in SPI framework has been picked[1]. [1]: http://comments.gmane.org/gmane.linux.kernel.spi.devel/14420 Commit Id:f477b7fb13df2b843997559ff34e87d054ba6538 Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> --- Documentation/devicetree/bindings/spi/spi-bus.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+)