| Message ID | 20230517223007.178432-1-boerge.struempfel@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4,1/3] spi: add SPI_MOSI_IDLE_LOW mode bit | expand |
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?
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?
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.
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?
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.
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
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 */
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(-)