spi_mpc8xxx.c: chip select polarity problem

[Anton Vorontsov]

On Mon, Nov 16, 2009 at 05:42:46PM +0100, Torsten Fleischer wrote:
> Hi all,
> 
> I have 3 devices connected to the SPI bus of the MPC8313. For the Chip Select 
> (CS) signals 3 GPIOs of the controller are used. But the driver uses the 
> inverse polarity of the CS either during the initialization or at the transfer 
> - depending on the setup of the flattened device tree.
> 
> Here is what I discovered:
> The driver uses a polarity flag for each CS signal (the alow_flags array). 
> These flags are set according to the 'gpios' property of the SPI node of the 
> flattened device tree.
> Is it correct that alow_flags[x] = 1 means CSx is active low?

Either way should work. Though, now I tend to I think that
'spi-cs-high' is actually already encoded in the compatible
name. E.g. for device X we always know that the device assumes
CS to be active low, so it has no spi-cs-high.

How chip selects are wired to a device is another matter.

> During the initialization the driver sets the CS to the value of 
> alow_flags[x]. I.e. CSx is High if alow_flags[x] = 1 and otherwise Low.
> 
> The flags are used in the function mpc8xxx_spi_cs_control() to take care about 
> the polarity when setting the appropriate GPIO pin. But the function 
> mpc8xxx_spi_chipselect() that calls the mpc8xxx_spi_cs_control() takes also 
> care about the polarity of the CS (bool pol = spi->mode & SPI_CS_HIGH).
> 
> Lets assume alow_flags[x] = 1 and the property 'spi-cs-high' is not set for 
> the SPI device. During initialization the driver sets the chip select signal 
> 'x' to High (see of_mpc8xxx_spi_get_chipselects()). This is OK if the chip 
> select is active low, because this disables the device on start-up. But during 
> the transfer the chip select signal is High and after the transfer is 
> completed the signal is set to Low. This is not the intended behavior for an 
> active low chip select.
> 
> I also tried to set alow_flags[x] = 0 for active low. In this case the 
> transfer works, but the initial value for the CS is wrong (Low instead of 
> High).

So it might be better to fix up initial value in the platform code?

> The problem seems to be that the polarity is taken into account twice (as 
> described above).

Yep. So, today I'd suggest to not use spe-cs-high, even though
I was OK with it before.

> So what would be the better solution: removing the usage of the alow_flags in 
> mpc8xxx_spi_cs_control() or the variable 'pol' in mpc8xxx_spi_chipselect()?

Neither. 'pol' is still needed. Don't mix device wiring and the
chip select type. Driver may play active-low/high games with
a device, some drivers pass or clear SPI_CS_HIGH flags by themselves
(e.g. mmc_spi.c), so device-tree don't have to have spi-cs-high
flag specified.

But the wire from a GPIO controller to a SPI device can be inverted,
so you'll have to account that too!

Thanks,

-- 
Anton Vorontsov
email: cbouatmailru at gmail.com
irc://irc.freenode.net/bd2