linux-kernel - RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec 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:	Fri, 6 Nov 2015 11:11:38 +0000
From:	"Opensource [Adam Thomson]" <Adam.Thomson.Opensource@...semi.com>
To:	Mark Brown <broonie@...nel.org>,
	"Opensource [Adam Thomson]" <Adam.Thomson.Opensource@...semi.com>
CC:	Liam Girdwood <lgirdwood@...il.com>,
	Jaroslav Kysela <perex@...ex.cz>,
	Takashi Iwai <tiwai@...e.com>,
	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>,
	"alsa-devel@...a-project.org" <alsa-devel@...a-project.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"Support Opensource" <Support.Opensource@...semi.com>
Subject: RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver

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.