linux-kernel - Re: [PATCH v2 15/24] Staging: winbond: reg: seventh of the patches that fixes lines over 80 characters

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 21 Sep 2013 12:32:17 +0300
From:	Dan Carpenter <dan.carpenter@...cle.com>
To:	Iker Pedrosa <ikerpedrosam@...il.com>
Cc:	devel@...verdev.osuosl.org, Greg KH <gregkh@...uxfoundation.org>,
	harsh1kumar@...il.com, linux-kernel@...r.kernel.org,
	Adil Mujeeb <mujeeb.adil@...il.com>,
	Pavel Machek <pavel@....cz>
Subject: Re: [PATCH v2 15/24] Staging: winbond: reg: seventh of the patches
 that fixes lines over 80 characters

On Fri, Sep 20, 2013 at 04:45:08PM +0200, Iker Pedrosa wrote:
> On Tue, 17 Sep 2013 12:43:26 +0300
> Dan Carpenter <dan.carpenter@...cle.com> wrote:
> 
> > On Tue, Sep 17, 2013 at 02:29:22PM +0530, Adil Mujeeb wrote:
> > > Hi,
> > > 
> > > 
> > > >-       PowerData = (1 << 31) | (0 << 30) | (24 << 24) |
> > > BitReverse(w89rf242_txvga_data[i][0], 24);
> > > >+       PowerData = (1 << 31) | (0 << 30) | (24 << 24)
> > > >+                           | BitReverse(w89rf242_txvga_data[i][0], 24);
> > > .....
> > > .....
> > > 
> > > >-       reg->M38_MacControl = (DEFAULT_RATE_RETRY_LIMIT << 8) |
> > > (DEFAULT_LONG_RETRY_LIMIT << 4) | DEFAULT_SHORT_RETRY_LIMIT;
> > > >+       reg->M38_MacControl = (DEFAULT_RATE_RETRY_LIMIT << 8) |
> > > >+                                      (DEFAULT_LONG_RETRY_LIMIT << 4) |
> > > >+                                      DEFAULT_SHORT_RETRY_LIMIT;
> > > 
> > > I think consistency is needed. Either break the lines putting the OR
> > > operator ( | ) at the start of new line or at the end of each line. People
> > > has its own preference but here you have used both style in single file.
> > > Just a suggestion.
> > > 
> > 
> > Put the '|' character at the end of the line.  That's the New Unwritten
> > Style Guidelines.  Most of the kernel does it this way and it's easier
> > to read.
> > 
> > But this could be fixed in a later patch instead of redoing the whole
> > series.
> > 
> > regards,
> > dan carpenter
> > 
> 
> I will change it for the new version of this patch but I want to wait
> some days until everybody looks the patches that I've sent.

If they haven't looked after three days then they won't look.  Greg will
look over the patches but everyone else is done.

If you can redo this patch without changing any of the others then just
redo it.  But otherwise I would prefer if you do it in a follow on
patch.  It gets tiresome to review patchsets over and over.

Joe's comment on [PATCH v2 21/24] is correct.  Technically the original
code was buggy as well, but using dev_err() means it will generate a
popup notification in Gnome so the new code is worse.  Redo that one.
Hopefully, you can just redo the one patch without redoing the following
3 patches.

So either send a single patch to Joe's comment, or send an email in
reply to Joe's comment, "Ok.  I will redo the last 4 patches".  Use the
--in-reply-to option of git to send email in reply to a thread.  Pratice
in an email to yourself before sending it to the list.

regards,
dan carpenter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/