Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758160AbcC2WRV (ORCPT ); Tue, 29 Mar 2016 18:17:21 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:53428 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755753AbcC2WRS (ORCPT ); Tue, 29 Mar 2016 18:17:18 -0400 Date: Tue, 29 Mar 2016 15:16:34 -0700 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 , Simran Rai , 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: <20160329221634.GQ2350@sirena.org.uk> References: <1459277192-10942-1-git-send-email-simran.rai@broadcom.com> <1459277192-10942-3-git-send-email-simran.rai@broadcom.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="2fV3mxwFPWhsKHyX" Content-Disposition: inline In-Reply-To: <1459277192-10942-3-git-send-email-simran.rai@broadcom.com> X-Cookie: If anything can go wrong, it will. User-Agent: Mutt/1.5.24 (2015-08-30) X-SA-Exim-Connect-IP: 64.55.107.4 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [PATCH resend v5 2/3] ASoC: cygnus: Add Cygnus audio DAI 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 Content-Length: 3266 Lines: 103 --2fV3mxwFPWhsKHyX Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Tue, Mar 29, 2016 at 11:46:31AM -0700, Simran Rai wrote: A few issues here, a lot of them are stylistic though there's what look like a couple of small bugs here too. > +static int cygnus_ssp_set_clocks(struct cygnus_aio_port *aio, > + struct cygnus_audio *cygaud) > +{ > + u32 value, i = 0; > + u32 mask = 0xf; > + u32 sclk; > + bool found = false; > + const struct _ssp_clk_coeff *p_entry = NULL; > + > + if ((!aio->lrclk) || (!aio->bit_per_frame)) { > + dev_err(aio->cygaud->dev, "First set up port through hw_params()\n"); > + return -EINVAL; > + } This function is only ever called from one site in hw_prams(). What is this defending against? A check like this seems very worrying, if it ever goes off that seems to indicate either something is seriously wrong or we should be recording something then coming back and trying again later. > + for (i = 0; i < ARRAY_SIZE(ssp_clk_coeff); i++) { > + p_entry = &ssp_clk_coeff[i]; > + if ((p_entry->rate == aio->lrclk) && > + (p_entry->sclk_rate == aio->bit_per_frame) && > + (p_entry->mclk == aio->mclk)) { Why the strange indentation here? > + /* Set sclk rate */ > + if (aio->port_type == PORT_TDM) { switch statment here for extensibility. > + /* Configure channels as mono or stereo */ > + if (params_channels(params) == 1) { > + value = readl(aio->cygaud->audio + > + aio->regs.bf_sourcech_cfg); > + value |= BIT(BF_SRC_CFGX_SAMPLE_CH_MODE); > + value &= ~BIT(BF_SRC_CFGX_BUFFER_PAIR_ENABLE); > + writel(value, aio->cygaud->audio + > + aio->regs.bf_sourcech_cfg); > + } else { > + value = readl(aio->cygaud->audio + > + aio->regs.bf_sourcech_cfg); > + value &= ~BIT(BF_SRC_CFGX_SAMPLE_CH_MODE); > + writel(value, aio->cygaud->audio + > + aio->regs.bf_sourcech_cfg); > + } Either this should be a switch statement or the comment should say we support more than stereo. It's also not clear to me how BUFFER_PAIR_ENABLE gets set again if we go from mono to stereo. > + if (!aio->is_slave) { > + if (aio->clk_trace.cap_clk_en) > + clk_prepare_enable(aio->cygaud-> > + audio_clk[aio->pll_clk_num]); Should check the return value of clk_prepare_enable(). > + .playback = { > + .channels_min = 2, > + .channels_max = 2, > + .rates = CYGNUS_TDM_RATE | SNDRV_PCM_RATE_88200 | > + SNDRV_PCM_RATE_96000 | SNDRV_PCM_RATE_176400 | > + SNDRV_PCM_RATE_192000, > + .formats = SNDRV_PCM_FMTBIT_S16_LE | > + SNDRV_PCM_FMTBIT_S32_LE, According to hw_params() the driver also supports S8 and S24. > + if (port_type == PORT_TDM) { > + } else { /* SPDIF case */ switch statement... --2fV3mxwFPWhsKHyX Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJW+v69AAoJECTWi3JdVIfQVRIH/jJDUHK7urUUrT/sk1YVKWIq mFzwJZ7LabExZHTZEh8aHzYh58egvje4us+mqOku995xIXxSO5VuE0uPBHzmZ7JV gb6/bZaQ5lthP4CXYowhvUjxqDs1f9oX/QnWQWBTQaUI5JxwUB99dPgO1h6PEdtC o9PfF+kmVDGlZL5C2utqPhtTFakb/nJSq1yhpSg3LDDjrbgZtv/hByH+e9f/CK1O npKLRM/abepOhZh7MZkUQ4E3kI24LaDkX2ey0qSKkj/I2+qPBlezUYcOQqRTvUrx S3xmQ0/nbypofWPui7BIaVZ9QMWiYR4rYEbV+FNarVdLEHHzKR1tqKs0cNqTXtM= =SyWT -----END PGP SIGNATURE----- --2fV3mxwFPWhsKHyX--