On Fri, Apr 24, 2015 at 06:51:15PM +0100, Mark Brown wrote:
On Sun, Apr 19, 2015 at 02:27:31AM +0530, Vinod Koul wrote:
+static int is_hda_widget_type(struct snd_soc_dapm_widget *w) +{
- return ((w->id == snd_soc_dapm_dai_link) ||
(w->id == snd_soc_dapm_dai_in) ||(w->id == snd_soc_dapm_aif_in) ||(w->id == snd_soc_dapm_aif_out) ||(w->id == snd_soc_dapm_dai_out)) ? 1 : 0;Please use switch statements, and again no need to convert logic values into logic values.
OK
+static int hda_sst_dapm_pre_pmu_event(struct snd_soc_dapm_widget *w,
- int w_type, struct ssth_lib *ctx, struct hda_platform_info *pinfo)
+{
- int ret = 0;
- struct ssth_module_config *mconfig = w->priv;
- struct ssth_pipe *s_pipe = mconfig->pipe;
- dev_dbg(ctx->dev, "%s: widget =%s type=%d\n", __func__, w->name, w_type);
- /*check resource available */
- if (!hda_sst_is_pipe_mcps_available(pinfo, ctx, mconfig))
return -1;Proper error code please.
Sure
- if (w_type == SSTH_WIDGET_VMIXER ||
w_type == SSTH_WIDGET_MIXER) {switch.
Since this is checking for these two types only, I think if maybe fine, but dont mind swicth too.
if (!hda_sst_is_pipe_mem_available(pinfo, ctx, mconfig))return -ENOMEM;ret = ssth_create_pipeline(ctx, mconfig->pipe);if (ret < 0)return ret;if (list_empty(&s_pipe->w_list)) {ret = hda_sst_get_pipe_widget(ctx->dev, w, s_pipe);if (ret < 0)return ret;}ret = hda_init_pipe_modules(ctx, s_pipe, pinfo);if (ret < 0)return ret;The error handling here appears to be a bit incomplete, we don't unwind anything we did.
Yes let me check that.
- switch (event) {
- case SND_SOC_DAPM_PRE_PMU:
return hda_sst_dapm_pre_pmu_event(w, w_type, ctx, pinfo);- break;
Please follow the kernel coding style.
oops, will fix
+static int hda_sst_vmixer_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *k, int event)+{
- struct snd_soc_dapm_context *dapm = w->dapm;
- dev_dbg(dapm->dev, "%s: widget = %s\n", __func__, w->name);
- return hda_sst_event_handler(w, event, SSTH_WIDGET_VMIXER);
+}
+static int hda_sst_mixer_event(struct snd_soc_dapm_widget *w,
struct snd_kcontrol *k, int event)+{
- struct snd_soc_dapm_context *dapm = w->dapm;
- dev_dbg(dapm->dev, "%s: widget = %s\n", __func__, w->name);
- return hda_sst_event_handler(w, event, SSTH_WIDGET_MIXER);
+}
Lots of indirection and wrapping going on here which seems to make things more confusing. Can you try writing out the event handling directly and having it call common functions to perform the shared operations rather than having a single event handler with per type cases? Or alternatively use the DAPM widget type rather than your own ones.
Okay let me cleanup this code is next series
- /* if FE - Playback, then parse sink list , Capture then source list
- if BE - Playback, then parse source list , Capture then sink list
- */
Coding style on comments too BTW.
will fix
- if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
w = dai->playback_widget;(is_fe) ? (dir = 1) : (dir = 0);This looks like a very complicated way of writing dir = is_fe.
Yes for sure :)