Re: [Outreachy kernel] [PATCH v2 1/5] staging: r8723au: break parameter list at separators and align to open braces

[Jes Sorensen <jes.sorensen@gmail.com>]

On 10/27/15 01:44, Greg KH wrote:
> On Sun, Oct 25, 2015 at 08:07:44PM -0700, Alison Schofield wrote:
>> On Sat, Oct 24, 2015 at 07:40:29PM -0700, Greg KH wrote:
>>> On Tue, Oct 20, 2015 at 11:01:58PM -0700, Alison Schofield wrote:
>>>> Improve readability by aligning the struct cmd_hdl wlancmds[] to fit
>>>> within 80 chars and cleaning up the adjacent index comments.
>>>>
>>>> Addresses checkpatch.pl WARNING: line over 80 characters
>>>>
>>>> Signed-off-by: Alison Schofield <amsfield22@gmail.com>
>>>> ---
>>>>  drivers/staging/rtl8723au/core/rtw_cmd.c | 64 +++++++++++++++++++-------------
>>>>  1 file changed, 38 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/staging/rtl8723au/core/rtw_cmd.c b/drivers/staging/rtl8723au/core/rtw_cmd.c
>>>> index 46aea16..c3579ac 100644
>>>> --- a/drivers/staging/rtl8723au/core/rtw_cmd.c
>>>> +++ b/drivers/staging/rtl8723au/core/rtw_cmd.c
>>>> @@ -22,7 +22,7 @@
>>>>  #include <rtw_sreset.h>
>>>>  
>>>>  static struct cmd_hdl wlancmds[] = {
>>>> -	GEN_DRV_CMD_HANDLER(0, NULL) /*0*/
>>>> +	GEN_DRV_CMD_HANDLER(0, NULL)					/*0*/
>>>>  	GEN_DRV_CMD_HANDLER(0, NULL)
>>>>  	GEN_DRV_CMD_HANDLER(0, NULL)
>>>>  	GEN_DRV_CMD_HANDLER(0, NULL)
>>>> @@ -32,18 +32,26 @@ static struct cmd_hdl wlancmds[] = {
>>>>  	GEN_MLME_EXT_HANDLER(0, NULL)
>>>>  	GEN_MLME_EXT_HANDLER(0, NULL)
>>>>  	GEN_MLME_EXT_HANDLER(0, NULL)
>>>> -	GEN_MLME_EXT_HANDLER(0, NULL) /*10*/
>>>> +	GEN_MLME_EXT_HANDLER(0, NULL)					/*10*/
>>>>  	GEN_MLME_EXT_HANDLER(0, NULL)
>>>>  	GEN_MLME_EXT_HANDLER(0, NULL)
>>>>  	GEN_MLME_EXT_HANDLER(0, NULL)
>>>> -	GEN_MLME_EXT_HANDLER(sizeof(struct wlan_bssid_ex), join_cmd_hdl23a) /*14*/
>>>> -	GEN_MLME_EXT_HANDLER(sizeof(struct disconnect_parm), disconnect_hdl23a)
>>>> -	GEN_MLME_EXT_HANDLER(sizeof(struct wlan_bssid_ex), createbss_hdl23a)
>>>> -	GEN_MLME_EXT_HANDLER(sizeof(struct setopmode_parm), setopmode_hdl23a)
>>>> -	GEN_MLME_EXT_HANDLER(sizeof(struct sitesurvey_parm), sitesurvey_cmd_hdl23a) /*18*/
>>>> -	GEN_MLME_EXT_HANDLER(sizeof(struct setauth_parm), setauth_hdl23a)
>>>> -	GEN_MLME_EXT_HANDLER(sizeof(struct setkey_parm), setkey_hdl23a) /*20*/
>>>> -	GEN_MLME_EXT_HANDLER(sizeof(struct set_stakey_parm), set_stakey_hdl23a)
>>>> +	GEN_MLME_EXT_HANDLER(sizeof(struct wlan_bssid_ex),
>>>> +			     join_cmd_hdl23a)
>>>> +	GEN_MLME_EXT_HANDLER(sizeof(struct disconnect_parm),
>>>> +			     disconnect_hdl23a)
>>>> +	GEN_MLME_EXT_HANDLER(sizeof(struct wlan_bssid_ex),
>>>> +			     createbss_hdl23a)
>>>> +	GEN_MLME_EXT_HANDLER(sizeof(struct setopmode_parm),
>>>> +			     setopmode_hdl23a)
>>>> +	GEN_MLME_EXT_HANDLER(sizeof(struct sitesurvey_parm),
>>>> +			     sitesurvey_cmd_hdl23a)
>>>> +	GEN_MLME_EXT_HANDLER(sizeof(struct setauth_parm),
>>>> +			     setauth_hdl23a)
>>>> +	GEN_MLME_EXT_HANDLER(sizeof(struct setkey_parm),		/*20*/
>>>> +			     setkey_hdl23a)
>>>> +	GEN_MLME_EXT_HANDLER(sizeof(struct set_stakey_parm),
>>>> +			     set_stakey_hdl23a)
>>>
>>>
>>> This seems harder to read to me than before, how about you?
>>>
>> It does look worse in the diff, but when I'm scanning the entire 62 item
>> list, keeping the items to the left and the index comments to the right
>> seemed to make it easier to scan and find the item I want. (A maintainer
>> probably wouldn't be reading this list top to bottom.)
>>
>> Is it appropriate for me to ask reviewers (you) to apply the patch to
>> review it?  It seems there is not always enough context in the diff.
> 
> There's enough context, we can read it :)
> 
> I think one-line-per-entry here is just fine, no need to change it at
> all, sorry.

I agree, definitely no reason to do this.

In addition, please remember to CC the driver maintainer when you post
patches for a driver.

Jes