Re: [PATCH v2] staging: greybus: break multi statement macro into multiple lines

[Greg KH <gregkh@linuxfoundation.org>]

On Wed, Jul 17, 2024 at 10:32:07PM -0500, Juan José Arboleda wrote:
> The macro `gb_loopback_stats_attrs` in the Greybus loopback driver
> contains multiple statements, which should be enclosed in a do-while
> loop to ensure proper execution and adhere to coding standards.
> 
> The latter is not possible as the macro expansion will create invalid C
> syntax. Theres is not such thing like inner function definition inside a
> global scope do-while in C.
> 
> This patch nukes the `gb_loopback_stats_attrs` macro to enclose,
> addressing the style error flagged by checkpatch.pl.

The style error here is just wrong, the original code is fine.
Remember, checkpatch provides hints, it isn't always correct, and many
times, like this one, it's completely incorrect.

> 
> Signed-off-by: Juan José Arboleda <soyjuanarbol@gmail.com>
> ---
> Notes:
> 
> This new version uses a bit different approach so the commit message
> will be a bit different as well.
> 
> P.S: Thanks for being that patience and apologies for that false
> positive of building the non-included driver. Now it is tested-built.
> 
>  drivers/staging/greybus/loopback.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c
> index 4313d3bbc23a..88d86b084f28 100644
> --- a/drivers/staging/greybus/loopback.c
> +++ b/drivers/staging/greybus/loopback.c
> @@ -162,11 +162,6 @@ static ssize_t name##_avg_show(struct device *dev,		\
>  }									\
>  static DEVICE_ATTR_RO(name##_avg)
>  
> -#define gb_loopback_stats_attrs(field)				\
> -	gb_loopback_ro_stats_attr(field, min, u);		\
> -	gb_loopback_ro_stats_attr(field, max, u);		\
> -	gb_loopback_ro_avg_attr(field)
> -
>  #define gb_loopback_attr(field, type)					\
>  static ssize_t field##_show(struct device *dev,				\
>  			    struct device_attribute *attr,		\
> @@ -268,15 +263,29 @@ static void gb_loopback_check_attr(struct gb_loopback *gb)
>  }
>  
>  /* Time to send and receive one message */
> -gb_loopback_stats_attrs(latency);
> +gb_loopback_ro_stats_attr(latency, min, u);
> +gb_loopback_ro_stats_attr(latency, max, u);
> +gb_loopback_ro_avg_attr(latency);

Unrolling the macro like this does fix the warning, but at the expense
of more code size and manual work.  We write code first for us to
understand, and second for the compiler.  The original code is just
fine, no need for a change at all.

sorry,

greg k-h