Rafał Miłecki [off-list ref] :
[...]

I'm not 100% sure if I understand... Do you mean I should declare variables
inside "while" loop when possible ?

Yes. Please consider the deepest inner block.

While at it:

[...]

+static int bgmac_dma_rx_read(struct bgmac *bgmac, struct bgmac_dma_ring *ring,

[...]

+		/* Check for poison and drop or pass the packet */
+		if (len == 0xdead && flags == 0xbeef) {
+			bgmac_err(bgmac, "Found poisoned packet at slot %d, DMA issue!\n", ring->start);
+		} else {
+			new_skb = netdev_alloc_skb(bgmac->net_dev, len);
+			if (new_skb) {
+				skb_put(new_skb, len);
+				skb_copy_from_linear_data_offset(skb, BGMAC_RX_FRAME_OFFSET,
+								 new_skb->data, len);
+				new_skb->protocol =
+					eth_type_trans(new_skb, bgmac->net_dev);
+				netif_receive_skb(new_skb);
+				handled++;
+			} else {
+				bgmac_err(bgmac, "Allocation of skb for copying packet failed!\n");
+			}

You should increase bgmac->net_dev->stats.rx_dropped

[...]

+static int bgmac_dma_alloc(struct bgmac *bgmac)
+{
+	struct device *dma_dev = bgmac->core->dma_dev;
+	struct bgmac_dma_ring *ring;
+	u16 ring_base[] = { BGMAC_DMA_BASE0, BGMAC_DMA_BASE1, BGMAC_DMA_BASE2,
+			    BGMAC_DMA_BASE3, };

ring_base could be 'static const'.

+	int size; /* ring size: different for Tx and Rx */
+	int err;
+	int i;
+
+	BUILD_BUG_ON(BGMAC_MAX_TX_RINGS > ARRAY_SIZE(ring_base));
+	BUILD_BUG_ON(BGMAC_MAX_RX_RINGS > ARRAY_SIZE(ring_base));
+
+	if (!(bcma_aread32(bgmac->core, BCMA_IOST) & BCMA_IOST_DMA64)) {
+		bgmac_err(bgmac, "Core does not report 64-bit DMA\n");
+		return -ENOTSUPP;
+	}
+
+	for (i = 0; i < BGMAC_MAX_TX_RINGS; i++) {
+		ring = &bgmac->tx_ring[i];
+		ring->tx = true;
+		ring->num_slots = BGMAC_TX_RING_SLOTS;
+		ring->mmio_base = ring_base[i];
+		if (bgmac_dma_unaligned(bgmac, ring))
+			bgmac_warn(bgmac, "TX on ring 0x%X supports unaligned addressing but this feature is not implemented\n", ring->mmio_base);

ring->tx is used once in bgmac_dma_unaligned.

Add bgmac_dma_unaligned an extra parameter for the BGMAC_DMA_[RT]X_RINGLO
offset and ring->[rt]x can go away.

+
+		/* Alloc ring of descriptors */
+		size = ring->num_slots * sizeof(struct bgmac_dma_desc);
+		ring->cpu_base = dma_zalloc_coherent(dma_dev, size,
+						     &(ring->dma_base),

Plese remove the useless parenthesis.

[...]

+static void bgmac_chip_stats_update(struct bgmac *bgmac)
+{
+	int i;
+
+	if (bgmac->core->id.id != BCMA_CORE_4706_MAC_GBIT) {
+		for (i = 0; i < BGMAC_NUM_MIB_TX_REGS; i++)
+			bgmac->mib_tx_regs[i] =
+				bgmac_read(bgmac,
+					   BGMAC_TX_GOOD_OCTETS + (i * 4));
+		for (i = 0; i < BGMAC_NUM_MIB_RX_REGS; i++)
+			bgmac->mib_rx_regs[i] =
+				bgmac_read(bgmac,
+					   BGMAC_RX_GOOD_OCTETS + (i * 4));
+	}

Neither of mib_[rt]x_regs is ever read.

[...]

+static void bgmac_chip_reset(struct bgmac *bgmac)
+{

[...]

+	if (ci->id == BCMA_CHIP_ID_BCM5357 || ci->id == BCMA_CHIP_ID_BCM4749 ||
+	    ci->id == BCMA_CHIP_ID_BCM53572) {
+		struct bcma_drv_cc *cc = &bgmac->core->bus->drv_cc;
+		u8 et_swtype = 0;
+		u8 sw_type = BGMAC_CHIPCTL_1_SW_TYPE_EPHY |
+			     BGMAC_CHIPCTL_1_IF_TYPE_RMII;
+		char buf[2];
+		if (nvram_getenv("et_swtype", buf, 1) > 0) {

Please keep an empty line between variables declaration and body.

[...]

+static irqreturn_t bgmac_interrupt(int irq, void *dev_id)
+{
+	struct bgmac *bgmac = netdev_priv(dev_id);
+
+	u32 int_status = bgmac_read(bgmac, BGMAC_INT_STATUS);
+	int_status &= bgmac->int_mask;

...


[...]

+/**************************************************
+ * net_device_ops
+ **************************************************/
+
+static int bgmac_open(struct net_device *net_dev)
+{
+	struct bgmac *bgmac = netdev_priv(net_dev);
+
+	/* Reset */
+	bgmac_chip_reset(bgmac);

Please remove the useless comment(s).

+	/* Specs say about reclaiming rings here, but we do that in DMA init */
+	bgmac_chip_init(bgmac, true);
+
+	/* Enable IRQs */
+	if (request_irq(bgmac->core->irq, bgmac_interrupt, IRQF_SHARED,
+			KBUILD_MODNAME, net_dev) < 0)
+		bgmac_err(bgmac, "IRQ request error!\n");
+	napi_enable(&bgmac->napi);
+
+	return 0;

bgmac_open should not succeed when request_irq fails. Please propagate its
error status code.

[...]

+static int bgmac_get_settings(struct net_device *net_dev,
+			      struct ethtool_cmd *cmd)
+{
+	struct bgmac *bgmac = netdev_priv(net_dev);
+
+	cmd->supported = SUPPORTED_10baseT_Half |
+			 SUPPORTED_10baseT_Full |
+			 SUPPORTED_100baseT_Half |
+			 SUPPORTED_100baseT_Full |
+			 SUPPORTED_1000baseT_Half |
+			 SUPPORTED_1000baseT_Full |
+			 SUPPORTED_Autoneg;
+
+	if (bgmac->autoneg) {
+		WARN_ON(cmd->advertising);
+		if (bgmac->full_duplex) {
+			if (bgmac->speed & BGMAC_SPEED_10)
+				cmd->advertising |= ADVERTISED_10baseT_Full;
+			if (bgmac->speed & BGMAC_SPEED_100)
+				cmd->advertising |= ADVERTISED_100baseT_Full;
+			if (bgmac->speed & BGMAC_SPEED_1000)
+				cmd->advertising |= ADVERTISED_1000baseT_Full;
+		} else {
+			if (bgmac->speed & BGMAC_SPEED_10)
+				cmd->advertising |= ADVERTISED_10baseT_Half;
+			if (bgmac->speed & BGMAC_SPEED_100)
+				cmd->advertising |= ADVERTISED_100baseT_Half;
+			if (bgmac->speed & BGMAC_SPEED_1000)
+				cmd->advertising |= ADVERTISED_1000baseT_Half;
+		}
+	} else {
+		switch (bgmac->speed) {
+		case BGMAC_SPEED_10:
+			ethtool_cmd_speed_set(cmd, SPEED_10);
+			break;
+		case BGMAC_SPEED_100:
+			ethtool_cmd_speed_set(cmd, SPEED_100);
+			break;
+		case BGMAC_SPEED_1000:
+			ethtool_cmd_speed_set(cmd, SPEED_1000);
+			break;
+		}
+	}

It should not take long for users to complain that the driver does not
report the current link speed. A struct mii_if_info will ease your life.

Btw the driver does not manage the link (netif_carrier_{on/off}).

[...]

[...]

+struct bgmac_dma_ring {
+	bool tx;
+	u16 num_slots;
+	u16 start;
+	u16 end;
+
+	u16 mmio_base;
+	void *cpu_base;

	struct bgmac_dma_desc *cpu_base; ?


-- 
Ueimor