| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CADB-GoLuw+voV6cbMSa68Yzhow78k6h82-m1VFa1V4v2_nUv0A@mail.gmail.com>
Date: Thu, 30 May 2013 10:56:29 +0800
From: Yuan-Hsin Chen <yuanlmm@...il.com>
To: Felipe Balbi <balbi@...com>
Cc: Yuan-Hsin Chen <yhchen@...aday-tech.com>,
Greg KH <gregkh@...uxfoundation.org>,
USB list <linux-usb@...r.kernel.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] usb: gadget: add Faraday fotg210_udc driver
Hi,
On Thu, May 30, 2013 at 7:55 AM, Felipe Balbi <balbi@...com> wrote:
> Hi,
>
> On Wed, May 29, 2013 at 06:31:50PM +0000, Yuan-Hsin Chen wrote:
>> Faraday fotg210 udc driver supports only Bulk transfer so far.
>> fotg210 could be configured as an USB2.0 peripheral.
>>
>> This driver is tested with mass storage gadget driver on Faraday
>> EVB a369.
>>
>> Signed-off-by: Yuan-Hsin Chen <yhchen@...aday-tech.com>
>
> Run through checkpatch.pl --strict and fix all errors, warnings and all
> checks which make sense.
>
> WARNING: please write a paragraph that describes the config symbol fully
> #81: FILE: drivers/usb/gadget/Kconfig:196:
> +config USB_FOTG210_UDC
>
> CHECK: braces {} should be used on all arms of this statement
> #183: FILE: drivers/usb/gadget/fotg210-udc.c:76:
> + if (ep->epnum) {
> [...]
> + } else
> [...]
>
> WARNING: line over 80 characters
> #190: FILE: drivers/usb/gadget/fotg210-udc.c:83:
> +static void fotg210_fifo_ep_mapping(struct fotg210_ep *ep, u32 epnum, u32 dir_in)
>
> WARNING: line over 80 characters
> #237: FILE: drivers/usb/gadget/fotg210-udc.c:130:
> +static void fotg210_set_mps(struct fotg210_ep *ep, u32 epnum, u32 mps, u32 dir_in)
>
> WARNING: line over 80 characters
> #241: FILE: drivers/usb/gadget/fotg210-udc.c:134:
> + u32 offset = dir_in ? FOTG210_INEPMPSR(epnum) : FOTG210_OUTEPMPSR(epnum);
>
> WARNING: line over 80 characters
> #349: FILE: drivers/usb/gadget/fotg210-udc.c:242:
> +static void fotg210_ep_free_request(struct usb_ep *_ep, struct usb_request *_req)
>
> WARNING: Avoid CamelCase: <DMATFNR_ACC_Fn>
> #372: FILE: drivers/usb/gadget/fotg210-udc.c:265:
> + value |= DMATFNR_ACC_Fn(ep->epnum - 1);
>
> WARNING: line over 80 characters
> #418: FILE: drivers/usb/gadget/fotg210-udc.c:311:
> + value = ioread32(ep->fotg210->reg + FOTG210_FIBCR(ep->epnum - 1));
>
> WARNING: line over 80 characters
> #420: FILE: drivers/usb/gadget/fotg210-udc.c:313:
> + iowrite32(value, ep->fotg210->reg + FOTG210_FIBCR(ep->epnum - 1));
>
> WARNING: line over 80 characters
> #441: FILE: drivers/usb/gadget/fotg210-udc.c:334:
> + length = ioread32(ep->fotg210->reg + FOTG210_FIBCR(ep->epnum - 1));
>
> WARNING: Avoid CamelCase: <FIBCR_BCFx>
> #442: FILE: drivers/usb/gadget/fotg210-udc.c:335:
> + length &= FIBCR_BCFx;
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> #456: FILE: drivers/usb/gadget/fotg210-udc.c:349:
> + printk(KERN_DEBUG "dma_mapping_error\n");
>
> CHECK: Alignment should match open parenthesis
> #461: FILE: drivers/usb/gadget/fotg210-udc.c:354:
> + dma_sync_single_for_device(NULL, d, length,
> + ep->dir_in ? DMA_TO_DEVICE : DMA_FROM_DEVICE);
>
> WARNING: line over 80 characters
> #476: FILE: drivers/usb/gadget/fotg210-udc.c:369:
> +static void fotg210_ep0_queue(struct fotg210_ep *ep, struct fotg210_request *req)
>
> CHECK: braces {} should be used on all arms of this statement
> #483: FILE: drivers/usb/gadget/fotg210-udc.c:376:
> + if (req->req.length) {
> [...]
> + } else
> [...]
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> #486: FILE: drivers/usb/gadget/fotg210-udc.c:379:
> + printk(KERN_DEBUG "%s : req->req.length = 0x%x\n",
>
> CHECK: Alignment should match open parenthesis
> #487: FILE: drivers/usb/gadget/fotg210-udc.c:380:
> + printk(KERN_DEBUG "%s : req->req.length = 0x%x\n",
> + __func__, req->req.length);
>
> CHECK: braces {} should be used on all arms of this statement
> #492: FILE: drivers/usb/gadget/fotg210-udc.c:385:
> + if (!req->req.length)
> [...]
> + else {
> [...]
>
> WARNING: line over 80 characters
> #495: FILE: drivers/usb/gadget/fotg210-udc.c:388:
> + u32 value = ioread32(ep->fotg210->reg + FOTG210_DMISGR0);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> #680: FILE: drivers/usb/gadget/fotg210-udc.c:573:
> + printk(KERN_DEBUG " 0x%x\n", data);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> #691: FILE: drivers/usb/gadget/fotg210-udc.c:584:
> + printk(KERN_DEBUG " 0x%x\n", data);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> #696: FILE: drivers/usb/gadget/fotg210-udc.c:589:
> + printk(KERN_DEBUG " 0x%x\n", data);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> #702: FILE: drivers/usb/gadget/fotg210-udc.c:595:
> + printk(KERN_DEBUG " 0x%x\n", data);
>
> WARNING: Prefer netdev_dbg(netdev, ... then dev_dbg(dev, ... then pr_debug(... to printk(KERN_DEBUG ...
> #741: FILE: drivers/usb/gadget/fotg210-udc.c:634:
> + printk(KERN_DEBUG "request error!!\n");
>
> WARNING: line over 80 characters
> #744: FILE: drivers/usb/gadget/fotg210-udc.c:637:
> +static void fotg210_set_address(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> WARNING: Avoid CamelCase: <ctrl->wValue>
> #746: FILE: drivers/usb/gadget/fotg210-udc.c:639:
> + if (ctrl->wValue >= 0x0100)
>
> CHECK: braces {} should be used on all arms of this statement
> #746: FILE: drivers/usb/gadget/fotg210-udc.c:639:
> + if (ctrl->wValue >= 0x0100)
> [...]
> + else {
> [...]
>
> WARNING: line over 80 characters
> #754: FILE: drivers/usb/gadget/fotg210-udc.c:647:
> +static void fotg210_set_feature(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> WARNING: Avoid CamelCase: <ctrl->bRequestType>
> #756: FILE: drivers/usb/gadget/fotg210-udc.c:649:
> + switch (ctrl->bRequestType & USB_RECIP_MASK) {
>
> WARNING: Avoid CamelCase: <ctrl->wIndex>
> #765: FILE: drivers/usb/gadget/fotg210-udc.c:658:
> + epnum = le16_to_cpu(ctrl->wIndex) & USB_ENDPOINT_NUMBER_MASK;
>
> WARNING: line over 80 characters
> #779: FILE: drivers/usb/gadget/fotg210-udc.c:672:
> +static void fotg210_clear_feature(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> WARNING: line over 80 characters
> #821: FILE: drivers/usb/gadget/fotg210-udc.c:714:
> +static void fotg210_get_status(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> WARNING: line over 80 characters
> #836: FILE: drivers/usb/gadget/fotg210-udc.c:729:
> + fotg210_is_epnstall(fotg210->ep[epnum]) << USB_ENDPOINT_HALT;
>
> WARNING: line over 80 characters
> #854: FILE: drivers/usb/gadget/fotg210-udc.c:747:
> +static int fotg210_setup_packet(struct fotg210_udc *fotg210, struct usb_ctrlrequest *ctrl)
>
> CHECK: braces {} should be used on all arms of this statement
> #870: FILE: drivers/usb/gadget/fotg210-udc.c:763:
> + if ((ctrl->bRequestType & USB_TYPE_MASK) == USB_TYPE_STANDARD) {
> [...]
> + } else
> [...]
>
> WARNING: Avoid CamelCase: <ctrl->bRequest>
> #871: FILE: drivers/usb/gadget/fotg210-udc.c:764:
> + switch (ctrl->bRequest) {
>
> CHECK: braces {} should be used on all arms of this statement
> #902: FILE: drivers/usb/gadget/fotg210-udc.c:795:
> + if (!list_empty(&ep->queue) && !ep->dir_in) {
> [...]
> + } else
> [...]
>
> CHECK: braces {} should be used on all arms of this statement
> #921: FILE: drivers/usb/gadget/fotg210-udc.c:814:
> + if ((!list_empty(&ep->queue)) && (ep->dir_in)) {
> [...]
> + } else
> [...]
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #989: FILE: drivers/usb/gadget/fotg210-udc.c:882:
> + printk(KERN_INFO"fotg210 udc reset\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #995: FILE: drivers/usb/gadget/fotg210-udc.c:888:
> + printk(KERN_INFO"fotg210 udc suspend\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1001: FILE: drivers/usb/gadget/fotg210-udc.c:894:
> + printk(KERN_INFO"fotg210 udc resume\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1007: FILE: drivers/usb/gadget/fotg210-udc.c:900:
> + printk(KERN_INFO"fotg210 iso sequence error\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1013: FILE: drivers/usb/gadget/fotg210-udc.c:906:
> + printk(KERN_INFO"fotg210 iso sequence abort\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1020: FILE: drivers/usb/gadget/fotg210-udc.c:913:
> + printk(KERN_INFO"fotg210 transferred 0 byte\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1027: FILE: drivers/usb/gadget/fotg210-udc.c:920:
> + printk(KERN_INFO"fotg210 received 0 byte\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1047: FILE: drivers/usb/gadget/fotg210-udc.c:940:
> + printk(KERN_INFO "fotg210 CX command abort\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1051: FILE: drivers/usb/gadget/fotg210-udc.c:944:
> + printk(KERN_INFO "fotg210 ep0 setup\n");
>
> WARNING: line over 80 characters
> #1054: FILE: drivers/usb/gadget/fotg210-udc.c:947:
> + if (fotg210->driver->setup(&fotg210->gadget, &ctrl) < 0)
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1060: FILE: drivers/usb/gadget/fotg210-udc.c:953:
> + printk(KERN_INFO "fotg210 cmd end\n");
>
> WARNING: Prefer netdev_info(netdev, ... then dev_info(dev, ... then pr_info(... to printk(KERN_INFO ...
> #1070: FILE: drivers/usb/gadget/fotg210-udc.c:963:
> + printk(KERN_INFO "fotg210 ep0 fail\n");
>
> CHECK: Alignment should match open parenthesis
> #1129: FILE: drivers/usb/gadget/fotg210-udc.c:1022:
> + iowrite32(GMIR_MHC_INT | GMIR_MOTG_INT | GMIR_INT_POLARITY,
> + fotg210->reg + FOTG210_GMIR);
>
> WARNING: space prohibited before semicolon
> #1237: FILE: drivers/usb/gadget/fotg210-udc.c:1130:
> + for (i = 0; i < FOTG210_MAX_NUM_EP ; i++) {
>
> CHECK: Alignment should match open parenthesis
> #1243: FILE: drivers/usb/gadget/fotg210-udc.c:1136:
> + list_add_tail(&fotg210->ep[i]->ep.ep_list,
> + &fotg210->gadget.ep_list);
>
> WARNING: kfree(NULL) is safe this check is probably not required
> #1291: FILE: drivers/usb/gadget/fotg210-udc.c:1184:
> + if (fotg210)
> + kfree(fotg210);
>
> CHECK: No space is necessary after a cast
> #1298: FILE: drivers/usb/gadget/fotg210-udc.c:1191:
> + .driver = {
> + .name = (char *) udc_name,
>
> WARNING: line over 80 characters
> #1477: FILE: drivers/usb/gadget/fotg210.h:162:
> +#define EPMAP_FIFONO(ep, dir) ((((ep) - 1) << ((ep) - 1) * 8) << ((dir) ? 0 : 4))
>
> WARNING: line over 80 characters
> #1478: FILE: drivers/usb/gadget/fotg210.h:163:
> +#define EPMAP_FIFONOMSK(ep, dir) ((3 << ((ep) - 1) * 8) << ((dir) ? 0 : 4))
>
> CHECK: spinlock_t definition without comment
> #1549: FILE: drivers/usb/gadget/fotg210.h:234:
> + spinlock_t lock;
>
> total: 0 errors, 45 warnings, 13 checks, 1472 lines checked
>
> Your patch has style problems, please review.
>
> If any of these errors are false positives, please report
> them to the maintainer, see CHECKPATCH in MAINTAINERS.
>
Sorry about my negligence. I'll fix and resend it later.
Thanks for your time.
Yuan-Hsin
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists