[PATCH v2] Staging: greybus: Cleanup in greybus driver

[PATCH v2] Staging: greybus: Cleanup in greybus driver

[Madhumitha Prabakaran]

Fix a blank line after structure declarations. Also, convert
macros into inline functions in order to maintain Linux kernel
coding style based on which the inline function is
preferable over the macro.

Blank line fixes are suggested by checkpatch.pl

Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>

Changes in v2:
- To maintain consistency in driver greybus, all the instances of macro
with container_of are fixed in a single patch.
---
 drivers/staging/greybus/bundle.h    |  6 +++++-
 drivers/staging/greybus/control.h   |  6 +++++-
 drivers/staging/greybus/gbphy.h     | 12 ++++++++++--
 drivers/staging/greybus/greybus.h   |  6 +++++-
 drivers/staging/greybus/hd.h        |  6 +++++-
 drivers/staging/greybus/interface.h |  6 +++++-
 drivers/staging/greybus/module.h    |  6 +++++-
 drivers/staging/greybus/svc.h       |  6 +++++-
 8 files changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/greybus/bundle.h b/drivers/staging/greybus/bundle.h
index 8734d2055657..84956eedb1c4 100644
--- a/drivers/staging/greybus/bundle.h
+++ b/drivers/staging/greybus/bundle.h
@@ -31,7 +31,11 @@ struct gb_bundle {
 
 	struct list_head	links;	/* interface->bundles */
 };
-#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
+
+static inline struct gb_bundle *to_gb_bundle(struct device *d)
+{
+	return container_of(d, struct gb_bundle, dev);
+}
 
 /* Greybus "private" definitions" */
 struct gb_bundle *gb_bundle_create(struct gb_interface *intf, u8 bundle_id,
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);
diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h
index 99463489d7d6..20307f6dfcb9 100644
--- a/drivers/staging/greybus/gbphy.h
+++ b/drivers/staging/greybus/gbphy.h
@@ -15,7 +15,11 @@ struct gbphy_device {
 	struct list_head list;
 	struct device dev;
 };
-#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
+
+static inline struct gbphy_device *to_gbphy_dev(struct device *d)
+{
+	return container_of(d, struct gbphy_device, dev);
+}
 
 static inline void *gb_gbphy_get_data(struct gbphy_device *gdev)
 {
@@ -43,7 +47,11 @@ struct gbphy_driver {
 
 	struct device_driver driver;
 };
-#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver)
+
+static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *d)
+{
+	return container_of(d, struct gbphy_driver, driver);
+}
 
 int gb_gbphy_register_driver(struct gbphy_driver *driver,
 			     struct module *owner, const char *mod_name);
diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h
index d03ddb7c9df0..a82d5002b4d5 100644
--- a/drivers/staging/greybus/greybus.h
+++ b/drivers/staging/greybus/greybus.h
@@ -64,7 +64,11 @@ struct greybus_driver {
 
 	struct device_driver driver;
 };
-#define to_greybus_driver(d) container_of(d, struct greybus_driver, driver)
+
+static inline struct greybus_driver *to_greybus_driver(struct device_driver *d)
+{
+	return container_of(d, struct greybus_driver, driver);
+}
 
 static inline void greybus_set_drvdata(struct gb_bundle *bundle, void *data)
 {
diff --git a/drivers/staging/greybus/hd.h b/drivers/staging/greybus/hd.h
index 6cf024a20a58..de7c49d05266 100644
--- a/drivers/staging/greybus/hd.h
+++ b/drivers/staging/greybus/hd.h
@@ -57,7 +57,11 @@ struct gb_host_device {
 	/* Private data for the host driver */
 	unsigned long hd_priv[0] __aligned(sizeof(s64));
 };
-#define to_gb_host_device(d) container_of(d, struct gb_host_device, dev)
+
+static inline struct gb_host_device *to_gb_host_device(struct device *d)
+{
+	return container_of(d, struct gb_host_device, dev);
+}
 
 int gb_hd_cport_reserve(struct gb_host_device *hd, u16 cport_id);
 void gb_hd_cport_release_reserved(struct gb_host_device *hd, u16 cport_id);
diff --git a/drivers/staging/greybus/interface.h b/drivers/staging/greybus/interface.h
index 1c00c5bb3ec9..f86c0d596dbe 100644
--- a/drivers/staging/greybus/interface.h
+++ b/drivers/staging/greybus/interface.h
@@ -63,7 +63,11 @@ struct gb_interface {
 	struct work_struct mode_switch_work;
 	struct completion mode_switch_completion;
 };
-#define to_gb_interface(d) container_of(d, struct gb_interface, dev)
+
+static inline struct gb_interface *to_gb_interface(struct device *d)
+{
+	return container_of(d, struct gb_interface, dev);
+}
 
 struct gb_interface *gb_interface_create(struct gb_module *module,
 					 u8 interface_id);
diff --git a/drivers/staging/greybus/module.h b/drivers/staging/greybus/module.h
index b1ebcc6636db..c427b788b677 100644
--- a/drivers/staging/greybus/module.h
+++ b/drivers/staging/greybus/module.h
@@ -22,7 +22,11 @@ struct gb_module {
 
 	struct gb_interface *interfaces[0];
 };
-#define to_gb_module(d) container_of(d, struct gb_module, dev)
+
+static inline struct gb_module *to_gb_module(struct device *d)
+{
+	return container_of(d, struct gb_module, dev);
+}
 
 struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id,
 				   size_t num_interfaces);
diff --git a/drivers/staging/greybus/svc.h b/drivers/staging/greybus/svc.h
index ad01783bac9c..4e35ac9ed0ff 100644
--- a/drivers/staging/greybus/svc.h
+++ b/drivers/staging/greybus/svc.h
@@ -52,7 +52,11 @@ struct gb_svc {
 	struct dentry *debugfs_dentry;
 	struct svc_debugfs_pwrmon_rail *pwrmon_rails;
 };
-#define to_gb_svc(d) container_of(d, struct gb_svc, dev)
+
+static inline struct gb_svc *to_gb_svc(struct device *d)
+{
+	return container_of(d, struct gb_svc, dev);
+}
 
 struct gb_svc *gb_svc_create(struct gb_host_device *hd);
 int gb_svc_add(struct gb_svc *svc);
-- 
2.17.1

Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Viresh Kumar]

On 16-04-19, 17:13, Madhumitha Prabakaran wrote:
> Fix a blank line after structure declarations. Also, convert
> macros into inline functions in order to maintain Linux kernel
> coding style based on which the inline function is
> preferable over the macro.
> 
> Blank line fixes are suggested by checkpatch.pl

This was all intentional and we use such macros everywhere in kernel. No need
for such a patch, sorry.

-- 
viresh

Re: [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Greg KH]

On Tue, Apr 16, 2019 at 05:13:18PM -0500, Madhumitha Prabakaran wrote:
> Fix a blank line after structure declarations. Also, convert
> macros into inline functions in order to maintain Linux kernel
> coding style based on which the inline function is
> preferable over the macro.
> 
> Blank line fixes are suggested by checkpatch.pl
> 
> Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> 
> Changes in v2:
> - To maintain consistency in driver greybus, all the instances of macro
> with container_of are fixed in a single patch.
> ---
>  drivers/staging/greybus/bundle.h    |  6 +++++-
>  drivers/staging/greybus/control.h   |  6 +++++-
>  drivers/staging/greybus/gbphy.h     | 12 ++++++++++--
>  drivers/staging/greybus/greybus.h   |  6 +++++-
>  drivers/staging/greybus/hd.h        |  6 +++++-
>  drivers/staging/greybus/interface.h |  6 +++++-
>  drivers/staging/greybus/module.h    |  6 +++++-
>  drivers/staging/greybus/svc.h       |  6 +++++-
>  8 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/greybus/bundle.h b/drivers/staging/greybus/bundle.h
> index 8734d2055657..84956eedb1c4 100644
> --- a/drivers/staging/greybus/bundle.h
> +++ b/drivers/staging/greybus/bundle.h
> @@ -31,7 +31,11 @@ struct gb_bundle {
>  
>  	struct list_head	links;	/* interface->bundles */
>  };
> -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> +
> +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> +{
> +	return container_of(d, struct gb_bundle, dev);
> +}

Why are we changing this to an inline function?  The #define is fine,
and how we "normally" do this type of container_of conversion.

I understand the objection of the "no blank line", but this was the
"original" style that we used to create these #defines from the very
beginning of the driver model work a decade ago.  Changing that
muscle-memory is going to be hard for some of us.  Look at
drivers/base/base.h for other examples of this.

thanks,

greg k-h

Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Alex Elder]

On 4/17/19 1:25 AM, Greg KH wrote:
> On Tue, Apr 16, 2019 at 05:13:18PM -0500, Madhumitha Prabakaran wrote:
>> Fix a blank line after structure declarations. Also, convert
>> macros into inline functions in order to maintain Linux kernel
>> coding style based on which the inline function is
>> preferable over the macro.
>>
>> Blank line fixes are suggested by checkpatch.pl
>>
>> Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
>>
>> Changes in v2:
>> - To maintain consistency in driver greybus, all the instances of macro
>> with container_of are fixed in a single patch.
>> ---
>>  drivers/staging/greybus/bundle.h    |  6 +++++-
>>  drivers/staging/greybus/control.h   |  6 +++++-
>>  drivers/staging/greybus/gbphy.h     | 12 ++++++++++--
>>  drivers/staging/greybus/greybus.h   |  6 +++++-
>>  drivers/staging/greybus/hd.h        |  6 +++++-
>>  drivers/staging/greybus/interface.h |  6 +++++-
>>  drivers/staging/greybus/module.h    |  6 +++++-
>>  drivers/staging/greybus/svc.h       |  6 +++++-
>>  8 files changed, 45 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/staging/greybus/bundle.h b/drivers/staging/greybus/bundle.h
>> index 8734d2055657..84956eedb1c4 100644
>> --- a/drivers/staging/greybus/bundle.h
>> +++ b/drivers/staging/greybus/bundle.h
>> @@ -31,7 +31,11 @@ struct gb_bundle {
>>  
>>  	struct list_head	links;	/* interface->bundles */
>>  };
>> -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
>> +
>> +static inline struct gb_bundle *to_gb_bundle(struct device *d)
>> +{
>> +	return container_of(d, struct gb_bundle, dev);
>> +}
> 
> Why are we changing this to an inline function?  The #define is fine,
> and how we "normally" do this type of container_of conversion.

I'm not completely sure about the inline function, but on the no blank
lines thing (and many other minor issues) "checkpatch.pl" is to blame.
There are lots of examples of issues that checkpatch points out that are
matters of opinion and not hardened kernel style rules.

We try to encourage people to get involved with kernel development by
fixing minor problems, and we tell them a good way to find them is
by running checkpatch and "fixing" what it reports.  Unfortunately,
it is often things of this type, and reviewers balk and say "no,
please leave it," and the poor new person has a bad first experience.

I *like* "checkpatch.pl".  And the fact that it can point out some
of these sorts of things is great.  But it would be nice if certain
types of problems (like multiple blank lines, or lines that are 81
characters wide for example) would only be reported when a "--strict"
option or something were supplied.

					-Alex

> I understand the objection of the "no blank line", but this was the
> "original" style that we used to create these #defines from the very
> beginning of the driver model work a decade ago.  Changing that
> muscle-memory is going to be hard for some of us.  Look at
> drivers/base/base.h for other examples of this.
> 
> thanks,
> 
> greg k-h
> _______________________________________________
> greybus-dev mailing list
> greybus-dev@lists.linaro.org
> https://lists.linaro.org/mailman/listinfo/greybus-dev
> 

Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Johan Hovold]

On Wed, Apr 17, 2019 at 06:19:50AM -0500, Alex Elder wrote:

> I'm not completely sure about the inline function, but on the no blank
> lines thing (and many other minor issues) "checkpatch.pl" is to blame.
> There are lots of examples of issues that checkpatch points out that are
> matters of opinion and not hardened kernel style rules.
> 
> We try to encourage people to get involved with kernel development by
> fixing minor problems, and we tell them a good way to find them is
> by running checkpatch and "fixing" what it reports.  Unfortunately,
> it is often things of this type, and reviewers balk and say "no,
> please leave it," and the poor new person has a bad first experience.
> 
> I *like* "checkpatch.pl".  And the fact that it can point out some
> of these sorts of things is great.  But it would be nice if certain
> types of problems (like multiple blank lines, or lines that are 81
> characters wide for example) would only be reported when a "--strict"
> option or something were supplied.

The problem is that --strict is enabled by default when running
checkpatch on code in staging (and net).

And it has all sorts of weird tests (prefixed as "CHECK" rather than
"WARNING") to catch everyone and their mom's pet peeve.

I don't think the intention ever was that all those should be "fixed",
but this appears to be where this checkpatch mission creep comes from
(and we're now seeing --strict being used on code outside of staging
too).

IMO we're setting a bad example for new contributers by accepting such
changes by default. Blindly trusting a tool is not how kernel
development works, but that seems to be the message currently sent.

Johan

Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Alex Elder]

On 4/16/19 5:13 PM, Madhumitha Prabakaran wrote:
> Fix a blank line after structure declarations. Also, convert
> macros into inline functions in order to maintain Linux kernel
> coding style based on which the inline function is
> preferable over the macro.

Madhumitha, here is my explanation for why *not* to convert these
container_of() macros to inline functions.  It's not just because
"we do this all over the kernel."  The reason is that it actually
does not improve the code.

Inline functions are often better than macros because they are
explicit about types, allowing the compiler to tell you if you
are passing parameters of the right type, and possibly assigning
results to objects of the right type.

Here is the definition of the container_of() macro in <linux/kernel.h>:

#define container_of(ptr, type, member) ({                      \
        const typeof(((type *)0)->member) * __mptr = (ptr);     \
        (type *)((char *)__mptr - offsetof(type, member)); })

You see that ptr is explicitly assigned to a local variable
of the type of the member field, so the compiler is able to
check that assignment.  And the return value is similarly
cast to the type of the containing structure, so the type
compatibility of the assignment can also be checked.  It is
assumed that where this macro is used, the caller knows it
is passing an appropriate address.

There is another thing about this particular definition make
it just as good as an inline function.  A macro expansion can
result in one of its parameters being used more than once, and
that allows those parameters to be *evaluated* more than once
in a single statement, which can produce incorrect code.

This macro is defined using the "statement expression"
extension to C--where a compound statement is enclosed in
parentheses--({ ... }).  This allows a local variable to be
used in the macro expansion, which avoids any reuse of the
macro parameters which might cause side-effects.

So anyway, I don't think there is any benefit to replacing
macros like this that do container_of() with inline functions.

					-Alex

> Blank line fixes are suggested by checkpatch.pl
> 
> Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> 
> Changes in v2:
> - To maintain consistency in driver greybus, all the instances of macro
> with container_of are fixed in a single patch.
> ---
>  drivers/staging/greybus/bundle.h    |  6 +++++-
>  drivers/staging/greybus/control.h   |  6 +++++-
>  drivers/staging/greybus/gbphy.h     | 12 ++++++++++--
>  drivers/staging/greybus/greybus.h   |  6 +++++-
>  drivers/staging/greybus/hd.h        |  6 +++++-
>  drivers/staging/greybus/interface.h |  6 +++++-
>  drivers/staging/greybus/module.h    |  6 +++++-
>  drivers/staging/greybus/svc.h       |  6 +++++-
>  8 files changed, 45 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/staging/greybus/bundle.h b/drivers/staging/greybus/bundle.h
> index 8734d2055657..84956eedb1c4 100644
> --- a/drivers/staging/greybus/bundle.h
> +++ b/drivers/staging/greybus/bundle.h
> @@ -31,7 +31,11 @@ struct gb_bundle {
>  
>  	struct list_head	links;	/* interface->bundles */
>  };
> -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> +
> +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> +{
> +	return container_of(d, struct gb_bundle, dev);
> +}
>  
>  /* Greybus "private" definitions" */
>  struct gb_bundle *gb_bundle_create(struct gb_interface *intf, u8 bundle_id,
> 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);
> diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h
> index 99463489d7d6..20307f6dfcb9 100644
> --- a/drivers/staging/greybus/gbphy.h
> +++ b/drivers/staging/greybus/gbphy.h
> @@ -15,7 +15,11 @@ struct gbphy_device {
>  	struct list_head list;
>  	struct device dev;
>  };
> -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> +
> +static inline struct gbphy_device *to_gbphy_dev(struct device *d)
> +{
> +	return container_of(d, struct gbphy_device, dev);
> +}
>  
>  static inline void *gb_gbphy_get_data(struct gbphy_device *gdev)
>  {
> @@ -43,7 +47,11 @@ struct gbphy_driver {
>  
>  	struct device_driver driver;
>  };
> -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver)
> +
> +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *d)
> +{
> +	return container_of(d, struct gbphy_driver, driver);
> +}
>  
>  int gb_gbphy_register_driver(struct gbphy_driver *driver,
>  			     struct module *owner, const char *mod_name);
> diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h
> index d03ddb7c9df0..a82d5002b4d5 100644
> --- a/drivers/staging/greybus/greybus.h
> +++ b/drivers/staging/greybus/greybus.h
> @@ -64,7 +64,11 @@ struct greybus_driver {
>  
>  	struct device_driver driver;
>  };
> -#define to_greybus_driver(d) container_of(d, struct greybus_driver, driver)
> +
> +static inline struct greybus_driver *to_greybus_driver(struct device_driver *d)
> +{
> +	return container_of(d, struct greybus_driver, driver);
> +}
>  
>  static inline void greybus_set_drvdata(struct gb_bundle *bundle, void *data)
>  {
> diff --git a/drivers/staging/greybus/hd.h b/drivers/staging/greybus/hd.h
> index 6cf024a20a58..de7c49d05266 100644
> --- a/drivers/staging/greybus/hd.h
> +++ b/drivers/staging/greybus/hd.h
> @@ -57,7 +57,11 @@ struct gb_host_device {
>  	/* Private data for the host driver */
>  	unsigned long hd_priv[0] __aligned(sizeof(s64));
>  };
> -#define to_gb_host_device(d) container_of(d, struct gb_host_device, dev)
> +
> +static inline struct gb_host_device *to_gb_host_device(struct device *d)
> +{
> +	return container_of(d, struct gb_host_device, dev);
> +}
>  
>  int gb_hd_cport_reserve(struct gb_host_device *hd, u16 cport_id);
>  void gb_hd_cport_release_reserved(struct gb_host_device *hd, u16 cport_id);
> diff --git a/drivers/staging/greybus/interface.h b/drivers/staging/greybus/interface.h
> index 1c00c5bb3ec9..f86c0d596dbe 100644
> --- a/drivers/staging/greybus/interface.h
> +++ b/drivers/staging/greybus/interface.h
> @@ -63,7 +63,11 @@ struct gb_interface {
>  	struct work_struct mode_switch_work;
>  	struct completion mode_switch_completion;
>  };
> -#define to_gb_interface(d) container_of(d, struct gb_interface, dev)
> +
> +static inline struct gb_interface *to_gb_interface(struct device *d)
> +{
> +	return container_of(d, struct gb_interface, dev);
> +}
>  
>  struct gb_interface *gb_interface_create(struct gb_module *module,
>  					 u8 interface_id);
> diff --git a/drivers/staging/greybus/module.h b/drivers/staging/greybus/module.h
> index b1ebcc6636db..c427b788b677 100644
> --- a/drivers/staging/greybus/module.h
> +++ b/drivers/staging/greybus/module.h
> @@ -22,7 +22,11 @@ struct gb_module {
>  
>  	struct gb_interface *interfaces[0];
>  };
> -#define to_gb_module(d) container_of(d, struct gb_module, dev)
> +
> +static inline struct gb_module *to_gb_module(struct device *d)
> +{
> +	return container_of(d, struct gb_module, dev);
> +}
>  
>  struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id,
>  				   size_t num_interfaces);
> diff --git a/drivers/staging/greybus/svc.h b/drivers/staging/greybus/svc.h
> index ad01783bac9c..4e35ac9ed0ff 100644
> --- a/drivers/staging/greybus/svc.h
> +++ b/drivers/staging/greybus/svc.h
> @@ -52,7 +52,11 @@ struct gb_svc {
>  	struct dentry *debugfs_dentry;
>  	struct svc_debugfs_pwrmon_rail *pwrmon_rails;
>  };
> -#define to_gb_svc(d) container_of(d, struct gb_svc, dev)
> +
> +static inline struct gb_svc *to_gb_svc(struct device *d)
> +{
> +	return container_of(d, struct gb_svc, dev);
> +}
>  
>  struct gb_svc *gb_svc_create(struct gb_host_device *hd);
>  int gb_svc_add(struct gb_svc *svc);
> 

Re: [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Madhumitha Prabakaran]

On 04/17  :25, Greg KH wrote:
> On Tue, Apr 16, 2019 at 05:13:18PM -0500, Madhumitha Prabakaran wrote:
> > Fix a blank line after structure declarations. Also, convert
> > macros into inline functions in order to maintain Linux kernel
> > coding style based on which the inline function is
> > preferable over the macro.
> > 
> > Blank line fixes are suggested by checkpatch.pl
> > 
> > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> > 
> > Changes in v2:
> > - To maintain consistency in driver greybus, all the instances of macro
> > with container_of are fixed in a single patch.
> > ---
> >  drivers/staging/greybus/bundle.h    |  6 +++++-
> >  drivers/staging/greybus/control.h   |  6 +++++-
> >  drivers/staging/greybus/gbphy.h     | 12 ++++++++++--
> >  drivers/staging/greybus/greybus.h   |  6 +++++-
> >  drivers/staging/greybus/hd.h        |  6 +++++-
> >  drivers/staging/greybus/interface.h |  6 +++++-
> >  drivers/staging/greybus/module.h    |  6 +++++-
> >  drivers/staging/greybus/svc.h       |  6 +++++-
> >  8 files changed, 45 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/bundle.h b/drivers/staging/greybus/bundle.h
> > index 8734d2055657..84956eedb1c4 100644
> > --- a/drivers/staging/greybus/bundle.h
> > +++ b/drivers/staging/greybus/bundle.h
> > @@ -31,7 +31,11 @@ struct gb_bundle {
> >  
> >  	struct list_head	links;	/* interface->bundles */
> >  };
> > -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> > +
> > +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> > +{
> > +	return container_of(d, struct gb_bundle, dev);
> > +}
> 
> Why are we changing this to an inline function?  The #define is fine,
> and how we "normally" do this type of container_of conversion.
> 
> I understand the objection of the "no blank line", but this was the
> "original" style that we used to create these #defines from the very
> beginning of the driver model work a decade ago.  Changing that
> muscle-memory is going to be hard for some of us.  Look at
> drivers/base/base.h for other examples of this.

Thanks for explaining it.

> 
> thanks,
> 
> greg k-h

Re: [greybus-dev] [PATCH v2] Staging: greybus: Cleanup in greybus driver

[Madhumitha Prabakaran]

On 04/17  :41, Alex Elder wrote:
> On 4/16/19 5:13 PM, Madhumitha Prabakaran wrote:
> > Fix a blank line after structure declarations. Also, convert
> > macros into inline functions in order to maintain Linux kernel
> > coding style based on which the inline function is
> > preferable over the macro.
> 
> Madhumitha, here is my explanation for why *not* to convert these
> container_of() macros to inline functions.  It's not just because
> "we do this all over the kernel."  The reason is that it actually
> does not improve the code.
> 
> Inline functions are often better than macros because they are
> explicit about types, allowing the compiler to tell you if you
> are passing parameters of the right type, and possibly assigning
> results to objects of the right type.


This makes more sense now.

> 
> Here is the definition of the container_of() macro in <linux/kernel.h>:
> 
> #define container_of(ptr, type, member) ({                      \
>         const typeof(((type *)0)->member) * __mptr = (ptr);     \
>         (type *)((char *)__mptr - offsetof(type, member)); })
> 
> You see that ptr is explicitly assigned to a local variable
> of the type of the member field, so the compiler is able to
> check that assignment.  And the return value is similarly
> cast to the type of the containing structure, so the type
> compatibility of the assignment can also be checked.  It is
> assumed that where this macro is used, the caller knows it
> is passing an appropriate address.
> 
> There is another thing about this particular definition make
> it just as good as an inline function.  A macro expansion can
> result in one of its parameters being used more than once, and
> that allows those parameters to be *evaluated* more than once
> in a single statement, which can produce incorrect code.
> 
> This macro is defined using the "statement expression"
> extension to C--where a compound statement is enclosed in
> parentheses--({ ... }).  This allows a local variable to be
> used in the macro expansion, which avoids any reuse of the
> macro parameters which might cause side-effects.
> 
> So anyway, I don't think there is any benefit to replacing
> macros like this that do container_of() with inline functions.
> 
> 					-Alex

Thanks for taking time to explain it in detailed way.

> 
> > Blank line fixes are suggested by checkpatch.pl
> > 
> > Signed-off-by: Madhumitha Prabakaran <madhumithabiw@gmail.com>
> > 
> > Changes in v2:
> > - To maintain consistency in driver greybus, all the instances of macro
> > with container_of are fixed in a single patch.
> > ---
> >  drivers/staging/greybus/bundle.h    |  6 +++++-
> >  drivers/staging/greybus/control.h   |  6 +++++-
> >  drivers/staging/greybus/gbphy.h     | 12 ++++++++++--
> >  drivers/staging/greybus/greybus.h   |  6 +++++-
> >  drivers/staging/greybus/hd.h        |  6 +++++-
> >  drivers/staging/greybus/interface.h |  6 +++++-
> >  drivers/staging/greybus/module.h    |  6 +++++-
> >  drivers/staging/greybus/svc.h       |  6 +++++-
> >  8 files changed, 45 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/bundle.h b/drivers/staging/greybus/bundle.h
> > index 8734d2055657..84956eedb1c4 100644
> > --- a/drivers/staging/greybus/bundle.h
> > +++ b/drivers/staging/greybus/bundle.h
> > @@ -31,7 +31,11 @@ struct gb_bundle {
> >  
> >  	struct list_head	links;	/* interface->bundles */
> >  };
> > -#define to_gb_bundle(d) container_of(d, struct gb_bundle, dev)
> > +
> > +static inline struct gb_bundle *to_gb_bundle(struct device *d)
> > +{
> > +	return container_of(d, struct gb_bundle, dev);
> > +}
> >  
> >  /* Greybus "private" definitions" */
> >  struct gb_bundle *gb_bundle_create(struct gb_interface *intf, u8 bundle_id,
> > 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);
> > diff --git a/drivers/staging/greybus/gbphy.h b/drivers/staging/greybus/gbphy.h
> > index 99463489d7d6..20307f6dfcb9 100644
> > --- a/drivers/staging/greybus/gbphy.h
> > +++ b/drivers/staging/greybus/gbphy.h
> > @@ -15,7 +15,11 @@ struct gbphy_device {
> >  	struct list_head list;
> >  	struct device dev;
> >  };
> > -#define to_gbphy_dev(d) container_of(d, struct gbphy_device, dev)
> > +
> > +static inline struct gbphy_device *to_gbphy_dev(struct device *d)
> > +{
> > +	return container_of(d, struct gbphy_device, dev);
> > +}
> >  
> >  static inline void *gb_gbphy_get_data(struct gbphy_device *gdev)
> >  {
> > @@ -43,7 +47,11 @@ struct gbphy_driver {
> >  
> >  	struct device_driver driver;
> >  };
> > -#define to_gbphy_driver(d) container_of(d, struct gbphy_driver, driver)
> > +
> > +static inline struct gbphy_driver *to_gbphy_driver(struct device_driver *d)
> > +{
> > +	return container_of(d, struct gbphy_driver, driver);
> > +}
> >  
> >  int gb_gbphy_register_driver(struct gbphy_driver *driver,
> >  			     struct module *owner, const char *mod_name);
> > diff --git a/drivers/staging/greybus/greybus.h b/drivers/staging/greybus/greybus.h
> > index d03ddb7c9df0..a82d5002b4d5 100644
> > --- a/drivers/staging/greybus/greybus.h
> > +++ b/drivers/staging/greybus/greybus.h
> > @@ -64,7 +64,11 @@ struct greybus_driver {
> >  
> >  	struct device_driver driver;
> >  };
> > -#define to_greybus_driver(d) container_of(d, struct greybus_driver, driver)
> > +
> > +static inline struct greybus_driver *to_greybus_driver(struct device_driver *d)
> > +{
> > +	return container_of(d, struct greybus_driver, driver);
> > +}
> >  
> >  static inline void greybus_set_drvdata(struct gb_bundle *bundle, void *data)
> >  {
> > diff --git a/drivers/staging/greybus/hd.h b/drivers/staging/greybus/hd.h
> > index 6cf024a20a58..de7c49d05266 100644
> > --- a/drivers/staging/greybus/hd.h
> > +++ b/drivers/staging/greybus/hd.h
> > @@ -57,7 +57,11 @@ struct gb_host_device {
> >  	/* Private data for the host driver */
> >  	unsigned long hd_priv[0] __aligned(sizeof(s64));
> >  };
> > -#define to_gb_host_device(d) container_of(d, struct gb_host_device, dev)
> > +
> > +static inline struct gb_host_device *to_gb_host_device(struct device *d)
> > +{
> > +	return container_of(d, struct gb_host_device, dev);
> > +}
> >  
> >  int gb_hd_cport_reserve(struct gb_host_device *hd, u16 cport_id);
> >  void gb_hd_cport_release_reserved(struct gb_host_device *hd, u16 cport_id);
> > diff --git a/drivers/staging/greybus/interface.h b/drivers/staging/greybus/interface.h
> > index 1c00c5bb3ec9..f86c0d596dbe 100644
> > --- a/drivers/staging/greybus/interface.h
> > +++ b/drivers/staging/greybus/interface.h
> > @@ -63,7 +63,11 @@ struct gb_interface {
> >  	struct work_struct mode_switch_work;
> >  	struct completion mode_switch_completion;
> >  };
> > -#define to_gb_interface(d) container_of(d, struct gb_interface, dev)
> > +
> > +static inline struct gb_interface *to_gb_interface(struct device *d)
> > +{
> > +	return container_of(d, struct gb_interface, dev);
> > +}
> >  
> >  struct gb_interface *gb_interface_create(struct gb_module *module,
> >  					 u8 interface_id);
> > diff --git a/drivers/staging/greybus/module.h b/drivers/staging/greybus/module.h
> > index b1ebcc6636db..c427b788b677 100644
> > --- a/drivers/staging/greybus/module.h
> > +++ b/drivers/staging/greybus/module.h
> > @@ -22,7 +22,11 @@ struct gb_module {
> >  
> >  	struct gb_interface *interfaces[0];
> >  };
> > -#define to_gb_module(d) container_of(d, struct gb_module, dev)
> > +
> > +static inline struct gb_module *to_gb_module(struct device *d)
> > +{
> > +	return container_of(d, struct gb_module, dev);
> > +}
> >  
> >  struct gb_module *gb_module_create(struct gb_host_device *hd, u8 module_id,
> >  				   size_t num_interfaces);
> > diff --git a/drivers/staging/greybus/svc.h b/drivers/staging/greybus/svc.h
> > index ad01783bac9c..4e35ac9ed0ff 100644
> > --- a/drivers/staging/greybus/svc.h
> > +++ b/drivers/staging/greybus/svc.h
> > @@ -52,7 +52,11 @@ struct gb_svc {
> >  	struct dentry *debugfs_dentry;
> >  	struct svc_debugfs_pwrmon_rail *pwrmon_rails;
> >  };
> > -#define to_gb_svc(d) container_of(d, struct gb_svc, dev)
> > +
> > +static inline struct gb_svc *to_gb_svc(struct device *d)
> > +{
> > +	return container_of(d, struct gb_svc, dev);
> > +}
> >  
> >  struct gb_svc *gb_svc_create(struct gb_host_device *hd);
> >  int gb_svc_add(struct gb_svc *svc);
> > 
>