On 7/22/24 10:16, Takashi Iwai wrote:
On Mon, 22 Jul 2024 07:58:41 +0200, Kuninori Morimoto wrote:
Hi Amadeusz, Takashi
Perhaps you could use generics here, so you could have one caller for both cases?
Something like: #define snd_pcm_is_playback(x) _Generic((x), \ int : (x == SNDRV_PCM_STREAM_PLAYBACK), \ struct snd_pcm_substream *substream * : (x->stream == SNDRV_PCM_STREAM_PLAYBACK))(x) or just call the above functions in it?
Hmm... I couldn't compile above inline style. I need to create function, and use it on _Generic().
And then, _Generic() is very picky for variable sytle. This means I need to create function for "int" / "const int", "unsigned int", "const unsigned int"
static inline int snd_pcm_stream_is_playback_(const int stream) { return stream == SNDRV_PCM_STREAM_PLAYBACK; }
static inline int snd_pcm_stream_is_playback(int stream) { return stream == SNDRV_PCM_STREAM_PLAYBACK; }
static inline int snd_pcm_stream_is_playback_u(const unsigned int stream) { return stream == SNDRV_PCM_STREAM_PLAYBACK; }
static inline int snd_pcm_stream_is_playbacku(unsigned int stream) { return stream == SNDRV_PCM_STREAM_PLAYBACK; }
I really don't see any merit of those inline. If this simplifies the code, I'd agree, but that's adding just more code...
static inline int snd_pcm_substream_is_playback_(const struct snd_pcm_substream *substream) { return snd_pcm_stream_is_playback(substream->stream); }
static inline int snd_pcm_substream_is_playback(struct snd_pcm_substream *substream) { return snd_pcm_stream_is_playback(substream->stream); }
#define snd_pcm_is_playback(x) _Generic((x), \ const int : snd_pcm_stream_is_playback_, \ int : snd_pcm_stream_is_playback, \ const unsigned int : snd_pcm_stream_is_playback_u, \ unsigned int : snd_pcm_stream_is_playbacku, \ const struct snd_pcm_substream * : snd_pcm_substream_is_playback_, \ struct snd_pcm_substream * : snd_pcm_substream_is_playback)(x)
And I'm not sure how to handle "bit field" by _Generic.
${LINUX}/sound/pci/ac97/ac97_pcm.c:153:13: note: in expansion of macro 'snd_pcm_is_playback' 153 | if (snd_pcm_is_playback(pcm->stream)) | ^~~~~~~~~~~~~~~~~~~ ${LINUX}/sound/pci/ac97/ac97_pcm.c: In function 'snd_ac97_pcm_assign': ${LINUX}/include/sound/pcm.h:544:41: error: '_Generic' selector of type 'unsigned char:1' is not compatible with any association 544 | #define snd_pcm_is_playback(x) _Generic((x), \
Not using _Generic() is easy, "const int" version can handle everything (= "int", "const int", "unsigned int", "const unsigned int", "short", "const short", "bit field", etc).
If there is better _Generic() grammar, I can follow it. If there wasn't, unfortunately let's give up this conversion...
IMO, if the retrieval of the stream direction and its evaluation are done separately, there is little use of the inline functions like the above. It doesn't give a better readability nor code safety.
That said, as of now, I'm not much convinced to go further. OTOH, I'm not strongly against taking this kind of change -- if other people do find it absolutely better, I'm ready to be convinced :)
The main issue I have with this patch is the continued confusion in variable naming between 'stream' and 'direction'. It's problematic on multiple platforms where a stream is typically identified by a DMA channel or ID of some sort. There's also the SoundWire 'stream' which has nothing to do with PCM devices. In the end people end-up drowning in too many 'streams', no one knows if the code refers to the data flow or the data direction.