[ Sorry, I am coming down with the flu today so I'm doing dorky things
like reviewing review comments. I'm not sure how coherent I am. ]
On Tue, Jan 14, 2014 at 01:04:09PM +0000, Lee Jones wrote:
> > +static void rtsx_usb_sg_timed_out(unsigned long data)
> > +{
> > + struct rtsx_ucr *ucr = (struct rtsx_ucr *)data;
>
> What's going to happen when your device runs 64bit?
>
I'm not sure I understand what you mean here. On linux sizeof(long) is
always the same as sizeof(void *).
> > + if (cmd_len > IOBUF_SIZE)
> > + return -EINVAL;
> > +
> > + if (cmd_len % 4)
> > + cmd_len += (4 - cmd_len % 4);
>
> Please document in a comment.
There is a kernel macro for this:
cmd_len = ALIGN(cmd_len, 4);
if (cmd_len > IOBUF_SIZE)
return -EINVAL;
>
> > +
> > +
>
> Extra '/n'
>
It weirds me out when you mix up '\n' and /n'.
> > +int rtsx_usb_ep0_write_register(struct rtsx_ucr *ucr, u16 addr,
> > + u8 mask, u8 data)
> > +{
> > + u16 value = 0, index = 0;
> > +
> > + value |= 0x03 << 14;
> > + value |= addr & 0x3FFF;
> > + value = ((value << 8) & 0xFF00) | ((value >> 8) & 0x00FF);
> > + index |= (u16)mask;
> > + index |= (u16)data << 8;
>
> Lots of random numbers here, please #define for clarity and ease of
> reading.
>
The only really random number is the 0x03, but yeah, it would help if
that we a define.
addr |= 0x03 << 14;
value = __swab16(addr);
index = mask | (data << 8);
> > +
> > + dev_dbg(&intf->dev,
> > + ": Realtek USB Card Reader found at bus %03d address %03d\n",
> > + usb_dev->bus->busnum, usb_dev->devnum);
> > +
> > + ucr = kzalloc(sizeof(struct rtsx_ucr), GFP_KERNEL);
>
> s/struct rtsx_ucr/*ucr/
>
> Any reason for not using managed resources?
>
Roger, he means the devm_kzalloc().
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
