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

[Simon Glass]

On 17 November 2014 at 19:23, Simon Glass <sjg at chromium.org> wrote:
> 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>

Applied to u-boot-dm.