[RFC 00/xx] ALSA: ALSA: add snd_stream_is_playback/capture()

[RFC 00/xx] ALSA: ALSA: add snd_stream_is_playback/capture()

[Kuninori Morimoto]

Hi Iwai-san, Mark

Current many drivers are using below code to know its direction.

	if (direction == SNDRV_PCM_STREAM_PLAYBACK)

I think it should be handled by function. But is it acceptable idea ?
Because it will be too many patch-set, I want to know it was acceptable
idea or not before posting patch-bomb.

I will post main patch, and sample driver patches.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

[RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Kuninori Morimoto]

Many drivers are using below code to know the direction.

	if (direction == SNDRV_PCM_STREAM_PLAYBACK)

Add snd_stream_is_playback/capture() macro to handle it.
It also adds snd_substream_is_playback/capture() to handle
snd_pcm_substream.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 include/sound/pcm.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 3edd7a7346daa..024dc2b337154 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -501,6 +501,25 @@ struct snd_pcm_substream {
 
 #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0)
 
+static inline int snd_stream_is_playback(int stream)
+{
+	return stream == SNDRV_PCM_STREAM_PLAYBACK;
+}
+
+static inline int snd_stream_is_capture(int stream)
+{
+	return stream == SNDRV_PCM_STREAM_CAPTURE;
+}
+
+static inline int snd_substream_is_playback(const struct snd_pcm_substream *substream)
+{
+	return snd_stream_is_playback(substream->stream);
+}
+
+static inline int snd_substream_is_capture(const struct snd_pcm_substream *substream)
+{
+	return snd_stream_is_capture(substream->stream);
+}
 
 struct snd_pcm_str {
 	int stream;				/* stream (direction) */
-- 
2.43.0

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Amadeusz Sławiński]

On 7/19/2024 1:34 AM, Kuninori Morimoto wrote:
> Many drivers are using below code to know the direction.
> 
> 	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> 
> Add snd_stream_is_playback/capture() macro to handle it.
> It also adds snd_substream_is_playback/capture() to handle
> snd_pcm_substream.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>   include/sound/pcm.h | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 3edd7a7346daa..024dc2b337154 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -501,6 +501,25 @@ struct snd_pcm_substream {
>   
>   #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0)
>   
> +static inline int snd_stream_is_playback(int stream)
> +{
> +	return stream == SNDRV_PCM_STREAM_PLAYBACK;
> +}
> +
> +static inline int snd_stream_is_capture(int stream)
> +{
> +	return stream == SNDRV_PCM_STREAM_CAPTURE;
> +}
> +
> +static inline int snd_substream_is_playback(const struct snd_pcm_substream *substream)
> +{
> +	return snd_stream_is_playback(substream->stream);
> +}
> +
> +static inline int snd_substream_is_capture(const struct snd_pcm_substream *substream)
> +{
> +	return snd_stream_is_capture(substream->stream);
> +}
>   
>   struct snd_pcm_str {
>   	int stream;				/* stream (direction) */

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?

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Kuninori Morimoto]

Hi Amadeusz

Thank you for comment

> > Many drivers are using below code to know the direction.
> > 
> > 	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> > 
> > Add snd_stream_is_playback/capture() macro to handle it.
> > It also adds snd_substream_is_playback/capture() to handle
> > snd_pcm_substream.
> > 
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > ---
(snip)
> 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?

Actually, I have tried _Generic() first, but changed to current style,
because many drivers are using own direction variable, and they are using
own variable types. But I think more again.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Kuninori Morimoto]

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;
}

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...

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Takashi Iwai]

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 :)


thanks,

Takashi

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Pierre-Louis Bossart]

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.

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Amadeusz Sławiński]

On 7/22/2024 10:47 AM, Pierre-Louis Bossart wrote:

> 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.
Oh yes, so I'm not the only one :D, I also would very much prefer if 
there was 'direction' instead of 'stream'.

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Kuninori Morimoto]

Hi Amadeusz, Pierre-Louis, Takashi

Thank you for your helps

> 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 :)
(snip)
> > 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.
(snip)
> Oh yes, so I'm not the only one :D, I also would very much prefer if 
> there was 'direction' instead of 'stream'.

For now, unfortunately, using _Generic() makes code more complex, because
many drivers are using own direction variables (with unsigned, const, short,
bitfield, etc), and _Generic() need to know it.

Not _Generic() code is not convenience, but not so bad (?).
If so, can below acceptable ?

	snd_pcm_direction_is_playback(const int direction);
	snd_pcm_direction_is_capture(const int direction);

	snd_pcm_substream_is_playback(const struct snd_pcm_substream *);
	snd_pcm_substream_is_capture(const struct snd_pcm_substream *);



... BTW, I noticed some drivers are using below, is there any difference ??

	substream->pstr->stream
	substream->stream


Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Amadeusz Sławiński]

On 7/22/2024 7:58 AM, 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;
> }
> 
> 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...
> 
> Thank you for your help !!
> 

My mistake in example, I've used function syntax, notice (x) at the end, 
if you remove it, it seems to build without need to call inline functions:

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 210096f124eed..e914fea59445e 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -496,6 +496,10 @@ struct snd_pcm_substream {

  #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0)

+#define snd_pcm_is_playback(x) _Generic((x), 
                \
+       int :                           (x == 
SNDRV_PCM_STREAM_PLAYBACK),               \
+       struct snd_pcm_substream * :    (x->stream == 
SNDRV_PCM_STREAM_PLAYBACK))
+

  struct snd_pcm_str {
         int stream;                             /* stream (direction) */
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
index 68aa8de2b0c4e..7e9f0ac6a5bc6 100644
--- a/sound/soc/intel/avs/pcm.c
+++ b/sound/soc/intel/avs/pcm.c
@@ -351,7 +351,7 @@ static int avs_dai_hda_be_hw_free(struct 
snd_pcm_substream *substream, struct sn
         data->path = NULL;

         /* clear link <-> stream mapping */
-       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+       if (snd_pcm_is_playback(substream))
                 snd_hdac_ext_bus_link_clear_stream_id(data->link,
 
hdac_stream(link_stream)->stream_tag);

@@ -383,7 +383,7 @@ static int avs_dai_hda_be_prepare(struct 
snd_pcm_substream *substream, struct sn
         snd_hdac_ext_stream_reset(link_stream);
         snd_hdac_ext_stream_setup(link_stream, format_val);

-       if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+       if (snd_pcm_is_playback(substream))
                 snd_hdac_ext_bus_link_set_stream_id(data->link,
 
hdac_stream(link_stream)->stream_tag);


I've test compiled the above fine.

As for ac97, seems like _Generic is impossible on bitfields, so perhaps 
just move it out of bitfield, to int?

Thanks,
Amadeusz

Re: [RFC 01/xx] ALSA: add snd_stream_is_playback/capture() macro

[Kuninori Morimoto]

Hi Amadeusz

Thank you for your help

> My mistake in example, I've used function syntax, notice (x) at the end, 
> if you remove it, it seems to build without need to call inline functions:

Thanks. I was aware of that.

Your example is calling snd_pcm_is_playback() with "snd_pcm_substream" only.
It works well indeed. But I will get error if I call it with "int", like below.
I don't know how to solve this issue and/or what does it mean...

${LINUX}/sound/soc/intel/avs/pcm.c: In function 'avs_dai_hda_be_prepare':                                           
${LINUX}/include/sound/pcm.h:506:40: error: invalid type argument of '->' (have 'int')                              
  506 |         struct snd_pcm_substream * : ((x)->stream == SNDRV_PCM_STREAM_PLAYBACK))                                                  
      |                                          ^~                                                                                       
${LINUX}/sound/soc/intel/avs/pcm.c:375:13: note: in expansion of macro 'snd_pcm_is_playback'
  375 |         if (snd_pcm_is_playback(substream->stream))                                                                             
      |             ^~~~~~~~~~~~~~~~~~~                                                                  


Below is the code. It is copied your example, and I updated it to use both
"int" and "snd_pcm_substream".

-	if (snd_pcm_is_playback(substream))
+	if (snd_pcm_is_playback(substream->stream))


----------------
diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 3edd7a7346da..a4916342f715 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -501,6 +501,9 @@ struct snd_pcm_substream {
 
 #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0)
 
+#define snd_pcm_is_playback(x) _Generic((x), \
+	int :                        ((x) == SNDRV_PCM_STREAM_PLAYBACK), \
+	struct snd_pcm_substream * : ((x)->stream == SNDRV_PCM_STREAM_PLAYBACK))
 
 struct snd_pcm_str {
 	int stream;				/* stream (direction) */
diff --git a/sound/soc/intel/avs/pcm.c b/sound/soc/intel/avs/pcm.c
index c76b86254a8b..79ae6a5df9c2 100644
--- a/sound/soc/intel/avs/pcm.c
+++ b/sound/soc/intel/avs/pcm.c
@@ -331,7 +331,7 @@ static int avs_dai_hda_be_hw_free(struct snd_pcm_substream *substream, struct sn
 	if (!link)
 		return -EINVAL;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		snd_hdac_ext_bus_link_clear_stream_id(link, hdac_stream(link_stream)->stream_tag);
 
 	return 0;
@@ -372,7 +372,7 @@ static int avs_dai_hda_be_prepare(struct snd_pcm_substream *substream, struct sn
 	if (!link)
 		return -EINVAL;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream->stream))
 		snd_hdac_ext_bus_link_set_stream_id(link, hdac_stream(link_stream)->stream_tag);
 
 	ret = avs_dai_prepare(substream, dai);
----------------


Thank you for your help !!

Best regards
---
Kuninori Morimoto

[RFC 02/xx] soundwire: intel: use snd_[sub]stream_is_playback/capture()

[Kuninori Morimoto]

We can use snd_[sub]stream_is_playback/capture(). Let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/soundwire/intel.c       | 4 ++--
 drivers/soundwire/intel_ace2x.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 01e1a0f3ec394..a87c8c8294d4e 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -736,7 +736,7 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
 		return -EIO;
 
 	ch = params_channels(params);
-	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+	if (snd_substream_is_capture(substream))
 		dir = SDW_DATA_DIR_RX;
 	else
 		dir = SDW_DATA_DIR_TX;
@@ -826,7 +826,7 @@ static int intel_prepare(struct snd_pcm_substream *substream,
 
 		/* configure stream */
 		ch = params_channels(hw_params);
-		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		if (snd_substream_is_capture(substream))
 			dir = SDW_DATA_DIR_RX;
 		else
 			dir = SDW_DATA_DIR_TX;
diff --git a/drivers/soundwire/intel_ace2x.c b/drivers/soundwire/intel_ace2x.c
index 8b1b6ad420cf1..b459795ee442d 100644
--- a/drivers/soundwire/intel_ace2x.c
+++ b/drivers/soundwire/intel_ace2x.c
@@ -304,7 +304,7 @@ static int intel_hw_params(struct snd_pcm_substream *substream,
 		return -EIO;
 
 	ch = params_channels(params);
-	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+	if (snd_substream_is_capture(substream))
 		dir = SDW_DATA_DIR_RX;
 	else
 		dir = SDW_DATA_DIR_TX;
@@ -398,7 +398,7 @@ static int intel_prepare(struct snd_pcm_substream *substream,
 
 		/* configure stream */
 		ch = params_channels(hw_params);
-		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		if (snd_substream_is_capture(substream))
 			dir = SDW_DATA_DIR_RX;
 		else
 			dir = SDW_DATA_DIR_TX;
-- 
2.43.0

[RFC 03/xx] ALSA: virtio: use snd_[sub]stream_is_playback/capture()

[Kuninori Morimoto]

We can use snd_[sub]stream_is_playback/capture(). Let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/virtio/virtio_card.h    | 2 +-
 sound/virtio/virtio_pcm_msg.c | 4 ++--
 sound/virtio/virtio_pcm_ops.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/virtio/virtio_card.h b/sound/virtio/virtio_card.h
index 3ceee4e416fc7..2febec09b13ab 100644
--- a/sound/virtio/virtio_card.h
+++ b/sound/virtio/virtio_card.h
@@ -107,7 +107,7 @@ virtsnd_rx_queue(struct virtio_snd *snd)
 static inline struct virtio_snd_queue *
 virtsnd_pcm_queue(struct virtio_pcm_substream *vss)
 {
-	if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_stream_is_playback(vss->direction))
 		return virtsnd_tx_queue(vss->snd);
 	else
 		return virtsnd_rx_queue(vss->snd);
diff --git a/sound/virtio/virtio_pcm_msg.c b/sound/virtio/virtio_pcm_msg.c
index 8c32efaf4c529..fd4111a558250 100644
--- a/sound/virtio/virtio_pcm_msg.c
+++ b/sound/virtio/virtio_pcm_msg.c
@@ -230,7 +230,7 @@ int virtsnd_pcm_msg_send(struct virtio_pcm_substream *vss, unsigned long offset,
 			msg->xfer.stream_id = cpu_to_le32(vss->sid);
 			memset(&msg->status, 0, sizeof(msg->status));
 
-			if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK)
+			if (snd_stream_is_playback(vss->direction))
 				rc = virtqueue_add_sgs(vqueue, psgs, 2, 1, msg,
 						       GFP_ATOMIC);
 			else
@@ -313,7 +313,7 @@ static void virtsnd_pcm_msg_complete(struct virtio_pcm_msg *msg,
 	 * If the capture substream returned an incorrect status, then just
 	 * increase the hw_ptr by the message size.
 	 */
-	if (vss->direction == SNDRV_PCM_STREAM_PLAYBACK ||
+	if (snd_stream_is_playback(vss->direction) ||
 	    written_bytes <= sizeof(msg->status))
 		vss->hw_ptr += msg->length;
 	else
diff --git a/sound/virtio/virtio_pcm_ops.c b/sound/virtio/virtio_pcm_ops.c
index ad12aae52fc32..6e8c4881b44f9 100644
--- a/sound/virtio/virtio_pcm_ops.c
+++ b/sound/virtio/virtio_pcm_ops.c
@@ -337,7 +337,7 @@ static int virtsnd_pcm_trigger(struct snd_pcm_substream *substream, int command)
 
 		spin_lock_irqsave(&queue->lock, flags);
 		spin_lock(&vss->lock);
-		if (vss->direction == SNDRV_PCM_STREAM_CAPTURE)
+		if (snd_stream_is_capture(vss->direction))
 			rc = virtsnd_pcm_msg_send(vss, 0, vss->buffer_bytes);
 		if (!rc)
 			vss->xfer_enabled = true;
-- 
2.43.0

[RFC 04/xx] ASoC: tegra: use snd_[sub]stream_is_playback/capture()

[Kuninori Morimoto]

We can use snd_[sub]stream_is_playback/capture(). Let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/tegra/tegra20_ac97.c    | 4 ++--
 sound/soc/tegra/tegra20_i2s.c     | 4 ++--
 sound/soc/tegra/tegra210_admaif.c | 2 +-
 sound/soc/tegra/tegra210_i2s.c    | 4 ++--
 sound/soc/tegra/tegra30_i2s.c     | 6 +++---
 sound/soc/tegra/tegra_pcm.c       | 2 +-
 6 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/sound/soc/tegra/tegra20_ac97.c b/sound/soc/tegra/tegra20_ac97.c
index 8011afe93c96e..c688d86966228 100644
--- a/sound/soc/tegra/tegra20_ac97.c
+++ b/sound/soc/tegra/tegra20_ac97.c
@@ -182,7 +182,7 @@ static int tegra20_ac97_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 	case SNDRV_PCM_TRIGGER_RESUME:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (snd_substream_is_playback(substream))
 			tegra20_ac97_start_playback(ac97);
 		else
 			tegra20_ac97_start_capture(ac97);
@@ -190,7 +190,7 @@ static int tegra20_ac97_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (snd_substream_is_playback(substream))
 			tegra20_ac97_stop_playback(ac97);
 		else
 			tegra20_ac97_stop_capture(ac97);
diff --git a/sound/soc/tegra/tegra20_i2s.c b/sound/soc/tegra/tegra20_i2s.c
index f11618e8f13ee..55bbddcf46516 100644
--- a/sound/soc/tegra/tegra20_i2s.c
+++ b/sound/soc/tegra/tegra20_i2s.c
@@ -232,7 +232,7 @@ static int tegra20_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 	case SNDRV_PCM_TRIGGER_RESUME:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (snd_substream_is_playback(substream))
 			tegra20_i2s_start_playback(i2s);
 		else
 			tegra20_i2s_start_capture(i2s);
@@ -240,7 +240,7 @@ static int tegra20_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (snd_substream_is_playback(substream))
 			tegra20_i2s_stop_playback(i2s);
 		else
 			tegra20_i2s_stop_capture(i2s);
diff --git a/sound/soc/tegra/tegra210_admaif.c b/sound/soc/tegra/tegra210_admaif.c
index 9f9334e480490..0e6a2e1cbc2f3 100644
--- a/sound/soc/tegra/tegra210_admaif.c
+++ b/sound/soc/tegra/tegra210_admaif.c
@@ -299,7 +299,7 @@ static int tegra_admaif_hw_params(struct snd_pcm_substream *substream,
 	cif_conf.client_ch = channels;
 	cif_conf.audio_ch = channels;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_substream_is_playback(substream)) {
 		path = ADMAIF_TX_PATH;
 		reg = CH_TX_REG(TEGRA_ADMAIF_CH_ACIF_TX_CTRL, dai->id);
 	} else {
diff --git a/sound/soc/tegra/tegra210_i2s.c b/sound/soc/tegra/tegra210_i2s.c
index fe4fde844d861..886528d81e985 100644
--- a/sound/soc/tegra/tegra210_i2s.c
+++ b/sound/soc/tegra/tegra210_i2s.c
@@ -673,12 +673,12 @@ static int tegra210_i2s_hw_params(struct snd_pcm_substream *substream,
 	srate = params_rate(params);
 
 	/* For playback I2S RX-CIF and for capture TX-CIF is used */
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_substream_is_playback(substream))
 		path = I2S_RX_PATH;
 	else
 		path = I2S_TX_PATH;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_substream_is_playback(substream)) {
 		unsigned int max_th;
 
 		/* FIFO threshold in terms of frames */
diff --git a/sound/soc/tegra/tegra30_i2s.c b/sound/soc/tegra/tegra30_i2s.c
index a8ff51d12edb5..6b1e1468ec806 100644
--- a/sound/soc/tegra/tegra30_i2s.c
+++ b/sound/soc/tegra/tegra30_i2s.c
@@ -188,7 +188,7 @@ static int tegra30_i2s_hw_params(struct snd_pcm_substream *substream,
 	cif_conf.truncate = 0;
 	cif_conf.mono_conv = 0;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_substream_is_playback(substream)) {
 		cif_conf.direction = TEGRA30_AUDIOCIF_DIRECTION_RX;
 		reg = TEGRA30_I2S_CIF_RX_CTRL;
 	} else {
@@ -244,7 +244,7 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
 	case SNDRV_PCM_TRIGGER_RESUME:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (snd_substream_is_playback(substream))
 			tegra30_i2s_start_playback(i2s);
 		else
 			tegra30_i2s_start_capture(i2s);
@@ -252,7 +252,7 @@ static int tegra30_i2s_trigger(struct snd_pcm_substream *substream, int cmd,
 	case SNDRV_PCM_TRIGGER_STOP:
 	case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
 	case SNDRV_PCM_TRIGGER_SUSPEND:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (snd_substream_is_playback(substream))
 			tegra30_i2s_stop_playback(i2s);
 		else
 			tegra30_i2s_stop_capture(i2s);
diff --git a/sound/soc/tegra/tegra_pcm.c b/sound/soc/tegra/tegra_pcm.c
index 4bdbcd2635ef5..996c0c6797516 100644
--- a/sound/soc/tegra/tegra_pcm.c
+++ b/sound/soc/tegra/tegra_pcm.c
@@ -164,7 +164,7 @@ int tegra_pcm_hw_params(struct snd_soc_component *component,
 		return ret;
 	}
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_substream_is_playback(substream)) {
 		slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
 		slave_config.dst_addr = dmap->addr;
 		slave_config.dst_maxburst = 8;
-- 
2.43.0

Re: [RFC 00/xx] ALSA: ALSA: add snd_stream_is_playback/capture()

[Takashi Sakamoto]

Hi,

On Thu, Jul 18, 2024 at 11:34:01PM +0000, Kuninori Morimoto wrote:
> Current many drivers are using below code to know its direction.
> 
> 	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> 
> I think it should be handled by function. But is it acceptable idea ?
> Because it will be too many patch-set, I want to know it was acceptable
> idea or not before posting patch-bomb.
> 
> I will post main patch, and sample driver patches.

It is better to rename these inline functions introduced in this
patchset so that they belong to PCM category, since in Linux sound
subsystem there is another type of substream in rawmidi category.

The concept of 'substream' corresponds to 'subdevice' in some operations
to PCM/RawMidi cdev, thus should be handled with enough care as much as
possible, in my opinion.


Regards

Takashi Sakamoto

Re: [RFC 00/xx] ALSA: ALSA: add snd_stream_is_playback/capture()

[Kuninori Morimoto]

Hi Sakamoto-san

> It is better to rename these inline functions introduced in this
> patchset so that they belong to PCM category, since in Linux sound
> subsystem there is another type of substream in rawmidi category.
> 
> The concept of 'substream' corresponds to 'subdevice' in some operations
> to PCM/RawMidi cdev, thus should be handled with enough care as much as
> possible, in my opinion.

Ah, indeed. Thank you for pointing it.
So something like

	snd_stream_is_playback()    -> snd_pcm_stream_is_playback()
	snd_substream_is_playback() -> snd_pcm_substream_is_playback()

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [RFC 00/xx] ALSA: ALSA: add snd_stream_is_playback/capture()

[Takashi Iwai]

On Fri, 19 Jul 2024 01:34:01 +0200,
Kuninori Morimoto wrote:
> 
> 
> Hi Iwai-san, Mark
> 
> Current many drivers are using below code to know its direction.
> 
> 	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> 
> I think it should be handled by function. But is it acceptable idea ?

Is the conversion just for readability / consistency reason?
Or would it bring other benefit like code safety?
Honestly speaking, I see no big advantage of conversion, if it's only
about the readability.

> Because it will be too many patch-set, I want to know it was acceptable
> idea or not before posting patch-bomb.

A generic macro like Amadeusz suggested would be an interesting idea,
and that can be seen as a cleanup.  But the straightforward conversion
for the mass, I don't know whether it's worth...


thanks,

Takashi

> 
> I will post main patch, and sample driver patches.
> 
> Thank you for your help !!
> 
> Best regards
> ---
> Kuninori Morimoto

Re: [RFC 00/xx] ALSA: ALSA: add snd_stream_is_playback/capture()

[Kuninori Morimoto]

Hi Takashi

Thank you for your reply

> Is the conversion just for readability / consistency reason?
> Or would it bring other benefit like code safety?
> Honestly speaking, I see no big advantage of conversion, if it's only
> about the readability.

Yes, it is just for readability, and I can agree that it is not so big
benefit. It is the reason why asked it before posting the patch-bomb.

> A generic macro like Amadeusz suggested would be an interesting idea,
> and that can be seen as a cleanup.  But the straightforward conversion
> for the mass, I don't know whether it's worth...

It can be adjusted to generic macro. I will post [RFC v2] patch.


Thank you for your help !!

Best regards
---
Kuninori Morimoto