Date:	Tue, 22 Dec 2009 11:46:39 +0000
From:	"Hennerich, Michael" <Michael.Hennerich@...log.com>
To:	"Ben Hutchings" <bhutchings@...arflare.com>,
	"Mike Frysinger" <vapier@...too.org>
CC:	<netdev@...r.kernel.org>, "David S. Miller" <davem@...emloft.net>,
	<uclinux-dist-devel@...ckfin.uclinux.org>
Subject: RE: [PATCH] wireless: adf702x: new driver for ADF7020/21 parts

Hi Ben,

Thanks for your feedback.
See comments below.

>From: Ben Hutchings
>On Mon, 2009-12-21 at 21:12 -0500, Mike Frysinger wrote:
>> From: Michael Hennerich <michael.hennerich@...log.com>
>>
>> This is a driver for Analog Devices series of ADF702x Narrow-Band
>> Short-Range Radio Transceiver chipsets, including the ADF7021 and
>> the ADF7025.  This Ethernet like driver implements a custom
>> software PHY.
>[...]
>> diff --git a/drivers/net/wireless/Kconfig
>b/drivers/net/wireless/Kconfig
>> index 56dd665..57974ac 100644
>> --- a/drivers/net/wireless/Kconfig
>> +++ b/drivers/net/wireless/Kconfig
>> @@ -44,6 +44,17 @@ config LIBERTAS_THINFIRM_USB
>>  	---help---
>>  	  A driver for Marvell Libertas 8388 USB devices using thinfirm.
>>
>> +config ADF702X
>> +	tristate "ADF702x Narrow-Band Short-Range Radio Transceiver"
>> +	depends on EXPERIMENTAL && SPI && BLACKFIN
>
>Any particular reason this should be EXPERIMENTAL?

No

>
>[...]
>> diff --git a/drivers/net/wireless/adf702x.c
>b/drivers/net/wireless/adf702x.c
>> new file mode 100644
>> index 0000000..ada0cb9
>> --- /dev/null
>> +++ b/drivers/net/wireless/adf702x.c
>[...]
>> +struct adf702x_priv {
>> +	struct spi_device *spi;
>> +	struct net_device *ndev;
>> +	struct sk_buff *tx_skb;
>> +	struct delayed_work tx_work;
>> +	struct work_struct tx_done_work;
>> +	wait_queue_head_t waitq;
>> +	dma_addr_t dma_handle;
>> +	spinlock_t lock;
>> +	unsigned rx_preample:1;
>
>Shouldn't this be named "rx_preamble" ("b" not "p")?

Right

>
>> +	unsigned rx:1;
>> +	unsigned rx_size;
>> +	u32 *rx_buf;
>> +	u32 *tx_buf;
>> +
>> +	/* Base reg base of SPORT controller */
>> +	volatile struct sport_register *sport;
>
>MMIO pointers should normally be declared like:
>
>	struct sport_register __iomem *sport;
>
>and used with readl, writel etc.

Well SPORT is a SoC peripheral it's mapped into Blackfin System MMR space.
Our versions of readX writeX are implemented for off-chip peripherals.
If you take a look at Blackfin io.h you will notice that we disable global interrupts, 
to avoid killed and reissued reads. This is not necessary for the System MMR space.   

Preferably I like to keep this as is, but I could also change it to use the 
Blackfin MMR accessor functions bfin_read{32,16} bfin_write{32,16}  

>
>[...]
>> +#define MAGIC		(0xA54662DA)
>
>You could choose a more meaningful name for this!

sure

>
>[...]
>> +/*
>> + * ONES: A instruction only DSPs feature
>> + * a XOR b -> return counted ONES (BIT Errors)
>> + */
>> +static inline unsigned short adf702x_xor_ones(unsigned int a,
>unsigned int b)
>> +{
>> +	unsigned short val;
>> +
>> +	__asm__ __volatile__ (
>> +				"%0 = %1 ^ %2;"
>> +				"%0.l = ONES %0;"
>> +				: "=d"(val) : "d"(a), "d"(b)
>> +				);
>> +	return val;
>> +}
>
>This could be written as hweight16(a ^ b).  I don't think the hweight
>functions have optimised implementations for Blackfin, but you can fix
>that.

This is what I was looking for - I wasn't aware of hweight{64,32,16,8}
Thanks!

>
>[...]
>> +/*
>> + * Get Packet size from header
>> + * Returns: size or 42 in case of an unrecoverable error
>> + */
>> +inline unsigned short adf702x_getrxsize(struct adf702x_priv *lp, int
>offset)
>
>Should be declared static inline, not just inline.

ok

>
>> +{
>> +	int size = adf702x_getsymbol(lp->rx_buf[offset + 1]) << 8 |
>> +			adf702x_getsymbol(lp->rx_buf[offset + 2]);
>> +
>> +	if (size > 0)
>> +		return size;
>> +
>> +	DBG(1, "%s :BITERR\n", __func__);
>> +	lp->ndev->stats.rx_errors++;
>> +	return 64;	/* Keep the Receiver busy for some time */
>
>64 != 42

I change the description

>
>[...]
>> +static int adf702x_net_xmit(struct sk_buff *skb, struct net_device
>*dev)
>> +{
>> +	struct adf702x_priv *lp = netdev_priv(dev);
>> +	unsigned char *buf_ptr = skb->data;
>> +	int i;
>> +	unsigned char delay;
>> +
>> +	DBG(2, "%s: %s(): transmitting %d bytes\n",
>> +		 dev->name, __func__, skb->len);
>> +
>> +	if (!skb->len)
>> +		return 0;
>
>I don't think this is possible.

Will remove

>
>> +	/* Only one packet at a time. Once TXDONE interrupt is serviced,
>the
>> +	 * queue will be restarted.
>> +	 */
>> +	netif_stop_queue(dev);
>> +	/* save the timestamp */
>> +	lp->ndev->trans_start = jiffies;
>
>Don't set trans_start; the networking core does this now.

ok

>
>> +	/* Remember the skb for deferred processing */
>> +	lp->tx_skb = skb;
>> +
>> +	BUG_ON(lp->tx_skb->len > 0xFFFF);
>
>If you're going to make assertions about the length, it would probably
>be more useful to check against the packet buffer size.

ok

>
>> +	lp->tx_buf[3] = adf702x_getchip(skb->len >> 8);
>> +	lp->tx_buf[4] = adf702x_getchip(skb->len & 0xFF);
>> +
>> +	DBG(3, "TX TX: ");
>> +	for (i = 0; i < skb->len; i++) {
>> +		lp->tx_buf[TX_HEADERSIZE + i] = adf702x_getchip(buf_ptr[i]);
>> +		DBG(3, "%x ", buf_ptr[i]);
>> +	}
>> +	DBG(3, "\n");
>> +
>> +	/* Avoid contentions
>> +	 * Schedule transmission randomly (0..64ms)
>> +	 * This allows other nodes to snip in
>> +	 */
>> +
>> +	get_random_bytes(&delay, sizeof(delay));
>> +	schedule_delayed_work(&lp->tx_work, msecs_to_jiffies(delay >> 2));
>
>get_random_bytes() uses up entropy that should be reserved for
>applications that really need it.  Instead of that, I think you should
>call random32() and use the most significant bits.

ok

>
>[...]
>> +static irqreturn_t adf702x_rx_interrupt(int irq, void *dev_id)
>> +{
>[...]
>> +			lp->rx_size = adf702x_getrxsize(lp, offset);
>> +			if (offset == 1) {
>> +				set_dma_x_count(lp->dma_ch_rx, lp->rx_size);
>> +				set_dma_start_addr(lp->dma_ch_rx,
>> +					(unsigned long)lp->rx_buf);
>> +			} else {
>> +				lp->rx_buf[0] = lp->rx_buf[3];
>> +				set_dma_x_count(lp->dma_ch_rx, lp->rx_size - 1);
>> +				set_dma_start_addr(lp->dma_ch_rx,
>> +					(unsigned long)&lp->rx_buf[1]);
>> +			}
>> +			enable_dma(lp->dma_ch_rx);
>> +			SSYNC();
>
>Is this some odd kind of memory barrier?

Well - it's an instruction that ensures that the write buffers are flushed and all instructions executed. 

>
>[...]
>> +static int __devinit adf702x_probe(struct spi_device *spi)
>> +{
>[...]
>> +	/*
>> +	 * MAC address? we use a
>> +	 * random one ...
>> +	 */
>> +
>> +	random_ether_addr(ndev->dev_addr);
>
>Assuming that the MAC address is not available in non-volatile memory or
>set directly by firmware, it should be provided in platform data.
>
>> +	ndev->mtu = 576;
>
>Even though you use Ethernet frames?

We could stick with the default MTU - but it would worsen the overall response time.

>
>> +	ndev->tx_queue_len = 10;
>> +	ndev->watchdog_timeo = 0;
>> +
>> +	err = register_netdev(ndev);
>> +	if (err) {
>> +		dev_err(&spi->dev, "failed to register netdev\n");
>> +		goto out1;
>> +	}
>
>Don't register the netdev yet!  It could be brought up immediately, even
>though your private structure is not initialised.

ups

>
>[...]
>> +	spin_lock_init(&lp->lock);
>> +	INIT_DELAYED_WORK(&lp->tx_work, adf702x_tx_work);
>> +	INIT_WORK(&lp->tx_done_work, adf702x_tx_done_work);
>> +	init_waitqueue_head(&lp->waitq);
>
>...and now you'll be ready to register the netdev.

ok

>
>> +static int __devexit adf702x_remove(struct spi_device *spi)
>> +{
>> +	struct adf702x_platform_data *pdata = spi->dev.platform_data;
>> +	struct net_device *dev = dev_get_drvdata(&spi->dev);
>> +	struct adf702x_priv *lp = netdev_priv(dev);
>
>First things first:
>
>	rtnl_lock();
>	dev_close(dev);
>	unregister_netdevice(dev);
>	rtnl_unlock();
>
>> +	dma_free_coherent(NULL, MAX_PACKET_SIZE, lp->rx_buf, lp-
>>dma_handle);
>> +	dma_free_coherent(NULL, MAX_PACKET_SIZE, lp->tx_buf, lp-
>>dma_handle);
>> +
>> +	if (lp->irq_sport_err)
>> +		free_irq(lp->irq_sport_err, dev);
>> +
>> +	gpio_free(lp->gpio_int_rfs);
>> +	free_dma(lp->dma_ch_rx);
>> +	free_dma(lp->dma_ch_tx);
>> +	peripheral_free_list(pdata->pin_req);
>> +
>> +	unregister_netdev(dev);
>[...]
>
>This unregister_netdev() should be replaced by the above additions.
>
>Ben.
>
>--
>Ben Hutchings, Senior Software Engineer, Solarflare Communications
>Not speaking for my employer; that's the marketing department's job.
>They asked us to note that Solarflare product names are trademarked.


Thanks and best regards,
Michael

Powered by blists - more mailing lists