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

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

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

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

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

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

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

[Oleksij Rempel]

On Fri, Oct 13, 2017 at 12:54:08PM -0700, Trent Piepho wrote:
> 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.
> 
> 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.13.6

Reviewed-by: Oleksij Rempel <o.rempel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

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

Signed-off-by: Trent Piepho <tpiepho-cgc2CodaaHDQT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi-imx.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index fea46cbf458a..3a2c34522655 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1364,13 +1364,15 @@ static int spi_imx_probe(struct platform_device *pdev)
 
 	if (mxc_platform_info) {
 		master->num_chipselect = mxc_platform_info->num_chipselect;
-		master->cs_gpios = devm_kzalloc(&master->dev,
-			sizeof(int) * master->num_chipselect, GFP_KERNEL);
-		if (!master->cs_gpios)
-			return -ENOMEM;
-
-		for (i = 0; i < master->num_chipselect; i++)
-			master->cs_gpios[i] = mxc_platform_info->chipselect[i];
+		if (mxc_platform_info->chipselect) {
+			master->cs_gpios = devm_kzalloc(&master->dev,
+				sizeof(int) * master->num_chipselect, GFP_KERNEL);
+			if (!master->cs_gpios)
+				return -ENOMEM;
+
+			for (i = 0; i < master->num_chipselect; i++)
+				master->cs_gpios[i] = mxc_platform_info->chipselect[i];
+		}
  	}
 
 	spi_imx->bitbang.chipselect = spi_imx_chipselect;
-- 
2.13.6

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

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

[Julien Thierry]

Hi Trent,

On 13/10/17 20:54, 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.
> 
> Signed-off-by: Trent Piepho <tpiepho@impinj.com>
> ---
>   drivers/spi/spi-imx.c | 16 +++++++++-------
>   1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index fea46cbf458a..3a2c34522655 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1364,13 +1364,15 @@ static int spi_imx_probe(struct platform_device *pdev)
>   
>   	if (mxc_platform_info) {
>   		master->num_chipselect = mxc_platform_info->num_chipselect;

nit:
This is only useful when num_chipselect is non-zero (master's memory is 
zeroed on allocation). So maybe this could be simplified a bit more as:

if (mxc_platform_info && mxc_platform_info->chipselect) {
	master->num_chipselect = mxc_platform_info->num_chipselect;
	[...]
}

Reducing an indentation level for all the following statements.

Cheers,

> -		master->cs_gpios = devm_kzalloc(&master->dev,
> -			sizeof(int) * master->num_chipselect, GFP_KERNEL);
> -		if (!master->cs_gpios)
> -			return -ENOMEM;
> -
> -		for (i = 0; i < master->num_chipselect; i++)
> -			master->cs_gpios[i] = mxc_platform_info->chipselect[i];
> +		if (mxc_platform_info->chipselect) {
> +			master->cs_gpios = devm_kzalloc(&master->dev,
> +				sizeof(int) * master->num_chipselect, GFP_KERNEL);
> +			if (!master->cs_gpios)
> +				return -ENOMEM;
> +
> +			for (i = 0; i < master->num_chipselect; i++)
> +				master->cs_gpios[i] = mxc_platform_info->chipselect[i];
> +		}
>    	}
>   
>   	spi_imx->bitbang.chipselect = spi_imx_chipselect;
> 

-- 
Julien Thierry

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

[Trent Piepho]

On Wed, 2017-10-18 at 10:02 +0100, Julien Thierry wrote:
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -1364,13 +1364,15 @@ static int spi_imx_probe(struct platform_device *pdev)
> >   
> >   	if (mxc_platform_info) {
> >   		master->num_chipselect = mxc_platform_info->num_chipselect;
> 
> nit:
> This is only useful when num_chipselect is non-zero (master's memory is 
> zeroed on allocation). So maybe this could be simplified a bit more as:
> 
> if (mxc_platform_info && mxc_platform_info->chipselect) {
> 	master->num_chipselect = mxc_platform_info->num_chipselect;
> 	[...]
> }
> 
> Reducing an indentation level for all the following statements.

Good point, there's nothing else in the platform info to use.

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

[Trent Piepho]

On Wed, 2017-10-18 at 10:02 +0100, Julien Thierry wrote:
> --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -1364,13 +1364,15 @@ static int spi_imx_probe(struct platform_device *pdev)
> >   
> >   	if (mxc_platform_info) {
> >   		master->num_chipselect = mxc_platform_info->num_chipselect;
> 
> nit:
> This is only useful when num_chipselect is non-zero (master's memory is 
> zeroed on allocation). So maybe this could be simplified a bit more as:
> 
> if (mxc_platform_info && mxc_platform_info->chipselect) {
> 	master->num_chipselect = mxc_platform_info->num_chipselect;
> 	[...]
> }
> 
> Reducing an indentation level for all the following statements.

Thought about this some more, and it doesn't work to do that.  If
chipselect is NULL, platform data is still allowed to set the number of
chipselects using mcx_platform_info->num_chipselect.

> 
> > -			sizeof(int) * master->num_chipselect, GFP_KERNEL);
> > -		if (!master->cs_gpios)
> > -			return -ENOMEM;
> > -
> > -		for (i = 0; i < master->num_chipselect; i++)
> > -			master->cs_gpios[i] = mxc_platform_info->chipselect[i];
> > +		if (mxc_platform_info->chipselect) {
> > +			master->cs_gpios = devm_kzalloc(&master->dev,
> > +				sizeof(int) * master->num_chipselect, GFP_KERNEL);
> > +			if (!master->cs_gpios)
> > +				return -ENOMEM;
> > +
> > +			for (i = 0; i < master->num_chipselect; i++)
> > +				master->cs_gpios[i] = mxc_platform_info->chipselect[i];
> > +		}
> >    	}
> >   
> >   	spi_imx->bitbang.chipselect = spi_imx_chipselect;
> > 
> 
> 

[PATCH 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: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@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>
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.13.6

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

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

[Greg Ungerer]

Hi Trent,

On 14/10/17 05:54, Trent Piepho wrote:
> 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: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>

Looks like a nice cleanup.
For whatever it is worth:

Acked-by: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org>

Regards
Greg



> 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>
> 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_*/
> 

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

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

[Shawn Guo]

On Fri, Oct 13, 2017 at 12:54:10PM -0700, Trent Piepho wrote:
> 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: Greg Ungerer <gerg-Td1EMuHUCqxL1ZNQvxDV9g@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>
> Signed-off-by: Trent Piepho <tpiepho-cgc2CodaaHDQT0dZR+AlfA@public.gmane.org>

Does this patch have any dependency on others in this series, or can it
be sent via arm-soc tree independently?

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

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

[Trent Piepho]

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

On Wed, 2017-10-18 at 10:17 +0800, Shawn Guo wrote:
> On Fri, Oct 13, 2017 at 12:54:10PM -0700, Trent Piepho wrote:
> > 
> > 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.
> > 
> > 
> Does this patch have any dependency on others in this series, or can it
> be sent via arm-soc tree independently?

It needs patch 1 and 3 in the series.  Currently supplying cs gpios is
mandatory for both platform data and DT based systems using spi-imx. 
The latter is a bug, as the DT binding clearly makes cs-gpios optional
and other drivers do not require it.  I figured I might as well clean
up the platform data while I fixed that bug and came across the
imx31moboard bug in that process.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±²˜¢žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f