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

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

[Mark Brown]

[-- 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 --]

Re: [PATCH 1/2] ASoC: zx: Add ZTE zx296702 pcm support

[Mark Brown]

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

On Thu, Apr 30, 2015 at 12:07:29PM +0800, Jun Nie wrote:

> +static const struct snd_pcm_hardware snd_zx_hardware = {
> +	.info			= SNDRV_PCM_INFO_MMAP |
> +				  SNDRV_PCM_INFO_MMAP_VALID |
> +				  SNDRV_PCM_INFO_INTERLEAVED |
> +				  SNDRV_PCM_INFO_PAUSE |
> +				  SNDRV_PCM_INFO_RESUME |
> +				  SNDRV_PCM_INFO_NO_PERIOD_WAKEUP,
> +	.period_bytes_min	= 32,
> +	.period_bytes_max	= 16 * 1024,
> +	.periods_min		= 2,
> +	.periods_max		= 32,
> +	.buffer_bytes_max	= 60 * 1024,
> +};

Can the generic DMA code not work this out by querying the dmaengine
driver?

> +int zx_pcm_platform_register(struct device *dev)
> +{
> +	return devm_snd_dmaengine_pcm_register(dev, &zx_dmaengine_pcm_config,
> +			SND_DMAENGINE_PCM_FLAG_CUSTOM_CHANNEL_NAME |
> +			SND_DMAENGINE_PCM_FLAG_NO_RESIDUE);

_NO_RESIDUE has been removed in current code, the core figures it out
automatically.  Like I said looking at the driver code I'm not clear why
this is specifying _CUSTOM_CHANNEL_NAME either, it looked like the
driver just used the one standard channel name.

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

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

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

[Mark Brown]

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

On Mon, May 04, 2015 at 09:13:48PM +0800, Jun Nie wrote:
> 2015-05-01 18:55 GMT+08:00 Mark Brown <broonie@kernel.org>:

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

> Plan to add a separate patch to device tree. Or you prefer to add
> device tree patch in this patch set and loop device tree maintainer?

You *always* need to document new device tree bindings at the same time
as adding new code.

> There are more than one SPDIF controller on ZTE SOC. One of them
> support HDMI output, while the other SPDIF controller support direct
> output. So I want to separate SPDIF and HDMI and add SPDIF id 0 test

So this special HDMI S/PDIF controller is directly integrated with the
HDMI IP in the SoC and not usable as a generic S/PDIF controller?  That
seems to mirror system designs which have an external HDMI encoder which
use S/PDIF or I2S to connect the audio portion to an external device.

> Do you have any suggestion on what driver to look at for the interface
> between audio and video?

Half the problem right now is that the people working on HDMI don't
appear to be talking to each other very much so everything is very
inconsistent with no real code sharing.  I don't really feel I've got a
good enough picture of what the hardware looks like in general to be
100% clear on what the best way is forward beyond the fact that we
should be sharing the EDID parsing code.

I'd suggest for future versions reorganizing things so that you add the
S/PDIF driver with only S/PDIF support and then have a separate patch
that layers on the HDMI support.

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

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

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

[Mark Brown]

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

On Tue, May 05, 2015 at 09:43:56AM +0800, Jun Nie wrote:
> 2015-05-04 21:54 GMT+08:00 Mark Brown <broonie@kernel.org>:

> > So this special HDMI S/PDIF controller is directly integrated with the
> > HDMI IP in the SoC and not usable as a generic S/PDIF controller?  That
> > seems to mirror system designs which have an external HDMI encoder which
> > use S/PDIF or I2S to connect the audio portion to an external device.

> The first SPDIF controller on ZTE SOC is dedicated for HDMI and the
> 2nd one is for external output only via physical pad. The two
> controller IPs are identical, also independent with each other and
> HDMI in register address region.

OK, so this does sound like the software should like one of the systems
with an external HDMI encoder and the HDMI handled separately rather
than in this driver anyway.

> > I'd suggest for future versions reorganizing things so that you add the
> > S/PDIF driver with only S/PDIF support and then have a separate patch
> > that layers on the HDMI support.

> Reasonable suggestion. Then I can have more time to think about HDMI
> part. More comments is welcome if any.

This sounds like the software should look like a TDA998x (which is
having patches posted still) so just have the S/PDIF controllers as pure
S/PDIF controllers and then a separate driver for the HDMI part that
it's hooked up to which deals with the audio configuration for the HDMI
side.

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

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