| Message ID | 20230803152824.7531-1-ivan.orlov0322@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | ALSA: pcmtest: Move buffer iterator initialization to prepare callback | expand |
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 <ivan.orlov0322@gmail.com> > --- > 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
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; }
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 <ivan.orlov0322@gmail.com> --- sound/drivers/pcmtest.c | 41 +++++++++++++++++++---------------------- 1 file changed, 19 insertions(+), 22 deletions(-)