leds: ti-lmu-common: Fix coccinelle issue in TI LMU

Message ID 20190823195523.20950-1-dmurphy@ti.com
State New
Headers show
Series
  • leds: ti-lmu-common: Fix coccinelle issue in TI LMU
Related show

Commit Message

Dan Murphy Aug. 23, 2019, 7:55 p.m.
Fix the coccinelle issues found in the TI LMU common code

drivers/leds/leds-ti-lmu-common.c:97:20-29: WARNING: Unsigned expression compared with zero: ramp_down < 0
drivers/leds/leds-ti-lmu-common.c:97:5-12: WARNING: Unsigned expression compared with zero: ramp_up < 0

Fixes: f717460ba4d7 ("leds: TI LMU: Add common code for TI LMU devices")
Signed-off-by: Dan Murphy <dmurphy@ti.com>

---
 drivers/leds/leds-ti-lmu-common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.22.0.214.g8dca754b1e

Comments

Jacek Anaszewski Aug. 24, 2019, 3:18 p.m. | #1
Hi Dan,

Thank you for the patch.

On 8/23/19 9:55 PM, Dan Murphy wrote:
> Fix the coccinelle issues found in the TI LMU common code

> 

> drivers/leds/leds-ti-lmu-common.c:97:20-29: WARNING: Unsigned expression compared with zero: ramp_down < 0

> drivers/leds/leds-ti-lmu-common.c:97:5-12: WARNING: Unsigned expression compared with zero: ramp_up < 0


Wouldn't it make more sense to remove those pointless checks?
Clearly a correct index of an array cannot be negative.
Looking at the code I would make more int -> unsigned int conversions:

- ramp_table should be unsigned int
- ti_lmu_common_convert_ramp_to_index should return unsigned int


> Fixes: f717460ba4d7 ("leds: TI LMU: Add common code for TI LMU devices")

> Signed-off-by: Dan Murphy <dmurphy@ti.com>

> ---

>  drivers/leds/leds-ti-lmu-common.c | 2 +-

>  1 file changed, 1 insertion(+), 1 deletion(-)

> 

> diff --git a/drivers/leds/leds-ti-lmu-common.c b/drivers/leds/leds-ti-lmu-common.c

> index adc7293004f1..c9ab40d5a6ba 100644

> --- a/drivers/leds/leds-ti-lmu-common.c

> +++ b/drivers/leds/leds-ti-lmu-common.c

> @@ -84,7 +84,7 @@ static int ti_lmu_common_convert_ramp_to_index(unsigned int usec)

>  int ti_lmu_common_set_ramp(struct ti_lmu_bank *lmu_bank)

>  {

>  	struct regmap *regmap = lmu_bank->regmap;

> -	u8 ramp, ramp_up, ramp_down;

> +	int ramp, ramp_up, ramp_down;

>  

>  	if (lmu_bank->ramp_up_usec == 0 && lmu_bank->ramp_down_usec == 0) {

>  		ramp_up = 0;

> 


-- 
Best regards,
Jacek Anaszewski
Dan Murphy Aug. 26, 2019, 2:53 p.m. | #2
Jacek

On 8/24/19 10:18 AM, Jacek Anaszewski wrote:
> Hi Dan,

>

> Thank you for the patch.

>

> On 8/23/19 9:55 PM, Dan Murphy wrote:

>> Fix the coccinelle issues found in the TI LMU common code

>>

>> drivers/leds/leds-ti-lmu-common.c:97:20-29: WARNING: Unsigned expression compared with zero: ramp_down < 0

>> drivers/leds/leds-ti-lmu-common.c:97:5-12: WARNING: Unsigned expression compared with zero: ramp_up < 0

> Wouldn't it make more sense to remove those pointless checks?

> Clearly a correct index of an array cannot be negative.

> Looking at the code I would make more int -> unsigned int conversions:

>

> - ramp_table should be unsigned int

> - ti_lmu_common_convert_ramp_to_index should return unsigned int

>

Yeah I was going to just remove the code but when I was writing the 
original code my intent was

to extend the ramp call to allow other TI LMU driver to pass in the 
device specific ramp table.

But since I don't currently have any devices on my plate that require 
that I can just remove the code as well

Dan

[...]
Jacek Anaszewski Aug. 26, 2019, 7:34 p.m. | #3
Dan,

On 8/26/19 4:53 PM, Dan Murphy wrote:
> Jacek

> 

> On 8/24/19 10:18 AM, Jacek Anaszewski wrote:

>> Hi Dan,

>>

>> Thank you for the patch.

>>

>> On 8/23/19 9:55 PM, Dan Murphy wrote:

>>> Fix the coccinelle issues found in the TI LMU common code

>>>

>>> drivers/leds/leds-ti-lmu-common.c:97:20-29: WARNING: Unsigned

>>> expression compared with zero: ramp_down < 0

>>> drivers/leds/leds-ti-lmu-common.c:97:5-12: WARNING: Unsigned

>>> expression compared with zero: ramp_up < 0

>> Wouldn't it make more sense to remove those pointless checks?

>> Clearly a correct index of an array cannot be negative.

>> Looking at the code I would make more int -> unsigned int conversions:

>>

>> - ramp_table should be unsigned int

>> - ti_lmu_common_convert_ramp_to_index should return unsigned int

>>

> Yeah I was going to just remove the code but when I was writing the

> original code my intent was

> 

> to extend the ramp call to allow other TI LMU driver to pass in the

> device specific ramp table.

> 

> But since I don't currently have any devices on my plate that require

> that I can just remove the code as well


You don't need to remove, just do the conversions I proposed.
Unless it introduces some other problems I am currently not aware of.

-- 
Best regards,
Jacek Anaszewski
Dan Murphy Aug. 27, 2019, 1:37 p.m. | #4
Jacek

On 8/26/19 2:34 PM, Jacek Anaszewski wrote:
> Dan,

>

> On 8/26/19 4:53 PM, Dan Murphy wrote:

>> Jacek

>>

>> On 8/24/19 10:18 AM, Jacek Anaszewski wrote:

>>> Hi Dan,

>>>

>>> Thank you for the patch.

>>>

>>> On 8/23/19 9:55 PM, Dan Murphy wrote:

>>>> Fix the coccinelle issues found in the TI LMU common code

>>>>

>>>> drivers/leds/leds-ti-lmu-common.c:97:20-29: WARNING: Unsigned

>>>> expression compared with zero: ramp_down < 0

>>>> drivers/leds/leds-ti-lmu-common.c:97:5-12: WARNING: Unsigned

>>>> expression compared with zero: ramp_up < 0

>>> Wouldn't it make more sense to remove those pointless checks?

>>> Clearly a correct index of an array cannot be negative.

>>> Looking at the code I would make more int -> unsigned int conversions:

>>>

>>> - ramp_table should be unsigned int

>>> - ti_lmu_common_convert_ramp_to_index should return unsigned int

>>>

>> Yeah I was going to just remove the code but when I was writing the

>> original code my intent was

>>

>> to extend the ramp call to allow other TI LMU driver to pass in the

>> device specific ramp table.

>>

>> But since I don't currently have any devices on my plate that require

>> that I can just remove the code as well

> You don't need to remove, just do the conversions I proposed.

> Unless it introduces some other problems I am currently not aware of.

>

Well just converting those two would/did not fix the issue.

But actually there is only 1 possibility that could happen if the 
convert function returns -EINVAL

So the check should be

if (ramp_up == -EINVAL || ramp_down == -EINVAL)

Because ramp_up/down should never be less then zero otherwise.

Dan
Jacek Anaszewski Aug. 27, 2019, 9:02 p.m. | #5
Dan,

On 8/27/19 3:37 PM, Dan Murphy wrote:
> Jacek

> 

> On 8/26/19 2:34 PM, Jacek Anaszewski wrote:

>> Dan,

>>

>> On 8/26/19 4:53 PM, Dan Murphy wrote:

>>> Jacek

>>>

>>> On 8/24/19 10:18 AM, Jacek Anaszewski wrote:

>>>> Hi Dan,

>>>>

>>>> Thank you for the patch.

>>>>

>>>> On 8/23/19 9:55 PM, Dan Murphy wrote:

>>>>> Fix the coccinelle issues found in the TI LMU common code

>>>>>

>>>>> drivers/leds/leds-ti-lmu-common.c:97:20-29: WARNING: Unsigned

>>>>> expression compared with zero: ramp_down < 0

>>>>> drivers/leds/leds-ti-lmu-common.c:97:5-12: WARNING: Unsigned

>>>>> expression compared with zero: ramp_up < 0

>>>> Wouldn't it make more sense to remove those pointless checks?

>>>> Clearly a correct index of an array cannot be negative.

>>>> Looking at the code I would make more int -> unsigned int conversions:

>>>>

>>>> - ramp_table should be unsigned int

>>>> - ti_lmu_common_convert_ramp_to_index should return unsigned int

>>>>

>>> Yeah I was going to just remove the code but when I was writing the

>>> original code my intent was

>>>

>>> to extend the ramp call to allow other TI LMU driver to pass in the

>>> device specific ramp table.

>>>

>>> But since I don't currently have any devices on my plate that require

>>> that I can just remove the code as well

>> You don't need to remove, just do the conversions I proposed.

>> Unless it introduces some other problems I am currently not aware of.

>>

> Well just converting those two would/did not fix the issue.


I implicitly assumed that you'd just drop the check since it
would make no sense to check unsigned int for being lower than 0.

And I propose to not return any error code from
ti_lmu_common_convert_ramp_to_index(), just make sure inside it
you return sane value. Ramp should, well, ramp.

> 

> But actually there is only 1 possibility that could happen if the

> convert function returns -EINVAL

> 

> So the check should be

> 

> if (ramp_up == -EINVAL || ramp_down == -EINVAL)

> 

> Because ramp_up/down should never be less then zero otherwise.

> 

> Dan

> 

> 

> 


-- 
Best regards,
Jacek Anaszewski

Patch

diff --git a/drivers/leds/leds-ti-lmu-common.c b/drivers/leds/leds-ti-lmu-common.c
index adc7293004f1..c9ab40d5a6ba 100644
--- a/drivers/leds/leds-ti-lmu-common.c
+++ b/drivers/leds/leds-ti-lmu-common.c
@@ -84,7 +84,7 @@  static int ti_lmu_common_convert_ramp_to_index(unsigned int usec)
 int ti_lmu_common_set_ramp(struct ti_lmu_bank *lmu_bank)
 {
 	struct regmap *regmap = lmu_bank->regmap;
-	u8 ramp, ramp_up, ramp_down;
+	int ramp, ramp_up, ramp_down;
 
 	if (lmu_bank->ramp_up_usec == 0 && lmu_bank->ramp_down_usec == 0) {
 		ramp_up = 0;