| Message ID | 20240710175246.3560207-1-abbotti@mev.co.uk |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [RESEND] rtc: ds1343: Force SPI chip select to be active high | expand |
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@vger.kernel.org> # 5.6+ >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Mark Brown <broonie@kernel.org> >> Signed-off-by: Ian Abbott <abbotti@mev.co.uk> >> --- >> 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 >> >
On Thu, Jul 11, 2024 at 03:05:01PM +0100, Ian Abbott wrote: > 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. Specifying spi-cs-high in the device tree should almost always be redundant or a mistake, if the device needs a high chip select then we already know that from the compatible. The property is adding nothing but potential confusion, in the normal course of affairs the driver should just specify the configuration it needs for the bus.
On 11/07/2024 15:21, Mark Brown wrote: > On Thu, Jul 11, 2024 at 03:05:01PM +0100, Ian Abbott wrote: > >> 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. > > Specifying spi-cs-high in the device tree should almost always be > redundant or a mistake, if the device needs a high chip select then we > already know that from the compatible. The property is adding nothing > but potential confusion, in the normal course of affairs the driver > should just specify the configuration it needs for the bus. So `spi->mode |= SPI_CS_HIGH;` is safer than `spi->mode ^= SPI_CS_HIGH;`? Regarding `spi-cs-high` in the device tree, what about the compatibility table for `spi-cs-high` and `cs-gpio` active level in "Documentation/devicetree/bindings/spi/spi-controller.yaml"?
On Thu, Jul 11, 2024 at 04:29:07PM +0100, Ian Abbott wrote: > Regarding `spi-cs-high` in the device tree, what about the compatibility > table for `spi-cs-high` and `cs-gpio` active level in > "Documentation/devicetree/bindings/spi/spi-controller.yaml"? Specifying spi-cs-high should be equivalent to setting the mode in the driver, it's just a redundant way of saying the same thing.
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@vger.kernel.org> # 5.6+ >> Cc: Linus Walleij <linus.walleij@linaro.org> >> Cc: Mark Brown <broonie@kernel.org> >> Signed-off-by: Ian Abbott <abbotti@mev.co.uk> >> --- >> 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. > > Are you sure you are not missing an active high/low flag on a gpio > definition? > >> spi->bits_per_word = 8; >> res = spi_setup(spi); >> if (res) >> -- >> 2.43.0 >> > I now have an actual SPI controller using cs-gpios with a DS1343 connected. I have tested all 8 combinations of: GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH, with/without spi-cs-high, without/with my patch. Here are my results and observations: Final Final GPIO_ACTIVE spi-cs-high Patcb GPIO active CS high Works? =========== =========== ===== =========== ======= ======= LOW No No LOW Yes No LOW No Yes LOW Yes No LOW Yes No HIGH(1) No Yes(3) LOW Yes Yes HIGH(1) Yes Yes HIGH No No LOW(2) Yes No HIGH No Yes LOW(2) Yes No HIGH Yes No HIGH No Yes(3) HIGH Yes Yes HIGH Yes Yes The "Final GPIO active" column refers to the GPIO active state after any quirks have been applied. The "Final CS High" column refers to whether SPI_CS_HIGH is set in spi->mode when spi_setup() is called. Notes: (1) GPIO was forced active high by of_gpio_flags_quirks() before RTC device probed. (2) GPIO was forced active low by of_gpio_flags_quirks() before RTC device probed. (3) Works if cs-gpios being used, but probably will not work for SPI controllers that do not use cs-gpios because SPI_CS_HIGH is not set. In summary: 1. Without the patch, the RTC device node requires the spi-cs-high property to be present if the SPI controller uses cs-gpios, and requires the spi-cs-high property to be omitted if the SPI controller does not use cs-gpios. (I think that is a confusing situation.) 2. With the patch, the RTC device node requires the spi-cs-high property to be present if the SPI controller uses cs-gpios, and it does not care about the spi-cs-high property if the SPI controller does not use cs-gpios. (I therefore think that the patch should be applied so that the device node can just set spi-cs-high without caring whether ht SPI controller uses cs-gpios or not.)
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; spi->bits_per_word = 8; res = spi_setup(spi); if (res)
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@vger.kernel.org> # 5.6+ Cc: Linus Walleij <linus.walleij@linaro.org> Cc: Mark Brown <broonie@kernel.org> Signed-off-by: Ian Abbott <abbotti@mev.co.uk> --- drivers/rtc/rtc-ds1343.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)