Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1202672

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

Show all headers | View raw

[Multipart message — attachments visible in raw view] - view raw

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.

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v3 2/4] ASoC: rockchip: Add rockchip SPDIF transceiver driver Sjoerd Simons <sjoerd.simons@collabora.co.uk> - 2015-08-07 13:50 +0200
  Re: [PATCH v3 2/4] ASoC: rockchip: Add rockchip SPDIF transceiver  driver Mark Brown <broonie@kernel.org> - 2015-08-07 15:10 +0200
  Re: [PATCH v3 2/4] ASoC: rockchip: Add rockchip SPDIF transceiver driver Heiko Stuebner <heiko@sntech.de> - 2015-08-18 20:30 +0200
    Re: [PATCH v3 2/4] ASoC: rockchip: Add rockchip SPDIF transceiver  driver Sjoerd Simons <sjoerd.simons@collabora.co.uk> - 2015-08-19 09:00 +0200
      Re: [PATCH v3 2/4] ASoC: rockchip: Add rockchip SPDIF transceiver driver Heiko Stuebner <heiko@sntech.de> - 2015-08-19 09:30 +0200

csiph-web