mfd: motorola-cpcap: Do not hardcode SPI mode flags

Message ID 20191204231931.21378-1-linus.walleij@linaro.org
State New
Headers show
Series
  • mfd: motorola-cpcap: Do not hardcode SPI mode flags
Related show

Commit Message

Linus Walleij Dec. 4, 2019, 11:19 p.m.
The current use of mode flags to us SPI_MODE_0 and
SPI_CS_HIGH is fragile: it overwrites anything already
assigned by the SPI core. Change it thusly:

- Just |= the SPI_MODE_0 so we keep other flags
- Assign ^= SPI_CS_HIGH since we might be active high
  already, and that is usually the case with GPIOs used
  for chip select, even if they are in practice active low.

Add a comment clarifying why ^= SPI_CS_HIGH is the right
choice here.

Reported-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/mfd/motorola-cpcap.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

-- 
2.23.0

Comments

Tony Lindgren Dec. 19, 2019, 5:04 p.m. | #1
Hi Linus,

* Linus Walleij <linus.walleij@linaro.org> [700101 00:00]:
> The current use of mode flags to us SPI_MODE_0 and

> SPI_CS_HIGH is fragile: it overwrites anything already

> assigned by the SPI core. Change it thusly:

> 

> - Just |= the SPI_MODE_0 so we keep other flags

> - Assign ^= SPI_CS_HIGH since we might be active high

>   already, and that is usually the case with GPIOs used

>   for chip select, even if they are in practice active low.

> 

> Add a comment clarifying why ^= SPI_CS_HIGH is the right

> choice here.


Looks like this breaks booting for droid4 with a cpcap
PMIC, probably as regulators won't work. There's no GPIO
controller involved in this case for the chip select, the
pins are directly controlled by the spi-omap2-mcspi.c
driver.

From the pin muxing setup we see there's a pull-down on
mcspi1_cs0 pin meaning it's active high:

/* 0x4a100138 mcspi1_cs0.mcspi1_cs0 ae23 */
OMAP4_IOPAD(0x138, PIN_INPUT_PULLDOWN | MUX_MODE0)

My guess a similar issue is with similar patches for
all non-gpio spi controllers?

Let me know if you want me to test some other changes,
or if this patch depends on some other changes.

Regards,

Tony


> Reported-by: Mark Brown <broonie@kernel.org>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  drivers/mfd/motorola-cpcap.c | 8 +++++++-

>  1 file changed, 7 insertions(+), 1 deletion(-)

> 

> diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c

> index 52f38e57cdc1..a3bc61b8008c 100644

> --- a/drivers/mfd/motorola-cpcap.c

> +++ b/drivers/mfd/motorola-cpcap.c

> @@ -279,7 +279,13 @@ static int cpcap_probe(struct spi_device *spi)

>  	spi_set_drvdata(spi, cpcap);

>  

>  	spi->bits_per_word = 16;

> -	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;

> +	spi->mode |= SPI_MODE_0;

> +	/*

> +	 * Active high should be defined as "inverse polarity" as GPIO-based

> +	 * chip selects can be logically active high but inverted by the GPIO

> +	 * library.

> +	 */

> +	spi->mode ^= SPI_CS_HIGH;

>  

>  	ret = spi_setup(spi);

>  	if (ret)

> -- 

> 2.23.0

>

Patch

diff --git a/drivers/mfd/motorola-cpcap.c b/drivers/mfd/motorola-cpcap.c
index 52f38e57cdc1..a3bc61b8008c 100644
--- a/drivers/mfd/motorola-cpcap.c
+++ b/drivers/mfd/motorola-cpcap.c
@@ -279,7 +279,13 @@  static int cpcap_probe(struct spi_device *spi)
 	spi_set_drvdata(spi, cpcap);
 
 	spi->bits_per_word = 16;
-	spi->mode = SPI_MODE_0 | SPI_CS_HIGH;
+	spi->mode |= SPI_MODE_0;
+	/*
+	 * Active high should be defined as "inverse polarity" as GPIO-based
+	 * chip selects can be logically active high but inverted by the GPIO
+	 * library.
+	 */
+	spi->mode ^= SPI_CS_HIGH;
 
 	ret = spi_setup(spi);
 	if (ret)