[PATCH] Staging: greybus: Cleanup in header file control.h

[PATCH] Staging: greybus: Cleanup in header file control.h

[Madhumitha Prabakaran]

Fix a blank line after function/struct/union/enum
declarations. Also, convert to_gb_control()  macro into an inline
function in order to maintain Linux kernel coding style based
on which the inline function is preferable over the macro.

Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
---
 drivers/staging/greybus/control.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/staging/greybus/control.h b/drivers/staging/greybus/control.h
index 3a29ec05f631..a681ef74e7fe 100644
--- a/drivers/staging/greybus/control.h
+++ b/drivers/staging/greybus/control.h
@@ -24,7 +24,11 @@ struct gb_control {
 	char *vendor_string;
 	char *product_string;
 };
-#define to_gb_control(d) container_of(d, struct gb_control, dev)
+
+static inline struct gb_control *to_gb_control(struct device *d)
+{
+	return container_of(d, struct gb_control, dev);
+}
 
 struct gb_control *gb_control_create(struct gb_interface *intf);
 int gb_control_enable(struct gb_control *control);
-- 
2.17.1

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Johan Hovold]

On Fri, Apr 05, 2019 at 03:14:37PM -0500, Madhumitha Prabakaran wrote:
> Fix a blank line after function/struct/union/enum
> declarations. Also, convert to_gb_control()  macro into an inline
> function in order to maintain Linux kernel coding style based
> on which the inline function is preferable over the macro.

There are about 1200 macros wrapping container_of() in the kernel, so
this is a common pattern which does not need to be changed (by contrast,
only a handful container_of are wrapped by functions).

> Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> ---
>  drivers/staging/greybus/control.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/greybus/control.h b/drivers/staging/greybus/control.h
> index 3a29ec05f631..a681ef74e7fe 100644
> --- a/drivers/staging/greybus/control.h
> +++ b/drivers/staging/greybus/control.h
> @@ -24,7 +24,11 @@ struct gb_control {
>  	char *vendor_string;
>  	char *product_string;
>  };
> -#define to_gb_control(d) container_of(d, struct gb_control, dev)
> +

No need for a blank line either as the macro is closely related to the
preceding struct declaration.

> +static inline struct gb_control *to_gb_control(struct device *d)
> +{
> +	return container_of(d, struct gb_control, dev);
> +}

Johan

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Dan Carpenter]

On Mon, Apr 15, 2019 at 03:10:02PM +0200, Johan Hovold wrote:
> On Fri, Apr 05, 2019 at 03:14:37PM -0500, Madhumitha Prabakaran wrote:
> > Fix a blank line after function/struct/union/enum
> > declarations. Also, convert to_gb_control()  macro into an inline
> > function in order to maintain Linux kernel coding style based
> > on which the inline function is preferable over the macro.
> 
> There are about 1200 macros wrapping container_of() in the kernel, so
> this is a common pattern which does not need to be changed (by contrast,
> only a handful container_of are wrapped by functions).
> 

A handful?

$ git grep "return container_of" | wc -l
1856

I'm like Donald Trump because I must have really small hands compared
to the rest of y'all.

I would prefer to apply these sorts of patches unless it causes an
issue...  Functions *are* better than macros.  It's a minor improvement,
but this is staging and we make tons minor style changes.  It's part of
the blessing and curse of being in staging...

regards,
dan carpenter

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Johan Hovold]

On Mon, Apr 15, 2019 at 04:33:57PM +0300, Dan Carpenter wrote:
> On Mon, Apr 15, 2019 at 03:10:02PM +0200, Johan Hovold wrote:
> > On Fri, Apr 05, 2019 at 03:14:37PM -0500, Madhumitha Prabakaran wrote:
> > > Fix a blank line after function/struct/union/enum
> > > declarations. Also, convert to_gb_control()  macro into an inline
> > > function in order to maintain Linux kernel coding style based
> > > on which the inline function is preferable over the macro.
> > 
> > There are about 1200 macros wrapping container_of() in the kernel, so
> > this is a common pattern which does not need to be changed (by contrast,
> > only a handful container_of are wrapped by functions).
> > 
> 
> A handful?
> 
> $ git grep "return container_of" | wc -l
> 1856
> 
> I'm like Donald Trump because I must have really small hands compared
> to the rest of y'all.

Heh, you're right, my grep-fu failed me.

Ok, so both styles are roughly as common (and a non-trivial portion of
those 1800 also are not simple wrappers).

> I would prefer to apply these sorts of patches unless it causes an
> issue...  Functions *are* better than macros.  It's a minor improvement,
> but this is staging and we make tons minor style changes.  It's part of
> the blessing and curse of being in staging...

While I agree functions are generally preferred, container_of() is
special as it includes type checking.

And again, using container_of() like this is a common pattern in the
kernel.

Johan

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Dan Carpenter]

What I'm saying is that if we just apply it then we avoid the long
discussion forever.  The macro is OK, sure, but it's not like anyone is
going to come back later and argue that macros are better or preferred.

regards,
dan carpenter

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Johan Hovold]

On Mon, Apr 15, 2019 at 05:40:16PM +0300, Dan Carpenter wrote:
> What I'm saying is that if we just apply it then we avoid the long
> discussion forever.  The macro is OK, sure, but it's not like anyone is
> going to come back later and argue that macros are better or preferred.

That may be a valid argument, at least with respect to staging
currently (even if checkpatch isn't entirely too blame for this
particular "cleanup").

But then all eight or so instances should be replaced in one go, to
maintain consistency.

Johan

Re: [PATCH] Staging: greybus: Cleanup in header file control.h

[Dan Carpenter]

On Mon, Apr 15, 2019 at 05:03:33PM +0200, Johan Hovold wrote:
> But then all eight or so instances should be replaced in one go, to
> maintain consistency.

Yeah, that's fair enough, I suppose.

regards,
dan carepenter