[PATCH 1/2] SPI: control CS via standard GPIO operations instead of SPI-HW

[PATCH 1/2] SPI: control CS via standard GPIO operations instead of SPI-HW

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Allow the cs-gpios property in DT to be used instead of the
fixed two chip-selects provided by the SPI-HW itself

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

---

There is the question if we still need to support the chip_selects
provided by the hardware (plus the buggy CSPOL_HIGH support for those cases)
or if we could just make the cs-gpios a required setting for this driver.
Going with the GPIO only solution would clean up the code a bit.

 drivers/spi/spi-bcm2835.c |   53 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 419a782..128a152 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2012 Chris Boot
  * Copyright (C) 2013 Stephen Warren
+ * Copyright (C) 2015 Martin Sperl
  *
  * This driver is inspired by:
  * spi-ath79.c, Copyright (C) 2009-2011 Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
@@ -30,6 +31,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/spi/spi.h>
 
 /* SPI register offsets */
@@ -116,6 +118,18 @@ static inline void bcm2835_wr_fifo(struct bcm2835_spi *bs, int len)
 	}
 }
 
+/* ideally spi_set_cs would be exported by spi-core */
+static inline void bcm2835_set_cs(struct spi_device *spi, bool enable)
+{
+
+	if (spi->mode & SPI_CS_HIGH)
+		enable = !enable;
+
+	if (spi->cs_gpio >= 0)
+		gpio_set_value(spi->cs_gpio, !enable);
+
+}
+
 static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
 {
 	struct spi_master *master = dev_id;
@@ -205,12 +219,24 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
 		cs |= BCM2835_SPI_CS_CPHA;
 
 	if (!(spi->mode & SPI_NO_CS)) {
-		if (spi->mode & SPI_CS_HIGH) {
-			cs |= BCM2835_SPI_CS_CSPOL;
-			cs |= BCM2835_SPI_CS_CSPOL0 << spi->chip_select;
-		}
+		if (spi->cs_gpio >= 0)
+			bcm2835_set_cs(spi, 1);
+		else {
+			/* do we need to support this ?
+			 * note that there is a bug in this when there are
+			 * multiple devices on the bus with at least one
+			 * having SPI_CS_HIGH set (the other CS_CSPOLX get
+			 * reset to 0 when any other device
+			 * starts a transfer
+			 */
+			if (spi->mode & SPI_CS_HIGH) {
+				cs |= BCM2835_SPI_CS_CSPOL;
+				cs |= BCM2835_SPI_CS_CSPOL0
+					<< spi->chip_select;
+			}
 
-		cs |= spi->chip_select;
+			cs |= spi->chip_select;
+		}
 	}
 
 	reinit_completion(&bs->done);
@@ -245,9 +271,12 @@ static int bcm2835_spi_finish_transfer(struct spi_device *spi,
 	if (tfr->delay_usecs)
 		udelay(tfr->delay_usecs);
 
-	if (cs_change)
+	if (cs_change) {
+		/* reset CS */
+		bcm2835_set_cs(spi, 0);
 		/* Clear TA flag */
 		bcm2835_wr(bs, BCM2835_SPI_CS, cs & ~BCM2835_SPI_CS_TA);
+	}
 
 	return 0;
 }
@@ -285,15 +314,24 @@ static int bcm2835_spi_transfer_one(struct spi_master *master,
 	}
 
 out:
-	/* Clear FIFOs, and disable the HW block */
+	/* Clear FIFOs, reset CS and disable the HW block */
 	bcm2835_wr(bs, BCM2835_SPI_CS,
 		   BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
+	bcm2835_set_cs(spi, 0);
 	mesg->status = err;
 	spi_finalize_current_message(master);
 
 	return 0;
 }
 
+static int bcm2835_spi_setup(struct spi_device *spi)
+{
+	/* setting up CS right from the start */
+	bcm2835_set_cs(spi, 0);
+
+	return 0;
+}
+
 static int bcm2835_spi_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;
@@ -313,6 +351,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
 	master->num_chipselect = 3;
 	master->transfer_one_message = bcm2835_spi_transfer_one;
+	master->setup = bcm2835_spi_setup;
 	master->dev.of_node = pdev->dev.of_node;
 
 	bs = spi_master_get_devdata(master);
-- 
1.7.10.4

[PATCH 2/2] dt/bindings: control CS via standard GPIO operations instead of SPI-HW

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Change the device tree to use cs-gpios for the spi bus master
and standard gpio operation instead of relying on the HW with
just 2 chip_selects using ALT0.

This reassigns the existing CS pins 7(=CS1) and 8(=CS0)
as output instead of ALT0 (=SPI HW block controlled) 
and adds them in the list of cs-gpios for the spi-bus.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

---
 arch/arm/boot/dts/bcm2835-rpi.dtsi |    4 ++--
 arch/arm/boot/dts/bcm2835.dtsi     |    1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
index c706448..f0e36ab 100644
--- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
+++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
@@ -20,12 +20,12 @@
 	pinctrl-names = "default";
 
 	gpioout: gpioout {
-		brcm,pins = <6>;
+		brcm,pins = <6 7 8>;
 		brcm,function = <1>; /* GPIO out */
 	};
 
 	alt0: alt0 {
-		brcm,pins = <0 1 2 3 4 5 7 8 9 10 11 14 15 40 45>;
+		brcm,pins = <0 1 2 3 4 5 9 10 11 14 15 40 45>;
 		brcm,function = <4>; /* alt0 */
 	};
 
diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi
index 3342cb1..74d08e1 100644
--- a/arch/arm/boot/dts/bcm2835.dtsi
+++ b/arch/arm/boot/dts/bcm2835.dtsi
@@ -110,6 +110,7 @@
 			#address-cells = <1>;
 			#size-cells = <0>;
 			status = "disabled";
+			cs-gpios = <&gpio 8 0>, <&gpio 7 0>;
 		};
 
 		i2c0: i2c@20205000 {
-- 
1.7.10.4

Re: [PATCH 2/2] dt/bindings: control CS via standard GPIO operations instead of SPI-HW

[Stephen Warren]

On 03/04/2015 09:40 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Change the device tree to use cs-gpios for the spi bus master
> and standard gpio operation instead of relying on the HW with
> just 2 chip_selects using ALT0.
> 
> This reassigns the existing CS pins 7(=CS1) and 8(=CS0)
> as output instead of ALT0 (=SPI HW block controlled) 
> and adds them in the list of cs-gpios for the spi-bus.

These pins aren't used by anything on the board, but are rather part of
the expansion header. I wonder if we wouldn't be better off removing any
configuration of the pins from the DT. After all, we can't guarantee how
the user has connected them. The "default" usage, a/k/a the expansion
header signal naming, isn't any guarantee.

Rather, the user should specify what they want to use the pin as; as a
GPIO input, GPIO output, or an SPI chip-select.

> diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi

>  	gpioout: gpioout {
> -		brcm,pins = <6>;
> +		brcm,pins = <6 7 8>;
>  		brcm,function = <1>; /* GPIO out */
>  	};
>  
>  	alt0: alt0 {
> -		brcm,pins = <0 1 2 3 4 5 7 8 9 10 11 14 15 40 45>;
> +		brcm,pins = <0 1 2 3 4 5 9 10 11 14 15 40 45>;
>  		brcm,function = <4>; /* alt0 */
>  	};

While the existing DT already has this issue, note that this forces
these pins to be driven as outputs. What if the user has hooked up an
external device that drives these signals, and wants to use the pins as
GPIO inputs?

> diff --git a/arch/arm/boot/dts/bcm2835.dtsi b/arch/arm/boot/dts/bcm2835.dtsi

>  			#address-cells = <1>;
>  			#size-cells = <0>;
>  			status = "disabled";
> +			cs-gpios = <&gpio 8 0>, <&gpio 7 0>;
>  		};

This shouldn't be in the SoC .dtsi file. It's quite possible for someone
to use other GPIOs as SPI CS. It's board or even use-case specific
whether those are the correct values.

I would argue that we should not put any cs-gpios into any in-kernel DT
file, since there's no on-board usage of SPI on the RPi boards.

For SPI to be useful, the user has to add a DT node to represent the SPI
device itself anyway, so adding some properties to the controller to
define which GPIOs to use for SPI CS can be done then too.
--
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/2] dt/bindings: control CS via standard GPIO operations instead of SPI-HW

[Martin Sperl]

> On 07.03.2015, at 06:47, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
> These pins aren't used by anything on the board, but are rather part of
> the expansion header. I wonder if we wouldn't be better off removing any
> configuration of the pins from the DT. After all, we can't guarantee how
> the user has connected them. The "default" usage, a/k/a the expansion
> header signal naming, isn't any guarantee.
> 
> Rather, the user should specify what they want to use the pin as; as a
> GPIO input, GPIO output, or an SPI chip-select.
...
> While the existing DT already has this issue, note that this forces
> these pins to be driven as outputs. What if the user has hooked up an
> external device that drives these signals, and wants to use the pins as
> GPIO inputs?


Actually if you look at the Documentation on page 102 you will find that
those pins are pulled high by default (if it is HW or firmware I am not
sure) - so a hat designer needs to take this into consideration anyway.
The only difference is that it is pulled high and not driven high/low.

Also with the "new" models there is the Firmware that will read those 
"hat-descriptors" from the eeprom and configure the GPIO "modes" and
pullups based on this information.

But then it means in principle that this is a more general issue
that just became apparent now.


> This shouldn't be in the SoC .dtsi file. It's quite possible for someone
> to use other GPIOs as SPI CS. It's board or even use-case specific
> whether those are the correct values.
> 
> I would argue that we should not put any cs-gpios into any in-kernel DT
> file, since there's no on-board usage of SPI on the RPi boards.

but then: why not just make it optional and NOT configuring it at all
and keep it "outside".

> For SPI to be useful, the user has to add a DT node to represent the SPI
> device itself anyway, so adding some properties to the controller to
> define which GPIOs to use for SPI CS can be done then too.

>From what I have seen (and I am not an expert) is that with the 
foundation device trees each "device" (spi/i2c/...) has a separate
Gpio section, which gets referenced inside the spi/i2c/... block.

As far as I understood with this setup the GPIOs ALT only gets set
up when the driver itself loads (probably while parsing the DT
for the device)

So this is maybe the way forward for the whole default-dt?

For SPI it would look like this:
&gpio {
        spi0_pins: spi0_pins {
                brcm,pins = <7 8 9 10 11>;
                brcm,function = <4>; /* alt0 */
        };
	...
}

&spi0 {
	...
        pinctrl-0 = <&spi0_pins>;
	...
}

And if you keep spi0 disabled in the dtsi files then the ALT
modes should not be set.

Obviously we could also split the gpio-block into 
"normal SPI" and "CS" pins, which would allow changing the
"defaults" also in the dts that gets build.

So how should we proceed?

Martin--
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/2] dt/bindings: control CS via standard GPIO operations instead of SPI-HW

[Stephen Warren]

On 03/11/2015 09:21 AM, Martin Sperl wrote:
>> On 07.03.2015, at 06:47, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>
>> These pins aren't used by anything on the board, but are rather part of
>> the expansion header. I wonder if we wouldn't be better off removing any
>> configuration of the pins from the DT. After all, we can't guarantee how
>> the user has connected them. The "default" usage, a/k/a the expansion
>> header signal naming, isn't any guarantee.
>>
>> Rather, the user should specify what they want to use the pin as; as a
>> GPIO input, GPIO output, or an SPI chip-select.
> ...
>> While the existing DT already has this issue, note that this forces
>> these pins to be driven as outputs. What if the user has hooked up an
>> external device that drives these signals, and wants to use the pins as
>> GPIO inputs?
> 
> Actually if you look at the Documentation on page 102 you will find that
> those pins are pulled high by default (if it is HW or firmware I am not
> sure) - so a hat designer needs to take this into consideration anyway.
> The only difference is that it is pulled high and not driven high/low.

Assuming that table shows HW defaults as opposed to the pin's
capabilities for pull-up-vs-down, then yes. Still, there is typically
quite a difference in strength between a pull-up and a drive-up.

> Also with the "new" models there is the Firmware that will read those 
> "hat-descriptors" from the eeprom and configure the GPIO "modes" and
> pullups based on this information.

Yes, my thinking is we should probably rely on that, or an explicit user
modification of the DT, to configure the pinmux, rather than making
assumptions.

> But then it means in principle that this is a more general issue
> that just became apparent now.

Yes.

>> This shouldn't be in the SoC .dtsi file. It's quite possible for someone
>> to use other GPIOs as SPI CS. It's board or even use-case specific
>> whether those are the correct values.
>>
>> I would argue that we should not put any cs-gpios into any in-kernel DT
>> file, since there's no on-board usage of SPI on the RPi boards.
> 
> but then: why not just make it optional and NOT configuring it at all
> and keep it "outside".

Sorry, I don't quite understand that comment; outside what?

>> For SPI to be useful, the user has to add a DT node to represent the SPI
>> device itself anyway, so adding some properties to the controller to
>> define which GPIOs to use for SPI CS can be done then too.
> 
> From what I have seen (and I am not an expert) is that with the 
> foundation device trees each "device" (spi/i2c/...) has a separate
> Gpio section, which gets referenced inside the spi/i2c/... block.
> 
> As far as I understood with this setup the GPIOs ALT only gets set
> up when the driver itself loads (probably while parsing the DT
> for the device)

Yes, that is certainly possible.

> So this is maybe the way forward for the whole default-dt?
> 
> For SPI it would look like this:
> &gpio {
>         spi0_pins: spi0_pins {
>                 brcm,pins = <7 8 9 10 11>;
>                 brcm,function = <4>; /* alt0 */
>         };
> 	...
> }
> 
> &spi0 {
> 	...
>         pinctrl-0 = <&spi0_pins>;
> 	...
> }
> 
> And if you keep spi0 disabled in the dtsi files then the ALT
> modes should not be set.

Yes, so long as it's disabled by default that would be OK. However, I
wonder why we don't just rely on the firmware to set up the pinmux,
since as you mentioned it does it now?

> Obviously we could also split the gpio-block into 
> "normal SPI" and "CS" pins, which would allow changing the
> "defaults" also in the dts that gets build.
> 
> So how should we proceed?

If we do put any default CS GPIO setup in the kernel DT, we should
indeed put it into a separate node (pinctrl state) so that the user can
override it easily without any interactions with any other pins/...
--
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/2] dt/bindings: control CS via standard GPIO operations instead of SPI-HW

[Martin Sperl]

> On 17.03.2015, at 04:18, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> But then it means in principle that this is a more general issue
>> that just became apparent now.
> 
> Yes.
> 
...
>> So this is maybe the way forward for the whole default-dt?
>> 
>> For SPI it would look like this:
>> &gpio {
>>        spi0_pins: spi0_pins {
>>                brcm,pins = <7 8 9 10 11>;
>>                brcm,function = <4>; /* alt0 */
>>        };
>> 	...
>> }
>> 
>> &spi0 {
>> 	...
>>        pinctrl-0 = <&spi0_pins>;
>> 	...
>> }
>> 
>> And if you keep spi0 disabled in the dtsi files then the ALT
>> modes should not be set.
> 
> Yes, so long as it's disabled by default that would be OK. However, I
> wonder why we don't just rely on the firmware to set up the pinmux,
> since as you mentioned it does it now?
...
>> Obviously we could also split the gpio-block into 
>> "normal SPI" and "CS" pins, which would allow changing the
>> "defaults" also in the dts that gets build.
>> 
>> So how should we proceed?
> 
> If we do put any default CS GPIO setup in the kernel DT, we should
> indeed put it into a separate node (pinctrl state) so that the user can
> override it easily without any interactions with any other pins/...
So I will create a patch to separate the spi portions out as 
mentioned above.

I will actually create 2 gpio-sections:
one for the SPI transfer pins (GPIO 9, 10, 11) 
and one for the chipselects (8, 7), which can get overridden
either as output or as ALT0 in a customized device tree.

SPI by default shall be disabled.

Martin
--
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/2] dt/bindings: control CS via standard GPIO operations instead of SPI-HW

[Stefan Wahren]

Hi Martin,

Am 04.03.2015 um 17:40 schrieb kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>
> Change the device tree to use cs-gpios for the spi bus master
> and standard gpio operation instead of relying on the HW with
> just 2 chip_selects using ALT0.
>
> This reassigns the existing CS pins 7(=CS1) and 8(=CS0)
> as output instead of ALT0 (=SPI HW block controlled) 
> and adds them in the list of cs-gpios for the spi-bus.
>
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>
> ---
>  arch/arm/boot/dts/bcm2835-rpi.dtsi |    4 ++--
>  arch/arm/boot/dts/bcm2835.dtsi     |    1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm/boot/dts/bcm2835-rpi.dtsi b/arch/arm/boot/dts/bcm2835-rpi.dtsi
> index c706448..f0e36ab 100644
> --- a/arch/arm/boot/dts/bcm2835-rpi.dtsi
> +++ b/arch/arm/boot/dts/bcm2835-rpi.dtsi
> @@ -20,12 +20,12 @@
>  	pinctrl-names = "default";
>  
>  	gpioout: gpioout {
> -		brcm,pins = <6>;
> +		brcm,pins = <6 7 8>;
>  		brcm,function = <1>; /* GPIO out */
>  	};
>  
>  	alt0: alt0 {
> -		brcm,pins = <0 1 2 3 4 5 7 8 9 10 11 14 15 40 45>;
> +		brcm,pins = <0 1 2 3 4 5 9 10 11 14 15 40 45>;
>  		brcm,function = <4>; /* alt0 */
>  	};

just a note for your next version. I think this patch won't apply
against for-rpi-next.

It misses that one:

https://git.kernel.org/cgit/linux/kernel/git/rpi/linux-rpi.git/commit/?h=for-rpi-next&id=72a6dbe0ea91e2a9c4357a3b0afbbd7b1f00dc8a

Regards
Stefan

--
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 1/2] SPI: control CS via standard GPIO operations instead of SPI-HW

[Marc Kleine-Budde]

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

On 03/04/2015 05:40 PM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Allow the cs-gpios property in DT to be used instead of the
> fixed two chip-selects provided by the SPI-HW itself
> 
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> ---
> 
> There is the question if we still need to support the chip_selects
> provided by the hardware (plus the buggy CSPOL_HIGH support for those cases)
> or if we could just make the cs-gpios a required setting for this driver.
> Going with the GPIO only solution would clean up the code a bit.
> 
>  drivers/spi/spi-bcm2835.c |   53 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 46 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
> index 419a782..128a152 100644
> --- a/drivers/spi/spi-bcm2835.c
> +++ b/drivers/spi/spi-bcm2835.c
> @@ -3,6 +3,7 @@
>   *
>   * Copyright (C) 2012 Chris Boot
>   * Copyright (C) 2013 Stephen Warren
> + * Copyright (C) 2015 Martin Sperl
>   *
>   * This driver is inspired by:
>   * spi-ath79.c, Copyright (C) 2009-2011 Gabor Juhos <juhosg-p3rKhJxN3nrF2uMehF1BdA@public.gmane.orgg>
> @@ -30,6 +31,7 @@
>  #include <linux/of.h>
>  #include <linux/of_irq.h>
>  #include <linux/of_device.h>
> +#include <linux/of_gpio.h>
>  #include <linux/spi/spi.h>
>  
>  /* SPI register offsets */
> @@ -116,6 +118,18 @@ static inline void bcm2835_wr_fifo(struct bcm2835_spi *bs, int len)
>  	}
>  }
>  
> +/* ideally spi_set_cs would be exported by spi-core */
> +static inline void bcm2835_set_cs(struct spi_device *spi, bool enable)
> +{
> +
> +	if (spi->mode & SPI_CS_HIGH)
> +		enable = !enable;
> +
> +	if (spi->cs_gpio >= 0)

You might want to use gpio_is_valid() instead of open coding it.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] SPI: control CS via standard GPIO operations instead of SPI-HW

[Martin Sperl]

> 
> On 04.03.2015, at 18:01, Marc Kleine-Budde <mkl-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> 
> You might want to use gpio_is_valid() instead of open coding it.
Under ideal circumstances spi_set_cs() would be GPL exported and then
I would not have to duplicate the code.

And there it is implemented like this, so my code includes it.

I just have kept the patch simple - in the long-run we should move that
route I assume and fix the code there to use gpio_is_valid().

Martin




--
To unsubscribe from this list: send the line "unsubscribe devicetree" 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 1/2] SPI: control CS via standard GPIO operations instead of SPI-HW

[Stephen Warren]

On 03/04/2015 09:40 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Allow the cs-gpios property in DT to be used instead of the
> fixed two chip-selects provided by the SPI-HW itself
> 
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> ---
> 
> There is the question if we still need to support the chip_selects
> provided by the hardware (plus the buggy CSPOL_HIGH support for those cases)
> or if we could just make the cs-gpios a required setting for this driver.
> Going with the GPIO only solution would clean up the code a bit.

Yes, I believe so. Any existing DT file needs to continue working with a
new kernel; DT is an ABI.

BTW, you forgot to Cc the SPI maintainer, so he probably won't see your
patch and apply it.

I assume that the SPI core parses the cs-gpios property from DT, hence
why this patch doesn't add any DT parsing, and doesn't modify any DT
bindings?

> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c

> @@ -205,12 +219,24 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
>  		cs |= BCM2835_SPI_CS_CPHA;
>  
>  	if (!(spi->mode & SPI_NO_CS)) {
> -		if (spi->mode & SPI_CS_HIGH) {
> -			cs |= BCM2835_SPI_CS_CSPOL;
> -			cs |= BCM2835_SPI_CS_CSPOL0 << spi->chip_select;
> -		}
> +		if (spi->cs_gpio >= 0)
> +			bcm2835_set_cs(spi, 1);
> +		else {
> +			/* do we need to support this ?

As mentioned above, yes I believe so. You'd need to amend this comment
to remove the question, I suspect.

This patch looks OK with that change made.

> +			 * note that there is a bug in this when there are
> +			 * multiple devices on the bus with at least one
> +			 * having SPI_CS_HIGH set (the other CS_CSPOLX get
> +			 * reset to 0 when any other device
> +			 * starts a transfer
> +			 */
> +			if (spi->mode & SPI_CS_HIGH) {
> +				cs |= BCM2835_SPI_CS_CSPOL;
> +				cs |= BCM2835_SPI_CS_CSPOL0
> +					<< spi->chip_select;
> +			}
>  
> -		cs |= spi->chip_select;
> +			cs |= spi->chip_select;
> +		}
>  	}

--
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 1/2] SPI: control CS via standard GPIO operations instead of SPI-HW

[Mark Brown]

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

On Fri, Mar 06, 2015 at 10:38:00PM -0700, Stephen Warren wrote:

> BTW, you forgot to Cc the SPI maintainer, so he probably won't see your
> patch and apply it.

Correct, if something doesn't end up in my inbox I'm not going to look
at it - see SubmittingPatches for the process.

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