Re: [PATCH 2/2] ASoC: zx: Add zx296702 SPDIF support

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 3585 bytes --]

On Thu, Apr 30, 2015 at 12:07:30PM +0800, Jun Nie wrote:
> Add zx296702 SPDIF support

This is adding a new device with DT bindings but there is no bindings
document.  All new DT bindings must be documented.

> --- /dev/null
> +++ b/include/sound/zx_hdmi_audio.h
> @@ -0,0 +1,7 @@
> +#ifndef __ZX_HDMI_AUDIO_H__
> +#define __ZX_HDMI_AUDIO_H__
> +
> +int zx_hdmi_audio_cfg(int audio_codec, int audio_way,
> +		      u32 sample_rate, u32 sample_len);
> +void zx_hdmi_audio_en(int on);
> +#endif /* __ZX_HDMI_AUDIO_H__ */

So this isn't a S/PDIF controller but rather a HDMI controller and from
the looks of it one with yet another custom interface between the video
part and the audio part.  As I keep saying I'd really like to see more
code sharing between the various controllers doing this.  Please take a
look at some of the other controllers and see what can be shared, and
also at this:

 http://thread.gmane.org/gmane.comp.video.dri.devel/126588

series from Russell King.

> +config ZX296702_SPDIF
> +        bool "ZX296702 spdif"
> +	select ZX_PCM_DMA
> +        help
> +	  Say Y or M if you want to add support for codecs attached to the
> +	  zx296702 spdif interface

The indentation above looks inconsistent - probably a mix of tabs and
spaces.

> +static struct snd_dmaengine_dai_dma_data zx_spdif_pcm_stereo_out = {
> +	.addr_width	= DMA_SLAVE_BUSWIDTH_4_BYTES,
> +	.chan_name	= "tx",
> +	.maxburst	= 8,
> +};

The DMA driver said it needed custom channel names but this looks like
the standard "tx" for transmit?

> +static int zx_spdif_dai_probe(struct snd_soc_dai *dai)
> +{
> +	struct zx_spdif_info *zx_spdif = dev_get_drvdata(dai->dev);
> +
> +	snd_soc_dai_set_drvdata(dai, zx_spdif);
> +	zx_spdif_pcm_stereo_out.addr = zx_spdif->mapbase + ZX_DATA;
> +	snd_soc_dai_init_dma_data(dai, &zx_spdif_pcm_stereo_out, NULL);
> +	return 0;
> +}

This appears to be modifying a global variable rather than some driver
data, please don't do that - store anything discovered per device at
runtime in the driver datta.

> +static void zx_spdif_chanstats(void __iomem *base, unsigned int rate)
> +{
> +	u32 cstas1;
> +
> +	switch (rate) {
> +	case 22050:

> +	default:
> +		return;
> +	}

This should handle and report errrors, not ignore them.

> +		return -EINVAL;
> +	}
> +	if (2 == params_channels(params))
> +		val |= ZX_CTRL_DOUBLE_TRACK;
> +	else
> +		val |= ZX_CTRL_LEFT_TRACK;
> +	writel_relaxed(val, zx_spdif->reg_base + ZX_CTRL);

Missing blank line between this and the above block and please reverse
the arguments for the == - it's the more normal kernel coding style.

> +#if defined(CONFIG_ZX_HDMI_SND_SPDIF)
> +	sample_len = params_physical_width(params);
> +	zx_hdmi_audio_cfg(1, 0, params_rate(params), sample_len);
> +#endif

But this is the S/PDIF dirver...  in any case, this looks like something
that depends on the hardware rather than something that should be a
compile time option.  We should be able to boot the same kernel on many
systems.

> +	val = readl_relaxed(base + ZX_CTRL);
> +	val &= ~(ZX_CTRL_ENB_MASK | ZX_CTRL_TX_MASK);
> +	val |= on ? (ZX_CTRL_TX_OPEN | ZX_CTRL_ENB)
> +		: (ZX_CTRL_TX_CLOSE | ZX_CTRL_DNB);
> +	writel_relaxed(val, base + ZX_CTRL);

Please don't use the ternery operator like this, it does nothing for
legibility.

> +	if (clk_prepare_enable(zx_spdif->dai_clk))
> +		return -EIO;

Don't discard return values, pass them back.  You should probably also
be managing this clock at runtime to save power.

> +					 &zx_spdif_dai, 1);
> +	if (ret) {
> +		dev_err(&pdev->dev, "Register DAI failed\n");

Print error codes as well.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]