| Message ID | 20230518140947.3725394-6-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: emu10k1: refactoring of the playback voice management | expand |
On Thu, 18 May 2023 16:09:45 +0200, Oswald Buddenhagen wrote: > -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices) > +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm) > { > - int err, i; > - > - for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) { > + for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) { We don't use this style. Declare the variable outside the for(). Also, as usual, it'd be still helpful if you show this is merely a code simplification without any functional change in the commit log. thanks, Takashi
On Thu, May 18, 2023 at 04:53:19PM +0200, Takashi Iwai wrote: >On Thu, 18 May 2023 16:09:45 +0200, >Oswald Buddenhagen wrote: >> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices) >> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm) >> { >> - int err, i; >> - >> - for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) { >> + for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) { > >We don't use this style. Declare the variable outside the for(). > ehm ... - "we" seems to be mostly true for alsa. but looking at the kernel as a whole, that ship has sailed since the adoption of c11. maybe time to adapt? - you're noticing this a bit late, after already merging 8 instances. how should i proceed? >Also, as usual, it'd be still helpful if you show this is merely a >code simplification without any functional change in the commit log. > right. i don't always remember to pre-emptively amend the patches i wrote quite a while ago ... regards
On Fri, 19 May 2023 12:43:24 +0200, Oswald Buddenhagen wrote: > > On Thu, May 18, 2023 at 04:53:19PM +0200, Takashi Iwai wrote: > > On Thu, 18 May 2023 16:09:45 +0200, > > Oswald Buddenhagen wrote: > >> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices) > >> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm) > >> { > >> - int err, i; > >> - > >> - for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) { > >> + for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) { > > > > We don't use this style. Declare the variable outside the for(). > > > ehm ... > - "we" seems to be mostly true for alsa. but looking at the kernel as > a whole, that ship has sailed since the adoption of c11. maybe time > to adapt? > - you're noticing this a bit late, after already merging 8 instances. > > how should i proceed? I'm not super-strict about it, but as checkpatch complaints, it's still not so widely adapted. Unless there is a reason that must be written in that way, let's avoid it as much as possible. That said, the already merged stuff, it's OK-ish, and we can correct only when anyone complains. For the new stuff, let's be careful from now on :) If we want really adapting this style, the coding style rule should be officially updated at first, followed by the update of tools. thanks, Takashi
On Fri, May 19, 2023 at 01:04:32PM +0200, Takashi Iwai wrote: >On Fri, 19 May 2023 12:43:24 +0200, >Oswald Buddenhagen wrote: >> >> On Thu, May 18, 2023 at 04:53:19PM +0200, Takashi Iwai wrote: >> > On Thu, 18 May 2023 16:09:45 +0200, >> > Oswald Buddenhagen wrote: >> >> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices) >> >> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm) >> >> { >> >> - int err, i; >> >> - >> >> - for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) { >> >> + for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) { >> > >I'm not super-strict about it, but >as checkpatch complaints, > it doesn't, so from that side it's settled. it's really just about the alsa-local policy. what it actually *does* complain about is the use of bare "unsigned". i don't like the excessively verbose "unsigned int", so i'll switch my uses over to "uint", which already has some use in alsa. ok? regards, ossi
On Fri, 19 May 2023 13:53:01 +0200, Oswald Buddenhagen wrote: > > On Fri, May 19, 2023 at 01:04:32PM +0200, Takashi Iwai wrote: > > On Fri, 19 May 2023 12:43:24 +0200, > > Oswald Buddenhagen wrote: > >> > >> On Thu, May 18, 2023 at 04:53:19PM +0200, Takashi Iwai wrote: > >> > On Thu, 18 May 2023 16:09:45 +0200, > >> > Oswald Buddenhagen wrote: > >> >> -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices) > >> >> +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm) > >> >> { > >> >> - int err, i; > >> >> - > >> >> - for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) { > >> >> + for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) { > >> > > > I'm not super-strict about it, but > > > as checkpatch complaints, > > > it doesn't, so from that side it's settled. it's really just about the > alsa-local policy. > > what it actually *does* complain about is the use of bare > "unsigned". Ah that's OK, then. > i don't like the excessively verbose "unsigned int", so > i'll switch my uses over to "uint", which already has some use in > alsa. ok? I don't mind much about the use of unsigned without int. Or it could be a simple int there, as that's nothing but a counter that is used locally that can't over 31bit. But the patch description could be still improved. thanks, Takashi
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index c4696c127028..63e085aa65fe 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -76,16 +76,22 @@ static void snd_emu10k1_pcm_efx_interrupt(struct snd_emu10k1 *emu, snd_pcm_period_elapsed(emu->pcm_capture_efx_substream); } -static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices) +static void snd_emu10k1_pcm_free_voices(struct snd_emu10k1_pcm *epcm) { - int err, i; - - for (i = 0; i < ARRAY_SIZE(epcm->voices); i++) { + for (unsigned i = 0; i < ARRAY_SIZE(epcm->voices); i++) { if (epcm->voices[i]) { snd_emu10k1_voice_free(epcm->emu, epcm->voices[i]); epcm->voices[i] = NULL; } } +} + +static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voices) +{ + int err, i; + + snd_emu10k1_pcm_free_voices(epcm); + err = snd_emu10k1_voice_alloc(epcm->emu, epcm->type == PLAYBACK_EMUVOICE ? EMU10K1_PCM : EMU10K1_EFX, voices, @@ -115,15 +121,13 @@ static int snd_emu10k1_pcm_channel_alloc(struct snd_emu10k1_pcm * epcm, int voic "failed extra: voices=%d, frame=%d\n", voices, frame); */ - for (i = 0; i < voices; i++) { - snd_emu10k1_voice_free(epcm->emu, epcm->voices[i]); - epcm->voices[i] = NULL; - } + snd_emu10k1_pcm_free_voices(epcm); return err; } epcm->extra->epcm = epcm; epcm->extra->interrupt = snd_emu10k1_pcm_interrupt; } + return 0; } @@ -342,21 +346,15 @@ static int snd_emu10k1_playback_hw_free(struct snd_pcm_substream *substream) struct snd_emu10k1 *emu = snd_pcm_substream_chip(substream); struct snd_pcm_runtime *runtime = substream->runtime; struct snd_emu10k1_pcm *epcm; - int i; if (runtime->private_data == NULL) return 0; epcm = runtime->private_data; if (epcm->extra) { snd_emu10k1_voice_free(epcm->emu, epcm->extra); epcm->extra = NULL; } - for (i = 0; i < NUM_EFX_PLAYBACK; i++) { - if (epcm->voices[i]) { - snd_emu10k1_voice_free(epcm->emu, epcm->voices[i]); - epcm->voices[i] = NULL; - } - } + snd_emu10k1_pcm_free_voices(epcm); if (epcm->memblk) { snd_emu10k1_free_pages(emu, epcm->memblk); epcm->memblk = NULL;
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- sound/pci/emu10k1/emupcm.c | 28 +++++++++++++--------------- 1 file changed, 13 insertions(+), 15 deletions(-)