[PATCH v4 1/3] spi: add SPI_MOSI_IDLE_LOW mode bit

[PATCH v4 1/3] 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 <boerge.struempfel@gmail.com>


Link for versions:
  v1 and v2: https://lore.kernel.org/linux-spi/20230511135632.78344-1-bstruempfel@ultratronik.de/
  v3: https://lore.kernel.org/linux-spi/20230517103007.26287-1-boerge.struempfel@gmail.com/T/#t

Changes from V3:
  - Added missing paranthesis which caused builderrors

Changes from V2:
  - Removed the device-tree binding since this should not be managed by
    the DT but by the device itself.
  - Replaced all occurences of spi->chip_select with the corresponding 
    macro spi_get_chipselect(spi,0)

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

---
 include/uapi/linux/spi/spi.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
index 9d5f58059703..ca56e477d161 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 v4 2/3] 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 <boerge.struempfel@gmail.com>
---
 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 34e5f81ec431..1c4172fcba2d 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_get_chipselect(spi, 0));
+	else
+		cfg &= ~MX51_ECSPI_CONFIG_DATACTL(spi_get_chipselect(spi, 0));
+
 	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 v4 3/3] 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 <boerge.struempfel@gmail.com>
---
 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 39d94c850839..e50da54468ec 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 v4 1/3] spi: add SPI_MOSI_IDLE_LOW mode bit

[Fabio Estevam]

On Wed, May 17, 2023 at 7:30 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 <boerge.struempfel@gmail.com>
>
>
> Link for versions:
>   v1 and v2: https://lore.kernel.org/linux-spi/20230511135632.78344-1-bstruempfel@ultratronik.de/
>   v3: https://lore.kernel.org/linux-spi/20230517103007.26287-1-boerge.struempfel@gmail.com/T/#t
>
> Changes from V3:
>   - Added missing paranthesis which caused builderrors
>
> Changes from V2:
>   - Removed the device-tree binding since this should not be managed by
>     the DT but by the device itself.
>   - Replaced all occurences of spi->chip_select with the corresponding
>     macro spi_get_chipselect(spi,0)
>
> 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

The change log should be placed below the --- line.

> ---
>  include/uapi/linux/spi/spi.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
> index 9d5f58059703..ca56e477d161 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 */

Should tools/spi/spidev_test.c be changed to include this new
mosi-idle-low option?

Re: [PATCH v4 1/3] spi: add SPI_MOSI_IDLE_LOW mode bit

[Börge Strümpfel]

Thank you for your prompt feedback

Am Do., 18. Mai 2023 um 00:43 Uhr schrieb Fabio Estevam <festevam@gmail.com>:
>
> On Wed, May 17, 2023 at 7:30 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 <boerge.struempfel@gmail.com>
> >
> >
> > Link for versions:
> >   v1 and v2: https://lore.kernel.org/linux-spi/20230511135632.78344-1-bstruempfel@ultratronik.de/
> >   v3: https://lore.kernel.org/linux-spi/20230517103007.26287-1-boerge.struempfel@gmail.com/T/#t
> >
> > Changes from V3:
> >   - Added missing paranthesis which caused builderrors
> >
> > Changes from V2:
> >   - Removed the device-tree binding since this should not be managed by
> >     the DT but by the device itself.
> >   - Replaced all occurences of spi->chip_select with the corresponding
> >     macro spi_get_chipselect(spi,0)
> >
> > 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
>
> The change log should be placed below the --- line.
>

My bad. Thanks for letting me know. Just to clarify: I put the
changelog directly below
the first ---? And do I then put another --- between the changelog and
the following
include/uapi/linux/spi/spi.h | 3 ++- line?  or is there just a
new-line seperating them.

And if you don't mind my trivial questions, am I supposed to write a
cover letter for
the patch-stack? I seem to find contradictory answers to this question online.

> > ---
> >  include/uapi/linux/spi/spi.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/spi/spi.h b/include/uapi/linux/spi/spi.h
> > index 9d5f58059703..ca56e477d161 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 */
>
> Should tools/spi/spidev_test.c be changed to include this new
> mosi-idle-low option?

Until now I actually wasn't aware of this tool. However on first
glance, it seems
reasonable to add this mode bit. I can certainly add this mode bit to
the spidev_test
if desired.

While looking through the code, I noticed, that the latest two
additions to the spi->mode
(SPI_3WIRE_HIZ and SPI_RX_CPHA_FLIP) are also missing from this tool. Is this
by design, or should they then be included as well?

Re: [PATCH v4 1/3] spi: add SPI_MOSI_IDLE_LOW mode bit

[Fabio Estevam]

On Wed, May 17, 2023 at 8:20 PM Börge Strümpfel
<boerge.struempfel@gmail.com> wrote:

> My bad. Thanks for letting me know. Just to clarify: I put the
> changelog directly below
> the first ---? And do I then put another --- between the changelog and
> the following
> include/uapi/linux/spi/spi.h | 3 ++- line?  or is there just a
> new-line seperating them.

It should look like this:

Commit log line 1
Commit log line 2
...
Commit log line n

Signed-off-by: Your name <your@email.com>
---
Changes since v3:
- Bla bla bla

> And if you don't mind my trivial questions, am I supposed to write a
> cover letter for
> the patch-stack? I seem to find contradictory answers to this question online.

Yes, for a patch series having a cover letter is helpful.

> > Should tools/spi/spidev_test.c be changed to include this new
> > mosi-idle-low option?
>
> Until now I actually wasn't aware of this tool. However on first
> glance, it seems
> reasonable to add this mode bit. I can certainly add this mode bit to
> the spidev_test
> if desired.

Yes, that would be great.

> While looking through the code, I noticed, that the latest two
> additions to the spi->mode
> (SPI_3WIRE_HIZ and SPI_RX_CPHA_FLIP) are also missing from this tool. Is this
> by design, or should they then be included as well?

Looks like these two are missing and would be good to get them included as well.

Re: [PATCH v4 1/3] spi: add SPI_MOSI_IDLE_LOW mode bit

[Börge Strümpfel]

Am Do., 18. Mai 2023 um 01:53 Uhr schrieb Fabio Estevam <festevam@gmail.com>:
>
> On Wed, May 17, 2023 at 8:20 PM Börge Strümpfel
> <boerge.struempfel@gmail.com> wrote:
>
> > My bad. Thanks for letting me know. Just to clarify: I put the
> > changelog directly below
> > the first ---? And do I then put another --- between the changelog and
> > the following
> > include/uapi/linux/spi/spi.h | 3 ++- line?  or is there just a
> > new-line seperating them.
>
> It should look like this:
>
> Commit log line 1
> Commit log line 2
> ...
> Commit log line n
>
> Signed-off-by: Your name <your@email.com>
> ---
> Changes since v3:
> - Bla bla bla
>

Thank you for taking the time to explain this!

> > And if you don't mind my trivial questions, am I supposed to write a
> > cover letter for
> > the patch-stack? I seem to find contradictory answers to this question online.
>
> Yes, for a patch series having a cover letter is helpful.
>

I will add one for the next version.

> > > Should tools/spi/spidev_test.c be changed to include this new
> > > mosi-idle-low option?
> >
> > Until now I actually wasn't aware of this tool. However on first
> > glance, it seems
> > reasonable to add this mode bit. I can certainly add this mode bit to
> > the spidev_test
> > if desired.
>
> Yes, that would be great.
>

Okay. I have begun to implement this. During this, I noticed, that if
I called the new option
"--mosi-idle-low", the alignment of the help-lines (and in the c code
itself) would break.
Should I therefore shorten the option name by using an abbreviation
like "--mil", which is
probably not very helpful as a "full option name", or should I touch
all the other lines and
insert necessary spaces, such that they are aligned once more? (And if
so, should I do
this in a seperate patch, preparing the addition of the new options?)

> > While looking through the code, I noticed, that the latest two
> > additions to the spi->mode
> > (SPI_3WIRE_HIZ and SPI_RX_CPHA_FLIP) are also missing from this tool. Is this
> > by design, or should they then be included as well?
>
> Looks like these two are missing and would be good to get them included as well.

Okay. Should this be a separate patch, or should I add the support for
all 3 mode bits in
one commit?

Re: [PATCH v4 1/3] spi: add SPI_MOSI_IDLE_LOW mode bit

[Andy Shevchenko]

On Thu, May 18, 2023 at 3:27 AM Börge Strümpfel
<boerge.struempfel@gmail.com> wrote:
> Am Do., 18. Mai 2023 um 01:53 Uhr schrieb Fabio Estevam <festevam@gmail.com>:

...


> Okay. I have begun to implement this. During this, I noticed, that if
> I called the new option
> "--mosi-idle-low", the alignment of the help-lines (and in the c code
> itself) would break.
> Should I therefore shorten the option name by using an abbreviation
> like "--mil", which is
> probably not very helpful as a "full option name", or should I touch
> all the other lines and
> insert necessary spaces, such that they are aligned once more? (And if
> so, should I do
> this in a seperate patch, preparing the addition of the new options?)

It's a user space tool where not so strict rules of commit splitting
apply (as far as I know), I would go with indention fixes in the same
patch that adds the option.

...

> > > While looking through the code, I noticed, that the latest two
> > > additions to the spi->mode
> > > (SPI_3WIRE_HIZ and SPI_RX_CPHA_FLIP) are also missing from this tool. Is this
> > > by design, or should they then be included as well?
> >
> > Looks like these two are missing and would be good to get them included as well.
>
> Okay. Should this be a separate patch, or should I add the support for
> all 3 mode bits in
> one commit?

Split them logically. Are they from the same group of bits? No? then split.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 1/3] spi: add SPI_MOSI_IDLE_LOW mode bit

[Börge Strümpfel]

Thank you for you response

Am Do., 18. Mai 2023 um 12:58 Uhr schrieb Andy Shevchenko
<andy.shevchenko@gmail.com>:
>
> On Thu, May 18, 2023 at 3:27 AM Börge Strümpfel
> <boerge.struempfel@gmail.com> wrote:
> > Am Do., 18. Mai 2023 um 01:53 Uhr schrieb Fabio Estevam <festevam@gmail.com>:
>
> ...
>
>
> > Okay. I have begun to implement this. During this, I noticed, that if
> > I called the new option
> > "--mosi-idle-low", the alignment of the help-lines (and in the c code
> > itself) would break.
> > Should I therefore shorten the option name by using an abbreviation
> > like "--mil", which is
> > probably not very helpful as a "full option name", or should I touch
> > all the other lines and
> > insert necessary spaces, such that they are aligned once more? (And if
> > so, should I do
> > this in a seperate patch, preparing the addition of the new options?)
>
> It's a user space tool where not so strict rules of commit splitting
> apply (as far as I know), I would go with indention fixes in the same
> patch that adds the option.
>

That's good to know. I will do that.

> ...
>
> > > > While looking through the code, I noticed, that the latest two
> > > > additions to the spi->mode
> > > > (SPI_3WIRE_HIZ and SPI_RX_CPHA_FLIP) are also missing from this tool. Is this
> > > > by design, or should they then be included as well?
> > >
> > > Looks like these two are missing and would be good to get them included as well.
> >
> > Okay. Should this be a separate patch, or should I add the support for
> > all 3 mode bits in
> > one commit?
>
> Split them logically. Are they from the same group of bits? No? then split.
>
Yes they are actually from the same group of bits. Therefore I'll add
them all in the same patch.
> --
> With Best Regards,
> Andy Shevchenko