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
