Hi Nicolin, Thank you for your review. > > +static const u32 fsl_xcvr_earc_channels[] = { 1, 2, 8, 16, 32, }; /* > > +one bit 6, 12 ? */ > > What's the meaning of the comments? Just a thought noted as comment. HDMI2.1 spec defines 6- and 12-channels layout when one bit audio stream is transmitted - I was wandering how can this be enforced. Is a @todo like of comment. > > > +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? Yes, will fix it in v2. > > > +/** 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. No particular reason to do it with no sleep or timeout here, will check and fix it in v2. > > > > +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? Yes, thank you, will fix in v2. > > > + 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? Yes, will fix in v2. > > > + 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 Ok. > > > + 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? Make sense, will fix in v2. > > > + } > > + > > + 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? No particular reason, will fix it in v2. Thank you, Viorel