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 :)

-- 
~Vinod
--