[PATCH] staging: pi433: use div_u64 for 64-bit division

[PATCH] staging: pi433: use div_u64 for 64-bit division

[Arnd Bergmann]

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

Re: [PATCH] staging: pi433: use div_u64 for 64-bit division

[Marcus Wolf]

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
>
>

Re: [PATCH] staging: pi433: use div_u64 for 64-bit division

[Dan Carpenter]

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

Re: [PATCH] staging: pi433: use div_u64 for 64-bit division

[Arnd Bergmann]

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

Re: [PATCH] staging: pi433: use div_u64 for 64-bit division

[Marcus Wolf]

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
>
>

Send a large patch right now or is it better to do it later?

[Marcus Wolf]

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...
 
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?
 
 
 

Re: Send a large patch right now or is it better to do it later?

[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

Re: Send a large patch right now or is it better to do it later?

[Greg Kroah-Hartman]

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