Re: [PATCH] USB device driver of Topcliff PCH

[Masayuki Ohtake]

Hi Micha?, Greg and Maurus.

Thank you for your comments.
I confirm them.

Thanks, Ohtake
----- Original Message ----- 
From: "Micha? Nazarewicz" <m.nazarewicz@xxxxxxxxxxx>
To: "Randy Dunlap" <randy.dunlap@xxxxxxxxxx>; "Peter Korsgaard" <peter.korsgaard@xxxxxxxxx>; "Nicolas Ferre"
<nicolas.ferre@xxxxxxxxx>; "Maurus Cuelenaere" <mcuelenaere@xxxxxxxxx>; "linux-usb" <linux-usb@xxxxxxxxxxxxxxx>;
"Laurent Pinchart" <laurent.pinchart@xxxxxxxxxxxxxxxx>; "Greg Kroah-Hartman" <gregkh@xxxxxxx>; "Fabien Chouteau"
<fabien.chouteau@xxxxxxxxx>; "david Brownell" <dbrownell@xxxxxxxxxxxxxxxxxxxxx>; "Christoph Egger"
<siccegge@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx>; "LKML" <linux-kernel@xxxxxxxxxxxxxxx>; "MeeGo" <meego-dev@xxxxxxxxx>;
"Masayuki Ohtake" <masa-korg@xxxxxxxxxxxxxxx>
Cc: "Wang, Qi" <qi.wang@xxxxxxxxx>; "Wang, Yong Y" <yong.y.wang@xxxxxxxxx>; "Andrew" <andrew.chih.howe.khor@xxxxxxxxx>;
"Intel OTC" <joel.clark@xxxxxxxxx>; "Foster, Margie" <margie.foster@xxxxxxxxx>; "Arjan" <arjan@xxxxxxxxxxxxxxx>;
"Toshiharu Okada" <okada533@xxxxxxxxxxxxxxx>; "Takahiro Shimizu" <shimizu394@xxxxxxxxxxxxxxx>; "Tomoya Morinaga"
<morinaga526@xxxxxxxxxxxxxxx>
Sent: Wednesday, September 08, 2010 10:58 AM
Subject: Re: [PATCH] USB device driver of Topcliff PCH


Not sure why I'm on the "To" list, but here are a few of my comments:

On Tue, 07 Sep 2010 09:49:03 +0200, Masayuki Ohtake <masa-korg@xxxxxxxxxxxxxxx> wrote:
> +/**
> + * pch_udc_write_csr - Write the command and status registers.

IIRC, the correct way to write kernel doc is:

+ * pch_udc_write_csr() - Write the command and status registers.

Note the "()".  This applies to other functions as well.

> + * @val: value to be written to CSR register
> + * @addr: address of CSR register
> + */
> +inline void pch_udc_write_csr(unsigned long val, unsigned long addr)

As it was pointed, unless those functions are extern, make them static and remove
the inline.

> +{
> + int count = MAX_LOOP;
> +
> + /* Wait till idle */
> + while ((count > 0) &&\
> + (ioread32((u32 *)(PCH_UDC_CSR_BUSY_ADDR + pch_udc_base)) &
> + PCH_UDC_CSR_BUSY))
> + count--;

I'd say: "while (ioread(...) && --count);"  Also, I'd make count to be unsigned.

> +
> + if (count < 0)
> + pr_debug("%s: wait error; count = %x", __func__, count);

Dead code.  If MAX_LOOP is >= 0 count will never get negative.  Did you mean
if (!count)?

> +
> + iowrite32(val, (u32 *)addr);
> + /* Wait till idle */
> + count = MAX_LOOP;
> + while ((count > 0) &&
> + (ioread32((u32 *)(PCH_UDC_CSR_BUSY_ADDR + pch_udc_base)) &
> + PCH_UDC_CSR_BUSY))
> + count--;
> +
> + if (count < 0)
> + pr_debug("%s: wait error; count = %x", __func__, count);

Dead code.

> +/**
> + * pch_udc_read_csr - Read the command and status registers.
> + * @addr: address of CSR register
> + * Returns
> + * content of CSR register
> + */
> +inline u32 pch_udc_read_csr(unsigned long addr)

All comments to the pch_udc_write_csr() function apply here as well.

> +/**
> + * pch_udc_get_speed - Return the speed status
> + * @dev: Reference to pch_udc_regs structure
> + * Retern The speed(LOW=1, FULL=2, HIGH=3)
> + */
> +inline int pch_udc_get_speed(struct pch_udc_regs __iomem *dev)
> +{
> + u32 val;
> +
> + val = ioread32(&dev->devsts);

It's just me, but why not join the two lines together:

+ u32 val = ioread32(&dev->devsts);

> + return (val & UDC_DEVSTS_ENUM_SPEED_MASK) >> UDC_DEVSTS_ENUM_SPEED_OFS;
> +}

> +/**
> + * pch_udc_ep_clear_nak - Set the bit 8 (CNAK field)
> + * of the endpoint control register
> + * @ep: reference to structure of type pch_udc_ep_regs
> + */
> +void pch_udc_ep_clear_nak(struct pch_udc_ep_regs __iomem *ep)
> +{
> + unsigned int loopcnt = 0;
> +
> + if (ioread32(&ep->epctl) & (1 << UDC_EPCTL_NAK)) {

if (!(ioread32(&ep->epctl) & (1 << UDC_EPCTL_NAK)))
return;

and then drop one indention level for the rest of the function.
This will help to keep indention level nearer the recommended
limit of 3.

> + if (!(EP_IS_IN(ep))) {
> + while ((pch_udc_read_ep_status(ep) &
> + (1 << UDC_EPSTS_MRXFIFO_EMP)) == 0) {
> + if (loopcnt++ > 100000) {
> + pr_debug("%s: RxFIFO not Empty "
> +   "loop count = %d",
> +   __func__, loopcnt);
> + break;
> + }
> + udelay(100);
> + }
> + }
> + while (ioread32(&ep->epctl) & (1 << UDC_EPCTL_NAK)) {
> + PCH_UDC_BIT_SET(&ep->epctl, 1 << UDC_EPCTL_CNAK);
> + udelay(5);
> + if (loopcnt++ >= 25) {
> + pr_debug("%s: Clear NAK not set for"
> +   "ep%d%s: counter=%d",
> +   __func__, EP_NUM(ep),
> +   (EP_IS_IN(ep) ? "in" : "out"),
> +   loopcnt);
> + break;
> + }
> + }
> + }
> +}
> +
> +/**
> + * pch_udc_ep_fifo_flush - Flush the endpoint fifo
> + * @ep: reference to structure of type pch_udc_ep_regs
> + * @dir: direction of endpoint
> + * - dir = 0 endpoint is OUT
> + * - dir != 0 endpoint is IN
> + */
> +void pch_udc_ep_fifo_flush(struct pch_udc_ep_regs __iomem *ep, int dir)
> +{
> + unsigned int loopcnt = 0;
> +
> + pr_debug("%s: ep%d%s", __func__, EP_NUM(ep),
> + (EP_IS_IN(ep) ? "in" : "out"));
> + if (dir) { /* IN ep */
> + PCH_UDC_BIT_SET(&ep->epctl, 1 << UDC_EPCTL_F);

I'd add "return;" here and move the else part out of the else dropping
one indention level.

> + } else {
> + if ((pch_udc_read_ep_status(ep) &
> +     (1 << UDC_EPSTS_MRXFIFO_EMP)) == 0) {

if (pch_udc_read_ep_status(ep) & (1 << UDC_EPSTS_MRXFIFO_EMP)
return;

and drop indention.

> + PCH_UDC_BIT_SET(&ep->epctl, 1 << UDC_EPCTL_MRXFLUSH);
> + /* Wait for RxFIFO Empty */
> + while ((pch_udc_read_ep_status(ep) &
> + (1 << UDC_EPSTS_MRXFIFO_EMP)) == 0) {
> + if (loopcnt++ > 1000000) {
> + pr_debug("RxFIFO not Empty loop"
> +   " count = %d", loopcnt);
> + break;
> + }
> + udelay(100);
> + }
> + PCH_UDC_BIT_CLR(&ep->epctl, 1 << UDC_EPCTL_MRXFLUSH);
> + }
> + }
> +}

> +/**
> + * pch_udc_pcd_pullup - This API is invoked to make the device
> + * visible/invisible to the host
> + * @gadget: Reference to the gadget driver
> + * @is_on: Specifies whether the pull up is made active or inactive
> + * Returns
> + * 0: Success
> + * -EINVAL: If the gadget passed is NULL
> + */
> +static int pch_udc_pcd_pullup(struct usb_gadget *gadget, int is_on)
> +{
> + struct pch_udc_dev *dev;
> +
> + pr_debug("%s: enter", __func__);

It just struck me.  Wouldn't it be feasible to use "dev_*" instead of "pr_*"?

> + if (gadget == NULL) {
> + pr_debug("%s: exit -EINVAL", __func__);
> + return -EINVAL;
> + }
> + dev = container_of(gadget, struct pch_udc_dev, gadget);
> + if (is_on == 0)
> + pch_udc_set_disconnect(dev->regs);
> + else
> + pch_udc_clear_disconnect(dev->regs);

There was function that did exactly that I think.  Wasn't there?

> + return 0;
> +}

> +/**
> + * pch_udc_start_next_txrequest - This function starts
> + * the next transmission requirement
> + * @ep: Reference to the endpoint structure
> + */
> +static void pch_udc_start_next_txrequest(struct pch_udc_ep *ep)
> +{
> + struct pch_udc_request *req;
> +
> + pr_debug("%s: enter", __func__);
> + if (pch_udc_read_ep_control(ep->regs) & (1 << UDC_EPCTL_P))
> + return;
> +
> + if (!list_empty(&ep->queue)) {

if (list_empty(...))
return;

and drop indention.

> + /* next request */
> + req = list_entry(ep->queue.next, struct pch_udc_request, queue);
> + if (req && !req->dma_going) {

Same here.

> + pr_debug("%s: Set request: req=%p req->td_data=%p",
> + __func__, req, req->td_data);
> + if (req->td_data) {

Same eher.

> + struct pch_udc_data_dma_desc *td_data;
> +
> + while (pch_udc_read_ep_control(ep->regs) &
> + (1 << UDC_EPCTL_S))
> + udelay(100);
> +
> + req->dma_going = 1;
> + /* Clear the descriptor pointer */
> + pch_udc_ep_set_ddptr(ep->regs, 0);
> +
> + td_data = req->td_data;
> + while (1) {
> + td_data->status = (td_data->status &
> + ~PCH_UDC_BUFF_STS) |
> + PCH_UDC_BS_HST_RDY;
> + if ((td_data->status &
> + PCH_UDC_DMA_LAST) ==
> + PCH_UDC_DMA_LAST)
> + break;

The line above has 6 levels of indention.  If you drop indentions the way
described above you get back to 3.

> +
> + td_data =
> + (struct pch_udc_data_dma_desc *)\
> + phys_to_virt(td_data->next);
> + }
> + /* Write the descriptor pointer */
> + pch_udc_ep_set_ddptr(ep->regs,
> + req->td_data_phys);
> + pch_udc_set_dma(ep->dev->regs, DMA_DIR_TX);
> + /* Set the poll demand bit */
> + pch_udc_ep_set_pd(ep->regs);
> + pch_udc_enable_ep_interrupts(ep->dev->regs,
> + 1 << (ep->in ? ep->num :\
> + ep->num + UDC_EPINT_OUT_EP0));
> + pch_udc_ep_clear_nak(ep->regs);
> + }
> + }
> + }
> +}
> +
> +/**
> + * pch_udc_complete_transfer - This function completes a transfer
> + * @ep: Reference to the endpoint structure
> + */
> +static void pch_udc_complete_transfer(struct pch_udc_ep *ep)

Same as with the function above.

> +/**
> + * pch_udc_complete_receiver - This function completes a receiver
> + * @ep: Reference to the endpoint structure
> + */
> +static void pch_udc_complete_receiver(struct pch_udc_ep *ep)

This function would use some indention fixing as well.

> + if (list_empty(&ep->queue)) {
> + /* enable DMA */
> + pch_udc_set_dma(dev->regs, DMA_DIR_RX);
> + }

Drop the "{" and "}". script/checkpatch.pl will find issues like this one.

> +}

> +static void pch_udc_read_all_epstatus(struct pch_udc_dev *dev, u32 ep_intr)
> +{
> + int i;
> + struct pch_udc_ep *ep;
> +
> + for (i = 0; i < PCH_UDC_USED_EP_NUM; i++) {
> + /* IN */
> + if (ep_intr & (0x1 << i)) {
> + ep = &dev->ep[2*i];
> + ep->epsts = pch_udc_read_ep_status(ep->regs);
> + pch_udc_clear_ep_status(ep->regs, ep->epsts);
> + }
> + /* OUT */
> + if (ep_intr & (0x10000 << i)) {
> + ep = &dev->ep[2*i+1];
> + ep->epsts = pch_udc_read_ep_status(ep->regs);
> + pch_udc_clear_ep_status(ep->regs, ep->epsts);
> + }
> + }
> + return;

Useless return.

> +}

> +/**
> + * pch_udc_svc_enum_interrupt - This function handles a USB speed enumeration
> + * done interrupt
> + * @dev: Reference to driver structure
> + */
> +void
> +pch_udc_svc_enum_interrupt(struct pch_udc_dev *dev)

Useless line break.  This applies not only to this function.

> +void
> +pch_udc_svc_intf_interrupt(struct pch_udc_dev *dev)
> +{
> + u32 reg, dev_stat = 0;
> + int i, ret;
> +
> + pr_debug("%s: enter", __func__);
> + dev_stat = pch_udc_read_device_status(dev->regs);
> + dev->cfg_data.cur_intf = (dev_stat & UDC_DEVSTS_INTF_MASK) >>
> + UDC_DEVSTS_INTF_OFS;
> + dev->cfg_data.cur_alt = (dev_stat & UDC_DEVSTS_ALT_MASK) >>
> + UDC_DEVSTS_ALT_OFS;
> + pr_debug("DVSTATUS=%08x, cfg=%d, intf=%d, alt=%d", dev_stat,
> + (dev_stat & UDC_CSR_NE_CFG_MASK) >> UDC_CSR_NE_CFG_OFS,
> + dev->cfg_data.cur_intf, dev->cfg_data.cur_alt);
> +
> + dev->set_cfg_not_acked = 1;
> +
> + /* Construct the usb request for gadget driver and inform it */
> + memset(&setup_data, 0 , sizeof setup_data);
> + setup_data.request.bRequest = USB_REQ_SET_INTERFACE;
> + setup_data.request.bRequestType = USB_RECIP_INTERFACE;
> + setup_data.request.wValue = cpu_to_le16(dev->cfg_data.cur_alt);
> + setup_data.request.wIndex = cpu_to_le16(dev->cfg_data.cur_intf);
> +
> + /* programm the Endpoint Cfg registers */
> + for (i = 0; i < PCH_UDC_USED_EP_NUM * 2; i++) {
> + if (i == 1) { /* Only one end point cfg register */
> + reg = pch_udc_read_csr((u32) (&dev->csr->ne[i]));
> + reg = (reg & ~UDC_CSR_NE_INTF_MASK) |
> + (dev->cfg_data.cur_intf << UDC_CSR_NE_INTF_OFS);
> + reg = (reg & ~UDC_CSR_NE_ALT_MASK) |
> + (dev->cfg_data.cur_alt << UDC_CSR_NE_ALT_OFS);
> + pch_udc_write_csr(reg, (u32) (&dev->csr->ne[i]));
> + }

Could this if be put outside of the loop? This applies not only to this function.

> + /* clear stall bits */
> + pch_udc_ep_clear_stall(dev->ep[i].regs);
> + dev->ep[i].halted = 0;
> + }
> + dev->stall = 0;
> + spin_unlock(&dev->lock);
> + ret = dev->driver->setup(&dev->gadget, &setup_data.request);
> + spin_lock(&dev->lock);
> +}

> +irqreturn_t pch_udc_isr(int irq, void *pdev)
> +{
> + struct pch_udc_dev *dev;
> + u32 dev_intr, ep_intr;
> + int i;
> +
> + pr_debug("%s: enter", __func__);
> + dev = (struct pch_udc_dev *) pdev;
> + dev_intr = pch_udc_read_device_interrupts(dev->regs);
> + ep_intr = pch_udc_read_ep_interrupts(dev->regs);
> +
> + if (dev_intr != 0) {
> + /* Clear device interrupts */
> + pch_udc_write_device_interrupts(dev->regs, dev_intr);
> + }
> + if (ep_intr != 0) {
> + /* Clear ep interrupts */
> + pch_udc_write_ep_interrupts(dev->regs, ep_intr);
> + }

Useless "{" and "}" in the two above ifs.

> + if ((dev_intr == 0) && (ep_intr == 0)) {
> + pr_debug("%s: exit IRQ_NONE", __func__);
> + return IRQ_NONE;
> + }
> + spin_lock(&dev->lock);
> +
> + if (dev_intr != 0) {
> + pr_debug("%s: device intr 0x%x", __func__, dev_intr);
> + pch_udc_dev_isr(dev, dev_intr);
> + }
> +
> + if (ep_intr != 0) {
> + pr_debug("%s: ep intr 0x%x", __func__, ep_intr);
> + pch_udc_read_all_epstatus(dev, ep_intr);
> +
> + /* Process Control In interrupts, if present */
> + if (ep_intr & (1 << UDC_EPINT_IN_EP0)) {
> + pch_udc_svc_control_in(dev);
> + pch_udc_postsvc_epinters(dev, 0);
> + }
> + /* Process Control Out interrupts, if present */
> + if (ep_intr & (1 << UDC_EPINT_OUT_EP0))
> + pch_udc_svc_control_out(dev);
> +
> + /* Process data in end point interrupts */
> + for (i = 1; i < PCH_UDC_USED_EP_NUM; i++) {
> + if (ep_intr & (1 <<  i)) {
> + pch_udc_svc_data_in(dev, i);
> + pch_udc_postsvc_epinters(dev, i);
> + }
> + }
> + /* Process data out end point interrupts */
> + for (i = UDC_EPINT_OUT_EP1; i < (UDC_EPINT_OUT_EP0 +
> + PCH_UDC_USED_EP_NUM); i++) {
> + if (ep_intr & (1 <<  i))
> + pch_udc_svc_data_out(dev, i -
> + UDC_EPINT_OUT_EP0);
> + }

Useless "{" and "}" in for.

> + }
> + spin_unlock(&dev->lock);
> + return IRQ_HANDLED;
> +}

> +int pch_udc_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> + unsigned long resource;
> + unsigned long len;
> + int retval = 0;
> + struct pch_udc_dev *dev;
> +
> + dev_dbg(&pdev->dev, "%s: enter", __func__);
> + /* one udc only */
> + if (pch_udc != NULL) {
> + dev_err(&pdev->dev, "%s: already probed", __func__);
> + return -EBUSY;
> + }
> +
> + /* init */
> + dev = kzalloc(sizeof(struct pch_udc_dev), GFP_KERNEL);

I just noticed it here but it may apply to other places as well.  I recommend:

+ dev = kzalloc(sizeof *dev, GFP_KERNEL);

> + if (dev == NULL) {
> + dev_err(&pdev->dev, "%s: no memory for device structure",
> + __func__);
> + return -ENOMEM;
> + }
> + memset(dev, 0, sizeof(struct pch_udc_dev));

kzalloc() does that.

> +/**
> + * pch_udc_cfg_data - Structure to hold current configuration
> + * and interface information

This applies to other places as well.  The above should read:

+ * struct pch_udc_cfg_data - ...

> + * @cur_cfg current configuration in use
> + * @cur_intf current interface in use
> + * @cur_alt current alt interface in use

And there should be ":" after member name.

> + */
> +struct pch_udc_cfg_data {
> + u16 cur_cfg;
> + u16 cur_intf;
> + u16 cur_alt;
> +};

> +/**
> + * pch_udc_dev - Structure holding complete information of the PCH USB device
> + *
> + * @gadget gadget driver data
> + * @driver; reference to gadget driver bound
> + * @pdev; reference to the PCI device
> + * @ep[PCH_UDC_EP_NUM]; array of endpoints
> + * @lock; protects all state
> + * @active:1, enabled the PCI device
> + * @stall:1, stall requested
> + * @prot_stall:1, protcol stall requested
> + * @irq_registered:1, irq registered with system
> + * @mem_region:1, device memory mapped
> + * @registered:1, driver regsitered with system
> + * @suspended:1, driver in suspended state
> + * @connected:1, gadget driver associated
> + * @set_cfg_not_acked:1, pending acknowledgement 4 setup
> + * @waiting_zlp_ack:1; pending acknowledgement 4 ZLP
> + * @csr; address of config & status
> + * @regs; address of device registers
> + * @*ep_regs; address of endpoint registers
> + * @data_requests; DMA pool for data requests
> + * @stp_requests; DMA pool for setup requests
> + * @phys_addr; of device memory
> + * @virt_addr; for mapped device memory
> + * @irq; IRQ line for the device
> + * @cfg_data; current cfg, intf, and alt in use
> + */

Useless ":1", there should be ":" after member name and not nothing, ";" or ",".

> +

Needless empty line.

> +struct pch_udc_dev {

-- 
Best regards,                                        _     _
| Humble Liege of Serenely Enlightened Majesty of  o' \,=./ `o
| Computer Science,  Micha? "mina86" Nazarewicz       (o o)
+----[mina86*mina86.com]---[mina86*jabber.org]----ooO--(_)--Ooo--


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/