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);
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));
- 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);
- *topology = topo;
- return 0;
} EXPORT_SYMBOL_GPL(gb_audio_gb_get_topology); -- 2.25.1
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
-
Greg Kroah-Hartman -
Julia Lawall -
Mark Thomas Heim