On 7/2/19 11:54 PM, Adam Ford wrote:
> On Tue, Jul 2, 2019 at 4:41 PM Marek Vasut <ma...@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. <sou...@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.
[...]
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
