[U-Boot] [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support for xhci controller

[Sriram Dash]

>-----Original Message-----
>From: Marek Vasut [mailto:marex at denx.de]
>Sent: Wednesday, March 02, 2016 3:55 AM
>To: Sriram Dash <sriram.dash at nxp.com>; u-boot at lists.denx.de
>Cc: york sun <york.sun at nxp.com>; Ramneek Mehresh
><ramneek.mehresh at nxp.com>; Rajesh Bhagat <rajesh.bhagat at nxp.com>
>Subject: Re: [PATCH v3 3/3] board:freescale:usb: Add device-tree fixup support for
>xhci controller
>
>On 03/01/2016 08:03 AM, Sriram Dash wrote:
>> Enables usb device-tree fixup code to incorporate xhci controller
>>
>> Signed-off-by: Ramneek Mehresh <ramneek.mehresh at nxp.com>
>> Signed-off-by: Sriram Dash <sriram.dash at nxp.com>
>
>Changelog ?
>

Will include changelog for v2 and v3 in v4.

>> ---
>>  board/freescale/common/Makefile |  1 +
>>  board/freescale/common/usb.c    | 30 +++++++++++++-----------------
>>  include/fdt_support.h           |  4 ++--
>>  3 files changed, 16 insertions(+), 19 deletions(-)
>>
>> diff --git a/board/freescale/common/Makefile
>> b/board/freescale/common/Makefile index 62de45c..c644896 100644
>> --- a/board/freescale/common/Makefile
>> +++ b/board/freescale/common/Makefile
>> @@ -14,6 +14,7 @@ endif
>>  endif
>>
>>  obj-$(CONFIG_USB_EHCI_FSL) += usb.o
>> +obj-$(CONFIG_USB_XHCI_FSL) += usb.o
>>
>>  ifdef MINIMAL
>>  # necessary to create built-in.o
>> diff --git a/board/freescale/common/usb.c
>> b/board/freescale/common/usb.c index d815dc1..8e423be 100644
>> --- a/board/freescale/common/usb.c
>> +++ b/board/freescale/common/usb.c
>> @@ -18,29 +18,25 @@
>>  #define CONFIG_USB_MAX_CONTROLLER_COUNT 1  #endif
>>
>> +const char compat_usb_fsl[][16] = {"fsl-usb2-mph",
>> +				   "fsl-usb2-dr",
>> +				   "snps,dwc3"};
>
>This is const char *foo[].
>

Reference from "char compat[][16] = { "cfi-flash", "jedec-flash" };"
I will change to const char *compat_usb_fsl[] in v4. 

>>  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;
>> +	int size = sizeof(compat_usb_fsl)/sizeof(compat_usb_fsl[0]);
>
>Oh the art of counting. Firstly, what you did here is reimplementation of
>ARRAY_SIZE(), but that's wrong in this context. Each one of the array elements is
>differently sized, so to avoid problems with this crap, the code hard-codes random
>constant defining the element size, which is another crap workaround as it will
>break once a longer element is added.
>And it wastes space. No, instead, use a terminating entry in the array.
>

Will change compat_usb_fsl[][16] into const char *compat_usb_fsl[], 
and use ARRAY_SIZE(compat_usb_fsl) . What is your opinion?

>btw use checkpatch before your next submission.
>
>> +	int i;
>> +
>> +	for (i = 0; i < size ; i++) {
>> +		*node_offset = fdt_node_offset_by_compatible
>> +				(blob, start_offset, compat_usb_fsl[i]);
>> +		if (*node_offset >= 0) {
>> +			node_type = compat_usb_fsl[i];
>> +			break;
>>  		}
>> -	} else {
>> -		node_type = compat_mph;
>>  	}
>> -
>>  	return node_type;
>>  }
>>
>> diff --git a/include/fdt_support.h b/include/fdt_support.h index
>> 296add0..d34e959 100644
>> --- a/include/fdt_support.h
>> +++ b/include/fdt_support.h
>> @@ -113,11 +113,11 @@ void fdt_fixup_qe_firmware(void *fdt);
>>   */
>>  int fdt_fixup_display(void *blob, const char *path, const char
>> *display);
>>
>> -#if defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB)
>> +#if defined(CONFIG_USB_EHCI_FSL) || defined(CONFIG_USB_XHCI_FSL)
>>  void fdt_fixup_dr_usb(void *blob, bd_t *bd);  #else  static inline
>> void fdt_fixup_dr_usb(void *blob, bd_t *bd) {} -#endif /*
>> defined(CONFIG_HAS_FSL_DR_USB) || defined(CONFIG_HAS_FSL_MPH_USB) */
>> +#endif /* defined(CONFIG_USB_EHCI_FSL) ||
>> +defined(CONFIG_USB_XHCI_FSL) */
>>
>>  #if defined(CONFIG_SYS_FSL_SEC_COMPAT)
>>  void fdt_fixup_crypto_node(void *blob, int sec_rev);
>>
>
>
>--
>Best regards,
>Marek Vasut