Re: [PATCH 4/9] ASoC: Intel: Skylake: Add FE and BE hw_params handling

[Mark Brown <broonie@kernel.org>]

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

On Sat, Aug 08, 2015 at 01:06:19AM +0530, Subhransu S. Prusty wrote:

> +	/*
> +	 * 16 bit is 16 bit container whereas 24 bit is in 32 bit container so
> +	 * update bit depth accordingly
> +	 */
> +	if (format->valid_bit_depth == SKL_DEPTH_16BIT)
> +		format->bit_depth = format->valid_bit_depth;
> +	else if (format->valid_bit_depth == SKL_DEPTH_24BIT)
> +		format->bit_depth = SKL_DEPTH_32BIT;
> +

What if the depth is neither 16 bit nor 24 bit?  Shouldn't this just be
a simple override for the 24 bit case?

> +/*
> + * Fill the BE gateway parameters
> + * The BE gateway expects a blob of parameters which are kept in the ACPI
> + * NHLT blob, so query the blob for interface type (i2s/pdm) and instance.
> + * The port cna have multiple settings so pick based on the PCM
> + * parameters
> + */
> +static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
> +		struct skl_module_cfg *mconfig, struct skl_pipe_params *params)
> +{
> +	struct skl_pipe *pipe = mconfig->pipe;
> +	struct nhlt_specific_cfg *cfg;
> +	struct skl *skl = get_skl_ctx(dai->dev);
> +	int link_type = skl_tplg_be_link_type(mconfig->dev_type);
> +
> +	memcpy(pipe->p_params, params, sizeof(*params));
> +
> +	/* update the blob based on virtual bus_id*/
> +	cfg = (struct nhlt_specific_cfg *) skl_get_ep_blob(skl,
> +			mconfig->vbus_id, link_type, params->s_fmt,
> +			params->ch, params->s_freq, params->stream);

I don't seem to have slk_get_ep_blob() but hopefully it's returning a
void * in which case why do we need to cast it?  In any case it needs to
be defined in mainline before this can be applied :(

> +	if (cfg) {
> +		mconfig->formats_config.caps_size = cfg->size;
> +		memcpy(mconfig->formats_config.caps, &cfg->caps, cfg->size);
> +	} else {
> +		dev_dbg(dai->dev, "Blob is NULL");
> +		return -EINVAL;
> +	}

This seems like a more serious problem than something we just dev_dbg()
about?

> +static int skl_tplg_be_set_params(struct snd_soc_dai *dai,
> +					struct snd_soc_dapm_widget *w,
> +					struct skl_pipe_params *params)
> +{
> +	if (!w->power) {
> +		dev_dbg(dai->dev, "set params for widget=%s\n", w->name);
> +		return skl_tplg_be_fill_pipe_params(dai, w->priv, params);
> +	}
> +
> +	return -EINVAL;

Shouldn't that be -EBUSY?  The normal idiom would be to check to see if
we were busy and error out rather than writing it as only configuing if
not busy (which looks like an error case now).

> +	list_for_each_entry(p, &w->sources, list_sink) {
> +		if (p->connect && is_skl_dsp_widget_type(p->source) &&
> +						 p->source->priv) {
> +			ret = skl_tplg_be_set_params(dai, p->source, params);
> +
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			ret = skl_tplg_be_set_src_pipe_params(dai, p->source,
> +									 params);
> +		}

These two cases appear to be identical apart from ignoring the error in
the else case...

> +	}
> +
> +	return ret;

...unless it happens to be the last entry in the list in which case we
pay attention.

> +static int skl_tplg_be_set_sink_pipe_params(struct snd_soc_dai *dai,
> +	struct snd_soc_dapm_widget *w, struct skl_pipe_params *params)
> +{
> +	struct snd_soc_dapm_path *p = NULL;
> +	int ret = 0;
> +
> +	dev_dbg(dai->dev, "widget name=%s\n", w->name);
> +
> +	list_for_each_entry(p, &w->sinks, list_source) {
> +		if (p->connect && is_skl_dsp_widget_type(p->sink) &&
> +						p->sink->priv) {
> +			ret = skl_tplg_be_set_params(dai, p->sink, params);
> +
> +			if (ret < 0)
> +				return ret;
> +		} else {
> +			ret = skl_tplg_be_set_sink_pipe_params(dai, p->sink,
> +									 params);
> +		}
> +	}

There's some more common code here and the same patterns as above...  :/

> +	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		w = dai->playback_widget;
> +
> +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> +		return skl_tplg_be_set_src_pipe_params(dai, w, params);
> +	}
> +
> +	if (params->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +		w = dai->capture_widget;
> +
> +		dev_dbg(dai->dev, "Stream name=%s\n", w->name);
> +		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
> +	}

Normally that'd be written as an if/else, and the only difference
between the two cases here is which widget we pick...

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

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