Re: [PATCH] staging: greybus: Multiple cleanups and refactors

[Alex Elder]

On 3/31/25 4:33 PM, gpittala wrote:
> This patch includes multiple meaningful cleanups for the Greybus staging driver:
>
> 1. firmware.c: Replaced deprecated 'strncpy()' with 'strscpy()'

This is a good type of change to make.

> 2. sysfs show functions: Replaced 'sprintf()' with 'sysfs_emit()'

This is also an improvement.

> 3. 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.

> 4. 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@xxxxxxxxx>
> ---
>   .../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,
>   		intf_fw_info.minor);
>   
>   	/* Try Interface Firmware load over Unipro */
. . .