[alsa-devel] [v4 05/12] ASoC: Intel: mrfld: add bytes control for modules

[Mark Brown]

On Mon, Aug 04, 2014 at 03:15:56PM +0530, Subhransu S. Prusty wrote:

> From: Vinod Koul <vinod.koul at intel.com>

> This patch add support for various modules like eq etc for mrfld DSP. All these
> modules will be exposed to usermode as bytes controls.

Indeed they are actually exposed by this code.

> +static inline void sst_fill_byte_control(char *param,
> +					 u8 ipc_msg, u8 block,
> +					 u8 task_id, u8 pipe_id,
> +					 u16 len, void *cmd_data)

Let the compiler figure out if this should be inline, this doesn't seem
something that should obviously be inline and isn't in a header.

> +	if (len > SST_MAX_BIN_BYTES - sizeof(*byte_data)) {
> +		WARN(1, "%s: command length too big (%u)", __func__, len); /* this happens only if code is wrong */
> +		len = SST_MAX_BIN_BYTES - sizeof(*byte_data);
> +	}

Coding style, 80 columns.  Since the only caller can return an error
code you could do that here too.

> +static int sst_fill_and_send_cmd_unlocked(struct sst_data *drv,
> +				 u8 ipc_msg, u8 block, u8 task_id, u8 pipe_id,
> +				 void *cmd_data, u16 len)
> +{
> +	sst_fill_byte_control(drv->byte_stream, ipc_msg, block, task_id, pipe_id,
> +			      len, cmd_data);
> +	return sst->ops->send_byte_stream(sst->dev,
> +			(struct snd_sst_bytes_v2 *)drv->byte_stream);
> +}

There's a lot of casting going on in this code.

> +static int sst_algo_bytes_ctl_info(struct snd_kcontrol *kcontrol,
> +			    struct snd_ctl_elem_info *uinfo)
> +{
> +	struct sst_algo_control *bc = (void *)kcontrol->private_value;
> +	struct snd_soc_component *component = snd_kcontrol_chip(kcontrol);
> +
> +	uinfo->type = SNDRV_CTL_ELEM_TYPE_BYTES;
> +	uinfo->count = bc->max;
> +
> +	/* allocate space to cache the algo parameters in the driver */
> +	if (bc->params == NULL) {
> +		bc->params = devm_kzalloc(component->dev, bc->max, GFP_KERNEL);
> +		if (bc->params == NULL)
> +			return -ENOMEM;
> +	}
> +	return 0;
> +}

I wouldn't expect an info call to be allocating anything - why is it
doing that?  It's not looking at the alocated data except to see if the
allocation succeeded.  What happens if someone manages to do a get or
set without having first done an info and why aren't we doing any
allocation on initialisation?

> +	case SST_ALGO_BYPASS:
> +		ucontrol->value.integer.value[0] = bc->bypass ? 1 : 0;
> +		pr_debug("%s: bypass  %d\n", __func__, bc->bypass);
> +		break;

Is bypass not a boolean value already, and shouldn't these just end up
with controls called something "Switch" - they look to be just directly
usable switches?

> +	default:
> +		pr_err("Invalid Input- algo type:%d\n", bc->type);

dev_err().

> +	switch (bc->type) {
> +	case SST_ALGO_PARAMS:
> +		if (bc->params)
> +			memcpy(bc->params, ucontrol->value.bytes.data, bc->max);
> +		break;

So if bc->params didn't get allocated somehow we just silently drop the
set?

> +	/*if pipe is enabled, need to send the algo params from here */
> +	if (bc->w && bc->w->power)
> +		sst_send_algo_cmd(drv, bc);

sst_send_algo_cmd() should be returning an eror code (it doesn't but the
functions it calls can).

I'm not seeing any code to restore these controls on power up here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://mailman.alsa-project.org/pipermail/alsa-devel/attachments/20140813/bbad7ca5/attachment-0001.sig>