=20
quoted text
> +config KORINA
> +	tristate "Korina (IDT rc32434) Ethernet support"
> +	depends on NET_ETHERNET && MIKROTIK_RB500
> +	help
> +	  If you have a Mikrotik RouterBoard 500 or IDT RC32434
> +	  based system say Y. Otherwise say N.

Since you'll never going to see that prompt if you don't have such a
board, that seems pretty unhelpful :)

quoted text
> +#define TX_TIMEOUT 	(HZ * 100)

Not sure where this is used, but 100 seconds seems quite long, no?

quoted text
> +enum status { filled, empty };

I'd use names with some prefixes, maybe descriptor_filled? Matter of
taste though I guess.

quoted text
> +static inline void korina_start_dma(struct dma_reg *ch, u32 dma_addr)

quoted text
> +static inline void korina_abort_dma(struct net_device *dev,

quoted text
> +static inline void korina_chain_dma(struct dma_reg *ch, u32 dma_addr)

quoted text
> +static inline void korina_abort_tx(struct net_device *dev)
> +{
> +	struct korina_private *lp =3D netdev_priv(dev);
> +
> +	korina_abort_dma(dev, lp->tx_dma_regs);
> +}

All those inlines seems a bit excessive since korina_abort_dma is an
inline too and then inlined into two other functions... Are those really
necessary?

quoted text
> +static inline void korina_start_tx(struct korina_private *lp,
> +					struct dma_desc *td)
> +{
> +	korina_start_dma(lp->tx_dma_regs, CPHYSADDR(td));

Similarly here although korina_start_dma is a lot shorter than stop_dma.

quoted text
> +	/* stop queue when full, drop pkts if queue already full */
> +	if (lp->tx_count >=3D (KORINA_NUM_TDS - 2)) {

quoted text
> +		if (lp->tx_count =3D=3D (KORINA_NUM_TDS - 2))
> +			netif_stop_queue(dev);

quoted text
> +	if (readl(&(lp->tx_dma_regs->dmandptr)) =3D=3D 0) {

Why so many parentheses? :)

quoted text
> +		/* Mask D H E bit in Rx DMA */
> +		dmasm =3D readl(&lp->rx_dma_regs->dmasm);
> +		writel(dmasm | (DMA_STAT_DONE |
> +				DMA_STAT_HALT | DMA_STAT_ERR),

Pretty useless comment especially since you have to read the code to
understand it! Or was this intended as a comment on how the register
write works? Similar instances all over the driver. Maybe a single
comment at the top of the file on how that register works would be good
instead.

quoted text
> +static void korina_multicast_list(struct net_device *dev)
> +{
> +	/* listen to broadcasts always and to treat 	*/
> +	/*       IFF bits independantly	*/

typo: independently

quoted text
> +	struct korina_private *lp =3D netdev_priv(dev);
> +	unsigned long flags;
> +	u32 recognise =3D ETH_ARC_AB;	/* always accept broadcasts */
> +
> +	/* Set promiscuouts mode */

Typo: promiscuous.

quoted text
> +	if ((dev->flags & IFF_ALLMULTI) || (dev->mc_count > 15))
> +		/* All multicast and broadcast */
> +		recognise |=3D ETH_ARC_AM;
> +	else if (dev->mc_count > 0)
> +		recognise |=3D ETH_ARC_AM;

Some stuff missing here? the mc_count > 15 checks seems superfluous
since >15 is always >0.

quoted text
> +static struct net_device_stats *korina_get_stats(struct net_device *dev)
> +{
> +	return &dev->stats;
> +}

Unnecessary, internal_stats is default and does exactly this.

quoted text
> +/*
> + *  * Restart the RC32434 ethernet controller.
> + *   */

Oddly formatted comment :)

quoted text
> +static int korina_close(struct net_device *dev)
> +{
> +	struct korina_private *lp =3D netdev_priv(dev);
> +	u32 tmp;
> +
> +	/* Disable interrupts */
> +	disable_irq(lp->rx_irq);
> +	disable_irq(lp->tx_irq);
> +	disable_irq(lp->ovr_irq);
> +	disable_irq(lp->und_irq);
> +
> +	tmp =3D readl(&lp->tx_dma_regs->dmasm);
> +	tmp =3D tmp | DMA_STAT_FINI | DMA_STAT_ERR;
> +	writel(tmp, &lp->tx_dma_regs->dmasm);
> +
> +	tmp =3D readl(&lp->rx_dma_regs->dmasm);
> +	tmp =3D tmp | DMA_STAT_DONE | DMA_STAT_HALT | DMA_STAT_ERR;
> +	writel(tmp, &lp->rx_dma_regs->dmasm);
> +
> +	free_irq(lp->rx_irq, dev);
> +	free_irq(lp->tx_irq, dev);
> +	free_irq(lp->ovr_irq, dev);
> +	free_irq(lp->und_irq, dev);
> +	return 0;

No need to abort DMA?

johannes