New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TARGET_STM: Add DEVICE_SPI_COUNT to use SPIs without interference #10841
Conversation
Extend to all STM targets the work done on PR10752. Signed-off-by: Vincent Veron <vincent.veron@st.com>
@VVESTM, thank you for your changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Content's fine, but I'm doubtful it counts as a "fix".
Possibly it could be "target update".
This will notably change behaviour and memory footprint of apps, so a bit wary about going to a patch release either way, but I wouldn't totally rule it out.
I guess it fixes #10827, sort of, but he could fix his issue without this by driving the API correctly, as I described there. This is more a workaround so it doesn't matter that he sets his GPIO before selecting the SPI.
@VVESTM, thank you for your changes. |
@kjbracey-arm, I have updated the PR to "target update". I agree with you and I get some doubts to choose between "fix" or "target update"... |
@kjbracey-arm I'm a bit worried now :-/ |
Oh, certainly it should be deployed for all, but the act of deploying it for any particular target does impact application behaviour on that target beyond a simple fix. It's a functional improvement, with a corresponding memory increase to implement that improvement. So it tends to fall closer to the "this should be deployed in the next minor release" than "this should be issued as a 5.13 patch". But there is also the counter-argument that "the other targets work nicely like this, this one doesn't, so that inconsistency/deficiency is a bug in this target". I've no strong feeling either way - I'm willing to leave the 5.13.y backport choice to other maintainers, and ST's own to decide. But this is good for master either way.
I phrased my comment above a bit badly. This patch isn't a workaround itself, but deploying it makes that reported issue go away - it's not the ultimate fix for his issue. The code in the issue is flawed, but works when his SPI devices aren't state sharing. This patch means that flaw stops being apparent, because we detect the separate buses and stop state sharing. His code would fail again if he put his devices on the same bus. |
Actually there is a remaining glitch for #10827 even with this patch - He needs to modify his code as I suggested there - either use |
@kjbracey-arm Thanks - all clear now ! |
@kjbracey-arm reading the last few comments I'm confused as to whether this is ready to go or not ? |
This is fine to go for master, the debate is just about whether it's applicable for a 5.13 patch release (consensus = no), and how it relates to that other issue. |
CI restarted |
Test run: SUCCESSSummary: 11 of 11 test jobs passed |
Description
Extend to all STM targets the work done on #10752
Half works around #10827
Pull request type
Reviewers
@embeddedteam103
@kjbracey-arm
@LMESTM