Re: [PATCH v2] staging: greybus: Align function arguments

[Alex Elder]

On 10/17/26 3:44 AM, pavankumaryalagada@xxxxxxxxx wrote:
> From: Yalagada Pavan Kumar <pavankumaryalagada@xxxxxxxxx>
>
> Align the function arguments to follow the coding style and
> improve readability, addressing the alignment warning reported by
> checkpatch.pl
>
> Signed-off-by: Yalagada Pavan Kumar <pavankumaryalagada@xxxxxxxxx>

This patch is OK, and it's a good kind of thing to do for
getting your feet wet with sending out patches for review.

But I encourage you to try to really *read* and understand
the code, and see if you can find anything you think is wrong,
or some bit of code or comments that you think could be more
clear.

I just scanned through "loopback.c" and one minor change (to
*code*) you could try is using fsleep() in one place that
uses other functions to delay.

Another thing you could look for is printf-style format
specifiers.  I see more than one place where an unsigned
value uses %d (which, for large values, produce misleading
results).

These suggestions aren't especially complex, but my hope
is they lead you further into understanding the code and
not relying heavily on checkpatch.pl to find things.

Anyway, this change looks fine to me.

Reviewed-by: Alex Elder <elder@xxxxxxxxxxxx>
> ---
> v2:
>   - Rebased onto staging-next.
> v1: https://lore.kernel.org/outreachy/C560A15E-6F28-4FC8-8BE2-277B503340AD@xxxxxxxx/T/#t
>
>   drivers/staging/greybus/loopback.c | 3 ++-
>   drivers/staging/greybus/spilib.c   | 3 ++-
>   2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index fe48a687377a..efc69c10cffe 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -652,7 +652,8 @@ static int gb_loopback_request_handler(struct gb_operation *operation)
>   		}
>   
>   		if (!gb_operation_response_alloc(operation,
> -						 len + sizeof(*response), GFP_KERNEL)) {
> +						 len + sizeof(*response),
> +						 GFP_KERNEL)) {
>   			dev_err(dev, "error allocating response\n");
>   			return -ENOMEM;
>   		}
> diff --git a/drivers/staging/greybus/spilib.c b/drivers/staging/greybus/spilib.c
> index e4d1ae8308aa..c1982f3fb931 100644
> --- a/drivers/staging/greybus/spilib.c
> +++ b/drivers/staging/greybus/spilib.c
> @@ -160,7 +160,8 @@ static struct spi_transfer *get_next_xfer(struct spi_transfer *xfer,
>   
>   /* Routines to transfer data */
>   static struct gb_operation *gb_spi_operation_create(struct gb_spilib *spi,
> -		struct gb_connection *connection, struct spi_message *msg)
> +						    struct gb_connection *connection,
> +						    struct spi_message *msg)
>   {
>   	struct gb_spi_transfer_request *request;
>   	struct spi_device *dev = msg->spi;