From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1161225AbbKFLL4 (ORCPT ); Fri, 6 Nov 2015 06:11:56 -0500 Received: from mail1.bemta5.messagelabs.com ([195.245.231.140]:7254 "EHLO mail1.bemta5.messagelabs.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1161193AbbKFLLr (ORCPT ); Fri, 6 Nov 2015 06:11:47 -0500 X-Env-Sender: Adam.Thomson.Opensource@diasemi.com X-Msg-Ref: server-14.tower-178.messagelabs.com!1446808299!6104010!1 X-Originating-IP: [94.185.165.51] X-StarScan-Received: X-StarScan-Version: 7.19.2; banners=-,-,- X-VirusChecked: Checked From: "Opensource [Adam Thomson]" To: Mark Brown , "Opensource [Adam Thomson]" CC: Liam Girdwood , Jaroslav Kysela , Takashi Iwai , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "alsa-devel@alsa-project.org" , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "Support Opensource" Subject: RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver Thread-Topic: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver Thread-Index: AQHRF7bMwuwKa2yx+U2JlonnXkDOk56NjZEAgAFKp+A= Date: Fri, 6 Nov 2015 11:11:38 +0000 Message-ID: <2E89032DDAA8B9408CB92943514A0337D460CA0C@SW-EX-MBX01.diasemi.com> References: <01a9b6e96b66327cf9a27781c13aa50545380acb.1446718490.git.Adam.Thomson.Opensource@diasemi.com> <20151105152736.GO18409@sirena.org.uk> In-Reply-To: <20151105152736.GO18409@sirena.org.uk> Accept-Language: en-GB, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.20.26.15] Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id tA6BC4WN027313 On November 5, 2015 15:28, Mark Brown wrote: > > +/* ALC */ > > +static void da7218_alc_calib(struct snd_soc_codec *codec) > > +{ > > + struct da7218_priv *da7218 = snd_soc_codec_get_drvdata(codec); > > + u8 calib_ctrl; > > + int i = 0; > > + bool calibrated = false; > > + > > + /* Bypass cache so it saves current settings */ > > + regcache_cache_bypass(da7218->regmap, true); > > What ensures that nothing else is running at the same time this is? Is a fair point. Originally I was saving the state of registers then re-instating them at the end, which worked fine, but then was trying to be clever and tidy things up by bypassing the cache instead. Will revert back to the previous method. > > > +static int da7218_mic_lvl_det_sw_put(struct snd_kcontrol *kcontrol, > > + struct snd_ctl_elem_value *ucontrol) > > +{ > > Why is this a user visible control? I can envisage in a system you may want to choose which capture channels can trigger level detection (if any), and this may change depending on the use-case at the time, so having it as a control makes sense to me. > > > + /* Default all mixers off */ > > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_1L, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_1R, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_2L, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_2R, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTFILT_1L, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTFILT_1R, 0); > > + snd_soc_write(codec, DA7218_DROUTING_ST_OUTFILT_1L, 0); > > + snd_soc_write(codec, DA7218_DROUTING_ST_OUTFILT_1R, 0); > > We generally just use the device defaults, why change them? I figured it made more sense to have the device start with audio routes disabled but I can remove this as it's really not essential. {.n++%ݶw{.n+{G{ayʇڙ,jfhz_(階ݢj"mG?&~iOzv^m ?I From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Opensource [Adam Thomson]" Subject: Re: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver Date: Fri, 6 Nov 2015 11:11:38 +0000 Message-ID: <2E89032DDAA8B9408CB92943514A0337D460CA0C@SW-EX-MBX01.diasemi.com> References: <01a9b6e96b66327cf9a27781c13aa50545380acb.1446718490.git.Adam.Thomson.Opensource@diasemi.com> <20151105152736.GO18409@sirena.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20151105152736.GO18409@sirena.org.uk> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Mark Brown , "Opensource [Adam Thomson]" Cc: Mark Rutland , "devicetree@vger.kernel.org" , "alsa-devel@alsa-project.org" , Support Opensource , Pawel Moll , Ian Campbell , "linux-kernel@vger.kernel.org" , Takashi Iwai , Liam Girdwood , Rob Herring , Kumar Gala List-Id: devicetree@vger.kernel.org On November 5, 2015 15:28, Mark Brown wrote: > > +/* ALC */ > > +static void da7218_alc_calib(struct snd_soc_codec *codec) > > +{ > > + struct da7218_priv *da7218 = snd_soc_codec_get_drvdata(codec); > > + u8 calib_ctrl; > > + int i = 0; > > + bool calibrated = false; > > + > > + /* Bypass cache so it saves current settings */ > > + regcache_cache_bypass(da7218->regmap, true); > > What ensures that nothing else is running at the same time this is? Is a fair point. Originally I was saving the state of registers then re-instating them at the end, which worked fine, but then was trying to be clever and tidy things up by bypassing the cache instead. Will revert back to the previous method. > > > +static int da7218_mic_lvl_det_sw_put(struct snd_kcontrol *kcontrol, > > + struct snd_ctl_elem_value *ucontrol) > > +{ > > Why is this a user visible control? I can envisage in a system you may want to choose which capture channels can trigger level detection (if any), and this may change depending on the use-case at the time, so having it as a control makes sense to me. > > > + /* Default all mixers off */ > > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_1L, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_1R, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_2L, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTDAI_2R, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTFILT_1L, 0); > > + snd_soc_write(codec, DA7218_DROUTING_OUTFILT_1R, 0); > > + snd_soc_write(codec, DA7218_DROUTING_ST_OUTFILT_1L, 0); > > + snd_soc_write(codec, DA7218_DROUTING_ST_OUTFILT_1R, 0); > > We generally just use the device defaults, why change them? I figured it made more sense to have the device start with audio routes disabled but I can remove this as it's really not essential.