Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <CAMRc=Mdxaa43_znQg3Pd8gdBC-kPuxFnxf3CiQ=k7nPT4EVraQ@mail.gmail.com>
References: <20180213183946.1662-1-brgl@bgdev.pl> <CAHp75VeqaOwaAriXw3Ppvi6dtcK+yyTJUUpmj6gaxY8PLtzwpA@mail.gmail.com>
 <CAMRc=Mdxaa43_znQg3Pd8gdBC-kPuxFnxf3CiQ=k7nPT4EVraQ@mail.gmail.com>
From:   Andy Shevchenko <andy.shevchenko@gmail.com>
Date:   Wed, 14 Feb 2018 13:32:26 +0200
Message-ID: <CAHp75Vcxwke9Uw5+7wVz9vuMT9xDASCekMQqa4mdsznjSvpEoQ@mail.gmail.com>
Subject: Re: [PATCH v2] reset: add support for non-DT systems
To:     Bartosz Golaszewski <brgl@bgdev.pl>
Cc:     Philipp Zabel <p.zabel@pengutronix.de>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>,
        Sekhar Nori <nsekhar@ti.com>,
        Kevin Hilman <khilman@baylibre.com>,
        David Lechner <david@lechnology.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Feb 14, 2018 at 10:59 AM, Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2018-02-13 20:17 GMT+01:00 Andy Shevchenko <andy.shevchenko@gmail.com>:
>> On Tue, Feb 13, 2018 at 8:39 PM, Bartosz Golaszewski <brgl@bgdev.pl> 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