Re: [PATCH v3 2/4] ASoC: rockchip: Add rockchip SPDIF transceiver driver

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2125 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]