| Message ID | 1600247876-8013-1-git-send-email-viorel.suman@oss.nxp.com |
|---|---|
| Headers | show |
| Series | DAI driver for new XCVR IP | expand |
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
Hi Mark, Thank you for your review. > On Wed, Sep 16, 2020 at 12:17:55PM +0300, Viorel Suman (OSS) wrote: > > +static int fsl_xcvr_load_firmware(struct fsl_xcvr *xcvr) { > > + struct device *dev = &xcvr->pdev->dev; > > + const struct firmware *fw; > > + int ret = 0, rem, off, out, page = 0, size = FSL_XCVR_REG_OFFSET; > > + u32 mask, val; > > + > > + ret = request_firmware(&fw, xcvr->fw_name, dev); > > + if (ret) { > > + dev_err(dev, "failed to request firmware.\n"); > > + return ret; > > + } > > + > > + rem = fw->size; > > It would be good to see some explicit validation of the image size, at least > printing an error message if the image is bigger than can be loaded. The code > should be safe in that it won't overflow the device region it's writing to but it > feels like it'd be better to tell people if we spot a problem rather than just silently > truncating the file. Make sense, will improve this part in the next version. > > +static irqreturn_t irq0_isr(int irq, void *devid) { > > + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid; > > + struct device *dev = &xcvr->pdev->dev; > > + struct regmap *regmap = xcvr->regmap; > > + void __iomem *reg_ctrl, *reg_buff; > > + u32 isr, val, i; > > + > > + regmap_read(regmap, FSL_XCVR_EXT_ISR, &isr); > > + regmap_write(regmap, FSL_XCVR_EXT_ISR_CLR, isr); > > This will unconditionally clear any interrupts, even those we don't understand - it > might be better to only clear bits that are supported so the IRQ core can > complain if there's something unexpected showing up. The ARM core registers itself in "fsl_xcvr_prepare" (the code below) just for a subset of all supported interrupts: ===== ret = regmap_update_bits(xcvr->regmap, FSL_XCVR_EXT_IER0, FSL_XCVR_IRQ_EARC_ALL, FSL_XCVR_IRQ_EARC_ALL); ===== FSL_XCVR_IRQ_EARC_ALL - this mask represents all the interrupts we are interested in and we handle in interrupt handler, But this is just a subset of all interrupts the M0+ core is able to assert. Not very intuitive, I think I need to reword it somehow. > > + if (isr & FSL_XCVR_IRQ_FIFO_UOFL_ERR) > > + dev_dbg(dev, "RX/TX FIFO full/empty\n"); > > Should this be dev_err()? The interrupt may be asserted right before DMA starts to fill the TX FIFO if I recall correctly. I've added it just to debug the IP behavior, will check and change it to err it in next version if it is the case. > > +static irqreturn_t irq1_isr(int irq, void *devid) { > > + struct fsl_xcvr *xcvr = (struct fsl_xcvr *)devid; > > + struct device *dev = &xcvr->pdev->dev; > > + > > + dev_dbg(dev, "irq[1]: %d\n", irq); > > + > > + return IRQ_HANDLED; > > +} > > Is there any value in even requesting this and irq2 given the lack of meaningful > handling? No, will remove it in v2. Thank you, Viorel
On 9/18/20 9:21 AM, Viorel Suman (OSS) wrote: >>> +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. Please don't add comments that other developers could never understand. The text that you just wrote here would be a great starting point for a better comment.
From: Viorel Suman <viorel.suman@nxp.com> DAI driver for new XCVR IP found in i.MX8MP. Viorel Suman (2): ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver ASoC: dt-bindings: fsl_xcvr: Add document for XCVR .../devicetree/bindings/sound/fsl,xcvr.yaml | 104 ++ sound/soc/fsl/Kconfig | 10 + sound/soc/fsl/Makefile | 2 + sound/soc/fsl/fsl_xcvr.c | 1352 ++++++++++++++++++++ sound/soc/fsl/fsl_xcvr.h | 266 ++++ 5 files changed, 1734 insertions(+) create mode 100644 Documentation/devicetree/bindings/sound/fsl,xcvr.yaml create mode 100644 sound/soc/fsl/fsl_xcvr.c create mode 100644 sound/soc/fsl/fsl_xcvr.h