| Message ID | 1456815817-18578-3-git-send-email-sriram.dash@nxp.com |
|---|---|
| State | Superseded |
| Delegated to: | York Sun |
| Headers | show |
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; > } 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. > +} > + > +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) > { >
>-----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; >> } 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. >> +} >> + >> +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
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; >>> } 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. >>> +} >>> + >>> +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
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; +} + +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) {