On Tue, Sep 16, 2014 at 12:30:53PM -0700, Mark Brown wrote:
On Tue, Sep 09, 2014 at 03:11:28PM +0530, Subhransu S. Prusty wrote:
- mutex_lock(&drv->lock);
- map = is_tx ? sst_ssp_channel_map : sst_ssp_slot_map;
- val = 1 << ctl_no;
- mux = ucontrol->value.enumerated.item[0];
- if (mux > e->max - 1) {
mutex_unlock(&drv->lock);return -EINVAL;- }
This is still in parameter validation so we don't need to have the lock yet at all.
Yes will move it.
- /*first clear all registers of this bit*/
- for (i = 0; i < e->max; i++)
map[i] &= ~val;- if (mux == 0) /*kctl set to 'none'*/
return 0;This is returning with the lock still held AFAICT. I'm a bit surprised that we don't need to interact with the hardware if we've disabled everything, shouldn't this have some effect on the hardware?
Also the coding style thing with the comments again.
Will fix the locking and comment.
Regarding interaction with the driver, the slot map is cached and sent in sst_set_be_modules event. This is sent only when that particular BE is active, otherwise driver will happily cache these values. This is the reason why we don't see trigger to DSP when usermode fiddles around.
In our model only when a particular FE/BE/Mixer/Pipe is active we forward the settings and parameters, rest we keep the values in driver and forward when DAPM enables them.
I think we can add this explanation here at top of this file to help.
+static void sst_find_and_send_pipe_algo(struct sst_data *drv,
const char *pipe, struct sst_ids *ids)+{
- struct sst_algo_control *bc;
- struct sst_module *algo = NULL;
- dev_dbg(&drv->pdev->dev, "Enter: widget=%s\n", pipe);
- list_for_each_entry(algo, &ids->algo_list, node) {
bc = (void *)algo->kctl->private_value;dev_dbg(&drv->pdev->dev, "Found algo control name=%s pipe=%s\n",algo->kctl->id.name, pipe);sst_send_algo_cmd(drv, bc);- }
+}
I'm not seeing any handling if we failed to find anything here.
- [SST_IP_MEDIA2] = SST_SWM_IN_MEDIA2,
- [SST_IP_MEDIA3] = SST_SWM_IN_MEDIA3,
+}; +static void sst_set_pipe_gain(struct sst_ids *ids,
Coding style - no blank line. This is quite common in this patch and something that's been an issue in previous versions of this series too.
Will fix.
- if (enable)
sst->ops->power(sst->dev, true);- mutex_lock(&drv->lock);
- if (enable)
timer_usage++;- else
timer_usage--;This seems a bit strange. We're doing the enable outside the lock, then taking the lock, then doing some refcounting outside the lock.
lock is required only to protect the timer_usage counter update, where as the power ops is to power up the DSP. If the DSP is already up, enable command is not sent and the counter is updated. DSP disable command is not sent until the last disable. Yes we need to add the error handling for power ops.
- mutex_unlock(&drv->lock);
- if (!enable)
sst->ops->power(sst->dev, false);Same thing here. How does the power management work here, it doesn't seem joined up? I'd really like to see some comments explaining what's going on with this stuff.
Power ops in SST takes care of the PM handling. Following comment is already added in the code which explains. "Send the command only if this call is the first enable or last disable"
Let us know if it is not clear enough.
+static bool is_sst_dapm_widget(struct snd_soc_dapm_widget *w) +{
- if ((w->id == snd_soc_dapm_pga) ||
(w->id == snd_soc_dapm_aif_in) ||(w->id == snd_soc_dapm_aif_out) ||(w->id == snd_soc_dapm_input) ||(w->id == snd_soc_dapm_output) ||(w->id == snd_soc_dapm_mixer))return true;- else
return false;+}
Use a switch statement please.
Sure.
--