linux-kernel - Re: [PATCH v3 2/4] ASoC: rockchip: Add rockchip SPDIF transceiver 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, 7 Aug 2015 14:03:11 +0100
From:	Mark Brown <broonie@...nel.org>
To:	Sjoerd Simons <sjoerd.simons@...labora.co.uk>
Cc:	linux-rockchip@...ts.infradead.org,
	Jaroslav Kysela <perex@...ex.cz>, devicetree@...r.kernel.org,
	alsa-devel@...a-project.org, Heiko Stuebner <heiko@...ech.de>,
	linux-kernel@...r.kernel.org, Kumar Gala <galak@...eaurora.org>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Takashi Iwai <tiwai@...e.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Pawel Moll <pawel.moll@....com>,
	Rob Herring <robh+dt@...nel.org>,
	Mark Rutland <mark.rutland@....com>,
	Russell King <linux@....linux.org.uk>,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v3 2/4] ASoC: rockchip: Add rockchip SPDIF transceiver
 driver

On Fri, Aug 07, 2015 at 01:44:12PM +0200, Sjoerd Simons wrote:

> Changes in v2:
> - Remove platform: module alias as it was unused

Ugh, I see Paul Bolle replied to one of your earlier patches :/  Please
just ignore him, he doesn't understand that we don't require machine
definitions to be in tree.  A platform: module alias is useful for any
platform device.

> +static inline struct rk_spdif_dev *to_info(struct snd_soc_dai *dai)
> +{
> +	return snd_soc_dai_get_drvdata(dai);
> +}

I'm not sure this is adding much to the two users but whatever.

> +static int rk_spdif_snd_txctrl(struct rk_spdif_dev *spdif, int on)
> +{
> +	int ret;
> +
> +	if (on) {
> +		ret = regmap_update_bits(spdif->regmap, SPDIF_DMACR,
> +				   SPDIF_DMACR_TDE_ENABLE,
> +				   SPDIF_DMACR_TDE_ENABLE);
> +
> +		if (ret != 0)
> +			goto bail;
> +
> +		ret = regmap_update_bits(spdif->regmap, SPDIF_XFER,
> +				   SPDIF_XFER_TXS_START,
> +				   SPDIF_XFER_TXS_START);

This function has no common code, just two branches in an if/else that
share nothing and is called from a single location.  Why not just inline
this into the trigger function?

> +	/* Set clock and calculate divider */
> +	clk_set_rate(spdif->mclk, mclk);

We should check the return value of clk_set_rate().

> +	spdif->hclk = devm_clk_get(&pdev->dev, "spdif_hclk");
> +	if (IS_ERR(spdif->hclk)) {
> +		dev_err(&pdev->dev, "Can't retrieve rk_spdif bus clock\n");
> +		return PTR_ERR(spdif->hclk);
> +	}
> +	ret = clk_prepare_enable(spdif->hclk);
> +	if (ret) {
> +		dev_err(spdif->dev, "hclock enable failed %d\n", ret);
> +		return ret;
> +	}

I notice that we don't manage the hclk over suspend/resume - should we
perhaps be using regmap_init_mmio_clk() together with runtime PM to
manage it?

> +	pm_runtime_enable(&pdev->dev);
> +	if (!pm_runtime_enabled(&pdev->dev)) {
> +		ret = rk_spdif_runtime_resume(&pdev->dev);
> +		if (ret)
> +			goto err_pm_disable;
> +	}

The normal pattern with this is to start the device powered up then idle
it - this does mean we need to bounce the power on but fitting in with
the common pattern is good and it's less conditional code.

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)