RE: [PATCH 2/2] ASoC: codecs: Add da7218 codec driver

[Opensource [Adam Thomson]]

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.