[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: Thursday, March 03, 2016 3:33 PM
>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/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.
>

Accepted. Will do it in v4.

>--
>Best regards,
>Marek Vasut