On Wed, Dec 04, 2019 at 03:46:56PM +0100, Linus Walleij wrote: > and I can find one more outlier: > drivers/mmc/host/mmc_spi.c, function mmc_spi_initsequence() > This very clearly want the inverse polarity of whatever was the default, > but the code assumes that SPI_CS_HIGH implies that this is the > actual physical level and forces it high, then low. I will send > a patch for this: what they want to achieve is the inverse of > whatever was configured. Yes, that's the first one I looked at when I was poking around and I agree with that analysis. > I am a bit sorry about the semantics of "HIGH" here when the right > word should rather be "asserted": it's named like this for historical > reasons. There's quite a few things I'd do differently if I were designing the API. > An alternative would be to let SPI use gpiod_set_raw_value() > to steamroll all the inversion semantics in the GPIO library, > and SPI_CS_HIGH would literally mean to drive the physical > line high. > This may seem like an intuitively correct solution but > it has implications for everything using device tree to define > SPI clients: spi-cs-high in a device tree means that the > CS is active high, which gpiolib currently detects and > handles by way of disabling internal inversion (the default > for SPI devices is active low). Previously the SPI core would > set SPI_CS_HIGH and we would then just double-assign > that in the SPI core and then again in some drivers, it doesn't > seem much better to me. Yeah, my general thought is that trying to do anything other than making a new interface for this and actively deleting all the old ones is probably just going to be pushing the problem around and not actually any better. > I could also additionally patch the whole kernel to rename > SPI_CS_HIGH into SPI_CS_ASSERTED if this makes things > better. (Would be a two-step patch rocket adding the additional > define and switching everyone over before deleting the > SPI_CS_HIGH.) I think that's probably worth it mainly in the context of also auditing all the users and possibly making a bigger change which makes it harder for users to trigger problems. That's a lot more work though and needs more thought about what a better interface would be.
Attachment:
signature.asc
Description: PGP signature
