| Message ID | 20200326152251.19094-1-ajay.kathat@microchip.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | [v3] staging: wilc1000: Use crc7 in lib/ rather than a private copy | expand |
On Thu, Mar 26, 2020 at 03:23:36PM +0000, Ajay.Kathat@microchip.com wrote: > From: George Spelvin <lkml@SDF.ORG> > > The code in lib/ is the desired polynomial, and even includes > the 1-bit left shift in the table rather than needing to code > it explicitly. > > While I'm in Kconfig, add a description of what a WILC1000 is. > Kconfig questions that require me to look up a data sheet to > find out that I probably don't have one are a pet peeve. > I don't know how this patch made it through two versions without anyone complaining that this paragraph should be done as a separate patch... > Cc: Adham Abozaeid <adham.abozaeid@microchip.com> > Cc: linux-wireless@vger.kernel.org > Reviewed-by: Ajay Singh <ajay.kathat@microchip.com> > Signed-off-by: George Spelvin <lkml@sdf.org> > --- This should have you Signed-off-by. The Reviewed-by is kind of assumed so you can drop that bit. But everyone who touches a patch needs to add their signed off by. regards, dan carpenter
Hi Dan, On 02/04/20 1:57 pm, Dan Carpenter wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Thu, Mar 26, 2020 at 03:23:36PM +0000, Ajay.Kathat@microchip.com wrote: >> From: George Spelvin <lkml@SDF.ORG> >> >> The code in lib/ is the desired polynomial, and even includes >> the 1-bit left shift in the table rather than needing to code >> it explicitly. >> >> While I'm in Kconfig, add a description of what a WILC1000 is. >> Kconfig questions that require me to look up a data sheet to >> find out that I probably don't have one are a pet peeve. >> > > I don't know how this patch made it through two versions without anyone > complaining that this paragraph should be done as a separate patch... > The first two version of patches were not submitted to devel@driverdev mailing list. I too missed the point to keep the Kconfig changes in separate patch. >> Cc: Adham Abozaeid <adham.abozaeid@microchip.com> >> Cc: linux-wireless@vger.kernel.org >> Reviewed-by: Ajay Singh <ajay.kathat@microchip.com> >> Signed-off-by: George Spelvin <lkml@sdf.org> >> --- > > This should have you Signed-off-by. The Reviewed-by is kind of assumed > so you can drop that bit. But everyone who touches a patch needs to > add their signed off by. > Thanks, I will make a note of it. Regards Ajay
On Thu, Apr 02, 2020 at 11:27:45AM +0300, Dan Carpenter wrote: > I don't know how this patch made it through two versions without anyone > complaining that this paragraph should be done as a separate patch... I often fold comment (and spacing/formatting) patches in to a main patch, when touching adjacent code anyway and it doesn't cause distracting clutter. This seemed like such a case, which is why I submitted it as one. But it's a bit of style thing. >> Cc: Adham Abozaeid <adham.abozaeid@microchip.com> >> Cc: linux-wireless@vger.kernel.org >> Reviewed-by: Ajay Singh <ajay.kathat@microchip.com> >> Signed-off-by: George Spelvin <lkml@sdf.org> >> --- > > This should have you Signed-off-by. The Reviewed-by is kind of assumed > so you can drop that bit. But everyone who touches a patch needs to > add their signed off by. Er... all he did was add "staging: " to the front of the title. That's not a change to the code at all, and as trivial a change to the commit message as adding "Reviewed-by:" to the end. We don't need S-o-b for such things or we'd end up in a horrible infinite recursion.
On Thu, Apr 02, 2020 at 03:30:34PM +0000, George Spelvin wrote: > On Thu, Apr 02, 2020 at 11:27:45AM +0300, Dan Carpenter wrote: > > I don't know how this patch made it through two versions without anyone > > complaining that this paragraph should be done as a separate patch... > > I often fold comment (and spacing/formatting) patches in to a main > patch, when touching adjacent code anyway and it doesn't cause > distracting clutter. > > This seemed like such a case, which is why I submitted it as one. > But it's a bit of style thing. > We're super strict in Staging. :P Greg is more strict than I am. > >> Cc: Adham Abozaeid <adham.abozaeid@microchip.com> > >> Cc: linux-wireless@vger.kernel.org > >> Reviewed-by: Ajay Singh <ajay.kathat@microchip.com> > >> Signed-off-by: George Spelvin <lkml@sdf.org> > >> --- > > > > This should have you Signed-off-by. The Reviewed-by is kind of assumed > > so you can drop that bit. But everyone who touches a patch needs to > > add their signed off by. > > Er... all he did was add "staging: " to the front of the title. > > That's not a change to the code at all, and as trivial a change > to the commit message as adding "Reviewed-by:" to the end. > We don't need S-o-b for such things or we'd end up in a horrible > infinite recursion. You've misunderstood. He sent the email so he has to add his Signed-off-by. It's not at all related to changing anything in the patch. That's how sign offs work. regards, dan carpenter
On Fri, Apr 03, 2020 at 12:10:29PM +0300, Dan Carpenter wrote: > On Thu, Apr 02, 2020 at 03:30:34PM +0000, George Spelvin wrote: > > On Thu, Apr 02, 2020 at 11:27:45AM +0300, Dan Carpenter wrote: > > > I don't know how this patch made it through two versions without anyone > > > complaining that this paragraph should be done as a separate patch... > > > > I often fold comment (and spacing/formatting) patches in to a main > > patch, when touching adjacent code anyway and it doesn't cause > > distracting clutter. > > > > This seemed like such a case, which is why I submitted it as one. > > But it's a bit of style thing. > > > > We're super strict in Staging. :P Greg is more strict than I am. Okay, but it's my fault, not his. >> This should have you Signed-off-by. The Reviewed-by is kind of assumed >>> so you can drop that bit. But everyone who touches a patch needs to >>> add their signed off by. >> >> Er... all he did was add "staging: " to the front of the title. >> >> That's not a change to the code at all, and as trivial a change >> to the commit message as adding "Reviewed-by:" to the end. >> We don't need S-o-b for such things or we'd end up in a horrible >> infinite recursion. > > You've misunderstood. He sent the email so he has to add his > Signed-off-by. It's not at all related to changing anything in the > patch. That's how sign offs work. Looking at my commits (just because I remember how they went in), you seem to be right, but damn, submitting-patches.rst could be clearer on the subject. I understand that it's addressed more to patch authors than maintainers forwarding them, but I've read that thing a dozen times, and the description of S-o-b always seemed to be about copyright. So I had assumed that edits which were below the de minimus standard of copyright didn't need a separate S-o-b. Am I right that there should be an S-o-b from everyone from the patch author to the patch committer (as recorded in git)? And the one exception is that we don't need S-o-b for git pulls after that, because the merge commits record the information? For example, my patch series ending at 4684fe95300c (v4.7-rc1~8^2) only has my S-o-b because it was pulled straight from my git server and merge 7e0fb73c52c4 (v4.7-rc1~8) records who merged it. But b5c56e0cdd62 has an S-o-b from both akpm and Linus because it went to akpm, into his quilt, and then as a patch series to Linus, who committed it. All of which is eactly why git-am has a -s option. That's not a hard rule to understand, but I wish submitting-patches *said* so somewhere, rather than having it be implied by the existence of option (c) in the DCO and the fact that it's *doesn't* say that someone else's S-o-b will suffice. And the git merge exception should be stated, because otherwise it's not clear what the limits of that exception are. I had assumed that accumulating and forwarding patches in general was okay without a S-o-b. So thank you for enlightening me, and if you can confirm the rules, I'll prepare a Documentation/ patch to reduce re-occurrence.
George Spelvin <lkml@SDF.ORG> writes: > On Fri, Apr 03, 2020 at 12:10:29PM +0300, Dan Carpenter wrote: >> On Thu, Apr 02, 2020 at 03:30:34PM +0000, George Spelvin wrote: >> > On Thu, Apr 02, 2020 at 11:27:45AM +0300, Dan Carpenter wrote: >> > > I don't know how this patch made it through two versions without anyone >> > > complaining that this paragraph should be done as a separate patch... >> > >> > I often fold comment (and spacing/formatting) patches in to a main >> > patch, when touching adjacent code anyway and it doesn't cause >> > distracting clutter. >> > >> > This seemed like such a case, which is why I submitted it as one. >> > But it's a bit of style thing. >> > >> >> We're super strict in Staging. :P Greg is more strict than I am. > > Okay, but it's my fault, not his. > >>> This should have you Signed-off-by. The Reviewed-by is kind of assumed >>>> so you can drop that bit. But everyone who touches a patch needs to >>>> add their signed off by. >>> >>> Er... all he did was add "staging: " to the front of the title. >>> >>> That's not a change to the code at all, and as trivial a change >>> to the commit message as adding "Reviewed-by:" to the end. >>> We don't need S-o-b for such things or we'd end up in a horrible >>> infinite recursion. >> >> You've misunderstood. He sent the email so he has to add his >> Signed-off-by. It's not at all related to changing anything in the >> patch. That's how sign offs work. > > Looking at my commits (just because I remember how they went in), > you seem to be right, but damn, submitting-patches.rst could be > clearer on the subject. > > I understand that it's addressed more to patch authors than > maintainers forwarding them, but I've read that thing a dozen times, > and the description of S-o-b always seemed to be about copyright. > > So I had assumed that edits which were below the de minimus standard > of copyright didn't need a separate S-o-b. > > Am I right that there should be an S-o-b from everyone from the > patch author to the patch committer (as recorded in git)? Yes, everyone either modifying or distributing (=submitting) the patch should add their s-o-b. > And the one exception is that we don't need S-o-b for git pulls after > that, because the merge commits record the information? Correct. When you do a git pull you are pulling the commits without any modification, so technically it's not even possible to add the s-o-b lines to the commits you are pulling. Modifying the commit logs would need a rebase and then it not would be a normal git pull anymore.
On Fri, Apr 03, 2020 at 11:40:28PM +0000, George Spelvin wrote: > I understand that it's addressed more to patch authors than > maintainers forwarding them, but I've read that thing a dozen times, > and the description of S-o-b always seemed to be about copyright. > It's to say that you didn't add anything which you shouldn't have, for example, secret SCO UnixWare stuff. > So I had assumed that edits which were below the de minimus standard > of copyright didn't need a separate S-o-b. > > Am I right that there should be an S-o-b from everyone from the > patch author to the patch committer (as recorded in git)? And the > one exception is that we don't need S-o-b for git pulls after that, > because the merge commits record the information? Yes. Also if people added their S-o-b for git merges it would change the git hash for the patch which would suck. regards, dan carpenter
On Sat, Apr 04, 2020 at 08:25:37PM +0300, Dan Carpenter wrote: > On Fri, Apr 03, 2020 at 11:40:28PM +0000, George Spelvin wrote: >> I understand that it's addressed more to patch authors than >> maintainers forwarding them, but I've read that thing a dozen times, >> and the description of S-o-b always seemed to be about copyright. > > It's to say that you didn't add anything which you shouldn't have, for > example, secret SCO UnixWare stuff. Yes, I'm familiar with the (irritating) history. Which is why I had the idea stuck in my head that that it was all about copyright and if you didn't add anything copyrightable, an S-o-b wasn't required. No more than I'd ask for one from the administrator of the e-mail system which delivered it. submitting-patches.rst says "sign your work". It didn't occur to me to sign something that wasn't my work. >> So I had assumed that edits which were below the de minimus standard >> of copyright didn't need a separate S-o-b. >> >> Am I right that there should be an S-o-b from everyone from the >> patch author to the patch committer (as recorded in git)? And the >> one exception is that we don't need S-o-b for git pulls after that, >> because the merge commits record the information? > > Yes. Also if people added their S-o-b for git merges it would change > the git hash for the patch which would suck. I understand the technical difficulties, but lawyers aren't always deterred by such things. :-) Seriously, it's clear there has to be an exception; the question was about the scope of the exception. Thank you for your patience clarifying this stuff for the nth time.
diff --git a/drivers/staging/wilc1000/Kconfig b/drivers/staging/wilc1000/Kconfig index 59e58550d139..80c92e8bf8a5 100644 --- a/drivers/staging/wilc1000/Kconfig +++ b/drivers/staging/wilc1000/Kconfig @@ -2,6 +2,10 @@ config WILC1000 tristate help + Add support for the Atmel WILC1000 802.11 b/g/n SoC. + This provides Wi-FI over an SDIO or SPI interface, and + is usually found in IoT devices. + This module only support IEEE 802.11n WiFi. config WILC1000_SDIO @@ -22,6 +26,7 @@ config WILC1000_SPI tristate "Atmel WILC1000 SPI (WiFi only)" depends on CFG80211 && INET && SPI select WILC1000 + select CRC7 help This module adds support for the SPI interface of adapters using WILC1000 chipset. The Atmel WILC1000 has a Serial Peripheral diff --git a/drivers/staging/wilc1000/spi.c b/drivers/staging/wilc1000/spi.c index 8d4b8c219c2f..3f19e3f38a39 100644 --- a/drivers/staging/wilc1000/spi.c +++ b/drivers/staging/wilc1000/spi.c @@ -6,6 +6,7 @@ #include <linux/clk.h> #include <linux/spi/spi.h> +#include <linux/crc7.h> #include "netdev.h" #include "cfg80211.h" @@ -16,64 +17,6 @@ struct wilc_spi { static const struct wilc_hif_func wilc_hif_spi; -/******************************************** - * - * Crc7 - * - ********************************************/ - -static const u8 crc7_syndrome_table[256] = { - 0x00, 0x09, 0x12, 0x1b, 0x24, 0x2d, 0x36, 0x3f, - 0x48, 0x41, 0x5a, 0x53, 0x6c, 0x65, 0x7e, 0x77, - 0x19, 0x10, 0x0b, 0x02, 0x3d, 0x34, 0x2f, 0x26, - 0x51, 0x58, 0x43, 0x4a, 0x75, 0x7c, 0x67, 0x6e, - 0x32, 0x3b, 0x20, 0x29, 0x16, 0x1f, 0x04, 0x0d, - 0x7a, 0x73, 0x68, 0x61, 0x5e, 0x57, 0x4c, 0x45, - 0x2b, 0x22, 0x39, 0x30, 0x0f, 0x06, 0x1d, 0x14, - 0x63, 0x6a, 0x71, 0x78, 0x47, 0x4e, 0x55, 0x5c, - 0x64, 0x6d, 0x76, 0x7f, 0x40, 0x49, 0x52, 0x5b, - 0x2c, 0x25, 0x3e, 0x37, 0x08, 0x01, 0x1a, 0x13, - 0x7d, 0x74, 0x6f, 0x66, 0x59, 0x50, 0x4b, 0x42, - 0x35, 0x3c, 0x27, 0x2e, 0x11, 0x18, 0x03, 0x0a, - 0x56, 0x5f, 0x44, 0x4d, 0x72, 0x7b, 0x60, 0x69, - 0x1e, 0x17, 0x0c, 0x05, 0x3a, 0x33, 0x28, 0x21, - 0x4f, 0x46, 0x5d, 0x54, 0x6b, 0x62, 0x79, 0x70, - 0x07, 0x0e, 0x15, 0x1c, 0x23, 0x2a, 0x31, 0x38, - 0x41, 0x48, 0x53, 0x5a, 0x65, 0x6c, 0x77, 0x7e, - 0x09, 0x00, 0x1b, 0x12, 0x2d, 0x24, 0x3f, 0x36, - 0x58, 0x51, 0x4a, 0x43, 0x7c, 0x75, 0x6e, 0x67, - 0x10, 0x19, 0x02, 0x0b, 0x34, 0x3d, 0x26, 0x2f, - 0x73, 0x7a, 0x61, 0x68, 0x57, 0x5e, 0x45, 0x4c, - 0x3b, 0x32, 0x29, 0x20, 0x1f, 0x16, 0x0d, 0x04, - 0x6a, 0x63, 0x78, 0x71, 0x4e, 0x47, 0x5c, 0x55, - 0x22, 0x2b, 0x30, 0x39, 0x06, 0x0f, 0x14, 0x1d, - 0x25, 0x2c, 0x37, 0x3e, 0x01, 0x08, 0x13, 0x1a, - 0x6d, 0x64, 0x7f, 0x76, 0x49, 0x40, 0x5b, 0x52, - 0x3c, 0x35, 0x2e, 0x27, 0x18, 0x11, 0x0a, 0x03, - 0x74, 0x7d, 0x66, 0x6f, 0x50, 0x59, 0x42, 0x4b, - 0x17, 0x1e, 0x05, 0x0c, 0x33, 0x3a, 0x21, 0x28, - 0x5f, 0x56, 0x4d, 0x44, 0x7b, 0x72, 0x69, 0x60, - 0x0e, 0x07, 0x1c, 0x15, 0x2a, 0x23, 0x38, 0x31, - 0x46, 0x4f, 0x54, 0x5d, 0x62, 0x6b, 0x70, 0x79 -}; - -static u8 crc7_byte(u8 crc, u8 data) -{ - return crc7_syndrome_table[(crc << 1) ^ data]; -} - -static u8 crc7(u8 crc, const u8 *buffer, u32 len) -{ - while (len--) - crc = crc7_byte(crc, *buffer++); - return crc; -} - -static u8 wilc_get_crc7(u8 *buffer, u32 len) -{ - return crc7(0x7f, (const u8 *)buffer, len) << 1; -} - /******************************************** * * Spi protocol Function @@ -403,6 +346,11 @@ static int spi_data_write(struct wilc *wilc, u8 *b, u32 sz) * Spi Internal Read/Write Function * ********************************************/ +static u8 wilc_get_crc7(u8 *buffer, u32 len) +{ + return crc7_be(0xfe, buffer, len); +} + static int wilc_spi_single_read(struct wilc *wilc, u8 cmd, u32 adr, void *b, u8 clockless) {