| Message ID | 20170728132339.332897-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 99859541a92de570eb624c2488754d2406375c74 |
| Headers | show |
Hi Arnd, we already have a patch for this: [PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation from 20.07.2017 Maybe I did something wrong, but my first solution was exactly like your proposal. As far as I remeber, I wasn't able to compile it that way. Therefore I made a little bit more complicated fix. If I did something wrong and yours is fine, we should go for yours, because it is a shorter solution. Thanks, Marcus > Arnd Bergmann <arnd@arndb.de> hat am 28. Juli 2017 um 15:23 geschrieben: > > > I ran into this link error on an ARM OABI build: > > drivers/staging/pi433/rf69.o: In function `rf69_set_frequency': > rf69.c:(.text+0xc9c): undefined reference to `__udivdi3' > > No idea why I didn't see it with the default EABI configurations, > but the right solution here seems to be to use div_u64() > to get the external division implementation. > > Fixes: 874bcba65f9a ("staging: pi433: New driver") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/staging/pi433/rf69.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c > index e391ce777bc7..e5267b5638c0 100644 > --- a/drivers/staging/pi433/rf69.c > +++ b/drivers/staging/pi433/rf69.c > @@ -238,7 +238,7 @@ int rf69_set_frequency(struct spi_device *spi, u32 > frequency) > do_div(f_step, 524288); // 524288 = 2^19 > > // check input value > - f_max = f_step * 8388608 / factor; > + f_max = div_u64(f_step * 8388608, factor); > if (frequency > f_max) > { > dev_dbg(&spi->dev, "setFrequency: illegal input param"); > -- > 2.9.0 > >
On Fri, Jul 28, 2017 at 04:21:05PM +0200, Marcus Wolf wrote: > Hi Arnd, > > we already have a patch for this: > [PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation > from 20.07.2017 https://patchwork.kernel.org/patch/9855261/ > > Maybe I did something wrong, but my first solution was exactly like your > proposal. As far as I remeber, I wasn't able to compile it that way. Therefore I > made a little bit more complicated fix. If I did something wrong and yours is > fine, we should go for yours, because it is a shorter solution. > Your patch doesn't apply for one thing.... :( Read Documentation/process/email-clients.rst It probably would have been Ok otherwise. I'm pretty sure that Arnd's patch is going to be fine. regards, dan carpenter
On Fri, Jul 28, 2017 at 4:21 PM, Marcus Wolf <marcus.wolf@wolf-entwicklungen.de> wrote: > Hi Arnd, > > we already have a patch for this: > [PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation > from 20.07.2017 > > Maybe I did something wrong, but my first solution was exactly like your > proposal. As far as I remeber, I wasn't able to compile it that way. Therefore I > made a little bit more complicated fix. If I did something wrong and yours is > fine, we should go for yours, because it is a shorter solution. I think the problem with your original patch is that it doesn't work for 'u64 factor': do_div() is a bit tricky to work with, and it does not always accept a 64-bit divisor, while div_u64 will simply convert the divisor to a u32. You can also make 'factor' a 'u32' and keep using do_div. Arnd
Hi Dan, Thanks for the hint. I don't get, what went wrong. If I take the mail from my outbox and view it, it looks nice. Seems, I really need to look for another mailtool. But for several reasons, that's not possible at the moment (slow move of 20 domains with someting arround 80 mail adresses and several mailboxes from one provider to another within the next two monthes). Sorry for any inconvenience, Marcus > Dan Carpenter <dan.carpenter@oracle.com> hat am 28. Juli 2017 um 16:26 > geschrieben: > > > On Fri, Jul 28, 2017 at 04:21:05PM +0200, Marcus Wolf wrote: > > Hi Arnd, > > > > we already have a patch for this: > > [PATCH 1/1] staging: pi433: fix problem with division in rf69_set_deviation > > from 20.07.2017 > > > https://patchwork.kernel.org/patch/9855261/ > > > > > Maybe I did something wrong, but my first solution was exactly like your > > proposal. As far as I remeber, I wasn't able to compile it that way. > > Therefore I > > made a little bit more complicated fix. If I did something wrong and yours > > is > > fine, we should go for yours, because it is a shorter solution. > > > > Your patch doesn't apply for one thing.... :( Read > Documentation/process/email-clients.rst It probably would have been Ok > otherwise. > > I'm pretty sure that Arnd's patch is going to be fine. > > regards, > dan carpenter > >
On Fri, Jul 28, 2017 at 05:16:56PM +0200, Marcus Wolf wrote: > Hi Greg, > > according to the proposals of Walter Harms, I revised the rf69.c: I replaced > some macros with inline functions and removed some obsolete ifdefs. According to > walter this will improve the resource situation. In addition the readybility is > enhanced, since lines got shorter. It's a quite big change, that touched nearly > every function in that file. Just send your patches, we can't discuss them without seeing them. > I was testing the new code for a while now and did not observer a problem so > far. But I don't have a kind of unit test, so my tests for sure didn't cover > everything. Any sort of testing is probably better than 70% of staging patches. :P > > Is it a good time, to submit such a change in these days, or is it prefrable to > submit it later? It doesn't matter when you send patches. > In adition, I am a bit afraid of my current mailtool doing something > unexpected... > > If you like, I can give it a try! > > Cheers, > > Marcus > P.S. Can you process diffs fom SVN, too, or is it mandatory to create the diff > with git? The patches have to be able to be applied with `cat email.txt | git am`. If you're renaming or moving files, then use git diff. Otherwise it's all fine. regards, dan carpenter
On Fri, Jul 28, 2017 at 05:16:56PM +0200, Marcus Wolf wrote: > Hi Greg, > > according to the proposals of Walter Harms, I revised the rf69.c: I replaced > some macros with inline functions and removed some obsolete ifdefs. According to > walter this will improve the resource situation. In addition the readybility is > enhanced, since lines got shorter. It's a quite big change, that touched nearly > every function in that file. > I was testing the new code for a while now and did not observer a problem so > far. But I don't have a kind of unit test, so my tests for sure didn't cover > everything. > > Is it a good time, to submit such a change in these days, or is it prefrable to > submit it later? > In adition, I am a bit afraid of my current mailtool doing something > unexpected... You can send patches anytime, don't worry about timing, I can handle putting them in the correct patch queue. > P.S. Can you process diffs fom SVN, too, or is it mandatory to create the diff > with git? As long as it is in normal patch format, I can apply it. Personally, I use quilt to generate patche, which uses 'diff', and that works just fine. thanks, greg k-h
diff --git a/drivers/staging/pi433/rf69.c b/drivers/staging/pi433/rf69.c index e391ce777bc7..e5267b5638c0 100644 --- a/drivers/staging/pi433/rf69.c +++ b/drivers/staging/pi433/rf69.c @@ -238,7 +238,7 @@ int rf69_set_frequency(struct spi_device *spi, u32 frequency) do_div(f_step, 524288); // 524288 = 2^19 // check input value - f_max = f_step * 8388608 / factor; + f_max = div_u64(f_step * 8388608, factor); if (frequency > f_max) { dev_dbg(&spi->dev, "setFrequency: illegal input param");
I ran into this link error on an ARM OABI build: drivers/staging/pi433/rf69.o: In function `rf69_set_frequency': rf69.c:(.text+0xc9c): undefined reference to `__udivdi3' No idea why I didn't see it with the default EABI configurations, but the right solution here seems to be to use div_u64() to get the external division implementation. Fixes: 874bcba65f9a ("staging: pi433: New driver") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/staging/pi433/rf69.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.9.0