Linux-LEDs Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] leds: ti-lmu-common: Fix coccinelle issue in TI LMU
@ 2019-08-23 19:55 Dan Murphy
  2019-08-24 15:18 ` Jacek Anaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Murphy @ 2019-08-23 19:55 UTC (permalink / raw)
  To: jacek.anaszewski, pavel; +Cc: linux-leds, linux-kernel, Dan Murphy

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

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;
-- 
2.22.0.214.g8dca754b1e


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] leds: ti-lmu-common: Fix coccinelle issue in TI LMU
  2019-08-23 19:55 [PATCH] leds: ti-lmu-common: Fix coccinelle issue in TI LMU Dan Murphy
@ 2019-08-24 15:18 ` Jacek Anaszewski
  2019-08-26 14:53   ` Dan Murphy
  0 siblings, 1 reply; 6+ messages in thread
From: Jacek Anaszewski @ 2019-08-24 15:18 UTC (permalink / raw)
  To: Dan Murphy, pavel; +Cc: linux-leds, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] leds: ti-lmu-common: Fix coccinelle issue in TI LMU
  2019-08-24 15:18 ` Jacek Anaszewski
@ 2019-08-26 14:53   ` Dan Murphy
  2019-08-26 19:34     ` Jacek Anaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Murphy @ 2019-08-26 14:53 UTC (permalink / raw)
  To: Jacek Anaszewski, pavel; +Cc: linux-leds, linux-kernel

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

[...]


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] leds: ti-lmu-common: Fix coccinelle issue in TI LMU
  2019-08-26 14:53   ` Dan Murphy
@ 2019-08-26 19:34     ` Jacek Anaszewski
  2019-08-27 13:37       ` Dan Murphy
  0 siblings, 1 reply; 6+ messages in thread
From: Jacek Anaszewski @ 2019-08-26 19:34 UTC (permalink / raw)
  To: Dan Murphy, pavel; +Cc: linux-leds, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] leds: ti-lmu-common: Fix coccinelle issue in TI LMU
  2019-08-26 19:34     ` Jacek Anaszewski
@ 2019-08-27 13:37       ` Dan Murphy
  2019-08-27 21:02         ` Jacek Anaszewski
  0 siblings, 1 reply; 6+ messages in thread
From: Dan Murphy @ 2019-08-27 13:37 UTC (permalink / raw)
  To: Jacek Anaszewski, pavel; +Cc: linux-leds, linux-kernel

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] leds: ti-lmu-common: Fix coccinelle issue in TI LMU
  2019-08-27 13:37       ` Dan Murphy
@ 2019-08-27 21:02         ` Jacek Anaszewski
  0 siblings, 0 replies; 6+ messages in thread
From: Jacek Anaszewski @ 2019-08-27 21:02 UTC (permalink / raw)
  To: Dan Murphy, pavel; +Cc: linux-leds, linux-kernel

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 19:55 [PATCH] leds: ti-lmu-common: Fix coccinelle issue in TI LMU Dan Murphy
2019-08-24 15:18 ` Jacek Anaszewski
2019-08-26 14:53   ` Dan Murphy
2019-08-26 19:34     ` Jacek Anaszewski
2019-08-27 13:37       ` Dan Murphy
2019-08-27 21:02         ` Jacek Anaszewski

Linux-LEDs Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-leds/0 linux-leds/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-leds linux-leds/ https://lore.kernel.org/linux-leds \
		linux-leds@vger.kernel.org linux-leds@archiver.kernel.org
	public-inbox-index linux-leds


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-leds


AGPL code for this site: git clone https://public-inbox.org/ public-inbox