| Message ID | 20180517084911.11868-3-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2018-05-17 at 10:49 +0200, Takashi Iwai wrote: > Instead of open-coding for getting the timer resolution, use the > standard snd_timer_resolution() helper. > > The original code falls back to the callback function when the > resolution is zero, but it must be always so when the callback > function is defined. So this should be no functional change. Maybe it *should* be so, but I can see three drivers where it isn't: sound/isa/ad1816a/ad1816a_lib.c sound/isa/wss/wss_lib.c sound/sparc/cs4231.c For ad1816a_lib.c, the c_resolution implementation always returns the same value that's in resolution, so this really doesn't make a functional change. However, for the other two, the c_resolution implementation can return several different values. Ben. > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > sound/core/seq/seq_timer.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/sound/core/seq/seq_timer.c b/sound/core/seq/seq_timer.c > index 23167578231f..f587d0e27476 100644 > --- a/sound/core/seq/seq_timer.c > +++ b/sound/core/seq/seq_timer.c > @@ -371,9 +371,7 @@ static int initialize_timer(struct snd_seq_timer *tmr) > > tmr->ticks = 1; > if (!(t->hw.flags & SNDRV_TIMER_HW_SLAVE)) { > - unsigned long r = t->hw.resolution; > - if (! r && t->hw.c_resolution) > - r = t->hw.c_resolution(t); > + unsigned long r = snd_timer_resolution(tmr->timeri); > if (r) { > tmr->ticks = (unsigned int)(1000000000uL / (r * freq)); > if (! tmr->ticks)
On Thu, 31 May 2018 19:18:53 +0200, Ben Hutchings wrote: > > On Thu, 2018-05-17 at 10:49 +0200, Takashi Iwai wrote: > > Instead of open-coding for getting the timer resolution, use the > > standard snd_timer_resolution() helper. > > > > The original code falls back to the callback function when the > > resolution is zero, but it must be always so when the callback > > function is defined. So this should be no functional change. > > Maybe it *should* be so, but I can see three drivers where it isn't: > > sound/isa/ad1816a/ad1816a_lib.c > sound/isa/wss/wss_lib.c > sound/sparc/cs4231.c > > For ad1816a_lib.c, the c_resolution implementation always returns the > same value that's in resolution, so this really doesn't make a > functional change. > > However, for the other two, the c_resolution implementation can return > several different values. A good point. This implies that the patch fixes a long-standing issue :) The ALSA timer API is very rarely used, however, maybe the only real application is the alsa-lib dmix/dsnoop. And the h/w resolution doesn't matter in that case (as it's deploying only the PCM slave timer), I don't think we'd hit any issues, practically seen. thanks, Takashi > > Ben. > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > --- > > sound/core/seq/seq_timer.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/sound/core/seq/seq_timer.c b/sound/core/seq/seq_timer.c > > index 23167578231f..f587d0e27476 100644 > > --- a/sound/core/seq/seq_timer.c > > +++ b/sound/core/seq/seq_timer.c > > @@ -371,9 +371,7 @@ static int initialize_timer(struct snd_seq_timer *tmr) > > > > tmr->ticks = 1; > > if (!(t->hw.flags & SNDRV_TIMER_HW_SLAVE)) { > > - unsigned long r = t->hw.resolution; > > - if (! r && t->hw.c_resolution) > > - r = t->hw.c_resolution(t); > > + unsigned long r = snd_timer_resolution(tmr->timeri); > > if (r) { > > tmr->ticks = (unsigned int)(1000000000uL / (r * freq)); > > if (! tmr->ticks) > -- > Ben Hutchings, Software Developer Codethink Ltd > https://www.codethink.co.uk/ Dale House, 35 Dale Street > Manchester, M1 2HF, United Kingdom >
diff --git a/sound/core/seq/seq_timer.c b/sound/core/seq/seq_timer.c index 23167578231f..f587d0e27476 100644 --- a/sound/core/seq/seq_timer.c +++ b/sound/core/seq/seq_timer.c @@ -371,9 +371,7 @@ static int initialize_timer(struct snd_seq_timer *tmr) tmr->ticks = 1; if (!(t->hw.flags & SNDRV_TIMER_HW_SLAVE)) { - unsigned long r = t->hw.resolution; - if (! r && t->hw.c_resolution) - r = t->hw.c_resolution(t); + unsigned long r = snd_timer_resolution(tmr->timeri); if (r) { tmr->ticks = (unsigned int)(1000000000uL / (r * freq)); if (! tmr->ticks)
Instead of open-coding for getting the timer resolution, use the standard snd_timer_resolution() helper. The original code falls back to the callback function when the resolution is zero, but it must be always so when the callback function is defined. So this should be no functional change. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/seq/seq_timer.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)