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

[Alexey Brodkin <Alexey.Brodkin@synopsys.com>]

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