| Message ID | 20230422161021.1143989-1-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: emu10k1: fix multi-channel playback device class | expand |
On Sat, 22 Apr 2023 18:10:21 +0200, Oswald Buddenhagen wrote: > > It's multi, not mono/stereo. > > AFAICT, this doesn't do anything in the kernel. ... but those values are read by user-space. > Also, I think the subclass is meaningless for devices with just one > stream, but whatever. Again, the value is read by user-space. So changing both have clear influence on the user-space program, and unless you have to change this for fixing a real bug (and there is no other way), this is too risky. IOW, too late to change, we have to accept those values. thanks, Takashi
On Sun, Apr 23, 2023 at 09:30:11AM +0200, Takashi Iwai wrote: >On Sat, 22 Apr 2023 18:10:21 +0200, >Oswald Buddenhagen wrote: >> >> It's multi, not mono/stereo. >> >> AFAICT, this doesn't do anything in the kernel. > >... but those values are read by user-space. > >> Also, I think the subclass is meaningless for devices with just one >> stream, but whatever. > >Again, the value is read by user-space. > i assumed that much. but these are another thing that appears to have exactly zero useful documentation. >So changing both have clear influence on the user-space program, and >unless you have to change this for fixing a real bug (and there is no >other way), this is too risky. IOW, too late to change, we have to >accept those values. > there aren't any precedents for use of SNDRV_PCM_CLASS_MULTI and SNDRV_PCM_SUBCLASS_MULTI_MIX in the kernel tree. there don't appear to be relevant hits outside the kernel, either. it's conceivable that some code would check for the *_GENERIC enums, but i didn't find such code. so i'd postulate that these enums are effectively dead, and both the risk and the gain of this change are about zero. i suggest to initiate a formal deprecation procedure for the MULTI enum values, however that's supposed to look like. regards
On Mon, Apr 24, 2023 at 12:32:58PM +0200, Oswald Buddenhagen wrote: >so i'd postulate that these enums are effectively dead, and both the >risk and the gain of this change are about zero. speaking of dead code, lots of drivers explicitly assign info_flags = 0, dev_class = SNDRV_PCM_CLASS_GENERIC, and dev_subclass = SNDRV_PCM_SUBCLASS_GENERIC_MIX, which are all technically pointless, as the pcm struct is kzalloc'd anyway. and of course it's entirely inconsistent (with just about every combination present), because it obviously works without this. how would you feel about a patch that just removes all these? regards
On Tue, 25 Apr 2023 10:00:45 +0200, Oswald Buddenhagen wrote: > > On Mon, Apr 24, 2023 at 12:32:58PM +0200, Oswald Buddenhagen wrote: > > so i'd postulate that these enums are effectively dead, and both the > > risk and the gain of this change are about zero. > > speaking of dead code, lots of drivers explicitly assign info_flags = > 0, dev_class = SNDRV_PCM_CLASS_GENERIC, and dev_subclass = > SNDRV_PCM_SUBCLASS_GENERIC_MIX, which are all technically pointless, > as the pcm struct is kzalloc'd anyway. and of course it's entirely > inconsistent (with just about every combination present), because it > obviously works without this. how would you feel about a patch that > just removes all these? The dev_class and info_flags initializations to zero aren't entirely pointless, IMO. It explicitly shows that the default value is used. OTOH, dev_sub_class is always 0 for the existing drivers, hence dropping the explicit initialization would be an acceptable cleanup. thanks, Takashi
On Tue, Apr 25, 2023 at 10:15:36AM +0200, Takashi Iwai wrote: >On Tue, 25 Apr 2023 10:00:45 +0200, >Oswald Buddenhagen wrote: >> speaking of dead code, lots of drivers explicitly assign info_flags = >> 0, dev_class = SNDRV_PCM_CLASS_GENERIC, and dev_subclass = >> SNDRV_PCM_SUBCLASS_GENERIC_MIX, which are all technically pointless, >> as the pcm struct is kzalloc'd anyway. > >The dev_class and info_flags initializations to zero aren't entirely >pointless, IMO. It explicitly shows that the default value is used. > that can be said about many other fields as well. but defaults are there for a reason, to reduce the noise. not even the example in the docu includes it. and if we were consistent about the "be explicit" approach, we should add it to a lot of drivers which lack it - which feels just wrong. regards
diff --git a/sound/pci/emu10k1/emupcm.c b/sound/pci/emu10k1/emupcm.c index 87c3b19c6b2d..c04ef6ea188e 100644 --- a/sound/pci/emu10k1/emupcm.c +++ b/sound/pci/emu10k1/emupcm.c @@ -1377,7 +1377,8 @@ int snd_emu10k1_pcm_multi(struct snd_emu10k1 *emu, int device) snd_pcm_set_ops(pcm, SNDRV_PCM_STREAM_PLAYBACK, &snd_emu10k1_efx_playback_ops); pcm->info_flags = 0; - pcm->dev_subclass = SNDRV_PCM_SUBCLASS_GENERIC_MIX; + pcm->dev_class = SNDRV_PCM_CLASS_MULTI; + pcm->dev_subclass = SNDRV_PCM_SUBCLASS_MULTI_MIX; strcpy(pcm->name, "Multichannel Playback"); emu->pcm_multi = pcm;
It's multi, not mono/stereo. AFAICT, this doesn't do anything in the kernel. Also, I think the subclass is meaningless for devices with just one stream, but whatever. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> --- sound/pci/emu10k1/emupcm.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)