Re: [PATCH v2] reset: add support for non-DT systems

[Andy Shevchenko]

On Wed, Feb 14, 2018 at 10:59 AM, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:
> 2018-02-13 20:17 GMT+01:00 Andy Shevchenko <andy.shevchenko@xxxxxxxxx>:
>> On Tue, Feb 13, 2018 at 8:39 PM, Bartosz Golaszewski <brgl@xxxxxxxx> wrote:

>>> +               for (index = 0; lookup->dev; index++, lookup++) {
>>> +                       if (strcmp(dev_id, lookup->dev))
>>> +                               continue;
>>> +
>>> +                       if ((!id && !lookup->id) ||
>>> +                           (id && lookup->id && !strcmp(id, lookup->id))) {
>>> +                               rstc = __reset_control_get_internal(rcdev,
>>> +                                                               index, shared);
>>> +                               break;
>>> +                       }
>>
>> Wouldn't be slightly more readable
>>
>>       if (id && lookup->id) {
>>         if (strcmp(id, lookup->id))
>>           continue;
>>       } else if (id || lookup->id) {
>>         continue;
>>       }
>>
>>       rstc = __reset_control_get_internal(rcdev, index, shared);
>>       break;
>>
>
> You'd get less indentations, yes but I wanted to emphasize the
> condition on which we want to stop in this line.

It doesn't matter (indentation) here, esp. taking into consideration
that you already have another condition by which you continue the
loop.
My proposal adds consistency and from my point of view makes easier to parse.

You check all false conditions in a row, end up with a true one.

>>> +       if (!rstc)
>>> +               return optional ? NULL : ERR_PTR(-ENOENT);
>>
>> Isn't simpler
>>
>> if (!optional && !rstc) // or other way around, depending which might
>> be more offten
>>  return ERR_PTR(...);
>>
>
> IMO it's just a matter of taste.

I think in my proposal it's more straight forward. Easier to parse by reader.

-- 
With Best Regards,
Andy Shevchenko