[PATCH v2 0/4] Fix for imx-spi CS GPIOs

[PATCH v2 0/4] Fix for imx-spi CS GPIOs

[Trent Piepho]

This fixes a bug where GPIO based chip selects must be specified in the OF
node for spi-imx when they should be optional.

Fixes a regression in the platform data of imx31moboard.

Clean up of some old platform data based spi-imx users.

Changes from V1:
 * Collected reviewed bys.
 * Minor change to coding style in one patch.

Trent Piepho (4):
  spi: imx: GPIO based chip selects should not be required
  spi: imx: Fix failure path leak on GPIO request error
  spi: imx: Don't require platform data chipselect array
  ARM: imx: Update spi_imx platform data to reflect current state

 arch/arm/mach-imx/mach-mx31_3ds.c     | 18 ++-------------
 arch/arm/mach-imx/mach-mx31lilly.c    | 12 ++--------
 arch/arm/mach-imx/mach-mx31lite.c     | 16 ++------------
 arch/arm/mach-imx/mach-mx31moboard.c  | 17 +++------------
 arch/arm/mach-imx/mach-pcm037_eet.c   |  5 +----
 drivers/spi/spi-imx.c                 | 41 ++++++++++++++++-------------------
 include/linux/platform_data/spi-imx.h | 29 +++++++++++++++----------
 7 files changed, 46 insertions(+), 92 deletions(-)

-- 
2.14.3

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

[PATCH v2 0/4] Fix for imx-spi CS GPIOs

[Trent Piepho]

This fixes a bug where GPIO based chip selects must be specified in the OF
node for spi-imx when they should be optional.

Fixes a regression in the platform data of imx31moboard.

Clean up of some old platform data based spi-imx users.

Changes from V1:
 * Collected reviewed bys.
 * Minor change to coding style in one patch.

Trent Piepho (4):
  spi: imx: GPIO based chip selects should not be required
  spi: imx: Fix failure path leak on GPIO request error
  spi: imx: Don't require platform data chipselect array
  ARM: imx: Update spi_imx platform data to reflect current state

 arch/arm/mach-imx/mach-mx31_3ds.c     | 18 ++-------------
 arch/arm/mach-imx/mach-mx31lilly.c    | 12 ++--------
 arch/arm/mach-imx/mach-mx31lite.c     | 16 ++------------
 arch/arm/mach-imx/mach-mx31moboard.c  | 17 +++------------
 arch/arm/mach-imx/mach-pcm037_eet.c   |  5 +----
 drivers/spi/spi-imx.c                 | 41 ++++++++++++++++-------------------
 include/linux/platform_data/spi-imx.h | 29 +++++++++++++++----------
 7 files changed, 46 insertions(+), 92 deletions(-)

-- 
2.14.3

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

The driver will fail to load if no gpio chip selects are specified,
this patch changes this so that it no longer fails.

It's possible to use all native chip selects, in which case there is
no reason to have a gpio chip select array.  This is what happens if
the *optional* device tree property "cs-gpios" is omitted.

The spi core already checks for the absence of gpio chip selects in
the master and assigns any slaves the gpio_cs value of -ENOENT.

CC: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
CC: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
Reviewed-by: Oleksij Rempel <o.rempel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Trent Piepho <tpiepho-cgc2CodaaHDQT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi-imx.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index babb15f07995..07e6250f2dad 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1457,22 +1457,19 @@ static int spi_imx_probe(struct platform_device *pdev)
 		goto out_clk_put;
 	}
 
-	if (!master->cs_gpios) {
-		dev_err(&pdev->dev, "No CS GPIOs available\n");
-		ret = -EINVAL;
-		goto out_clk_put;
-	}
-
-	for (i = 0; i < master->num_chipselect; i++) {
-		if (!gpio_is_valid(master->cs_gpios[i]))
-			continue;
-
-		ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i],
-					DRIVER_NAME);
-		if (ret) {
-			dev_err(&pdev->dev, "Can't get CS GPIO %i\n",
-				master->cs_gpios[i]);
-			goto out_clk_put;
+	/* Request GPIO CS lines, if any */
+	if (master->cs_gpios) {
+		for (i = 0; i < master->num_chipselect; i++) {
+			if (!gpio_is_valid(master->cs_gpios[i]))
+				continue;
+
+			ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i],
+						DRIVER_NAME);
+			if (ret) {
+				dev_err(&pdev->dev, "Can't get CS GPIO %i\n",
+					master->cs_gpios[i]);
+				goto out_clk_put;
+			}
 		}
 	}
 
-- 
2.14.3

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

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

The driver will fail to load if no gpio chip selects are specified,
this patch changes this so that it no longer fails.

It's possible to use all native chip selects, in which case there is
no reason to have a gpio chip select array.  This is what happens if
the *optional* device tree property "cs-gpios" is omitted.

The spi core already checks for the absence of gpio chip selects in
the master and assigns any slaves the gpio_cs value of -ENOENT.

CC: Mark Brown <broonie@kernel.org>
CC: Shawn Guo <shawnguo@kernel.org>
CC: Sascha Hauer <kernel@pengutronix.de>
CC: Fabio Estevam <fabio.estevam@nxp.com>
Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/spi/spi-imx.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index babb15f07995..07e6250f2dad 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1457,22 +1457,19 @@ static int spi_imx_probe(struct platform_device *pdev)
 		goto out_clk_put;
 	}
 
-	if (!master->cs_gpios) {
-		dev_err(&pdev->dev, "No CS GPIOs available\n");
-		ret = -EINVAL;
-		goto out_clk_put;
-	}
-
-	for (i = 0; i < master->num_chipselect; i++) {
-		if (!gpio_is_valid(master->cs_gpios[i]))
-			continue;
-
-		ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i],
-					DRIVER_NAME);
-		if (ret) {
-			dev_err(&pdev->dev, "Can't get CS GPIO %i\n",
-				master->cs_gpios[i]);
-			goto out_clk_put;
+	/* Request GPIO CS lines, if any */
+	if (master->cs_gpios) {
+		for (i = 0; i < master->num_chipselect; i++) {
+			if (!gpio_is_valid(master->cs_gpios[i]))
+				continue;
+
+			ret = devm_gpio_request(&pdev->dev, master->cs_gpios[i],
+						DRIVER_NAME);
+			if (ret) {
+				dev_err(&pdev->dev, "Can't get CS GPIO %i\n",
+					master->cs_gpios[i]);
+				goto out_clk_put;
+			}
 		}
 	}
 
-- 
2.14.3

[PATCH v2 2/4] spi: imx: Fix failure path leak on GPIO request error

[Trent Piepho]

If the code that requests any chip select GPIOs fails, the cleanup of
spi_bitbang_start() by calling spi_bitbang_stop() is not done.

Fix this by moving spi_bitbang_start() to after the code that requets
GPIOs.  The GPIOs are dev managed and don't need explicit cleanup.
Since spi_bitbang_start() is now the last operation, it doesn't need
to be cleaned up in the failure path.

CC: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
CC: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
CC: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Reviewed-by: Oleksij Rempel <o.rempel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Signed-off-by: Trent Piepho <tpiepho-cgc2CodaaHDQT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi-imx.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 07e6250f2dad..fea46cbf458a 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1451,11 +1451,6 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->devtype_data->intctrl(spi_imx, 0);
 
 	master->dev.of_node = pdev->dev.of_node;
-	ret = spi_bitbang_start(&spi_imx->bitbang);
-	if (ret) {
-		dev_err(&pdev->dev, "bitbang start failed with %d\n", ret);
-		goto out_clk_put;
-	}
 
 	/* Request GPIO CS lines, if any */
 	if (master->cs_gpios) {
@@ -1473,6 +1468,12 @@ static int spi_imx_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = spi_bitbang_start(&spi_imx->bitbang);
+	if (ret) {
+		dev_err(&pdev->dev, "bitbang start failed with %d\n", ret);
+		goto out_clk_put;
+	}
+
 	dev_info(&pdev->dev, "probed\n");
 
 	clk_disable(spi_imx->clk_ipg);
-- 
2.14.3

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

[PATCH v2 2/4] spi: imx: Fix failure path leak on GPIO request error

[Trent Piepho]

If the code that requests any chip select GPIOs fails, the cleanup of
spi_bitbang_start() by calling spi_bitbang_stop() is not done.

Fix this by moving spi_bitbang_start() to after the code that requets
GPIOs.  The GPIOs are dev managed and don't need explicit cleanup.
Since spi_bitbang_start() is now the last operation, it doesn't need
to be cleaned up in the failure path.

CC: Shawn Guo <shawnguo@kernel.org>
CC: Sascha Hauer <kernel@pengutronix.de>
CC: Fabio Estevam <fabio.estevam@nxp.com>
CC: Mark Brown <broonie@kernel.org>
Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/spi/spi-imx.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 07e6250f2dad..fea46cbf458a 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1451,11 +1451,6 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->devtype_data->intctrl(spi_imx, 0);
 
 	master->dev.of_node = pdev->dev.of_node;
-	ret = spi_bitbang_start(&spi_imx->bitbang);
-	if (ret) {
-		dev_err(&pdev->dev, "bitbang start failed with %d\n", ret);
-		goto out_clk_put;
-	}
 
 	/* Request GPIO CS lines, if any */
 	if (master->cs_gpios) {
@@ -1473,6 +1468,12 @@ static int spi_imx_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = spi_bitbang_start(&spi_imx->bitbang);
+	if (ret) {
+		dev_err(&pdev->dev, "bitbang start failed with %d\n", ret);
+		goto out_clk_put;
+	}
+
 	dev_info(&pdev->dev, "probed\n");
 
 	clk_disable(spi_imx->clk_ipg);
-- 
2.14.3

[PATCH v2 3/4] spi: imx: Don't require platform data chipselect array

[Trent Piepho]

If the array is not present, assume all chip selects are native.  This
is the standard behavior for SPI masters configured via the device
tree and the behavior of this driver as well when it is configured via
device tree.

This reduces platform data vs DT differences and allows most of the
platform data based boards to remove their chip select arrays.

CC: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
CC: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
CC: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Signed-off-by: Trent Piepho <tpiepho-cgc2CodaaHDQT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi-imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index fea46cbf458a..535378ebab18 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1362,8 +1362,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->devtype_data = of_id ? of_id->data :
 		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
 
-	if (mxc_platform_info) {
-		master->num_chipselect = mxc_platform_info->num_chipselect;
+	if (mxc_platform_info && master->num_chipselect) {
 		master->cs_gpios = devm_kzalloc(&master->dev,
 			sizeof(int) * master->num_chipselect, GFP_KERNEL);
 		if (!master->cs_gpios)
@@ -1371,7 +1370,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 
 		for (i = 0; i < master->num_chipselect; i++)
 			master->cs_gpios[i] = mxc_platform_info->chipselect[i];
- 	}
+	}
 
 	spi_imx->bitbang.chipselect = spi_imx_chipselect;
 	spi_imx->bitbang.setup_transfer = spi_imx_setupxfer;
-- 
2.14.3

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

[PATCH v2 3/4] spi: imx: Don't require platform data chipselect array

[Trent Piepho]

If the array is not present, assume all chip selects are native.  This
is the standard behavior for SPI masters configured via the device
tree and the behavior of this driver as well when it is configured via
device tree.

This reduces platform data vs DT differences and allows most of the
platform data based boards to remove their chip select arrays.

CC: Shawn Guo <shawnguo@kernel.org>
CC: Sascha Hauer <kernel@pengutronix.de>
CC: Fabio Estevam <fabio.estevam@nxp.com>
CC: Mark Brown <broonie@kernel.org>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 drivers/spi/spi-imx.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index fea46cbf458a..535378ebab18 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1362,8 +1362,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->devtype_data = of_id ? of_id->data :
 		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
 
-	if (mxc_platform_info) {
-		master->num_chipselect = mxc_platform_info->num_chipselect;
+	if (mxc_platform_info && master->num_chipselect) {
 		master->cs_gpios = devm_kzalloc(&master->dev,
 			sizeof(int) * master->num_chipselect, GFP_KERNEL);
 		if (!master->cs_gpios)
@@ -1371,7 +1370,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 
 		for (i = 0; i < master->num_chipselect; i++)
 			master->cs_gpios[i] = mxc_platform_info->chipselect[i];
- 	}
+	}
 
 	spi_imx->bitbang.chipselect = spi_imx_chipselect;
 	spi_imx->bitbang.setup_transfer = spi_imx_setupxfer;
-- 
2.14.3

[PATCH v2 4/4] ARM: imx: Update spi_imx platform data to reflect current state

[Trent Piepho]

The docs for the spi_imx platform data still refer to a -32 offset used to
specify a native chip select.  This was removed in commit 602c8f4485cd
("spi: imx: fix use of native chip-selects with devicetree") and no
longer works as documented.  Update documentation.

The macro MXC_SPI_CS() is no longer is needed.

If a board uses all native chip selects, then it's not necessary to
specify a chip select array at all, as all native is the default (this is
how device-tree configured SPI masters work too).  Most of the spi-imx
platform data users have their chip select arrays removed by this patch.

This patch also fixes a bug in mx31moboard introduced in the '602 commit.
When that board was updated in commit 901f26bce64a ("ARM: imx: set
correct chip_select in platform setup") to reflect the SPI change, only
SPI bus 2 was updated and SPI bus 1 was left with non-sequential chip
selects.  The mc13783 spi device on bus 1 had its chip select updated as
if it were on bus 2.

CC: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
CC: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
CC: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
Acked-by: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>
Signed-off-by: Trent Piepho <tpiepho-cgc2CodaaHDQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/mach-imx/mach-mx31_3ds.c     | 18 ++----------------
 arch/arm/mach-imx/mach-mx31lilly.c    | 12 ++----------
 arch/arm/mach-imx/mach-mx31lite.c     | 16 ++--------------
 arch/arm/mach-imx/mach-mx31moboard.c  | 17 +++--------------
 arch/arm/mach-imx/mach-pcm037_eet.c   |  5 +----
 include/linux/platform_data/spi-imx.h | 29 +++++++++++++++++------------
 6 files changed, 27 insertions(+), 70 deletions(-)

diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c b/arch/arm/mach-imx/mach-mx31_3ds.c
index 68c3f0799d5b..9d87f1dcf7bb 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -374,26 +374,12 @@ static struct imx_ssi_platform_data mx31_3ds_ssi_pdata = {
 };
 
 /* SPI */
-static int spi0_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(1),
-	MXC_SPI_CS(2),
-};
-
 static const struct spi_imx_master spi0_pdata __initconst = {
-	.chipselect	= spi0_internal_chipselect,
-	.num_chipselect	= ARRAY_SIZE(spi0_internal_chipselect),
-};
-
-static int spi1_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(1),
-	MXC_SPI_CS(2),
+	.num_chipselect	= 3,
 };
 
 static const struct spi_imx_master spi1_pdata __initconst = {
-	.chipselect	= spi1_internal_chipselect,
-	.num_chipselect	= ARRAY_SIZE(spi1_internal_chipselect),
+	.num_chipselect	= 3,
 };
 
 static struct spi_board_info mx31_3ds_spi_devs[] __initdata = {
diff --git a/arch/arm/mach-imx/mach-mx31lilly.c b/arch/arm/mach-imx/mach-mx31lilly.c
index 6fd463642954..8bf52819d4d9 100644
--- a/arch/arm/mach-imx/mach-mx31lilly.c
+++ b/arch/arm/mach-imx/mach-mx31lilly.c
@@ -226,20 +226,12 @@ static void __init lilly1131_usb_init(void)
 
 /* SPI */
 
-static int spi_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(1),
-	MXC_SPI_CS(2),
-};
-
 static const struct spi_imx_master spi0_pdata __initconst = {
-	.chipselect = spi_internal_chipselect,
-	.num_chipselect = ARRAY_SIZE(spi_internal_chipselect),
+	.num_chipselect = 3,
 };
 
 static const struct spi_imx_master spi1_pdata __initconst = {
-	.chipselect = spi_internal_chipselect,
-	.num_chipselect = ARRAY_SIZE(spi_internal_chipselect),
+	.num_chipselect = 3,
 };
 
 static struct mc13xxx_platform_data mc13783_pdata __initdata = {
diff --git a/arch/arm/mach-imx/mach-mx31lite.c b/arch/arm/mach-imx/mach-mx31lite.c
index f033a57d5694..fcbaf0070ccf 100644
--- a/arch/arm/mach-imx/mach-mx31lite.c
+++ b/arch/arm/mach-imx/mach-mx31lite.c
@@ -83,15 +83,8 @@ static const struct imxuart_platform_data uart_pdata __initconst = {
 };
 
 /* SPI */
-static int spi0_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(1),
-	MXC_SPI_CS(2),
-};
-
 static const struct spi_imx_master spi0_pdata __initconst = {
-	.chipselect	= spi0_internal_chipselect,
-	.num_chipselect	= ARRAY_SIZE(spi0_internal_chipselect),
+	.num_chipselect	= 3,
 };
 
 static const struct mxc_nand_platform_data
@@ -133,13 +126,8 @@ static struct platform_device smsc911x_device = {
  * The MC13783 is the only hard-wired SPI device on the module.
  */
 
-static int spi1_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-};
-
 static const struct spi_imx_master spi1_pdata __initconst = {
-	.chipselect	= spi1_internal_chipselect,
-	.num_chipselect	= ARRAY_SIZE(spi1_internal_chipselect),
+	.num_chipselect	= 1,
 };
 
 static struct mc13xxx_platform_data mc13783_pdata __initdata = {
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c b/arch/arm/mach-imx/mach-mx31moboard.c
index 7716f83aecdd..643a3d749703 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -152,14 +152,8 @@ static const struct imxi2c_platform_data moboard_i2c1_data __initconst = {
 	.bitrate = 100000,
 };
 
-static int moboard_spi1_cs[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(2),
-};
-
 static const struct spi_imx_master moboard_spi1_pdata __initconst = {
-	.chipselect	= moboard_spi1_cs,
-	.num_chipselect	= ARRAY_SIZE(moboard_spi1_cs),
+	.num_chipselect	= 3,
 };
 
 static struct regulator_consumer_supply sdhc_consumers[] = {
@@ -296,19 +290,14 @@ static struct spi_board_info moboard_spi_board_info[] __initdata = {
 		/* irq number is run-time assigned */
 		.max_speed_hz = 300000,
 		.bus_num = 1,
-		.chip_select = 1,
+		.chip_select = 0,
 		.platform_data = &moboard_pmic,
 		.mode = SPI_CS_HIGH,
 	},
 };
 
-static int moboard_spi2_cs[] = {
-	MXC_SPI_CS(0), MXC_SPI_CS(1),
-};
-
 static const struct spi_imx_master moboard_spi2_pdata __initconst = {
-	.chipselect	= moboard_spi2_cs,
-	.num_chipselect	= ARRAY_SIZE(moboard_spi2_cs),
+	.num_chipselect	= 2,
 };
 
 #define SDHC1_CD IOMUX_TO_GPIO(MX31_PIN_ATA_CS0)
diff --git a/arch/arm/mach-imx/mach-pcm037_eet.c b/arch/arm/mach-imx/mach-pcm037_eet.c
index 95bd97710494..15bc956d466b 100644
--- a/arch/arm/mach-imx/mach-pcm037_eet.c
+++ b/arch/arm/mach-imx/mach-pcm037_eet.c
@@ -56,11 +56,8 @@ static struct spi_board_info pcm037_spi_dev[] = {
 };
 
 /* Platform Data for MXC CSPI */
-static int pcm037_spi1_cs[] = { MXC_SPI_CS(0), MXC_SPI_CS(1), };
-
 static const struct spi_imx_master pcm037_spi1_pdata __initconst = {
-	.chipselect = pcm037_spi1_cs,
-	.num_chipselect = ARRAY_SIZE(pcm037_spi1_cs),
+	.num_chipselect = 2,
 };
 
 /* GPIO-keys input device */
diff --git a/include/linux/platform_data/spi-imx.h b/include/linux/platform_data/spi-imx.h
index 08be445e8eb8..9b2ed66ef7a2 100644
--- a/include/linux/platform_data/spi-imx.h
+++ b/include/linux/platform_data/spi-imx.h
@@ -4,24 +4,29 @@
 
 /*
  * struct spi_imx_master - device.platform_data for SPI controller devices.
- * @chipselect: Array of chipselects for this master. Numbers >= 0 mean gpio
- *              pins, numbers < 0 mean internal CSPI chipselects according
- *              to MXC_SPI_CS(). Normally you want to use gpio based chip
- *              selects as the CSPI module tries to be intelligent about
- *              when to assert the chipselect: The CSPI module deasserts the
- *              chipselect once it runs out of input data. The other problem
- *              is that it is not possible to mix between high active and low
- *              active chipselects on one single bus using the internal
- *              chipselects. Unfortunately Freescale decided to put some
+ * @chipselect: Array of chipselects for this master or NULL.  Numbers >= 0
+ *              mean GPIO pins, -ENOENT means internal CSPI chipselect
+ *              matching the position in the array.  E.g., if chipselect[1] =
+ *              -ENOENT then a SPI slave using chip select 1 will use the
+ *              native SS1 line of the CSPI.  Omitting the array will use
+ *              all native chip selects.
+
+ *              Normally you want to use gpio based chip selects as the CSPI
+ *              module tries to be intelligent about when to assert the
+ *              chipselect:  The CSPI module deasserts the chipselect once it
+ *              runs out of input data.  The other problem is that it is not
+ *              possible to mix between high active and low active chipselects
+ *              on one single bus using the internal chipselects.
+ *              Unfortunately, on some SoCs, Freescale decided to put some
  *              chipselects on dedicated pins which are not usable as gpios,
  *              so we have to support the internal chipselects.
- * @num_chipselect: ARRAY_SIZE(chipselect)
+ *
+ * @num_chipselect: If @chipselect is specified, ARRAY_SIZE(chipselect),
+ *                  otherwise the number of native chip selects.
  */
 struct spi_imx_master {
 	int	*chipselect;
 	int	num_chipselect;
 };
 
-#define MXC_SPI_CS(no)	((no) - 32)
-
 #endif /* __MACH_SPI_H_*/
-- 
2.14.3

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

[PATCH v2 4/4] ARM: imx: Update spi_imx platform data to reflect current state

[Trent Piepho]

The docs for the spi_imx platform data still refer to a -32 offset used to
specify a native chip select.  This was removed in commit 602c8f4485cd
("spi: imx: fix use of native chip-selects with devicetree") and no
longer works as documented.  Update documentation.

The macro MXC_SPI_CS() is no longer is needed.

If a board uses all native chip selects, then it's not necessary to
specify a chip select array at all, as all native is the default (this is
how device-tree configured SPI masters work too).  Most of the spi-imx
platform data users have their chip select arrays removed by this patch.

This patch also fixes a bug in mx31moboard introduced in the '602 commit.
When that board was updated in commit 901f26bce64a ("ARM: imx: set
correct chip_select in platform setup") to reflect the SPI change, only
SPI bus 2 was updated and SPI bus 1 was left with non-sequential chip
selects.  The mc13783 spi device on bus 1 had its chip select updated as
if it were on bus 2.

CC: Shawn Guo <shawnguo@kernel.org>
CC: Sascha Hauer <kernel@pengutronix.de>
CC: Fabio Estevam <fabio.estevam@nxp.com>
Acked-by: Greg Ungerer <gerg@linux-m68k.org>
Signed-off-by: Trent Piepho <tpiepho@impinj.com>
---
 arch/arm/mach-imx/mach-mx31_3ds.c     | 18 ++----------------
 arch/arm/mach-imx/mach-mx31lilly.c    | 12 ++----------
 arch/arm/mach-imx/mach-mx31lite.c     | 16 ++--------------
 arch/arm/mach-imx/mach-mx31moboard.c  | 17 +++--------------
 arch/arm/mach-imx/mach-pcm037_eet.c   |  5 +----
 include/linux/platform_data/spi-imx.h | 29 +++++++++++++++++------------
 6 files changed, 27 insertions(+), 70 deletions(-)

diff --git a/arch/arm/mach-imx/mach-mx31_3ds.c b/arch/arm/mach-imx/mach-mx31_3ds.c
index 68c3f0799d5b..9d87f1dcf7bb 100644
--- a/arch/arm/mach-imx/mach-mx31_3ds.c
+++ b/arch/arm/mach-imx/mach-mx31_3ds.c
@@ -374,26 +374,12 @@ static struct imx_ssi_platform_data mx31_3ds_ssi_pdata = {
 };
 
 /* SPI */
-static int spi0_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(1),
-	MXC_SPI_CS(2),
-};
-
 static const struct spi_imx_master spi0_pdata __initconst = {
-	.chipselect	= spi0_internal_chipselect,
-	.num_chipselect	= ARRAY_SIZE(spi0_internal_chipselect),
-};
-
-static int spi1_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(1),
-	MXC_SPI_CS(2),
+	.num_chipselect	= 3,
 };
 
 static const struct spi_imx_master spi1_pdata __initconst = {
-	.chipselect	= spi1_internal_chipselect,
-	.num_chipselect	= ARRAY_SIZE(spi1_internal_chipselect),
+	.num_chipselect	= 3,
 };
 
 static struct spi_board_info mx31_3ds_spi_devs[] __initdata = {
diff --git a/arch/arm/mach-imx/mach-mx31lilly.c b/arch/arm/mach-imx/mach-mx31lilly.c
index 6fd463642954..8bf52819d4d9 100644
--- a/arch/arm/mach-imx/mach-mx31lilly.c
+++ b/arch/arm/mach-imx/mach-mx31lilly.c
@@ -226,20 +226,12 @@ static void __init lilly1131_usb_init(void)
 
 /* SPI */
 
-static int spi_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(1),
-	MXC_SPI_CS(2),
-};
-
 static const struct spi_imx_master spi0_pdata __initconst = {
-	.chipselect = spi_internal_chipselect,
-	.num_chipselect = ARRAY_SIZE(spi_internal_chipselect),
+	.num_chipselect = 3,
 };
 
 static const struct spi_imx_master spi1_pdata __initconst = {
-	.chipselect = spi_internal_chipselect,
-	.num_chipselect = ARRAY_SIZE(spi_internal_chipselect),
+	.num_chipselect = 3,
 };
 
 static struct mc13xxx_platform_data mc13783_pdata __initdata = {
diff --git a/arch/arm/mach-imx/mach-mx31lite.c b/arch/arm/mach-imx/mach-mx31lite.c
index f033a57d5694..fcbaf0070ccf 100644
--- a/arch/arm/mach-imx/mach-mx31lite.c
+++ b/arch/arm/mach-imx/mach-mx31lite.c
@@ -83,15 +83,8 @@ static const struct imxuart_platform_data uart_pdata __initconst = {
 };
 
 /* SPI */
-static int spi0_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(1),
-	MXC_SPI_CS(2),
-};
-
 static const struct spi_imx_master spi0_pdata __initconst = {
-	.chipselect	= spi0_internal_chipselect,
-	.num_chipselect	= ARRAY_SIZE(spi0_internal_chipselect),
+	.num_chipselect	= 3,
 };
 
 static const struct mxc_nand_platform_data
@@ -133,13 +126,8 @@ static struct platform_device smsc911x_device = {
  * The MC13783 is the only hard-wired SPI device on the module.
  */
 
-static int spi1_internal_chipselect[] = {
-	MXC_SPI_CS(0),
-};
-
 static const struct spi_imx_master spi1_pdata __initconst = {
-	.chipselect	= spi1_internal_chipselect,
-	.num_chipselect	= ARRAY_SIZE(spi1_internal_chipselect),
+	.num_chipselect	= 1,
 };
 
 static struct mc13xxx_platform_data mc13783_pdata __initdata = {
diff --git a/arch/arm/mach-imx/mach-mx31moboard.c b/arch/arm/mach-imx/mach-mx31moboard.c
index 7716f83aecdd..643a3d749703 100644
--- a/arch/arm/mach-imx/mach-mx31moboard.c
+++ b/arch/arm/mach-imx/mach-mx31moboard.c
@@ -152,14 +152,8 @@ static const struct imxi2c_platform_data moboard_i2c1_data __initconst = {
 	.bitrate = 100000,
 };
 
-static int moboard_spi1_cs[] = {
-	MXC_SPI_CS(0),
-	MXC_SPI_CS(2),
-};
-
 static const struct spi_imx_master moboard_spi1_pdata __initconst = {
-	.chipselect	= moboard_spi1_cs,
-	.num_chipselect	= ARRAY_SIZE(moboard_spi1_cs),
+	.num_chipselect	= 3,
 };
 
 static struct regulator_consumer_supply sdhc_consumers[] = {
@@ -296,19 +290,14 @@ static struct spi_board_info moboard_spi_board_info[] __initdata = {
 		/* irq number is run-time assigned */
 		.max_speed_hz = 300000,
 		.bus_num = 1,
-		.chip_select = 1,
+		.chip_select = 0,
 		.platform_data = &moboard_pmic,
 		.mode = SPI_CS_HIGH,
 	},
 };
 
-static int moboard_spi2_cs[] = {
-	MXC_SPI_CS(0), MXC_SPI_CS(1),
-};
-
 static const struct spi_imx_master moboard_spi2_pdata __initconst = {
-	.chipselect	= moboard_spi2_cs,
-	.num_chipselect	= ARRAY_SIZE(moboard_spi2_cs),
+	.num_chipselect	= 2,
 };
 
 #define SDHC1_CD IOMUX_TO_GPIO(MX31_PIN_ATA_CS0)
diff --git a/arch/arm/mach-imx/mach-pcm037_eet.c b/arch/arm/mach-imx/mach-pcm037_eet.c
index 95bd97710494..15bc956d466b 100644
--- a/arch/arm/mach-imx/mach-pcm037_eet.c
+++ b/arch/arm/mach-imx/mach-pcm037_eet.c
@@ -56,11 +56,8 @@ static struct spi_board_info pcm037_spi_dev[] = {
 };
 
 /* Platform Data for MXC CSPI */
-static int pcm037_spi1_cs[] = { MXC_SPI_CS(0), MXC_SPI_CS(1), };
-
 static const struct spi_imx_master pcm037_spi1_pdata __initconst = {
-	.chipselect = pcm037_spi1_cs,
-	.num_chipselect = ARRAY_SIZE(pcm037_spi1_cs),
+	.num_chipselect = 2,
 };
 
 /* GPIO-keys input device */
diff --git a/include/linux/platform_data/spi-imx.h b/include/linux/platform_data/spi-imx.h
index 08be445e8eb8..9b2ed66ef7a2 100644
--- a/include/linux/platform_data/spi-imx.h
+++ b/include/linux/platform_data/spi-imx.h
@@ -4,24 +4,29 @@
 
 /*
  * struct spi_imx_master - device.platform_data for SPI controller devices.
- * @chipselect: Array of chipselects for this master. Numbers >= 0 mean gpio
- *              pins, numbers < 0 mean internal CSPI chipselects according
- *              to MXC_SPI_CS(). Normally you want to use gpio based chip
- *              selects as the CSPI module tries to be intelligent about
- *              when to assert the chipselect: The CSPI module deasserts the
- *              chipselect once it runs out of input data. The other problem
- *              is that it is not possible to mix between high active and low
- *              active chipselects on one single bus using the internal
- *              chipselects. Unfortunately Freescale decided to put some
+ * @chipselect: Array of chipselects for this master or NULL.  Numbers >= 0
+ *              mean GPIO pins, -ENOENT means internal CSPI chipselect
+ *              matching the position in the array.  E.g., if chipselect[1] =
+ *              -ENOENT then a SPI slave using chip select 1 will use the
+ *              native SS1 line of the CSPI.  Omitting the array will use
+ *              all native chip selects.
+
+ *              Normally you want to use gpio based chip selects as the CSPI
+ *              module tries to be intelligent about when to assert the
+ *              chipselect:  The CSPI module deasserts the chipselect once it
+ *              runs out of input data.  The other problem is that it is not
+ *              possible to mix between high active and low active chipselects
+ *              on one single bus using the internal chipselects.
+ *              Unfortunately, on some SoCs, Freescale decided to put some
  *              chipselects on dedicated pins which are not usable as gpios,
  *              so we have to support the internal chipselects.
- * @num_chipselect: ARRAY_SIZE(chipselect)
+ *
+ * @num_chipselect: If @chipselect is specified, ARRAY_SIZE(chipselect),
+ *                  otherwise the number of native chip selects.
  */
 struct spi_imx_master {
 	int	*chipselect;
 	int	num_chipselect;
 };
 
-#define MXC_SPI_CS(no)	((no) - 32)
-
 #endif /* __MACH_SPI_H_*/
-- 
2.14.3

Re: [PATCH v2 3/4] spi: imx: Don't require platform data chipselect array

[Oleksij Rempel]

Hi Trent,

On Thu, Oct 26, 2017 at 06:08:40PM -0700, Trent Piepho wrote:
> If the array is not present, assume all chip selects are native.  This
> is the standard behavior for SPI masters configured via the device
> tree and the behavior of this driver as well when it is configured via
> device tree.
> 
> This reduces platform data vs DT differences and allows most of the
> platform data based boards to remove their chip select arrays.
> 
> CC: Shawn Guo <shawnguo-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Sascha Hauer <kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> CC: Fabio Estevam <fabio.estevam-3arQi8VN3Tc@public.gmane.org>
> CC: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Trent Piepho <tpiepho-cgc2CodaaHDQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/spi/spi-imx.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index fea46cbf458a..535378ebab18 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1362,8 +1362,7 @@ static int spi_imx_probe(struct platform_device *pdev)
>  	spi_imx->devtype_data = of_id ? of_id->data :
>  		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
>  
> -	if (mxc_platform_info) {
> -		master->num_chipselect = mxc_platform_info->num_chipselect;
> +	if (mxc_platform_info && master->num_chipselect) {

after spi_alloc_master(), master->num_chipselect should be always 1.
This check makes no sense for me. Probably you wonted to use
mxc_platform_info->num_chipselect instead?

>  		master->cs_gpios = devm_kzalloc(&master->dev,
>  			sizeof(int) * master->num_chipselect, GFP_KERNEL);
>  		if (!master->cs_gpios)
> @@ -1371,7 +1370,7 @@ static int spi_imx_probe(struct platform_device *pdev)
>  
>  		for (i = 0; i < master->num_chipselect; i++)
>  			master->cs_gpios[i] = mxc_platform_info->chipselect[i];
> - 	}
> +	}
>  
>  	spi_imx->bitbang.chipselect = spi_imx_chipselect;
>  	spi_imx->bitbang.setup_transfer = spi_imx_setupxfer;
> -- 
> 2.14.3
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 3/4] spi: imx: Don't require platform data chipselect array

[Oleksij Rempel]

Hi Trent,

On Thu, Oct 26, 2017 at 06:08:40PM -0700, Trent Piepho wrote:
> If the array is not present, assume all chip selects are native.  This
> is the standard behavior for SPI masters configured via the device
> tree and the behavior of this driver as well when it is configured via
> device tree.
> 
> This reduces platform data vs DT differences and allows most of the
> platform data based boards to remove their chip select arrays.
> 
> CC: Shawn Guo <shawnguo@kernel.org>
> CC: Sascha Hauer <kernel@pengutronix.de>
> CC: Fabio Estevam <fabio.estevam@nxp.com>
> CC: Mark Brown <broonie@kernel.org>
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>
> ---
>  drivers/spi/spi-imx.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index fea46cbf458a..535378ebab18 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1362,8 +1362,7 @@ static int spi_imx_probe(struct platform_device *pdev)
>  	spi_imx->devtype_data = of_id ? of_id->data :
>  		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
>  
> -	if (mxc_platform_info) {
> -		master->num_chipselect = mxc_platform_info->num_chipselect;
> +	if (mxc_platform_info && master->num_chipselect) {

after spi_alloc_master(), master->num_chipselect should be always 1.
This check makes no sense for me. Probably you wonted to use
mxc_platform_info->num_chipselect instead?

>  		master->cs_gpios = devm_kzalloc(&master->dev,
>  			sizeof(int) * master->num_chipselect, GFP_KERNEL);
>  		if (!master->cs_gpios)
> @@ -1371,7 +1370,7 @@ static int spi_imx_probe(struct platform_device *pdev)
>  
>  		for (i = 0; i < master->num_chipselect; i++)
>  			master->cs_gpios[i] = mxc_platform_info->chipselect[i];
> - 	}
> +	}
>  
>  	spi_imx->bitbang.chipselect = spi_imx_chipselect;
>  	spi_imx->bitbang.setup_transfer = spi_imx_setupxfer;
> -- 
> 2.14.3
> 
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

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

On Thu, Oct 26, 2017 at 06:08:38PM -0700, Trent Piepho wrote:
> The driver will fail to load if no gpio chip selects are specified,
> this patch changes this so that it no longer fails.
> 
> It's possible to use all native chip selects, in which case there is
> no reason to have a gpio chip select array.  This is what happens if
> the *optional* device tree property "cs-gpios" is omitted.

Do the native chip selects actually work usefully on this hardware?
There used to be problems with it wanting to do things like bounce the
chip select on every word which made it extremely difficult to use with
Linux.

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

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

On Thu, Oct 26, 2017 at 06:08:38PM -0700, Trent Piepho wrote:
> The driver will fail to load if no gpio chip selects are specified,
> this patch changes this so that it no longer fails.
> 
> It's possible to use all native chip selects, in which case there is
> no reason to have a gpio chip select array.  This is what happens if
> the *optional* device tree property "cs-gpios" is omitted.

Do the native chip selects actually work usefully on this hardware?
There used to be problems with it wanting to do things like bounce the
chip select on every word which made it extremely difficult to use with
Linux.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171031/7298bf15/attachment.sig>

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote:
> On Thu, Oct 26, 2017 at 06:08:38PM -0700, Trent Piepho wrote:
> > The driver will fail to load if no gpio chip selects are specified,
> > this patch changes this so that it no longer fails.
> > 
> > It's possible to use all native chip selects, in which case there is
> > no reason to have a gpio chip select array.  This is what happens if
> > the *optional* device tree property "cs-gpios" is omitted.
> 
> Do the native chip selects actually work usefully on this hardware?
> There used to be problems with it wanting to do things like bounce the
> chip select on every word which made it extremely difficult to use with
> Linux.

Still are annoying, but on the device we have connected to it, it ends
up working as desired.

I've not thoroughly investigated this hardware to find the details. 
IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it
was a flaw in the driver and I was able to fix it.  I've come to expect
it, as every new SPI master I use doesn't work properly in some
different way.

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote:
> On Thu, Oct 26, 2017 at 06:08:38PM -0700, Trent Piepho wrote:
> > The driver will fail to load if no gpio chip selects are specified,
> > this patch changes this so that it no longer fails.
> > 
> > It's possible to use all native chip selects, in which case there is
> > no reason to have a gpio chip select array.  This is what happens if
> > the *optional* device tree property "cs-gpios" is omitted.
> 
> Do the native chip selects actually work usefully on this hardware?
> There used to be problems with it wanting to do things like bounce the
> chip select on every word which made it extremely difficult to use with
> Linux.

Still are annoying, but on the device we have connected to it, it ends
up working as desired.

I've not thoroughly investigated this hardware to find the details. 
IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it
was a flaw in the driver and I was able to fix it.  I've come to expect
it, as every new SPI master I use doesn't work properly in some
different way.

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

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

On Tue, Oct 31, 2017 at 04:57:42PM +0000, Trent Piepho wrote:
> On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote:

> > Do the native chip selects actually work usefully on this hardware?
> > There used to be problems with it wanting to do things like bounce the
> > chip select on every word which made it extremely difficult to use with
> > Linux.

> Still are annoying, but on the device we have connected to it, it ends
> up working as desired.

> I've not thoroughly investigated this hardware to find the details. 
> IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it
> was a flaw in the driver and I was able to fix it.  I've come to expect
> it, as every new SPI master I use doesn't work properly in some
> different way.

It's one of the reasons why I'm suspicous of making the GPIO optional,
lots of chips use GPIO chipselects because a lot of the breakage is
confined to chip select handling and I remember the i.MX as being
especially far from useful.

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

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

On Tue, Oct 31, 2017 at 04:57:42PM +0000, Trent Piepho wrote:
> On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote:

> > Do the native chip selects actually work usefully on this hardware?
> > There used to be problems with it wanting to do things like bounce the
> > chip select on every word which made it extremely difficult to use with
> > Linux.

> Still are annoying, but on the device we have connected to it, it ends
> up working as desired.

> I've not thoroughly investigated this hardware to find the details. 
> IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it
> was a flaw in the driver and I was able to fix it.  I've come to expect
> it, as every new SPI master I use doesn't work properly in some
> different way.

It's one of the reasons why I'm suspicous of making the GPIO optional,
lots of chips use GPIO chipselects because a lot of the breakage is
confined to chip select handling and I remember the i.MX as being
especially far from useful.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171102/07e88703/attachment.sig>

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2889 bytes --]

On Thu, 2017-11-02 at 15:14 +0000, Mark Brown wrote:
> On Tue, Oct 31, 2017 at 04:57:42PM +0000, Trent Piepho wrote:
> > On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote:
> > > Do the native chip selects actually work usefully on this hardware?
> > > There used to be problems with it wanting to do things like bounce the
> > > chip select on every word which made it extremely difficult to use with
> > > Linux.
> > Still are annoying, but on the device we have connected to it, it ends
> > up working as desired.
> > I've not thoroughly investigated this hardware to find the details. 
> > IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it
> > was a flaw in the driver and I was able to fix it.  I've come to expect
> > it, as every new SPI master I use doesn't work properly in some
> > different way.
> 
> It's one of the reasons why I'm suspicous of making the GPIO optional,
> lots of chips use GPIO chipselects because a lot of the breakage is
> confined to chip select handling and I remember the i.MX as being
> especially far from useful.

Just to be clear, one doesn't need to use GPIOs with the driver as is. 
But the bindings to do that are non-standard and these patches make the
driver follow the standard. (and don't break anyone).

It's unfortunate that this, and in my experience every, SPI master
doesn't always (or ever) generate the waveforms it should according to
the Linux API.  But I don't think not following the specification for
the device tree bindings is a mitigation of this.  If anything, it just
creates more problems.

As someone who has done bringup a more than a few devices, I know what
I want the hardware to do, and I know the proper bindings to do that. 
But someone comes back and says they don't work and now I need to dig
into the driver source and figure out what its particular flavor of
non-standard behavior is.  I don't think that helped me any.  It didn't
tell what the hardware/driver's quirks are w.r.t. ability to follow the
Linux SPI API either.

This also happens after the hardware is designed and built, so it's a
little late to choose another pin.

If the goal is to document the hardware quirks, then shouldn't this be
done through documentation?  A note or pointer in the kconfig,
something in the source, a better description in Documention/
somewhere, etc.  That would have a better chance of being seen before
hardware is designed, and would explain the issue too, instead of just
appearing as another quirk in device tree bindings.

Also consider there is a lot of re-implemented code in each driver
relating to device tree parsing and also GPIO CS.  Non-standard
behavior in a driver doesn't help any refactoring effort.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

On Thu, 2017-11-02 at 15:14 +0000, Mark Brown wrote:
> On Tue, Oct 31, 2017 at 04:57:42PM +0000, Trent Piepho wrote:
> > On Tue, 2017-10-31 at 11:19 +0000, Mark Brown wrote:
> > > Do the native chip selects actually work usefully on this hardware?
> > > There used to be problems with it wanting to do things like bounce the
> > > chip select on every word which made it extremely difficult to use with
> > > Linux.
> > Still are annoying, but on the device we have connected to it, it ends
> > up working as desired.
> > I've not thoroughly investigated this hardware to find the details. 
> > IIRC, the designware SPI on Altera SoCFPGA had the same issue, but it
> > was a flaw in the driver and I was able to fix it.  I've come to expect
> > it, as every new SPI master I use doesn't work properly in some
> > different way.
> 
> It's one of the reasons why I'm suspicous of making the GPIO optional,
> lots of chips use GPIO chipselects because a lot of the breakage is
> confined to chip select handling and I remember the i.MX as being
> especially far from useful.

Just to be clear, one doesn't need to use GPIOs with the driver as is. 
But the bindings to do that are non-standard and these patches make the
driver follow the standard. (and don't break anyone).

It's unfortunate that this, and in my experience every, SPI master
doesn't always (or ever) generate the waveforms it should according to
the Linux API.  But I don't think not following the specification for
the device tree bindings is a mitigation of this.  If anything, it just
creates more problems.

As someone who has done bringup a more than a few devices, I know what
I want the hardware to do, and I know the proper bindings to do that. 
But someone comes back and says they don't work and now I need to dig
into the driver source and figure out what its particular flavor of
non-standard behavior is.  I don't think that helped me any.  It didn't
tell what the hardware/driver's quirks are w.r.t. ability to follow the
Linux SPI API either.

This also happens after the hardware is designed and built, so it's a
little late to choose another pin.

If the goal is to document the hardware quirks, then shouldn't this be
done through documentation?  A note or pointer in the kconfig,
something in the source, a better description in Documention/
somewhere, etc.  That would have a better chance of being seen before
hardware is designed, and would explain the issue too, instead of just
appearing as another quirk in device tree bindings.

Also consider there is a lot of re-implemented code in each driver
relating to device tree parsing and also GPIO CS.  Non-standard
behavior in a driver doesn't help any refactoring effort.

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

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

On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote:

> Just to be clear, one doesn't need to use GPIOs with the driver as is. 
> But the bindings to do that are non-standard and these patches make the
> driver follow the standard. (and don't break anyone).

If there are non-standard bindings then mark them as deprecated.  I
can't immediately find *any* binding documentation for this controller.
The last commit looks like it was more attempting to work round broken
board bindings and do something sensible than add a new binding, at
least that's what I remember my sense of it being.

> It's unfortunate that this, and in my experience every, SPI master
> doesn't always (or ever) generate the waveforms it should according to
> the Linux API.  But I don't think not following the specification for
> the device tree bindings is a mitigation of this.  If anything, it just
> creates more problems.

If the hardware is as broken as these controllers always were in the
past and there are workarounds which work in all practical situations
(AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)
documenting something as supported is just going to make people
miserable.  The reason I know about this breakage is that I had to go
through the process of working out that the native chip select support
didn't work on a system.

> If the goal is to document the hardware quirks, then shouldn't this be
> done through documentation?  A note or pointer in the kconfig,
> something in the source, a better description in Documention/
> somewhere, etc.  That would have a better chance of being seen before
> hardware is designed, and would explain the issue too, instead of just
> appearing as another quirk in device tree bindings.

Yes, better documentation would be great.

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

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote:

> Just to be clear, one doesn't need to use GPIOs with the driver as is. 
> But the bindings to do that are non-standard and these patches make the
> driver follow the standard. (and don't break anyone).

If there are non-standard bindings then mark them as deprecated.  I
can't immediately find *any* binding documentation for this controller.
The last commit looks like it was more attempting to work round broken
board bindings and do something sensible than add a new binding, at
least that's what I remember my sense of it being.

> It's unfortunate that this, and in my experience every, SPI master
> doesn't always (or ever) generate the waveforms it should according to
> the Linux API.  But I don't think not following the specification for
> the device tree bindings is a mitigation of this.  If anything, it just
> creates more problems.

If the hardware is as broken as these controllers always were in the
past and there are workarounds which work in all practical situations
(AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)
documenting something as supported is just going to make people
miserable.  The reason I know about this breakage is that I had to go
through the process of working out that the native chip select support
didn't work on a system.

> If the goal is to document the hardware quirks, then shouldn't this be
> done through documentation?  A note or pointer in the kconfig,
> something in the source, a better description in Documention/
> somewhere, etc.  That would have a better chance of being seen before
> hardware is designed, and would explain the issue too, instead of just
> appearing as another quirk in device tree bindings.

Yes, better documentation would be great.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171103/bcafb781/attachment.sig>

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3189 bytes --]

On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote:
> On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote:
> 
> > Just to be clear, one doesn't need to use GPIOs with the driver as is. 
> > But the bindings to do that are non-standard and these patches make the
> > driver follow the standard. (and don't break anyone).
> 
> If there are non-standard bindings then mark them as deprecated.  I
> can't immediately find *any* binding documentation for this controller.
> The last commit looks like it was more attempting to work round broken
> board bindings and do something sensible than add a new binding, at
> least that's what I remember my sense of it being.

The non-standard part is needing to add cs-gpios = <0> to get a native
chip select when that is documented as being optional.  It doesn't
follow the spec.  It doesn't match other drivers (and dw-spi is equally
as broken in the same manner, probably others too) that do follow the
spec.

> If the hardware is as broken as these controllers always were in the
> past and there are workarounds which work in all practical situations
> (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)

Comments in the driver indicate that some SoCs do not allow GPIO usage
on all chip select pins.

> documenting something as supported is just going to make people
> miserable.  The reason I know about this breakage is that I had to go
> through the process of working out that the native chip select support
> didn't work on a system.

I just don't see how not following the device tree binding
specification documents the hardware flaw, or how following the spec
documents that the flaw does not exist.

> > If the goal is to document the hardware quirks, then shouldn't this be
> > done through documentation?  A note or pointer in the kconfig,
> > something in the source, a better description in Documention/
> > somewhere, etc.  That would have a better chance of being seen before
> > hardware is designed, and would explain the issue too, instead of just
> > appearing as another quirk in device tree bindings.
> 
> Yes, better documentation would be great.

How about I add something to Documentation/spi and add a note in
Kconfig, but make the driver standard compliant in its device tree
bindings?

The goal here is that everyone doesn't have to stick a scope on the
board and re-discover that the native CS doesn't do what they want,
right? Been there, done that, and can appreciate the sentiment.

I don't think not following the standard is an effective way to do
that.  From a sample of three developers, two came up with dt bindings
to use native cs and decided they apparently misunderstood the spi dt
spec to explain why what should have worked did not.  The third (me)
looked into the driver source to find the why, and even then it wasn't
clear the behavior was intentional or an oversight.  I think
documenting the known flaws would be a better and more effective way to
get that information across.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote:
> On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote:
> 
> > Just to be clear, one doesn't need to use GPIOs with the driver as is. 
> > But the bindings to do that are non-standard and these patches make the
> > driver follow the standard. (and don't break anyone).
> 
> If there are non-standard bindings then mark them as deprecated.  I
> can't immediately find *any* binding documentation for this controller.
> The last commit looks like it was more attempting to work round broken
> board bindings and do something sensible than add a new binding, at
> least that's what I remember my sense of it being.

The non-standard part is needing to add cs-gpios = <0> to get a native
chip select when that is documented as being optional.  It doesn't
follow the spec.  It doesn't match other drivers (and dw-spi is equally
as broken in the same manner, probably others too) that do follow the
spec.

> If the hardware is as broken as these controllers always were in the
> past and there are workarounds which work in all practical situations
> (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)

Comments in the driver indicate that some SoCs do not allow GPIO usage
on all chip select pins.

> documenting something as supported is just going to make people
> miserable.  The reason I know about this breakage is that I had to go
> through the process of working out that the native chip select support
> didn't work on a system.

I just don't see how not following the device tree binding
specification documents the hardware flaw, or how following the spec
documents that the flaw does not exist.

> > If the goal is to document the hardware quirks, then shouldn't this be
> > done through documentation?  A note or pointer in the kconfig,
> > something in the source, a better description in Documention/
> > somewhere, etc.  That would have a better chance of being seen before
> > hardware is designed, and would explain the issue too, instead of just
> > appearing as another quirk in device tree bindings.
> 
> Yes, better documentation would be great.

How about I add something to Documentation/spi and add a note in
Kconfig, but make the driver standard compliant in its device tree
bindings?

The goal here is that everyone doesn't have to stick a scope on the
board and re-discover that the native CS doesn't do what they want,
right? Been there, done that, and can appreciate the sentiment.

I don't think not following the standard is an effective way to do
that.  From a sample of three developers, two came up with dt bindings
to use native cs and decided they apparently misunderstood the spi dt
spec to explain why what should have worked did not.  The third (me)
looked into the driver source to find the why, and even then it wasn't
clear the behavior was intentional or an oversight.  I think
documenting the known flaws would be a better and more effective way to
get that information across.

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

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

On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote:
> On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote:

> > If there are non-standard bindings then mark them as deprecated.  I
> > can't immediately find *any* binding documentation for this controller.
> > The last commit looks like it was more attempting to work round broken
> > board bindings and do something sensible than add a new binding, at
> > least that's what I remember my sense of it being.

> The non-standard part is needing to add cs-gpios = <0> to get a native
> chip select when that is documented as being optional.  It doesn't
> follow the spec.  It doesn't match other drivers (and dw-spi is equally
> as broken in the same manner, probably others too) that do follow the
> spec.

Is there any documentation of the bindings for this driver at all?  I
wasn't able to find it.

> > If the hardware is as broken as these controllers always were in the
> > past and there are workarounds which work in all practical situations
> > (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)

> Comments in the driver indicate that some SoCs do not allow GPIO usage
> on all chip select pins.

Ah, that's an issue.  We will need support for the hardware chip selects
then.

> > documenting something as supported is just going to make people
> > miserable.  The reason I know about this breakage is that I had to go
> > through the process of working out that the native chip select support
> > didn't work on a system.

> I just don't see how not following the device tree binding
> specification documents the hardware flaw, or how following the spec
> documents that the flaw does not exist.

Hardware chip selects aren't present in all controllers and at times
have entertaining enumerations in the hardware, the details on them need
to be covered in the device specific binding (which like I say seems to
be missing for this controller).  If they just don't work well the
kindest thing may be to not support them and document the binding that
way.

> > Yes, better documentation would be great.

> How about I add something to Documentation/spi and add a note in
> Kconfig, but make the driver standard compliant in its device tree
> bindings?

Writing a binding document for the controller would probably cover it.

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

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote:
> On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote:

> > If there are non-standard bindings then mark them as deprecated.  I
> > can't immediately find *any* binding documentation for this controller.
> > The last commit looks like it was more attempting to work round broken
> > board bindings and do something sensible than add a new binding, at
> > least that's what I remember my sense of it being.

> The non-standard part is needing to add cs-gpios = <0> to get a native
> chip select when that is documented as being optional.  It doesn't
> follow the spec.  It doesn't match other drivers (and dw-spi is equally
> as broken in the same manner, probably others too) that do follow the
> spec.

Is there any documentation of the bindings for this driver at all?  I
wasn't able to find it.

> > If the hardware is as broken as these controllers always were in the
> > past and there are workarounds which work in all practical situations
> > (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)

> Comments in the driver indicate that some SoCs do not allow GPIO usage
> on all chip select pins.

Ah, that's an issue.  We will need support for the hardware chip selects
then.

> > documenting something as supported is just going to make people
> > miserable.  The reason I know about this breakage is that I had to go
> > through the process of working out that the native chip select support
> > didn't work on a system.

> I just don't see how not following the device tree binding
> specification documents the hardware flaw, or how following the spec
> documents that the flaw does not exist.

Hardware chip selects aren't present in all controllers and at times
have entertaining enumerations in the hardware, the details on them need
to be covered in the device specific binding (which like I say seems to
be missing for this controller).  If they just don't work well the
kindest thing may be to not support them and document the binding that
way.

> > Yes, better documentation would be great.

> How about I add something to Documentation/spi and add a note in
> Kconfig, but make the driver standard compliant in its device tree
> bindings?

Writing a binding document for the controller would probably cover it.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171103/f7be4aaa/attachment.sig>

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2922 bytes --]

On Fri, 2017-11-03 at 19:36 +0000, Mark Brown wrote:
> On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote:
> > On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote:
> > > If there are non-standard bindings then mark them as deprecated.  I
> > > can't immediately find *any* binding documentation for this controller.
> > > The last commit looks like it was more attempting to work round broken
> > > board bindings and do something sensible than add a new binding, at
> > > least that's what I remember my sense of it being.
> > The non-standard part is needing to add cs-gpios = <0> to get a native
> > chip select when that is documented as being optional.  It doesn't
> > follow the spec.  It doesn't match other drivers (and dw-spi is equally
> > as broken in the same manner, probably others too) that do follow the
> > spec.
> 
> Is there any documentation of the bindings for this driver at all?  I
> wasn't able to find it.

Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt

Doesn't note anything special about the native chip selects.

> > > If the hardware is as broken as these controllers always were in the
> > > past and there are workarounds which work in all practical situations
> > > (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)
> > Comments in the driver indicate that some SoCs do not allow GPIO usage
> > on all chip select pins.
> 
> Ah, that's an issue.  We will need support for the hardware chip selects
> then.

And they are already supported too.  The issue I want to fix is the
need for non-standard bindings to use them.

> > > documenting something as supported is just going to make people
> > > miserable.  The reason I know about this breakage is that I had to go
> > > through the process of working out that the native chip select support
> > > didn't work on a system.
> > I just don't see how not following the device tree binding
> > specification documents the hardware flaw, or how following the spec
> > documents that the flaw does not exist.
> 
> Hardware chip selects aren't present in all controllers and at times
> have entertaining enumerations in the hardware, the details on them need
> to be covered in the device specific binding (which like I say seems to
> be missing for this controller).  If they just don't work well the
> kindest thing may be to not support them and document the binding that
> way.
> 
> > > Yes, better documentation would be great.
> > How about I add something to Documentation/spi and add a note in
> > Kconfig, but make the driver standard compliant in its device tree
> > bindings?
> 
> Writing a binding document for the controller would probably cover it.

Ok, I'll adjust the series to document the real issue.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

On Fri, 2017-11-03 at 19:36 +0000, Mark Brown wrote:
> On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote:
> > On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote:
> > > If there are non-standard bindings then mark them as deprecated.  I
> > > can't immediately find *any* binding documentation for this controller.
> > > The last commit looks like it was more attempting to work round broken
> > > board bindings and do something sensible than add a new binding, at
> > > least that's what I remember my sense of it being.
> > The non-standard part is needing to add cs-gpios = <0> to get a native
> > chip select when that is documented as being optional.  It doesn't
> > follow the spec.  It doesn't match other drivers (and dw-spi is equally
> > as broken in the same manner, probably others too) that do follow the
> > spec.
> 
> Is there any documentation of the bindings for this driver at all?  I
> wasn't able to find it.

Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt

Doesn't note anything special about the native chip selects.

> > > If the hardware is as broken as these controllers always were in the
> > > past and there are workarounds which work in all practical situations
> > > (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)
> > Comments in the driver indicate that some SoCs do not allow GPIO usage
> > on all chip select pins.
> 
> Ah, that's an issue.  We will need support for the hardware chip selects
> then.

And they are already supported too.  The issue I want to fix is the
need for non-standard bindings to use them.

> > > documenting something as supported is just going to make people
> > > miserable.  The reason I know about this breakage is that I had to go
> > > through the process of working out that the native chip select support
> > > didn't work on a system.
> > I just don't see how not following the device tree binding
> > specification documents the hardware flaw, or how following the spec
> > documents that the flaw does not exist.
> 
> Hardware chip selects aren't present in all controllers and at times
> have entertaining enumerations in the hardware, the details on them need
> to be covered in the device specific binding (which like I say seems to
> be missing for this controller).  If they just don't work well the
> kindest thing may be to not support them and document the binding that
> way.
> 
> > > Yes, better documentation would be great.
> > How about I add something to Documentation/spi and add a note in
> > Kconfig, but make the driver standard compliant in its device tree
> > bindings?
> 
> Writing a binding document for the controller would probably cover it.

Ok, I'll adjust the series to document the real issue.

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

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

On Fri, Nov 03, 2017 at 08:09:19PM +0000, Trent Piepho wrote:
> On Fri, 2017-11-03 at 19:36 +0000, Mark Brown wrote:

> > Is there any documentation of the bindings for this driver at all?  I
> > wasn't able to find it.

> Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt

Ah, good - for some reason I'd thought that was a different IP block
(SPI controllers are like serial ports, endless room for innovation!).

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

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Mark Brown]

On Fri, Nov 03, 2017 at 08:09:19PM +0000, Trent Piepho wrote:
> On Fri, 2017-11-03 at 19:36 +0000, Mark Brown wrote:

> > Is there any documentation of the bindings for this driver at all?  I
> > wasn't able to find it.

> Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt

Ah, good - for some reason I'd thought that was a different IP block
(SPI controllers are like serial ports, endless room for innovation!).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171103/89aabf5a/attachment.sig>

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Sascha Hauer]

On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote:
> On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote:
> > On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote:
> > 
> > > Just to be clear, one doesn't need to use GPIOs with the driver as is. 
> > > But the bindings to do that are non-standard and these patches make the
> > > driver follow the standard. (and don't break anyone).
> > 
> > If there are non-standard bindings then mark them as deprecated.  I
> > can't immediately find *any* binding documentation for this controller.
> > The last commit looks like it was more attempting to work round broken
> > board bindings and do something sensible than add a new binding, at
> > least that's what I remember my sense of it being.
> 
> The non-standard part is needing to add cs-gpios = <0> to get a native
> chip select when that is documented as being optional.  It doesn't
> follow the spec.  It doesn't match other drivers (and dw-spi is equally
> as broken in the same manner, probably others too) that do follow the
> spec.
> 
> > If the hardware is as broken as these controllers always were in the
> > past and there are workarounds which work in all practical situations
> > (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)
> 
> Comments in the driver indicate that some SoCs do not allow GPIO usage
> on all chip select pins.

Which comments do you mean? I remember there were comments, but I can't
find any in the driver.

The only SoC which has dedicated SPI CS pins which can't be configured
as GPIOs is the i.MX31. Unless you have an i.MX31 you shouldn't be
forced to use native chip selects.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Sascha Hauer]

On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote:
> On Fri, 2017-11-03 at 18:37 +0000, Mark Brown wrote:
> > On Fri, Nov 03, 2017 at 05:53:59PM +0000, Trent Piepho wrote:
> > 
> > > Just to be clear, one doesn't need to use GPIOs with the driver as is. 
> > > But the bindings to do that are non-standard and these patches make the
> > > driver follow the standard. (and don't break anyone).
> > 
> > If there are non-standard bindings then mark them as deprecated.  I
> > can't immediately find *any* binding documentation for this controller.
> > The last commit looks like it was more attempting to work round broken
> > board bindings and do something sensible than add a new binding, at
> > least that's what I remember my sense of it being.
> 
> The non-standard part is needing to add cs-gpios = <0> to get a native
> chip select when that is documented as being optional.  It doesn't
> follow the spec.  It doesn't match other drivers (and dw-spi is equally
> as broken in the same manner, probably others too) that do follow the
> spec.
> 
> > If the hardware is as broken as these controllers always were in the
> > past and there are workarounds which work in all practical situations
> > (AFAIK all the relevant SoCs permit GPIO usage on the chip select pins)
> 
> Comments in the driver indicate that some SoCs do not allow GPIO usage
> on all chip select pins.

Which comments do you mean? I remember there were comments, but I can't
find any in the driver.

The only SoC which has dedicated SPI CS pins which can't be configured
as GPIOs is the i.MX31. Unless you have an i.MX31 you shouldn't be
forced to use native chip selects.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

On Mon, 2017-11-06 at 08:32 +0100, Sascha Hauer wrote:
> On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote:
> > 
> > Comments in the driver indicate that some SoCs do not allow GPIO usage
> > on all chip select pins.
> 
> Which comments do you mean? I remember there were comments, but I can't
> find any in the driver.

It's in arch/arm/plat-mxc/include/mach/spi.h.

> The only SoC which has dedicated SPI CS pins which can't be configured
> as GPIOs is the i.MX31. Unless you have an i.MX31 you shouldn't be
> forced to use native chip selects.

Indeed I'm not forced to, but native is more efficient and faster, and
since I'm lucky enough that they work for me, I'd like to be able to
use them.  And I can already, but the DT bindings are non-standard and
I don't see any value in that. DT bindings are complex enough without
drivers adding quirks. And my experience was no one connected the non-
standard bindings with native CS not working well.  I think explicit
documentation will be more effective at that.

[PATCH v2 1/4] spi: imx: GPIO based chip selects should not be required

[Trent Piepho]

On Mon, 2017-11-06 at 08:32 +0100, Sascha Hauer wrote:
> On Fri, Nov 03, 2017 at 07:18:56PM +0000, Trent Piepho wrote:
> > 
> > Comments in the driver indicate that some SoCs do not allow GPIO usage
> > on all chip select pins.
> 
> Which comments do you mean? I remember there were comments, but I can't
> find any in the driver.

It's in arch/arm/plat-mxc/include/mach/spi.h.

> The only SoC which has dedicated SPI CS pins which can't be configured
> as GPIOs is the i.MX31. Unless you have an i.MX31 you shouldn't be
> forced to use native chip selects.

Indeed I'm not forced to, but native is more efficient and faster, and
since I'm lucky enough that they work for me, I'd like to be able to
use them.  And I can already, but the DT bindings are non-standard and
I don't see any value in that. DT bindings are complex enough without
drivers adding quirks. And my experience was no one connected the non-
standard bindings with native CS not working well.  I think explicit
documentation will be more effective at that.