Re: [PATCH 4/4] ASoC: mid-x86 add jack detect support

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Wed, Jan 19, 2011 at 06:18:16PM +0530, Harsha Priya wrote:

>  sound/soc/codecs/sn95031.c             |   82 +++++++++++++-
>  sound/soc/codecs/sn95031.h             |   20 ++++
>  sound/soc/codecs/sn95031_adc_control.h |  194 ++++++++++++++++++++++++++++++++
>  sound/soc/mid-x86/mfld_machine.c       |  159 +++++++++++++++++++++-----

This should be split into two: one patch adding the feature in the
CODEC, the other adding the use of the feature in the machine driver.

> +struct snd_soc_codec *sn95031_codec;
> +

Why do we need a global variable?

>  	/* all end points mic, hs etc */
> +	SND_SOC_DAPM_HP("HeadPhones", NULL),

Again this is a machine driver widget.

> +int sn95031_get_headset_state(void)
> +{
> +	unsigned int mic_bias = sn95031_get_mic_bias(sn95031_codec);
> +
> +	if (mic_bias >= SN_HP_START && mic_bias < SN_HP_END) {
> +		pr_debug("sst: Detected Headphone!!!\n");
> +		snd_soc_update_bits(sn95031_codec, AUDIO_GPIO_CTRL,
> +					BIT(1), BIT(0));
> +	} else if (mic_bias >= SN_AM_HS_START && mic_bias < SN_AM_HS_END) {
> +		pr_debug("sst: Detected American headset\n");
> +		snd_soc_update_bits(sn95031_codec, AUDIO_GPIO_CTRL,
> +					BIT(1), BIT(1));
> +	} else if (mic_bias >= SN_HS_START && mic_bias < SN_HS_END) {
> +		pr_debug("sst: Detected Headset!!!\n");
> +		snd_soc_update_bits(sn95031_codec, AUDIO_GPIO_CTRL,
> +					BIT(1), BIT(0));
> +		enable_jack_btn(sn95031_codec);
> +		return SND_JACK_HEADSET;
> +	} else
> +		pr_debug("sst: Detected Open Cable!!!\n");

What exactly is this doing?  It looks like it's measuring the voltage on
micbias and using that to identify the accessory connected?  That's
normally a machine specific thing - different manufacturers have
different ideas about what they want to do with this.  Some will do
things like have multiple buttons on the jack with different resistances
connected between them and measure the voltage to determine which button
has been pressed, take a look at the Nexus S kernel for one example.

Looking at all this fairly briefly it looks like all the CODEC is doing
is providing an ADC and possibly some signals fed directly into the CPU.
The detection then mostly belongs externally to the CODEC driver - the
CPU then generates interrupts and the machine driver goes off and does
some ADC readings using the facility provided by the CODEC.

Since this is a common pattern we should ideally have some utility code
which can take any table of ADC values in conjunction with any ADC and
use those to do detection.

> +static int sn95031_initialize_adc(struct snd_soc_codec *sn95031_codec)
> +{
> +	int base_addr, chnl_addr;
> +	int value;
> +	static int channel_index;

The functions added in this header look like they'd be better placed in
the source file?