[PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[Colin King]

From: Colin Ian King <colin.king@canonical.com>

The check to see if trigger is less than zero is always false, trigger
is always in the range 0..255.  Hence the check is redundant and can
be removed.

Addresses-Coverity: ("Logically dead code")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/net/can/kvaser_pciefd.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
index 3af747cbbde4..68e00aad0810 100644
--- a/drivers/net/can/kvaser_pciefd.c
+++ b/drivers/net/can/kvaser_pciefd.c
@@ -652,9 +652,6 @@ static void kvaser_pciefd_pwm_stop(struct kvaser_pciefd_can *can)
 	top = (pwm_ctrl >> KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT) & 0xff;
 
 	trigger = (100 * top + 50) / 100;
-	if (trigger < 0)
-		trigger = 0;
-
 	pwm_ctrl = trigger & 0xff;
 	pwm_ctrl |= (top & 0xff) << KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT;
 	iowrite32(pwm_ctrl, can->reg_base + KVASER_PCIEFD_KCAN_PWM_REG);
-- 
2.20.1

Re: [PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[walter harms]

Am 25.07.2019 13:25, schrieb Colin King:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The check to see if trigger is less than zero is always false, trigger
> is always in the range 0..255.  Hence the check is redundant and can
> be removed.
> 
> Addresses-Coverity: ("Logically dead code")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index 3af747cbbde4..68e00aad0810 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -652,9 +652,6 @@ static void kvaser_pciefd_pwm_stop(struct kvaser_pciefd_can *can)
>  	top = (pwm_ctrl >> KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT) & 0xff;
>  
>  	trigger = (100 * top + 50) / 100;
> -	if (trigger < 0)
> -		trigger = 0;
> -
	to be fair i do not understand the calculation here here.
	(100*top+50)/100 = top+50/100

	but seems to be int so it should be =top

	did i missunderstand something here ?

	re,
	 wh


>  	pwm_ctrl = trigger & 0xff;
>  	pwm_ctrl |= (top & 0xff) << KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT;
>  	iowrite32(pwm_ctrl, can->reg_base + KVASER_PCIEFD_KCAN_PWM_REG);

Re: [PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[Colin Ian King]

On 25/07/2019 13:37, walter harms wrote:
> 
> 
> Am 25.07.2019 13:25, schrieb Colin King:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The check to see if trigger is less than zero is always false, trigger
>> is always in the range 0..255.  Hence the check is redundant and can
>> be removed.
>>
>> Addresses-Coverity: ("Logically dead code")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/net/can/kvaser_pciefd.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
>> index 3af747cbbde4..68e00aad0810 100644
>> --- a/drivers/net/can/kvaser_pciefd.c
>> +++ b/drivers/net/can/kvaser_pciefd.c
>> @@ -652,9 +652,6 @@ static void kvaser_pciefd_pwm_stop(struct kvaser_pciefd_can *can)
>>  	top = (pwm_ctrl >> KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT) & 0xff;
>>  
>>  	trigger = (100 * top + 50) / 100;
>> -	if (trigger < 0)
>> -		trigger = 0;
>> -
> 	to be fair i do not understand the calculation here here.
> 	(100*top+50)/100 = top+50/100
> 
> 	but seems to be int so it should be =top

Indeed it does not do anything, that does look like an unintentional
bug. Good catch.

> 
> 	did i missunderstand something here ?
> 
> 	re,
> 	 wh
> 
> 
>>  	pwm_ctrl = trigger & 0xff;
>>  	pwm_ctrl |= (top & 0xff) << KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT;
>>  	iowrite32(pwm_ctrl, can->reg_base + KVASER_PCIEFD_KCAN_PWM_REG);

Re: [PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1407 bytes --]

Adding the Author(s) to Cc.

On 7/25/19 1:25 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
> 
> The check to see if trigger is less than zero is always false, trigger
> is always in the range 0..255.  Hence the check is redundant and can
> be removed.
> 
> Addresses-Coverity: ("Logically dead code")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>  drivers/net/can/kvaser_pciefd.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
> index 3af747cbbde4..68e00aad0810 100644
> --- a/drivers/net/can/kvaser_pciefd.c
> +++ b/drivers/net/can/kvaser_pciefd.c
> @@ -652,9 +652,6 @@ static void kvaser_pciefd_pwm_stop(struct kvaser_pciefd_can *can)
>  	top = (pwm_ctrl >> KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT) & 0xff;
>  
>  	trigger = (100 * top + 50) / 100;
> -	if (trigger < 0)
> -		trigger = 0;
> -
>  	pwm_ctrl = trigger & 0xff;
>  	pwm_ctrl |= (top & 0xff) << KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT;
>  	iowrite32(pwm_ctrl, can->reg_base + KVASER_PCIEFD_KCAN_PWM_REG);
> 

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1657 bytes --]

On 8/2/19 2:07 PM, Marc Kleine-Budde wrote:
> Adding the Author(s) to Cc.
> 
> On 7/25/19 1:25 PM, Colin King wrote:
>> From: Colin Ian King <colin.king@canonical.com>
>>
>> The check to see if trigger is less than zero is always false, trigger
>> is always in the range 0..255.  Hence the check is redundant and can
>> be removed.
>>
>> Addresses-Coverity: ("Logically dead code")
>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>> ---
>>  drivers/net/can/kvaser_pciefd.c | 3 ---
>>  1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
>> index 3af747cbbde4..68e00aad0810 100644
>> --- a/drivers/net/can/kvaser_pciefd.c
>> +++ b/drivers/net/can/kvaser_pciefd.c
>> @@ -652,9 +652,6 @@ static void kvaser_pciefd_pwm_stop(struct kvaser_pciefd_can *can)
>>  	top = (pwm_ctrl >> KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT) & 0xff;
>>  
>>  	trigger = (100 * top + 50) / 100;

As Walter pointed out the above code makes no sense in the first place.

>> -	if (trigger < 0)
>> -		trigger = 0;
>> -

Can someone have a deeper look at this code section and decide what to
do with this finding.

>>  	pwm_ctrl = trigger & 0xff;
>>  	pwm_ctrl |= (top & 0xff) << KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT;
>>  	iowrite32(pwm_ctrl, can->reg_base + KVASER_PCIEFD_KCAN_PWM_REG);
>>

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[Christer Beskow]

On 2019-08-02 14:12,  Marc Kleine-Budde wrote:
> On 8/2/19 2:07 PM, Marc Kleine-Budde wrote:
>> Adding the Author(s) to Cc.
>>
>> On 7/25/19 1:25 PM, Colin King wrote:
>>> From: Colin Ian King <colin.king@canonical.com>
>>>
>>> The check to see if trigger is less than zero is always false, trigger
>>> is always in the range 0..255.  Hence the check is redundant and can
>>> be removed.
>>>
>>> Addresses-Coverity: ("Logically dead code")
>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>> ---
>>>   drivers/net/can/kvaser_pciefd.c | 3 ---
>>>   1 file changed, 3 deletions(-)
>>>
>>> diff --git a/drivers/net/can/kvaser_pciefd.c b/drivers/net/can/kvaser_pciefd.c
>>> index 3af747cbbde4..68e00aad0810 100644
>>> --- a/drivers/net/can/kvaser_pciefd.c
>>> +++ b/drivers/net/can/kvaser_pciefd.c
>>> @@ -652,9 +652,6 @@ static void kvaser_pciefd_pwm_stop(struct kvaser_pciefd_can *can)
>>>   	top = (pwm_ctrl >> KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT) & 0xff;
>>>   
>>>   	trigger = (100 * top + 50) / 100;
> As Walter pointed out the above code makes no sense in the first place.
>
>>> -	if (trigger < 0)
>>> -		trigger = 0;
>>> -
> Can someone have a deeper look at this code section and decide what to
> do with this finding.

I will look at this at once and come back with a patch later today.

>>>   	pwm_ctrl = trigger & 0xff;
>>>   	pwm_ctrl |= (top & 0xff) << KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT;
>>>   	iowrite32(pwm_ctrl, can->reg_base + KVASER_PCIEFD_KCAN_PWM_REG);
>>>
> regards,
> Marc

regards,

Christer Beskow

Re: [PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[Christer Beskow]

Den 2019-08-05 kl. 09:21, skrev Christer Beskow:
> On 2019-08-02 14:12,  Marc Kleine-Budde wrote:
>> On 8/2/19 2:07 PM, Marc Kleine-Budde wrote:
>>> Adding the Author(s) to Cc.
>>>
>>> On 7/25/19 1:25 PM, Colin King wrote:
>>>> From: Colin Ian King <colin.king@canonical.com>
>>>>
>>>> The check to see if trigger is less than zero is always false, trigger
>>>> is always in the range 0..255.  Hence the check is redundant and can
>>>> be removed.
>>>>
>>>> Addresses-Coverity: ("Logically dead code")
>>>> Signed-off-by: Colin Ian King <colin.king@canonical.com>
>>>> ---
>>>>   drivers/net/can/kvaser_pciefd.c | 3 ---
>>>>   1 file changed, 3 deletions(-)
>>>>
>>>> diff --git a/drivers/net/can/kvaser_pciefd.c 
>>>> b/drivers/net/can/kvaser_pciefd.c
>>>> index 3af747cbbde4..68e00aad0810 100644
>>>> --- a/drivers/net/can/kvaser_pciefd.c
>>>> +++ b/drivers/net/can/kvaser_pciefd.c
>>>> @@ -652,9 +652,6 @@ static void kvaser_pciefd_pwm_stop(struct 
>>>> kvaser_pciefd_can *can)
>>>>       top = (pwm_ctrl >> KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT) & 0xff;
>>>>         trigger = (100 * top + 50) / 100;
>> As Walter pointed out the above code makes no sense in the first place.
>>
>>>> -    if (trigger < 0)
>>>> -        trigger = 0;
>>>> -
>> Can someone have a deeper look at this code section and decide what to
>> do with this finding.
>
> I will look at this at once and come back with a patch later today.
>
>>>>       pwm_ctrl = trigger & 0xff;
>>>>       pwm_ctrl |= (top & 0xff) << KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT;
>>>>       iowrite32(pwm_ctrl, can->reg_base + KVASER_PCIEFD_KCAN_PWM_REG);
>>>>
To set the duty cycle to zero (i.e. pwm_stop), the trigger value shall 
be equal to the top value.

This is achieved by reading the value of the top bit field from the pwm 
register and then writing back this value to the trigger and top bit 
fields.

The current code is, as you all pointed out, a bit cumbersome. I will 
send a new patch in a new thread.

regards

Christer

Re: [PATCH][next] can: kvaser_pciefd: remove redundant negative check on trigger

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 1006 bytes --]

On 8/6/19 8:43 AM, Christer Beskow wrote:
>>>>>       pwm_ctrl = trigger & 0xff;
>>>>>       pwm_ctrl |= (top & 0xff) << KVASER_PCIEFD_KCAN_PWM_TOP_SHIFT;
>>>>>       iowrite32(pwm_ctrl, can->reg_base + KVASER_PCIEFD_KCAN_PWM_REG);
>>>>>
> To set the duty cycle to zero (i.e. pwm_stop), the trigger value shall 
> be equal to the top value.
> 
> This is achieved by reading the value of the top bit field from the pwm 
> register and then writing back this value to the trigger and top bit 
> fields.
> 
> The current code is, as you all pointed out, a bit cumbersome. I will 
> send a new patch in a new thread.

Thanks. Please include this description in the patch description.

regards,
Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]