[U-Boot] [PATCH 2/5] sunxi: add USB EHCI driver

Ian Campbell
2014-07-09T14:54:18Z

On Tue, 2014-07-08 at 22:21 +0200, Roman Byshko wrote:
> diff --git a/drivers/usb/host/ehci-sunxi.c b/drivers/usb/host/ehci-sunxi.c
> new file mode 100644
> index 0000000..5817fc7
> --- /dev/null
> +++ b/drivers/usb/host/ehci-sunxi.c
> @@ -0,0 +1,236 @@
> +/*
> + * Copyright (C) 2014 arokux
> + *
> + * arokux <arokux@gmail.com>

The authorship info here contradicts the authorship suggested by the
commit email metadata and the S-o-b.

If arokux wrote this patch then the commit message should begin with

From: Arokux X <arokux@gmail.com>

on the first line and arokux needs to have Signed-off on the patch.

I'm not sure what u-boot policy says about pseudonyms.

> + *
> + * Based on code from
> + * Allwinner Technology Co., Ltd. <www.allwinnertech.com>
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.

Please use the SPDX tag instead of the full license.

> + dat = readl(&pio->dat);
> + if (val)
> + dat |= 0x1 << num;
> + else
> + dat &= ~(0x1 << num);
> +
> + writel(dat, &pio->dat);

This is
if (val)
setbits_le32(&pio->dat, 1<<num);
else
clrbits_le32(&pio->dat, 1<<num);

It seems like there are a bunch of similar constructs around the place.

> + return 0;
> +}
> +
> +static void usb_phy_write(struct sunxi_ehci_hcd *sunxi_ehci, int addr,
> + int data, int len)
> +{
> + int temp = 0, j = 0, usbc_bit = 0;
> + void *dest = sunxi_ehci->csr;
> +
> + usbc_bit = BIT(sunxi_ehci->id * 2);
> + for (j = 0; j < len; j++) {
> + /* set the bit address to be written */
> + temp = readl(dest);
> + temp &= ~(0xff << 8);
> + temp |= ((addr + j) << 8);
> + writel(temp, dest);

clrsetbits?

> +
> + clrbits_le32(dest, usbc_bit);
> + /* set data bit */
> + if (data & 0x1)
> + temp |= BIT(7);
> + else
> + temp &= ~BIT(7);
> + writeb(temp, dest);

AFAICT this will clobber the clearing of usbc_bit which you just did,
since temp was read before that, is that right?

This should probably use the clr/setbits_le32 stuff and if it really is
deliberately clobbering usbc_bit I think it needs a comment.

> +
> + setbits_le32(dest, usbc_bit);
> +
> + clrbits_le32( dest, usbc_bit);

Extra space in this line.

> + /* gpio_direction_output(sunxi_ehci->gpio_vbus, 1); */

???

> +static void sunxi_ehci_disable(struct sunxi_ehci_hcd *sunxi_ehci)
> +{
> + struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
> +
> + /* gpio_direction_output(sunxi_ehci->gpio_vbus, 0); */

???

Ian.