Re: [PATCH] ARM: at91: spi: request all csgpio in spi probe

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




Hi,

Dne 28.7.2014 v 14:21 Alexandre Belloni napsal(a):
Hi,

Thank you for that patch, a few questions/comments below.

On 28/07/2014 at 13:43:40 +0200, Jiri Prchal wrote :
This fix problem with PA14 set as periph A left from ROMBOOT and need it
to be set to gpio before spi bus communicate with chip on CS0 on other
gpio. As I reported a week ago.
Request all csgpios in spi_probe since cs depends on each other and must
be all of them in right state before any communication on bus. They are
part of bus, like miso, mosi, clk, not part of chips, they are defined
in parent spi node, not in child chip node.
Csgpio moved to atmel_spi struct as part of master bus.

Signed-off-by: Jiri Prchal <jiri.prchal@xxxxxxxxxxx>
---
  drivers/spi/spi-atmel.c | 68 +++++++++++++++++++++++++++++--------------------
  1 file changed, 41 insertions(+), 27 deletions(-)

diff --git a/drivers/spi/spi-atmel.c b/drivers/spi/spi-atmel.c
index 92a6f0d..1fb7bb9 100644
--- a/drivers/spi/spi-atmel.c
+++ b/drivers/spi/spi-atmel.c
@@ -22,11 +22,14 @@
  #include <linux/platform_data/atmel.h>
  #include <linux/platform_data/dma-atmel.h>
  #include <linux/of.h>
+#include <linux/of_gpio.h>

  #include <linux/io.h>
  #include <linux/gpio.h>
  #include <linux/pinctrl/consumer.h>

+#define DRIVER_NAME "atmel-spi"
+

This is not really related to the issue solved by that patch, maybe it
could go in another patch ?
Probably yes, but is used for this patch, I based it upon spi-efm32.

  /* SPI register offsets */
  #define SPI_CR					0x0000
  #define SPI_MR					0x0004
@@ -242,11 +245,13 @@ struct atmel_spi {

  	bool			keep_cs;
  	bool			cs_active;
+
+	/* chip selects */
+	unsigned int		csgpio[];
  };

  /* Controller-specific per-slave state */
  struct atmel_spi_device {
-	unsigned int		npcs_pin;
  	u32			csr;
  };

@@ -312,7 +317,7 @@ static void cs_activate(struct atmel_spi *as, struct spi_device *spi)
  		}

  		mr = spi_readl(as, MR);
-		gpio_set_value(asd->npcs_pin, active);
+		gpio_set_value(as->csgpio[spi->chip_select], active);
  	} else {
  		u32 cpol = (spi->mode & SPI_CPOL) ? SPI_BIT(CPOL) : 0;
  		int i;
@@ -329,18 +334,16 @@ 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 (spi->chip_select != 0)
-			gpio_set_value(asd->npcs_pin, active);
+			gpio_set_value(as->csgpio[spi->chip_select], active);
  		spi_writel(as, MR, mr);
  	}

  	dev_dbg(&spi->dev, "activate %u%s, mr %08x\n",
-			asd->npcs_pin, active ? " (high)" : "",
-			mr);
+		as->csgpio[spi->chip_select], active ? " (high)" : "", mr);
  }

  static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)
  {
-	struct atmel_spi_device *asd = spi->controller_state;
  	unsigned active = spi->mode & SPI_CS_HIGH;
  	u32 mr;

@@ -354,11 +357,10 @@ static void cs_deactivate(struct atmel_spi *as, struct spi_device *spi)
  	}

  	dev_dbg(&spi->dev, "DEactivate %u%s, mr %08x\n",
-			asd->npcs_pin, active ? " (low)" : "",
-			mr);
+		as->csgpio[spi->chip_select], active ? " (low)" : "", mr);

  	if (atmel_spi_is_v2(as) || spi->chip_select != 0)
-		gpio_set_value(asd->npcs_pin, !active);
+		gpio_set_value(as->csgpio[spi->chip_select], !active);
  }

  static void atmel_spi_lock(struct atmel_spi *as) __acquires(&as->lock)
@@ -989,8 +991,6 @@ static int atmel_spi_setup(struct spi_device *spi)
  	struct atmel_spi_device	*asd;
  	u32			csr;
  	unsigned int		bits = spi->bits_per_word;
-	unsigned int		npcs_pin;
-	int			ret;

  	as = spi_master_get_devdata(spi->master);

@@ -1017,27 +1017,13 @@ 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 int)spi->controller_data;
-
-	if (gpio_is_valid(spi->cs_gpio))
-		npcs_pin = spi->cs_gpio;
-
  	asd = spi->controller_state;
  	if (!asd) {
  		asd = kzalloc(sizeof(struct atmel_spi_device), GFP_KERNEL);
  		if (!asd)
  			return -ENOMEM;

-		ret = gpio_request(npcs_pin, dev_name(&spi->dev));
-		if (ret) {
-			kfree(asd);
-			return ret;
-		}
-
-		asd->npcs_pin = npcs_pin;
  		spi->controller_state = asd;
-		gpio_direction_output(npcs_pin, !(spi->mode & SPI_CS_HIGH));
  	}

  	asd->csr = csr;
@@ -1290,6 +1276,15 @@ static int atmel_spi_probe(struct platform_device *pdev)
  	int			ret;
  	struct spi_master	*master;
  	struct atmel_spi	*as;
+	struct device_node *np = pdev->dev.of_node;
+	int num_cs, i;
+
+	if (!np)
+		return -EINVAL;
+
+	num_cs = of_gpio_named_count(np, "cs-gpios");
+	if (num_cs < 0)
+		return num_cs;


This driver can still be probed without DT, this will break here, please
don't assume that DT is present.
Yes, but I copied it from spi-efm32 and looked at others, is almost the same.


  	/* Select default pin state */
  	pinctrl_pm_select_default_state(&pdev->dev);
@@ -1308,7 +1303,7 @@ static int atmel_spi_probe(struct platform_device *pdev)

  	/* setup spi core then atmel-specific driver state */
  	ret = -ENOMEM;
-	master = spi_alloc_master(&pdev->dev, sizeof(*as));
+	master = spi_alloc_master(&pdev->dev, sizeof(*as) + num_cs * sizeof(unsigned));
  	if (!master)
  		goto out_free;

@@ -1317,7 +1312,7 @@ static int atmel_spi_probe(struct platform_device *pdev)
  	master->bits_per_word_mask = SPI_BPW_RANGE_MASK(8, 16);
  	master->dev.of_node = pdev->dev.of_node;
  	master->bus_num = pdev->id;
-	master->num_chipselect = master->dev.of_node ? 0 : 4;
+	master->num_chipselect = num_cs;

My guess is that you don't need to change that part.
I think I need it. What about 3 cs?

  	master->setup = atmel_spi_setup;
  	master->transfer_one_message = atmel_spi_transfer_one_message;
  	master->cleanup = atmel_spi_cleanup;
@@ -1325,6 +1320,25 @@ static int atmel_spi_probe(struct platform_device *pdev)

  	as = spi_master_get_devdata(master);

+	for (i = 0; i < master->num_chipselect; ++i) {
+		ret = of_get_named_gpio(np, "cs-gpios", i);

Again, DT maybe not be compiled in or that driver may be probed from
platform data.
Is another way how to do it? I see cs-gpios in master struct, but where it gets filled.

+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to get csgpio#%u (%d)\n",
+				i, ret);
+			goto out_free;
+		}
+		as->csgpio[i] = ret;
+		dev_dbg(&pdev->dev, "csgpio#%u = %u\n", i, as->csgpio[i]);
+		ret = devm_gpio_request_one(&pdev->dev, as->csgpio[i],
+					    GPIOF_OUT_INIT_HIGH, DRIVER_NAME);
+		if (ret < 0) {
+			dev_err(&pdev->dev,
+				"failed to configure csgpio#%u (%d)\n",
+				i, ret);
+			goto out_free;
+		}
+	}
+
  	/*
  	 * Scratch buffer is used for throwaway rx and tx data.
  	 * It's coherent to minimize dcache pollution.
--
1.9.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux