et131x: Moving the et131x driver out of staging? (a.k.a. filling up the TODO list again)

et131x: Moving the et131x driver out of staging? (a.k.a. filling up the TODO list again)

[Mark Einon]

Hello Greg,

I've managed to clear out the current TODO list for the staging/et131x
driver, and hopefully its looking in a much better shape because of
it.

I'd like to propose moving it out of the staging area, or at least
getting some review comments regarding what work is still needed.

Is it a good time to propose this bearing in mind the reorganisation
of drivers/net/ and the fallout from the kernel.org issues?

The only outstanding issue I know of, which is yet to hit your staging tree is:

On 1 October 2011 20:58, Dan Carpenter <dan.carpenter@oracle.com> wrote:
>... the driver doesn't
> compile any more.  The Kconfig file says it "depends on NETDEV_1000"
> but NETDEV_1000 isn't around any more since f860b0522f65d3
> "drivers/net: Kconfig and Makefile cleanup" was merged.

Cheers,

Mark

Re: et131x: Moving the et131x driver out of staging? (a.k.a. filling up the TODO list again)

[Francois Romieu]

Mark Einon <mark.einon@gmail.com> :
[...]
> I'd like to propose moving it out of the staging area, or at least
> getting some review comments regarding what work is still needed.

Is there a chance we could have a single file driver - may be with a .h for
the registers layout ? It should be less than 200kbytes, especially if
forward declarations go away.

rx_ring.fbr{0, 1} can probably share a common structure.

Use of kmem_cache seems a bit unusual.

Use dma_alloc_... in place of pci_alloc_...

It's too late stopping the tx queue when there is no room for the current
packet. The condition should be detected for the next packet.

PCI_VDEVICE would not hurt.

My 0.02 € (whatever the CDS spread).

-- 
Ueimor

Re: et131x: Moving the et131x driver out of staging? (a.k.a. filling up the TODO list again)

[Dan Carpenter]

There are some trivial static checker things that could be cleaned
up.

drivers/staging/et131x/et131x_initpci.c +489 et131x_adjust_link(45) error: we previously assumed 'phydev' could be null (see line 475)
drivers/staging/et131x/et131x_initpci.c +505 et131x_adjust_link(61) warn: variable dereferenced before check 'phydev' (see line 498)

(probably the null check should be removed?)

drivers/staging/et131x/et1310_mac.c:376:44: warning: incorrect type in initializer (different address spaces)
drivers/staging/et131x/et1310_mac.c:376:44:    expected struct txmac_regs *txmac
drivers/staging/et131x/et1310_mac.c:376:44:    got struct txmac_regs [noderef] <asn:2>*<noident>
drivers/staging/et131x/et1310_mac.c:383:28: warning: incorrect type in argument 2 (different address spaces)
drivers/staging/et131x/et1310_mac.c:383:28:    expected void volatile [noderef] <asn:2>*addr
drivers/staging/et131x/et1310_mac.c:383:28:    got unsigned int *<noident>
drivers/staging/et131x/et1310_mac.c:385:31: warning: incorrect type in argument 2 (different address spaces)
drivers/staging/et131x/et1310_mac.c:385:31:    expected void volatile [noderef] <asn:2>*addr
drivers/staging/et131x/et1310_mac.c:385:31:    got unsigned int *<noident>

(it just needs an __iomem anotation).

drivers/staging/et131x/et131x_initpci.c:556:5: warning: symbol 'et131x_mii_probe' was not declared. Should it be static?
drivers/staging/et131x/et131x_netdev.c:246:5: warning: symbol 'et131x_ioctl' was not declared. Should it be static?
drivers/staging/et131x/et131x_netdev.c:264:5: warning: symbol 'et131x_set_packet_filter' was not declared. Should it be static?
drivers/staging/et131x/et131x_netdev.c:326:6: warning: symbol 'et131x_multicast' was not declared. Should it be static?
drivers/staging/et131x/et131x_netdev.c:399:5: warning: symbol 'et131x_tx' was not declared. Should it be static?
drivers/staging/et131x/et131x_netdev.c:432:6: warning: symbol 'et131x_tx_timeout' was not declared. Should it be static?
drivers/staging/et131x/et131x_netdev.c:490:5: warning: symbol 'et131x_change_mtu' was not declared. Should it be static?
drivers/staging/et131x/et131x_netdev.c:543:5: warning: symbol 'et131x_set_mac_addr' was not declared. Should it be static?

regards,
dan carpenter

Re: et131x: Moving the et131x driver out of staging? (a.k.a. filling up the TODO list again)

[Greg KH]

On Fri, Oct 07, 2011 at 11:07:00PM +0100, Mark Einon wrote:
> Hello Greg,
> 
> I've managed to clear out the current TODO list for the staging/et131x
> driver, and hopefully its looking in a much better shape because of
> it.

It is, thanks for the work.

> I'd like to propose moving it out of the staging area, or at least
> getting some review comments regarding what work is still needed.

I think you got some comments and have a bit more work to do now, right?

> Is it a good time to propose this bearing in mind the reorganisation
> of drivers/net/ and the fallout from the kernel.org issues?

I don't think either of those have any bearing on this, does it?

thanks,

greg k-h