Hi Viorel,
It looks pretty clean to me, though some small comments inline.
On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote:
From: Viorel Suman viorel.suman@nxp.com
XCVR (Audio Transceiver) is a on-chip functional module found on i.MX8MP. It support HDMI2.1 eARC, HDMI1.4 ARC and SPDIF.
Signed-off-by: Viorel Suman viorel.suman@nxp.com
+static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /* one bit 6, 12 ? */
What's the meaning of the comments?
+static const int fsl_xcvr_phy_arc_cfg[] = {
- FSL_XCVR_PHY_CTRL_ARC_MODE_SE_EN, FSL_XCVR_PHY_CTRL_ARC_MODE_CM_EN,
+};
Nit: better be u32 vs. int?
+/** phy: true => phy, false => pll */ +static int fsl_xcvr_ai_write(struct fsl_xcvr *xcvr, u8 reg, u32 data, bool phy) +{
- u32 val, idx, tidx;
- idx = BIT(phy ? 26 : 24);
- tidx = BIT(phy ? 27 : 25);
- regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_CLR, 0xFF);
- regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_SET, reg);
- regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_WDATA, data);
- regmap_write(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL_TOG, idx);
- do {
regmap_read(xcvr->regmap, FSL_XCVR_PHY_AI_CTRL, &val);- } while ((val & idx) != ((val & tidx) >> 1));
Might regmap_read_poll_timeout() be better? And it seems to poll intentionally with no sleep nor timeout -- would be nice to have a line of comments to explain why.
+static int fsl_xcvr_runtime_resume(struct device *dev)
+{
- struct fsl_xcvr *xcvr = dev_get_drvdata(dev);
- int ret;
- ret = clk_prepare_enable(xcvr->ipg_clk);
- if (ret) {
dev_err(dev, "failed to start IPG clock.\n");return ret;- }
- ret = clk_prepare_enable(xcvr->pll_ipg_clk);
- if (ret) {
dev_err(dev, "failed to start PLL IPG clock.\n");
Should it disable ipg_clk?
return ret;- }
- ret = clk_prepare_enable(xcvr->phy_clk);
- if (ret) {
dev_err(dev, "failed to start PHY clock: %d\n", ret);clk_disable_unprepare(xcvr->ipg_clk);
Should it disable pll_ipg_clk?
return ret;- }
- ret = clk_prepare_enable(xcvr->spba_clk);
- if (ret) {
dev_err(dev, "failed to start SPBA clock.\n");clk_disable_unprepare(xcvr->phy_clk);clk_disable_unprepare(xcvr->ipg_clk);
Ditto
return ret;- }
- regcache_cache_only(xcvr->regmap, false);
- regcache_mark_dirty(xcvr->regmap);
- ret = regcache_sync(xcvr->regmap);
- if (ret) {
dev_err(dev, "failed to sync regcache.\n");return ret;
What about those clocks? Probably better to have some error-out labels at the end of the function?
- }
- reset_control_assert(xcvr->reset);
- reset_control_deassert(xcvr->reset);
- ret = fsl_xcvr_load_firmware(xcvr);
- if (ret) {
dev_err(dev, "failed to load firmware.\n");return ret;
Ditto
- }
- /* Release M0+ reset */
- ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_CTRL,
FSL_XCVR_EXT_CTRL_CORE_RESET, 0);- if (ret < 0) {
dev_err(dev, "M0+ core release failed: %d\n", ret);return ret;
Ditto
- }
- mdelay(50);
Any reason to use mdelay over msleep for a 50ms wait? May add a line of comments if mdelay is a must?