[PATCH v2] spi: atmel: improve internal vs gpio chip-select choice

Subsystems: microchip spi driver, spi subsystem, the rest

STALE3936d

4 messages, 4 authors, 2016-01-12 · open the first message on its own page

[PATCH v2] spi: atmel: improve internal vs gpio chip-select choice

From: Mans Rullgard <hidden>
Date: 2016-01-11 16:02:26

The driver currently chooses between internal chip-select or gpio
based on the existence of the cs-gpios DT property which fails on
non-DT systems and also enforces the same choice for all devices.

This patch makes the method per device instead of per controller
and fixes the selection on non-DT systems.  With these changes,
the chip-select method for each device is chosen according to the
following priority:

1. GPIO from device tree
2. GPIO from platform data
3. Internal chip-select

Tested on AVR32 ATSTK1000.

Signed-off-by: Mans Rullgard <redacted>
---
Changes:
- allow internal CS only hardware v2
- retain seting of master->num_chipselect to 4 on hardware v2 if
  cs-gpios property is missing
---
 drivers/spi/spi-atmel.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 08cbb3e43c76..d4a806e24060 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -312,7 +312,6 @@ struct atmel_spi {
 
 	bool			use_dma;
 	bool			use_pdc;
-	bool			use_cs_gpios;
 	/* dmaengine data */
 	struct atmel_spi_dma	dma;
 
@@ -323,6 +322,7 @@ struct atmel_spi {
 struct atmel_spi_device {
 	unsigned int		npcs_pin;
 	u32			csr;
+	bool			use_cs_gpio;
 };
 
 #define BUFFER_SIZE		PAGE_SIZE
@@ -387,7 +387,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 		}
 
 		mr = spi_readl(as, MR);
-		if (as->use_cs_gpios)
+		if (asd->use_cs_gpio)
 			gpio_set_value(asd->npcs_pin, active);
 	} else {
 		u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0;
@@ -404,7 +404,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 
 		mr = spi_readl(as, MR);
 		mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr);
-		if (as->use_cs_gpios && spi->chip_select != 0)
+		if (asd->use_cs_gpio && spi->chip_select != 0)
 			gpio_set_value(asd->npcs_pin, active);
 		spi_writel(as, MR, mr);
 	}
@@ -433,7 +433,7 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)
 			asd->npcs_pin, active ? " (low)" : "",
 			mr);
 
-	if (!as->use_cs_gpios)
+	if (!asd->use_cs_gpio)
 		spi_writel(as, CR, SPI_BIT(LASTXFER));
 	else if (atmel_spi_is_v2(as) || spi->chip_select != 0)
 		gpio_set_value(asd->npcs_pin, !active);
@@ -1539,6 +1539,7 @@ static int atmel_spi_setup(struct spi_device *spi)
 	unsigned int		bits = spi->bits_per_word;
 	unsigned int		npcs_pin;
 	int			ret;
+	bool			use_cs_gpio;
 
 	as = spi_master_get_devdata(spi->master);
 
@@ -1565,8 +1566,6 @@ static int atmel_spi_setup(struct spi_device *spi)
 		csr |= SPI_BIT(CPOL);
 	if (!(spi->mode & SPI_CPHA))
 		csr |= SPI_BIT(NCPHA);
-	if (!as->use_cs_gpios)
-		csr |= SPI_BIT(CSAAT);
 
 	/* DLYBS is mostly irrelevant since we manage chipselect using GPIOs.
 	 *
@@ -1577,13 +1576,22 @@ static int atmel_spi_setup(struct spi_device *spi)
 	csr |= SPI_BF(DLYBS, 0);
 	csr |= SPI_BF(DLYBCT, 0);
 
-	/* chipselect must have been muxed as GPIO (e.g. in board setup) */
 	npcs_pin = (unsigned long)spi->controller_data;
 
-	if (!as->use_cs_gpios)
-		npcs_pin = spi->chip_select;
-	else if (gpio_is_valid(spi->cs_gpio))
+	if (gpio_is_valid(spi->cs_gpio)) {
+		/* GPIO from DT */
 		npcs_pin = spi->cs_gpio;
+		use_cs_gpio = true;
+	} else if (npcs_pin && gpio_is_valid(npcs_pin)) {
+		/* GPIO from platform data */
+		use_cs_gpio = true;
+	} else if (atmel_spi_is_v2(as)) {
+		/* internal chip-select */
+		npcs_pin = spi->chip_select;
+		use_cs_gpio = false;
+	} else {
+		return -EINVAL;
+	}
 
 	asd = spi->controller_state;
 	if (!asd) {
@@ -1591,7 +1599,7 @@ static int atmel_spi_setup(struct spi_device *spi)
 		if (!asd)
 			return -ENOMEM;
 
-		if (as->use_cs_gpios) {
+		if (use_cs_gpio) {
 			ret = gpio_request(npcs_pin, dev_name(&spi->dev));
 			if (ret) {
 				kfree(asd);
@@ -1603,6 +1611,7 @@ static int atmel_spi_setup(struct spi_device *spi)
 		}
 
 		asd->npcs_pin = npcs_pin;
+		asd->use_cs_gpio = use_cs_gpio;
 		spi->controller_state = asd;
 	} else {
 		atmel_spi_lock(as);
@@ -1612,6 +1621,8 @@ static int atmel_spi_setup(struct spi_device *spi)
 		atmel_spi_unlock(as);
 	}
 
+	if (!asd->use_cs_gpio)
+		csr |= SPI_BIT(CSAAT);
 	asd->csr = csr;
 
 	dev_dbg(&spi->dev,
@@ -1807,12 +1818,9 @@ static int atmel_spi_probe(struct platform_device *pdev)
 
 	atmel_get_caps(as);
 
-	as->use_cs_gpios = true;
 	if (atmel_spi_is_v2(as) &&
-	    !of_get_property(pdev->dev.of_node, "cs-gpios", NULL)) {
-		as->use_cs_gpios = false;
+	    !of_get_property(pdev->dev.of_node, "cs-gpios", NULL))
 		master->num_chipselect = 4;
-	}
 
 	as->use_dma = false;
 	as->use_pdc = false;
-- 
2.7.0

Re: [PATCH v2] spi: atmel: improve internal vs gpio chip-select choice

From: Mark Brown <broonie@kernel.org>
Date: 2016-01-11 16:13:53

On Mon, Jan 11, 2016 at 04:01:42PM +0000, Mans Rullgard wrote:
The driver currently chooses between internal chip-select or gpio
based on the existence of the cs-gpios DT property which fails on
non-DT systems and also enforces the same choice for all devices.
Normally you shouldn't send incremental patches against patches that
have already been applied.  However in this case I'm going to drop your
original patch anyway due to the issue that was found so it's OK.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160111/0ffbe160/attachment.sig>

Re: [PATCH v2] spi: atmel: improve internal vs gpio chip-select choice

From: Cyrille Pitchen <hidden>
Date: 2016-01-12 08:39:47

Hi Mans,

I've tried to apply your patch on a next-20160112 branch to test it but it
failed. I've also looked at the for-next branch of the SPI sub-system git
tree.

It seems that one issue occurred within a chunk patching the atmel_spi_setup()
function. Please see below.

Le 11/01/2016 17:01, Mans Rullgard a ?crit :
[...]
quoted hunk
diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 08cbb3e43c76..d4a806e24060 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
[...]
quoted hunk
@@ -1603,6 +1611,7 @@ static int atmel_spi_setup(struct spi_device *spi)
 		}
 
 		asd->npcs_pin = npcs_pin;
+		asd->use_cs_gpio = use_cs_gpio;
 		spi->controller_state = asd;
 	} else {
 		atmel_spi_lock(as);
[...]

There is no 'else' statement in the source code I'm looking at.

Best regards,

Cyrille

Re: [PATCH v2] spi: atmel: improve internal vs gpio chip-select choice

From: Nicolas Ferre <hidden>
Date: 2016-01-12 08:52:04

Le 11/01/2016 17:01, Mans Rullgard a ?crit :
The driver currently chooses between internal chip-select or gpio
based on the existence of the cs-gpios DT property which fails on
non-DT systems and also enforces the same choice for all devices.

This patch makes the method per device instead of per controller
and fixes the selection on non-DT systems.
Sorry Mans, but it's still a NACK for me on this "mixed CS feature" (Cf.
my answer to the previous version).

Bye,
quoted hunk
 With these changes,
the chip-select method for each device is chosen according to the
following priority:

1. GPIO from device tree
2. GPIO from platform data
3. Internal chip-select

Tested on AVR32 ATSTK1000.

Signed-off-by: Mans Rullgard <redacted>
---
Changes:
- allow internal CS only hardware v2
- retain seting of master->num_chipselect to 4 on hardware v2 if
  cs-gpios property is missing
---
 drivers/spi/spi-atmel.c | 38 +++++++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 08cbb3e43c76..d4a806e24060 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -312,7 +312,6 @@ struct atmel_spi {
 
 	bool			use_dma;
 	bool			use_pdc;
-	bool			use_cs_gpios;
 	/* dmaengine data */
 	struct atmel_spi_dma	dma;
 
@@ -323,6 +322,7 @@ struct atmel_spi {
 struct atmel_spi_device {
 	unsigned int		npcs_pin;
 	u32			csr;
+	bool			use_cs_gpio;
 };
 
 #define BUFFER_SIZE		PAGE_SIZE
@@ -387,7 +387,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 		}
 
 		mr = spi_readl(as, MR);
-		if (as->use_cs_gpios)
+		if (asd->use_cs_gpio)
 			gpio_set_value(asd->npcs_pin, active);
 	} else {
 		u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0;
@@ -404,7 +404,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
 
 		mr = spi_readl(as, MR);
 		mr = SPI_BFINS(PCS, ~(1 << spi->chip_select), mr);
-		if (as->use_cs_gpios && spi->chip_select != 0)
+		if (asd->use_cs_gpio && spi->chip_select != 0)
 			gpio_set_value(asd->npcs_pin, active);
 		spi_writel(as, MR, mr);
 	}
@@ -433,7 +433,7 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)
 			asd->npcs_pin, active ? " (low)" : "",
 			mr);
 
-	if (!as->use_cs_gpios)
+	if (!asd->use_cs_gpio)
 		spi_writel(as, CR, SPI_BIT(LASTXFER));
 	else if (atmel_spi_is_v2(as) || spi->chip_select != 0)
 		gpio_set_value(asd->npcs_pin, !active);
@@ -1539,6 +1539,7 @@ static int atmel_spi_setup(struct spi_device *spi)
 	unsigned int		bits = spi->bits_per_word;
 	unsigned int		npcs_pin;
 	int			ret;
+	bool			use_cs_gpio;
 
 	as = spi_master_get_devdata(spi->master);
 
@@ -1565,8 +1566,6 @@ static int atmel_spi_setup(struct spi_device *spi)
 		csr |= SPI_BIT(CPOL);
 	if (!(spi->mode & SPI_CPHA))
 		csr |= SPI_BIT(NCPHA);
-	if (!as->use_cs_gpios)
-		csr |= SPI_BIT(CSAAT);
 
 	/* DLYBS is mostly irrelevant since we manage chipselect using GPIOs.
 	 *
@@ -1577,13 +1576,22 @@ static int atmel_spi_setup(struct spi_device *spi)
 	csr |= SPI_BF(DLYBS, 0);
 	csr |= SPI_BF(DLYBCT, 0);
 
-	/* chipselect must have been muxed as GPIO (e.g. in board setup) */
 	npcs_pin = (unsigned long)spi->controller_data;
 
-	if (!as->use_cs_gpios)
-		npcs_pin = spi->chip_select;
-	else if (gpio_is_valid(spi->cs_gpio))
+	if (gpio_is_valid(spi->cs_gpio)) {
+		/* GPIO from DT */
 		npcs_pin = spi->cs_gpio;
+		use_cs_gpio = true;
+	} else if (npcs_pin && gpio_is_valid(npcs_pin)) {
+		/* GPIO from platform data */
+		use_cs_gpio = true;
+	} else if (atmel_spi_is_v2(as)) {
+		/* internal chip-select */
+		npcs_pin = spi->chip_select;
+		use_cs_gpio = false;
+	} else {
+		return -EINVAL;
+	}
 
 	asd = spi->controller_state;
 	if (!asd) {
@@ -1591,7 +1599,7 @@ static int atmel_spi_setup(struct spi_device *spi)
 		if (!asd)
 			return -ENOMEM;
 
-		if (as->use_cs_gpios) {
+		if (use_cs_gpio) {
 			ret = gpio_request(npcs_pin, dev_name(&spi->dev));
 			if (ret) {
 				kfree(asd);
@@ -1603,6 +1611,7 @@ static int atmel_spi_setup(struct spi_device *spi)
 		}
 
 		asd->npcs_pin = npcs_pin;
+		asd->use_cs_gpio = use_cs_gpio;
 		spi->controller_state = asd;
 	} else {
 		atmel_spi_lock(as);
@@ -1612,6 +1621,8 @@ static int atmel_spi_setup(struct spi_device *spi)
 		atmel_spi_unlock(as);
 	}
 
+	if (!asd->use_cs_gpio)
+		csr |= SPI_BIT(CSAAT);
 	asd->csr = csr;
 
 	dev_dbg(&spi->dev,
@@ -1807,12 +1818,9 @@ static int atmel_spi_probe(struct platform_device *pdev)
 
 	atmel_get_caps(as);
 
-	as->use_cs_gpios = true;
 	if (atmel_spi_is_v2(as) &&
-	    !of_get_property(pdev->dev.of_node, "cs-gpios", NULL)) {
-		as->use_cs_gpios = false;
+	    !of_get_property(pdev->dev.of_node, "cs-gpios", NULL))
 		master->num_chipselect = 4;
-	}
 
 	as->use_dma = false;
 	as->use_pdc = false;

-- 
Nicolas Ferre
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help