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

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

[Kuninori Morimoto]

Hi Iwai-san, Mark
Cc Sakamoto-san, Amadeusz, Pierre-Louis

It seems we can use _Generic() more simply, v2 RFC is using it.
I'm not 100% sure but unfortunately we can't use bit-field with _Generic()
in gcc (clang seems possible to handle it ?).
pci/ac97 is the only user of bit-field direction.

To avoid posting patch-bomb, I will post main patch and some sample patches
as RFC. Please review it.

Link: https://lore.kernel.org/r/87zfqel1g7.wl-kuninori.morimoto.gx@renesas.com

Thank you for your help !!

Best regards
---
Kuninori Morimoto

[PATCH 001/112] ALSA: add snd_pcm_is_playback/capture() macro

[Kuninori Morimoto]

Many drivers are using below code to know the direction.

	if (direction == SNDRV_PCM_STREAM_PLAYBACK)

Add snd_pcm_is_playback/capture() macro to handle it.

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

diff --git a/include/sound/pcm.h b/include/sound/pcm.h
index 3edd7a7346daa..b3d4a928e41a4 100644
--- a/include/sound/pcm.h
+++ b/include/sound/pcm.h
@@ -501,6 +501,41 @@ struct snd_pcm_substream {
 
 #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0)
 
+static inline int snd_pcm_direction_is_playback(const int stream)
+{
+	return stream == SNDRV_PCM_STREAM_PLAYBACK;
+}
+
+static inline int snd_pcm_direction_is_capture(const int stream)
+{
+	return stream == SNDRV_PCM_STREAM_CAPTURE;
+}
+
+static inline int snd_pcm_substream_is_playback(const struct snd_pcm_substream *substream)
+{
+	return snd_pcm_direction_is_playback(substream->stream);
+}
+
+static inline int snd_pcm_substream_is_capture(const struct snd_pcm_substream *substream)
+{
+	return snd_pcm_direction_is_capture(substream->stream);
+}
+
+#define snd_pcm_is_playback(x) _Generic((x), \
+		 int			: snd_pcm_direction_is_playback, \
+	unsigned int			: snd_pcm_direction_is_playback, \
+	unsigned char			: snd_pcm_direction_is_playback, \
+	unsigned short			: snd_pcm_direction_is_playback, \
+	      struct snd_pcm_substream *: snd_pcm_substream_is_playback, \
+	const struct snd_pcm_substream *: snd_pcm_substream_is_playback)(x)
+
+#define snd_pcm_is_capture(x) _Generic((x),				\
+		 int			: snd_pcm_direction_is_capture, \
+	unsigned int			: snd_pcm_direction_is_capture, \
+	unsigned char			: snd_pcm_direction_is_capture, \
+	unsigned short			: snd_pcm_direction_is_capture, \
+	      struct snd_pcm_substream *: snd_pcm_substream_is_capture, \
+	const struct snd_pcm_substream *: snd_pcm_substream_is_capture)(x)
 
 struct snd_pcm_str {
 	int stream;				/* stream (direction) */
-- 
2.43.0

Re: [PATCH 001/112] ALSA: add snd_pcm_is_playback/capture() macro

[Takashi Sakamoto]

Hi,

On Wed, Jul 24, 2024 at 01:59:48AM +0000, Kuninori Morimoto wrote:
> Many drivers are using below code to know the direction.
> 
> 	if (direction == SNDRV_PCM_STREAM_PLAYBACK)
> 
> Add snd_pcm_is_playback/capture() macro to handle it.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
>  include/sound/pcm.h | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/include/sound/pcm.h b/include/sound/pcm.h
> index 3edd7a7346daa..b3d4a928e41a4 100644
> --- a/include/sound/pcm.h
> +++ b/include/sound/pcm.h
> @@ -501,6 +501,41 @@ struct snd_pcm_substream {
>  
>  #define SUBSTREAM_BUSY(substream) ((substream)->ref_count > 0)
>  
> +static inline int snd_pcm_direction_is_playback(const int stream)
> +{
> +	return stream == SNDRV_PCM_STREAM_PLAYBACK;
> +}
> +
> +static inline int snd_pcm_direction_is_capture(const int stream)
> +{
> +	return stream == SNDRV_PCM_STREAM_CAPTURE;
> +}
> +
> +static inline int snd_pcm_substream_is_playback(const struct snd_pcm_substream *substream)
> +{
> +	return snd_pcm_direction_is_playback(substream->stream);
> +}
> +
> +static inline int snd_pcm_substream_is_capture(const struct snd_pcm_substream *substream)
> +{
> +	return snd_pcm_direction_is_capture(substream->stream);
> +}
> +
> +#define snd_pcm_is_playback(x) _Generic((x), \
> +		 int			: snd_pcm_direction_is_playback, \
> +	unsigned int			: snd_pcm_direction_is_playback, \
> +	unsigned char			: snd_pcm_direction_is_playback, \
> +	unsigned short			: snd_pcm_direction_is_playback, \
> +	      struct snd_pcm_substream *: snd_pcm_substream_is_playback, \
> +	const struct snd_pcm_substream *: snd_pcm_substream_is_playback)(x)
> +
> +#define snd_pcm_is_capture(x) _Generic((x),				\
> +		 int			: snd_pcm_direction_is_capture, \
> +	unsigned int			: snd_pcm_direction_is_capture, \
> +	unsigned char			: snd_pcm_direction_is_capture, \
> +	unsigned short			: snd_pcm_direction_is_capture, \
> +	      struct snd_pcm_substream *: snd_pcm_substream_is_capture, \
> +	const struct snd_pcm_substream *: snd_pcm_substream_is_capture)(x)
>  
>  struct snd_pcm_str {
>  	int stream;				/* stream (direction) */
> -- 
> 2.43.0

In my opinion, it is not so important to distinguish some types which can
be converted to integer implicitly/explicitly in the above case. The
'default' association is available in such case, like:

+#define snd_pcm_is_playback(x) _Generic((x), \
+	      struct snd_pcm_substream *: snd_pcm_substream_is_playback, \
+	const struct snd_pcm_substream *: snd_pcm_substream_is_playback, \
+	                         default: snd_pcm_direction_is_playback)(x)

The association would match [u|i][8|16|32|64] and f[32|64] types, and would
not match to any type of pointers. However, it depends on your preference.


Regards

Takashi Sakamoto

Re: [PATCH 001/112] ALSA: add snd_pcm_is_playback/capture() macro

[Kuninori Morimoto]

Hi Sakamoto-san

> > +#define snd_pcm_is_playback(x) _Generic((x), \
> > +		 int			: snd_pcm_direction_is_playback, \
> > +	unsigned int			: snd_pcm_direction_is_playback, \
> > +	unsigned char			: snd_pcm_direction_is_playback, \
> > +	unsigned short			: snd_pcm_direction_is_playback, \
> > +	      struct snd_pcm_substream *: snd_pcm_substream_is_playback, \
> > +	const struct snd_pcm_substream *: snd_pcm_substream_is_playback)(x)
(snip)
> In my opinion, it is not so important to distinguish some types which can
> be converted to integer implicitly/explicitly in the above case. The
> 'default' association is available in such case, like:
> 
> +#define snd_pcm_is_playback(x) _Generic((x), \
> +	      struct snd_pcm_substream *: snd_pcm_substream_is_playback, \
> +	const struct snd_pcm_substream *: snd_pcm_substream_is_playback, \
> +	                         default: snd_pcm_direction_is_playback)(x)
> 
> The association would match [u|i][8|16|32|64] and f[32|64] types, and would
> not match to any type of pointers. However, it depends on your preference.

Wow ! I didn't know _Generic() can use default, and more good things is that
it could handle bit-field, too!! It is perfect.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

[PATCH 015/112] ALSA: pci: ac97: use snd_pcm_is_playback/capture()

[Kuninori Morimoto]

We can use snd_pcm_is_playback/capture(). Let's use it.

Because it is using bit-field, can't use snd_pcm_is_playback(),
uses snd_pcm_direction_is_playback() directly.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/pci/ac97/ac97_pcm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/pci/ac97/ac97_pcm.c b/sound/pci/ac97/ac97_pcm.c
index 5fee8e89790fb..2d8276d2d32f4 100644
--- a/sound/pci/ac97/ac97_pcm.c
+++ b/sound/pci/ac97/ac97_pcm.c
@@ -150,7 +150,7 @@ static unsigned char get_slot_reg(struct ac97_pcm *pcm, unsigned short cidx,
 		return 0xff;
 	if (pcm->spdif)
 		return AC97_SPDIF; /* pseudo register */
-	if (pcm->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_direction_is_playback(pcm->stream))
 		return rate_reg_tables[dbl][pcm->r[dbl].rate_table[cidx]][slot - 3];
 	else
 		return rate_cregs[slot - 3];
@@ -512,7 +512,7 @@ int snd_ac97_pcm_assign(struct snd_ac97_bus *bus,
 			rpcm->rates &= rates;
 		}
 		/* for double rate, we check the first codec only */
-		if (pcm->stream == SNDRV_PCM_STREAM_PLAYBACK &&
+		if (snd_pcm_direction_is_playback(pcm->stream) &&
 		    bus->codec[0] && (bus->codec[0]->flags & AC97_DOUBLE_RATE) &&
 		    rate_table[pcm->stream][0] == 0) {
 			tmp = (1<<AC97_SLOT_PCM_LEFT) | (1<<AC97_SLOT_PCM_RIGHT) |
-- 
2.43.0

[PATCH 027/112] ALSA: pci: use snd_pcm_is_playback/capture()

[Kuninori Morimoto]

We can use snd_pcm_is_playback/capture(). Let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/pci/intel8x0.c | 2 +-
 sound/pci/maestro3.c | 6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/pci/intel8x0.c b/sound/pci/intel8x0.c
index dae3e15ba534d..d9a6a9477bccc 100644
--- a/sound/pci/intel8x0.c
+++ b/sound/pci/intel8x0.c
@@ -838,7 +838,7 @@ static int snd_intel8x0_ali_trigger(struct snd_pcm_substream *substream, int cmd
 		fallthrough;
 	case SNDRV_PCM_TRIGGER_START:
 	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		if (snd_pcm_is_playback(substream)) {
 			/* clear FIFO for synchronization of channels */
 			fifo = igetdword(chip, fiforeg[ichdev->ali_slot / 4]);
 			fifo &= ~(0xff << (ichdev->ali_slot % 4));  
diff --git a/sound/pci/maestro3.c b/sound/pci/maestro3.c
index f4d211970d7ec..28634b2d8e5bd 100644
--- a/sound/pci/maestro3.c
+++ b/sound/pci/maestro3.c
@@ -1130,7 +1130,7 @@ snd_m3_pcm_setup1(struct snd_m3 *chip, struct m3_dma *s, struct snd_pcm_substrea
 	int dsp_in_size, dsp_out_size, dsp_in_buffer, dsp_out_buffer;
 	struct snd_pcm_runtime *runtime = subs->runtime;
 
-	if (subs->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(subs)) {
 		dsp_in_size = MINISRC_IN_BUFFER_SIZE - (0x20 * 2);
 		dsp_out_size = MINISRC_OUT_BUFFER_SIZE - (0x20 * 2);
 	} else {
@@ -1416,7 +1416,7 @@ snd_m3_pcm_prepare(struct snd_pcm_substream *subs)
 
 	snd_m3_pcm_setup1(chip, s, subs);
 
-	if (subs->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(subs))
 		snd_m3_playback_setup(chip, s, subs);
 	else
 		snd_m3_capture_setup(chip, s, subs);
@@ -1724,7 +1724,7 @@ snd_m3_substream_open(struct snd_m3 *chip, struct snd_pcm_substream *subs)
 	s->substream = subs;
 
 	/* set list owners */
-	if (subs->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(subs)) {
 		s->index_list[0] = &chip->mixer_list;
 	} else
 		s->index_list[0] = &chip->adc1_list;
-- 
2.43.0

[PATCH 062/112] ASoC: intel: use snd_pcm_is_playback/capture()

[Kuninori Morimoto]

We can use snd_pcm_is_playback/capture(). Let's use it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 sound/soc/intel/atom/sst-atom-controls.c     |  2 +-
 sound/soc/intel/atom/sst/sst_drv_interface.c |  2 +-
 sound/soc/intel/boards/bdw-rt5650.c          |  2 +-
 sound/soc/intel/boards/sof_maxim_common.c    |  2 +-
 sound/soc/intel/boards/sof_sdw.c             |  6 ++---
 sound/soc/intel/boards/sof_sdw_maxim.c       |  2 +-
 sound/soc/intel/catpt/pcm.c                  |  4 +--
 sound/soc/intel/keembay/kmb_platform.c       | 28 ++++++++++----------
 sound/soc/intel/skylake/skl-pcm.c            | 14 +++++-----
 sound/soc/intel/skylake/skl-topology.c       | 18 ++++++-------
 10 files changed, 40 insertions(+), 40 deletions(-)

diff --git a/sound/soc/intel/atom/sst-atom-controls.c b/sound/soc/intel/atom/sst-atom-controls.c
index 38116c7587174..0c6ce403148f8 100644
--- a/sound/soc/intel/atom/sst-atom-controls.c
+++ b/sound/soc/intel/atom/sst-atom-controls.c
@@ -1333,7 +1333,7 @@ int sst_send_pipe_gains(struct snd_soc_dai *dai, int stream, int mute)
 	dev_dbg(dai->dev, "enter, dai-name=%s dir=%d\n", dai->name, stream);
 	dev_dbg(dai->dev, "Stream name=%s\n", w->name);
 
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(stream)) {
 		snd_soc_dapm_widget_for_each_sink_path(w, p) {
 			if (p->connected && !p->connected(w, p->sink))
 				continue;
diff --git a/sound/soc/intel/atom/sst/sst_drv_interface.c b/sound/soc/intel/atom/sst/sst_drv_interface.c
index dc31c2c8f54c8..f02ee7f48a2a4 100644
--- a/sound/soc/intel/atom/sst/sst_drv_interface.c
+++ b/sound/soc/intel/atom/sst/sst_drv_interface.c
@@ -487,7 +487,7 @@ static inline int sst_calc_tstamp(struct intel_sst_drv *ctx,
 			fw_tstamp->ring_buffer_counter);
 	dev_dbg(ctx->dev, "mrfld hardware_counter %llu in bytes\n",
 			 fw_tstamp->hardware_counter);
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		delay_bytes = (size_t) (fw_tstamp->ring_buffer_counter -
 					fw_tstamp->hardware_counter);
 	else
diff --git a/sound/soc/intel/boards/bdw-rt5650.c b/sound/soc/intel/boards/bdw-rt5650.c
index 3c7cee03a02e6..a5df4d3067d71 100644
--- a/sound/soc/intel/boards/bdw-rt5650.c
+++ b/sound/soc/intel/boards/bdw-rt5650.c
@@ -150,7 +150,7 @@ static int bdw_rt5650_fe_startup(struct snd_pcm_substream *substream)
 	struct snd_pcm_runtime *runtime = substream->runtime;
 
 	/* Board supports stereo and quad configurations for capture */
-	if (substream->stream != SNDRV_PCM_STREAM_CAPTURE)
+	if (!snd_pcm_is_capture(substream))
 		return 0;
 
 	runtime->hw.channels_max = 4;
diff --git a/sound/soc/intel/boards/sof_maxim_common.c b/sound/soc/intel/boards/sof_maxim_common.c
index fcc3b95e57a4f..f520442bbb096 100644
--- a/sound/soc/intel/boards/sof_maxim_common.c
+++ b/sound/soc/intel/boards/sof_maxim_common.c
@@ -196,7 +196,7 @@ static int max_98373_trigger(struct snd_pcm_substream *substream, int cmd)
 	int ret = 0;
 
 	/* set spk pin by playback only */
-	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+	if (snd_pcm_is_capture(substream))
 		return 0;
 
 	cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index e5feaef669d14..778190cbe3dbc 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -671,7 +671,7 @@ int sdw_hw_params(struct snd_pcm_substream *substream,
 		return 0;
 
 	/* Identical data will be sent to all codecs in playback */
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(substream)) {
 		ch_mask = GENMASK(ch - 1, 0);
 		step = 0;
 	} else {
@@ -1731,8 +1731,8 @@ static int create_sdw_dailink(struct snd_soc_card *card,
 
 		WARN_ON(i != num_cpus || j != num_codecs);
 
-		playback = (stream == SNDRV_PCM_STREAM_PLAYBACK);
-		capture = (stream == SNDRV_PCM_STREAM_CAPTURE);
+		playback = snd_pcm_is_playback(stream);
+		capture  = snd_pcm_is_capture(stream);
 
 		init_dai_link(dev, *dai_links, be_id, name, playback, capture,
 			      cpus, num_cpus, codecs, num_codecs,
diff --git a/sound/soc/intel/boards/sof_sdw_maxim.c b/sound/soc/intel/boards/sof_sdw_maxim.c
index b7f73177867f4..b3d51d345964f 100644
--- a/sound/soc/intel/boards/sof_sdw_maxim.c
+++ b/sound/soc/intel/boards/sof_sdw_maxim.c
@@ -51,7 +51,7 @@ static int mx8373_enable_spk_pin(struct snd_pcm_substream *substream, bool enabl
 	int j;
 
 	/* set spk pin by playback only */
-	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+	if (snd_pcm_is_capture(substream))
 		return 0;
 
 	cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
diff --git a/sound/soc/intel/catpt/pcm.c b/sound/soc/intel/catpt/pcm.c
index 81a2f0339e055..c32c101e65b9c 100644
--- a/sound/soc/intel/catpt/pcm.c
+++ b/sound/soc/intel/catpt/pcm.c
@@ -83,11 +83,11 @@ catpt_get_stream_template(struct snd_pcm_substream *substream)
 	/* account for capture in bidirectional dais */
 	switch (type) {
 	case CATPT_STRM_TYPE_SYSTEM:
-		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		if (snd_pcm_is_capture(substream))
 			type = CATPT_STRM_TYPE_CAPTURE;
 		break;
 	case CATPT_STRM_TYPE_BLUETOOTH_RENDER:
-		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+		if (snd_pcm_is_capture(substream))
 			type = CATPT_STRM_TYPE_BLUETOOTH_CAPTURE;
 		break;
 	default:
diff --git a/sound/soc/intel/keembay/kmb_platform.c b/sound/soc/intel/keembay/kmb_platform.c
index 37ea2e1d2e922..eab7b8223b51b 100644
--- a/sound/soc/intel/keembay/kmb_platform.c
+++ b/sound/soc/intel/keembay/kmb_platform.c
@@ -170,7 +170,7 @@ static inline void kmb_i2s_disable_channels(struct kmb_i2s_info *kmb_i2s,
 	u32 i;
 
 	/* Disable all channels regardless of configuration*/
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(stream)) {
 		for (i = 0; i < MAX_ISR; i++)
 			writel(0, kmb_i2s->i2s_base + TER(i));
 	} else {
@@ -184,7 +184,7 @@ static inline void kmb_i2s_clear_irqs(struct kmb_i2s_info *kmb_i2s, u32 stream)
 	struct i2s_clk_config_data *config = &kmb_i2s->config;
 	u32 i;
 
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(stream)) {
 		for (i = 0; i < config->chan_nr / 2; i++)
 			readl(kmb_i2s->i2s_base + TOR(i));
 	} else {
@@ -199,7 +199,7 @@ static inline void kmb_i2s_irq_trigger(struct kmb_i2s_info *kmb_i2s,
 	u32 i, irq;
 	u32 flag;
 
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(stream))
 		flag = TX_INT_FLAG;
 	else
 		flag = RX_INT_FLAG;
@@ -270,7 +270,7 @@ static int kmb_pcm_trigger(struct snd_soc_component *component,
 
 	switch (cmd) {
 	case SNDRV_PCM_TRIGGER_START:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		if (snd_pcm_is_playback(substream)) {
 			kmb_i2s->tx_ptr = 0;
 			kmb_i2s->tx_substream = substream;
 		} else {
@@ -279,7 +279,7 @@ static int kmb_pcm_trigger(struct snd_soc_component *component,
 		}
 		break;
 	case SNDRV_PCM_TRIGGER_STOP:
-		if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+		if (snd_pcm_is_playback(substream))
 			kmb_i2s->tx_substream = NULL;
 		else
 			kmb_i2s->rx_substream = NULL;
@@ -378,7 +378,7 @@ static snd_pcm_uframes_t kmb_pcm_pointer(struct snd_soc_component *component,
 	struct kmb_i2s_info *kmb_i2s = runtime->private_data;
 	snd_pcm_uframes_t pos;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		pos = kmb_i2s->tx_ptr;
 	else
 		pos = kmb_i2s->rx_ptr;
@@ -419,7 +419,7 @@ static inline void kmb_i2s_enable_dma(struct kmb_i2s_info *kmb_i2s, u32 stream)
 
 	dma_reg = readl(kmb_i2s->i2s_base + I2S_DMACR);
 	/* Enable DMA handshake for stream */
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(stream))
 		dma_reg |= I2S_DMAEN_TXBLOCK;
 	else
 		dma_reg |= I2S_DMAEN_RXBLOCK;
@@ -433,7 +433,7 @@ static inline void kmb_i2s_disable_dma(struct kmb_i2s_info *kmb_i2s, u32 stream)
 
 	dma_reg = readl(kmb_i2s->i2s_base + I2S_DMACR);
 	/* Disable DMA handshake for stream */
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(stream)) {
 		dma_reg &= ~I2S_DMAEN_TXBLOCK;
 		writel(1, kmb_i2s->i2s_base + I2S_RTXDMA);
 	} else {
@@ -451,7 +451,7 @@ static void kmb_i2s_start(struct kmb_i2s_info *kmb_i2s,
 	/* I2S Programming sequence in Keem_Bay_VPU_DB_v1.1 */
 	writel(1, kmb_i2s->i2s_base + IER);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		writel(1, kmb_i2s->i2s_base + ITER);
 	else
 		writel(1, kmb_i2s->i2s_base + IRER);
@@ -474,7 +474,7 @@ static void kmb_i2s_stop(struct kmb_i2s_info *kmb_i2s,
 	/* I2S Programming sequence in Keem_Bay_VPU_DB_v1.1 */
 	kmb_i2s_clear_irqs(kmb_i2s, substream->stream);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		writel(0, kmb_i2s->i2s_base + ITER);
 	else
 		writel(0, kmb_i2s->i2s_base + IRER);
@@ -556,7 +556,7 @@ static void kmb_i2s_config(struct kmb_i2s_info *kmb_i2s, int stream)
 	kmb_i2s_disable_channels(kmb_i2s, stream);
 
 	for (ch_reg = 0; ch_reg < config->chan_nr / 2; ch_reg++) {
-		if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		if (snd_pcm_is_playback(stream)) {
 			writel(kmb_i2s->xfer_resolution,
 			       kmb_i2s->i2s_base + TCR(ch_reg));
 
@@ -678,7 +678,7 @@ static int kmb_dai_prepare(struct snd_pcm_substream *substream,
 {
 	struct kmb_i2s_info *kmb_i2s = snd_soc_dai_get_drvdata(cpu_dai);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		writel(1, kmb_i2s->i2s_base + TXFFR);
 	else
 		writel(1, kmb_i2s->i2s_base + RXFFR);
@@ -695,7 +695,7 @@ static int kmb_dai_startup(struct snd_pcm_substream *substream,
 	if (kmb_i2s->use_pio)
 		return 0;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		dma_data = &kmb_i2s->play_dma_data;
 	else
 		dma_data = &kmb_i2s->capture_dma_data;
@@ -713,7 +713,7 @@ static int kmb_dai_hw_free(struct snd_pcm_substream *substream,
 	if (kmb_i2s->use_pio)
 		kmb_i2s_clear_irqs(kmb_i2s, substream->stream);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		writel(0, kmb_i2s->i2s_base + ITER);
 	else
 		writel(0, kmb_i2s->i2s_base + IRER);
diff --git a/sound/soc/intel/skylake/skl-pcm.c b/sound/soc/intel/skylake/skl-pcm.c
index 613b27b8da134..2ffd511eedfe4 100644
--- a/sound/soc/intel/skylake/skl-pcm.c
+++ b/sound/soc/intel/skylake/skl-pcm.c
@@ -189,7 +189,7 @@ int skl_pcm_link_dma_prepare(struct device *dev, struct skl_pipe_params *params)
 	snd_hdac_ext_stream_setup(stream, format_val);
 
 	stream_tag = hstream->stream_tag;
-	if (stream->hstream.direction == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(stream->hstream.direction)) {
 		list_for_each_entry(link, &bus->hlink_list, list) {
 			if (link->index == params->link_index)
 				snd_hdac_ext_bus_link_set_stream_id(link,
@@ -225,7 +225,7 @@ static int skl_pcm_open(struct snd_pcm_substream *substream,
 	 * disable WALLCLOCK timestamps for capture streams
 	 * until we figure out how to handle digital inputs
 	 */
-	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+	if (snd_pcm_is_capture(substream)) {
 		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_WALL_CLOCK; /* legacy */
 		runtime->hw.info &= ~SNDRV_PCM_INFO_HAS_LINK_ATIME;
 	}
@@ -319,7 +319,7 @@ static int skl_pcm_hw_params(struct snd_pcm_substream *substream,
 	p_params.host_dma_id = dma_id;
 	p_params.stream = substream->stream;
 	p_params.format = params_format(params);
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		p_params.host_bps = dai->driver->playback.sig_bits;
 	else
 		p_params.host_bps = dai->driver->capture.sig_bits;
@@ -574,7 +574,7 @@ static int skl_link_hw_params(struct snd_pcm_substream *substream,
 	p_params.link_index = link->index;
 	p_params.format = params_format(params);
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(substream))
 		p_params.link_bps = codec_dai->driver->playback.sig_bits;
 	else
 		p_params.link_bps = codec_dai->driver->capture.sig_bits;
@@ -645,7 +645,7 @@ static int skl_link_hw_free(struct snd_pcm_substream *substream,
 	if (!link)
 		return -EINVAL;
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(substream)) {
 		stream_tag = hdac_stream(link_dev)->stream_tag;
 		snd_hdac_ext_bus_link_clear_stream_id(link, stream_tag);
 	}
@@ -1193,7 +1193,7 @@ static snd_pcm_uframes_t skl_platform_soc_pointer(
 	 * or greater than period boundary.
 	 */
 
-	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(substream)) {
 		pos = readl(bus->remap_addr + AZX_REG_VS_SDXDPIB_XBASE +
 				(AZX_REG_VS_SDXDPIB_XINTERVAL *
 				hdac_stream(hstream)->index));
@@ -1226,7 +1226,7 @@ static u64 skl_adjust_codec_delay(struct snd_pcm_substream *substream,
 	codec_nsecs = div_u64(codec_frames * 1000000000LL,
 			      substream->runtime->rate);
 
-	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE)
+	if (snd_pcm_is_capture(substream))
 		return nsec + codec_nsecs;
 
 	return (nsec > codec_nsecs) ? nsec - codec_nsecs : 0;
diff --git a/sound/soc/intel/skylake/skl-topology.c b/sound/soc/intel/skylake/skl-topology.c
index 602ef43211221..cb51b98b92c9a 100644
--- a/sound/soc/intel/skylake/skl-topology.c
+++ b/sound/soc/intel/skylake/skl-topology.c
@@ -198,7 +198,7 @@ static void skl_tplg_update_params_fixup(struct skl_module_cfg *m_cfg,
 	in_fmt = &m_cfg->module->formats[m_cfg->fmt_idx].inputs[0].fmt;
 	out_fmt = &m_cfg->module->formats[m_cfg->fmt_idx].outputs[0].fmt;
 
-	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(params->stream)) {
 		if (is_fe) {
 			in_fixup = m_cfg->params_fixup;
 			out_fixup = (~m_cfg->converter) &
@@ -618,9 +618,9 @@ skl_tplg_get_pipe_config(struct skl_dev *skl, struct skl_module_cfg *mconfig)
 	}
 
 	if ((pipe->conn_type == SKL_PIPE_CONN_TYPE_FE &&
-	     pipe->direction == SNDRV_PCM_STREAM_PLAYBACK) ||
+	     snd_pcm_is_playback(pipe->direction)) ||
 	     (pipe->conn_type == SKL_PIPE_CONN_TYPE_BE &&
-	     pipe->direction == SNDRV_PCM_STREAM_CAPTURE))
+	      snd_pcm_is_capture(pipe->direction)))
 		in_fmt = true;
 
 	for (i = 0; i < pipe->nr_cfgs; i++) {
@@ -1612,7 +1612,7 @@ int skl_tplg_update_pipe_params(struct device *dev,
 	if (skl->nr_modules)
 		return 0;
 
-	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(params->stream))
 		format = &mconfig->module->formats[mconfig->fmt_idx].inputs[0].fmt;
 	else
 		format = &mconfig->module->formats[mconfig->fmt_idx].outputs[0].fmt;
@@ -1642,7 +1642,7 @@ int skl_tplg_update_pipe_params(struct device *dev,
 		return -EINVAL;
 	}
 
-	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(params->stream)) {
 		res->ibs = (format->s_freq / 1000) *
 				(format->channels) *
 				(format->bit_depth >> 3);
@@ -1666,7 +1666,7 @@ skl_tplg_fe_get_cpr_module(struct snd_soc_dai *dai, int stream)
 	struct snd_soc_dapm_widget *w = snd_soc_dai_get_widget(dai, stream);
 	struct snd_soc_dapm_path *p = NULL;
 
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(stream)) {
 		snd_soc_dapm_widget_for_each_sink_path(w, p) {
 			if (p->connect && p->sink->power &&
 				!is_skl_dsp_widget_type(p->sink, dai->dev))
@@ -1745,7 +1745,7 @@ skl_tplg_be_get_cpr_module(struct snd_soc_dai *dai, int stream)
 	struct snd_soc_dapm_widget *w = snd_soc_dai_get_widget(dai, stream);
 	struct skl_module_cfg *mconfig;
 
-	if (stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(stream)) {
 		mconfig = skl_get_mconfig_pb_cpr(dai, w);
 	} else {
 		mconfig = skl_get_mconfig_cap_cpr(dai, w);
@@ -1813,7 +1813,7 @@ static int skl_tplg_be_fill_pipe_params(struct snd_soc_dai *dai,
 		goto err;
 
 	dev_dbg(skl->dev, "%s using pipe config: %d\n", __func__, pipe->cur_config_idx);
-	if (pipe->direction == SNDRV_PCM_STREAM_PLAYBACK)
+	if (snd_pcm_is_playback(pipe->direction))
 		pipe_fmt = &pipe->configs[pipe->cur_config_idx].out_fmt;
 	else
 		pipe_fmt = &pipe->configs[pipe->cur_config_idx].in_fmt;
@@ -1903,7 +1903,7 @@ int skl_tplg_be_update_params(struct snd_soc_dai *dai,
 {
 	struct snd_soc_dapm_widget *w = snd_soc_dai_get_widget(dai, params->stream);
 
-	if (params->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+	if (snd_pcm_is_playback(params->stream)) {
 		return skl_tplg_be_set_src_pipe_params(dai, w, params);
 	} else {
 		return skl_tplg_be_set_sink_pipe_params(dai, w, params);
-- 
2.43.0

Re: [PATCH 062/112] ASoC: intel: use snd_pcm_is_playback/capture()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 168 bytes --]

On Wed, Jul 24, 2024 at 02:00:15AM +0000, Kuninori Morimoto wrote:
> We can use snd_pcm_is_playback/capture(). Let's use it.

Acked-by: Mark Brown <broonie@kernel.org>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Jaroslav Kysela]

Dne 24. 07. 24 v 3:59 Kuninori Morimoto napsal(a):
> 
> Hi Iwai-san, Mark
> Cc Sakamoto-san, Amadeusz, Pierre-Louis
> 
> It seems we can use _Generic() more simply, v2 RFC is using it.
> I'm not 100% sure but unfortunately we can't use bit-field with _Generic()
> in gcc (clang seems possible to handle it ?).
> pci/ac97 is the only user of bit-field direction.
> 
> To avoid posting patch-bomb, I will post main patch and some sample patches
> as RFC. Please review it.
> 
> Link: https://lore.kernel.org/r/87zfqel1g7.wl-kuninori.morimoto.gx@renesas.com
> 
> Thank you for your help !!

Hi,

	my opinion is that this massive patch-set is just an overdesign. Even using 
the _Generic macro does not bring anything new and the code readability is not 
improved. If we add such macros for all simple cases (condition expressions), 
the code will be unreadable at some point.

					Jaroslav

-- 
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.

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

[Kuninori Morimoto]

Hi Jaroslav
Cc Takashi, Mark

Thank you for your comment

> my opinion is that this massive patch-set is just an overdesign. Even using
> the _Generic macro does not bring anything new and the code readability is not 
> improved. If we add such macros for all simple cases (condition expressions), 
> the code will be unreadable at some point.

I can understand that there are pros and cons.

I don't like redundant long and hard-to-read code to getting simple result.
If there is no way to do it, then there is no other way, but if there is an
easy way to shorten and easy-to-read it, then I would like to use it.

But Yes, I can agree that it will be huge patch-set, and indeed this code
does nothing.

Thank you for your help !!

Best regards
---
Kuninori Morimoto