Dear Ilya Yanok,
In message 1233589490-14293-3-git-send-email-yanok@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