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

[Pierre-Louis Bossart]

On 03/14/2018 04:41 PM, Ranjani wrote:
> On Wed, 2018-03-14 at 14:31 -0500, Pierre-Louis Bossart wrote:
>> 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?
> Pierre, the question I was trying to answer with this patch is, would
> we ever have more than one mixer control for volume in user-space. I
> guess you have answered the question here. In that case, we should
> leave this logic in volume to handle it.
Yes we absolutely want the switch/mute separate from the volume for user 
interaction
I am hoping that some day AI will be smart enough to remove the mute 
button and avoid the 'sorry I was on mute' but for now it's a manual 
override separate from volume control - which is changed less often in 
practice.

>
>>> 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;
>>>    }
>>
> _______________________________________________
> Sound-open-firmware mailing list
> Sound-open-firmware at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware