[U-Boot] [PATCH 2/8] dm: core: remove meaningless if conditional

[Simon Glass]

Hi Masahiro,

On 17 November 2014 11:19, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
> Hi Simon,
>
>
>
> On Mon, 17 Nov 2014 09:14:15 +0000
> Simon Glass <sjg at chromium.org> wrote:
>
>> Hi Masahiro,
>>
>> On 17 November 2014 08:19, Masahiro Yamada <yamada.m at jp.panasonic.com> wrote:
>> > If the variable "ret" is equal to "-ENOENT", it is trapped at [1] and
>> > never reaches [2].  At [3], the condition "ret != -ENOENT" is always
>> > true.
>> >
>> >   if (ret == -ENOENT) {                       <------------------ [1]
>> >           continue;
>> >   } else if (ret == -ENODEV) {
>> >           dm_dbg("Device '%s' has no compatible string\n", name);
>> >           break;
>> >   } else if (ret) {                           <------------------ [2]
>> >           dm_warn("Device tree error at offset %d\n", offset);
>> >           if (!result || ret != -ENOENT)      <------------------ [3]
>> >                   result = ret;
>> >           break;
>> >   }
>> >
>> > Signed-off-by: Masahiro Yamada <yamada.m at jp.panasonic.com>
>> > ---
>> >
>> >  drivers/core/lists.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/core/lists.c b/drivers/core/lists.c
>> > index 3a1ea85..0aad56d 100644
>> > --- a/drivers/core/lists.c
>> > +++ b/drivers/core/lists.c
>> > @@ -136,8 +136,7 @@ int lists_bind_fdt(struct udevice *parent, const void *blob, int offset,
>> >                         break;
>> >                 } else if (ret) {
>> >                         dm_warn("Device tree error at offset %d\n", offset);
>> > -                       if (!result || ret != -ENOENT)
>> > -                               result = ret;
>> > +                       result = ret;
>>
>> Thanks for the clear explanation.
>> It looks good, except my intent was to return the first error, hence
>> the 'if (!result ...'.
>>
>> Your code will return the last error. Can we preserve the current bahaviour?
>
>
> Do you mean, like this?
>
>            dm_warn("Device tree error at offset %d\n", offset);
>            if (!result)
>                    result = ret;
>            break;
>
>
> I think there is no difference in the behaviour, because
> this "for" loop is escaped out at the first encounter with an error
> except -ENOENT.
>
> Here is the only line to set "result" and it is followed by "break" statement,
> so the last error is also the first error, I think.

Ah yes, OK. The intent here is to provide a useful error message
without adding a lot of noise. It's a bit of a pain but we should keep
it I think. Your patch looks good.

Acked-by: Simon Glass <sjg at chromium.org>

Regards,
Simon