[PATCH] spi: rb4xx: Fix set_cs logic.

[PATCH] spi: rb4xx: Fix set_cs logic.

[Bert Vermeulen]

As it turns out, the set_cs() enable parameter refers to the logic level
on the CS pin, not the state of chip selection.

This broke functionality of the LEDs behind the CPLD, or at least delayed
the commands until another one came in to toggle CS.

Signed-off-by: Bert Vermeulen <bert@biot.com>
---
 drivers/spi/spi-rb4xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spi-rb4xx.c b/drivers/spi/spi-rb4xx.c
index 9b449d4..50f49f3 100644
--- a/drivers/spi/spi-rb4xx.c
+++ b/drivers/spi/spi-rb4xx.c
@@ -90,7 +90,7 @@ static void rb4xx_set_cs(struct spi_device *spi, bool enable)
 	 * since it's all on the same hardware register. However the
 	 * CPLD needs CS deselected after every command.
 	 */
-	if (!enable)
+	if (enable)
 		rb4xx_write(rbspi, AR71XX_SPI_REG_IOC,
 			    AR71XX_SPI_IOC_CS0 | AR71XX_SPI_IOC_CS1);
 }
-- 
1.9.1

Re: [PATCH] spi: rb4xx: Fix set_cs logic.

[Mark Brown]

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

On Mon, Apr 20, 2015 at 03:53:25PM +0200, Bert Vermeulen wrote:
> As it turns out, the set_cs() enable parameter refers to the logic level
> on the CS pin, not the state of chip selection.

> This broke functionality of the LEDs behind the CPLD, or at least delayed
> the commands until another one came in to toggle CS.

No, the enable parameter *should* refer to chip select assertion (see
how we handle GPIO chip selects).  However it's possible that this
device has an inverted chip select and should be registered with the
SPI_CS_HIGH flag?

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

Re: [PATCH] spi: rb4xx: Fix set_cs logic.

[Geert Uytterhoeven]

On Mon, Apr 20, 2015 at 10:37 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Apr 20, 2015 at 03:53:25PM +0200, Bert Vermeulen wrote:
>> As it turns out, the set_cs() enable parameter refers to the logic level
>> on the CS pin, not the state of chip selection.
>
>> This broke functionality of the LEDs behind the CPLD, or at least delayed
>> the commands until another one came in to toggle CS.
>
> No, the enable parameter *should* refer to chip select assertion (see
> how we handle GPIO chip selects).  However it's possible that this
> device has an inverted chip select and should be registered with the
> SPI_CS_HIGH flag?

It's logic level:

 * @set_cs: set the logic level of the chip select line.  May be called
 *          from interrupt context.

See commit bd6857a0c630207484a03ddc470fab34b23f80bb
Author: Geert Uytterhoeven <geert+renesas@linux-m68k.org>
Date:   Tue Jan 21 16:10:07 2014 +0100

    spi: Correct set_cs() documentation

    The documentation for spi_master.set_cs() says:

        assert or deassert chip select, true to assert

    i.e. its "enable" parameter uses assertion-level logic.

    This does not match the implementation of spi_set_cs(), which calls
    spi_master.set_cs() with the wanted logic level of the chip select line,
    which depends on the polarity of the chip select signal.

    Correct the documentation to match the implementation.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH] spi: rb4xx: Fix set_cs logic.

[Bert Vermeulen]

On 04/21/2015 11:46 AM, Geert Uytterhoeven wrote:
> On Mon, Apr 20, 2015 at 10:37 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Mon, Apr 20, 2015 at 03:53:25PM +0200, Bert Vermeulen wrote:
>>> As it turns out, the set_cs() enable parameter refers to the logic level
>>> on the CS pin, not the state of chip selection.
>>
>>> This broke functionality of the LEDs behind the CPLD, or at least delayed
>>> the commands until another one came in to toggle CS.
>>
>> No, the enable parameter *should* refer to chip select assertion (see
>> how we handle GPIO chip selects).  However it's possible that this
>> device has an inverted chip select and should be registered with the
>> SPI_CS_HIGH flag?
> 
> It's logic level:
> 
>  * @set_cs: set the logic level of the chip select line.  May be called
>  *          from interrupt context.

Right It's the implementation which doesn't really make sense IMHO: it
always inverts the "enable" parameter (unless SPI_CS_HIGH is set), in
keeping with the default active-low.

So the docs are right, but "enable" doesn't match what it does. Chip select
assertion would be a better API here. Is it worth fixing?


-- 
Bert Vermeulen        bert@biot.com          email/xmpp

Re: [PATCH] spi: rb4xx: Fix set_cs logic.

[Mark Brown]

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

On Tue, Apr 21, 2015 at 01:01:22PM +0200, Bert Vermeulen wrote:
> On 04/21/2015 11:46 AM, Geert Uytterhoeven wrote:
> > On Mon, Apr 20, 2015 at 10:37 PM, Mark Brown <broonie@kernel.org> wrote:

> >> No, the enable parameter *should* refer to chip select assertion (see
> >> how we handle GPIO chip selects).  However it's possible that this
> >> device has an inverted chip select and should be registered with the
> >> SPI_CS_HIGH flag?

> > It's logic level:

> >  * @set_cs: set the logic level of the chip select line.  May be called
> >  *          from interrupt context.

> Right It's the implementation which doesn't really make sense IMHO: it
> always inverts the "enable" parameter (unless SPI_CS_HIGH is set), in
> keeping with the default active-low.

The default chip select is active low so an enebled chip select is low.

> So the docs are right, but "enable" doesn't match what it does. Chip select
> assertion would be a better API here. Is it worth fixing?

I suspect it's going to cause more breakage than it fixes with people
upstreaming things to change the sense of the paramter betwen kernel
versions - renaming the parameter to be clearer is probably about as
good as it gets.

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