Greetings,
On 10/07/2024 19:40, Alexandre Belloni wrote:
Hello,
On 10/07/2024 18:52:07+0100, Ian Abbott wrote:
Commit 3b52093dc917 ("rtc: ds1343: Do not hardcode SPI mode flags")
bit-flips (^=) the existing SPI_CS_HIGH setting in the SPI mode during
device probe. This will set it to the wrong value if the spi-cs-high
property has been set in the devicetree node. Just force it to be set
active high and get rid of some commentary that attempted to explain why
flipping the bit was the correct choice.
Fixes: 3b52093dc917 ("rtc: ds1343: Do not hardcode SPI mode flags")
Cc: <stable@xxxxxxxxxxxxxxx> # 5.6+
Cc: Linus Walleij <linus.walleij@xxxxxxxxxx>
Cc: Mark Brown <broonie@xxxxxxxxxx>
Signed-off-by: Ian Abbott <abbotti@xxxxxxxxx>
---
drivers/rtc/rtc-ds1343.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/rtc/rtc-ds1343.c b/drivers/rtc/rtc-ds1343.c
index ed5a6ba89a3e..484b5756b55c 100644
--- a/drivers/rtc/rtc-ds1343.c
+++ b/drivers/rtc/rtc-ds1343.c
@@ -361,13 +361,10 @@ static int ds1343_probe(struct spi_device *spi)
if (!priv)
return -ENOMEM;
- /* RTC DS1347 works in spi mode 3 and
- * its chip select is active high. Active high should be defined as
- * "inverse polarity" as GPIO-based chip selects can be logically
- * active high but inverted by the GPIO library.
+ /*
+ * RTC DS1347 works in spi mode 3 and its chip select is active high.
*/
- spi->mode |= SPI_MODE_3;
- spi->mode ^= SPI_CS_HIGH;
+ spi->mode |= SPI_MODE_3 | SPI_CS_HIGH;
Linus being the gpio maintainer and Mark being the SPI maintainer, I'm
pretty sure this was correct at the time.
I'm not convinced. What value of `spi->mode & SPI_CS_HIGH` do you think
the device should end up using? (I think it should end up using
`SPI_CS_HIGH`, which was the case before commit 3b52093dc917, because
the RTC chip requires active high CS.) What do you think the value of
`spi->mode & SPI_CS_HIGH` will be *before* the `^=` operation? (For
devicetree, that depends on the `spi-cs-high` property.)
I think the devicetree node for the RTC device ought to be setting
`spi-cs-high` but cannot do so at the moment because the driver clobbers it.
Are you sure you are not missing an active high/low flag on a gpio
definition?
The CS might be internal to the SPI controller, not using a GPIO line
(`cs-gpios` property). SPI peripheral device drivers shouldn't care if
CS is using GPIO or not.
spi->bits_per_word = 8;
res = spi_setup(spi);
if (res)
--
2.43.0
--
-=( Ian Abbott <abbotti@xxxxxxxxx> || MEV Ltd. is a company )=-
-=( registered in England & Wales. Regd. number: 02862268. )=-
-=( Regd. addr.: S11 & 12 Building 67, Europa Business Park, )=-
-=( Bird Hall Lane, STOCKPORT, SK3 0XA, UK. || www.mev.co.uk )=-
