[PATCH] spi: Add option to keep the MOSI line low, when it is idle.

[PATCH] spi: Add option to keep the MOSI line low, when it is idle.

[Boerge Struempfel]

By default, the imx spi controller uses a high mosi line, whenever it is
idle. This may not be desired in all use cases. For example neopixel
leds can get confused and flicker due to misinterpreting the idle state.
Therefore, we introduce a new spi-mode bit, with which the idle behaviour
can be overwritten on a per device basis.

Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
---
 drivers/spi/spi-imx.c        | 9 ++++++++-
 drivers/spi/spi.c            | 2 ++
 include/uapi/linux/spi/spi.h | 3 ++-
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 34e5f81ec431e..6acab2b4ffaa5 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -281,6 +281,7 @@ static bool spi_imx_can_dma(struct spi_controller *controller, struct spi_device
 #define MX51_ECSPI_CONFIG_SCLKPOL(cs)	(1 << ((cs & 3) +  4))
 #define MX51_ECSPI_CONFIG_SBBCTRL(cs)	(1 << ((cs & 3) +  8))
 #define MX51_ECSPI_CONFIG_SSBPOL(cs)	(1 << ((cs & 3) + 12))
+#define MX51_ECSPI_CONFIG_DATACTL(cs)	(1 << ((cs & 3) + 16))
 #define MX51_ECSPI_CONFIG_SCLKCTL(cs)	(1 << ((cs & 3) + 20))
 
 #define MX51_ECSPI_INT		0x10
@@ -573,6 +574,11 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
 		cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi_get_chipselect(spi, 0));
 	}
 
+	if (spi->mode & SPI_MOSI_IDLE_LOW)
+		cfg |= MX51_ECSPI_CONFIG_DATACTL(spi->chip_select);
+	else
+		cfg &= ~MX51_ECSPI_CONFIG_DATACTL(spi->chip_select);
+
 	if (spi->mode & SPI_CS_HIGH)
 		cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi_get_chipselect(spi, 0));
 	else
@@ -1743,7 +1749,8 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->controller->prepare_message = spi_imx_prepare_message;
 	spi_imx->controller->unprepare_message = spi_imx_unprepare_message;
 	spi_imx->controller->slave_abort = spi_imx_slave_abort;
-	spi_imx->controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_NO_CS;
+	spi_imx->controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_NO_CS |
+					 SPI_MOSI_IDLE_LOW;
 
 	if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx) ||
 	    is_imx53_ecspi(spi_imx))
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9291b2a0e8871..3ad538b317a84 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2260,6 +2260,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 		spi->mode |= SPI_LSB_FIRST;
 	if (of_property_read_bool(nc, "spi-cs-high"))
 		spi->mode |= SPI_CS_HIGH;
+	if (of_property_read_bool(nc, "spi-mosi-idle-low"))
+		spi->mode |= SPI_MOSI_IDLE_LOW;
 
 	/* Device DUAL/QUAD mode */
 	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
index 9d5f580597039..ca56e477d1619 100644
--- a/include/uapi/linux/spi/spi.h
+++ b/include/uapi/linux/spi/spi.h
@@ -28,6 +28,7 @@
 #define	SPI_RX_OCTAL		_BITUL(14)	/* receive with 8 wires */
 #define	SPI_3WIRE_HIZ		_BITUL(15)	/* high impedance turnaround */
 #define	SPI_RX_CPHA_FLIP	_BITUL(16)	/* flip CPHA on Rx only xfer */
+#define SPI_MOSI_IDLE_LOW	_BITUL(17)	/* leave mosi line low when idle */
 
 /*
  * All the bits defined above should be covered by SPI_MODE_USER_MASK.
@@ -37,6 +38,6 @@
  * These bits must not overlap. A static assert check should make sure of that.
  * If adding extra bits, make sure to increase the bit index below as well.
  */
-#define SPI_MODE_USER_MASK	(_BITUL(17) - 1)
+#define SPI_MODE_USER_MASK	(_BITUL(18) - 1)
 
 #endif /* _UAPI_SPI_H */
-- 
2.25.1

Re: [PATCH] spi: Add option to keep the MOSI line low, when it is idle.

[Fabio Estevam]

Hi Boerge,

On Thu, May 11, 2023 at 10:58 AM Boerge Struempfel
<boerge.struempfel@gmail.com> wrote:
>
> By default, the imx spi controller uses a high mosi line, whenever it is
> idle. This may not be desired in all use cases. For example neopixel
> leds can get confused and flicker due to misinterpreting the idle state.
> Therefore, we introduce a new spi-mode bit, with which the idle behaviour
> can be overwritten on a per device basis.
...
> +       if (of_property_read_bool(nc, "spi-mosi-idle-low"))
> +               spi->mode |= SPI_MOSI_IDLE_LOW;

Yes, this is useful.

As this is a new property,  please send a patch that adds it to:
Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml

Thanks

[PATCH v2 0/4] spi: Add option to keep the MOSI line low, when it is idle

[Boerge Struempfel]

Some spi controller like the imx spi controller switch the mosi line to
high, whenever they are idle. This may not be desired in all use cases.
For example neopixel leds can get confused and flicker due to
misinterpreting the idle state. Therefore, we introduce a new spi-mode
bit, with which the idle behaviour can be overwritten on a per device
basis.

Changes from V1:
  - Added patch, introducing the new devicetree binding flag
  - Split the generic spi part of the patch from the imx-spi specific
    part
  - Replaced SPI_CPOL and SPI_CPHA by the combined SPI_MODE_X_MASK bit
    in the imx-spi.c modebits.
  - Added the SPI_MOSI_IDLE_LOW bit to spidev

Boerge Struempfel (4):
  spi: dt-bindings: Introduce spi-mosi-idle-low flag
  spi: add SPI_MOSI_IDLE_LOW mode bit
  spi: spi-imx: add support for SPI_MOSI_IDLE_LOW mode bit
  spi: spidev: add SPI_MOSI_IDLE_LOW mode bit

 .../devicetree/bindings/spi/spi-peripheral-props.yaml    | 6 ++++++
 drivers/spi/spi-imx.c                                    | 9 ++++++++-
 drivers/spi/spi.c                                        | 2 ++
 drivers/spi/spidev.c                                     | 2 +-
 include/uapi/linux/spi/spi.h                             | 3 ++-
 5 files changed, 19 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH v2 1/4] spi: dt-bindings: Introduce spi-mosi-idle-low flag

[Boerge Struempfel]

Some spi controller switch the mosi line to high, whenever they are
idle. This may not be desired in all use cases. For example neopixel
leds can get confused and flicker due to misinterpreting the idle state.
Therefore, we introduce a new spi-mode bit, with which the idle behaviour
can be overwritten on a per device basis.

Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
---
 .../devicetree/bindings/spi/spi-peripheral-props.yaml       | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 782a014b63a76..46d5acc1fc232 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -34,6 +34,12 @@ properties:
     description:
       The device requires the chip select active high.
 
+  spi-mosi-idle-low:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      The device requires the mosi line to be low when idle and the
+      chip select is asserted.
+
   spi-lsb-first:
     $ref: /schemas/types.yaml#/definitions/flag
     description:
-- 
2.25.1

[PATCH v2 2/4] spi: add SPI_MOSI_IDLE_LOW mode bit

[Boerge Struempfel]

Some spi controller switch the mosi line to high, whenever they are
idle. This may not be desired in all use cases. For example neopixel
leds can get confused and flicker due to misinterpreting the idle state.
Therefore, we introduce a new spi-mode bit, with which the idle behaviour
can be overwritten on a per device basis.

Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
---
 drivers/spi/spi.c            | 2 ++
 include/uapi/linux/spi/spi.h | 3 ++-
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 9291b2a0e8871..3ad538b317a84 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -2260,6 +2260,8 @@ static int of_spi_parse_dt(struct spi_controller *ctlr, struct spi_device *spi,
 		spi->mode |= SPI_LSB_FIRST;
 	if (of_property_read_bool(nc, "spi-cs-high"))
 		spi->mode |= SPI_CS_HIGH;
+	if (of_property_read_bool(nc, "spi-mosi-idle-low"))
+		spi->mode |= SPI_MOSI_IDLE_LOW;
 
 	/* Device DUAL/QUAD mode */
 	if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) {
diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
index 9d5f580597039..ca56e477d1619 100644
--- a/include/uapi/linux/spi/spi.h
+++ b/include/uapi/linux/spi/spi.h
@@ -28,6 +28,7 @@
 #define	SPI_RX_OCTAL		_BITUL(14)	/* receive with 8 wires */
 #define	SPI_3WIRE_HIZ		_BITUL(15)	/* high impedance turnaround */
 #define	SPI_RX_CPHA_FLIP	_BITUL(16)	/* flip CPHA on Rx only xfer */
+#define SPI_MOSI_IDLE_LOW	_BITUL(17)	/* leave mosi line low when idle */
 
 /*
  * All the bits defined above should be covered by SPI_MODE_USER_MASK.
@@ -37,6 +38,6 @@
  * These bits must not overlap. A static assert check should make sure of that.
  * If adding extra bits, make sure to increase the bit index below as well.
  */
-#define SPI_MODE_USER_MASK	(_BITUL(17) - 1)
+#define SPI_MODE_USER_MASK	(_BITUL(18) - 1)
 
 #endif /* _UAPI_SPI_H */
-- 
2.25.1

[PATCH v2 3/4] spi: spi-imx: add support for SPI_MOSI_IDLE_LOW mode bit

[Boerge Struempfel]

By default, the spi-imx controller pulls the mosi line high, whenever it
is idle. This behaviour can be inverted per CS by setting the
corresponding DATA_CTL bit in the config register of the controller.

Also, since the controller mode-bits have to be touched anyways, the
SPI_CPOL and SPI_CPHA are replaced by the combined SPI_MODE_X_MASK flag.

Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
---
 drivers/spi/spi-imx.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 34e5f81ec431e..cb6c088706b21 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -281,6 +281,7 @@ static bool spi_imx_can_dma(struct spi_controller *controller, struct spi_device
 #define MX51_ECSPI_CONFIG_SCLKPOL(cs)	(1 << ((cs & 3) +  4))
 #define MX51_ECSPI_CONFIG_SBBCTRL(cs)	(1 << ((cs & 3) +  8))
 #define MX51_ECSPI_CONFIG_SSBPOL(cs)	(1 << ((cs & 3) + 12))
+#define MX51_ECSPI_CONFIG_DATACTL(cs)	(1 << ((cs & 3) + 16))
 #define MX51_ECSPI_CONFIG_SCLKCTL(cs)	(1 << ((cs & 3) + 20))
 
 #define MX51_ECSPI_INT		0x10
@@ -573,6 +574,11 @@ static int mx51_ecspi_prepare_message(struct spi_imx_data *spi_imx,
 		cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi_get_chipselect(spi, 0));
 	}
 
+	if (spi->mode & SPI_MOSI_IDLE_LOW)
+		cfg |= MX51_ECSPI_CONFIG_DATACTL(spi->chip_select);
+	else
+		cfg &= ~MX51_ECSPI_CONFIG_DATACTL(spi->chip_select);
+
 	if (spi->mode & SPI_CS_HIGH)
 		cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi_get_chipselect(spi, 0));
 	else
@@ -1743,7 +1749,8 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->controller->prepare_message = spi_imx_prepare_message;
 	spi_imx->controller->unprepare_message = spi_imx_unprepare_message;
 	spi_imx->controller->slave_abort = spi_imx_slave_abort;
-	spi_imx->controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_NO_CS;
+	spi_imx->controller->mode_bits = SPI_MODE_X_MASK | SPI_CS_HIGH | SPI_NO_CS |
+					 SPI_MOSI_IDLE_LOW;
 
 	if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx) ||
 	    is_imx53_ecspi(spi_imx))
-- 
2.25.1

[PATCH v2 4/4] spi: spidev: add SPI_MOSI_IDLE_LOW mode bit

[Boerge Struempfel]

Allow userspace to set SPI_MOSI_IDLE_LOW mode bit using the
SPI_IOC_WR_MODE32 ioctl.

Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
---
 drivers/spi/spidev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index 39d94c8508390..e50da54468ec6 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -64,7 +64,7 @@ static_assert(N_SPI_MINORS > 0 && N_SPI_MINORS <= 256);
 				| SPI_NO_CS | SPI_READY | SPI_TX_DUAL \
 				| SPI_TX_QUAD | SPI_TX_OCTAL | SPI_RX_DUAL \
 				| SPI_RX_QUAD | SPI_RX_OCTAL \
-				| SPI_RX_CPHA_FLIP)
+				| SPI_RX_CPHA_FLIP | SPI_MOSI_IDLE_LOW)
 
 struct spidev_data {
 	dev_t			devt;
-- 
2.25.1

Re: [PATCH v2 1/4] spi: dt-bindings: Introduce spi-mosi-idle-low flag

[Fabio Estevam]

On Thu, May 11, 2023 at 8:14 PM Boerge Struempfel
<boerge.struempfel@gmail.com> wrote:
>
> Some spi controller switch the mosi line to high, whenever they are
> idle. This may not be desired in all use cases. For example neopixel
> leds can get confused and flicker due to misinterpreting the idle state.
> Therefore, we introduce a new spi-mode bit, with which the idle behaviour
> can be overwritten on a per device basis.
>
> Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>

Please run checkpatch on all the patches.

The following issue reported by checkpatch needs to be fixed:

WARNING: From:/Signed-off-by: email address mismatch: 'From: Boerge
Struempfel <boerge.struempfel@gmail.com>' != 'Signed-off-by: Boerge
Struempfel <bstruempfel@ultratronik.de>'

Re: [PATCH v2 0/4] spi: Add option to keep the MOSI line low, when it is idle

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On Fri, May 12, 2023 at 01:13:13AM +0200, Boerge Struempfel wrote:
> Some spi controller like the imx spi controller switch the mosi line to
> high, whenever they are idle. This may not be desired in all use cases.
> For example neopixel leds can get confused and flicker due to
> misinterpreting the idle state. Therefore, we introduce a new spi-mode
> bit, with which the idle behaviour can be overwritten on a per device
> basis.

Please don't send new patches in reply to old patches or serieses, this
makes it harder for both people and tools to understand what is going
on - it can bury things in mailboxes and make it difficult to keep track
of what current patches are, both for the new patches and the old ones.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] spi: dt-bindings: Introduce spi-mosi-idle-low flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 798 bytes --]

On Fri, May 12, 2023 at 01:13:14AM +0200, Boerge Struempfel wrote:
> Some spi controller switch the mosi line to high, whenever they are
> idle. This may not be desired in all use cases. For example neopixel
> leds can get confused and flicker due to misinterpreting the idle state.
> Therefore, we introduce a new spi-mode bit, with which the idle behaviour
> can be overwritten on a per device basis.
> 
> Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
> ---
>  .../devicetree/bindings/spi/spi-peripheral-props.yaml       | 6 ++++++

If this is always required for a given device (which I'd expect to be
the case) why configure it through DT?  I know we've got some legacy
stuff like that but not all legacy DT choices were good and no need to
continue the pattern.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/4] spi: dt-bindings: Introduce spi-mosi-idle-low flag

[Börge Strümpfel]

Am Fr., 12. Mai 2023 um 05:30 Uhr schrieb Mark Brown <broonie@kernel.org>:
>
> On Fri, May 12, 2023 at 01:13:14AM +0200, Boerge Struempfel wrote:
> > Some spi controller switch the mosi line to high, whenever they are
> > idle. This may not be desired in all use cases. For example neopixel
> > leds can get confused and flicker due to misinterpreting the idle state.
> > Therefore, we introduce a new spi-mode bit, with which the idle behaviour
> > can be overwritten on a per device basis.
> >
> > Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
> > ---
> >  .../devicetree/bindings/spi/spi-peripheral-props.yaml       | 6 ++++++
>
> If this is always required for a given device (which I'd expect to be
> the case) why configure it through DT?  I know we've got some legacy
> stuff like that but not all legacy DT choices were good and no need to
> continue the pattern.

Yes this will always be the case for specific spi-device, spi-controller
combinations. Just to make sure, that I understand your suggestion
correctly: You propose to check from the specific spi-device-driver, if
the spi-controller supports this particular mode-bit, and then set it if
it does and thereby loose the need for the DT entry completely?

Re: [PATCH v2 1/4] spi: dt-bindings: Introduce spi-mosi-idle-low flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 888 bytes --]

On Fri, May 12, 2023 at 08:54:19AM +0200, Börge Strümpfel wrote:
> Am Fr., 12. Mai 2023 um 05:30 Uhr schrieb Mark Brown <broonie@kernel.org>:

> > If this is always required for a given device (which I'd expect to be
> > the case) why configure it through DT?  I know we've got some legacy
> > stuff like that but not all legacy DT choices were good and no need to
> > continue the pattern.

> Yes this will always be the case for specific spi-device, spi-controller
> combinations. Just to make sure, that I understand your suggestion
> correctly: You propose to check from the specific spi-device-driver, if
> the spi-controller supports this particular mode-bit, and then set it if
> it does and thereby loose the need for the DT entry completely?

Yes, we shouldn't need DT here.  Though the device should just be
setting this unconditionally if it's always required.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

RE: [PATCH] spi: Add option to keep the MOSI line low, when it is idle.

[Mahapatra, Amit Kumar]

Hello,

> -----Original Message-----
> From: Boerge Struempfel <boerge.struempfel@gmail.com>
> Sent: Thursday, May 11, 2023 7:27 PM
> Cc: boerge.struempfel@gmail.com; bstruempfel@ultratronik.de; Mark
> Brown <broonie@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha
> Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; NXP
> Linux Team <linux-imx@nxp.com>; linux-spi@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: [PATCH] spi: Add option to keep the MOSI line low, when it is idle.
> 
> CAUTION: This message has originated from an External Source. Please use
> proper judgment and caution when opening attachments, clicking links, or
> responding to this email.
> 
> 
> By default, the imx spi controller uses a high mosi line, whenever it is idle.
> This may not be desired in all use cases. For example neopixel leds can get
> confused and flicker due to misinterpreting the idle state.
> Therefore, we introduce a new spi-mode bit, with which the idle behaviour
> can be overwritten on a per device basis.
> 
> Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
> ---
>  drivers/spi/spi-imx.c        | 9 ++++++++-
>  drivers/spi/spi.c            | 2 ++
>  include/uapi/linux/spi/spi.h | 3 ++-
>  3 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> 34e5f81ec431e..6acab2b4ffaa5 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -281,6 +281,7 @@ static bool spi_imx_can_dma(struct spi_controller
> *controller, struct spi_device  #define MX51_ECSPI_CONFIG_SCLKPOL(cs)  (1
> << ((cs & 3) +  4))  #define MX51_ECSPI_CONFIG_SBBCTRL(cs)  (1 << ((cs & 3) +
> 8))
>  #define MX51_ECSPI_CONFIG_SSBPOL(cs)   (1 << ((cs & 3) + 12))
> +#define MX51_ECSPI_CONFIG_DATACTL(cs)  (1 << ((cs & 3) + 16))
>  #define MX51_ECSPI_CONFIG_SCLKCTL(cs)  (1 << ((cs & 3) + 20))
> 
>  #define MX51_ECSPI_INT         0x10
> @@ -573,6 +574,11 @@ static int mx51_ecspi_prepare_message(struct
> spi_imx_data *spi_imx,
>                 cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi_get_chipselect(spi, 0));
>         }
> 
> +       if (spi->mode & SPI_MOSI_IDLE_LOW)
> +               cfg |= MX51_ECSPI_CONFIG_DATACTL(spi->chip_select);

Kindly replace all occurrence of spi->chip_select with spi_get_chipselect(spi, 0)
https://github.com/torvalds/linux/commit/9e264f3f85a56cc109cc2d6010a48aa89d5c1ff1

> +       else
> +               cfg &= ~MX51_ECSPI_CONFIG_DATACTL(spi->chip_select);

> +
>         if (spi->mode & SPI_CS_HIGH)
>                 cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi_get_chipselect(spi, 0));
>         else
> @@ -1743,7 +1749,8 @@ static int spi_imx_probe(struct platform_device
> *pdev)
>         spi_imx->controller->prepare_message = spi_imx_prepare_message;
>         spi_imx->controller->unprepare_message =
> spi_imx_unprepare_message;
>         spi_imx->controller->slave_abort = spi_imx_slave_abort;
> -       spi_imx->controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH |
> SPI_NO_CS;
> +       spi_imx->controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH
> | SPI_NO_CS |
> +                                        SPI_MOSI_IDLE_LOW;
> 
>         if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx) ||
>             is_imx53_ecspi(spi_imx))
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index
> 9291b2a0e8871..3ad538b317a84 100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -2260,6 +2260,8 @@ static int of_spi_parse_dt(struct spi_controller
> *ctlr, struct spi_device *spi,
>                 spi->mode |= SPI_LSB_FIRST;
>         if (of_property_read_bool(nc, "spi-cs-high"))
>                 spi->mode |= SPI_CS_HIGH;
> +       if (of_property_read_bool(nc, "spi-mosi-idle-low"))
> +               spi->mode |= SPI_MOSI_IDLE_LOW;
> 
>         /* Device DUAL/QUAD mode */
>         if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { diff --git
> a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h index
> 9d5f580597039..ca56e477d1619 100644
> --- a/include/uapi/linux/spi/spi.h
> +++ b/include/uapi/linux/spi/spi.h
> @@ -28,6 +28,7 @@
>  #define        SPI_RX_OCTAL            _BITUL(14)      /* receive with 8 wires */
>  #define        SPI_3WIRE_HIZ           _BITUL(15)      /* high impedance
> turnaround */
>  #define        SPI_RX_CPHA_FLIP        _BITUL(16)      /* flip CPHA on Rx only xfer
> */
> +#define SPI_MOSI_IDLE_LOW      _BITUL(17)      /* leave mosi line low when
> idle */
> 
>  /*
>   * All the bits defined above should be covered by SPI_MODE_USER_MASK.
> @@ -37,6 +38,6 @@
>   * These bits must not overlap. A static assert check should make sure of
> that.
>   * If adding extra bits, make sure to increase the bit index below as well.
>   */
> -#define SPI_MODE_USER_MASK     (_BITUL(17) - 1)
> +#define SPI_MODE_USER_MASK     (_BITUL(18) - 1)
> 
>  #endif /* _UAPI_SPI_H */
> --
> 2.25.1

Re: [PATCH v2 1/4] spi: dt-bindings: Introduce spi-mosi-idle-low flag

[Börge Strümpfel]

Thank you for your feedback

Am Mo., 15. Mai 2023 um 02:34 Uhr schrieb Mark Brown <broonie@kernel.org>:
>
> On Fri, May 12, 2023 at 08:54:19AM +0200, Börge Strümpfel wrote:
> > Am Fr., 12. Mai 2023 um 05:30 Uhr schrieb Mark Brown <broonie@kernel.org>:
>
> > > If this is always required for a given device (which I'd expect to be
> > > the case) why configure it through DT?  I know we've got some legacy
> > > stuff like that but not all legacy DT choices were good and no need to
> > > continue the pattern.
>
> > Yes this will always be the case for specific spi-device, spi-controller
> > combinations. Just to make sure, that I understand your suggestion
> > correctly: You propose to check from the specific spi-device-driver, if
> > the spi-controller supports this particular mode-bit, and then set it if
> > it does and thereby loose the need for the DT entry completely?
>
> Yes, we shouldn't need DT here.  Though the device should just be
> setting this unconditionally if it's always required.

I agree with you, that we should not need DT here. I will remove the
dt-binding in the next patch version.

However I am not so sure about setting it unconditionally, since this
is dependent on the spi-controller. Not all spi-controller show this
behavior, that they use a high mosi line in idle mode and have the
ability to change this. As far as I know, another common behavior
is that the mosi just keeps the last state which it transmitted. In this
case, devices like Neopixel would still work without this mode bit.

Re: [PATCH] spi: Add option to keep the MOSI line low, when it is idle.

[Börge Strümpfel]

Am Mo., 15. Mai 2023 um 08:37 Uhr schrieb Mahapatra, Amit Kumar
<amit.kumar-mahapatra@amd.com>:
>
> Hello,
>
> > -----Original Message-----
> > From: Boerge Struempfel <boerge.struempfel@gmail.com>
> > Sent: Thursday, May 11, 2023 7:27 PM
> > Cc: boerge.struempfel@gmail.com; bstruempfel@ultratronik.de; Mark
> > Brown <broonie@kernel.org>; Shawn Guo <shawnguo@kernel.org>; Sascha
> > Hauer <s.hauer@pengutronix.de>; Pengutronix Kernel Team
> > <kernel@pengutronix.de>; Fabio Estevam <festevam@gmail.com>; NXP
> > Linux Team <linux-imx@nxp.com>; linux-spi@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> > Subject: [PATCH] spi: Add option to keep the MOSI line low, when it is idle.
> >
> > CAUTION: This message has originated from an External Source. Please use
> > proper judgment and caution when opening attachments, clicking links, or
> > responding to this email.
> >
> >
> > By default, the imx spi controller uses a high mosi line, whenever it is idle.
> > This may not be desired in all use cases. For example neopixel leds can get
> > confused and flicker due to misinterpreting the idle state.
> > Therefore, we introduce a new spi-mode bit, with which the idle behaviour
> > can be overwritten on a per device basis.
> >
> > Signed-off-by: Boerge Struempfel <bstruempfel@ultratronik.de>
> > ---
> >  drivers/spi/spi-imx.c        | 9 ++++++++-
> >  drivers/spi/spi.c            | 2 ++
> >  include/uapi/linux/spi/spi.h | 3 ++-
> >  3 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c index
> > 34e5f81ec431e..6acab2b4ffaa5 100644
> > --- a/drivers/spi/spi-imx.c
> > +++ b/drivers/spi/spi-imx.c
> > @@ -281,6 +281,7 @@ static bool spi_imx_can_dma(struct spi_controller
> > *controller, struct spi_device  #define MX51_ECSPI_CONFIG_SCLKPOL(cs)  (1
> > << ((cs & 3) +  4))  #define MX51_ECSPI_CONFIG_SBBCTRL(cs)  (1 << ((cs & 3) +
> > 8))
> >  #define MX51_ECSPI_CONFIG_SSBPOL(cs)   (1 << ((cs & 3) + 12))
> > +#define MX51_ECSPI_CONFIG_DATACTL(cs)  (1 << ((cs & 3) + 16))
> >  #define MX51_ECSPI_CONFIG_SCLKCTL(cs)  (1 << ((cs & 3) + 20))
> >
> >  #define MX51_ECSPI_INT         0x10
> > @@ -573,6 +574,11 @@ static int mx51_ecspi_prepare_message(struct
> > spi_imx_data *spi_imx,
> >                 cfg &= ~MX51_ECSPI_CONFIG_SCLKCTL(spi_get_chipselect(spi, 0));
> >         }
> >
> > +       if (spi->mode & SPI_MOSI_IDLE_LOW)
> > +               cfg |= MX51_ECSPI_CONFIG_DATACTL(spi->chip_select);
>
> Kindly replace all occurrence of spi->chip_select with spi_get_chipselect(spi, 0)
> https://github.com/torvalds/linux/commit/9e264f3f85a56cc109cc2d6010a48aa89d5c1ff1
Thank you very much for noticing this. I have changed it for the next
version of the patch.
>
> > +       else
> > +               cfg &= ~MX51_ECSPI_CONFIG_DATACTL(spi->chip_select);
>
> > +
> >         if (spi->mode & SPI_CS_HIGH)
> >                 cfg |= MX51_ECSPI_CONFIG_SSBPOL(spi_get_chipselect(spi, 0));
> >         else
> > @@ -1743,7 +1749,8 @@ static int spi_imx_probe(struct platform_device
> > *pdev)
> >         spi_imx->controller->prepare_message = spi_imx_prepare_message;
> >         spi_imx->controller->unprepare_message =
> > spi_imx_unprepare_message;
> >         spi_imx->controller->slave_abort = spi_imx_slave_abort;
> > -       spi_imx->controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH |
> > SPI_NO_CS;
> > +       spi_imx->controller->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH
> > | SPI_NO_CS |
> > +                                        SPI_MOSI_IDLE_LOW;
> >
> >         if (is_imx35_cspi(spi_imx) || is_imx51_ecspi(spi_imx) ||
> >             is_imx53_ecspi(spi_imx))
> > diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index
> > 9291b2a0e8871..3ad538b317a84 100644
> > --- a/drivers/spi/spi.c
> > +++ b/drivers/spi/spi.c
> > @@ -2260,6 +2260,8 @@ static int of_spi_parse_dt(struct spi_controller
> > *ctlr, struct spi_device *spi,
> >                 spi->mode |= SPI_LSB_FIRST;
> >         if (of_property_read_bool(nc, "spi-cs-high"))
> >                 spi->mode |= SPI_CS_HIGH;
> > +       if (of_property_read_bool(nc, "spi-mosi-idle-low"))
> > +               spi->mode |= SPI_MOSI_IDLE_LOW;
> >
> >         /* Device DUAL/QUAD mode */
> >         if (!of_property_read_u32(nc, "spi-tx-bus-width", &value)) { diff --git
> > a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h index
> > 9d5f580597039..ca56e477d1619 100644
> > --- a/include/uapi/linux/spi/spi.h
> > +++ b/include/uapi/linux/spi/spi.h
> > @@ -28,6 +28,7 @@
> >  #define        SPI_RX_OCTAL            _BITUL(14)      /* receive with 8 wires */
> >  #define        SPI_3WIRE_HIZ           _BITUL(15)      /* high impedance
> > turnaround */
> >  #define        SPI_RX_CPHA_FLIP        _BITUL(16)      /* flip CPHA on Rx only xfer
> > */
> > +#define SPI_MOSI_IDLE_LOW      _BITUL(17)      /* leave mosi line low when
> > idle */
> >
> >  /*
> >   * All the bits defined above should be covered by SPI_MODE_USER_MASK.
> > @@ -37,6 +38,6 @@
> >   * These bits must not overlap. A static assert check should make sure of
> > that.
> >   * If adding extra bits, make sure to increase the bit index below as well.
> >   */
> > -#define SPI_MODE_USER_MASK     (_BITUL(17) - 1)
> > +#define SPI_MODE_USER_MASK     (_BITUL(18) - 1)
> >
> >  #endif /* _UAPI_SPI_H */
> > --
> > 2.25.1
>

Re: [PATCH v2 1/4] spi: dt-bindings: Introduce spi-mosi-idle-low flag

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 606 bytes --]

On Wed, May 17, 2023 at 10:26:07AM +0200, Börge Strümpfel wrote:

> However I am not so sure about setting it unconditionally, since this
> is dependent on the spi-controller. Not all spi-controller show this
> behavior, that they use a high mosi line in idle mode and have the
> ability to change this. As far as I know, another common behavior
> is that the mosi just keeps the last state which it transmitted. In this
> case, devices like Neopixel would still work without this mode bit.

The behaviour the device needs is that the device have a low MOSI, how
that is achieved is immaterial.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]