[U-Boot] [PATCH v4 1/5] Add support for SMSC95XX USB 2.0 10/100MBit Ethernet Adapter

[Wolfgang Denk]

Dear Simon Glass,

In message <1302748176-4324-1-git-send-email-sjg at chromium.org> you wrote:
> The SMSC95XX is a USB hub with a built-in Ethernet adapter. This adds support
> for this, using the USB host network framework.
> 
> TEST=usb start; bootp; tftp ...
> 
> Signed-off-by: Simon Glass <sjg at chromium.org>
...
> +#define TX_CMD_A_FIRST_SEG_		(0x00002000)
> +#define TX_CMD_A_LAST_SEG_		(0x00001000)
> +
> +/* Rx status word */
> +#define RX_STS_FL_			(0x3FFF0000)	/* Frame Length */
> +#define RX_STS_ES_			(0x00008000)	/* Error Summary */
...

Please drop the parens around all these constants.  Please fix
globally.

> +/* SCSRs */
> +#define ID_REV				(0x00)
> +
> +#define INT_STS				(0x08)
> +
> +#define TX_CFG				(0x10)
> +#define TX_CFG_ON_			(0x00000004)
> +
> +#define HW_CFG				(0x14)
> +#define HW_CFG_BIR_			(0x00001000)
> +#define HW_CFG_RXDOFF_			(0x00000600)
> +#define HW_CFG_MEF_			(0x00000020)
> +#define HW_CFG_BCE_			(0x00000002)
> +#define HW_CFG_LRST_			(0x00000008)
> +
> +#define PM_CTRL				(0x20)
> +#define PM_CTL_PHY_RST_			(0x00000010)
> +
> +#define AFC_CFG				(0x2C)
> +
> +/*
> + * Hi watermark = 15.5Kb (~10 mtu pkts)
> + * low watermark = 3k (~2 mtu pkts)
> + * backpressure duration = ~ 350us
> + * Apply FC on any frame.
> + */
> +#define AFC_CFG_DEFAULT			(0x00F830A1)
> +
> +#define E2P_CMD				(0x30)
> +#define E2P_CMD_BUSY_			(0x80000000)
> +#define E2P_CMD_READ_			(0x00000000)
> +#define E2P_CMD_TIMEOUT_		(0x00000400)
> +#define E2P_CMD_LOADED_			(0x00000200)
> +#define E2P_CMD_ADDR_			(0x000001FF)
> +
> +#define E2P_DATA			(0x34)
> +
> +#define BURST_CAP			(0x38)
> +
> +#define INT_EP_CTL			(0x68)
> +#define INT_EP_CTL_PHY_INT_		(0x00008000)
> +
> +#define BULK_IN_DLY			(0x6C)
> +
> +/* MAC CSRs */
> +#define MAC_CR				(0x100)
> +#define MAC_CR_MCPAS_			(0x00080000)
> +#define MAC_CR_PRMS_			(0x00040000)
> +#define MAC_CR_HPFILT_			(0x00002000)
> +#define MAC_CR_TXEN_			(0x00000008)
> +#define MAC_CR_RXEN_			(0x00000004)
> +
> +#define ADDRH				(0x104)
> +
> +#define ADDRL				(0x108)
> +
> +#define MII_ADDR			(0x114)
> +#define MII_WRITE_			(0x02)
> +#define MII_BUSY_			(0x01)
> +#define MII_READ_			(0x00) /* ~of MII Write bit */
> +
> +#define MII_DATA			(0x118)
> +
> +#define FLOW				(0x11C)
> +
> +#define VLAN1				(0x120)
> +
> +#define COE_CR				(0x130)
> +#define Tx_COE_EN_			(0x00010000)
> +#define Rx_COE_EN_			(0x00000001)
> +
> +/* Vendor-specific PHY Definitions */
> +#define PHY_INT_SRC			(29)
> +
> +#define PHY_INT_MASK			(30)
> +#define PHY_INT_MASK_ANEG_COMP_		((u16)0x0040)
> +#define PHY_INT_MASK_LINK_DOWN_		((u16)0x0010)
> +#define PHY_INT_MASK_DEFAULT_		(PHY_INT_MASK_ANEG_COMP_ | \
> +					 PHY_INT_MASK_LINK_DOWN_)
> +
> +/* USB Vendor Requests */
> +#define USB_VENDOR_REQUEST_WRITE_REGISTER	0xA0
...

> +	ret = smsc95xx_read_reg(dev, HW_CFG, &read_buf);
> +	if (ret < 0) {
> +		debug("Failed to read HW_CFG: %d\n", ret);
> +		return ret;
> +	}
> +	debug("Read Value from HW_CFG : 0x%08x\n", read_buf);
> +
> +	read_buf |= HW_CFG_BIR_;
> +	ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
> +	if (ret < 0) {
> +		debug("Failed to write HW_CFG_BIR_ bit in HW_CFG "
> +			"register, ret = %d\n", ret);
> +		return ret;
> +	}

You repeat these sequences of smsc95xx_read_reg() resp.
smsc95xx_write_reg() followed by the "if (ret < 0)" part about 20
times.

Please move the respective debug() message for the failure case into
the functions, and consider using a loop here instead.

> +	read_buf = DEFAULT_BULK_IN_DELAY;
> +	ret = smsc95xx_write_reg(dev, BULK_IN_DLY, read_buf);
> +	if (ret < 0) {
> +		debug("ret = %d", ret);
> +		return ret;
> +	}

Is it intentional to use the read_buf with a smsc95xx_write_reg()
call?

> +	ret = smsc95xx_write_reg(dev, HW_CFG, read_buf);
> +	if (ret < 0) {
> +		debug("Failed to write HW_CFG register, ret=%d\n", ret);
> +		return ret;
> +	}

Ditto here?

> +	ret = smsc95xx_write_reg(dev, AFC_CFG, read_buf);
> +	if (ret < 0) {
> +		debug("Failed to write AFC_CFG: %d\n", ret);
> +		return ret;
> +	}

And here?

> +	ret = smsc95xx_read_reg(dev, INT_EP_CTL, &read_buf);
> +	if (ret < 0) {
> +		debug("Failed to read INT_EP_CTL: %d", ret);
> +		return ret;
> +	}

And here we have &read_buf while everywhere else we have read_buf
only?



Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"I like your game but we have to change the rules."