-
-
Notifications
You must be signed in to change notification settings - Fork 92
fix for https://github.com/arduino/ArduinoCore-renesas/issues/12 #31
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
Conversation
CLA assistant check
All committers have signed the CLA.
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 👍
Please cleanup the comments and possible consider replacing ONE_BIT, TWO_BITS with relevant constants from the FSP layer.
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.
Is this comment // reset bit 0 true? It looks to me like its copy and paste from the baud rate generator clock divider configuration.
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.
I'm not really happy with the generic ONE_BIT, TWO__BITS defines. Maybe consider using the FSP layer provided constants, i.e.
-mask = (ONE_BIT << SCI_CKPOL_POS); +mask = (SPI_CLK_POLARITY_HIGH << SCI_CKPOL_POS);
? This one's a matter of taste though.
Superseded by #13
I think this should be reopened since it fixes additional problems I didn't catch in my PR: clearing the mode and bit order bits so you can revert back if talking to multiple devices, and applies the same fixes to configSpiSci.
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.
I wonder if it would be clearer to just modify the register in-place to make use of Renesas' struct definitions, so all of the masking and bit shifting would be handled implicitly. Like so:
auto& spcmd0 = _spi_ctrl.p_regs->SPCMD_b[0]; // note SPCMD_b not SPCMD
spcmd0.CPHA = clk_phase;
spcmd0.CPOL = clk_polarity;
spcmd0.LSBF = bit_order;
spcmd0.BRDV = spck_div.brdv;
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.
See definition here
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.
Likewise, maybe this could be:
auto& spmr = _spi_sci_ctrl.p_reg->SPMR_b;
spmr.CKPH = clk_phase;
spmr.CKPOL = clk_polarity;
auto& scmr = _spi_sci_ctrl.p_reg->SCMR_b;
scmr.SDIR = bit_order;
auto& smr = _spi_sci_ctrl.p_reg->SMR_b;
smr.CKS = clk_div.cks;
Superseded by #45 .
Tone: fix infinite duration
Tone: fix infinite duration Former-commit-id: edbcbd3
fix for #12