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

[Ranjani]

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.

> 
> > 
> > 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;
> >   }
> 
>