spi: rb4xx: Fix set_cs logic.
PATCH 1/1 spi: rb4xx: Fix set_cs logic.
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.
--- drivers/spi/spi-rb4xx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/spi/spi-rb4xx.c +1 -1
@@ -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 ); }
Signature · 6 lines
1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
maintainerQuoted 5 lines · 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.
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?
Quoted 11 lines · Mark Brown
Quoted 5 lines · 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.
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_HIGHflag?
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 <[email protected]> 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 -- [email protected]
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
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · Geert Uytterhoeven
Quoted 11 lines · Mark Brown
Quoted 5 lines · 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.
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_HIGHflag?
Quoted 4 lines
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?
Signature · 1 line
Bert Vermeulen [email protected] email/xmpp
Mark Brown
maintainerQuoted 8 lines · Bert Vermeulen
Quoted 6 lines · Geert Uytterhoeven
Quoted 4 lines · Mark Brown
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_HIGHflag?
Quoted 8 lines
Quoted 4 lines
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_HIGHis set), in keeping with the default active-low.
The default chip select is active low so an enebled chip select is low.
Quoted 2 lines
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.