[U-Boot] [PATCH 2/3] dnet: driver for Dave DNET ethernet controller

[Wolfgang Denk]

Dear Ilya Yanok,

In message <1233589490-14293-3-git-send-email-yanok at emcraft.com> you wrote:
> Driver for Dave DNET ethernet controller (used on Dave/DENX
> QongEVB-LITE board).
...
> --- a/drivers/net/Makefile
> +++ b/drivers/net/Makefile
> @@ -69,6 +69,7 @@ COBJS-$(CONFIG_VSC7385_ENET) += vsc7385.o
>  COBJS-$(CONFIG_XILINX_EMAC) += xilinx_emac.o
>  COBJS-$(CONFIG_XILINX_EMACLITE) += xilinx_emaclite.o
>  COBJS-$(CONFIG_SH_ETHER) += sh_eth.o
> +COBJS-$(CONFIG_DRIVER_DNET) += dnet.o

Please omit the "DIVER_" part in the name.

> +struct dnet_device {
> +	void			*regs;
> +
> +	const struct device	*dev;
> +	struct eth_device	netdev;
> +	unsigned short		phy_addr;
> +};

Please no empty line in the struct declaration.

> +#define to_dnet(_nd) container_of(_nd, struct dnet_device, netdev)

Maybe a comment what this is good for?

> +static void dnet_mdio_write(struct dnet_device *dnet, u8 reg, u16 value)
> +{
> +	u16 tmp;
> +
> +	debug(DRIVERNAME "dnet_mdio_write %02x:%02x <- %04x\n",
> +			dnet->phy_addr, reg, value);
> +
> +	while (!(dnet_readw_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG) &
> +				DNET_INTERNAL_GMII_MNG_CMD_FIN));

Please move the semicolon to a separate line.

> +	while (!(dnet_readw_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG) &
> +				DNET_INTERNAL_GMII_MNG_CMD_FIN));

Ditto.

> +static u16 dnet_mdio_read(struct dnet_device *dnet, u8 reg)
> +{
> +	u16 value;
> +
> +	while (!(dnet_readw_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG) &
> +				DNET_INTERNAL_GMII_MNG_CMD_FIN));
> +
> +	/* only 5 bits allowed for register offset*/
> +	reg &= 0x1f;
> +
> +	/* prepare reg_value for a read */
> +	value = (dnet->phy_addr << 8);
> +	value |= reg;
> +
> +	/* write control word */
> +	dnet_writew_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG, value);
> +
> +	/* wait for end of transfer */
> +	while (!(dnet_readw_mac(dnet, DNET_INTERNAL_GMII_MNG_CTL_REG) &
> +				DNET_INTERNAL_GMII_MNG_CMD_FIN));

Ditto - and so on.

> +static int dnet_recv(struct eth_device *netdev)
> +{
> +	struct dnet_device *dnet = to_dnet(netdev);
> +	unsigned int * data_ptr;
> +	int pkt_len, poll, i;
> +	u32 cmd_word;
> +
> +	debug("Waiting for pkt (polling)\n");
> +	poll = 50;
> +	while ((dnet_readl(dnet, RX_FIFO_WCNT) >> 16) == 0) {
> +		udelay(10);  // wait 10 usec
----------------------------^^^
> +		--poll;
> +		if (poll == 0)
> +			return 0;	// no pkt available
---------------------------------------^^^

No C++ comments, please.

> +/* Register access macros */
> +#define dnet_writel(port, value, reg)	\
> +	writel((value), (port)->regs + DNET_##reg)

Please do not swap arguments.

> +#define dnet_readl(port, reg)		readl((port)->regs + DNET_##reg)

Hmm... Why do we need this?

> +/* ALL DNET FIFO REGISTERS */
> +#define DNET_RX_LEN_FIFO			(0x000)	/* RX_LEN_FIFO */
> +#define DNET_RX_DATA_FIFO			(0x004)	/* RX_DATA_FIFO */
> +#define DNET_TX_LEN_FIFO			(0x008)	/* TX_LEN_FIFO */
> +#define DNET_TX_DATA_FIFO			(0x00C)	/* TX_DATA_FIFO */
> +
> +/* ALL DNET CONTROL/STATUS REGISTERS OFFSETS */
> +#define DNET_VERCAPS				(0x100)	/* VERCAPS */
> +#define DNET_INTR_SRC				(0x104)	/* INTR_SRC */
> +#define DNET_INTR_ENB				(0x108)	/* INTR_ENB */
> +#define DNET_RX_STATUS				(0x10C)	/* RX_STATUS */
> +#define DNET_TX_STATUS				(0x110)	/* TX_STATUS */
> +#define DNET_RX_FRAMES_CNT			(0x114)	/* RX_FRAMES_CNT */
> +#define DNET_TX_FRAMES_CNT			(0x118)	/* TX_FRAMES_CNT */
> +#define DNET_RX_FIFO_TH				(0x11C)	/* RX_FIFO_TH */
> +#define DNET_TX_FIFO_TH				(0x120)	/* TX_FIFO_TH */
> +#define DNET_SYS_CTL				(0x124)	/* SYS_CTL */
> +#define DNET_PAUSE_TMR				(0x128)	/* PAUSE_TMR */
> +#define DNET_RX_FIFO_WCNT			(0x12C)	/* RX_FIFO_WCNT */
> +#define DNET_TX_FIFO_WCNT			(0x130)	/* TX_FIFO_WCNT */
...

I see. 

Please do not operate on base register plus magic offset - implement a
proper C structure instead to describe the controller hardware.

> +/*
> + * Capabilities. Used by the driver to know the capabilities that
> + * the ethernet controller inside the FPGA have.
> + */
> +
> +#define DNET_HAS_MDIO 		(1 << 0)
> +#define DNET_HAS_IRQ 		(1 << 1)
> +#define DNET_HAS_GIGABIT 	(1 << 2)
> +#define DNET_HAS_DMA 		(1 << 3)
> +
> +#define DNET_HAS_MII		(1 << 4) // or GMII
> +#define DNET_HAS_RMII		(1 << 5) // or RGMII

No C++ comments, please.

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
Killing is wrong.
	-- Losira, "That Which Survives", stardate unknown