On Fri, Mar 02, 2012 at 09:26:30PM +0100, Dmitry Tarnyagin wrote:
> WSM is a message interface between the host and the device.
> The patch defines and implements WSM abstraction layer.
Hi!
I just started reading through this driver and had a few comments/
questions; apologies in advance if they've been changed in more
recent versions.
> +void wsm_lock_tx(struct cw1200_common *priv)
> +{
> + wsm_cmd_lock(priv);
> + if (atomic_add_return(1, &priv->tx_lock) == 1) {
> + if (wsm_flush_tx(priv))
> + wsm_printk(KERN_DEBUG "[WSM] TX is locked.\n");
> + }
> + wsm_cmd_unlock(priv);
> +}
Can you explain how tx_lock works and how it's different from existing
locking primitives? As a newcomer I find it surprising that two different
callers could call wsm_lock_tx{_async}() and neither of them would wait,
but maybe that's just a naming thing.
> +void wsm_buf_init(struct wsm_buf *buf)
> +{
> + BUG_ON(buf->begin);
> + buf->begin = kmalloc(SDIO_BLOCK_SIZE, GFP_KERNEL | GFP_DMA);
Does your platform really need GFP_DMA? It's a no-op on many platforms
and on x86 it will restrict this to low 16 MB of physical memory.
> + if (size & (SDIO_BLOCK_SIZE - 1)) {
> + size &= SDIO_BLOCK_SIZE;
> + size += SDIO_BLOCK_SIZE;
> + }
This looks odd, should you use round_up(size, SDIO_BLOCK_SIZE) instead?
For example, if size = 2 * SDIO_BLOCK_SIZE + 1, it'll silently truncate
to SDIO_BLOCK_SIZE. This code is in a couple of places.
--
Bob Copeland %% www.bobcopeland.com
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
