* [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 related [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, other threads:[~2019-08-27 21:02 UTC | newest]
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
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).