On 2023/6/7 19:44, Mark Brown wrote:
On Wed, Jun 07, 2023 at 04:14:39PM +0800, Walker Chen wrote:
Some minor issues were found during addtional testing and static analysis. The patch fixed these minor issues. 1.Use BIT() macro to indicate configuration for TDM registers.
2.Fix the check for devm_reset_control_array_get_exclusive return value. The devm_reset_control_array_get_exclusive() function may return NULL if it's an optional request. If optional is intended then NULL should not be treated as an error case, but as a special kind of success case. So here the IS_ERR() is used to check better.
As covered in submitting-patches.rst please submit one patch per change rather than combining multiple changes into a single patch, it makes things much easier to review and handle.
Hi Mark,
Thanks for your review. OK, I will submit a single patch for each change in the next version.
- datarx = (tdm->rx.ifl << IFL_BIT) |
(tdm->rx.wl << WL_BIT) |(tdm->rx.sscale << SSCALE_BIT) |(tdm->rx.sl << SL_BIT) |(tdm->rx.lrj << LRJ_BIT);
- datarx = (tdm->rxwl << 8) |
(tdm->rxsscale << 4) |(tdm->rxsl << 2) |TDM_PCMRXCR_LEFT_J;I'm not sure this change to use numbers here is a win - the _BIT definitions look fine (I might've called them _SHIFT but whatever).
This is Claudiu's advice. Using the macro BIT() to replace these definition of *_BIT, it will result in big changes in the code. Please refer to previous comments: https://lore.kernel.org/all/143e2fa2-e85d-8036-4f74-ca250c026c1b@microchip.c...
@Claudiu What do think about this ?
-static const struct of_device_id jh7110_tdm_of_match[] = { +static const struct of_device_id jh7110_tdm_match[] = { { .compatible = "starfive,jh7110-tdm", }, {} };
-MODULE_DEVICE_TABLE(of, jh7110_tdm_of_match); +MODULE_DEVICE_TABLE(of, jh7110_tdm_match);
This rename wasn't mentioned in the changelog.
Will be added in the change log.
Best regards, Walker