| Message ID | 1429538005-1985-1-git-send-email-bert@biot.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4a1ae8be4563d29ddd36f46759191f4e867ed954 |
| Headers | show |
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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.
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); }
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(-)