Re: [PATCH v6] ethernet/arc/arc_emac - Add new driver

[Andy Shevchenko]

On Thu, Jun 20, 2013 at 2:53 PM, Alexey Brodkin
<Alexey.Brodkin@xxxxxxxxxxxx> wrote:
> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> ARCAngel4/ML50x.

Few nitpicks below.

> please take a look at 6th re-spin of the patch.

Usually it's better to start new thread for new version of the patchset.

> +++ b/drivers/net/ethernet/arc/emac_main.c

> +static void arc_emac_tx_clean(struct net_device *ndev)
> +{
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &priv->stats;
> +       unsigned int i;
> +
> +       for (i = 0; i < TX_BD_NUM; i++) {
> +               unsigned int *txbd_dirty = &priv->txbd_dirty;
> +               struct arc_emac_bd *txbd = &priv->txbd[*txbd_dirty];
> +               struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty];
> +               struct sk_buff *skb = tx_buff->skb;
> +               unsigned int info = le32_to_cpu(txbd->info);
> +
> +               *txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM;
> +
> +               if ((info & FOR_EMAC) || !txbd->data)

Redundant internal braces. Up to you.

> +static int arc_emac_rx(struct net_device *ndev, int budget)
> +{
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +       unsigned int work_done;
> +
> +       for (work_done = 0; work_done <= budget; work_done++) {

> +               /* Make a note that we saw a packet at this BD.
> +                * So next time, driver starts from this + 1
> +                */
> +               *last_rx_bd = (*last_rx_bd + 1) % RX_BD_NUM;
> +
> +#define FRST_OR_LAST_MASK      (FRST_MASK | LAST_MASK)

You may re-use it in *_tx_clean() above.
Also, please use full English word FIRST. Actually is there any
conflicts with existing drivers? Should it be with [ARC_]EMAC prefix?

> +               /* Make sure pointer to data buffer is set */
> +               mb();

I'm not so familiar with memory barriers, but is this somehow differ
from smp_mb()?
Which one suits better? And what about the other places in this file?

> +       /* Allocate Tx BD's similar to Rx BD's. All Tx BD's owned by CPU */
> +       bd = priv->txbd;
> +       for (i = 0; i < TX_BD_NUM; i++) {
> +               bd->data = 0;
> +               bd->info = 0;
> +               bd++;
> +       }

Looks like
memset(priv->txbd, 0, siezeof(*priv->txbd) * TX_BD_NUM);

> +       *info = cpu_to_le32(FOR_EMAC | FRST_MASK | LAST_MASK | len);

FOR_EMAC | FRST_OR_LAST_MASK | len ?

> +       /* Increment index to point to the next BD */
> +       *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;

+ Empty line?

> +       /* Get "info" of the next BD */

> +       /* Get PHY from device tree */
> +       phy_node = of_parse_phandle(pdev->dev.of_node, "phy", 0);

> +       /* Get EMAC registers base address from device tree */
> +       err = of_address_to_resource(pdev->dev.of_node, 0, &res_regs);

> +       /* Get CPU clock frequency from device tree */
> +       if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +                                &clock_frequency)) {

> +       /* Get IRQ from device tree */
> +       err = of_irq_to_resource(pdev->dev.of_node, 0, &res_irq);

So, you don't like the idea to get as much as possible before
alloc_netdev(), don't you?

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/