Hi Greg,
2012/5/11 Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>:
> On Wed, May 09, 2012 at 12:17:35PM +0800, Yu Xu wrote:
>> @@ -0,0 +1,2197 @@
>> +/*
>> + * Copyright (C) 2011 Marvell International Ltd. All rights reserved.
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License as published by the
>> + * Free Software Foundation; either version 2 of the License, or (at your
>> + * option) any later version.
>
> Do you really mean "any later version"? Be careful here and ensure that
> is what you are wanting.
>
>> +#define DRIVER_DESC "Marvell PXA USB3.0 Device Controller driver"
>> +#define DRIVER_VERSION "31 July 2011"
>
> Why do you need a version? That just got out of date. And is obviously
> wrong, right?
>
>> +static struct mv_u3d *the_controller;
>
> You only allow one per instance of the kernel? Is that wise?
>
>> +static ssize_t
>> +usb_reg_show(struct device *dev, struct device_attribute *attr, char *buf)
>> +{
>> + struct mv_u3d *u3d = the_controller;
>> + struct mv_u3d_op_regs *op_regs = u3d->op_regs;
>> + struct mv_u3d_vuc_regs *vuc_regs = u3d->vuc_regs;
>> + int ret, i;
>> +
>> + ret = sprintf(buf, "[usbcmd] = 0x%x\n",
>> + op_regs->usbcmd);
>> + ret += sprintf(buf + ret, "[dcbaapl] = 0x%x\n",
>> + op_regs->dcbaapl);
>> + ret += sprintf(buf + ret, "[dcbaaph] = 0x%x\n",
>> + op_regs->dcbaaph);
>> + ret += sprintf(buf + ret, "[portsc] = 0x%x\n",
>> + op_regs->portsc);
>> + ret += sprintf(buf + ret, "[portlinkinfo] = 0x%x\n",
>> + op_regs->portlinkinfo);
>> + ret += sprintf(buf + ret, "[doorbell] = 0x%x\n",
>> + op_regs->doorbell);
>> + ret += sprintf(buf + ret, "[ctrlepenable] = 0x%x\n",
>> + vuc_regs->ctrlepenable);
>> + ret += sprintf(buf + ret, "[setuplock] = 0x%x\n",
>> + vuc_regs->setuplock);
>> + ret += sprintf(buf + ret, "[endcomplete] = 0x%x\n",
>> + vuc_regs->endcomplete);
>> + ret += sprintf(buf + ret, "[intrcause] = 0x%x\n",
>> + vuc_regs->intrcause);
>> + ret += sprintf(buf + ret, "[intrenable] = 0x%x\n",
>> + vuc_regs->intrenable);
>> + ret += sprintf(buf + ret, "[trbcomplete] = 0x%x\n",
>> + vuc_regs->trbcomplete);
>> + ret += sprintf(buf + ret, "[linkchange] = 0x%x\n",
>> + vuc_regs->linkchange);
>> + ret += sprintf(buf + ret, "[bridgesetting] = 0x%x\n",
>> + vuc_regs->bridgesetting);
>> + ret += sprintf(buf + ret, "[ltssm] = 0x%x\n",
>> + vuc_regs->ltssm);
>> + ret += sprintf(buf + ret, "[linkcr0] = 0x%x\n",
>> + vuc_regs->linkcr0);
>> + ret += sprintf(buf + ret, "[linkcr1] = 0x%x\n",
>> + vuc_regs->linkcr1);
>> + ret += sprintf(buf + ret, "[usblink] = 0x%x\n",
>> + vuc_regs->usblink);
>> + ret += sprintf(buf + ret, "[ltssmstate] = 0x%x\n",
>> + vuc_regs->ltssmstate);
>> + ret += sprintf(buf + ret, "[linkerrorcause] = 0x%x\n",
>> + vuc_regs->linkerrorcause);
>> + ret += sprintf(buf + ret, "[devaddrtiebrkr] = 0x%x\n",
>> + vuc_regs->devaddrtiebrkr);
>> + for (i = 0; i < 16; i++) {
>> + ret += sprintf(buf + ret, "[ep%doutcr0] = 0x%x\n",
>> + i, vuc_regs->epcr[i].epxoutcr0);
>> + ret += sprintf(buf + ret, "[ep%doutcr1] = 0x%x\n",
>> + i, vuc_regs->epcr[i].epxoutcr1);
>> + ret += sprintf(buf + ret, "[ep%dincr0] = 0x%x\n",
>> + i, vuc_regs->epcr[i].epxincr0);
>> + ret += sprintf(buf + ret, "[ep%dincr1] = 0x%x\n",
>> + i, vuc_regs->epcr[i].epxincr1);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static DEVICE_ATTR(u3d_regs, S_IRUGO|S_IWUSR, usb_reg_show, NULL);
>
> Oh my no.
>
> sysfs files are "one value per file" only. This looks like a debugging
> file, if so, put it in debugfs, it does not belong in sysfs.
>
> Also, remember, if you create/modify sysfs files, you HAVE to have a
> change to Documentation/ABI at the same time, which this patch does not
> have.
>
>> + /* reset ep state machin */
>
> Watch out, the comment spelling fixers will want to fix this up (yes, we
> have people doing that.) Get it right the first time and they will not
> complain.
>
>> + /* Set the max burst size */
>> + switch (desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
>> + case USB_ENDPOINT_XFER_BULK:
>> + if (maxburst > 16) {
>> + dev_info(&u3d->dev->dev,
>> + "max burst should not be greater "
>> + "than 1 on bulk ep\n");
>
> If it shouldn't, then shouln'd this be dev_err()?
>
>> + _ep->maxburst = maxburst = 1;
>> + }
>> + dev_dbg(&u3d->dev->dev,
>> + "maxburst: %d on bulk %s\n", maxburst, ep->name);
>> + break;
>> + case USB_ENDPOINT_XFER_CONTROL:
>> + if (maxburst != 1) {
>> + dev_info(&u3d->dev->dev,
>> + "max burst should be 1 on control ep\n");
>> + _ep->maxburst = maxburst = 1;
>> + }
>> + case USB_ENDPOINT_XFER_INT:
>> + if (maxburst != 1) {
>> + dev_info(&u3d->dev->dev,
>> + "max burst should be 1 on int ep "
>> + "if transfer size is not 1024\n");
>> + _ep->maxburst = maxburst = 1;
>> + }
>> + break;
>> + case USB_ENDPOINT_XFER_ISOC:
>> + if (maxburst != 1) {
>> + dev_info(&u3d->dev->dev,
>> + "max burst should be 1 on isoc ep "
>> + "if transfer size is not 1024\n");
>
> Same for the above calls.
>
>> +static void irq_process_error(struct mv_u3d *u3d)
>> +{
>> + /* Increment the error count */
>> + u3d->errors++;
>> + dev_err(&u3d->dev->dev, "%s\n", __func__);
>
> What is someone going to be able to do with this error message?
>
>> +static void irq_process_link_change(struct mv_u3d *u3d)
>> +{
>> + u32 linkchange;
>> +
>> + linkchange = readl(&u3d->vuc_regs->linkchange);
>> + writel(linkchange, &u3d->vuc_regs->linkchange);
>> +
>> + dev_info(&u3d->dev->dev, "linkchange: 0x%x\n", linkchange);
>> +
>> + if (linkchange & LINK_CHANGE_LINK_UP) {
>> + dev_info(&u3d->dev->dev, "link up: ltssm state: 0x%x\n",
>> + readl(&u3d->vuc_regs->ltssmstate));
>> +
>> + u3d->ep0_dir = EP_DIR_OUT;
>> + u3d->ep0_state = WAIT_FOR_SETUP;
>> +
>> + /* set speed */
>> + u3d->gadget.speed = USB_SPEED_SUPER;
>> + }
>> +
>> + if (linkchange & LINK_CHANGE_SUSPEND)
>> + dev_info(&u3d->dev->dev, "link suspend\n");
>> +
>> + if (linkchange & LINK_CHANGE_RESUME)
>> + dev_info(&u3d->dev->dev, "link resume\n");
>> +
>> + if (linkchange & LINK_CHANGE_WRESET)
>> + dev_info(&u3d->dev->dev, "warm reset\n");
>> +
>> + if (linkchange & LINK_CHANGE_HRESET)
>> + dev_info(&u3d->dev->dev, "hot reset\n");
>> +
>> + if (linkchange & LINK_CHANGE_INACT)
>> + dev_info(&u3d->dev->dev, "inactive\n");
>> +
>> + if (linkchange & LINK_CHANGE_DISABLE_AFTER_U0)
>> + dev_info(&u3d->dev->dev, "ss.disabled\n");
>> +
>> + if (linkchange & LINK_CHANGE_VBUS_INVALID) {
>> + dev_info(&u3d->dev->dev, "vbus invalid\n");
>> + u3d->vbus_valid_detect = 1;
>> + /* if external vbus detect is not supported,
>> + * we handle it here.
>> + */
>> + if (!u3d->pdata->vbus) {
>> + spin_unlock(&u3d->lock);
>> + mv_u3d_vbus_session(&u3d->gadget, 0);
>> + spin_lock(&u3d->lock);
>> + }
>> + }
>> +}
>
> Why is the driver so noisy? Please don't spit all of that out to the
> kernel log unless someone can actually do something with it. This looks
> like it really is debugging information, right? Please change it to
> dev_dbg() than.
>
>> +static void ch9setaddress(struct mv_u3d *u3d, struct usb_ctrlrequest *setup)
>> +{
>> + u32 tmp;
>> + u3d->dev_addr = (u8)setup->wValue;
>
> extra line needed between these lines please.
>
>> + /* add a delay here, or hot reset will occur */
>> + dev_info(&u3d->dev->dev, "%s: 0x%x\n", __func__, u3d->dev_addr);
>
> Why dev_info()?
>
>> + dev_info(&dev->dev, "successful probe usb3 device %s clock gating.\n",
>> + u3d->clock_gating ? "with" : "without");
>
> Why do we care? Please make this dev_dbg().
>
> If a driver successfully loads and starts up, no messages should ever be
> printed out.
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for your review and suggestion indeed! I'll do some modification.
Thanks,
Yu Xu
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
