[U-Boot] [PATCH 1/2] usb: musb-new: Add DA8xx MUSB Glue and enable Host mode

[Marek Vasut]

On 7/2/19 11:54 PM, Adam Ford wrote:
> On Tue, Jul 2, 2019 at 4:41 PM Marek Vasut <marex at denx.de> wrote:
>>
>> On 7/2/19 11:30 PM, Adam Ford wrote:
>>
>> [...]
>>
>>> @@ -0,0 +-2,466 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Texas Instruments da8xx "glue layer"
>>> + *
>>> + * Copyright (c) 2010, by Texas Instruments
>>> + *
>>> + * Based on the DA8xx "glue layer" code.
>>> + * Copyright (c) 2008-2009, MontaVista Software, Inc. <source at mvista.com>
>>> + *
>>> + * This file is part of the Inventra Controller Driver for Linux.
>>
>> Shouldn't you grow copyright on this too ?
> 
> Sure.  I borrowed much of this code from the Linux kernel, so I'll
> reference that too.
>>
>> [...]
>>
>>> +                     musb->int_usb &= ~MUSB_INTR_VBUSERROR;
>>> +                     //musb->xceiv->otg->state = OTG_STATE_A_WAIT_VFALL;
>>> +                     //mod_timer(&musb->dev_timer, jiffies + POLL_SECONDS * HZ);
>>
>> Why is this commented out in such a crude c++ way ? Surely , checkpatch
>> complains about it ?
> 
> I was part of the Linux kernel code, and I didn't want to lose it.
> I'll put an #ifdef __UBOOT__ around it like other drivers do.

This will become just a disgusting ifdef mess, just drop it.

[...]

>>> +/*
>>> + * Disable the usb phy
>>> + */
>>> +static void phy_off(void)
>>> +{
>>> +     u32 cfgchip2;
>>
>> Implement proper PHY driver.
> 
> I don't have an example to use, and other OMAP drivers do it this way.

See drivers/phy/

[...]

>>> +static int da8xx_musb_init(struct musb *musb)
>>> +{
>>> +     u32  revision;
>>> +     void __iomem *reg_base = musb->ctrl_base;
>>> +
>>> +     /* enable psc for usb2.0 */
>>> +     lpsc_on(33);
>>> +
>>> +     /* enable usb vbus */
>>> +     enable_vbus();
>>> +
>>> +     /* reset the controller */
>>> +     writel(0x1, &da8xx_usb_regs->control);
>>> +     udelay(5000);
>>> +
>>> +     /* start the on-chip usb phy and its pll */
>>> +     if (phy_on() == 0) {
>>> +             return -1;
>>
>> Use errno.h error codes
> 
> This was how the older, deleted driver did it, so I thought if it
> worked well enough before, i would work again.  I can update it to use
> erro codes.

That's called stagnation. I prefer continuous improvement.

[...]

>>> +     platdata->plat.mode = fdtdec_get_int(fdt, node,
>>> +                                          "mode", -1);
>>> +     if (platdata->plat.mode < 0) {
>>> +             pr_err("MUSB mode DT entry missing\n");
>>> +             return -ENOENT;
>>> +     }
>>> +#else /* MUSB_OTG, it doesn't work */
>>
>> Why are you submitting stuff that does not work ?
> 
> Because the code nearly works, and I am not sure when I can get back
> to it again.  The host portion does, and currently we have nothing.
> Having these comments in there leave breadcrumbs for myself or someone
> else to try and get it working.  Partially working seems better than
> not working at all.

I don't want any "almost working" code in mainline, fix it or drop it.

[...]