Re: [PATCH v2 1/2] wireless: Driver for 60GHz card wil6210

[Vladimir Kondratiev <qca_vkondrat@qca.qualcomm.com>]

On Tuesday, October 30, 2012 06:23:37 PM Luis R. Rodriguez wrote:
> On Mon, Oct 29, 2012 at 01:58:25PM +0200, Vladimir Kondratiev wrote:
> > Card wil6210 by Wilocity supports operation on the 60GHz band
> > 
> > See
> > http://wireless.kernel.org/en/users/Drivers/wil6210
> 
> Can you update that page to reflect the latest information?
> Its out of date now.

Sure I will. Wanted to do it after driver merge, to reflect driver status as 
well.

> 
> > diff --git a/drivers/net/wireless/ath/wil6210/pcie_bus.c
> > b/drivers/net/wireless/ath/wil6210/pcie_bus.c new file mode 100644
> > index 0000000..d2b14fb
> > --- /dev/null
> > +++ b/drivers/net/wireless/ath/wil6210/pcie_bus.c
> > +static int wil_pcie_probe(struct pci_dev *pdev, const struct
> > pci_device_id *id) +{
> > +	{ /* print various info */
> > +		struct net_device *ndev = wil_to_ndev(wil);
> > +		const char *pdev_name = pci_name(pdev);
> > +		const char *wiphydev_name = dev_name(wil_to_dev(wil));
> > +		const char *ndev_name = netdev_name(ndev);
> > +		const char *ifc_name = ndev->name;
> > +		struct pci_driver *drv = pci_dev_driver(pdev);
> > +		const char *drv_name = drv ? drv->name : "(no drv)";
> > +		pr_info("Driver  : <%s>\n", drv_name ?: "(null)");
> > +		pr_info("PCI dev : <%s>\n", pdev_name ?: "(null)");
> > +		pr_info("Net dev : <%s>\n", ndev_name ?: "(null)");
> > +		pr_info("Net ifc : <%s>\n", ifc_name ?: "(null)");
> > +		pr_info("Wiphy   : <%s>\n", wiphydev_name ?: "(null)");
> > +
> > +	}
> 
> Die nasty code, die! Bleed!

Well, agree. Removed.

> 
> > diff --git a/drivers/net/wireless/ath/wil6210/txrx.c
> > b/drivers/net/wireless/ath/wil6210/txrx.c new file mode 100644
> > index 0000000..463c68e
> > --- /dev/null
> > +++ b/drivers/net/wireless/ath/wil6210/txrx.c
> > +void tx_complete(struct wil6210_priv *wil, int ringid)
> > +{
> 
> ...
> 
> > +	{
> > +		u32 swhead = vring->swhead;
> > +		u32 swtail = vring->swtail;
> > +		int used = (vring->size + swhead - swtail) % vring->size;
> > +		int avail = vring->size - used - 1;
> > +		if (avail > vring->size/4)
> > +			netif_tx_wake_all_queues(wil_to_ndev(wil));
> > +	}
> > +}
> 
> Die!

Done

> 
> > diff --git a/drivers/net/wireless/ath/wil6210/wil6210.h
> > b/drivers/net/wireless/ath/wil6210/wil6210.h new file mode 100644
> > index 0000000..529b790
> > --- /dev/null
> > +++ b/drivers/net/wireless/ath/wil6210/wil6210.h
> > @@ -0,0 +1,299 @@
> 
> ...
> 
> > +#define WIL6210_DRV_VERSION "0.4" /* Driver version */
> 
> Again, death to this. I don't care what other drivers do, this
> is silly practice.

Well, if you insist... done.
By the way, Atheros driver carl9170 uses MODULE_VERSION, should we consider 
killing it?

> 
> You also still use generic routine names, please clean those
> up. Few examples: tx_complete() rx_handle(), iftype_nl2wmi(),
> and I'm sure there are plenty others, please prefix
> these for the driver.

Prefix is cheap, done for all driver's functions.

> 
> > diff --git a/drivers/net/wireless/ath/wil6210/wil6210_rgf.h
> > b/drivers/net/wireless/ath/wil6210/wil6210_rgf.h
> Any reason why this small file is required? Why not just stuff it into
> one of the other header files?

Initially, it was auto-generated by the hardware compiler; but now I can merge 
it into other include. Done.

> 
> You didn't also address my comments regarding WIL6210_ISR_COR #ifdef code.

I added string with short explanation in Kconfig.
Longer explanation: while clear-on-read is good for production mode, it makes 
debugging much harder - reading ISR registers clears interrupt, and one can no 
more monitor ISR with debugfs. So, when debugging ISR flows - and they still 
need some debugging - one have to use W1C mode. That's why it is still 
present.

There are exactly 2 #ifdefs: one in ISR acknowledge routine, and other one in 
ISR configuration routine.

Converting this #ifdef to run-time switch (say, module parameter) is possible, 
but I am not sure it would be better - changing ISR mode will cause reset; ans 
code will not more readable.

> 
> The second patch no longer applies as a new driver was merged.

Fixed this too

> 
> Apart from all this, looks good.
> 
>   Luis

Sending updated version in a minute.

Thanks, Vladimir.