On Wed, Apr 27, 2011 at 06:57:00AM +0800, Arjan van de Ven wrote:
> On 4/26/2011 12:19 AM, Lu Guanqun wrote:
> > drivers/staging/intel_sst/intelmid_v2_control.c | 19 +++++++++++++++----
> > 1 files changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/staging/intel_sst/intelmid_v2_control.c
> > b/drivers/staging/intel_sst/intelmid_v2_control.c
> > index 9f1b4a5..ad1b03e 100644
> > --- a/drivers/staging/intel_sst/intelmid_v2_control.c
> > +++ b/drivers/staging/intel_sst/intelmid_v2_control.c
> > @@ -217,6 +217,8 @@ static int nc_power_up_pb(unsigned int port)
> >
> > msleep(30);
> >
> > + snd_pmic_ops_nc.pb_on = 1;
>
> what is the locking model for this variable? which locks prevents this
> from going out of sync ?
Hi Arjan,
Giving it a second look, this variable is only used at three places:
1. in nc_power_up_pb() -> set to 1
2. in nc_power_down_pb() -> set to 0
3. in nc_set_selected_output_dev() -> read the value.
first, it's impossible to have conflicts between 1 and 2.
nc_power_up_pb() and nc_power_down_pb() will be serialized by upper
sound subsystem, if these two calls messed up, then we have a more
serious problem than this out-of-sync variable.
second, the possible conflict is read and write conflict. e.g. between 1
and 3, or between 2 and 3.
let's see what would happen if there's a possible conflict between 1 and
3. the pb_on variable is set to 1 in nc_power_up_pb, but when it reads
back, it's still the old value. but it doesn't matter, the power of
headphone is already setup in nc_power_up_pb() and if the output device
is speaker, it will later be powered on in nc_power_up_pb().
let's see what would happen if there's a possible conflict between 2 and
3. that means pb_on variable is ready to be set to 0, but it reads back
to value 1 in nc_set_selected_output_dev(). Then headphone might be
powered on while it souldn't. Then later, if no active stream,
nc_power_down() will be called and headphone will be powered off.
So, yes, there will be a period between nc_power_down_pb() and
nc_power_down() that could be wrong.
Please see the following patch:
Author: Lu Guanqun <guanqun...@intel.com>
Date: Wed Apr 27 22:57:42 2011 +0800
sst: add mutex lock to serialize three functions
add a mutex lock to serialize three functions:
nc_power_up_pb, nc_power_down_pb and nc_set_selected_output_dev.
Signed-off-by: Lu Guanqun <guanqun...@intel.com>
diff --git a/drivers/staging/intel_sst/intel_sst.h
b/drivers/staging/intel_sst/intel_sst.h
index 635cf58..4ad2829 100644
--- a/drivers/staging/intel_sst/intel_sst.h
+++ b/drivers/staging/intel_sst/intel_sst.h
@@ -84,6 +84,7 @@ struct snd_pmic_ops {
int num_channel;
int input_dev_id;
int mute_status;
+ struct mutex lock;
int pb_on, pbhs_on;
int cap_on;
int output_dev_id;
diff --git a/drivers/staging/intel_sst/intelmid_v2_control.c
b/drivers/staging/intel_sst/intelmid_v2_control.c
index 149a387..cd03f7c 100644
--- a/drivers/staging/intel_sst/intelmid_v2_control.c
+++ b/drivers/staging/intel_sst/intelmid_v2_control.c
@@ -131,6 +131,7 @@ static int nc_init_card(void)
snd_pmic_ops_nc.master_mute = UNMUTE;
snd_pmic_ops_nc.mute_status = UNMUTE;
sst_sc_reg_access(sc_access, PMIC_WRITE, 27);
+ mutex_init(&snd_pmic_ops_nc.lock);
pr_debug("sst: init complete!!\n");
return 0;
}
@@ -177,6 +178,7 @@ static int nc_power_up_pb(unsigned int port)
return retval;
if (port == 0xFF)
return 0;
+ mutex_lock(&snd_pmic_ops_nc.lock);
nc_enable_audiodac(MUTE);
msleep(30);
@@ -229,8 +231,9 @@ static int nc_power_up_pb(unsigned int port)
if (snd_pmic_ops_nc.output_dev_id == MONO_EARPIECE ||
snd_pmic_ops_nc.output_dev_id == INTERNAL_SPKR)
nc_set_amp_power(1);
- return nc_enable_audiodac(UNMUTE);
-
+ nc_enable_audiodac(UNMUTE);
+ mutex_unlock(&snd_pmic_ops_nc.lock);
+ return 0;
}
static int nc_power_up_cp(unsigned int port)
@@ -351,7 +354,7 @@ static int nc_power_down_pb(unsigned int device)
return retval;
pr_debug("sst: powering dn pb....\n");
-
+ mutex_lock(&snd_pmic_ops_nc.lock);
nc_enable_audiodac(MUTE);
@@ -380,9 +383,9 @@ static int nc_power_down_pb(unsigned int device)
snd_pmic_ops_nc.pb_on = 0;
- return nc_enable_audiodac(UNMUTE);
-
-
+ nc_enable_audiodac(UNMUTE);
+ mutex_unlock(&snd_pmic_ops_nc.lock);
+ return 0;
}
static int nc_power_down_cp(unsigned int device)
@@ -539,6 +542,7 @@ static int nc_set_selected_output_dev(u8 value)
if (retval)
return retval;
pr_debug("sst: nc set selected output:%d\n", value);
+ mutex_lock(&snd_pmic_ops_nc.lock);
switch (value) {
case STEREO_HEADPHONE:
if (snd_pmic_ops_nc.pb_on) {
@@ -556,8 +560,10 @@ static int nc_set_selected_output_dev(u8 value)
break;
default:
pr_err("sst: rcvd illegal request: %d\n", value);
+ mutex_unlock(&snd_pmic_ops_nc.lock);
return -EINVAL;
}
+ mutex_unlock(&snd_pmic_ops_nc.lock);
return retval;
}
With this patch, the power on/off of headphone and speaker in these
three functions will be serialized. And the conflict between read and
write will be eliminated.
How do you think the above analysis and the patch?
> case STEREO_HEADPHONE:
>
> > + if (snd_pmic_ops_nc.pb_on) {
> > + sst_sc_reg_access(sc_access_HP+2, PMIC_WRITE, 1);
> > + msleep(100);
>
> 100 milliseconds?????? that's an eternity. You're going to make the app
> that caused this code to be called visibly stutter with this.
> Why is this so long (magic number)? And is there some way to offload all
> of this to some work thread ?
>
I'll simply remove the time delay. How do you think of this patch?
Author: Lu Guanqun <guanqun...@intel.com>
Date: Wed Apr 27 10:29:01 2011 +0800
sst: remove time delay when select output
According to Arjan's advice, this time delay is removed.
Signed-off-by: Lu Guanqun <guanqun...@intel.com>
diff --git a/drivers/staging/intel_sst/intelmid_v2_control.c
b/drivers/staging/intel_sst/intelmid_v2_control.c
index 149a387..0953530 100644
--- a/drivers/staging/intel_sst/intelmid_v2_control.c
+++ b/drivers/staging/intel_sst/intelmid_v2_control.c
@@ -541,10 +541,8 @@ static int nc_set_selected_output_dev(u8 value)
pr_debug("sst: nc set selected output:%d\n", value);
switch (value) {
case STEREO_HEADPHONE:
- if (snd_pmic_ops_nc.pb_on) {
+ if (snd_pmic_ops_nc.pb_on)
sst_sc_reg_access(sc_access_HP+2, PMIC_WRITE, 1);
- msleep(100);
- }
retval = sst_sc_reg_access(sc_access_HP, PMIC_WRITE, 2);
nc_set_amp_power(0);
break;
Hi Arjan, if you have no problem of these two patches, I'll send these
two patches.
--
guanqun
_______________________________________________
MeeGo-kernel mailing list
MeeGo-kernel@lists.meego.com
http://lists.meego.com/listinfo/meego-kernel
