[PATCH] staging: greybus: Reformat code in gb_operation_sync_timeout()

[PATCH] staging: greybus: Reformat code in gb_operation_sync_timeout()

[Thorsten Blum]

Remove any unnecessary curly braces and combine 'else' and 'if' to an
'else if' to improve the code's readability and reduce indentation.

Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/greybus/operation.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
index 8459e9bc0749..ba26504ccac3 100644
--- a/drivers/greybus/operation.c
+++ b/drivers/greybus/operation.c
@@ -1157,16 +1157,12 @@ int gb_operation_sync_timeout(struct gb_connection *connection, int type,
 		memcpy(operation->request->payload, request, request_size);
 
 	ret = gb_operation_request_send_sync_timeout(operation, timeout);
-	if (ret) {
+	if (ret)
 		dev_err(&connection->hd->dev,
 			"%s: synchronous operation id 0x%04x of type 0x%02x failed: %d\n",
 			connection->name, operation->id, type, ret);
-	} else {
-		if (response_size) {
-			memcpy(response, operation->response->payload,
-			       response_size);
-		}
-	}
+	else if (response_size)
+		memcpy(response, operation->response->payload, response_size);
 
 	gb_operation_put(operation);
 
-- 
2.49.0

Re: [PATCH] staging: greybus: Reformat code in gb_operation_sync_timeout()

[Johan Hovold]

On Sun, Apr 13, 2025 at 12:48:03PM +0200, Thorsten Blum wrote:
> Remove any unnecessary curly braces and combine 'else' and 'if' to an
> 'else if' to improve the code's readability and reduce indentation.
> 
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>  drivers/greybus/operation.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/greybus/operation.c b/drivers/greybus/operation.c
> index 8459e9bc0749..ba26504ccac3 100644
> --- a/drivers/greybus/operation.c
> +++ b/drivers/greybus/operation.c
> @@ -1157,16 +1157,12 @@ int gb_operation_sync_timeout(struct gb_connection *connection, int type,
>  		memcpy(operation->request->payload, request, request_size);
>  
>  	ret = gb_operation_request_send_sync_timeout(operation, timeout);
> -	if (ret) {
> +	if (ret)
>  		dev_err(&connection->hd->dev,
>  			"%s: synchronous operation id 0x%04x of type 0x%02x failed: %d\n",
>  			connection->name, operation->id, type, ret);
> -	} else {
> -		if (response_size) {
> -			memcpy(response, operation->response->payload,
> -			       response_size);
> -		}
> -	}
> +	else if (response_size)
> +		memcpy(response, operation->response->payload, response_size);

NAK

This just makes the code *less* readable.

Johan

Re: [PATCH] staging: greybus: Reformat code in gb_operation_sync_timeout()

[Thorsten Blum]

On 14. Apr 2025, at 09:05, Johan Hovold wrote:
> This just makes the code *less* readable.

I guess you prefer the code with curly braces?

What about the Linux kernel coding style [1]? Specifically "Do not
unnecessarily use braces where a single statement will do."

My patch just removes any unnecessary curly braces, resulting in less
lines of code and no line break in the memcpy() arguments.

Thanks,
Thorsten

[1] https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces

Re: [PATCH] staging: greybus: Reformat code in gb_operation_sync_timeout()

[Johan Hovold]

On Mon, Apr 14, 2025 at 05:00:41PM +0200, Thorsten Blum wrote:
> On 14. Apr 2025, at 09:05, Johan Hovold wrote:
> > This just makes the code *less* readable.
> 
> I guess you prefer the code with curly braces?

Around multiline statements yes, but the proposed if-else here also
obscures the logic for no good reason.

> What about the Linux kernel coding style [1]? Specifically "Do not
> unnecessarily use braces where a single statement will do."
> 
> My patch just removes any unnecessary curly braces, resulting in less
> lines of code and no line break in the memcpy() arguments.

I really don't care, the code is more readable as it stands which is
what matters.

If you want to practise sending patches you can send all the "cleanups"
you want for code in drivers/staging where churn like this may be
accepted (and where the core greybus code no longer lives, contrary to
what your Subject suggests).

Johan