All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] ALSA: pcmtest: Move buffer iterator initialization to prepare callback
@ 2023-08-03 15:28 Ivan Orlov
  2023-08-04 10:46 ` Takashi Iwai
  0 siblings, 1 reply; 3+ messages in thread
From: Ivan Orlov @ 2023-08-03 15:28 UTC (permalink / raw)
  To: perex, tiwai; +Cc: Ivan Orlov, alsa-devel, linux-kernel

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(-)

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: pcmtest: Move buffer iterator initialization to prepare callback
  2023-08-03 15:28 [PATCH] ALSA: pcmtest: Move buffer iterator initialization to prepare callback Ivan Orlov
@ 2023-08-04 10:46 ` Takashi Iwai
  2023-08-04 11:02   ` Ivan Orlov
  0 siblings, 1 reply; 3+ messages in thread
From: Takashi Iwai @ 2023-08-04 10:46 UTC (permalink / raw)
  To: Ivan Orlov; +Cc: perex, tiwai, alsa-devel, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] ALSA: pcmtest: Move buffer iterator initialization to prepare callback
  2023-08-04 10:46 ` Takashi Iwai
@ 2023-08-04 11:02   ` Ivan Orlov
  0 siblings, 0 replies; 3+ messages in thread
From: Ivan Orlov @ 2023-08-04 11:02 UTC (permalink / raw)
  To: Takashi Iwai; +Cc: perex, tiwai, alsa-devel, linux-kernel

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


^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-08-04 11:03 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-03 15:28 [PATCH] ALSA: pcmtest: Move buffer iterator initialization to prepare callback Ivan Orlov
2023-08-04 10:46 ` Takashi Iwai
2023-08-04 11:02   ` Ivan Orlov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.