>-----Original Message-----
>From: Marek Vasut [mailto:ma...@denx.de]
>Sent: Thursday, June 02, 2016 6:25 PM
>To: Sriram Dash <sriram.d...@nxp.com>; u-boot@lists.denx.de
>Cc: york sun <york....@nxp.com>; albert.u.b...@aribaud.net; Rajesh Bhagat
><rajesh.bha...@nxp.com>
>Subject: Re: [PATCH v2 2/5] usb: xhci: fsl: code cleanup for device tree fixup
>for fsl
>usb controllers
>
>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.bha...@nxp.com>
>> Signed-off-by: Sriram Dash <sriram.d...@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.
>
Thank you for the suggestion. Looks better in a single line. Will change in V3.
>
>> + *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.
>
Yes. You are correct. I will now return ENOSPC in case of failure and check
that in
fdt_fixup_dr_usb function to maintain the same logic as before.
>> + 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.
>
usb_erratum_aNNNNNN_off variable are keeping track of the device tree fixup for
errata NNNNNN. The code checks errata applicability for each controller and
tries to fix the device tree accordingly. During this time, the variable
usb_erratum_aNNNNNN_off is updated to the offset of device tree, if the device
tree is fixed. Now, for the second controller, when it tries to fix the device
tree, it checks from the same offset obtained. As the API for fdt_setprop is
such that the fixup will occur as soon as the API finds first match, if this
variable usb_erratum_aNNNNNN_off is not maintained, the errata will be fixed
always for the first usb controller it comes across the device tree. So, we
need this variable.
Now, we cannot locally deal with the variable, reason being, in function
fdt_fixup_erratum, To make it keep track of the offset, I have again generate
multiple variables for that many erratas.
Also, I have to make each variable static, in order to keep track of the
controller specific errata Applicability.
>> - 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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
