[Sound-open-firmware] [PATCH] volume: remove unnecessary logic for mute/unmute
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.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@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; }
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@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);elsevolume_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; }
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@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);
elsevolume_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; }
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@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);
elsevolume_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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (3)
-
Pierre-Louis Bossart -
Ranjani -
Ranjani Sridharan