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.
