[PATCH] staging: greybus: extract a fxn to improve clarity

[PATCH] staging: greybus: extract a fxn to improve clarity

[Mark Thomas Heim]

The gb_audio_gb_get_topology function at the top of the file
needs to be split per a TODO comment above the function. It
is necessary to refactor the code to pull out a method
that has fewer parameters to improve readability. A
prototype for the new function is now in the relevant header,
and the simpler function calls replace the old ones.

Signed-off-by: Mark Thomas Heim <questioneight@gmail.com>
---
 drivers/staging/greybus/audio_codec.h |  2 ++
 drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
 2 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
index ce15e800e607..a2e8361952b8 100644
--- a/drivers/staging/greybus/audio_codec.h
+++ b/drivers/staging/greybus/audio_codec.h
@@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
 void gbaudio_unregister_module(struct gbaudio_module_info *module);
 
 /* protocol related */
+int fetch_gb_audio_data(struct gb_connection *connection, int type,
+			void *response, int response_size);
 int gb_audio_gb_get_topology(struct gb_connection *connection,
 			     struct gb_audio_topology **topology);
 int gb_audio_gb_get_control(struct gb_connection *connection,
diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
index 9d8994fdb41a..3c924d13f0e7 100644
--- a/drivers/staging/greybus/audio_gb.c
+++ b/drivers/staging/greybus/audio_gb.c
@@ -8,7 +8,13 @@
 #include <linux/greybus.h>
 #include "audio_codec.h"
 
-/* TODO: Split into separate calls */
+int fetch_gb_audio_data(struct gb_connection *connection,
+			int type, void *response, int response_size)
+{
+	return gb_operation_sync(connection, type, NULL, 0,
+				 response, response_size);
+}
+
 int gb_audio_gb_get_topology(struct gb_connection *connection,
 			     struct gb_audio_topology **topology)
 {
@@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
 	u16 size;
 	int ret;
 
-	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
-				NULL, 0, &size_resp, sizeof(size_resp));
+	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
+				  &size_resp, sizeof(size_resp));
 	if (ret)
 		return ret;
-
 	size = le16_to_cpu(size_resp.size);
 	if (size < sizeof(*topo))
 		return -ENODATA;
-
 	topo = kzalloc(size, GFP_KERNEL);
 	if (!topo)
 		return -ENOMEM;
-
-	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
-				topo, size);
+	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY,
+				  topo, size);
 	if (ret) {
 		kfree(topo);
 		return ret;
 	}
-
 	*topology = topo;
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology);
-- 
2.25.1

Re: [PATCH] staging: greybus: extract a fxn to improve clarity

[Julia Lawall]

On Fri, 17 Mar 2023, Mark Thomas Heim wrote:

> The gb_audio_gb_get_topology function at the top of the file
> needs to be split per a TODO comment above the function. It
> is necessary to refactor the code to pull out a method
> that has fewer parameters to improve readability. A
> prototype for the new function is now in the relevant header,
> and the simpler function calls replace the old ones.

Mark,

Please go back and read the outreachy tutorial, in particular

https://kernelnewbies.org/PatchPhilosophy

The commit message is not written in the imperative, as it is required to
be.

Also, words like "needs to" and "necessary" express an opinion.  These
things may indeed be good things to do, but "needs to" and "necessary"
don't help to understand why the change is being made.

julia

>
> Signed-off-by: Mark Thomas Heim <questioneight@gmail.com>
> ---
>  drivers/staging/greybus/audio_codec.h |  2 ++
>  drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
>  2 files changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> index ce15e800e607..a2e8361952b8 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
>  void gbaudio_unregister_module(struct gbaudio_module_info *module);
>
>  /* protocol related */
> +int fetch_gb_audio_data(struct gb_connection *connection, int type,
> +			void *response, int response_size);
>  int gb_audio_gb_get_topology(struct gb_connection *connection,
>  			     struct gb_audio_topology **topology);
>  int gb_audio_gb_get_control(struct gb_connection *connection,
> diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> index 9d8994fdb41a..3c924d13f0e7 100644
> --- a/drivers/staging/greybus/audio_gb.c
> +++ b/drivers/staging/greybus/audio_gb.c
> @@ -8,7 +8,13 @@
>  #include <linux/greybus.h>
>  #include "audio_codec.h"
>
> -/* TODO: Split into separate calls */
> +int fetch_gb_audio_data(struct gb_connection *connection,
> +			int type, void *response, int response_size)
> +{
> +	return gb_operation_sync(connection, type, NULL, 0,
> +				 response, response_size);
> +}
> +
>  int gb_audio_gb_get_topology(struct gb_connection *connection,
>  			     struct gb_audio_topology **topology)
>  {
> @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
>  	u16 size;
>  	int ret;
>
> -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> -				NULL, 0, &size_resp, sizeof(size_resp));
> +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> +				  &size_resp, sizeof(size_resp));
>  	if (ret)
>  		return ret;
> -
>  	size = le16_to_cpu(size_resp.size);
>  	if (size < sizeof(*topo))
>  		return -ENODATA;
> -
>  	topo = kzalloc(size, GFP_KERNEL);
>  	if (!topo)
>  		return -ENOMEM;
> -
> -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
> -				topo, size);
> +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY,
> +				  topo, size);
>  	if (ret) {
>  		kfree(topo);
>  		return ret;
>  	}
> -
>  	*topology = topo;
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology);
> --
> 2.25.1
>
>
>

Re: [PATCH] staging: greybus: extract a fxn to improve clarity

[Greg Kroah-Hartman]

On Fri, Mar 17, 2023 at 08:17:56AM -0600, Mark Thomas Heim wrote:
> The gb_audio_gb_get_topology function at the top of the file
> needs to be split per a TODO comment above the function. It
> is necessary to refactor the code to pull out a method
> that has fewer parameters to improve readability. A
> prototype for the new function is now in the relevant header,
> and the simpler function calls replace the old ones.

Note, you have a full 72 characters to use for a changelog, please use
the whole line.

And what is "fxn" in the subject line?  Ironic you use an abbreviation
when trying to improve clarity :)

> Signed-off-by: Mark Thomas Heim <questioneight@gmail.com>
> ---
>  drivers/staging/greybus/audio_codec.h |  2 ++
>  drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> index ce15e800e607..a2e8361952b8 100644
> --- a/drivers/staging/greybus/audio_codec.h
> +++ b/drivers/staging/greybus/audio_codec.h
> @@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
>  void gbaudio_unregister_module(struct gbaudio_module_info *module);
>  
>  /* protocol related */
> +int fetch_gb_audio_data(struct gb_connection *connection, int type,
> +			void *response, int response_size);

Why is this a global function?

And why if it is a global function, are you not using the gb_audio_*
prefix?  Be aware of the global namespace please.

>  int gb_audio_gb_get_topology(struct gb_connection *connection,
>  			     struct gb_audio_topology **topology);
>  int gb_audio_gb_get_control(struct gb_connection *connection,
> diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> index 9d8994fdb41a..3c924d13f0e7 100644
> --- a/drivers/staging/greybus/audio_gb.c
> +++ b/drivers/staging/greybus/audio_gb.c
> @@ -8,7 +8,13 @@
>  #include <linux/greybus.h>
>  #include "audio_codec.h"
>  
> -/* TODO: Split into separate calls */
> +int fetch_gb_audio_data(struct gb_connection *connection,
> +			int type, void *response, int response_size)
> +{
> +	return gb_operation_sync(connection, type, NULL, 0,
> +				 response, response_size);
> +}
> +
>  int gb_audio_gb_get_topology(struct gb_connection *connection,
>  			     struct gb_audio_topology **topology)
>  {
> @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
>  	u16 size;
>  	int ret;
>  
> -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> -				NULL, 0, &size_resp, sizeof(size_resp));
> +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> +				  &size_resp, sizeof(size_resp));

What are you actually changing here besides the name?

How did this fix up the TODO at all?

confused,

greg k-h

Re: [PATCH] staging: greybus: extract a fxn to improve clarity

[Mark Heim]

On Fri, Mar 17, 2023 at 03:32:24PM +0100, Julia Lawall wrote:
> 
> 
> On Fri, 17 Mar 2023, Mark Thomas Heim wrote:
> 
> > The gb_audio_gb_get_topology function at the top of the file
> > needs to be split per a TODO comment above the function. It
> > is necessary to refactor the code to pull out a method
> > that has fewer parameters to improve readability. A
> > prototype for the new function is now in the relevant header,
> > and the simpler function calls replace the old ones.
> 
> Mark,
> 
> Please go back and read the outreachy tutorial, in particular
> 
> https://kernelnewbies.org/PatchPhilosophy
> 
> The commit message is not written in the imperative, as it is required to
> be.
> 

I indeed tried to make it imperative... which is why it was so awkward and is
why I used phrasing like "it is necessary to refactor" instead of saying flat
out "I refactored it" (the latter is definitely not imperative voice, but I
am still not sure I follow you). I always want to write in first person and
past tense, and I thought I made it as close as I could get. How does this sound:

"Refactor the gb_audio_gb_get_topology by extracting a function to improve
clarity. Create a helper function with fewer parameters to replace
tricky code. "

I'm not sure if that's any better. Is it like writing commands or
directions?

> Also, words like "needs to" and "necessary" express an opinion.  These
> things may indeed be good things to do, but "needs to" and "necessary"
> don't help to understand why the change is being made.
> 

Yes, I was trying to figure out how to write in the imperative voice but
got lost in the details. My first draft that I never submitted was out in
left field, but what I thought I got right in the draft that I actually
submitted for the patch had (a) no use of personal pronouns and (b) no
verbs in past tense. Are those two ingredients part of imperative voice?

> julia
> 
> >
> > Signed-off-by: Mark Thomas Heim <questioneight@gmail.com>
> > ---
> >  drivers/staging/greybus/audio_codec.h |  2 ++
> >  drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
> >  2 files changed, 13 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> > index ce15e800e607..a2e8361952b8 100644
> > --- a/drivers/staging/greybus/audio_codec.h
> > +++ b/drivers/staging/greybus/audio_codec.h
> > @@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
> >  void gbaudio_unregister_module(struct gbaudio_module_info *module);
> >
> >  /* protocol related */
> > +int fetch_gb_audio_data(struct gb_connection *connection, int type,
> > +			void *response, int response_size);
> >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> >  			     struct gb_audio_topology **topology);
> >  int gb_audio_gb_get_control(struct gb_connection *connection,
> > diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> > index 9d8994fdb41a..3c924d13f0e7 100644
> > --- a/drivers/staging/greybus/audio_gb.c
> > +++ b/drivers/staging/greybus/audio_gb.c
> > @@ -8,7 +8,13 @@
> >  #include <linux/greybus.h>
> >  #include "audio_codec.h"
> >
> > -/* TODO: Split into separate calls */
> > +int fetch_gb_audio_data(struct gb_connection *connection,
> > +			int type, void *response, int response_size)
> > +{
> > +	return gb_operation_sync(connection, type, NULL, 0,
> > +				 response, response_size);
> > +}
> > +
> >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> >  			     struct gb_audio_topology **topology)
> >  {
> > @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
> >  	u16 size;
> >  	int ret;
> >
> > -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > -				NULL, 0, &size_resp, sizeof(size_resp));
> > +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > +				  &size_resp, sizeof(size_resp));
> >  	if (ret)
> >  		return ret;
> > -
> >  	size = le16_to_cpu(size_resp.size);
> >  	if (size < sizeof(*topo))
> >  		return -ENODATA;
> > -
> >  	topo = kzalloc(size, GFP_KERNEL);
> >  	if (!topo)
> >  		return -ENOMEM;
> > -
> > -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
> > -				topo, size);
> > +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY,
> > +				  topo, size);
> >  	if (ret) {
> >  		kfree(topo);
> >  		return ret;
> >  	}
> > -
> >  	*topology = topo;
> > -
> >  	return 0;
> >  }
> >  EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology);
> > --
> > 2.25.1
> >
> >
> >

Re: [PATCH] staging: greybus: extract a fxn to improve clarity

[Julia Lawall]

> > The commit message is not written in the imperative, as it is required to
> > be.
>
> I indeed tried to make it imperative... which is why it was so awkward and is
> why I used phrasing like "it is necessary to refactor" instead of saying flat
> out "I refactored it" (the latter is definitely not imperative voice, but I
> am still not sure I follow you). I always want to write in first person and
> past tense, and I thought I made it as close as I could get. How does this sound:
>
> "Refactor the gb_audio_gb_get_topology by extracting a function to improve
> clarity. Create a helper function with fewer parameters to replace
> tricky code. "
>
> I'm not sure if that's any better. Is it like writing commands or
> directions?

This is now in the imperative.  "tricky code" still seems like an opinion.
You could probably be clearer about why you have made a particular choice.

>
> > Also, words like "needs to" and "necessary" express an opinion.  These
> > things may indeed be good things to do, but "needs to" and "necessary"
> > don't help to understand why the change is being made.
> >
>
> Yes, I was trying to figure out how to write in the imperative voice but
> got lost in the details. My first draft that I never submitted was out in
> left field, but what I thought I got right in the draft that I actually
> submitted for the patch had (a) no use of personal pronouns and (b) no
> verbs in past tense. Are those two ingredients part of imperative voice?

Not at all.  Imperative is like what you have above.  Just a verb in the
third person and no subject.  Look at patches from others, eg git log
--no-merges.  Not everyone follows the "use the imperative" rule to the
letter, but you will see many examples of clear and concise explanations
of what is done and why that thing is done that can help you think about
the problem in the right way.

Despite the deviations in practice, for outreachy you should always try to
use the imperative as much as possible.

julia

> > julia
> >
> > >
> > > Signed-off-by: Mark Thomas Heim <questioneight@gmail.com>
> > > ---
> > >  drivers/staging/greybus/audio_codec.h |  2 ++
> > >  drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
> > >  2 files changed, 13 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> > > index ce15e800e607..a2e8361952b8 100644
> > > --- a/drivers/staging/greybus/audio_codec.h
> > > +++ b/drivers/staging/greybus/audio_codec.h
> > > @@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
> > >  void gbaudio_unregister_module(struct gbaudio_module_info *module);
> > >
> > >  /* protocol related */
> > > +int fetch_gb_audio_data(struct gb_connection *connection, int type,
> > > +			void *response, int response_size);
> > >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  			     struct gb_audio_topology **topology);
> > >  int gb_audio_gb_get_control(struct gb_connection *connection,
> > > diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> > > index 9d8994fdb41a..3c924d13f0e7 100644
> > > --- a/drivers/staging/greybus/audio_gb.c
> > > +++ b/drivers/staging/greybus/audio_gb.c
> > > @@ -8,7 +8,13 @@
> > >  #include <linux/greybus.h>
> > >  #include "audio_codec.h"
> > >
> > > -/* TODO: Split into separate calls */
> > > +int fetch_gb_audio_data(struct gb_connection *connection,
> > > +			int type, void *response, int response_size)
> > > +{
> > > +	return gb_operation_sync(connection, type, NULL, 0,
> > > +				 response, response_size);
> > > +}
> > > +
> > >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  			     struct gb_audio_topology **topology)
> > >  {
> > > @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  	u16 size;
> > >  	int ret;
> > >
> > > -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > > -				NULL, 0, &size_resp, sizeof(size_resp));
> > > +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > > +				  &size_resp, sizeof(size_resp));
> > >  	if (ret)
> > >  		return ret;
> > > -
> > >  	size = le16_to_cpu(size_resp.size);
> > >  	if (size < sizeof(*topo))
> > >  		return -ENODATA;
> > > -
> > >  	topo = kzalloc(size, GFP_KERNEL);
> > >  	if (!topo)
> > >  		return -ENOMEM;
> > > -
> > > -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY, NULL, 0,
> > > -				topo, size);
> > > +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY,
> > > +				  topo, size);
> > >  	if (ret) {
> > >  		kfree(topo);
> > >  		return ret;
> > >  	}
> > > -
> > >  	*topology = topo;
> > > -
> > >  	return 0;
> > >  }
> > >  EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology);
> > > --
> > > 2.25.1
> > >
> > >
> > >
>

Re: [PATCH] staging: greybus: extract a fxn to improve clarity

[Mark Heim]

On Fri, Mar 17, 2023 at 03:33:21PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 17, 2023 at 08:17:56AM -0600, Mark Thomas Heim wrote:
> > The gb_audio_gb_get_topology function at the top of the file
> > needs to be split per a TODO comment above the function. It
> > is necessary to refactor the code to pull out a method
> > that has fewer parameters to improve readability. A
> > prototype for the new function is now in the relevant header,
> > and the simpler function calls replace the old ones.
> 
> Note, you have a full 72 characters to use for a changelog, please use
> the whole line.
> 

If I'm not mistaken, that's the first time this issue has appeared in
the Outreachy mailing board this round :-). I have learned a lot from
other people's mistakes and will try to avoid making new ones.

> And what is "fxn" in the subject line?  Ironic you use an abbreviation
> when trying to improve clarity :)
> 

I thought fxn was an acceptable abbreviation that saved me a few characters
in the subject line. I thought I was crunched for space.

> > Signed-off-by: Mark Thomas Heim <questioneight@gmail.com>
> > ---
> >  drivers/staging/greybus/audio_codec.h |  2 ++
> >  drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
> >  2 files changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> > index ce15e800e607..a2e8361952b8 100644
> > --- a/drivers/staging/greybus/audio_codec.h
> > +++ b/drivers/staging/greybus/audio_codec.h
> > @@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
> >  void gbaudio_unregister_module(struct gbaudio_module_info *module);
> >  
> >  /* protocol related */
> > +int fetch_gb_audio_data(struct gb_connection *connection, int type,
> > +			void *response, int response_size);
> 
> Why is this a global function?
> 

Global function? Do you mean the EXPORT_SYMBOL_GPL makes it global?
That's an issue since I was trying to imitate the structure of the
functions that were already in the C code, though I am not exactly sure
the functionality of that statement. I thought this function should
match.

> And why if it is a global function, are you not using the gb_audio_*
> prefix?  Be aware of the global namespace please.
> 

I thought namespaces were a C++ specific issue, not C. I think I was
ignorant of the C namespace until just now. I have used C++ namespaces
and Java packages, but wasn't aware of the C analogue.

> >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> >  			     struct gb_audio_topology **topology);
> >  int gb_audio_gb_get_control(struct gb_connection *connection,
> > diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> > index 9d8994fdb41a..3c924d13f0e7 100644
> > --- a/drivers/staging/greybus/audio_gb.c
> > +++ b/drivers/staging/greybus/audio_gb.c
> > @@ -8,7 +8,13 @@
> >  #include <linux/greybus.h>
> >  #include "audio_codec.h"
> >  
> > -/* TODO: Split into separate calls */
> > +int fetch_gb_audio_data(struct gb_connection *connection,
> > +			int type, void *response, int response_size)
> > +{
> > +	return gb_operation_sync(connection, type, NULL, 0,
> > +				 response, response_size);
> > +}
> > +
> >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> >  			     struct gb_audio_topology **topology)
> >  {
> > @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
> >  	u16 size;
> >  	int ret;
> >  
> > -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > -				NULL, 0, &size_resp, sizeof(size_resp));
> > +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > +				  &size_resp, sizeof(size_resp));
> 
> What are you actually changing here besides the name?
> 

In an earlier revision, I replaced each gb_operation_sync call with one
of two functions: fetch_gb_audio_data_size and fetch_gb_audio_data.
Next, I added in comments to explain what all the code was doing. To
satisfy the restriction that the computations done by the code were the
same as before the refactor, I would pass in 4 parameters into each
of the functions, which saves the issue of magic numbers being fed into 
gb_operation_sync. Then, the two new functions I consolidated down to 
1 function because the method bodies were identical. Finally, per the
code refactoring guides, I eliminated the comments.

I thought about adding in a function to encapsulate the code between
the two refactored methods, but you run into an issue with needing to
return too many values without doing something like defining a struct to
return those values. Thus, there's almost no change to the code.

> How did this fix up the TODO at all?
> 

It depends how you interpret the TODO, I suppose. My original intent
was to encapsulate code into some function calls with a helpful name, and
the end result of this refactor is more or less a U-turn. 

Should I update this patch or start something new? Any other feedback is
also welcome.

> confused,
> 
> greg k-h

Re: [PATCH] staging: greybus: extract a fxn to improve clarity

[Julia Lawall]

> Global function? Do you mean the EXPORT_SYMBOL_GPL makes it global?
> That's an issue since I was trying to imitate the structure of the
> functions that were already in the C code, though I am not exactly sure
> the functionality of that statement. I thought this function should
> match.

Global means that the function doesn't have static in its declaration.  So
it is visible to files other than this one.  EXPORT_SYMBOL has to do with
modules.

>
> > And why if it is a global function, are you not using the gb_audio_*
> > prefix?  Be aware of the global namespace please.
> >
>
> I thought namespaces were a C++ specific issue, not C. I think I was
> ignorant of the C namespace until just now. I have used C++ namespaces
> and Java packages, but wasn't aware of the C analogue.

The point is the concept of visible names, not a declaration.

julia

> > >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  			     struct gb_audio_topology **topology);
> > >  int gb_audio_gb_get_control(struct gb_connection *connection,
> > > diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> > > index 9d8994fdb41a..3c924d13f0e7 100644
> > > --- a/drivers/staging/greybus/audio_gb.c
> > > +++ b/drivers/staging/greybus/audio_gb.c
> > > @@ -8,7 +8,13 @@
> > >  #include <linux/greybus.h>
> > >  #include "audio_codec.h"
> > >
> > > -/* TODO: Split into separate calls */
> > > +int fetch_gb_audio_data(struct gb_connection *connection,
> > > +			int type, void *response, int response_size)
> > > +{
> > > +	return gb_operation_sync(connection, type, NULL, 0,
> > > +				 response, response_size);
> > > +}
> > > +
> > >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  			     struct gb_audio_topology **topology)
> > >  {
> > > @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  	u16 size;
> > >  	int ret;
> > >
> > > -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > > -				NULL, 0, &size_resp, sizeof(size_resp));
> > > +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > > +				  &size_resp, sizeof(size_resp));
> >
> > What are you actually changing here besides the name?
> >
>
> In an earlier revision, I replaced each gb_operation_sync call with one
> of two functions: fetch_gb_audio_data_size and fetch_gb_audio_data.
> Next, I added in comments to explain what all the code was doing. To
> satisfy the restriction that the computations done by the code were the
> same as before the refactor, I would pass in 4 parameters into each
> of the functions, which saves the issue of magic numbers being fed into
> gb_operation_sync. Then, the two new functions I consolidated down to
> 1 function because the method bodies were identical. Finally, per the
> code refactoring guides, I eliminated the comments.
>
> I thought about adding in a function to encapsulate the code between
> the two refactored methods, but you run into an issue with needing to
> return too many values without doing something like defining a struct to
> return those values. Thus, there's almost no change to the code.
>
> > How did this fix up the TODO at all?
> >
>
> It depends how you interpret the TODO, I suppose. My original intent
> was to encapsulate code into some function calls with a helpful name, and
> the end result of this refactor is more or less a U-turn.
>
> Should I update this patch or start something new? Any other feedback is
> also welcome.
>
> > confused,
> >
> > greg k-h
>
>

Re: [PATCH] staging: greybus: extract a fxn to improve clarity

[Greg Kroah-Hartman]

On Fri, Mar 17, 2023 at 11:25:42AM -0600, Mark Heim wrote:
> On Fri, Mar 17, 2023 at 03:33:21PM +0100, Greg Kroah-Hartman wrote:
> > And what is "fxn" in the subject line?  Ironic you use an abbreviation
> > when trying to improve clarity :)
> > 
> 
> I thought fxn was an acceptable abbreviation that saved me a few characters
> in the subject line. I thought I was crunched for space.

You are, but you still have to make it legible.

> > > Signed-off-by: Mark Thomas Heim <questioneight@gmail.com>
> > > ---
> > >  drivers/staging/greybus/audio_codec.h |  2 ++
> > >  drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
> > >  2 files changed, 13 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> > > index ce15e800e607..a2e8361952b8 100644
> > > --- a/drivers/staging/greybus/audio_codec.h
> > > +++ b/drivers/staging/greybus/audio_codec.h
> > > @@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
> > >  void gbaudio_unregister_module(struct gbaudio_module_info *module);
> > >  
> > >  /* protocol related */
> > > +int fetch_gb_audio_data(struct gb_connection *connection, int type,
> > > +			void *response, int response_size);
> > 
> > Why is this a global function?
> > 
> 
> Global function? Do you mean the EXPORT_SYMBOL_GPL makes it global?

No, EXPORT_SYMBOL*() is to provide a symbol to a module.

The normal C namespace rules still apply here if you were to build this
code into the kernel not as a module, right?  There is only one
namespace for a C program, so be aware of that.

> That's an issue since I was trying to imitate the structure of the
> functions that were already in the C code, though I am not exactly sure
> the functionality of that statement. I thought this function should
> match.

For a static function perhaps, but that's not what you did here (also
you made it global for no apparent reason.)

> > And why if it is a global function, are you not using the gb_audio_*
> > prefix?  Be aware of the global namespace please.
> > 
> 
> I thought namespaces were a C++ specific issue, not C. I think I was
> ignorant of the C namespace until just now. I have used C++ namespaces
> and Java packages, but wasn't aware of the C analogue.

There is only one "namespace" in C programs, so be aware of that.

We do have the idea of "module namespaces" for symbols that are exported
for modules to use, but that's not being used here either so I will not
get into that.  There's documentation in the kernel about that if you
are curious.

> > >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  			     struct gb_audio_topology **topology);
> > >  int gb_audio_gb_get_control(struct gb_connection *connection,
> > > diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> > > index 9d8994fdb41a..3c924d13f0e7 100644
> > > --- a/drivers/staging/greybus/audio_gb.c
> > > +++ b/drivers/staging/greybus/audio_gb.c
> > > @@ -8,7 +8,13 @@
> > >  #include <linux/greybus.h>
> > >  #include "audio_codec.h"
> > >  
> > > -/* TODO: Split into separate calls */
> > > +int fetch_gb_audio_data(struct gb_connection *connection,
> > > +			int type, void *response, int response_size)
> > > +{
> > > +	return gb_operation_sync(connection, type, NULL, 0,
> > > +				 response, response_size);
> > > +}
> > > +
> > >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  			     struct gb_audio_topology **topology)
> > >  {
> > > @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
> > >  	u16 size;
> > >  	int ret;
> > >  
> > > -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > > -				NULL, 0, &size_resp, sizeof(size_resp));
> > > +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > > +				  &size_resp, sizeof(size_resp));
> > 
> > What are you actually changing here besides the name?
> > 
> 
> In an earlier revision, I replaced each gb_operation_sync call with one
> of two functions: fetch_gb_audio_data_size and fetch_gb_audio_data.
> Next, I added in comments to explain what all the code was doing. To
> satisfy the restriction that the computations done by the code were the
> same as before the refactor, I would pass in 4 parameters into each
> of the functions, which saves the issue of magic numbers being fed into 
> gb_operation_sync. Then, the two new functions I consolidated down to 
> 1 function because the method bodies were identical. Finally, per the
> code refactoring guides, I eliminated the comments.
> 
> I thought about adding in a function to encapsulate the code between
> the two refactored methods, but you run into an issue with needing to
> return too many values without doing something like defining a struct to
> return those values. Thus, there's almost no change to the code.
> 
> > How did this fix up the TODO at all?
> > 
> 
> It depends how you interpret the TODO, I suppose. My original intent
> was to encapsulate code into some function calls with a helpful name, and
> the end result of this refactor is more or less a U-turn. 
> 
> Should I update this patch or start something new? Any other feedback is
> also welcome.

If you want to update the patch to do this correctly, that's up to you,
but note that your current implementation is not correct.

Perhaps start out with something simple like coding style fixes.  There
are still loads of them to do in the codebase.

good luck!

greg k-h

Re: [PATCH] staging: greybus: extract a fxn to improve clarity

[Mark Heim]

On Sat, Mar 18, 2023 at 08:07:46AM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 17, 2023 at 11:25:42AM -0600, Mark Heim wrote:
> > On Fri, Mar 17, 2023 at 03:33:21PM +0100, Greg Kroah-Hartman wrote:
> > > And what is "fxn" in the subject line?  Ironic you use an abbreviation
> > > when trying to improve clarity :)
> > > 
> > 
> > I thought fxn was an acceptable abbreviation that saved me a few characters
> > in the subject line. I thought I was crunched for space.
> 
> You are, but you still have to make it legible.
> 

Okay, got it.

> > > > Signed-off-by: Mark Thomas Heim <questioneight@gmail.com>
> > > > ---
> > > >  drivers/staging/greybus/audio_codec.h |  2 ++
> > > >  drivers/staging/greybus/audio_gb.c    | 21 +++++++++++----------
> > > >  2 files changed, 13 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/staging/greybus/audio_codec.h b/drivers/staging/greybus/audio_codec.h
> > > > index ce15e800e607..a2e8361952b8 100644
> > > > --- a/drivers/staging/greybus/audio_codec.h
> > > > +++ b/drivers/staging/greybus/audio_codec.h
> > > > @@ -177,6 +177,8 @@ int gbaudio_register_module(struct gbaudio_module_info *module);
> > > >  void gbaudio_unregister_module(struct gbaudio_module_info *module);
> > > >  
> > > >  /* protocol related */
> > > > +int fetch_gb_audio_data(struct gb_connection *connection, int type,
> > > > +			void *response, int response_size);
> > > 
> > > Why is this a global function?
> > > 
> > 
> > Global function? Do you mean the EXPORT_SYMBOL_GPL makes it global?
> 
> No, EXPORT_SYMBOL*() is to provide a symbol to a module.
> 
> The normal C namespace rules still apply here if you were to build this
> code into the kernel not as a module, right?  There is only one
> namespace for a C program, so be aware of that.
> 

Okay, I'll have to keep that in mind.

> > That's an issue since I was trying to imitate the structure of the
> > functions that were already in the C code, though I am not exactly sure
> > the functionality of that statement. I thought this function should
> > match.
> 
> For a static function perhaps, but that's not what you did here (also
> you made it global for no apparent reason.)
> 
> > > And why if it is a global function, are you not using the gb_audio_*
> > > prefix?  Be aware of the global namespace please.
> > > 
> > 
> > I thought namespaces were a C++ specific issue, not C. I think I was
> > ignorant of the C namespace until just now. I have used C++ namespaces
> > and Java packages, but wasn't aware of the C analogue.
> 
> There is only one "namespace" in C programs, so be aware of that.
> 
> We do have the idea of "module namespaces" for symbols that are exported
> for modules to use, but that's not being used here either so I will not
> get into that.  There's documentation in the kernel about that if you
> are curious.
> 

I'll try to start with something else that is simpler for a first fix.

> > > >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> > > >  			     struct gb_audio_topology **topology);
> > > >  int gb_audio_gb_get_control(struct gb_connection *connection,
> > > > diff --git a/drivers/staging/greybus/audio_gb.c b/drivers/staging/greybus/audio_gb.c
> > > > index 9d8994fdb41a..3c924d13f0e7 100644
> > > > --- a/drivers/staging/greybus/audio_gb.c
> > > > +++ b/drivers/staging/greybus/audio_gb.c
> > > > @@ -8,7 +8,13 @@
> > > >  #include <linux/greybus.h>
> > > >  #include "audio_codec.h"
> > > >  
> > > > -/* TODO: Split into separate calls */
> > > > +int fetch_gb_audio_data(struct gb_connection *connection,
> > > > +			int type, void *response, int response_size)
> > > > +{
> > > > +	return gb_operation_sync(connection, type, NULL, 0,
> > > > +				 response, response_size);
> > > > +}
> > > > +
> > > >  int gb_audio_gb_get_topology(struct gb_connection *connection,
> > > >  			     struct gb_audio_topology **topology)
> > > >  {
> > > > @@ -17,28 +23,23 @@ int gb_audio_gb_get_topology(struct gb_connection *connection,
> > > >  	u16 size;
> > > >  	int ret;
> > > >  
> > > > -	ret = gb_operation_sync(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > > > -				NULL, 0, &size_resp, sizeof(size_resp));
> > > > +	ret = fetch_gb_audio_data(connection, GB_AUDIO_TYPE_GET_TOPOLOGY_SIZE,
> > > > +				  &size_resp, sizeof(size_resp));
> > > 
> > > What are you actually changing here besides the name?
> > > 
> > 
> > In an earlier revision, I replaced each gb_operation_sync call with one
> > of two functions: fetch_gb_audio_data_size and fetch_gb_audio_data.
> > Next, I added in comments to explain what all the code was doing. To
> > satisfy the restriction that the computations done by the code were the
> > same as before the refactor, I would pass in 4 parameters into each
> > of the functions, which saves the issue of magic numbers being fed into 
> > gb_operation_sync. Then, the two new functions I consolidated down to 
> > 1 function because the method bodies were identical. Finally, per the
> > code refactoring guides, I eliminated the comments.
> > 
> > I thought about adding in a function to encapsulate the code between
> > the two refactored methods, but you run into an issue with needing to
> > return too many values without doing something like defining a struct to
> > return those values. Thus, there's almost no change to the code.
> > 
> > > How did this fix up the TODO at all?
> > > 
> > 
> > It depends how you interpret the TODO, I suppose. My original intent
> > was to encapsulate code into some function calls with a helpful name, and
> > the end result of this refactor is more or less a U-turn. 
> > 
> > Should I update this patch or start something new? Any other feedback is
> > also welcome.
> 
> If you want to update the patch to do this correctly, that's up to you,
> but note that your current implementation is not correct.
> 
> Perhaps start out with something simple like coding style fixes.  There
> are still loads of them to do in the codebase.
> 

I have another idea in the works that I'll try regarding spelling fixes,
and that issue is probably a chance to make changes that is more suited for
beginners. It's mentioned elsewhere in the mailing list.

> good luck!
> 

Thank you for your repsonse.

> greg k-h