Messages in this thread | | | From | Mike Frysinger <> | Subject | Re: [PATCH RFC] sh: maple: Add support for SEGA Dreamcast VMU and clean up maple bus driver (1/3) | Date | Wed, 28 Jan 2009 19:42:20 -0500 |
| |
On Wednesday 28 January 2009 18:57:49 Adrian McMenamin wrote: > --- a/drivers/sh/maple/maple.c > +++ b/drivers/sh/maple/maple.c > -int maple_add_packet_sleeps(struct maple_device *mdev, u32 function, > - u32 command, size_t length, void *data) > -{ > - int locking, ret = 0; > - void *sendbuf = NULL; > - > - locking = mutex_lock_interruptible(&mdev->mq->mutex); > - if (locking) { > - ret = -EIO; > - goto out; > - } > + int ret = 0; > + void *sendbuf; > > + sendbuf = kzalloc(length * 4, GFP_KERNEL); > if (length) { > - sendbuf = kmalloc(length * 4, GFP_KERNEL);
that looks wonky ... you do the alloc first and then check the len ?
> failed_nomem: > + printk(KERN_INFO "maple: could not allocate memory\n");
should be a KERN_NOTICE or worse ... KERN_INFO doesnt seem appropriate
> + if (device_register(&mdev->dev)) { > printk(KERN_INFO > - "Maple bus: Attempt to register device" > - " (%x, %x) failed.\n", > + "Maple bus: Attempt to register device" > + " (%x, %x) failed.\n",
same here ... KERN_INFO shouldnt be used in error handlers
> +static void maple_response_fileerr(struct maple_device *mdev, void > *recvbuf) +{ > + if (mdev->fileerrhandler) { > + mdev->fileerrhandler(mdev, recvbuf); > + return; > + } else > + printk(KERN_INFO "maple: device (%d, %d) reports file error %d\n", > + mdev->port, mdev->unit, ((int *)recvbuf)[1]); > +}
and here ...
> case MAPLE_RESPONSE_AGAIN: > case MAPLE_RESPONSE_BADCMD: > case MAPLE_RESPONSE_BADFUNC: > - printk(KERN_DEBUG > + printk(KERN_INFO > "Maple non-fatal error 0x%X\n", > code);
and here ...
> /* Maple Bus command and response codes */ > enum maple_code { > + MAPLE_RESPONSE_FILEERR = -5, > + MAPLE_RESPONSE_AGAIN = -4, /* retransmit */
do you really need to set every value ? i would only bother when there's a discontinuity ... -mike [unhandled content-type:application/pgp-signature] | |