[alsa-devel] [PATCH v2 2/2] ASoC: add support for Conexant CX2072X CODEC

[Simon Ho]

On Wed, Apr 12, 2017 at 09:50:17AM +0200, Takashi Iwai wrote:
> On Wed, 12 Apr 2017 09:41:01 +0200,
> Takashi Iwai wrote:
> > 
> > On Wed, 12 Apr 2017 07:45:20 +0200,
> > Takashi Iwai wrote:
> > > 
> > > On Tue, 11 Apr 2017 15:31:37 +0200,
> > > Takashi Iwai wrote:
> > > > 
> > > > On Wed, 05 Apr 2017 11:07:14 +0200,
> > > > <simon.ho.cnxt at gmail.com> wrote:
> > > > > 
> > > > > --- /dev/null
> > > > > +++ b/sound/soc/codecs/cx2072x.c
> > > > > +/**
> > > > > + * cx2072x_enable_detect - Enable CX2072X jack detection
> > > > > + * @codec : pointer variable to codec having information related to codec
> > > > > + *
> > > > > + */
> > > > > +int cx2072x_enable_detect(struct snd_soc_codec *codec)
> > > > > +{
> > > > .....
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(cx2072x_enable_detect);
> > > > > +
> > > > > +/*
> > > > > + * cx2072x_get_jack_state: Return current jack state.
> > > > > + * @codec : pointer variable to codec having information related to codec
> > > > > + *
> > > > > + */
> > > > > +int cx2072x_get_jack_state(struct snd_soc_codec *codec)
> > > > > +{
> > > > ....
> > > > > +}
> > > > > +EXPORT_SYMBOL_GPL(cx2072x_get_jack_state);
> > > > > --- /dev/null
> > > > > +++ b/sound/soc/codecs/cx2072x.h
> > > > ....
> > > > > +enum cx2072x_jack_types {
> > > > > +	CX_JACK_NONE = 0x0000,
> > > > > +	CX_JACK_HEADPHONE = 0x0001,
> > > > > +	CX_JACK_APPLE_HEADSET = 0x0002,
> > > > > +	CX_JACK_NOKIE_HEADSET = 0x0003,
> > > > > +};
> > > > > +
> > > > > +int cx2072x_hs_jack_report(struct snd_soc_codec *codec);
> > > > 
> > > > This function is nowhere defined.  And the new jack functions
> > > > cx2072x_enable_detect() and cx2072x_get_jack_state() are not declared
> > > > here, either.
> > > 
> > > Also, some DAPM entries are missing / wrong, obviously.
> > 
> > ... and yet another missing ones, the powers for some pins aren't
> > hooked.
> 
> Last but not least: the regcache_cache_bypass() calls in the jack
> detection function don't look good.
> 
> These registers are volatile, and if the cache_only isn't set via bias
> ctl, we need no bypass.  And, if cache_only is at BIAS_OFF and the
> jack detection gets triggered during that, it screws up, spewing a
> kernel warning because of the check below:
> 
> void regcache_cache_bypass(struct regmap *map, bool enable)
> {
> 	map->lock(map->lock_arg);
> 	WARN_ON(map->cache_only && enable);
> 
> 
> So, either we should remove these bypass calls, or turn on/off
> cache_only temporarily during the jack detection like below -- the
> latter is so ugly, and better to avoid, though.
> 
> 
> Takashi
> 

Thank you for the detailed and informative review. I will try both
appraoches.

Thanks,

Simon
> ===
> diff --git a/sound/soc/codecs/cx2072x.c b/sound/soc/codecs/cx2072x.c
> index fab564cb1176..5b3c4939e653 100644
> --- a/sound/soc/codecs/cx2072x.c
> +++ b/sound/soc/codecs/cx2072x.c
> @@ -1246,13 +1246,17 @@ int cx2072x_get_jack_state(struct snd_soc_codec *codec)
>  	unsigned int type = 0;
>  	int  state = 0;
>  	struct cx2072x_priv *cx2072x = snd_soc_codec_get_drvdata(codec);
> +	bool need_cache_bypass =
> +		snd_soc_codec_get_bias_level(codec) == SND_SOC_BIAS_OFF;
>  
> -	regcache_cache_bypass(cx2072x->regmap, true);
> +	if (need_cache_bypass)
> +		regcache_cache_only(cx2072x->regmap, false);
>  	cx2072x->jack_state = CX_JACK_NONE;
>  	regmap_read(cx2072x->regmap, CX2072X_PORTA_PIN_SENSE, &jack);
>  	jack = jack >> 24;
>  	regmap_read(cx2072x->regmap, CX2072X_DIGITAL_TEST11, &type);
> -	regcache_cache_bypass(cx2072x->regmap, false);
> +	if (need_cache_bypass)
> +		regcache_cache_only(cx2072x->regmap, true);
>  	if (jack == 0x80) {
>  		type = type >> 8;