Messages in this thread | | | From | Alexey Brodkin <> | Subject | Re: [PATCH v6] ethernet/arc/arc_emac - Add new driver | Date | Fri, 21 Jun 2013 07:05:44 +0000 |
| |
On 06/20/2013 08:00 PM, Andy Shevchenko wrote: > On Thu, Jun 20, 2013 at 2:53 PM, Alexey Brodkin >> + if ((info & FOR_EMAC) || !txbd->data) > > Redundant internal braces. Up to you.
I left them so priorities of operations will be obvious.
>> +#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?
Re-used it in 2 mentioned places. And changed FRST with FIRST. I don't see defines with the same name anywhere else.
> >> + /* 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?
Since for now driver itself is not SMP-safe so I don't see any reason for "smp_" barriers. Later I plan to add SMP compatibility and that this point barriers will be replaced as well.
Another thing is I switched to "wmb()" (which is a barrier fror write in particular) instead of generic "mb()".
>> + /* 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);
Correct.
>> + /* 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?
Ok, I did this change)))
Regards, Alexey
| |