* [PATCH 1/2] spi: dual and quad support(device tree) @ 2013-08-26 5:47 wangyuhang 2013-08-26 5:48 ` [PATCH 2/2] " wangyuhang 2013-08-26 6:59 ` [PATCH 1/2] " Gupta, Pekon 0 siblings, 2 replies; 16+ messages in thread From: wangyuhang @ 2013-08-26 5:47 UTC (permalink / raw) To: broonie, linux-spi, devicetree, linux-mtd Cc: sourav.poddar, pekon, wangyuhang 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(+) 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 -- 1.7.9.5 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] spi: dual and quad support(device tree) 2013-08-26 5:47 [PATCH 1/2] spi: dual and quad support(device tree) wangyuhang @ 2013-08-26 5:48 ` wangyuhang 2013-08-26 7:18 ` Gupta, Pekon 2013-08-26 6:59 ` [PATCH 1/2] " Gupta, Pekon 1 sibling, 1 reply; 16+ messages in thread From: wangyuhang @ 2013-08-26 5:48 UTC (permalink / raw) To: broonie, linux-spi, devicetree, linux-mtd Cc: sourav.poddar, pekon, wangyuhang fix one thing in patch: commit id:f477b7fb13df2b843997559ff34e87d054ba6538 change property spi-tx-nbits and spi-rx-nbits to optional. If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode should be regarded as SINGLE, not return an error. Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> --- drivers/spi/spi.c | 68 +++++++++++++++++++++++++---------------------------- 1 file changed, 32 insertions(+), 36 deletions(-) diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 50f7fc3..46a55f0 100644 --- a/drivers/spi/spi.c +++ b/drivers/spi/spi.c @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct spi_master *master) /* Device DUAL/QUAD mode */ prop = of_get_property(nc, "spi-tx-nbits", &len); if (!prop || len < sizeof(*prop)) { - dev_err(&master->dev, "%s has no 'spi-tx-nbits' property\n", - nc->full_name); - spi_dev_put(spi); - continue; - } - switch (be32_to_cpup(prop)) { - case SPI_NBITS_SINGLE: - break; - case SPI_NBITS_DUAL: - spi->mode |= SPI_TX_DUAL; - break; - case SPI_NBITS_QUAD: - spi->mode |= SPI_TX_QUAD; - break; - default: - dev_err(&master->dev, "spi-tx-nbits value is not supported\n"); - spi_dev_put(spi); - continue; + /* set tx mode in SINGLE as default */ + } else { + switch (be32_to_cpup(prop)) { + case SPI_NBITS_SINGLE: + break; + case SPI_NBITS_DUAL: + spi->mode |= SPI_TX_DUAL; + break; + case SPI_NBITS_QUAD: + spi->mode |= SPI_TX_QUAD; + break; + default: + dev_err(&master->dev, + "spi-tx-nbits value is not supported\n"); + spi_dev_put(spi); + continue; } prop = of_get_property(nc, "spi-rx-nbits", &len); if (!prop || len < sizeof(*prop)) { - dev_err(&master->dev, "%s has no 'spi-rx-nbits' property\n", - nc->full_name); - spi_dev_put(spi); - continue; - } - switch (be32_to_cpup(prop)) { - case SPI_NBITS_SINGLE: - break; - case SPI_NBITS_DUAL: - spi->mode |= SPI_RX_DUAL; - break; - case SPI_NBITS_QUAD: - spi->mode |= SPI_RX_QUAD; - break; - default: - dev_err(&master->dev, "spi-rx-nbits value is not supported\n"); - spi_dev_put(spi); - continue; + /* set rx mode in SINGLE as default */ + } else { + switch (be32_to_cpup(prop)) { + case SPI_NBITS_SINGLE: + break; + case SPI_NBITS_DUAL: + spi->mode |= SPI_RX_DUAL; + break; + case SPI_NBITS_QUAD: + spi->mode |= SPI_RX_QUAD; + break; + default: + dev_err(&master->dev, + "spi-rx-nbits value is not supported\n"); + spi_dev_put(spi); + continue; } /* Device speed */ -- 1.7.9.5 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply related [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] spi: dual and quad support(device tree) 2013-08-26 5:48 ` [PATCH 2/2] " wangyuhang @ 2013-08-26 7:18 ` Gupta, Pekon 2013-08-26 12:50 ` Gupta, Pekon 2013-08-27 6:11 ` yuhang wang 0 siblings, 2 replies; 16+ messages in thread From: Gupta, Pekon @ 2013-08-26 7:18 UTC (permalink / raw) To: wangyuhang Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org > > fix one thing in patch: > commit id:f477b7fb13df2b843997559ff34e87d054ba6538 > > change property spi-tx-nbits and spi-rx-nbits to optional. > If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode > should be regarded as SINGLE, not return an error. > > Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> > --- > drivers/spi/spi.c | 68 +++++++++++++++++++++++++-------------------------- > -- > 1 file changed, 32 insertions(+), 36 deletions(-) > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > index 50f7fc3..46a55f0 100644 > --- a/drivers/spi/spi.c > +++ b/drivers/spi/spi.c > @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct > spi_master *master) > /* Device DUAL/QUAD mode */ > prop = of_get_property(nc, "spi-tx-nbits", &len); > if (!prop || len < sizeof(*prop)) { > - dev_err(&master->dev, "%s has no 'spi-tx-nbits' > property\n", > - nc->full_name); > - spi_dev_put(spi); > - continue; > - } > - switch (be32_to_cpup(prop)) { > - case SPI_NBITS_SINGLE: > - break; > - case SPI_NBITS_DUAL: > - spi->mode |= SPI_TX_DUAL; > - break; > - case SPI_NBITS_QUAD: > - spi->mode |= SPI_TX_QUAD; > - break; > - default: > - dev_err(&master->dev, "spi-tx-nbits value is not > supported\n"); > - spi_dev_put(spi); > - continue; > + /* set tx mode in SINGLE as default */ [Pekon]: I think you should explicitly set, and not trust spi->mode defaults. spi->mode |= SPI_TX_SINGLE; > + } else { > + switch (be32_to_cpup(prop)) { > + case SPI_NBITS_SINGLE: > + break; > + case SPI_NBITS_DUAL: > + spi->mode |= SPI_TX_DUAL; > + break; > + case SPI_NBITS_QUAD: > + spi->mode |= SPI_TX_QUAD; > + break; [Pekon]: This is incomplete definition, as indicated in [Patch 1/2] comments. SPI_NBITS_QUAD does not clearly indicates whether SPI_TX_DUAL mode is also supported or not. That is.. spi->mode |= SPI_TX_QUAD; or spi->mode |= SPI_TX_QUAD | SPI_TX_DUAL; > + default: > + dev_err(&master->dev, > + "spi-tx-nbits value is not > supported\n"); > + spi_dev_put(spi); > + continue; > } > > prop = of_get_property(nc, "spi-rx-nbits", &len); > if (!prop || len < sizeof(*prop)) { > - dev_err(&master->dev, "%s has no 'spi-rx-nbits' > property\n", > - nc->full_name); > - spi_dev_put(spi); > - continue; > - } > - switch (be32_to_cpup(prop)) { > - case SPI_NBITS_SINGLE: > - break; > - case SPI_NBITS_DUAL: > - spi->mode |= SPI_RX_DUAL; > - break; > - case SPI_NBITS_QUAD: > - spi->mode |= SPI_RX_QUAD; > - break; > - default: > - dev_err(&master->dev, "spi-rx-nbits value is not > supported\n"); > - spi_dev_put(spi); > - continue; > + /* set rx mode in SINGLE as default */ > + } else { > + switch (be32_to_cpup(prop)) { > + case SPI_NBITS_SINGLE: > + break; > + case SPI_NBITS_DUAL: > + spi->mode |= SPI_RX_DUAL; > + break; > + case SPI_NBITS_QUAD: > + spi->mode |= SPI_RX_QUAD; > + break; > + default: > + dev_err(&master->dev, > + "spi-rx-nbits value is not > supported\n"); > + spi_dev_put(spi); > + continue; > } > > /* Device speed */ > -- > 1.7.9.5 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] spi: dual and quad support(device tree) 2013-08-26 7:18 ` Gupta, Pekon @ 2013-08-26 12:50 ` Gupta, Pekon 2013-08-27 6:11 ` yuhang wang 1 sibling, 0 replies; 16+ messages in thread From: Gupta, Pekon @ 2013-08-26 12:50 UTC (permalink / raw) To: wangyuhang Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org > > > > > fix one thing in patch: > > commit id:f477b7fb13df2b843997559ff34e87d054ba6538 > > > > change property spi-tx-nbits and spi-rx-nbits to optional. > > If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode > > should be regarded as SINGLE, not return an error. > > > > Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> > > --- > > drivers/spi/spi.c | 68 +++++++++++++++++++++++++------------------------ > -- > > -- > > 1 file changed, 32 insertions(+), 36 deletions(-) > > > > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > > index 50f7fc3..46a55f0 100644 > > --- a/drivers/spi/spi.c > > +++ b/drivers/spi/spi.c > > @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct > > spi_master *master) > > /* Device DUAL/QUAD mode */ > > prop = of_get_property(nc, "spi-tx-nbits", &len); > > if (!prop || len < sizeof(*prop)) { > > - dev_err(&master->dev, "%s has no 'spi-tx-nbits' > > property\n", > > - nc->full_name); > > - spi_dev_put(spi); > > - continue; > > - } > > - switch (be32_to_cpup(prop)) { > > - case SPI_NBITS_SINGLE: > > - break; > > - case SPI_NBITS_DUAL: > > - spi->mode |= SPI_TX_DUAL; > > - break; > > - case SPI_NBITS_QUAD: > > - spi->mode |= SPI_TX_QUAD; > > - break; > > - default: > > - dev_err(&master->dev, "spi-tx-nbits value is not > > supported\n"); > > - spi_dev_put(spi); > > - continue; > > + /* set tx mode in SINGLE as default */ [Pekon]: I think you should explicitly set, and not trust spi->mode defaults. spi->mode |= SPI_TX_SINGLE; > > > + } else { > > + switch (be32_to_cpup(prop)) { > > + case SPI_NBITS_SINGLE: > > + break; > > + case SPI_NBITS_DUAL: > > + spi->mode |= SPI_TX_DUAL; > > + break; > > + case SPI_NBITS_QUAD: > > + spi->mode |= SPI_TX_QUAD; > > + break; > [Pekon]: And there please check for spi_master->mode_bits before setting spi_device->mode. spi_device->mode should be sub-set of spi_master->mode_bits. Example: switch (be32_to_cpup(prop)) { case SPI_NBITS_SINGLE: if (spi_master->mode_bits & SPI_TX_SINGLE) spi-mode |= SPI_TX_SINGLE; else pr_err("SPI controller does not support SPI_TX_SINGLE\n"); break; case SPI_NBITS_DUAL: if (spi_master->mode_bits & SPI_TX_DUAL) spi-mode |= SPI_TX_DUAL; else pr_err("SPI controller does not support SPI_TX_DUAL\n"); break; case SPI_NBITS_QUAD: if (spi_master->mode_bits & SPI_TX_QUAD) spi-mode |= SPI_TX_QUAD; else pr_err("SPI controller does not support SPI_TX_QUAD\n"); break; } This way you will do initial compatibility check between the device and controller right at the probe, and you don't have to worry about it later. > spi->mode |= SPI_TX_QUAD; > or > spi->mode |= SPI_TX_QUAD | SPI_TX_DUAL; > > > > + default: > > + dev_err(&master->dev, > > + "spi-tx-nbits value is not > > supported\n"); > > + spi_dev_put(spi); > > + continue; > > } > > > > prop = of_get_property(nc, "spi-rx-nbits", &len); > > if (!prop || len < sizeof(*prop)) { > > - dev_err(&master->dev, "%s has no 'spi-rx-nbits' > > property\n", > > - nc->full_name); > > - spi_dev_put(spi); > > - continue; > > - } > > - switch (be32_to_cpup(prop)) { > > - case SPI_NBITS_SINGLE: > > - break; > > - case SPI_NBITS_DUAL: > > - spi->mode |= SPI_RX_DUAL; > > - break; > > - case SPI_NBITS_QUAD: > > - spi->mode |= SPI_RX_QUAD; > > - break; > > - default: > > - dev_err(&master->dev, "spi-rx-nbits value is not > > supported\n"); > > - spi_dev_put(spi); > > - continue; > > + /* set rx mode in SINGLE as default */ > > + } else { > > + switch (be32_to_cpup(prop)) { > > + case SPI_NBITS_SINGLE: > > + break; > > + case SPI_NBITS_DUAL: > > + spi->mode |= SPI_RX_DUAL; > > + break; > > + case SPI_NBITS_QUAD: > > + spi->mode |= SPI_RX_QUAD; > > + break; > > + default: > > + dev_err(&master->dev, > > + "spi-rx-nbits value is not > > supported\n"); > > + spi_dev_put(spi); > > + continue; > > } > > > > /* Device speed */ > > -- > > 1.7.9.5 > with regards, pekon ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] spi: dual and quad support(device tree) 2013-08-26 7:18 ` Gupta, Pekon 2013-08-26 12:50 ` Gupta, Pekon @ 2013-08-27 6:11 ` yuhang wang 2013-08-27 6:31 ` Gupta, Pekon 1 sibling, 1 reply; 16+ messages in thread From: yuhang wang @ 2013-08-27 6:11 UTC (permalink / raw) To: Gupta, Pekon Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org Hi, Pekon 2013/8/26 Gupta, Pekon <pekon@ti.com>: >> >> fix one thing in patch: >> commit id:f477b7fb13df2b843997559ff34e87d054ba6538 >> >> change property spi-tx-nbits and spi-rx-nbits to optional. >> If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode >> should be regarded as SINGLE, not return an error. >> >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> >> --- >> drivers/spi/spi.c | 68 +++++++++++++++++++++++++-------------------------- >> -- >> 1 file changed, 32 insertions(+), 36 deletions(-) >> >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c >> index 50f7fc3..46a55f0 100644 >> --- a/drivers/spi/spi.c >> +++ b/drivers/spi/spi.c >> @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct >> spi_master *master) >> /* Device DUAL/QUAD mode */ >> prop = of_get_property(nc, "spi-tx-nbits", &len); >> if (!prop || len < sizeof(*prop)) { >> - dev_err(&master->dev, "%s has no 'spi-tx-nbits' >> property\n", >> - nc->full_name); >> - spi_dev_put(spi); >> - continue; >> - } >> - switch (be32_to_cpup(prop)) { >> - case SPI_NBITS_SINGLE: >> - break; >> - case SPI_NBITS_DUAL: >> - spi->mode |= SPI_TX_DUAL; >> - break; >> - case SPI_NBITS_QUAD: >> - spi->mode |= SPI_TX_QUAD; >> - break; >> - default: >> - dev_err(&master->dev, "spi-tx-nbits value is not >> supported\n"); >> - spi_dev_put(spi); >> - continue; >> + /* set tx mode in SINGLE as default */ > [Pekon]: I think you should explicitly set, and not trust spi->mode defaults. > spi->mode |= SPI_TX_SINGLE; > Now @spi_device->mode [bit8: SPI_TX_DUAL] [bit9: SPI_TX_QUAD] [bit10: SPI_RX_DUAL] [bit11: SPI_RX_QUAD]. Because to every spi controller, SPI SINGLE must be supported, so I did not add that as a mode. My initial thought is if user don't select SPI_TX_DUAL and SPI_TX_QUAD, then it is TX_SINGLE mode[bit8: 0 bit9: 0]. Now 2 approaches as follows: 1) #define SPI_TX_SINGLE ~(SPI_TX_DUAL | SPI_TX_QUAD) case SPI_NBITS_SINGLE: spi->mode & SPI_TX_SINGLE; break; 2)Adding a new bit in mode to specify SPI_SINGLE. But this bit also should be reflected to @master->mode_bits. To the probe of each spi controller driver, it is necessary to do "master->mode_bits |= SPI_TX_SINGLE | SPI_RX_SINGLE;", in such case, it's not like a selected mode, it's like a compulsary condition. Since all controller do support, there's no need to make it a mode. So I prefer the previous correction, what's your idea. Best regards ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 2/2] spi: dual and quad support(device tree) 2013-08-27 6:11 ` yuhang wang @ 2013-08-27 6:31 ` Gupta, Pekon 0 siblings, 0 replies; 16+ messages in thread From: Gupta, Pekon @ 2013-08-27 6:31 UTC (permalink / raw) To: yuhang wang Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org > > Hi, Pekon > > 2013/8/26 Gupta, Pekon <pekon@ti.com>: > >> > >> fix one thing in patch: > >> commit id:f477b7fb13df2b843997559ff34e87d054ba6538 > >> > >> change property spi-tx-nbits and spi-rx-nbits to optional. > >> If User don't set spi-tx-nbits or spi-rx-nbits, spi device mode > >> should be regarded as SINGLE, not return an error. > >> > >> Signed-off-by: wangyuhang <wangyuhang2014@gmail.com> > >> --- > >> drivers/spi/spi.c | 68 +++++++++++++++++++++++++---------------------- > ---- > >> -- > >> 1 file changed, 32 insertions(+), 36 deletions(-) > >> > >> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c > >> index 50f7fc3..46a55f0 100644 > >> --- a/drivers/spi/spi.c > >> +++ b/drivers/spi/spi.c > >> @@ -872,46 +872,42 @@ static void of_register_spi_devices(struct > >> spi_master *master) > >> /* Device DUAL/QUAD mode */ > >> prop = of_get_property(nc, "spi-tx-nbits", &len); > >> if (!prop || len < sizeof(*prop)) { > >> - dev_err(&master->dev, "%s has no 'spi-tx-nbits' > >> property\n", > >> - nc->full_name); > >> - spi_dev_put(spi); > >> - continue; > >> - } > >> - switch (be32_to_cpup(prop)) { > >> - case SPI_NBITS_SINGLE: > >> - break; > >> - case SPI_NBITS_DUAL: > >> - spi->mode |= SPI_TX_DUAL; > >> - break; > >> - case SPI_NBITS_QUAD: > >> - spi->mode |= SPI_TX_QUAD; > >> - break; > >> - default: > >> - dev_err(&master->dev, "spi-tx-nbits value is not > >> supported\n"); > >> - spi_dev_put(spi); > >> - continue; > >> + /* set tx mode in SINGLE as default */ > > [Pekon]: I think you should explicitly set, and not trust spi->mode defaults. > > spi->mode |= SPI_TX_SINGLE; > > > Now @spi_device->mode [bit8: SPI_TX_DUAL] [bit9: SPI_TX_QUAD] [bit10: > SPI_RX_DUAL] [bit11: SPI_RX_QUAD]. Because to every spi controller, > SPI SINGLE must be supported, so I did not add that as a mode. My > initial thought is if user don't select SPI_TX_DUAL and SPI_TX_QUAD, > then it is TX_SINGLE mode[bit8: 0 bit9: 0]. > Now 2 approaches as follows: > 1) #define SPI_TX_SINGLE ~(SPI_TX_DUAL | SPI_TX_QUAD) > case SPI_NBITS_SINGLE: > spi->mode & SPI_TX_SINGLE; > break; > 2)Adding a new bit in mode to specify SPI_SINGLE. But this bit also > should be reflected to @master->mode_bits. To the probe of each spi > controller driver, it is necessary to do "master->mode_bits |= > SPI_TX_SINGLE | SPI_RX_SINGLE;", in such case, it's not like a > selected mode, it's like a compulsary condition. Since all controller > do support, there's no need to make it a mode. > > So I prefer the previous correction, what's your idea. > [Pekon]: Ok makes sense.. (1) is preferred, as it is compatible to existing layout with regards, pekon ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 5:47 [PATCH 1/2] spi: dual and quad support(device tree) wangyuhang 2013-08-26 5:48 ` [PATCH 2/2] " wangyuhang @ 2013-08-26 6:59 ` Gupta, Pekon 2013-08-26 8:09 ` yuhang wang 1 sibling, 1 reply; 16+ messages in thread From: Gupta, Pekon @ 2013-08-26 6:59 UTC (permalink / raw) To: wangyuhang Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org > > 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 6:59 ` [PATCH 1/2] " Gupta, Pekon @ 2013-08-26 8:09 ` yuhang wang 2013-08-26 9:29 ` Gupta, Pekon 0 siblings, 1 reply; 16+ messages in thread From: yuhang wang @ 2013-08-26 8:09 UTC (permalink / raw) To: Gupta, Pekon Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 8:09 ` yuhang wang @ 2013-08-26 9:29 ` Gupta, Pekon 2013-08-26 9:45 ` Gupta, Pekon 2013-08-26 11:13 ` yuhang wang 0 siblings, 2 replies; 16+ messages in thread From: Gupta, Pekon @ 2013-08-26 9:29 UTC (permalink / raw) To: yuhang wang Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org > 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 9:29 ` Gupta, Pekon @ 2013-08-26 9:45 ` Gupta, Pekon 2013-08-26 10:41 ` yuhang wang 2013-08-26 11:13 ` yuhang wang 1 sibling, 1 reply; 16+ messages in thread From: Gupta, Pekon @ 2013-08-26 9:45 UTC (permalink / raw) To: yuhang wang Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org > > > 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/ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 9:45 ` Gupta, Pekon @ 2013-08-26 10:41 ` yuhang wang 2013-08-26 12:20 ` Gupta, Pekon 0 siblings, 1 reply; 16+ messages in thread From: yuhang wang @ 2013-08-26 10:41 UTC (permalink / raw) To: Gupta, Pekon Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org 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/ ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 10:41 ` yuhang wang @ 2013-08-26 12:20 ` Gupta, Pekon 2013-08-26 13:23 ` yuhang wang 0 siblings, 1 reply; 16+ messages in thread From: Gupta, Pekon @ 2013-08-26 12:20 UTC (permalink / raw) To: yuhang wang Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org > > 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 12:20 ` Gupta, Pekon @ 2013-08-26 13:23 ` yuhang wang 0 siblings, 0 replies; 16+ messages in thread From: yuhang wang @ 2013-08-26 13:23 UTC (permalink / raw) To: Gupta, Pekon Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 9:29 ` Gupta, Pekon 2013-08-26 9:45 ` Gupta, Pekon @ 2013-08-26 11:13 ` yuhang wang 2013-08-26 12:37 ` Gupta, Pekon 1 sibling, 1 reply; 16+ messages in thread From: yuhang wang @ 2013-08-26 11:13 UTC (permalink / raw) To: Gupta, Pekon Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 11:13 ` yuhang wang @ 2013-08-26 12:37 ` Gupta, Pekon 2013-08-26 13:35 ` yuhang wang 0 siblings, 1 reply; 16+ messages in thread From: Gupta, Pekon @ 2013-08-26 12:37 UTC (permalink / raw) To: yuhang wang Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org > > 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/2] spi: dual and quad support(device tree) 2013-08-26 12:37 ` Gupta, Pekon @ 2013-08-26 13:35 ` yuhang wang 0 siblings, 0 replies; 16+ messages in thread From: yuhang wang @ 2013-08-26 13:35 UTC (permalink / raw) To: Gupta, Pekon Cc: devicetree@vger.kernel.org, Poddar, Sourav, broonie@kernel.org, linux-mtd@lists.infradead.org, linux-spi@vger.kernel.org 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-08-27 6:31 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-26 5:47 [PATCH 1/2] spi: dual and quad support(device tree) wangyuhang 2013-08-26 5:48 ` [PATCH 2/2] " wangyuhang 2013-08-26 7:18 ` Gupta, Pekon 2013-08-26 12:50 ` Gupta, Pekon 2013-08-27 6:11 ` yuhang wang 2013-08-27 6:31 ` Gupta, Pekon 2013-08-26 6:59 ` [PATCH 1/2] " Gupta, Pekon 2013-08-26 8:09 ` yuhang wang 2013-08-26 9:29 ` Gupta, Pekon 2013-08-26 9:45 ` Gupta, Pekon 2013-08-26 10:41 ` yuhang wang 2013-08-26 12:20 ` Gupta, Pekon 2013-08-26 13:23 ` yuhang wang 2013-08-26 11:13 ` yuhang wang 2013-08-26 12:37 ` Gupta, Pekon 2013-08-26 13:35 ` yuhang wang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).