From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030238AbbKDO3I (ORCPT ); Wed, 4 Nov 2015 09:29:08 -0500 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:47237 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755885AbbKDO3F (ORCPT ); Wed, 4 Nov 2015 09:29:05 -0500 Date: Wed, 4 Nov 2015 14:28:32 +0000 From: Mark Brown To: Simran Rai Cc: Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Ray Jui , Scott Branden , Liam Girdwood , Jaroslav Kysela , Takashi Iwai , Lori Hikichi , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, bcm-kernel-feedback-list@broadcom.com, linux-kernel@vger.kernel.org, Arun Parameswaran , alsa-devel@alsa-project.org Message-ID: <20151104142832.GF1717@sirena.org.uk> References: <1446502284-23907-1-git-send-email-ssimran@broadcom.com> <1446502284-23907-3-git-send-email-ssimran@broadcom.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="ChQOR20MqfxkMJg9" Content-Disposition: inline In-Reply-To: <1446502284-23907-3-git-send-email-ssimran@broadcom.com> X-Cookie: He who hesitates is sometimes saved. User-Agent: Mutt/1.5.24 (2015-08-30) X-SA-Exim-Connect-IP: 92.40.248.4 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH v2 2/2] sound: soc: Add Cygnus audio driver X-SA-Exim-Version: 4.2.1 (built Mon, 26 Dec 2011 16:24:06 +0000) X-SA-Exim-Scanned: Yes (on mezzanine.sirena.org.uk) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --ChQOR20MqfxkMJg9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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? --ChQOR20MqfxkMJg9 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWOhYQAAoJECTWi3JdVIfQjQYH/0YBwb+pyFyY1yZLhXs+2wi9 vLqNXkBeAxs6zbhoTz3S8wxJnL/ZszQ8wfiJ68gBGIQAM6mUuyMiCcfQvcLCIEfi ke/2UKSjjYI6+9rysgz8JTv8SsyBzLLdQ404+ETmyDI0DKozaxN93VBcW9xo5E6k O6AXLl6AJvKcszFDFdB2Bdck7zrX4Tcbi/L1IXUHiSdOpMe8E/fZ+S0UE3IQwRiL /8rVnB3+v3AS00EYSdxHhD/Ooi7vT8tQWWtQL5Eiw31dFTjaR9jx5se5Q6h1ybkb VumL9WIV5W5RYOPYjoGSTIMEyHyFtfHv7OAq1SEB8QtdxxXiIaYXEp68NKwHayk= =wu88 -----END PGP SIGNATURE----- --ChQOR20MqfxkMJg9--