On 03/03/2016 12:11 PM, Sriram Dash wrote:
> 
> 
>> -----Original Message-----
>> From: Marek Vasut [mailto:ma...@denx.de]
>> Sent: Thursday, March 03, 2016 3:29 PM
>> To: Sriram Dash <sriram.d...@nxp.com>; u-boot@lists.denx.de
>> Cc: york sun <york....@nxp.com>; Ramneek Mehresh
>> <ramneek.mehr...@nxp.com>; Rajesh Bhagat <rajesh.bha...@nxp.com>
>> Subject: Re: [PATCH v3 2/3] board:freescale:usb: Remove code duplication for
>> fdt_usb_get_node_type
>>
>> On 03/03/2016 09:30 AM, Sriram Dash wrote:
>>>
>>>> -----Original Message-----
>>>> From: Marek Vasut [mailto:ma...@denx.de]
>>>> Sent: Wednesday, March 02, 2016 3:57 AM
>>>> To: Sriram Dash <sriram.d...@nxp.com>; u-boot@lists.denx.de
>>>> Cc: york sun <york....@nxp.com>; Ramneek Mehresh
>>>> <ramneek.mehr...@nxp.com>; Rajesh Bhagat <rajesh.bha...@nxp.com>
>>>> Subject: Re: [PATCH v3 2/3] board:freescale:usb: Remove code
>>>> duplication for fdt_usb_get_node_type
>>>>
>>>> On 03/01/2016 08:03 AM, Sriram Dash wrote:
>>>>> Call fdt_usb_get_node_type() from fdt_fixup_usb_mode_phy_type() to
>>>>> avoid code duplication.
>>>>>
>>>>> Signed-off-by: Ramneek Mehresh <ramneek.mehr...@nxp.com>
>>>>> Signed-off-by: Sriram Dash <sriram.d...@nxp.com>
>>>>> ---
>>>>>  board/freescale/common/usb.c | 72
>>>>> ++++++++++++++++++--------------------------
>>>>>  1 file changed, 29 insertions(+), 43 deletions(-)
>>>>>
>>>>> diff --git a/board/freescale/common/usb.c
>>>>> b/board/freescale/common/usb.c index 85cb1bf..d815dc1 100644
>>>>> --- a/board/freescale/common/usb.c
>>>>> +++ b/board/freescale/common/usb.c
>>>>> @@ -18,33 +18,45 @@
>>>>>  #define CONFIG_USB_MAX_CONTROLLER_COUNT 1  #endif
>>>>>
>>>>> -static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
>>>>> -                                const char *phy_type, int start_offset)
>>>>> +static const char *fdt_usb_get_node_type(void *blob, int start_offset,
>>>>> +                                  int *node_offset)
>>>>>  {
>>>>>   const char *compat_dr = "fsl-usb2-dr";
>>>>>   const char *compat_mph = "fsl-usb2-mph";
>>>>> - const char *prop_mode = "dr_mode";
>>>>> - const char *prop_type = "phy_type";
>>>>>   const char *node_type = NULL;
>>>>> - int node_offset;
>>>>> - int err;
>>>>>
>>>>> - node_offset = fdt_node_offset_by_compatible(blob,
>>>>> -                                             start_offset, compat_mph);
>>>>> - if (node_offset < 0) {
>>>>> -         node_offset = fdt_node_offset_by_compatible(blob,
>>>>> -                                                     start_offset,
>>>>> -                                                     compat_dr);
>>>>> -         if (node_offset < 0) {
>>>>> -                 printf("WARNING: could not find compatible node: %s",
>>>>> -                        fdt_strerror(node_offset));
>>>>> -                 return -1;
>>>>> + *node_offset = fdt_node_offset_by_compatible(blob, start_offset,
>>>>> +                                              compat_mph);
>>>>> + if (*node_offset < 0) {
>>>>> +         *node_offset = fdt_node_offset_by_compatible(blob,
>>>>> +                                                      start_offset,
>>>>> +                                                      compat_dr);
>>>>> +         if (*node_offset < 0) {
>>>>> +                 printf("ERROR: could not find compatible node: %s\n",
>>>>> +                        fdt_strerror(*node_offset));
>>>>> +         } else {
>>>>> +                 node_type = compat_dr;
>>>>>           }
>>>>> -         node_type = compat_dr;
>>>>>   } else {
>>>>>           node_type = compat_mph;
>>>>>   }
>>>>>
>>>>> + return node_type;
>>>>
>>>> The function should be able to return error code. If you need to
>>>> return some more stuff from the function, return it via reference.
>>>>
>>>
>>> This patch is not altering the fdt_usb_get_node_type(). It is only
>>> calling the function from fdt_fixup_usb_mode_phy_type(), to avoid code
>> duplication.
>>
>> I am not complaining about that part. I am complaining about the new 
>> function,
>> fdt_usb_get_node_type(), which returns pointer as a return value instead of
>> returning proper error code as a return value.
>>
> 
> I think git diff tool is creating confusion here, I have just moved function 
> fdt_usb_get_node_type
> above fdt_fixup_usb_mode_phy_type which created all the diff and looked as if 
> new function is 
> added. 
> 
> Please check below created patch where declaration is added to call 
> fdt_usb_get_node_type 
> in fdt_usb_get_mode_phy_type. We will be sending below in V4. 
> 
> #################################################################
> 
> diff --git a/board/freescale/common/usb.c b/board/freescale/common/usb.c
> index 85cb1bf..5553225 100644
> --- a/board/freescale/common/usb.c
> +++ b/board/freescale/common/usb.c
> @@ -18,32 +18,21 @@
>  #define CONFIG_USB_MAX_CONTROLLER_COUNT 1
>  #endif
>  
> +const char *fdt_usb_get_node_type(void *blob, int start_offset,
> +                                        int *node_offset);
> +

Ah I see, I am wrong, sorry.

btw can you fix the fdt_usb_get_node_type() in another patch to return int ?

Thanks!
Best regards,
Marek Vasut
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to