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 <[email protected]>
---
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
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 <[email protected]> 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 <[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
On 04/21/2015 11:46 AM, Geert Uytterhoeven wrote:
> On Mon, Apr 20, 2015 at 10:37 PM, Mark Brown <[email protected]> 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 [email protected] email/xmpp
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 <[email protected]> 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.