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

[Marek Vasut]

On 03/03/2016 09:32 AM, Sriram Dash wrote:
>> -----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?

My opinion is to use a terminating NULL entry and iterate over the array
until you reach it.

-- 
Best regards,
Marek Vasut