From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753840AbbJSSIW (ORCPT ); Mon, 19 Oct 2015 14:08:22 -0400 Received: from mezzanine.sirena.org.uk ([106.187.55.193]:37608 "EHLO mezzanine.sirena.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751705AbbJSSIU (ORCPT ); Mon, 19 Oct 2015 14:08:20 -0400 Date: Mon, 19 Oct 2015 19:07:57 +0100 From: Mark Brown To: Damien Horsley Cc: alsa-devel@alsa-project.org, James Hartley , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Liam Girdwood , Jaroslav Kysela , Takashi Iwai , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Message-ID: <20151019180757.GJ32054@sirena.org.uk> References: <1444653637-14711-1-git-send-email-Damien.Horsley@imgtec.com> <1444653637-14711-7-git-send-email-Damien.Horsley@imgtec.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="07FIeBX8hApXX6Bi" Content-Disposition: inline In-Reply-To: <1444653637-14711-7-git-send-email-Damien.Horsley@imgtec.com> X-Cookie: HUGH BEAUMONT died in 1982!! User-Agent: Mutt/1.5.24 (2015-08-30) X-SA-Exim-Connect-IP: 94.175.94.161 X-SA-Exim-Mail-From: broonie@sirena.org.uk Subject: Re: [alsa-devel] [PATCH V2 06/10] ASoC: img: Add driver for parallel output controller 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 --07FIeBX8hApXX6Bi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Mon, Oct 12, 2015 at 01:40:33PM +0100, Damien Horsley wrote: > + spin_lock_irqsave(&prl->lock, flags); > + reg = img_prl_out_readl(prl, IMG_PRL_OUT_CTL); > + ucontrol->value.integer.value[0] = !!(reg & IMG_PRL_OUT_CTL_EDGE_MASK); > + spin_unlock_irqrestore(&prl->lock, flags); Do you need to lock a single register read? > +static struct snd_kcontrol_new img_prl_out_controls[] = { > + { > + .iface = SNDRV_CTL_ELEM_IFACE_PCM, > + .name = "Parallel Out Edge Falling", > + .info = img_prl_out_edge_info, > + .get = img_prl_out_get_edge, > + .put = img_prl_out_set_edge > + } > +}; If this is a boolean control (it looked like one) it should be called Switch but it's not clear to me what exactly is being controlled here or why it's something that should be exposed to userspace. > + switch (cmd) { > + case SNDRV_PCM_TRIGGER_START: > + case SNDRV_PCM_TRIGGER_RESUME: > + case SNDRV_PCM_TRIGGER_PAUSE_RELEASE: > + reg = img_prl_out_readl(prl, IMG_PRL_OUT_CTL); > + reg |= IMG_PRL_OUT_CTL_ME_MASK; > + img_prl_out_writel(prl, reg, IMG_PRL_OUT_CTL); > + prl->active = true; > + break; > + case SNDRV_PCM_TRIGGER_STOP: > + case SNDRV_PCM_TRIGGER_SUSPEND: > + case SNDRV_PCM_TRIGGER_PAUSE_PUSH: > + img_prl_out_reset(prl); > + prl->active = false; > + break; No need for locking on the reset (and doesn't this mean that the control state is going to be changed underneath userspace)? --07FIeBX8hApXX6Bi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQEcBAEBCAAGBQJWJTF8AAoJECTWi3JdVIfQ+k0H/1ZJ5Xzxm4Ay4fUY8Epnr0o8 t5ZYKMUQ15VUQCVFc3gCfd81eeHH4DX+WwWRFMmqyugWyqAq+aV/cjP3l2yb+QyA fyc/KB08oyghZ8xf64oUYGghjbIZb9NJIMM9W6AmwKcDzCUVSwXkCZwbcQgAzUyh UTSXMsGHERnC6POXAO51KqwcwFfR0ClPfK01XAdDazyVNDStUvkh1IvCEHpDj2iY YwBYeFKerm8DXamyrfxSqtHHGWzXTPKuROZWcJISrOWgaGveJjJ9hTMhuglI6azd A8gpk7NTk6W4smDxYbWKXTBffuc/I2qRIdyiCxi6Qs5scO/Lp4w2lcBiWviCN+o= =g+p1 -----END PGP SIGNATURE----- --07FIeBX8hApXX6Bi--