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

[Ian Campbell]

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 at 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 at 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.