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 <[email protected]>
Link for versions:
v1 and v2: https://lore.kernel.org/linux-spi/[email protected]/
v3: https://lore.kernel.org/linux-spi/[email protected]/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
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 <[email protected]>
---
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
On Wed, May 17, 2023 at 7:30 PM Boerge Struempfel
<[email protected]> 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 <[email protected]>
>
>
> Link for versions:
> v1 and v2: https://lore.kernel.org/linux-spi/[email protected]/
> v3: https://lore.kernel.org/linux-spi/[email protected]/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?
Thank you for your prompt feedback
Am Do., 18. Mai 2023 um 00:43 Uhr schrieb Fabio Estevam <[email protected]>:
>
> On Wed, May 17, 2023 at 7:30 PM Boerge Struempfel
> <[email protected]> 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 <[email protected]>
> >
> >
> > Link for versions:
> > v1 and v2: https://lore.kernel.org/linux-spi/[email protected]/
> > v3: https://lore.kernel.org/linux-spi/[email protected]/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?
On Wed, May 17, 2023 at 8:20 PM Börge Strümpfel
<[email protected]> 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 <[email protected]>
---
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.
Am Do., 18. Mai 2023 um 01:53 Uhr schrieb Fabio Estevam <[email protected]>:
>
> On Wed, May 17, 2023 at 8:20 PM Börge Strümpfel
> <[email protected]> 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 <[email protected]>
> ---
> 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?
On Thu, May 18, 2023 at 3:27 AM Börge Strümpfel
<[email protected]> wrote:
> Am Do., 18. Mai 2023 um 01:53 Uhr schrieb Fabio Estevam <[email protected]>:
...
> 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
Thank you for you response
Am Do., 18. Mai 2023 um 12:58 Uhr schrieb Andy Shevchenko
<[email protected]>:
>
> On Thu, May 18, 2023 at 3:27 AM Börge Strümpfel
> <[email protected]> wrote:
> > Am Do., 18. Mai 2023 um 01:53 Uhr schrieb Fabio Estevam <[email protected]>:
>
> ...
>
>
> > 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