[alsa-devel] [PATCH 1/2] ASoC: rt5645: Adds push button support for rt5650

[Bard Liao]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at kernel.org]
> Sent: Tuesday, April 21, 2015 6:48 PM
> To: Bard Liao
> Cc: lgirdwood at gmail.com; alsa-devel at alsa-project.org; lars at metafoo.de;
> Flove; Oder Chiou; John Lin; koro.chen at mediatek.com;
> yang.a.fang at intel.com; zhengxing at rock-chips.com
> Subject: Re: [PATCH 1/2] ASoC: rt5645: Adds push button support for
> rt5650
> 
> On Tue, Apr 21, 2015 at 02:08:18AM +0000, Bard Liao wrote:
> 
> > > > -	{ 0xdd, 0x001b },
> 
> > > That's a bit weird, why is this in here?
> 
> > The default value of register 0xdd is different between rt5645 and
> rt5650.
> > And it is for button detection configuration. That's why I put it in
> > this patch instead of a separate patch.
> 
> OK, you probaly want to move to separate default tables per device.
> This is something that should have been called out in the commit log.

Do you mean we need to write it down in the commit log and send
another patch to use separate default tables for different device?
In fact, there are about 10 different registers which default value
are different between rt5645 and rt5650. We are planning to send
a separate patch for that after this patch is applied.

Regarding the different default tables, do you mean something like:
	case RT5645_DEVICE_ID:
		rt5645->codec_type = CODEC_TYPE_RT5645;
		dev_info(&i2c->dev,	"Codec type is: RT5645\n");
+		rt5645_regmap. reg_defaults = rt5645_reg;
		break;
	case RT5650_DEVICE_ID:
		rt5645->codec_type = CODEC_TYPE_RT5650;
		dev_info(&i2c->dev,	"Codec type is: RT5650\n");
+		rt5645_regmap. reg_defaults = rt5650_reg;
		break;

> 
> > > > +			switch (btn_type) {
> > > > +			case 0x8000:
> > > > +			case 0x4000:
> > > > +			case 0x2000:
> > > > +				report |= SND_JACK_BTN_0;
> > > > +				break;
> 
> > > Why do we mash all these values together?
> 
> > rt5650 can report the action of button (one click, double click or hold).
> > That is basically for Windows OS. For linux, we only care about
> > whether the button is pressed or released. So all of the three actions
> > are treated as button pressed.
> 
> OK, can we have a comment for that please?  We might want to extend
> this in the future as the userspace ABI grows richer (and depending on
> how things get reported we may need to synthesize at least a double press
> for userspace).

Do you mean comment in the source code?

> 
> ------Please consider the environment before printing this e-mail.