| Message ID | 20211014145323.26506-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: pcm: Unify snd_pcm_delay() and snd_pcm_hwsync() | expand |
On 10/14/21 9:53 AM, Takashi Iwai wrote: > Both snd_pcm_delay() and snd_pcm_hwsync() do the almost same thing. > The only difference is that the former calculate the delay, so unify > them as a code cleanup, and treat NULL delay argument only for hwsync > operation. > > Also, the patch does a slight code refactoring in snd_pcm_delay(). > The initialization of the delay value is done in the caller side now. I would have done the opposite change, i.e. keep snd_pcm_hwsync() but add an optional delay argument/calculation. 'snd_pcm_delay' doesn't really hint at any hwsync operation. Just a naming difference really. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/core/pcm_native.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > index 46c643db18eb..627b201b6084 100644 > --- a/sound/core/pcm_native.c > +++ b/sound/core/pcm_native.c > @@ -2932,32 +2932,24 @@ static snd_pcm_sframes_t snd_pcm_forward(struct snd_pcm_substream *substream, > return ret; > } > > -static int snd_pcm_hwsync(struct snd_pcm_substream *substream) > -{ > - int err; > - > - snd_pcm_stream_lock_irq(substream); > - err = do_pcm_hwsync(substream); > - snd_pcm_stream_unlock_irq(substream); > - return err; > -} > - > static int snd_pcm_delay(struct snd_pcm_substream *substream, > snd_pcm_sframes_t *delay) > { > int err; > - snd_pcm_sframes_t n = 0; > > snd_pcm_stream_lock_irq(substream); > err = do_pcm_hwsync(substream); > - if (!err) > - n = snd_pcm_calc_delay(substream); > + if (delay && !err) > + *delay = snd_pcm_calc_delay(substream); > snd_pcm_stream_unlock_irq(substream); > - if (!err) > - *delay = n; > return err; > } > > +static inline int snd_pcm_hwsync(struct snd_pcm_substream *substream) > +{ > + return snd_pcm_delay(substream, NULL); > +} > + > static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, > struct snd_pcm_sync_ptr __user *_sync_ptr) > { > @@ -3275,7 +3267,7 @@ static int snd_pcm_common_ioctl(struct file *file, > return snd_pcm_hwsync(substream); > case SNDRV_PCM_IOCTL_DELAY: > { > - snd_pcm_sframes_t delay; > + snd_pcm_sframes_t delay = 0; > snd_pcm_sframes_t __user *res = arg; > int err; > >
On Thu, 14 Oct 2021 17:59:21 +0200, Pierre-Louis Bossart wrote: > > > > On 10/14/21 9:53 AM, Takashi Iwai wrote: > > Both snd_pcm_delay() and snd_pcm_hwsync() do the almost same thing. > > The only difference is that the former calculate the delay, so unify > > them as a code cleanup, and treat NULL delay argument only for hwsync > > operation. > > > > Also, the patch does a slight code refactoring in snd_pcm_delay(). > > The initialization of the delay value is done in the caller side now. > > I would have done the opposite change, i.e. keep snd_pcm_hwsync() but > add an optional delay argument/calculation. > > 'snd_pcm_delay' doesn't really hint at any hwsync operation. > > Just a naming difference really. Yes, but also the difference of the number of arguments. Changing other way round would need to more modifications in the end. Which calls what is rather an implementation detail :) Takashi > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > sound/core/pcm_native.c | 24 ++++++++---------------- > > 1 file changed, 8 insertions(+), 16 deletions(-) > > > > diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c > > index 46c643db18eb..627b201b6084 100644 > > --- a/sound/core/pcm_native.c > > +++ b/sound/core/pcm_native.c > > @@ -2932,32 +2932,24 @@ static snd_pcm_sframes_t snd_pcm_forward(struct snd_pcm_substream *substream, > > return ret; > > } > > > > -static int snd_pcm_hwsync(struct snd_pcm_substream *substream) > > -{ > > - int err; > > - > > - snd_pcm_stream_lock_irq(substream); > > - err = do_pcm_hwsync(substream); > > - snd_pcm_stream_unlock_irq(substream); > > - return err; > > -} > > - > > static int snd_pcm_delay(struct snd_pcm_substream *substream, > > snd_pcm_sframes_t *delay) > > { > > int err; > > - snd_pcm_sframes_t n = 0; > > > > snd_pcm_stream_lock_irq(substream); > > err = do_pcm_hwsync(substream); > > - if (!err) > > - n = snd_pcm_calc_delay(substream); > > + if (delay && !err) > > + *delay = snd_pcm_calc_delay(substream); > > snd_pcm_stream_unlock_irq(substream); > > - if (!err) > > - *delay = n; > > return err; > > } > > > > +static inline int snd_pcm_hwsync(struct snd_pcm_substream *substream) > > +{ > > + return snd_pcm_delay(substream, NULL); > > +} > > + > > static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, > > struct snd_pcm_sync_ptr __user *_sync_ptr) > > { > > @@ -3275,7 +3267,7 @@ static int snd_pcm_common_ioctl(struct file *file, > > return snd_pcm_hwsync(substream); > > case SNDRV_PCM_IOCTL_DELAY: > > { > > - snd_pcm_sframes_t delay; > > + snd_pcm_sframes_t delay = 0; > > snd_pcm_sframes_t __user *res = arg; > > int err; > > > > >
On 10/14/21 11:08 AM, Takashi Iwai wrote: > On Thu, 14 Oct 2021 17:59:21 +0200, > Pierre-Louis Bossart wrote: >> >> >> >> On 10/14/21 9:53 AM, Takashi Iwai wrote: >>> Both snd_pcm_delay() and snd_pcm_hwsync() do the almost same thing. >>> The only difference is that the former calculate the delay, so unify >>> them as a code cleanup, and treat NULL delay argument only for hwsync >>> operation. >>> >>> Also, the patch does a slight code refactoring in snd_pcm_delay(). >>> The initialization of the delay value is done in the caller side now. >> >> I would have done the opposite change, i.e. keep snd_pcm_hwsync() but >> add an optional delay argument/calculation. >> >> 'snd_pcm_delay' doesn't really hint at any hwsync operation. >> >> Just a naming difference really. > > Yes, but also the difference of the number of arguments. Changing > other way round would need to more modifications in the end. Ah yes, makes sense Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
diff --git a/sound/core/pcm_native.c b/sound/core/pcm_native.c index 46c643db18eb..627b201b6084 100644 --- a/sound/core/pcm_native.c +++ b/sound/core/pcm_native.c @@ -2932,32 +2932,24 @@ static snd_pcm_sframes_t snd_pcm_forward(struct snd_pcm_substream *substream, return ret; } -static int snd_pcm_hwsync(struct snd_pcm_substream *substream) -{ - int err; - - snd_pcm_stream_lock_irq(substream); - err = do_pcm_hwsync(substream); - snd_pcm_stream_unlock_irq(substream); - return err; -} - static int snd_pcm_delay(struct snd_pcm_substream *substream, snd_pcm_sframes_t *delay) { int err; - snd_pcm_sframes_t n = 0; snd_pcm_stream_lock_irq(substream); err = do_pcm_hwsync(substream); - if (!err) - n = snd_pcm_calc_delay(substream); + if (delay && !err) + *delay = snd_pcm_calc_delay(substream); snd_pcm_stream_unlock_irq(substream); - if (!err) - *delay = n; return err; } +static inline int snd_pcm_hwsync(struct snd_pcm_substream *substream) +{ + return snd_pcm_delay(substream, NULL); +} + static int snd_pcm_sync_ptr(struct snd_pcm_substream *substream, struct snd_pcm_sync_ptr __user *_sync_ptr) { @@ -3275,7 +3267,7 @@ static int snd_pcm_common_ioctl(struct file *file, return snd_pcm_hwsync(substream); case SNDRV_PCM_IOCTL_DELAY: { - snd_pcm_sframes_t delay; + snd_pcm_sframes_t delay = 0; snd_pcm_sframes_t __user *res = arg; int err;
Both snd_pcm_delay() and snd_pcm_hwsync() do the almost same thing. The only difference is that the former calculate the delay, so unify them as a code cleanup, and treat NULL delay argument only for hwsync operation. Also, the patch does a slight code refactoring in snd_pcm_delay(). The initialization of the delay value is done in the caller side now. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/pcm_native.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)