Allow MISO/MOSI set to NC during SPI initialisation (fix for issue #12435) #12460
Conversation
Static pinmap extension required to use pin_function() and pin_mode() functions instead of pinmap_pinout(). Unfortunatelly pin_function() does not allow passing NC pin. Call pin_function() and pin_mode() only if MISO/MOSI pin is not NC.
Because of this, we end-up with a lot of checks (if (pin != NC)) everywhere in the code, including in this PR. Shouldn't we actually rather change pin_function to simply return when NC pin is passed as a parameter ? Other than that this question, I am fine with the change as it is now - so I will approve it. |
ciarmcom
requested review from
fkjagodzinski,
jamesbeyond,
jeromecoutant,
LMESTM,
maciejbocianski,
maclobdell,
mmahadevan108 and
a team
|
@mprse, thank you for your changes. |
|
Can this update be extended to include K66F and KW41D? |
I agree this is good. This should be changed in the future if there is no objection to change |
|
Quick test done: Seems that none of MOSI/MISO are set to NC ? |
Good point. I used |
Seems fairly straightforward for ST targets, we would need to replace the ASSERT by a simple return. |
OK, seems it is only print issue as test is doing 2 sub cases: 1 with only MOSI pin, 1 with MISO... Maybe a new type "SPINoCs1Line" could be good instead of SPINoCSPort ? |
|
With the extra prints it will look like this: Is that acceptable? |
Additionally, remove redundant pin checks against NC when above functions are used.
mprse
force-pushed
the
spi_init_nc_fix
branch
from
741107d to
713be4f
Compare
@LMESTM please review 713be4f. @mmahadevan108 Is such change acceptable also for NXP targets? Test results (NUCLEO_F429ZI): |
|
ping @mmahadevan108 |
| } else { | ||
| } else if (mosi == NC) { | ||
| spi_per = (SPIName)pinmap_merge(spi_miso, spi_sclk); | ||
| } |
|
Started CI meanwhile we complete reviews |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
0xc0170
added
needs: review
release-version: 6.0.0-alpha-3
and removed
ready for merge
labels
|
CI completed, just one approval. Please complete your reviews |
|
Marked as ready for merge, would like to integrate later today |
Summary of changes
This PR fixes issue #12435.
Static pin-map extension required to use
pin_function()andpin_mode()functions instead ofpinmap_pinout(). Unfortunatelypin_function()does not allow passingNCpin.The proposition is to call
pin_function()andpin_mode()only ifMISO/MOSIpin is notNC.Additionally extended SPI FPGA test to verify all possible pin configurations including MISO and MOSI unconnected.
The idea is to add also similar tests for other peripherals if this proposal will be approved. I think that this should be added for peripherals that use more than one pin and provide the availability to specify at least one pin as NC.
Impact of changes
Migration actions required
Documentation
Pull request type
Test results
Reviewers
@jeromecoutant @LMESTM @mmahadevan108
@fkjagodzinski @maciejbocianski
@jamesbeyond