[Sound-open-firmware] [PATCH] volume: ctrl cmd id check and prepare fix
This patch fixes the following:
a. source_period_bytes/sink_period_bytes calculation fix with the correct frame_bytes b. check if the ctrl cmd is for the volume before processing it
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com --- src/audio/volume.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/audio/volume.c b/src/audio/volume.c index 5808bed..82b8848 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -471,9 +471,17 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data)
switch (cmd) { case COMP_CMD_SET_VALUE: - return volume_ctrl_set_cmd(dev, cdata); + /* check if the ctrl cmd is for this component */ + if ((cdata != NULL) && (cdata->comp_id != dev->comp.id)) + return 0; + else + return volume_ctrl_set_cmd(dev, cdata); case COMP_CMD_GET_VALUE: - return volume_ctrl_get_cmd(dev, cdata); + /* check if the ctrl cmd is for this component */ + if ((cdata != NULL) && (cdata->comp_id != dev->comp.id)) + return 0; + else + return volume_ctrl_get_cmd(dev, cdata); case COMP_CMD_START: case COMP_CMD_STOP: case COMP_CMD_PAUSE: @@ -555,7 +563,7 @@ static int volume_prepare(struct comp_dev *dev) sconfig = COMP_GET_CONFIG(sourceb->source); cd->source_format = sconfig->frame_fmt; cd->source_period_bytes = dev->frames * - sourceb->source->frame_bytes; + comp_frame_bytes(sourceb->source); break; }
@@ -575,7 +583,7 @@ static int volume_prepare(struct comp_dev *dev) sconfig = COMP_GET_CONFIG(sinkb->sink); cd->sink_format = sconfig->frame_fmt; cd->sink_period_bytes = dev->frames * - sinkb->sink->frame_bytes; + comp_frame_bytes(sinkb->sink); break; }
On Mon, 2017-09-11 at 22:21 +0100, Ranjani Sridharan wrote:
This patch fixes the following:
a. source_period_bytes/sink_period_bytes calculation fix with the correct frame_bytes b. check if the ctrl cmd is for the volume before processing it
Could you do this as 2 patches. One minor comment below too.
Signed-off-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
src/audio/volume.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/audio/volume.c b/src/audio/volume.c index 5808bed..82b8848 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -471,9 +471,17 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data)
switch (cmd) { case COMP_CMD_SET_VALUE:
return volume_ctrl_set_cmd(dev, cdata);
/* check if the ctrl cmd is for this component */
if ((cdata != NULL) && (cdata->comp_id != dev->comp.id))
return 0;
We should return an error if cdata is NULL or there is an ID mismatch. These can also both be check once at the function entry rather than in the switch statement.
Liam
else
case COMP_CMD_GET_VALUE:return volume_ctrl_set_cmd(dev, cdata);
return volume_ctrl_get_cmd(dev, cdata);
/* check if the ctrl cmd is for this component */
if ((cdata != NULL) && (cdata->comp_id != dev->comp.id))
return 0;
else
case COMP_CMD_START: case COMP_CMD_STOP: case COMP_CMD_PAUSE:return volume_ctrl_get_cmd(dev, cdata);
@@ -555,7 +563,7 @@ static int volume_prepare(struct comp_dev *dev) sconfig = COMP_GET_CONFIG(sourceb->source); cd->source_format = sconfig->frame_fmt; cd->source_period_bytes = dev->frames *
sourceb->source->frame_bytes;
break; }comp_frame_bytes(sourceb->source);
@@ -575,7 +583,7 @@ static int volume_prepare(struct comp_dev *dev) sconfig = COMP_GET_CONFIG(sinkb->sink); cd->sink_format = sconfig->frame_fmt; cd->sink_period_bytes = dev->frames *
sinkb->sink->frame_bytes;
break; }comp_frame_bytes(sinkb->sink);
Thanks, Liam. I'll split up the patch into 2.
Also, just a comment that it should not return an error when there is an ID mismatch. It just means that the command is not meant for this component and it should just pass it forward. I will change it to return an error if cdata is NULL.
On Tue, 2017-09-12 at 12:36 +0100, Liam Girdwood wrote:
On Mon, 2017-09-11 at 22:21 +0100, Ranjani Sridharan wrote:
This patch fixes the following:
a. source_period_bytes/sink_period_bytes calculation fix with the correct frame_bytes b. check if the ctrl cmd is for the volume before processing it
Could you do this as 2 patches. One minor comment below too.
Signed-off-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com
src/audio/volume.c | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-)
diff --git a/src/audio/volume.c b/src/audio/volume.c index 5808bed..82b8848 100644 --- a/src/audio/volume.c +++ b/src/audio/volume.c @@ -471,9 +471,17 @@ static int volume_cmd(struct comp_dev *dev, int cmd, void *data) switch (cmd) { case COMP_CMD_SET_VALUE:
return volume_ctrl_set_cmd(dev, cdata);
/* check if the ctrl cmd is for this component */
if ((cdata != NULL) && (cdata->comp_id != dev-
comp.id))
return 0;
We should return an error if cdata is NULL or there is an ID mismatch. These can also both be check once at the function entry rather than in the switch statement.
Liam
else
return volume_ctrl_set_cmd(dev, cdata);
case COMP_CMD_GET_VALUE:
return volume_ctrl_get_cmd(dev, cdata);
/* check if the ctrl cmd is for this component */
if ((cdata != NULL) && (cdata->comp_id != dev-
comp.id))
return 0;
else
return volume_ctrl_get_cmd(dev, cdata);
case COMP_CMD_START: case COMP_CMD_STOP: case COMP_CMD_PAUSE: @@ -555,7 +563,7 @@ static int volume_prepare(struct comp_dev *dev) sconfig = COMP_GET_CONFIG(sourceb->source); cd->source_format = sconfig->frame_fmt; cd->source_period_bytes = dev->frames *
sourceb->source->frame_bytes;
comp_frame_bytes(sourceb->source);
break; } @@ -575,7 +583,7 @@ static int volume_prepare(struct comp_dev *dev) sconfig = COMP_GET_CONFIG(sinkb->sink); cd->sink_format = sconfig->frame_fmt; cd->sink_period_bytes = dev->frames *
sinkb->sink->frame_bytes;
comp_frame_bytes(sinkb->sink);
break; }
Sound-open-firmware mailing list Sound-open-firmware@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
participants (2)
-
Liam Girdwood
-
Ranjani Sridharan