linux-kernel - Re: [PATCH v2 2/2] sound: soc: Add Cygnus audio driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 4 Nov 2015 14:28:32 +0000
From:	Mark Brown <broonie@...nel.org>
To:	Simran Rai <ssimran@...adcom.com>
Cc:	Rob Herring <robh+dt@...nel.org>, Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>, Ray Jui <rjui@...adcom.com>,
	Scott Branden <sbranden@...adcom.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	Lori Hikichi <lhikichi@...adcom.com>,
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	bcm-kernel-feedback-list@...adcom.com,
	linux-kernel@...r.kernel.org,
	Arun Parameswaran <arunp@...adcom.com>,
	alsa-devel@...a-project.org
Subject: Re: [PATCH v2 2/2] sound: soc: Add Cygnus audio driver

On Mon, Nov 02, 2015 at 02:11:24PM -0800, Simran Rai wrote:

>  sound/soc/bcm/Kconfig      |   18 +
>  sound/soc/bcm/Makefile     |    5 +
>  sound/soc/bcm/cygnus-pcm.c |  903 ++++++++++++++++++++++++++
>  sound/soc/bcm/cygnus-ssp.c | 1532 ++++++++++++++++++++++++++++++++++++++++++++
>  sound/soc/bcm/cygnus-ssp.h |  129 ++++

This is a very big patch which contains at least two drivers (a DMA
driver and a DAI driver).  Please split it into at least per-driver
patches for ease of review.

> +config SND_SOC_CYGNUS_DIAG
> +	bool "SoC platform audio for Broadcom Cygnus chips diagnostics"
> +	depends on SND_SOC_CYGNUS
> +	help
> +	  Say Y if you want to add diagnostics support in ASoC audio
> +	  on Broadcom Cygnus chips (bcm958300, bcm958305, bcm911360)
> +
> +	  If you don't know what to do here, say N.

These look like extremely specific diagnostics that I'd have expected to
be mostly doable using the standard kernel trace infrastructure which is
very low overhead and can just be left in the kernel all the time.  Why
is this a configurable option?

> +/*
> + * Enable diagnostics through menuconfig to debug the time intervals
> + * when each playback interrupt happens.
> + */

This should've been in the Kconfig help text.

> +	is_play = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK);

Why is_play?  It's only looked at once and makes things a bit more
confusing.

> +	/* If playback interrupt happened */
> +	if (ANY_PLAYBACK_IRQ & r5_status)
> +		handle_playback_irq(cygaud);
> +
> +	/* If  capture interrupt happened */
> +	if (ANY_CAPTURE_IRQ & r5_status)
> +		handle_capture_irq(cygaud);
> +
> +	/*
> +	 * clear r5 interrupts after servicing them
> +	 */
> +	writel(r5_status, cygaud->audio + INTH_R5F_CLEAR_OFFSET);

This will ack interrupts we didn't handle, it'd be better to mask out
unhandled interrupts.

> +	if (aio->port_type == PORT_TDM) {

> +	} else if (aio->port_type == PORT_SPDIF) {

> +	} else {
> +		dev_err(aio->cygaud->dev, "Port not supported\n");
> +		return -EINVAL;
> +	}

This looks like it should be a switch statement, you've got some other
similar constructs in the code.

> +	error = configure_vco(cygaud, p_entry);
> +	if (error)
> +		return error;

We appear to have multiple things calling configure_vco() but I can't
see what's ensuring that they all agree with each other about the
settings.

> +	/* Slot Width is either 16 or 32 */
> +	if (slot_width <= 16)
> +		bits_per_slot = 1;

The check doesnn't match the comment here.

> +}
> +static int cygnus_ssp_resume(struct snd_soc_dai *cpu_dai)

Blank line between functions and remove empty functions.  Though I'm not
clear why the result doesn't undo what the suspend did...

> +		ssp_regs[0] = (struct cygnus_ssp_regs) INIT_SSP_REGS(0);

Why the casts?

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)