* [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
@ 2017-09-06 13:56 ` Arnd Bergmann
0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-09-06 13:56 UTC (permalink / raw)
To: Ben Skeggs, David Airlie; +Cc: Arnd Bergmann, dri-devel, nouveau, linux-kernel
gcc thinks that interpreting a multiplication result as a bool
is confusing:
drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c: In function 'read_pll':
drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c:133:8: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
In this instance, I think using multiplication is more intuitive
than '&&', so I'm adding a comparison to zero instead to shut up
the warning. To further improve readability, I also make the
error case indented and leave the normal case as the final 'return'
statement.
Fixes: 7632b30e4b8b ("drm/nouveau/clk: namespace + nvidia gpu names (no binary change)")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Originally submitted on July 14, but no reply. This is the same
patch again. The warning is currently disabled in mainline, but
I think we can turn it back on in the future, and this change here
seems harmless.
---
drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
index 96e0941c8edd..04b4f4ccf186 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
@@ -130,10 +130,10 @@ read_pll(struct gt215_clk *clk, int idx, u32 pll)
sclk = read_clk(clk, 0x10 + idx, false);
}
- if (M * P)
- return sclk * N / (M * P);
+ if (M * P == 0)
+ return 0;
- return 0;
+ return sclk * N / (M * P);
}
static int
--
2.9.0
^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
@ 2017-09-06 13:56 ` Arnd Bergmann
0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-09-06 13:56 UTC (permalink / raw)
To: Ben Skeggs, David Airlie; +Cc: nouveau, dri-devel, Arnd Bergmann, linux-kernel
gcc thinks that interpreting a multiplication result as a bool
is confusing:
drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c: In function 'read_pll':
drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c:133:8: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
In this instance, I think using multiplication is more intuitive
than '&&', so I'm adding a comparison to zero instead to shut up
the warning. To further improve readability, I also make the
error case indented and leave the normal case as the final 'return'
statement.
Fixes: 7632b30e4b8b ("drm/nouveau/clk: namespace + nvidia gpu names (no binary change)")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
Originally submitted on July 14, but no reply. This is the same
patch again. The warning is currently disabled in mainline, but
I think we can turn it back on in the future, and this change here
seems harmless.
---
drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
index 96e0941c8edd..04b4f4ccf186 100644
--- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
+++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
@@ -130,10 +130,10 @@ read_pll(struct gt215_clk *clk, int idx, u32 pll)
sclk = read_clk(clk, 0x10 + idx, false);
}
- if (M * P)
- return sclk * N / (M * P);
+ if (M * P == 0)
+ return 0;
- return 0;
+ return sclk * N / (M * P);
}
static int
--
2.9.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
2017-09-06 13:56 ` Arnd Bergmann
(?)
@ 2017-09-06 14:20 ` Karol Herbst
2017-09-06 20:11 ` Arnd Bergmann
-1 siblings, 1 reply; 9+ messages in thread
From: Karol Herbst @ 2017-09-06 14:20 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ben Skeggs, David Airlie, ML nouveau, dri-devel,
Linux Kernel Mailing List
On Wed, Sep 6, 2017 at 3:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> gcc thinks that interpreting a multiplication result as a bool
> is confusing:
>
> drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c: In function 'read_pll':
> drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c:133:8: error: '*' in boolean context, suggest '&&' instead [-Werror=int-in-bool-context]
>
> In this instance, I think using multiplication is more intuitive
> than '&&', so I'm adding a comparison to zero instead to shut up
> the warning. To further improve readability, I also make the
> error case indented and leave the normal case as the final 'return'
> statement.
>
I think to make perfectly clear why this check is done, we simply
should precompute the denominator and do something like this:
int denominator = M * P
if (denominator == 0)
return 0;
return sclk * N / denominator;
but with a better name for "denominator". I even imagine, that there
are some macros in the kernel for dealing with something like this
already, so that we could do instead:
return AWESOME_DIV_MACRO(sclk * N, M * P)
> Fixes: 7632b30e4b8b ("drm/nouveau/clk: namespace + nvidia gpu names (no binary change)")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Originally submitted on July 14, but no reply. This is the same
> patch again. The warning is currently disabled in mainline, but
> I think we can turn it back on in the future, and this change here
> seems harmless.
> ---
> drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
> index 96e0941c8edd..04b4f4ccf186 100644
> --- a/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
> +++ b/drivers/gpu/drm/nouveau/nvkm/subdev/clk/gt215.c
> @@ -130,10 +130,10 @@ read_pll(struct gt215_clk *clk, int idx, u32 pll)
> sclk = read_clk(clk, 0x10 + idx, false);
> }
>
> - if (M * P)
> - return sclk * N / (M * P);
> + if (M * P == 0)
> + return 0;
>
> - return 0;
> + return sclk * N / (M * P);
> }
>
> static int
> --
> 2.9.0
>
> _______________________________________________
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
@ 2017-09-06 20:11 ` Arnd Bergmann
0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-09-06 20:11 UTC (permalink / raw)
To: Karol Herbst
Cc: Ben Skeggs, David Airlie, ML nouveau, dri-devel,
Linux Kernel Mailing List
On Wed, Sep 6, 2017 at 4:20 PM, Karol Herbst <karolherbst@gmail.com> wrote:
>> In this instance, I think using multiplication is more intuitive
>> than '&&', so I'm adding a comparison to zero instead to shut up
>> the warning. To further improve readability, I also make the
>> error case indented and leave the normal case as the final 'return'
>> statement.
>>
>
> I think to make perfectly clear why this check is done, we simply
> should precompute the denominator and do something like this:
>
> int denominator = M * P
> if (denominator == 0)
> return 0;
> return sclk * N / denominator;
>
> but with a better name for "denominator".
I don't know what M and P actually are in this function, so I couldn't
come up with a much better name either, how about simply 'divisor'?
> I even imagine, that there
> are some macros in the kernel for dealing with something like this
> already, so that we could do instead:
>
> return AWESOME_DIV_MACRO(sclk * N, M * P)
I don't think that exists, the behavior for divide-by-zero really
depends a lot on the context, and returning zero is probably
often not a good solution.
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
@ 2017-09-06 20:11 ` Arnd Bergmann
0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-09-06 20:11 UTC (permalink / raw)
To: Karol Herbst
Cc: David Airlie, ML nouveau, Ben Skeggs, dri-devel,
Linux Kernel Mailing List
On Wed, Sep 6, 2017 at 4:20 PM, Karol Herbst <karolherbst@gmail.com> wrote:
>> In this instance, I think using multiplication is more intuitive
>> than '&&', so I'm adding a comparison to zero instead to shut up
>> the warning. To further improve readability, I also make the
>> error case indented and leave the normal case as the final 'return'
>> statement.
>>
>
> I think to make perfectly clear why this check is done, we simply
> should precompute the denominator and do something like this:
>
> int denominator = M * P
> if (denominator == 0)
> return 0;
> return sclk * N / denominator;
>
> but with a better name for "denominator".
I don't know what M and P actually are in this function, so I couldn't
come up with a much better name either, how about simply 'divisor'?
> I even imagine, that there
> are some macros in the kernel for dealing with something like this
> already, so that we could do instead:
>
> return AWESOME_DIV_MACRO(sclk * N, M * P)
I don't think that exists, the behavior for divide-by-zero really
depends a lot on the context, and returning zero is probably
often not a good solution.
Arnd
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
@ 2017-09-06 20:15 ` Karol Herbst
0 siblings, 0 replies; 9+ messages in thread
From: Karol Herbst @ 2017-09-06 20:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Ben Skeggs, David Airlie, ML nouveau, dri-devel,
Linux Kernel Mailing List
On Wed, Sep 6, 2017 at 10:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Sep 6, 2017 at 4:20 PM, Karol Herbst <karolherbst@gmail.com> wrote:
>>> In this instance, I think using multiplication is more intuitive
>>> than '&&', so I'm adding a comparison to zero instead to shut up
>>> the warning. To further improve readability, I also make the
>>> error case indented and leave the normal case as the final 'return'
>>> statement.
>>>
>>
>> I think to make perfectly clear why this check is done, we simply
>> should precompute the denominator and do something like this:
>>
>> int denominator = M * P
>> if (denominator == 0)
>> return 0;
>> return sclk * N / denominator;
>>
>> but with a better name for "denominator".
>
> I don't know what M and P actually are in this function, so I couldn't
> come up with a much better name either, how about simply 'divisor'?
>
what about "MP"? M and P are simply dividers for the PLL configuration
and there are two of them. I think "P" is the post divider.
>> I even imagine, that there
>> are some macros in the kernel for dealing with something like this
>> already, so that we could do instead:
>>
>> return AWESOME_DIV_MACRO(sclk * N, M * P)
>
> I don't think that exists, the behavior for divide-by-zero really
> depends a lot on the context, and returning zero is probably
> often not a good solution.
>
> Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
@ 2017-09-06 20:15 ` Karol Herbst
0 siblings, 0 replies; 9+ messages in thread
From: Karol Herbst @ 2017-09-06 20:15 UTC (permalink / raw)
To: Arnd Bergmann
Cc: David Airlie, ML nouveau, Ben Skeggs, dri-devel,
Linux Kernel Mailing List
On Wed, Sep 6, 2017 at 10:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wed, Sep 6, 2017 at 4:20 PM, Karol Herbst <karolherbst@gmail.com> wrote:
>>> In this instance, I think using multiplication is more intuitive
>>> than '&&', so I'm adding a comparison to zero instead to shut up
>>> the warning. To further improve readability, I also make the
>>> error case indented and leave the normal case as the final 'return'
>>> statement.
>>>
>>
>> I think to make perfectly clear why this check is done, we simply
>> should precompute the denominator and do something like this:
>>
>> int denominator = M * P
>> if (denominator == 0)
>> return 0;
>> return sclk * N / denominator;
>>
>> but with a better name for "denominator".
>
> I don't know what M and P actually are in this function, so I couldn't
> come up with a much better name either, how about simply 'divisor'?
>
what about "MP"? M and P are simply dividers for the PLL configuration
and there are two of them. I think "P" is the post divider.
>> I even imagine, that there
>> are some macros in the kernel for dealing with something like this
>> already, so that we could do instead:
>>
>> return AWESOME_DIV_MACRO(sclk * N, M * P)
>
> I don't think that exists, the behavior for divide-by-zero really
> depends a lot on the context, and returning zero is probably
> often not a good solution.
>
> Arnd
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
2017-09-06 20:15 ` Karol Herbst
@ 2017-09-06 21:40 ` Arnd Bergmann
-1 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-09-06 21:40 UTC (permalink / raw)
To: Karol Herbst
Cc: Ben Skeggs, David Airlie, ML nouveau, dri-devel,
Linux Kernel Mailing List
On Wed, Sep 6, 2017 at 10:15 PM, Karol Herbst <karolherbst@gmail.com> wrote:
> On Wed, Sep 6, 2017 at 10:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> but with a better name for "denominator".
>>
>> I don't know what M and P actually are in this function, so I couldn't
>> come up with a much better name either, how about simply 'divisor'?
>>
>
> what about "MP"? M and P are simply dividers for the PLL configuration
> and there are two of them. I think "P" is the post divider.
>
Fine with me. That was actually what I initially did after your reply and
then changed it to divisor. I've changed it back now and sent the new
version.
Arnd
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Nouveau] [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning
@ 2017-09-06 21:40 ` Arnd Bergmann
0 siblings, 0 replies; 9+ messages in thread
From: Arnd Bergmann @ 2017-09-06 21:40 UTC (permalink / raw)
To: Karol Herbst; +Cc: ML nouveau, Ben Skeggs, dri-devel, Linux Kernel Mailing List
On Wed, Sep 6, 2017 at 10:15 PM, Karol Herbst <karolherbst@gmail.com> wrote:
> On Wed, Sep 6, 2017 at 10:11 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> but with a better name for "denominator".
>>
>> I don't know what M and P actually are in this function, so I couldn't
>> come up with a much better name either, how about simply 'divisor'?
>>
>
> what about "MP"? M and P are simply dividers for the PLL configuration
> and there are two of them. I think "P" is the post divider.
>
Fine with me. That was actually what I initially did after your reply and
then changed it to divisor. I've changed it back now and sent the new
version.
Arnd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-09-06 21:41 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-09-06 13:56 [PATCH] [RESEND] drm/nouveau/clk: fix gcc-7 -Wint-in-bool-context warning Arnd Bergmann
2017-09-06 13:56 ` Arnd Bergmann
2017-09-06 14:20 ` [Nouveau] " Karol Herbst
2017-09-06 20:11 ` Arnd Bergmann
2017-09-06 20:11 ` Arnd Bergmann
2017-09-06 20:15 ` [Nouveau] " Karol Herbst
2017-09-06 20:15 ` Karol Herbst
2017-09-06 21:40 ` [Nouveau] " Arnd Bergmann
2017-09-06 21:40 ` Arnd Bergmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.