Hi Mark,
On 02/12/2019 13:43, Mark Brown wrote:
On Mon, Dec 02, 2019 at 12:10:11PM +0000, Phil Elwell wrote:
A relatively recent patch [1] to the spi-bcm2835 driver modified it to use
GPIO descriptors for chip select handling. A side effect of this change is
to set the SPI_MODE_CS_HIGH flag for devices connected to the controller,
which seems strange since it happens for devices that require the usual
active-low chip select.
I take it you mean SPI_CS_HIGH rather than SPI_MODE_CS_HIGH?
Sorry, yes - SPI_CS_HIGH.
I
can't see anything in that change which sets the flag, can you be
more specific?
bcm2835_spi_probe in spi-bcm2835.c sets ctrl->use_gpio_descriptors to
true, and of_spi_parse_dt in spi.c includes the following:
/*
* For descriptors associated with the device, polarity inversion is
* handled in the gpiolib, so all gpio chip selects are "active high"
* in the logical sense, the gpiolib will invert the line if need be.
*/
if ((ctlr->use_gpio_descriptors) && ctlr->cs_gpiods &&
ctlr->cs_gpiods[spi->chip_select])
spi->mode |= SPI_CS_HIGH;
The change no longer acts on SPI_CS_HIGH when
requesting the GPIO but I can't see anything which *sets* the
flag. It does not visbily modify mode at all, nor did the
original code.
This change came to light when a user reported that the SPI-Py library
(a client of the spidev driver) wasn't working on 5.4, which was traced to
it overwriting the SPI mode flags when it was only trying to set the
CPHA and CPOL flags. This had the affect of inverting the chip select
line, with the obvious consequences. That corruption of the flags is clearly
an error, but what if the application and library were genuinely trying to
specify that the attached device required an active-high chip select? Would
it now require that they _clear_ the CS_HIGH flag?
I can't follow what you're saying here at all, sorry. Can you
please be more specific? You appear to be saying that the issue
is that the application modified the mode in some way and this
was acted on apparently not in the expected way.
That's correct. Although the trampling of the mode flags was erroneous,
actually the resulting flags were correct - SPI_CS_HIGH was not set,
yet the chip select line was clearly being driven in an active-high
fashion.
In general it's a bad idea to modify mode at runtime, and it's a
bad idea to mix multiple means of configuring the polarity of the
chip select (eg, mixing DT configuration with other means).
Applications using spidev to implement user-space drivers need to be
able to set SPI mode, CS polarity etc. at run time. I agree that there
are many ways to set the polarity of a chip select, and it may be that
too many are being used with GPIO CSs declared by DT, but I don't think
that completely explains the connection between use_gpio_descriptors
and the automatic setting of SPI_CS_HIGH.
Phil
