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

[Subhransu S. Prusty]

On Wed, Aug 13, 2014 at 09:00:12PM +0100, Mark Brown wrote:
> 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.
Ok
> 
> > +	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.
Yes.
> 
> > +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.
Not required. Should have been removed.
> 
> > +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?

Will move after control initialization.
> 
> > +	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?
Actually BYPASS is not required. Will remove.
> 
> > +	default:
> > +		pr_err("Invalid Input- algo type:%d\n", bc->type);
> 
> dev_err().
We are using pr_err() in all the error conditions for our driver. If it is
really required to change, we can submit a single patch to change to
dev_err() once the patches are merged.
> 
> > +	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?
With above params initialization changes this check will not be required any
more.
> 
> > +	/*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.

Will take care of the return code.

sst_find_and_send_pipe_algo takes care of restoring the controls on power
up.

--