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

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

[Andi Shyti]

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

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

[Andi Shyti]

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

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

[Andi Shyti]

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

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

[Andi Shyti]

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

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

[Andi Shyti]

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

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

[Andi Shyti]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 527 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Andi Shyti]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Andi Shyti]

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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Sun, Jun 19, 2016 at 03:09:02PM +0900, Andi Shyti wrote:

> > > 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.

I'm sorry but I just don't understand what you're saying here.  You are
saying that there is no chip select but you need to enable the chip
select?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Andi Shyti]

Hi Mark,

> > > > 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.
> 
> I'm sorry but I just don't understand what you're saying here.  You are
> saying that there is no chip select but you need to enable the chip
> select?

yes, sorry, I have to admit that didn't explain myself properly,
but code speaks better than words.

What I meant is that if we do not like num-cs = <0>, the
unlinked CS line can be handled only this way (case of the
s3c64xx driver):



diff --git a/Documentation/devicetree/bindings/spi/spi-samsung.txt b/Documentation/devicetree/bindings/spi/spi-samsung.txt
index 6dbdeb3..62f19b6 100644
--- a/Documentation/devicetree/bindings/spi/spi-samsung.txt
+++ b/Documentation/devicetree/bindings/spi/spi-samsung.txt
@@ -40,6 +40,9 @@ Optional Board Specific Properties:
 
 - cs-gpios: should specify GPIOs used for chipselects (see spi-bus.txt)
 
+- broken-cs: the CS line is disconnected, therefore the device should not wait
+  for the CS protocol to be established
+
 SPI Controller specific data in SPI slave nodes:
 
 - The spi slave nodes should provide the following information which is required
diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 972367d..5e6ad59 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->cntrlr_info->broken_cs)
+               return;
+
        if (enable) {
                if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO)) {
                        writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);

[ ... ]

@@ -1015,6 +1020,8 @@ static struct s3c64xx_spi_info *s3c64xx_spi_parse_dt(struct device *dev)
                sci->num_cs = temp;
        }
 
+       sci->broken_cs = of_property_read_bool(dev->of_node, "broken-cs");
+
        return sci;
 }
 #else
diff --git a/include/linux/platform_data/spi-s3c64xx.h b/include/linux/platform_data/spi-s3c64xx.h
index fb5625b..75793b9 100644
--- a/include/linux/platform_data/spi-s3c64xx.h
+++ b/include/linux/platform_data/spi-s3c64xx.h
@@ -38,6 +38,7 @@ struct s3c64xx_spi_csinfo {
 struct s3c64xx_spi_info {
        int src_clk_nr;
        int num_cs;
+       bool broken_cs;
        int (*cfg_gpio)(void);
        dma_filter_fn filter;
        void *dma_tx;



Which is not something I like, because it means adding a new
flag in the dts.

What I want to suggest, instead, is to slightly change the logic
behind the num-cs property: i.e. if "num-cs = <0>", doesn't
necessarily mean that we don't have a CS controller, but, while
we can have as many as we wish, non of them is connected.

Looking around in the spi drivers, apparently I am not the only
one who has this need, as I pointed out in my previous mail.

Thanks a lot,
Andi

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

On Mon, Jun 27, 2016 at 07:57:16PM +0900, Andi Shyti wrote:

> What I meant is that if we do not like num-cs = <0>, the
> unlinked CS line can be handled only this way (case of the
> s3c64xx driver):

> +- broken-cs: the CS line is disconnected, therefore the device should not wait
> +  for the CS protocol to be established

So what you're saying here is that you just need a property for the
inability to read back the chip select status?  That seems like a
totally reasonable thing to have which fits in idiomatically with the
rest of the subsystem.  I might call it no-cs-readback or something.

> Which is not something I like, because it means adding a new
> flag in the dts.

> What I want to suggest, instead, is to slightly change the logic
> behind the num-cs property: i.e. if "num-cs = <0>", doesn't
> necessarily mean that we don't have a CS controller, but, while
> we can have as many as we wish, non of them is connected.

I disagree, I think from a system integration point of view this is just
a chip select which can't be changed and it's less likely that we will
run into nasty surprises later on with things assuming that chip selects
exist.  AFAICT you only need this property in your case because this
controller has some features that rely on readback of the chip select
status, that's not very common - normally it'd be write only.  I'd
expect most controllers would just say they have one chip select and
that'd be that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

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

[Andi Shyti]

Hi Mark,

> > What I meant is that if we do not like num-cs = <0>, the
> > unlinked CS line can be handled only this way (case of the
> > s3c64xx driver):
> 
> > +- broken-cs: the CS line is disconnected, therefore the device should not wait
> > +  for the CS protocol to be established
> 
> So what you're saying here is that you just need a property for the
> inability to read back the chip select status?  That seems like a
> totally reasonable thing to have which fits in idiomatically with the
> rest of the subsystem.  I might call it no-cs-readback or something.
> 
> > Which is not something I like, because it means adding a new
> > flag in the dts.
> 
> > What I want to suggest, instead, is to slightly change the logic
> > behind the num-cs property: i.e. if "num-cs = <0>", doesn't
> > necessarily mean that we don't have a CS controller, but, while
> > we can have as many as we wish, non of them is connected.
> 
> I disagree, I think from a system integration point of view this is just
> a chip select which can't be changed and it's less likely that we will
> run into nasty surprises later on with things assuming that chip selects
> exist.  AFAICT you only need this property in your case because this
> controller has some features that rely on readback of the chip select
> status, that's not very common - normally it'd be write only.  I'd
> expect most controllers would just say they have one chip select and
> that'd be that.

thanks for your feedback, I will then do as you say, I will use
the no-cs-readback flag.

Thanks again,
Andi

Applied "spi: s3c64xx: simplify if statement in prepare_transfer function" to the spi tree

[Mark Brown]

The patch

   spi: s3c64xx: simplify if statement in prepare_transfer function

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 730d9d4d11cf48b17d07daaff1f0540642317e09 Mon Sep 17 00:00:00 2001
From: Andi Shyti <andi.shyti@samsung.com>
Date: Tue, 28 Jun 2016 11:41:14 +0900
Subject: [PATCH] spi: s3c64xx: simplify if statement in prepare_transfer
 function

The whole function is inside an 'if' statement
("!is_polling(sdd)").

Check the opposite of that statement at the beginning and exit,
this way we can have one level less of indentation.

Remove the goto paths as they are redundant.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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 c65a9e6baefd..6d8486fc668f 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

Applied "spi: s3c64xx: do not configure the device twice" to the spi tree

[Mark Brown]

The patch

   spi: s3c64xx: do not configure the device twice

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 11f66f0927bd044c17b2c2884d5103f8eb6cab38 Mon Sep 17 00:00:00 2001
From: Andi Shyti <andi.shyti@samsung.com>
Date: Tue, 28 Jun 2016 11:41:13 +0900
Subject: [PATCH] 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 first call at the prepare message because in that
point we don't have the imformation about "bit per word" and
frequency.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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 14269b07804c..c65a9e6baefd 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

Applied "spi: s3c64xx: group the CS signalling writes in a single function" to the spi tree

[Mark Brown]

The patch

   spi: s3c64xx: group the CS signalling writes in a single function

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From aa4964c4eb3ed38666023bcb805403cb7cf2af63 Mon Sep 17 00:00:00 2001
From: Andi Shyti <andi.shyti@samsung.com>
Date: Tue, 28 Jun 2016 11:41:11 +0900
Subject: [PATCH] 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. Group the register's
configuration in a single function.

Signed-off-by: Andi Shyti <andi.shyti@samsung.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 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 5a76a50063b5..972367d5c7f2 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