Chibios SPI driver: allow some SPI pins to be left unassigned

[purdeaandrei]

Description

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[purdeaandrei]

I don't understand why lint is complaining, what it suggests makes no sense to me. I didn't even touch those lines.

[sigprof]

I had some different ideas in mind when suggesting to add the NO_PIN support to the SPI driver; the suggested changes are mostly about that (except one block that might fix the formatting issues).

[sigprof]

Can we just rely on the default zero initialization for static variables here?

static SPIConfig spiConfig;

(GitHub does not allow adding a proper suggestion for this code block.)

[purdeaandrei]

DONE

[purdeaandrei]

@sigprof I applied all your suggestions and rerun with my keyboard and things work.

[elpekenin]

Im no big expert but i would get rid of #if SPI_SELECT_MODE == SPI_SELECT_MODE_NONE checks around writePin{Low/High}.

• Setting the pin low/high manually, even if ChibiOS also does it internally, shouldn't be a real performance difference and code is slightly easier to read (imo)
• Removing those checks (and maybe some others in the file too) -or moving them to runtime- is probably a good idea for multi-bus SPI support and/or dynamic configuration of the bus(es) if such things ever get added.

PS: This addition is great, thx. It should be used by #20258 if it gets somewhere. Since CS could be a "virtual pin" (instead of completely unused), ChibiOS wouldn't be able to control it, so we would want:

• spiSelectI and spiUnselectI (macros that control CS) to become no-ops (using SPI_SELECT_MODE_NONE)
• writePin{Low/High} to control te "pin" (no matter what it actually is) ourselves and ensure it has the correct value

[purdeaandrei]

@elpekenin I don't think ChibiOS currently supports configuring some SPI buses with SPI_SELECT_MODE_NONE and some others with SPI_SELECT_MODE_PAD, correct me if I'm wrong. So not sure what advantage it would have to move those checks to runtime. However if this changes in the future then absolutely yes.

I don't think the current PR implementation would prevent it from working with 20258.

[elpekenin]

not sure what advantage it would have

Indeed, it isnt an option currently, as spiSelectI gets expanded to different things based on the selected mode. That's why i said runtime would allow such thing, we could have switch (bus_config.select_mode) to use the different things that the macro expands to... However im not sure how different those are from each other, maybe the addition isnt very useful

I don't think the current PR implementation would prevent it from working with 20258.

It wouldnt, in fact it helps (in case someone tries to use virtual pins for MISO/MOSI, we can ignore those as it makes no sense... and virtual CS could be used thanks to the manual control of the pin over letting ChibiOS handle that)

[purdeaandrei]

Indeed, it isnt an option currently, as spiSelectI gets expanded to different things based on the selected mode. That's why i said runtime would allow such thing, we could have switch (bus_config.select_mode) to use the different things that the macro expands to... However im not sure how different those are from each other, maybe the addition isnt very useful

I like the idea of having this all be runtime selected if that becomes an option in the future in Chibios upstream. But i guess I don't see the point in writing code for it now, since we don't even know if it's ever gonna happen, and we don't know what it's gonna look like. If it becomes possible we can always switch to runtime later as a response to the upstream change.

And just going from #if SPI_SELECT_MODE == SPI_SELECT_MODE_NONE to if (SPI_SELECT_MODE == SPI_SELECT_MODE_NONE) doesn't make sense to me either, since pretty much everywhere in chibios code the checks are done at the preprocessor level. And some of these decisions cannot even be made at runtime, because depending on SPI_SELECT_MODE, the spiConfig structure gets different fields, so the following decision cannot be made runtime:

#if SPI_SELECT_MODE == SPI_SELECT_MODE_PAD
    spiConfig.ssport = PAL_PORT(slavePin);
    spiConfig.sspad  = PAL_PAD(slavePin);

[sigprof]

The ChibiOS code looks like it should work.

One potential issue is that the AVR SPI driver might eventually need a similar change to make it accept NO_PIN, but I'm not sure about any actual users for that feature.

[drashna]

This has a merge conflict that needs resolving.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[github-actions]

Thank you for your contribution!
This pull request has been automatically closed because it has not had activity in the last 30 days. Please feel free to give a status update now, ping for review, or re-open when it's ready.
// [stale-action-closed]