Hi Pavel

+#define for_each_dpcm_fe(be, stream, dpcm) \
+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)

It should be:

+#define for_each_dpcm_fe(be, stream, dpcm, list_de) \
+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)

[accessing variables not passed as macro arguments is strange/surprising].

Ahh, OK. But, I would say, Yes and No.

Indeed, using not passed parameter in macro is strange.
But, this time, it is used at list_for_each_entry().
Last one is struct location/member, not parameter.

In this case, in order to use properly,
macro name is indicating "for what".

We have xx_fe and xx_be.

#define for_each_dpcm_fe(be, stream, _dpcm) \
list_for_each_entry(_dpcm, &(be)->dpcm[stream].fe_clients, list_fe)

#define for_each_dpcm_be(fe, stream, _dpcm) \
list_for_each_entry(_dpcm, &(fe)->dpcm[stream].be_clients, list_be)



Thank you for your help !!
Best regards
---
Kuninori Morimoto

On Wed 2019-10-23 10:28:36, Kuninori Morimoto wrote:

Hi

--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -103,6 +103,9 @@ struct snd_soc_dpcm_runtime {
int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */ };

+#define for_each_dpcm_fe(be, stream, dpcm)

\

+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)
+

This macro is really confusing. dpcm is used as both control variable of the
loop and name of the field in *be.

Ohh, yes, indeed.
Thank you for pointing it. I will post fixup patch

Thanks!

Plus it relies on list_fe variable to be

present in the context including it... that's non-standard.

sorry I couldn't understand about this

Macro is:

+#define for_each_dpcm_fe(be, stream, dpcm) \
+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)

It should be:

+#define for_each_dpcm_fe(be, stream, dpcm, list_de) \
+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)

[accessing variables not passed as macro arguments is strange/surprising].

Oh and "&(be)->" can be written as "(be)." AFAICT.

This "&" is for fe_clients

Aha, sorry, I misparsed that one.
Pavel

--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Hi

--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -103,6 +103,9 @@ struct snd_soc_dpcm_runtime {
int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */ };

+#define for_each_dpcm_fe(be, stream, dpcm)

\

+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)
+

This macro is really confusing. dpcm is used as both control variable of the
loop and name of the field in *be.

Ohh, yes, indeed.
Thank you for pointing it. I will post fixup patch

Plus it relies on list_fe variable to be

present in the context including it... that's non-standard.

sorry I couldn't understand about this

Oh and "&(be)->" can be written as "(be)." AFAICT.

This "&" is for fe_clients

Thank you for your help !!
Best regards
---
Kuninori Morimoto

+ Morimoto-San,

Subject: Re: [PATCH 4.19.y-cip 16/57] ASoC: add for_each_dpcm_fe() macro

Hi!

--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -103,6 +103,9 @@ struct snd_soc_dpcm_runtime {
int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */ };

+#define for_each_dpcm_fe(be, stream, dpcm)

\

+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)
+

This macro is really confusing. dpcm is used as both control variable of the
loop and name of the field in *be. Plus it relies on list_fe variable to be
present in the context including it... that's non-standard.

Oh and "&(be)->" can be written as "(be)." AFAICT.

Morimoto-San,
Do you have any opinion on Pavel's findings?

Regards,
Biju

Hi!

--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -103,6 +103,9 @@ struct snd_soc_dpcm_runtime {
int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */
};

+#define for_each_dpcm_fe(be, stream, dpcm) \
+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)
+

This macro is really confusing. dpcm is used as both control variable
of the loop and name of the field in *be. Plus it relies on list_fe
variable to be present in the context including it... that's
non-standard.

Oh and "&(be)->" can be written as "(be)." AFAICT.

Best regards,
Pavel
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

From: Kuninori Morimoto <kuninori.morimoto.gx@...>

commit d2e24d64652bf9d272e5496ae8a562bc64facff3 upstream.

To be more readable code, this patch adds
new for_each_dpcm_fe() macro, and replace existing code to it.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@...>
Signed-off-by: Mark Brown <broonie@...>
Signed-off-by: Biju Das <biju.das@...>
---
include/sound/soc-dpcm.h | 3 +++
sound/soc/soc-pcm.c | 6 +++---
2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
index 9bb92f1..f130de6 100644
--- a/include/sound/soc-dpcm.h
+++ b/include/sound/soc-dpcm.h
@@ -103,6 +103,9 @@ struct snd_soc_dpcm_runtime {
int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */
};

+#define for_each_dpcm_fe(be, stream, dpcm) \
+ list_for_each_entry(dpcm, &(be)->dpcm[stream].fe_clients, list_fe)
+
/* can this BE stop and free */
int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
struct snd_soc_pcm_runtime *be, int stream);
diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index 06eedb5..14132f6 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -1255,7 +1255,7 @@ static void dpcm_be_reparent(struct snd_soc_pcm_runtime *fe,

be_substream = snd_soc_dpcm_get_substream(be, stream);

- list_for_each_entry(dpcm, &be->dpcm[stream].fe_clients, list_fe) {
+ for_each_dpcm_fe(be, stream, dpcm) {
if (dpcm->fe == fe)
continue;

@@ -3223,7 +3223,7 @@ int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
struct snd_soc_dpcm *dpcm;
int state;

- list_for_each_entry(dpcm, &be->dpcm[stream].fe_clients, list_fe) {
+ for_each_dpcm_fe(be, stream, dpcm) {

if (dpcm->fe == fe)
continue;
@@ -3250,7 +3250,7 @@ int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
struct snd_soc_dpcm *dpcm;
int state;

- list_for_each_entry(dpcm, &be->dpcm[stream].fe_clients, list_fe) {
+ for_each_dpcm_fe(be, stream, dpcm) {

if (dpcm->fe == fe)
continue;
--
2.7.4