On 3/31/25 4:33 PM, gpittala wrote:
This patch includes multiple meaningful cleanups for the Greybus staging driver:
- firmware.c: Replaced deprecated 'strncpy()' with 'strscpy()'
This is a good type of change to make.
- sysfs show functions: Replaced 'sprintf()' with 'sysfs_emit()'
This is also an improvement.
- loopback.c: Refactored a large function (gp_loopback_fn) to improve readability
I have only glanced at this, but refactoring something can sometimes be clearer if you do it in several small patches.
- audio_gb.c: Split logic in get_topology() into separate calls as per TODO
I'll comment more below, but you should almost always have only one change per patch. So each of the four items listed above deserves its own patch. You could send them separately (because they're unrelated), or as a series of cleanups.
Note that "one change per patch" is a logical (not literal) statement. For example, you could do a single patch that replaces *all* calls to strncpy() with strcspy().
All changes are tested and pass checkpatch.pl
Signed-off-by: gpittala ganeshkpittala@gmail.com
.../greybus/Documentation/firmware/firmware.c | 32 ++-- drivers/staging/greybus/arche-apb-ctrl.c | 11 +- drivers/staging/greybus/arche-platform.c | 11 +- drivers/staging/greybus/audio_gb.c | 37 +++- .../staging/greybus/audio_manager_module.c | 13 +- drivers/staging/greybus/gbphy.c | 3 +- drivers/staging/greybus/light.c | 5 +- drivers/staging/greybus/loopback.c | 170 ++++++++++-------- 8 files changed, 159 insertions(+), 123 deletions(-)
diff --git a/drivers/staging/greybus/Documentation/firmware/firmware.c b/drivers/staging/greybus/Documentation/firmware/firmware.c index 765d69faa9cc..8e375c88c881 100644 --- a/drivers/staging/greybus/Documentation/firmware/firmware.c +++ b/drivers/staging/greybus/Documentation/firmware/firmware.c @@ -47,12 +47,12 @@ static int update_intf_firmware(int fd) ret = ioctl(fd, FW_MGMT_IOC_GET_INTF_FW, &intf_fw_info); if (ret < 0) { printf("Failed to get interface firmware version: %s (%d)\n",
fwdev, ret);
fwdev, ret);
The two changes in this hunk are not mentioned in the description above. Please remove these changes. If you want to do reformatting like this, do it in a separate patch.
While it might be reasonable to include a little white space change like this occasionally, you should avoid doing it. It is unrelated, and complicates your patch unnecessarily.
This comment applies to several other changes you've made below. It also applies to removal (or addition) of blank lines, or really, any other white space changes.
-Alex
return -1;} printf("Interface Firmware tag (%s), major (%d), minor (%d)\n",
intf_fw_info.firmware_tag, intf_fw_info.major,
intf_fw_info.firmware_tag, intf_fw_info.major,/* Try Interface Firmware load over Unipro */
. . .