diff mbox series

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

Message ID 1571295929-47286-17-git-send-email-biju.das@bp.renesas.com (mailing list archive)
State Changes Requested
Headers show
Series Audio improvements/SSIU BUSIF/ | expand

Commit Message

Biju Das Oct. 17, 2019, 7:04 a.m. UTC 
From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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@renesas.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Biju Das <biju.das@bp.renesas.com>
---
 include/sound/soc-dpcm.h | 3 +++
 sound/soc/soc-pcm.c      | 6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

Comments

Pavel Machek Oct. 20, 2019, 9:48 a.m. UTC | #1 
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
Biju Das Oct. 21, 2019, 12:26 p.m. UTC | #2 
+ 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
Kuninori Morimoto Oct. 23, 2019, 1:28 a.m. UTC | #3 
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
Pavel Machek Nov. 18, 2019, 3:30 p.m. UTC | #4 
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
Kuninori Morimoto Nov. 19, 2019, 1:24 a.m. UTC | #5 
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
diff mbox series

Patch

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;