[PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Michael Welling]

GPIO chip select patch series appears to have broken the native chip select
support. This patch pulls the manual native chip select toggling out of
the transfer_one routine and adds a set_cs routine.

Tested natively on AM3354 with SPI serial flash on spi0cs0.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/spi/spi-omap2-mcspi.c |   33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 90cf7e7..a7d85c5 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -243,17 +243,20 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
 	mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCTRL0);
 }
 
-static void omap2_mcspi_force_cs(struct spi_device *spi, int cs_active)
+static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
 {
 	u32 l;
 
-	l = mcspi_cached_chconf0(spi);
-	if (cs_active)
-		l |= OMAP2_MCSPI_CHCONF_FORCE;
-	else
-		l &= ~OMAP2_MCSPI_CHCONF_FORCE;
+	if (spi->controller_state) {
+		l = mcspi_cached_chconf0(spi);
 
-	mcspi_write_chconf0(spi, l);
+		if (enable)
+			l &= ~OMAP2_MCSPI_CHCONF_FORCE;
+		else
+			l |= OMAP2_MCSPI_CHCONF_FORCE;
+
+		mcspi_write_chconf0(spi, l);
+	}
 }
 
 static void omap2_mcspi_set_master_mode(struct spi_master *master)
@@ -1075,7 +1078,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 
 	struct spi_master		*master;
 	struct omap2_mcspi_dma		*mcspi_dma;
-	int				cs_active = 0;
 	struct omap2_mcspi_cs		*cs;
 	struct omap2_mcspi_device_config *cd;
 	int				par_override = 0;
@@ -1118,11 +1120,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 			mcspi_read_cs_reg(spi, OMAP2_MCSPI_MODULCTRL);
 	}
 
-	if (!cs_active) {
-		omap2_mcspi_force_cs(spi, 1);
-		cs_active = 1;
-	}
-
 	chconf = mcspi_cached_chconf0(spi);
 	chconf &= ~OMAP2_MCSPI_CHCONF_TRM_MASK;
 	chconf &= ~OMAP2_MCSPI_CHCONF_TURBO;
@@ -1169,12 +1166,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 	if (t->delay_usecs)
 		udelay(t->delay_usecs);
 
-	/* ignore the "leave it on after last xfer" hint */
-	if (t->cs_change) {
-		omap2_mcspi_force_cs(spi, 0);
-		cs_active = 0;
-	}
-
 	omap2_mcspi_set_enable(spi, 0);
 
 	if (mcspi->fifo_depth > 0)
@@ -1187,9 +1178,6 @@ out:
 		status = omap2_mcspi_setup_transfer(spi, NULL);
 	}
 
-	if (cs_active)
-		omap2_mcspi_force_cs(spi, 0);
-
 	if (cd && cd->cs_per_word) {
 		chconf = mcspi->ctx.modulctrl;
 		chconf |= OMAP2_MCSPI_MODULCTRL_SINGLE;
@@ -1334,6 +1322,7 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
 	master->setup = omap2_mcspi_setup;
 	master->auto_runtime_pm = true;
 	master->transfer_one = omap2_mcspi_transfer_one;
+	master->set_cs = omap2_mcspi_set_cs;
 	master->cleanup = omap2_mcspi_cleanup;
 	master->dev.of_node = node;
 	master->max_speed_hz = OMAP2_MCSPI_MAX_FREQ;
-- 
1.7.9.5

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Nishanth Menon]

On 05/12/2015 12:38 PM, Michael Welling wrote:
> GPIO chip select patch series appears to have broken the native chip select
> support. This patch pulls the manual native chip select toggling out of
> the transfer_one routine and adds a set_cs routine.
> 
> Tested natively on AM3354 with SPI serial flash on spi0cs0.
> 
> Signed-off-by: Michael Welling <mwelling@ieee.org>
> ---
>  drivers/spi/spi-omap2-mcspi.c |   33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> index 90cf7e7..a7d85c5 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -243,17 +243,20 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
>  	mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCTRL0);
>  }
>  
> -static void omap2_mcspi_force_cs(struct spi_device *spi, int cs_active)
> +static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>  {
>  	u32 l;
>  
> -	l = mcspi_cached_chconf0(spi);
> -	if (cs_active)
> -		l |= OMAP2_MCSPI_CHCONF_FORCE;
> -	else
> -		l &= ~OMAP2_MCSPI_CHCONF_FORCE;
> +	if (spi->controller_state) {
> +		l = mcspi_cached_chconf0(spi);
>  
> -	mcspi_write_chconf0(spi, l);
> +		if (enable)
> +			l &= ~OMAP2_MCSPI_CHCONF_FORCE;
> +		else
> +			l |= OMAP2_MCSPI_CHCONF_FORCE;
> +
> +		mcspi_write_chconf0(spi, l);
> +	}
>  }
>  
>  static void omap2_mcspi_set_master_mode(struct spi_master *master)
> @@ -1075,7 +1078,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
>  
>  	struct spi_master		*master;
>  	struct omap2_mcspi_dma		*mcspi_dma;
> -	int				cs_active = 0;
>  	struct omap2_mcspi_cs		*cs;
>  	struct omap2_mcspi_device_config *cd;
>  	int				par_override = 0;
> @@ -1118,11 +1120,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
>  			mcspi_read_cs_reg(spi, OMAP2_MCSPI_MODULCTRL);
>  	}
>  
> -	if (!cs_active) {
> -		omap2_mcspi_force_cs(spi, 1);
> -		cs_active = 1;
> -	}
> -
>  	chconf = mcspi_cached_chconf0(spi);
>  	chconf &= ~OMAP2_MCSPI_CHCONF_TRM_MASK;
>  	chconf &= ~OMAP2_MCSPI_CHCONF_TURBO;
> @@ -1169,12 +1166,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
>  	if (t->delay_usecs)
>  		udelay(t->delay_usecs);
>  
> -	/* ignore the "leave it on after last xfer" hint */
> -	if (t->cs_change) {
> -		omap2_mcspi_force_cs(spi, 0);
> -		cs_active = 0;
> -	}
> -
>  	omap2_mcspi_set_enable(spi, 0);
>  
>  	if (mcspi->fifo_depth > 0)
> @@ -1187,9 +1178,6 @@ out:
>  		status = omap2_mcspi_setup_transfer(spi, NULL);
>  	}
>  
> -	if (cs_active)
> -		omap2_mcspi_force_cs(spi, 0);
> -
>  	if (cd && cd->cs_per_word) {
>  		chconf = mcspi->ctx.modulctrl;
>  		chconf |= OMAP2_MCSPI_MODULCTRL_SINGLE;
> @@ -1334,6 +1322,7 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
>  	master->setup = omap2_mcspi_setup;
>  	master->auto_runtime_pm = true;
>  	master->transfer_one = omap2_mcspi_transfer_one;
> +	master->set_cs = omap2_mcspi_set_cs;
>  	master->cleanup = omap2_mcspi_cleanup;
>  	master->dev.of_node = node;
>  	master->max_speed_hz = OMAP2_MCSPI_MAX_FREQ;
> 

Tested on next-20150512
http://paste.ubuntu.org.cn/2600748
Since the original issue was reported by me in
http://marc.info/?t=143136312900001&r=1&w=2
Reported-by: Nishanth Menon <nm@ti.com>

Tested-by: Nishanth Menon <nm@ti.com>

-- 
Regards,
Nishanth Menon

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Mark Brown]

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

On Tue, May 12, 2015 at 12:38:57PM -0500, Michael Welling wrote:
> GPIO chip select patch series appears to have broken the native chip select
> support. This patch pulls the manual native chip select toggling out of
> the transfer_one routine and adds a set_cs routine.

Applied, thanks

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

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Mark Brown]

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

On Tue, May 12, 2015 at 01:57:36PM -0500, Nishanth Menon wrote:
> On 05/12/2015 12:38 PM, Michael Welling wrote:
> > GPIO chip select patch series appears to have broken the native chip select
> > support. This patch pulls the manual native chip select toggling out of
> > the transfer_one routine and adds a set_cs routine.

Please remember to delete unneeded context from your replies, the reader
shouldn't need to page through the entire patch to find out you reviewed
it.

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

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Nishanth Menon]

On 05/12/2015 02:19 PM, Mark Brown wrote:
> On Tue, May 12, 2015 at 01:57:36PM -0500, Nishanth Menon wrote:
>> On 05/12/2015 12:38 PM, Michael Welling wrote:
>>> GPIO chip select patch series appears to have broken the native chip select
>>> support. This patch pulls the manual native chip select toggling out of
>>> the transfer_one routine and adds a set_cs routine.
> 
> Please remember to delete unneeded context from your replies, the reader
> shouldn't need to page through the entire patch to find out you reviewed
> it.

Will do so. Anyways, I did test it to be accurate - have'nt reviewed it.


-- 
Regards,
Nishanth Menon

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Michael Welling]

On Tue, May 12, 2015 at 08:17:58PM +0100, Mark Brown wrote:
> On Tue, May 12, 2015 at 12:38:57PM -0500, Michael Welling wrote:
> > GPIO chip select patch series appears to have broken the native chip select
> > support. This patch pulls the manual native chip select toggling out of
> > the transfer_one routine and adds a set_cs routine.
> 
> Applied, thanks

It appears that in haste, this fix for the native cs support broke
the GPIO chip select support that I was originally shooting for.

[    2.653658] mcp23s08 spi1.1: TXS timed out
[    2.657961] mcp23s08 spi1.1: SPI transfer failed: -5
[    2.663305] spi_master spi1: failed to transfer one message from queue
[    2.670172] mcp23s08 spi1.1: can't setup chip 64, --> -5
[    2.675784] GPIO chip mcp23s08: gpiochip_unexport: status -19

My guess is that the set_cs needs to be called even when toggling as GPIO.

How should I handle this?


--
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] spi: omap2-mcspi: Fix native cs with new set_cs

[Mark Brown]

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

On Wed, May 20, 2015 at 09:07:09PM -0500, Michael Welling wrote:

> My guess is that the set_cs needs to be called even when toggling as GPIO.

> How should I handle this?

It shouldn't be part of a set_cs() operation but rather part of the main
transfer operation.

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

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Michael Welling]

On Thu, May 21, 2015 at 11:18:57AM +0100, Mark Brown wrote:
> On Wed, May 20, 2015 at 09:07:09PM -0500, Michael Welling wrote:
> 
> > My guess is that the set_cs needs to be called even when toggling as GPIO.
> 
> > How should I handle this?
> 
> It shouldn't be part of a set_cs() operation but rather part of the main
> transfer operation.


Okay then this patch should be reverted.

Do you want to revert the patch and apply a new one or should I provide a
patch that reverts the changes and fixes it all in one?

Sorry for this mess.

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Mark Brown]

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

On Thu, May 21, 2015 at 04:04:11PM -0500, Michael Welling wrote:

> Do you want to revert the patch and apply a new one or should I provide a
> patch that reverts the changes and fixes it all in one?

Can you please send me separate revert and re-add patches, that's
probably going to be easier to review.

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

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Michael Welling]

On Thu, May 21, 2015 at 10:16:38PM +0100, Mark Brown wrote:
> On Thu, May 21, 2015 at 04:04:11PM -0500, Michael Welling wrote:
> 
> > Do you want to revert the patch and apply a new one or should I provide a
> > patch that reverts the changes and fixes it all in one?
> 
> Can you please send me separate revert and re-add patches, that's
> probably going to be easier to review.

So after reverting this patch I found there is a issue in that it is difficult
to determine when a transfer is complete to properly drive the chipselect from
within the transfer_one function.

Then I figured that we could handle the case when GPIOs are being used with
some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one
function.

Near the beginning of the function I added:
	if (gpio_is_valid(spi->cs_gpio))
		omap2_mcspi_set_cs(spi, 0);

Near the end of the function I added:
	if (gpio_is_valid(spi->cs_gpio))
		omap2_mcspi_set_cs(spi, 1);

This makes GPIO chip select support work while leaving the native working
as previous.

Is this solution acceptible?

In the process of reviewing the changes I found a few other things that
should be changed as well.

Here you will see a delay that is already handled by the core spi driver:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166

In the set_cs function the inversion of the enable that occurs in the
core spi_set_cs function based on SPI_CS_HIGH needs to revert as the
controller handles the inversion with OMAP2_MCSPI_CHCONF_EPOL bit in the
CHCONF register.

If you approve of the fix for the GPIO support, I will provide a patch
series with all of these above mentioned fixes.

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Mark Brown]

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

On Thu, May 21, 2015 at 06:48:33PM -0500, Michael Welling wrote:

> So after reverting this patch I found there is a issue in that it is difficult
> to determine when a transfer is complete to properly drive the chipselect from
> within the transfer_one function.

Is unprepare_message() a suitable place here?  I've got a feeling the
answer is no...

> Then I figured that we could handle the case when GPIOs are being used with
> some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one
> function.
> 
> Near the beginning of the function I added:
> 	if (gpio_is_valid(spi->cs_gpio))
> 		omap2_mcspi_set_cs(spi, 0);
> 
> Near the end of the function I added:
> 	if (gpio_is_valid(spi->cs_gpio))
> 		omap2_mcspi_set_cs(spi, 1);
> 
> This makes GPIO chip select support work while leaving the native working
> as previous.
> 
> Is this solution acceptible?

I think that's probably OK as well, it's not ideal though (and risky if
the chip select is routed somewhere...).  

> In the process of reviewing the changes I found a few other things that
> should be changed as well.

Please send fixes for these as separate patches (ideally without any
dependency on your new work so we can send them to Linus as fixes).

> Here you will see a delay that is already handled by the core spi driver:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166

I can't actually see that since I have no internet access right now!

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

Re: [PATCH] spi: omap2-mcspi: Fix native cs with new set_cs

[Michael Welling]

On Fri, May 22, 2015 at 01:25:44PM +0100, Mark Brown wrote:
> On Thu, May 21, 2015 at 06:48:33PM -0500, Michael Welling wrote:
> 
> > So after reverting this patch I found there is a issue in that it is difficult
> > to determine when a transfer is complete to properly drive the chipselect from
> > within the transfer_one function.
> 
> Is unprepare_message() a suitable place here?  I've got a feeling the
> answer is no...
> 
> > Then I figured that we could handle the case when GPIOs are being used with
> > some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one
> > function.
> > 
> > Near the beginning of the function I added:
> > 	if (gpio_is_valid(spi->cs_gpio))
> > 		omap2_mcspi_set_cs(spi, 0);
> > 
> > Near the end of the function I added:
> > 	if (gpio_is_valid(spi->cs_gpio))
> > 		omap2_mcspi_set_cs(spi, 1);
> > 
> > This makes GPIO chip select support work while leaving the native working
> > as previous.
> > 
> > Is this solution acceptible?
> 
> I think that's probably OK as well, it's not ideal though (and risky if
> the chip select is routed somewhere...).  
> 
> > In the process of reviewing the changes I found a few other things that
> > should be changed as well.
> 
> Please send fixes for these as separate patches (ideally without any
> dependency on your new work so we can send them to Linus as fixes).
>

Well actually these fixes are needed as the results of the first three patches
pushed into next.

When switching from transfer_one_message to tranfer_one I did not realize that
the delay was handled in the core.

When adding the set_cs function I did not notice that the enable was
complemented by the core driver.

> > Here you will see a delay that is already handled by the core spi driver:
> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166
> 
> I can't actually see that since I have no internet access right now!

That's like a fish out of water.

[PATCH 0/4] spi: omap2-mcspi: Fixes for recent updates

[Michael Welling]

The recent update of the OMAP2 McSPI driver left some unresolved issues.
These patches should take care them and again allow for GPIO chip selects
and native chip selects.

Michael Welling (4):
  spi: omap2-mcspi: Remove unnecessary delay
  spi: omap2-mcspi: Fix set_cs function for active high
  spi: omap2-mcspi: Fix GPIO chip select support
  spi: omap2-mcspi: Handle error on gpio_request

 drivers/spi/spi-omap2-mcspi.c |   25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

-- 
1.7.9.5

[PATCH 1/4] spi: omap2-mcspi: Remove unnecessary delay

[Michael Welling]

The core spi driver handles the delay between transactions.
This is a remanant from the transfer_one conversion.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/spi/spi-omap2-mcspi.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index a7d85c5..304b427 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -1163,9 +1163,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 		}
 	}
 
-	if (t->delay_usecs)
-		udelay(t->delay_usecs);
-
 	omap2_mcspi_set_enable(spi, 0);
 
 	if (mcspi->fifo_depth > 0)
-- 
1.7.9.5

[PATCH 2/4] spi: omap2-mcspi: Fix set_cs function for active high

[Michael Welling]

The core spi driver swaps the polarity of the enable based on SPI_CS_HIGH.
The omap2 controller has an internal configuration register bit called
OMAP2_MCSPI_CHCONF_EPOL to handle active high chip selects as well.

So we have to revert swap the polarity back for the correct setting of the
OMAP2_MCSPI_CHCONF_FORCE bit in omap2_mcspi_set_cs.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/spi/spi-omap2-mcspi.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 304b427..502db29 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -247,6 +247,13 @@ static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
 {
 	u32 l;
 
+	/* The controller handles the inverted chip selects
+	 * using the OMAP2_MCSPI_CHCONF_EPOL bit so revert
+	 * the inversion from the core spi_set_cs function.
+	 */
+	if (spi->mode & SPI_CS_HIGH)
+		enable = !enable;
+
 	if (spi->controller_state) {
 		l = mcspi_cached_chconf0(spi);
 
-- 
1.7.9.5

[PATCH 3/4] spi: omap2-mcspi: Fix GPIO chip select support

[Michael Welling]

The OMAP2_MCSPI_CHCONF_FORCE must be toggled even when using GPIO
chip selects. This patch conditionally calls the omap2_mcspi_set_cs 
function to do so when using GPIO chip selects.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/spi/spi-omap2-mcspi.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 502db29..c4e21ad 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -1108,6 +1108,9 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 
 	omap2_mcspi_set_enable(spi, 0);
 
+	if (gpio_is_valid(spi->cs_gpio))
+		omap2_mcspi_set_cs(spi, spi->mode & SPI_CS_HIGH);
+
 	if (par_override ||
 	    (t->speed_hz != spi->max_speed_hz) ||
 	    (t->bits_per_word != spi->bits_per_word)) {
@@ -1192,6 +1195,9 @@ out:
 
 	omap2_mcspi_set_enable(spi, 0);
 
+	if (gpio_is_valid(spi->cs_gpio))
+		omap2_mcspi_set_cs(spi, !(spi->mode & SPI_CS_HIGH));
+
 	if (mcspi->fifo_depth > 0 && t)
 		omap2_mcspi_set_fifo(spi, t, 0);
 
-- 
1.7.9.5

[PATCH 4/4] spi: omap2-mcspi: Handle error on gpio_request

[Michael Welling]

If a valid GPIO is specified but cannot be requested by the driver, print a
message and error out of omap2_mcspi_setup.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/spi/spi-omap2-mcspi.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index c4e21ad..5867384 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -1023,9 +1023,12 @@ static int omap2_mcspi_setup(struct spi_device *spi)
 	}
 
 	if (gpio_is_valid(spi->cs_gpio)) {
-		if (gpio_request(spi->cs_gpio, dev_name(&spi->dev)) == 0)
-			gpio_direction_output(spi->cs_gpio,
-			!(spi->mode & SPI_CS_HIGH));
+		ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev));
+		if (ret) {
+			dev_err(&spi->dev, "failed to request gpio\n");
+			return ret;
+		}
+		gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
 	}
 
 	ret = pm_runtime_get_sync(mcspi->dev);
-- 
1.7.9.5

Re: [PATCH 4/4] spi: omap2-mcspi: Handle error on gpio_request

[Nicholas Mc Guire]

On Sat, 23 May 2015, Michael Welling wrote:

> If a valid GPIO is specified but cannot be requested by the driver, print a
> message and error out of omap2_mcspi_setup.
> 
> Signed-off-by: Michael Welling <mwelling@ieee.org>
> ---
>  drivers/spi/spi-omap2-mcspi.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> index c4e21ad..5867384 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -1023,9 +1023,12 @@ static int omap2_mcspi_setup(struct spi_device *spi)
>  	}
>  
>  	if (gpio_is_valid(spi->cs_gpio)) {
> -		if (gpio_request(spi->cs_gpio, dev_name(&spi->dev)) == 0)
> -			gpio_direction_output(spi->cs_gpio,
> -			!(spi->mode & SPI_CS_HIGH));
> +		ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev));
> +		if (ret) {
> +			dev_err(&spi->dev, "failed to request gpio\n");
> +			return ret;
> +		}
> +		gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
>  	}

just wondering if the outer gpio_is_valid is actually needed as it seems
gpio_request() is actually calling gpio_is_valid() anyway and would return
non 0 if it were not,

thx!
hofrat

Re: [PATCH 4/4] spi: omap2-mcspi: Handle error on gpio_request

[Michael Welling]

On Sun, May 24, 2015 at 10:13:07AM +0200, Nicholas Mc Guire wrote:
> On Sat, 23 May 2015, Michael Welling wrote:
> 
> > If a valid GPIO is specified but cannot be requested by the driver, print a
> > message and error out of omap2_mcspi_setup.
> > 
> > Signed-off-by: Michael Welling <mwelling@ieee.org>
> > ---
> >  drivers/spi/spi-omap2-mcspi.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> > index c4e21ad..5867384 100644
> > --- a/drivers/spi/spi-omap2-mcspi.c
> > +++ b/drivers/spi/spi-omap2-mcspi.c
> > @@ -1023,9 +1023,12 @@ static int omap2_mcspi_setup(struct spi_device *spi)
> >  	}
> >  
> >  	if (gpio_is_valid(spi->cs_gpio)) {
> > -		if (gpio_request(spi->cs_gpio, dev_name(&spi->dev)) == 0)
> > -			gpio_direction_output(spi->cs_gpio,
> > -			!(spi->mode & SPI_CS_HIGH));
> > +		ret = gpio_request(spi->cs_gpio, dev_name(&spi->dev));
> > +		if (ret) {
> > +			dev_err(&spi->dev, "failed to request gpio\n");
> > +			return ret;
> > +		}
> > +		gpio_direction_output(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
> >  	}
> 
> just wondering if the outer gpio_is_valid is actually needed as it seems
> gpio_request() is actually calling gpio_is_valid() anyway and would return
> non 0 if it were not,

In this case we have to check first because if the GPIO is not registered the 
native chip select is assumed. If the GPIO is registered, is valid and can be
requested we use it as the chip select. If the GPIO is registered and valid but
cannot be requested we return an error.

> 
> thx!
> hofrat

Re: [PATCH 0/4] spi: omap2-mcspi: Fixes for recent updates

[Mark Brown]

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

On Sat, May 23, 2015 at 09:13:41PM -0500, Michael Welling wrote:
> The recent update of the OMAP2 McSPI driver left some unresolved issues.
> These patches should take care them and again allow for GPIO chip selects
> and native chip selects.

Applied all, thanks.

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