[U-Boot] [PATCH v2 2/5] usb: xhci: fsl: code cleanup for device tree fixup for fsl usb controllers

[Marek Vasut <marex@denx.de>]

On 06/02/2016 08:54 AM, Sriram Dash wrote:
> Performs code cleanup for device tree fixup for fsl usb controllers by
> making functions to handle these similar errata checking code.
> 
> Signed-off-by: Rajesh Bhagat <rajesh.bhagat@nxp.com>
> Signed-off-by: Sriram Dash <sriram.dash@nxp.com>
> ---
> Changes in v2:
>   - added patch description
>   - remove the MACRO and use fdt_fixup_erratum function instead
> 
> 
>  drivers/usb/common/fsl-dt-fixup.c | 58 +++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/usb/common/fsl-dt-fixup.c b/drivers/usb/common/fsl-dt-fixup.c
> index 6f31932..cbb008b 100644
> --- a/drivers/usb/common/fsl-dt-fixup.c
> +++ b/drivers/usb/common/fsl-dt-fixup.c
> @@ -99,6 +99,23 @@ static int fdt_fixup_usb_erratum(void *blob, const char *prop_erratum,
>  	return node_offset;
>  }
>  
> +void fdt_fixup_erratum(int *usb_erratum_off, void *blob,
> +		       char *str, bool (*has_erratum)(void))
> +{
> +	char buf[32] = {0};
> +
> +	snprintf(buf, sizeof(buf), "fsl,usb-erratum-%s", str);
> +	if (has_erratum()) {

Invert the condition here so you won't have the indentation problems below:

if (!(has_erratum())
	return;
... other stuff.


> +		*usb_erratum_off =  fdt_fixup_usb_erratum
> +				   (blob,
> +				    buf,
> +				    *usb_erratum_off);
> +		if (*usb_erratum_off < 0)
> +			return;

If fdt_fixup_usb_erratum() fails, this function will return, but the
code below will continue. This changes the logic of the code to the
worse, so fix this.

> +		debug("Adding USB erratum %s\n", str);
> +	}
> +}
> +
>  void fdt_fixup_dr_usb(void *blob, bd_t *bd)
>  {
>  	static const char * const modes[] = { "host", "peripheral", "otg" };
> @@ -164,39 +181,14 @@ void fdt_fixup_dr_usb(void *blob, bd_t *bd)
>  		if (usb_phy_off < 0)
>  			return;
>  
> -		if (has_erratum_a006261()) {
> -			usb_erratum_a006261_off =  fdt_fixup_usb_erratum
> -						   (blob,
> -						    "fsl,usb-erratum-a006261",
> -						    usb_erratum_a006261_off);
> -			if (usb_erratum_a006261_off < 0)
> -				return;
> -		}
> -
> -		if (has_erratum_a007075()) {
> -			usb_erratum_a007075_off =  fdt_fixup_usb_erratum
> -						   (blob,
> -						    "fsl,usb-erratum-a007075",
> -						    usb_erratum_a007075_off);
> -			if (usb_erratum_a007075_off < 0)
> -				return;
> -		}
> +		fdt_fixup_erratum(&usb_erratum_a006261_off, blob,
> +				  "a006261", has_erratum_a006261);
> +		fdt_fixup_erratum(&usb_erratum_a007075_off, blob,
> +				  "a007075", has_erratum_a007075);
> +		fdt_fixup_erratum(&usb_erratum_a007792_off, blob,
> +				  "a007792", has_erratum_a007792);
> +		fdt_fixup_erratum(&usb_erratum_a005697_off, blob,
> +				  "a005697", has_erratum_a005697);

Are these usb_erratum_aNNNNNN_off variables needed at all ? Why are they
even passed into the function ? I think they can be local to the function.

> -		if (has_erratum_a007792()) {
> -			usb_erratum_a007792_off =  fdt_fixup_usb_erratum
> -						   (blob,
> -						    "fsl,usb-erratum-a007792",
> -						    usb_erratum_a007792_off);
> -			if (usb_erratum_a007792_off < 0)
> -				return;
> -		}
> -		if (has_erratum_a005697()) {
> -			usb_erratum_a005697_off =  fdt_fixup_usb_erratum
> -						   (blob,
> -						    "fsl,usb-erratum-a005697",
> -						    usb_erratum_a005697_off);
> -			if (usb_erratum_a005697_off < 0)
> -				return;
> -		}
>  	}
>  }
> 


-- 
Best regards,
Marek Vasut