-----Original Message----- From: Marek Vasut [mailto:marex@denx.de] Sent: Thursday, March 03, 2016 3:29 PM To: Sriram Dash sriram.dash@nxp.com; u-boot@lists.denx.de Cc: york sun york.sun@nxp.com; Ramneek Mehresh ramneek.mehresh@nxp.com; Rajesh Bhagat rajesh.bhagat@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:marex@denx.de] Sent: Wednesday, March 02, 2016 3:57 AM To: Sriram Dash sriram.dash@nxp.com; u-boot@lists.denx.de Cc: york sun york.sun@nxp.com; Ramneek Mehresh ramneek.mehresh@nxp.com; Rajesh Bhagat rajesh.bhagat@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.mehresh@nxp.com Signed-off-by: Sriram Dash sriram.dash@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;
- 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); + static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode, const char *phy_type, int start_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_type = compat_dr; - } else { - node_type = compat_mph; - } + node_type = fdt_usb_get_node_type(blob, start_offset, &node_offset); + if (!node_type) + return -1;
if (mode) { err = fdt_setprop(blob, node_offset, prop_mode, mode,
#################################################################
+}
+static int fdt_fixup_usb_mode_phy_type(void *blob, const char *mode,
const char *phy_type, int start_offset) {- const char *prop_mode = "dr_mode";
- const char *prop_type = "phy_type";
- const char *node_type = NULL;
- int node_offset;
- int err;
- node_type = fdt_usb_get_node_type(blob, start_offset, &node_offset);
- if (!node_type)
return -1;- if (mode) { err = fdt_setprop(blob, node_offset, prop_mode, mode, strlen(mode) + 1);
@@ -64,32 +76,6 @@ static int fdt_fixup_usb_mode_phy_type(void *blob, const
char *mode,
return node_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 *node_type = NULL;
- *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;}- } else {
node_type = compat_mph;- }
- return node_type;
-}
static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum, int start_offset) {
-- Best regards, Marek Vasut
-- Best regards, Marek Vasut