Trigger callback is not the best place for buffer iterator
initialization, so move it out to the prepare callback, where it
have to be.
Minor enhancement: remove redundant definitions and blank line.
Signed-off-by: Ivan Orlov <[email protected]>
---
sound/drivers/pcmtest.c | 41 +++++++++++++++++++----------------------
1 file changed, 19 insertions(+), 22 deletions(-)
diff --git a/sound/drivers/pcmtest.c b/sound/drivers/pcmtest.c
index 08e14b5eb772..7f170429eb8f 100644
--- a/sound/drivers/pcmtest.c
+++ b/sound/drivers/pcmtest.c
@@ -41,8 +41,6 @@
#include <linux/debugfs.h>
#include <linux/delay.h>
-#define DEVNAME "pcmtestd"
-#define CARD_NAME "pcm-test-card"
#define TIMER_PER_SEC 5
#define TIMER_INTERVAL (HZ / TIMER_PER_SEC)
#define DELAY_JIFFIES HZ
@@ -74,11 +72,11 @@ static u8 ioctl_reset_test;
static struct dentry *driver_debug_dir;
module_param(index, int, 0444);
-MODULE_PARM_DESC(index, "Index value for " CARD_NAME " soundcard");
+MODULE_PARM_DESC(index, "Index value for pcmtest soundcard");
module_param(id, charp, 0444);
-MODULE_PARM_DESC(id, "ID string for " CARD_NAME " soundcard");
+MODULE_PARM_DESC(id, "ID string for pcmtest soundcard");
module_param(enable, bool, 0444);
-MODULE_PARM_DESC(enable, "Enable " CARD_NAME " soundcard.");
+MODULE_PARM_DESC(enable, "Enable pcmtest soundcard.");
module_param(fill_mode, short, 0600);
MODULE_PARM_DESC(fill_mode, "Buffer fill mode: rand(0) or pattern(1)");
module_param(inject_delay, int, 0600);
@@ -92,7 +90,6 @@ MODULE_PARM_DESC(inject_trigger_err, "Inject EINVAL error in the 'trigger' callb
module_param(inject_open_err, bool, 0600);
MODULE_PARM_DESC(inject_open_err, "Inject EBUSY error in the 'open' callback");
-
struct pcmtst {
struct snd_pcm *pcm;
struct snd_card *card;
@@ -405,25 +402,9 @@ static int snd_pcmtst_pcm_close(struct snd_pcm_substream *substream)
static int snd_pcmtst_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
{
- struct snd_pcm_runtime *runtime = substream->runtime;
- struct pcmtst_buf_iter *v_iter = runtime->private_data;
-
if (inject_trigger_err)
return -EINVAL;
- v_iter->sample_bytes = runtime->sample_bits / 8;
- v_iter->period_bytes = frames_to_bytes(runtime, runtime->period_size);
- if (runtime->access == SNDRV_PCM_ACCESS_RW_NONINTERLEAVED ||
- runtime->access == SNDRV_PCM_ACCESS_MMAP_NONINTERLEAVED) {
- v_iter->chan_block = runtime->dma_bytes / runtime->channels;
- v_iter->interleaved = false;
- } else {
- v_iter->interleaved = true;
- }
- // We want to record RATE * ch_cnt samples per sec, it is rate * sample_bytes * ch_cnt bytes
- v_iter->s_rw_ch = runtime->rate / TIMER_PER_SEC;
- v_iter->b_rw = v_iter->s_rw_ch * v_iter->sample_bytes * runtime->channels;
-
return 0;
}
@@ -454,8 +435,24 @@ static void pcmtst_pdev_release(struct device *dev)
static int snd_pcmtst_pcm_prepare(struct snd_pcm_substream *substream)
{
+ struct snd_pcm_runtime *runtime = substream->runtime;
+ struct pcmtst_buf_iter *v_iter = runtime->private_data;
+
if (inject_prepare_err)
return -EINVAL;
+
+ v_iter->sample_bytes = samples_to_bytes(runtime, 1);
+ v_iter->period_bytes = snd_pcm_lib_period_bytes(substream);
+ v_iter->interleaved = true;
+ if (runtime->access == SNDRV_PCM_ACCESS_RW_NONINTERLEAVED ||
+ runtime->access == SNDRV_PCM_ACCESS_MMAP_NONINTERLEAVED) {
+ v_iter->chan_block = snd_pcm_lib_buffer_bytes(substream) / runtime->channels;
+ v_iter->interleaved = false;
+ }
+ // We want to record RATE * ch_cnt samples per sec, it is rate * sample_bytes * ch_cnt bytes
+ v_iter->s_rw_ch = runtime->rate / TIMER_PER_SEC;
+ v_iter->b_rw = v_iter->s_rw_ch * v_iter->sample_bytes * runtime->channels;
+
return 0;
}
--
2.34.1
On 8/4/23 14:46, Takashi Iwai wrote:
> A blank line removal is OK, but other changes are too much to fold
> into. Please split.
>
Hi Takashi,
Thank you for the review, I'll split the changes and send them as a
patch series in a few minutes.
--
Kind regards,
Ivan Orlov
On Thu, 03 Aug 2023 17:28:24 +0200,
Ivan Orlov wrote:
>
> Trigger callback is not the best place for buffer iterator
> initialization, so move it out to the prepare callback, where it
> have to be.
>
> Minor enhancement: remove redundant definitions and blank line.
A blank line removal is OK, but other changes are too much to fold
into. Please split.
thanks,
Takashi
>
> Signed-off-by: Ivan Orlov <[email protected]>
> ---
> sound/drivers/pcmtest.c | 41 +++++++++++++++++++----------------------
> 1 file changed, 19 insertions(+), 22 deletions(-)
>
> diff --git a/sound/drivers/pcmtest.c b/sound/drivers/pcmtest.c
> index 08e14b5eb772..7f170429eb8f 100644
> --- a/sound/drivers/pcmtest.c
> +++ b/sound/drivers/pcmtest.c
> @@ -41,8 +41,6 @@
> #include <linux/debugfs.h>
> #include <linux/delay.h>
>
> -#define DEVNAME "pcmtestd"
> -#define CARD_NAME "pcm-test-card"
> #define TIMER_PER_SEC 5
> #define TIMER_INTERVAL (HZ / TIMER_PER_SEC)
> #define DELAY_JIFFIES HZ
> @@ -74,11 +72,11 @@ static u8 ioctl_reset_test;
> static struct dentry *driver_debug_dir;
>
> module_param(index, int, 0444);
> -MODULE_PARM_DESC(index, "Index value for " CARD_NAME " soundcard");
> +MODULE_PARM_DESC(index, "Index value for pcmtest soundcard");
> module_param(id, charp, 0444);
> -MODULE_PARM_DESC(id, "ID string for " CARD_NAME " soundcard");
> +MODULE_PARM_DESC(id, "ID string for pcmtest soundcard");
> module_param(enable, bool, 0444);
> -MODULE_PARM_DESC(enable, "Enable " CARD_NAME " soundcard.");
> +MODULE_PARM_DESC(enable, "Enable pcmtest soundcard.");
> module_param(fill_mode, short, 0600);
> MODULE_PARM_DESC(fill_mode, "Buffer fill mode: rand(0) or pattern(1)");
> module_param(inject_delay, int, 0600);
> @@ -92,7 +90,6 @@ MODULE_PARM_DESC(inject_trigger_err, "Inject EINVAL error in the 'trigger' callb
> module_param(inject_open_err, bool, 0600);
> MODULE_PARM_DESC(inject_open_err, "Inject EBUSY error in the 'open' callback");
>
> -
> struct pcmtst {
> struct snd_pcm *pcm;
> struct snd_card *card;
> @@ -405,25 +402,9 @@ static int snd_pcmtst_pcm_close(struct snd_pcm_substream *substream)
>
> static int snd_pcmtst_pcm_trigger(struct snd_pcm_substream *substream, int cmd)
> {
> - struct snd_pcm_runtime *runtime = substream->runtime;
> - struct pcmtst_buf_iter *v_iter = runtime->private_data;
> -
> if (inject_trigger_err)
> return -EINVAL;
>
> - v_iter->sample_bytes = runtime->sample_bits / 8;
> - v_iter->period_bytes = frames_to_bytes(runtime, runtime->period_size);
> - if (runtime->access == SNDRV_PCM_ACCESS_RW_NONINTERLEAVED ||
> - runtime->access == SNDRV_PCM_ACCESS_MMAP_NONINTERLEAVED) {
> - v_iter->chan_block = runtime->dma_bytes / runtime->channels;
> - v_iter->interleaved = false;
> - } else {
> - v_iter->interleaved = true;
> - }
> - // We want to record RATE * ch_cnt samples per sec, it is rate * sample_bytes * ch_cnt bytes
> - v_iter->s_rw_ch = runtime->rate / TIMER_PER_SEC;
> - v_iter->b_rw = v_iter->s_rw_ch * v_iter->sample_bytes * runtime->channels;
> -
> return 0;
> }
>
> @@ -454,8 +435,24 @@ static void pcmtst_pdev_release(struct device *dev)
>
> static int snd_pcmtst_pcm_prepare(struct snd_pcm_substream *substream)
> {
> + struct snd_pcm_runtime *runtime = substream->runtime;
> + struct pcmtst_buf_iter *v_iter = runtime->private_data;
> +
> if (inject_prepare_err)
> return -EINVAL;
> +
> + v_iter->sample_bytes = samples_to_bytes(runtime, 1);
> + v_iter->period_bytes = snd_pcm_lib_period_bytes(substream);
> + v_iter->interleaved = true;
> + if (runtime->access == SNDRV_PCM_ACCESS_RW_NONINTERLEAVED ||
> + runtime->access == SNDRV_PCM_ACCESS_MMAP_NONINTERLEAVED) {
> + v_iter->chan_block = snd_pcm_lib_buffer_bytes(substream) / runtime->channels;
> + v_iter->interleaved = false;
> + }
> + // We want to record RATE * ch_cnt samples per sec, it is rate * sample_bytes * ch_cnt bytes
> + v_iter->s_rw_ch = runtime->rate / TIMER_PER_SEC;
> + v_iter->b_rw = v_iter->s_rw_ch * v_iter->sample_bytes * runtime->channels;
> +
> return 0;
> }
>
> --
> 2.34.1
>