[Sound-open-firmware] [PATCH] volume: remove unnecessary logic for mute/unmute

[Pierre-Louis Bossart]

On 03/14/2018 02:12 PM, Ranjani Sridharan wrote:
> This patch removes unnecessary logic for mute/unmute
> in volume component. Mute is handled by the driver by sending 0 for
> volume gain and unmute is handled by sending a non-zero value.
So we will have two mixers on the user-space side, one for volume and 
one for Switch on/off.
But you have a single IPC.
So if alsactl restores the mixer values with the Switch on first, what 
is the value for the volume at the firmware level?
Are we sure this design is rock-solid?

>
> Signed-off-by: Ranjani Sridharan <ranjani.sridharan at linux.intel.com>
> ---
>   src/audio/volume.c | 75 ++++++++++++------------------------------------------
>   1 file changed, 16 insertions(+), 59 deletions(-)
>
> diff --git a/src/audio/volume.c b/src/audio/volume.c
> index 0620e18..23e801c 100644
> --- a/src/audio/volume.c
> +++ b/src/audio/volume.c
> @@ -74,7 +74,6 @@ struct comp_data {
>   	uint32_t chan[SOF_IPC_MAX_CHANNELS];
>   	uint32_t volume[SOF_IPC_MAX_CHANNELS];	/* current volume */
>   	uint32_t tvolume[SOF_IPC_MAX_CHANNELS];	/* target volume */
> -	uint32_t mvolume[SOF_IPC_MAX_CHANNELS];	/* mute volume */
>   	void (*scale_vol)(struct comp_dev *dev, struct comp_buffer *sink,
>   		struct comp_buffer *source, uint32_t frames);
>   	struct work volwork;
> @@ -441,25 +440,6 @@ static inline void volume_set_chan(struct comp_dev *dev, int chan, uint32_t vol)
>   	cd->tvolume[chan] = v;
>   }
>   
> -static inline void volume_set_chan_mute(struct comp_dev *dev, int chan)
> -{
> -	struct comp_data *cd = comp_get_drvdata(dev);
> -
> -	/* Check if not muted already */
> -	if (cd->volume[chan] != 0)
> -		cd->mvolume[chan] = cd->volume[chan];
> -	cd->tvolume[chan] = 0;
> -}
> -
> -static inline void volume_set_chan_unmute(struct comp_dev *dev, int chan)
> -{
> -	struct comp_data *cd = comp_get_drvdata(dev);
> -
> -	/* Check if muted */
> -	if (cd->volume[chan] == 0)
> -		cd->tvolume[chan] = cd->mvolume[chan];
> -}
> -
>   static int volume_ctrl_set_cmd(struct comp_dev *dev, struct sof_ipc_ctrl_data *cdata)
>   {
>   	struct comp_data *cd = comp_get_drvdata(dev);
> @@ -472,48 +452,25 @@ static int volume_ctrl_set_cmd(struct comp_dev *dev, struct sof_ipc_ctrl_data *c
>   		return -EINVAL;
>   	}
>   
> -	switch (cdata->cmd) {
> -	case SOF_CTRL_CMD_VOLUME:
> -		trace_volume("vst");
> -		trace_value(cdata->comp_id);
> -		for (j = 0; j < cdata->num_elems; j++) {
> -			trace_value(cdata->chanv[j].channel);
> -			trace_value(cdata->chanv[j].value);
> -			i = cdata->chanv[j].channel;
> -			if ((i >= 0) && (i < SOF_IPC_MAX_CHANNELS))
> -				volume_set_chan(dev, i, cdata->chanv[j].value);
> -			else {
> -				trace_volume_error("gs2");
> -				tracev_value(i);
> -			}
> -		}
> -		work_schedule_default(&cd->volwork, VOL_RAMP_US);
> -		break;
> +	if (cdata->cmd != SOF_CTRL_CMD_VOLUME) {
> +		trace_volume_error("ev0");
> +		return -EINVAL;
> +	}
>   
> -	case SOF_CTRL_CMD_SWITCH:
> -		trace_volume("mst");
> -		trace_value(cdata->comp_id);
> -		for (j = 0; j < cdata->num_elems; j++) {
> -			trace_value(cdata->chanv[j].channel);
> -			trace_value(cdata->chanv[j].value);
> -			i = cdata->chanv[j].channel;
> -			if ((i >= 0) && (i < SOF_IPC_MAX_CHANNELS)) {
> -				if (cdata->chanv[j].value)
> -					volume_set_chan_unmute(dev, i);
> -				else
> -					volume_set_chan_mute(dev, i);
> -			} else {
> -				trace_volume_error("gs3");
> -				tracev_value(i);
> -			}
> +	trace_volume("vst");
> +	trace_value(cdata->comp_id);
> +	for (j = 0; j < cdata->num_elems; j++) {
> +		trace_value(cdata->chanv[j].channel);
> +		trace_value(cdata->chanv[j].value);
> +		i = cdata->chanv[j].channel;
> +		if (i >= 0 && i < SOF_IPC_MAX_CHANNELS) {
> +			volume_set_chan(dev, i, cdata->chanv[j].value);
> +		} else {
> +			trace_volume_error("gs2");
> +			tracev_value(i);
>   		}
> -		work_schedule_default(&cd->volwork, VOL_RAMP_US);
> -		break;
> -
> -	default:
> -		trace_volume_error("gs1");
> -		return -EINVAL;
>   	}
> +	work_schedule_default(&cd->volwork, VOL_RAMP_US);
>   
>   	return 0;
>   }