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@xxxxxxxxx>
> ---
> 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
>
>
>
_______________________________________________
greybus-dev mailing list -- greybus-dev@xxxxxxxxxxxxxxxx
To unsubscribe send an email to greybus-dev-leave@xxxxxxxxxxxxxxxx
|