Re: [RFC 5/7] ASOC: hda: add DSP platfrom controls widget event handlers

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2995 bytes --]

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.

> +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.

> +	if (w_type == SSTH_WIDGET_VMIXER ||
> +		w_type == SSTH_WIDGET_MIXER) {

switch.

> +
> +		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.

> +	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.

> +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.

> +	/* 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.

> +	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.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]