Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1424766 > unrolled thread

[PATCH 0/5] SPI CS line logical change and s3c64xx code rework

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/5] SPI CS line logical change and s3c64xx code rework Andi Shyti <andi.shyti@samsung.com> - 2016-06-17 10:00 +0200
    [PATCH 5/5] spi: s3c63xx: simplify if statement in prepare_transfer  function Andi Shyti <andi.shyti@samsung.com> - 2016-06-17 10:10 +0200
    [PATCH 2/5] spi: s3c64xx: group the CS signalling writes in a single  function Andi Shyti <andi.shyti@samsung.com> - 2016-06-17 10:10 +0200
    [PATCH 4/5] spi: s3c64xx: do not configure the device twice Andi Shyti <andi.shyti@samsung.com> - 2016-06-17 10:10 +0200
    [PATCH 3/5] spi: s3c64xx: consider the case where the CS line is not  connected Andi Shyti <andi.shyti@samsung.com> - 2016-06-17 10:10 +0200
    [PATCH 1/5] spi: do not fail if the CS line is not connected Andi Shyti <andi.shyti@samsung.com> - 2016-06-17 10:10 +0200
      Re: [PATCH 1/5] spi: do not fail if the CS line is not connected Mark Brown <broonie@kernel.org> - 2016-06-17 12:50 +0200
        Re: [PATCH 1/5] spi: do not fail if the CS line is not connected Andi Shyti <andi@etezian.org> - 2016-06-17 13:50 +0200
          Re: [PATCH 1/5] spi: do not fail if the CS line is not connected Mark Brown <broonie@kernel.org> - 2016-06-17 14:30 +0200
            Re: [PATCH 1/5] spi: do not fail if the CS line is not connected Andi Shyti <andi@etezian.org> - 2016-06-19 08:10 +0200

#1424766 — [PATCH 0/5] SPI CS line logical change and s3c64xx code rework

Hi,

the main goal of the patchset is to support SPI cnnected device
without CS line link.

The first patch removes two 'if' conditions in the spi core
framework, leaving this way the responsibility for checking
whether the CS line is connected to the specific SPI driver.
Because the spi core doesn't allow a disconnected CS line, the
drivers were assigning -1 or 255 as number of CS lines, which is
not correct also considering the variables type.

The second and third patches enable this case in the s3c64xx
driver.

The fourth and fith patches are code rework related to the
s3c64xx driver.

Andi

Andi Shyti (5):
  spi: do not fail if the CS line is not connected
  spi: s3c64xx: group the CS signalling writes in a single function
  spi: s3c64xx: consider the case where the CS line is not connected
  spi: s3c64xx: do not configure the device twice
  spi: s3c63xx: simplify if statement in prepare_transfer function

 drivers/spi/spi-s3c64xx.c | 104 ++++++++++++++++++++++++----------------------
 drivers/spi/spi.c         |  15 ++++---
 2 files changed, 62 insertions(+), 57 deletions(-)

-- 
2.8.1

[toc] | [next] | [standalone]

#1424783 — [PATCH 5/5] spi: s3c63xx: simplify if statement in prepare_transfer function

The whole function is in an if statement ("!is_polling(sdd)").
Check the opposite of that statement at the beginning and exit,
this way we can have a level less of indentation.

Remove the goto paths as they are redundant.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 50 +++++++++++++++++++++--------------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index a98930a..c45c88c 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -341,38 +341,32 @@ static int s3c64xx_spi_prepare_transfer(struct spi_master *spi)
 	dma_filter_fn filter = sdd->cntrlr_info->filter;
 	struct device *dev = &sdd->pdev->dev;
 	dma_cap_mask_t mask;
-	int ret;
 
-	if (!is_polling(sdd)) {
-		dma_cap_zero(mask);
-		dma_cap_set(DMA_SLAVE, mask);
-
-		/* Acquire DMA channels */
-		sdd->rx_dma.ch = dma_request_slave_channel_compat(mask, filter,
-				   sdd->cntrlr_info->dma_rx, dev, "rx");
-		if (!sdd->rx_dma.ch) {
-			dev_err(dev, "Failed to get RX DMA channel\n");
-			ret = -EBUSY;
-			goto out;
-		}
-		spi->dma_rx = sdd->rx_dma.ch;
-
-		sdd->tx_dma.ch = dma_request_slave_channel_compat(mask, filter,
-				   sdd->cntrlr_info->dma_tx, dev, "tx");
-		if (!sdd->tx_dma.ch) {
-			dev_err(dev, "Failed to get TX DMA channel\n");
-			ret = -EBUSY;
-			goto out_rx;
-		}
-		spi->dma_tx = sdd->tx_dma.ch;
+	if (is_polling(sdd))
+		return 0;
+
+	dma_cap_zero(mask);
+	dma_cap_set(DMA_SLAVE, mask);
+
+	/* Acquire DMA channels */
+	sdd->rx_dma.ch = dma_request_slave_channel_compat(mask, filter,
+			   sdd->cntrlr_info->dma_rx, dev, "rx");
+	if (!sdd->rx_dma.ch) {
+		dev_err(dev, "Failed to get RX DMA channel\n");
+		return -EBUSY;
 	}
+	spi->dma_rx = sdd->rx_dma.ch;
 
-	return 0;
+	sdd->tx_dma.ch = dma_request_slave_channel_compat(mask, filter,
+			   sdd->cntrlr_info->dma_tx, dev, "tx");
+	if (!sdd->tx_dma.ch) {
+		dev_err(dev, "Failed to get TX DMA channel\n");
+		dma_release_channel(sdd->rx_dma.ch);
+		return -EBUSY;
+	}
+	spi->dma_tx = sdd->tx_dma.ch;
 
-out_rx:
-	dma_release_channel(sdd->rx_dma.ch);
-out:
-	return ret;
+	return 0;
 }
 
 static int s3c64xx_spi_unprepare_transfer(struct spi_master *spi)
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1424784 — [PATCH 2/5] spi: s3c64xx: group the CS signalling writes in a single function

To enable/disable the CS line, the driver performs a writel in
the S3C64XX_SPI_SLAVE_SEL registers. Put together all the writes
in that register in a single function.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 5a76a50..972367d 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -310,6 +310,28 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
 	dma_async_issue_pending(dma->ch);
 }
 
+static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
+{
+	struct s3c64xx_spi_driver_data *sdd =
+					spi_master_get_devdata(spi->master);
+
+	if (enable) {
+		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO)) {
+			writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+		} else {
+			u32 ssel = readl(sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+
+			ssel |= (S3C64XX_SPI_SLAVE_AUTO |
+						S3C64XX_SPI_SLAVE_NSC_CNT_2);
+			writel(ssel, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+		}
+	} else {
+		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
+		writel(S3C64XX_SPI_SLAVE_SIG_INACT,
+		sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+	}
+}
+
 static int s3c64xx_spi_prepare_transfer(struct spi_master *spi)
 {
 	struct s3c64xx_spi_driver_data *sdd = spi_master_get_devdata(spi);
@@ -706,12 +728,7 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 	enable_datapath(sdd, spi, xfer, use_dma);
 
 	/* Start the signals */
-	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-		writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
-	else
-		writel(readl(sdd->regs + S3C64XX_SPI_SLAVE_SEL)
-			| S3C64XX_SPI_SLAVE_AUTO | S3C64XX_SPI_SLAVE_NSC_CNT_2,
-			sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+	s3c64xx_spi_set_cs(spi, true);
 
 	spin_unlock_irqrestore(&sdd->lock, flags);
 
@@ -861,16 +878,15 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 
 	pm_runtime_mark_last_busy(&sdd->pdev->dev);
 	pm_runtime_put_autosuspend(&sdd->pdev->dev);
-	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-		writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+	s3c64xx_spi_set_cs(spi, false);
+
 	return 0;
 
 setup_exit:
 	pm_runtime_mark_last_busy(&sdd->pdev->dev);
 	pm_runtime_put_autosuspend(&sdd->pdev->dev);
 	/* setup() returns with device de-selected */
-	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-		writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+	s3c64xx_spi_set_cs(spi, false);
 
 	if (gpio_is_valid(spi->cs_gpio))
 		gpio_free(spi->cs_gpio);
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1424788 — [PATCH 4/5] spi: s3c64xx: do not configure the device twice

At the start of the transfer, the spi_config function is called
twice, the first time when the 3c64xx_spi_prepare_message is
called and the second time with the s3c64xx_spi_transfer_one,
both called from the spi framework.

Remove the call from the prepare message as at that point we
don't have the imformation about "bit per word" and frequency.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 3d54c5f..a98930a 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -676,16 +676,6 @@ static int s3c64xx_spi_prepare_message(struct spi_master *master,
 	struct spi_device *spi = msg->spi;
 	struct s3c64xx_spi_csinfo *cs = spi->controller_data;
 
-	/* If Master's(controller) state differs from that needed by Slave */
-	if (sdd->cur_speed != spi->max_speed_hz
-			|| sdd->cur_mode != spi->mode
-			|| sdd->cur_bpw != spi->bits_per_word) {
-		sdd->cur_bpw = spi->bits_per_word;
-		sdd->cur_speed = spi->max_speed_hz;
-		sdd->cur_mode = spi->mode;
-		s3c64xx_spi_config(sdd);
-	}
-
 	/* Configure feedback delay */
 	writel(cs->fb_delay & 0x3, sdd->regs + S3C64XX_SPI_FB_CLK);
 
@@ -712,6 +702,7 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 	if (bpw != sdd->cur_bpw || speed != sdd->cur_speed) {
 		sdd->cur_bpw = bpw;
 		sdd->cur_speed = speed;
+		sdd->cur_mode = spi->mode;
 		s3c64xx_spi_config(sdd);
 	}
 
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1424789 — [PATCH 3/5] spi: s3c64xx: consider the case where the CS line is not connected

When the CS line is not connected, it is not needed to enable or
disable the chip selection functionality from the s3c64xx
devices. Keep et enable already at the initialization (by writing
'0' in the S3C64XX_SPI_SLAVE_SEL register) and never disable it.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 972367d..3d54c5f 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -315,6 +315,9 @@ static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
 	struct s3c64xx_spi_driver_data *sdd =
 					spi_master_get_devdata(spi->master);
 
+	if (!sdd->master->num_chipselect)
+		return;
+
 	if (enable) {
 		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO)) {
 			writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
@@ -960,7 +963,9 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
 
 	sdd->cur_speed = 0;
 
-	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
+	if (!sdd->master->num_chipselect)
+		writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+	else if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
 		writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 
 	/* Disable Interrupts - we use Polling if not DMA mode */
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1424792 — [PATCH 1/5] spi: do not fail if the CS line is not connected

Some SPI connected devices do not have any CS line connected as
some devices are alway enabled. Indeed, until now, a common
workaround was to assign to num_chipselect a -1 value or 255
(num_chipselect is unsigned).

In this case do not fail and defer to the SPI device drivers the
responsibility to check whether the num-cs is '0'.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
---
 drivers/spi/spi.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 77e6e45..f22dc27 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -504,8 +504,13 @@ int spi_add_device(struct spi_device *spi)
 	struct device *dev = master->dev.parent;
 	int status;
 
-	/* Chipselects are numbered 0..max; validate. */
-	if (spi->chip_select >= master->num_chipselect) {
+	/*
+	 * Chipselects are numbered 0..max; validate.
+	 * If there is no chip select (i.e. num_chipselect == 0),
+	 * the comparison doesn't make sense.
+	 */
+	if (master->num_chipselect &&
+				spi->chip_select >= master->num_chipselect) {
 		dev_err(dev, "cs%d >= max %d\n",
 			spi->chip_select,
 			master->num_chipselect);
@@ -1851,12 +1856,6 @@ int spi_register_master(struct spi_master *master)
 	if (status)
 		return status;
 
-	/* even if it's just one always-selected device, there must
-	 * be at least one chipselect
-	 */
-	if (master->num_chipselect == 0)
-		return -EINVAL;
-
 	if ((master->bus_num < 0) && master->dev.of_node)
 		master->bus_num = of_alias_get_id(master->dev.of_node, "spi");
 
-- 
2.8.1

[toc] | [prev] | [next] | [standalone]

#1424923 — Re: [PATCH 1/5] spi: do not fail if the CS line is not connected

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jun 17, 2016 at 04:57:21PM +0900, Andi Shyti wrote:
> Some SPI connected devices do not have any CS line connected as
> some devices are alway enabled. Indeed, until now, a common
> workaround was to assign to num_chipselect a -1 value or 255
> (num_chipselect is unsigned).
> 
> In this case do not fail and defer to the SPI device drivers the
> responsibility to check whether the num-cs is '0'.

A SPI controller always has one chip seelct, it may not be controllable
but it's at least logically present.

[toc] | [prev] | [next] | [standalone]

#1424972 — Re: [PATCH 1/5] spi: do not fail if the CS line is not connected

Hi Mark,

> > Some SPI connected devices do not have any CS line connected as
> > some devices are alway enabled. Indeed, until now, a common
> > workaround was to assign to num_chipselect a -1 value or 255
> > (num_chipselect is unsigned).
> > 
> > In this case do not fail and defer to the SPI device drivers the
> > responsibility to check whether the num-cs is '0'.
> 
> A SPI controller always has one chip seelct, it may not be controllable
> but it's at least logically present.

This is true, but there are cases where the CS is not connected
and this case needs to be treated separately to allow the device
to work.

This is the case of:

./drivers/spi/spi-mpc52xx-psc.c:391: master->num_chipselect = 255;
./drivers/spi/spi-oc-tiny.c:256:        master->num_chipselect = 255;

and

./drivers/spi/spi-pxa2xx-pci.c:64:              .num_chipselect = -1,
./drivers/spi/spi-bcm2835aux.c:436:     master->num_chipselect = -1;

that in my opinion make even less sense. The latter is completely
broken, as num_chipselect is an u16 variable. Because of this I
had to do the same in a driver that I will send next (and perhaps
it will be rejected).

So, IMHO, it's better to not be that strong on num_chipselect
being non zero, as there are cases where the CS _line_ is not
connected.

Andi

[toc] | [prev] | [next] | [standalone]

#1425002 — Re: [PATCH 1/5] spi: do not fail if the CS line is not connected

[Multipart message — attachments visible in raw view] — view raw

On Fri, Jun 17, 2016 at 08:36:22PM +0900, Andi Shyti wrote:

> > > In this case do not fail and defer to the SPI device drivers the
> > > responsibility to check whether the num-cs is '0'.

> > A SPI controller always has one chip seelct, it may not be controllable
> > but it's at least logically present.

> This is true, but there are cases where the CS is not connected
> and this case needs to be treated separately to allow the device
> to work.

In what way?  It is just as easy for a device with no physical chip
select to have a logical chip select of 0 that it does nothing with as
it is for that device to handle any other number.

> This is the case of:

> ./drivers/spi/spi-mpc52xx-psc.c:391: master->num_chipselect = 255;
> ./drivers/spi/spi-oc-tiny.c:256:        master->num_chipselect = 255;

> ./drivers/spi/spi-pxa2xx-pci.c:64:              .num_chipselect = -1,
> ./drivers/spi/spi-bcm2835aux.c:436:     master->num_chipselect = -1;

These need fixing.

[toc] | [prev] | [next] | [standalone]

#1425937 — Re: [PATCH 1/5] spi: do not fail if the CS line is not connected

Hi Mark,

> > > > In this case do not fail and defer to the SPI device drivers the
> > > > responsibility to check whether the num-cs is '0'.
> 
> > > A SPI controller always has one chip seelct, it may not be controllable
> > > but it's at least logically present.
> 
> > This is true, but there are cases where the CS is not connected
> > and this case needs to be treated separately to allow the device
> > to work.
> 
> In what way?  It is just as easy for a device with no physical chip
> select to have a logical chip select of 0 that it does nothing with as
> it is for that device to handle any other number.

That is indeed my case: the s3c64xx doesn't send anything, unless
I manually enable CS (from the next patches I need to write '0'
in the CS register). But I need smoething to tell to the
device that the CS line is not connected, for example a flag in
the DTS.

It comes natural to me to set "num-cs = <0>" instead of defining
a new property. In this case I will consider that there is no CS
line, even though there is a CS controller.

Thanks,
Andi

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web