On Fri, Sep 23, 2011 at 10:54:05PM +0300, Dan Carpenter wrote:
> On Fri, Sep 23, 2011 at 06:38:02PM +0200, Julian Andres Klode wrote:
> > Introduce two new functions nvec_msg_alloc() and nvec_msg_free()
> > that allocate and free message buffers from the internal pool
> > of messages.
> >
> > Signed-off-by: Julian Andres Klode <jak@xxxxxxxxxxxxx>
> > ---
> > drivers/staging/nvec/nvec.c | 21 +++++++++++++++++++++
> > drivers/staging/nvec/nvec.h | 6 ++++++
> > 2 files changed, 27 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/staging/nvec/nvec.c b/drivers/staging/nvec/nvec.c
> > index 43a83a9..8a97241 100644
> > --- a/drivers/staging/nvec/nvec.c
> > +++ b/drivers/staging/nvec/nvec.c
> > @@ -17,6 +17,7 @@
> >
> > #include <asm/irq.h>
> >
> > +#include <linux/atomic.h>
> > #include <linux/completion.h>
> > #include <linux/interrupt.h>
> > #include <linux/io.h>
> > @@ -91,6 +92,26 @@ static int nvec_status_notifier(struct notifier_block *nb,
> > return NOTIFY_OK;
> > }
> >
> > +static struct nvec_msg *nvec_msg_alloc(struct nvec_chip *nvec)
> > +{
> > + size_t i;
> This should be "int i;" not "size_t i;" It's a number between 0 and
> 64. Also it would let you avoid the cast below.
Yes, somehow I'm a size_t person, especially when counting
array indices. On ARM it won't make a difference technically,
but on e.g. amd64, the size_t version is supposed to be
faster (according to some). But if we want to be pedantic,
we can make that "unsigned int i;"
>
> Add a blank line between declarations and code.
>
> > + for (i = 0; i < NVEC_POOL_SIZE; i++)
>
> Put a '{' for multi-line blocks for style reasons even if it's not
> needed for semantic reasons.
I also thought that but then forgot it.
> > + if (atomic_xchg(&nvec->msg_pool[i].used, 1) == 0) {
> > + dev_vdbg(nvec->dev, "INFO: Alloc %u\n", (uint) i);
> > + return &nvec->msg_pool[i];
> > + }
> > +
> > + dev_err(nvec->dev, "could not allocate buffer\n");
> > +
> > + return NULL;
> > +}
> > +
> > +static void nvec_msg_free(struct nvec_chip *nvec, struct nvec_msg *msg)
> > +{
> > + dev_vdbg(nvec->dev, "INFO: Free %i\n", (int) (msg - nvec->msg_pool));
>
> I don't have a cross compile environment set up so I can't compile
> this, but surely (msg - nvec->msg_pool) generates some kind of
> compile warning. I'd think you'd have to cast the struct pointers to
> unsigned long or something. I'm not sure also what the printk tells
> us.
They are two pointers, you can subtract two pointers and get an
ptrdiff_t. There is one problem with tx_scratch though, as that's
not part of the array, it is undefined behavior if called on nvec->tx
when nvec->tx == nvec->tx_scratch.
--
Julian Andres Klode - Debian Developer, Ubuntu Member
See http://wiki.debian.org/JulianAndresKlode and http://jak-linux.org/.
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
