[PATCH v5 1/4] ARM: mvebu: add broken-idle option

[gregory.clement@free-electrons.com (Gregory CLEMENT)]

Hi Simon and Vincent,
 
 On jeu., oct. 22 2015, Olof Johansson <olof@lixom.net> wrote:

>> +static int broken_idle(struct device_node *np)
>> +{
>> +       if (of_property_read_bool(np, "broken-idle")) {
>> +               pr_warn("CPU idle is currently broken: disabling\n");
>> +               return 0;
>> +       }
>> +
>> +       return 1;
>> +}
>
> This is confusing. The function is called broken_idle(), but it
> returns 0 if idle is broken and 1 if it isn't.
>
> It means these tests look odd:
>
>> +
>>  static __init int armada_370_cpuidle_init(void)
>>  {
>>         struct device_node *np;
>> @@ -387,7 +397,9 @@ static __init int armada_370_cpuidle_init(void)
>>         np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric");
>>         if (!np)
>>                 return -ENODEV;
>> -       of_node_put(np);
>> +
>> +       if (!broken_idle(np))
>> +               goto end;
>
> So, the way I read this when I read just this code is: "If idle is NOT
> broken, then don't bother set up any of the idle stuff".
>
> Please turn this the other way around so others don't make the same
> mistake when reading the code.
>
> I know it might come across as bikesheddy and nitpicky, but
> readability trumps most other things when it comes to new code. :-/

Could you send an updated version ? Then I will be able to make a new
pull request following it as requested by Olof. Then it will still be
part of 4.4.

Thanks,

Gregory

>
>
>
> -Olof

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com