[RFC 00/xx] ALSA: ALSA: add snd_stream_is_playback/capture()
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
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) */
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?
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
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
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
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.
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'.
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
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
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
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;
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;
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;
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
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
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
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
participants (5)
-
Amadeusz Sławiński -
Kuninori Morimoto -
Pierre-Louis Bossart -
Takashi Iwai -
Takashi Sakamoto