Re: [linux-sunxi][alsa-devel][PATCH 3/3] ASOC: sunxi: Add support for the spdif block

[Mark Brown <broonie@kernel.org>]

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

On Thu, Sep 24, 2015 at 04:30:05PM +0200, codekipper@gmail.com wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> The sun4i, sun6i and sun7i SoC families have an SPDIF
> block which is capable of playback and capture.

I'm not seeing patches 1 or 2 - what's the story here, are there
dependencies?  Please use subject lines matching the style for the
subsystem and also don't fill your subject lines with noisy tags beyond
"[PATCH n/x]", when I look at this in my mail client what I see is:

->  432   C 09/24 codekipper@gmai ( 27K) [linux-sunxi][alsa-devel][PATCH 3/3] AS

>  sound/soc/sunxi/Kconfig               |  10 +
>  sound/soc/sunxi/Makefile              |   4 +
>  sound/soc/sunxi/sunxi-machine-spdif.c | 110 +++++
>  sound/soc/sunxi/sunxi-spdif.c         | 801 ++++++++++++++++++++++++++++++++++

The machine driver and controller driver should be submitted as separate
patches for ease of review.  Is there a strong reason for not using
simple-card?

> +void sunxi_snd_txctrl(struct snd_pcm_substream *substream,
> +					struct sunxi_spdif_dev *host, int on)
> +{
> +	u32 tmp;

There's no meaningful sharing between the enable and disable paths and
only one place either is called, it's better to just inline this into
the callers.

> +	if (!cpu_dai->active) {
> +		ret = clk_prepare_enable(host->clk);
> +		if (ret)
> +			return ret;
> +	}

Can you move the clock enables to runtime PM and let the core do runtime
PM for you?

> +static int sunxi_spdif_set_clkdiv(struct snd_soc_dai *cpu_dai,
> +						unsigned int rate, int div)
> +{
> +	struct sunxi_spdif_dev *host = snd_soc_dai_get_drvdata(cpu_dai);
> +	int sample_freq, original_sample_freq;

Why are you implementing a set_clkdiv() operation - is the driver not
capable of working out its internal clocking automaticallly?

> +static int sunxi_spdif_hw_params(struct snd_pcm_substream *substream,
> +					struct snd_pcm_hw_params *params,
> +					struct snd_soc_dai *cpu_dai)
> +{

> +	ret = snd_soc_dai_set_fmt(cpu_dai, fmt);
> +	if (ret < 0)
> +		return ret;

This looks very broken - what is this doing and why?

> +static struct snd_soc_dai_driver sunxi_spdif_dai = {
> +	.playback = {
> +		.channels_min = 2,
> +		.channels_max = 2,
> +		.rates = SUNXI_RATES,

There was code in the driver to handle mono signals but this says only
stereo is supported?

> +	if (clk_prepare_enable(host->apb_clk)) {
> +		dev_err(&pdev->dev, "try to enable apb_spdif_clk failed\n");
> +		return -EINVAL;
> +	}

Don't ignore the error code you got from the API, print it and pass it
back.

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