Re: SPI not working on 5.10 and 5.11, bisected to 766c6b63aa04 ("spi: fix client driver breakages when using GPIO descriptors")

On Thu, Jan 14, 2021 at 01:33:50PM +0100, Christophe Leroy wrote:

Le 14/01/2021 à 12:59, Mark Brown a écrit :

quoted

On Thu, Jan 14, 2021 at 12:27:42PM +0100, Christophe Leroy wrote:

quoted

quoted

Today I have in the DTS the CS GPIOs declared as ACTIVE_LOW.

quoted

quoted

If I declare them as ACTIVE_HIGH instead, then I also have to set
spi-cs-high property, otherwise of_gpio_flags_quirks() is not happy and
forces the GPIO ACTIVE LOW.

quoted

quoted

When I set spi-cs-high property, it sets the SPI_CS_HIGH bit in spi->mode.

quoted

OK, so it sounds like you want SPI_CS_HIGH and that is being set
correctly?

quoted

quoted

In fsl_spi_chipselect(), we have

	bool pol = spi->mode & SPI_CS_HIGH

Then
	pdata->cs_control(spi, pol);

quoted

quoted

So changing the board config is compensated by the above, and at the end it still doesn't work.

quoted

This is a driver bug, the driver set_cs() operation should not be
modifying the value it is told to set.

A driver bug ? Or maybe a change forgotten in commit  766c6b63aa04 ("spi:
fix client driver breakages when using GPIO descriptors") ?

The expectation that the driver will be using the chip select exactly as
passed in and not attempting to implement SPI_CS_HIGH itself when it has
set_cs() has been there for a considerable time now, that's not new with
the cleanup.  Drivers should only be paying attention to SPI_CS_HIGH in
cases where the hardware controls the chip select autonomously and so
set_cs() can't be provided.

I'm almost sure it was not a bug, it is in line which what is said in
the comment removed by the above mentionned commit.

Please take a look at the list archive discussions around this - there's
been a lot of confusion with GPIO descriptors in particular due to there
being multiple places where you can set the inversion.  Note that the
situation you describe above is that you end up with all the various
places other than your driver agreeing that the chip select is active
high as it (AFAICT from what you're saying) actually is.  

For GPIO chipselects you should really fix the driver to just hand the
GPIO off to the core rather than trying to implement this itself, that
will avoid driver specific differences like this.