linux-kernel - RE: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Sep 2020 14:21:31 +0000
From:   "Viorel Suman (OSS)" <viorel.suman@....nxp.com>
To:     Nicolin Chen <nicoleotsuka@...il.com>,
        "Viorel Suman (OSS)" <viorel.suman@....nxp.com>
CC:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Jaroslav Kysela <perex@...ex.cz>,
        Takashi Iwai <tiwai@...e.com>, Timur Tabi <timur@...nel.org>,
        Xiubo Li <Xiubo.Lee@...il.com>,
        Fabio Estevam <festevam@...il.com>,
        Shengjiu Wang <shengjiu.wang@...il.com>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Matthias Schiffer <matthias.schiffer@...tq-group.com>,
        Cosmin-Gabriel Samoila <cosmin.samoila@....com>,
        "alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        dl-linux-imx <linux-imx@....com>,
        Viorel Suman <viorel.suman@...il.com>
Subject: RE: [PATCH 1/2] ASoC: fsl_xcvr: Add XCVR ASoC CPU DAI driver

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