[PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Michael Welling]

This patch allows for GPIOs specified in the devicetree to be used as SPI
chipselects on TI OMAP2 SoCs.

Tested on the AM3354.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---

v2: Considers the possible use of SPI_CS_HIGH during chip select activation.

 drivers/spi/spi-omap2-mcspi.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index d1a5b9f..5e388a8 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -35,6 +35,7 @@
 #include <linux/gcd.h>
 
 #include <linux/spi/spi.h>
+#include <linux/gpio.h>
 
 #include <linux/platform_data/spi-omap2-mcspi.h>
 
@@ -246,6 +247,12 @@ static void omap2_mcspi_force_cs(struct spi_device *spi, int cs_active)
 {
 	u32 l;
 
+	if (gpio_is_valid(spi->cs_gpio)) {
+		gpio_set_value(spi->cs_gpio, (cs_active) ?
+			!!(spi->mode & SPI_CS_HIGH) :
+			!(spi->mode & SPI_CS_HIGH));
+	}
+
 	l = mcspi_cached_chconf0(spi);
 	if (cs_active)
 		l |= OMAP2_MCSPI_CHCONF_FORCE;
@@ -1015,6 +1022,12 @@ static int omap2_mcspi_setup(struct spi_device *spi)
 	if (ret < 0)
 		return ret;
 
+	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 = omap2_mcspi_setup_transfer(spi, NULL);
 	pm_runtime_mark_last_busy(mcspi->dev);
 	pm_runtime_put_autosuspend(mcspi->dev);
-- 
1.7.9.5

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Mark Brown]

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

On Sun, Apr 26, 2015 at 10:44:30PM -0500, Michael Welling wrote:

> +	if (gpio_is_valid(spi->cs_gpio)) {
> +		gpio_set_value(spi->cs_gpio, (cs_active) ?
> +			!!(spi->mode & SPI_CS_HIGH) :
> +			!(spi->mode & SPI_CS_HIGH));
> +	}

Two problems here.  One is that the above logic statement is just not
readable (the repitition of hecks, the ternery operator, the
indentation...) and the other is that the core chipselect support
already handles GPIO chipselects so you should really be converting the
driver to use that.  At the very least the code needs to be legible
though.

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

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Michael Welling]

On Mon, Apr 27, 2015 at 08:55:50PM +0100, Mark Brown wrote:
> On Sun, Apr 26, 2015 at 10:44:30PM -0500, Michael Welling wrote:
> 
> > +	if (gpio_is_valid(spi->cs_gpio)) {
> > +		gpio_set_value(spi->cs_gpio, (cs_active) ?
> > +			!!(spi->mode & SPI_CS_HIGH) :
> > +			!(spi->mode & SPI_CS_HIGH));
> > +	}
> 
> Two problems here.  One is that the above logic statement is just not
> readable (the repitition of hecks, the ternery operator, the
> indentation...) and the other is that the core chipselect support
> already handles GPIO chipselects so you should really be converting the
> driver to use that.  At the very least the code needs to be legible
> though.

Before I send another patch how does this look?

	if (gpio_is_valid(spi->cs_gpio)) {
		if (cs_active)
			gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH);
		else
			gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
	}

If I were to attempt to convert the driver to use the core chipselect support,
how would I go about doing it?

Is there another driver that I can use for reference?

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Martin Sperl]

> On 28.04.2015, at 03:21, Michael Welling <mwelling@ieee.org> wrote:
> If I were to attempt to convert the driver to use the core chipselect support,
> how would I go about doing it?
> 
> Is there another driver that I can use for reference?
You may look into this patch: e34ff011c70e5f4ef219141711142d5111ae6ebb
for the spi-bcm2835 driver, which did the conversion to the new transfer_one
interface (and framework based GPIO chipselects).

For most parts all you have to do is take the contents of the loop over all the
spi_transfers inside your master->transfer_one_message method and create a new
method with it and assign it to master->transfer_one.

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Michael Welling]

On Tue, Apr 28, 2015 at 07:32:21AM +0200, Martin Sperl wrote:
> 
> > On 28.04.2015, at 03:21, Michael Welling <mwelling@ieee.org> wrote:
> > If I were to attempt to convert the driver to use the core chipselect support,
> > how would I go about doing it?
> > 
> > Is there another driver that I can use for reference?
> You may look into this patch: e34ff011c70e5f4ef219141711142d5111ae6ebb
> for the spi-bcm2835 driver, which did the conversion to the new transfer_one
> interface (and framework based GPIO chipselects).

I will take a look.

Thanks.

> 
> For most parts all you have to do is take the contents of the loop over all the
> spi_transfers inside your master->transfer_one_message method and create a new
> method with it and assign it to master->transfer_one.
> 

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Ezequiel Garcia]

On 04/27/2015 10:21 PM, Michael Welling wrote:
> On Mon, Apr 27, 2015 at 08:55:50PM +0100, Mark Brown wrote:
>> On Sun, Apr 26, 2015 at 10:44:30PM -0500, Michael Welling wrote:
>>
>>> +	if (gpio_is_valid(spi->cs_gpio)) {
>>> +		gpio_set_value(spi->cs_gpio, (cs_active) ?
>>> +			!!(spi->mode & SPI_CS_HIGH) :
>>> +			!(spi->mode & SPI_CS_HIGH));
>>> +	}
>>
>> Two problems here.  One is that the above logic statement is just not
>> readable (the repitition of hecks, the ternery operator, the
>> indentation...) and the other is that the core chipselect support
>> already handles GPIO chipselects so you should really be converting the
>> driver to use that.  At the very least the code needs to be legible
>> though.
> 
> Before I send another patch how does this look?
> 
> 	if (gpio_is_valid(spi->cs_gpio)) {
> 		if (cs_active)
> 			gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH);
> 		else
> 			gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
> 	}
> 
> If I were to attempt to convert the driver to use the core chipselect support,
> how would I go about doing it?
> 
> Is there another driver that I can use for reference?

We've recently done that for spi-img-spfi.c.

-- 
Ezequiel

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Michael Welling]

On Tue, Apr 28, 2015 at 11:04:09AM -0300, Ezequiel Garcia wrote:
> On 04/27/2015 10:21 PM, Michael Welling wrote:
> > On Mon, Apr 27, 2015 at 08:55:50PM +0100, Mark Brown wrote:
> >> On Sun, Apr 26, 2015 at 10:44:30PM -0500, Michael Welling wrote:
> >>
> >>> +	if (gpio_is_valid(spi->cs_gpio)) {
> >>> +		gpio_set_value(spi->cs_gpio, (cs_active) ?
> >>> +			!!(spi->mode & SPI_CS_HIGH) :
> >>> +			!(spi->mode & SPI_CS_HIGH));
> >>> +	}
> >>
> >> Two problems here.  One is that the above logic statement is just not
> >> readable (the repitition of hecks, the ternery operator, the
> >> indentation...) and the other is that the core chipselect support
> >> already handles GPIO chipselects so you should really be converting the
> >> driver to use that.  At the very least the code needs to be legible
> >> though.
> > 
> > Before I send another patch how does this look?
> > 
> > 	if (gpio_is_valid(spi->cs_gpio)) {
> > 		if (cs_active)
> > 			gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH);
> > 		else
> > 			gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
> > 	}
> > 
> > If I were to attempt to convert the driver to use the core chipselect support,
> > how would I go about doing it?
> > 
> > Is there another driver that I can use for reference?
> 
> We've recently done that for spi-img-spfi.c.
>

Okay, thanks for pointing that out.
 
> -- 
> Ezequiel

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Mark Brown]

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

On Mon, Apr 27, 2015 at 08:21:50PM -0500, Michael Welling wrote:

> Before I send another patch how does this look?

> 	if (gpio_is_valid(spi->cs_gpio)) {
> 		if (cs_active)
> 			gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH);
> 		else
> 			gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
> 	}

That's good but please have braces around the inner if for clarity.

> If I were to attempt to convert the driver to use the core chipselect support,
> how would I go about doing it?

> Is there another driver that I can use for reference?

Martin and Ezequiel already gave you some good examples - in general
anything providing the set_cs() operation should be reasonable to look
at.

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

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Michael Welling]

On Tue, Apr 28, 2015 at 03:34:25PM +0100, Mark Brown wrote:
> On Mon, Apr 27, 2015 at 08:21:50PM -0500, Michael Welling wrote:
> 
> > Before I send another patch how does this look?
> 
> > 	if (gpio_is_valid(spi->cs_gpio)) {
> > 		if (cs_active)
> > 			gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH);
> > 		else
> > 			gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
> > 	}
> 
> That's good but please have braces around the inner if for clarity.

I try to follow the CodingStyle documentation and it says differently:
http://lxr.free-electrons.com/source/Documentation/CodingStyle#L156

> 
> > If I were to attempt to convert the driver to use the core chipselect support,
> > how would I go about doing it?
> 
> > Is there another driver that I can use for reference?
> 
> Martin and Ezequiel already gave you some good examples - in general
> anything providing the set_cs() operation should be reasonable to look
> at.

I was trying to minimise the impact of the change but will look into using
this method.

I have also been looking at ways of allowing more GPIO chipselects than
DMA channels. Currently this driver is limited to one chipselect per
DMA channel.

Do you have any ideas on how to accomplish this?

The number of chipselects on each channel is very restrictive on the mcspi.
By using multiple GPIO chipselects per DMA channel it opens many more
applications. 

Re: [PATCH v2] spi: omap2-mcspi: Add support for GPIO chipselects

[Mark Brown]

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

On Tue, Apr 28, 2015 at 06:06:43PM -0500, Michael Welling wrote:
> On Tue, Apr 28, 2015 at 03:34:25PM +0100, Mark Brown wrote:

> > > 	if (gpio_is_valid(spi->cs_gpio)) {
> > > 		if (cs_active)
> > > 			gpio_set_value(spi->cs_gpio, spi->mode & SPI_CS_HIGH);
> > > 		else
> > > 			gpio_set_value(spi->cs_gpio, !(spi->mode & SPI_CS_HIGH));
> > > 	}

> > That's good but please have braces around the inner if for clarity.

> I try to follow the CodingStyle documentation and it says differently:
> http://lxr.free-electrons.com/source/Documentation/CodingStyle#L156

That's for free standing if statements, the reason I'm asking for braces
at both levels here is that you've got a nested if - it's similar to
having braces on both branches of an if, being consistent within the
same statement.

> I have also been looking at ways of allowing more GPIO chipselects than
> DMA channels. Currently this driver is limited to one chipselect per
> DMA channel.

> Do you have any ideas on how to accomplish this?

Just use a single channel for the controller rather than allocating them
per device?  It's not like the controller is going to be able to
interact with multiple devics simultaneously.

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