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

[Alex Elder <elder@linaro.org>]

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
>