On Mon, Dec 02, 2019 at 02:11:51PM +0000, Phil Elwell wrote: > On 02/12/2019 13:43, Mark Brown wrote: > > On Mon, Dec 02, 2019 at 12:10:11PM +0000, Phil Elwell wrote: > > 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; Right, I see what you're saying now. > > 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 I'm nervous of spidev user doing stuff like that with the chip selects, with DT even spidev devices should be registered normally, you will get a complaint if you register a raw spidev. There's no free pass for "oh, spidev can do anything we don't care" here - the DT should describe the hardware, if some of the hardware happens to be implemented by spidev then fine. That said we do have other in kernel users that do change modes at runtime, though I'm not convinced many of them have GPIO chip selects. Linus? > 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. That's the result of there being too many places where the polarity of the chip select is set so the code is pushing to at least only implement this in one place so things are clearer and more consistent.
Attachment:
signature.asc
Description: PGP signature
