Subject: Re: [PATCH 16/16] Supporting spidernet on Celleb

From: Arnd Bergmann <[email-masked]>
Date: Thu, 16 Nov 2006 11:15:48 +0100

On Wednesday 15 November 2006 11:02, Ishizaki Kou wrote:
> This patch adds auto-negotiation.
> Downloading firmware operation moves into spider_net_open().
> This patch is not tested on Cell-Blade.

This patch needs to be submitted to [email-masked] instead
of linuxppc-dev. Please also Cc: the maintainer of the driver,
James K Lewis <[email-masked]>, he at least needs to test
the changes.

The patch looks good overall, I have only a small cosmetic comments:

> @@ -1837,7 +1994,7 @@
>         }
>  
>  try_host_fw:
> -       dn = pci_device_to_OF_node(card->pdev);
> +       dn = (struct device_node *)card->pdev->sysdata;
>         if (!dn)
>                 goto out_err;
>  

> @@ -2071,7 +2234,7 @@
>  
>         netdev->irq = card->pdev->irq;
>  
> -       dn = pci_device_to_OF_node(card->pdev);
> +       dn = (struct device_node *)card->pdev->sysdata;
>         if (!dn)
>                 return -EIO;
>  

This should not be required. If you PCI bus is set up
correctly, pci_device_to_OF_node() will do the right thing.
What problem are you trying to solve here?

> +static void
> +spider_net_link_phy(struct spider_net_card *card)

> @@ -2058,6 +2215,12 @@
>         card->tx_timer.data = (unsigned long) card;
>         netdev->irq = card->pdev->irq;
>  
> +       card->aneg_count = 0;
> +       init_timer(&card->aneg_timer);
> +       card->aneg_timer.function =
> +               (void (*)(unsigned long)) spider_net_link_phy;
> +       card->aneg_timer.data = (unsigned long) card;
> +
>         card->options.rx_csum = SPIDER_NET_RX_CSUM_DEFAULT;
>  
>         card->num_tx_desc = tx_descriptors;

You should not need a cast here. It's better to cast the data value
in the function itself than casting the function pointer. This makes
sure we notice when the prototype of the function pointer changes.

	Arnd <><