Message-ID: <20101103183750.GA23043@hexapodia.org>
Date:	Wed, 3 Nov 2010 11:37:50 -0700
From:	Andy Isaacson <adi@...apodia.org>
To:	Toshiharu Okada <toshiharu-linux@....okisemi.com>
Cc:	Greg Kroah-Hartman <gregkh@...e.de>,
	Michal Nazarewicz <m.nazarewicz@...sung.com>,
	LKML <linux-kernel@...r.kernel.org>,
	linux-usb <linux-usb@...r.kernel.org>,
	"Wang, Qi" <qi.wang@...el.com>,
	"Wang, Yong Y" <yong.y.wang@...el.com>,
	Andrew <andrew.chih.howe.khor@...el.com>,
	Intel OTC <joel.clark@...el.com>,
	"Foster, Margie" <margie.foster@...el.com>,
	"Ewe, Kok Howg" <kok.howg.ewe@...el.com>
Subject: Re: [PATCH v7] USB device driver of Topcliff PCH

On Mon, Oct 25, 2010 at 07:24:25PM +0900, Toshiharu Okada wrote:
> This patch adds the USB device driver of EG20T PCH.
> 
> EG20T PCH is the platform controller hub that is going to be used in
> Intel's upcoming general embedded platform. All IO peripherals in
> EG20T PCH are actually devices sitting on AMBA bus. 
> EG20T PCH has USB device I/F. Using this I/F, it is able to access system
> devices connected to USB device.
> 
> +config USB_GADGET_PCH
> +	boolean "PCH USB Device controller"

This description is way too generic.  "Intel EG20T (Topcliff) USB Device
controller" would be better.  The config option should probably be
USB_GADGET_EG20T as well.

> +	  EG20T PCH is the platform controller hub that is used in Intel's
> +	  general embedded platform. EG20T PCH has USB device interface.
> +	  Using this interface, it is able to access system devices connected
> +	  to USB device.
> +	  This driver enables USB device function.
> +	  USB device is a USB peripheral controller which
> +	  supports both full and high speed USB 2.0 data transfers.
> +	  This driver supports for control transfer, and bulk transfer modes.

"This driver supports both control transfer and bulk transfer modes."

> +	  This driver dose not support interrupt transfer, and isochronous
> +	  transfer modes.

"This driver does not support interrupt transfer or isochronous transfer
modes."

> +config USB_PCH
> +	tristate

Does this option need a description string?

> +++ b/drivers/usb/gadget/pch_udc.c
[snip]
> +#include <linux/types.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/kernel.h>
> +#include <linux/delay.h>
> +#include <linux/ioport.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/smp_lock.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/timer.h>
> +#include <linux/list.h>
> +#include <linux/interrupt.h>
> +#include <linux/ioctl.h>
> +#include <linux/fs.h>
> +#include <linux/dmapool.h>
> +#include <linux/moduleparam.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>

Do you really need all of these includes?

> +#define UDC_DEVSTS_TS_OFS		18
> +#define UDC_DEVSTS_ENUM_SPEED_OFS	13
> +#define UDC_DEVSTS_ALT_OFS		8
> +#define UDC_DEVSTS_INTF_OFS		4
> +#define UDC_DEVSTS_CFG_OFS		0

Please rename all _OFS constants to _SHIFT for clarity.  (However, if
the rename will cause many lines to exceed 80 characters, then please
choose another name.)

> +static void pch_udc_write_csr(struct pch_udc_dev *dev, unsigned long val,
> +			       unsigned int ep)
> +{
> +	unsigned long reg = PCH_UDC_CSR(ep);

Add a blank line between variables and code.

> +	pch_udc_csr_busy(dev);		/* Wait till idle */
> +	pch_udc_writel(dev, val, reg);


> +/**
> + * pch_udc_read_csr() - Read the command and status registers.
> + * @dev:	Reference to pch_udc_dev structure
> + * @addr:	address of CSR register
> + * Returns
> + *	content of CSR register
> + */

Please read Documentation/kernel-doc-nano-HOWTO.txt and verify this is
correct.  I am not sure, but I think you need a blank line after the
last @variable, and should say "Return value:".

> +static u32 pch_udc_read_csr(struct pch_udc_dev *dev, unsigned int ep)
> +{
> +	unsigned long reg = PCH_UDC_CSR(ep);

Please check all your functions and ensure there is a proper blank line
here.

> +	pch_udc_csr_busy(dev);		/* Wait till idle */
> +	pch_udc_readl(dev, reg);	/* Dummy read */
> +	pch_udc_csr_busy(dev);		/* Wait till idle */


> +/**
> + * pch_udc_clear_dma() - Clear the 'TDE' or RDE bit of device control
> + *				 register depending on the direction specified
> + * @dev:	Reference to structure of type pch_udc_regs
> + * @dir:	Whether Tx or Rx
> + *		- dir = DMA_DIR_RX Receive
> + *		- dir = DMA_DIR_TX Transmit
> + */

This will not format correctly, please read nano-howto.txt.

> +static inline void pch_udc_clear_dma(struct pch_udc_dev *dev, int dir)
> +{
> +	if (dir == DMA_DIR_RX)
> +		pch_udc_bit_clr(dev, UDC_DEVCTL_ADDR, UDC_DEVCTL_RDE);
> +	else if (dir == DMA_DIR_TX)
> +		pch_udc_bit_clr(dev, UDC_DEVCTL_ADDR, UDC_DEVCTL_TDE);
> +

Delete this extra blank line.  Check for unneeded blank lines
throughout.

> +}


> +	pr_debug("%s: enter  req->req.dma=0x%08x", __func__, req->req.dma);
> +	/* set buffer pointer */
> +	req->td_data->dataptr = req->req.dma;
> +	/* set last bit */

These comments are useless, because they just repeat what the code says.
Please remove them or replace them with comments that actually explain
what is happening.  For example, this line could use a comment:

> +	req->td_data->status |= PCH_UDC_DMA_LAST;

*Why* are we setting DMA_LAST?  (I *think* I know, but am not entirely
sure.)  Probably it doesn't need a comment at all, but if it does, the
comment should answer the "Why?" question.

You have many examples of this pattern of one-comment-per-line-of-code.
Please review them all.

[snip]

> +	/* if bytes < max packet then tx bytes must
> +	 * be written in packet per buffer mode
> +	 */

This is a good comment!

> +	if ((req->req.length < ep->ep.maxpacket) || !ep->num)
> +		/* write the count */

This is a bad comment.

> +		req->td_data->status = (req->td_data->status &
> +					~PCH_UDC_RXTX_BYTES) | req->req.length;

[snip]

> +static void pch_udc_start_rxrequest(struct pch_udc_ep *ep,
> +					 struct pch_udc_request *req)
> +{
> +	struct pch_udc_data_dma_desc *td_data;
> +
> +	pch_udc_clear_dma(ep->dev, DMA_DIR_RX);
> +	td_data = req->td_data;
> +	ep->td_data = req->td_data;
> +	/* Set the status bits for all descriptors */

Here is an example of a one-line comment that is not bad, since it
explains why the loop is here.  So in your review, be careful not to
remove good comments like this...

> +	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;
> +		td_data = phys_to_virt(td_data->next);
> +	}
> +	/* Write the descriptor pointer */
> +	pch_udc_ep_set_ddptr(ep, req->td_data_phys);

Suprisingly, this comment is good too, I think, since it helps explain
the effect of set_ddptr.

> +	dev_dbg(&dev->pdev->dev, "%s: req = 0x%p dma_desc = 0x%p, td_phys = "
> +		"0x%08lx", __func__, req, dma_desc,
> +		(unsigned long)req->td_data_phys);
> +	/* prevent from using desc. - set HOST BUSY */
> +	dma_desc->status |= PCH_UDC_BS_HST_BSY;

This comment should explain what is being prevented from using desc, I
think it's the USB controller but the comment should explain clearly.

> +static int pch_udc_pcd_set_halt(struct usb_ep *usbep, int halt)
> +{
[snip]
> +	if (!list_empty(&ep->queue)) {
> +		dev_dbg(&dev->pdev->dev, "%s: list not empty", __func__);
> +		ret = -EAGAIN;
> +	} else if (!halt) {	/* halt or clear halt */
> +		pch_udc_ep_clear_stall(ep);
> +		ret = 0;
> +	} else {
> +		if (ep->num == PCH_UDC_EP0)
> +			ep->dev->stall = 1;
> +		pch_udc_ep_set_stall(ep);
> +		pch_udc_enable_ep_interrupts(ep->dev,
> +					     PCH_UDC_EPINT(ep->in, ep->num));
> +		ret = 0;
> +	}

I think this would be clearer as

+	if (!list_empty(&ep->queue)) {
+		dev_dbg(&dev->pdev->dev, "%s: list not empty", __func__);
+		ret = -EAGAIN;
+	}
+	if (halt) {
+		if (ep->num == PCH_UDC_EP0)
+			ep->dev->stall = 1;
+		pch_udc_ep_set_stall(ep);
+		pch_udc_enable_ep_interrupts(ep->dev,
+					     PCH_UDC_EPINT(ep->in, ep->num));
+		ret = 0;
+	} else {
+		pch_udc_ep_clear_stall(ep);
+		ret = 0;
+	}

because:
1. if (!list_empty  is a standalone check, there is no if/else if/else
connection between list_empty() and halt.
2. I prefer if (foo) {} else {} instead of if (!foo) {} else {}, unless
there is a significant reason to do the negated test.

> +	struct pch_udc_ep  *ep ;

random extra whitespace before ;.

> +	pr_debug("%s: %s", __func__, usbep->name);

There are probably too many pr_debug() and dev_dbg()s in this driver.
Please reconsider which ones are appropriate for mainline.

> +#ifdef DMA_PPB_WITH_DESC_UPDATE

I don't see this defined anywhere, is this dead code?

> +	} else if ((((stat & UDC_EPSTS_OUT_MASK) >> UDC_EPSTS_OUT_OFS) ==
> +		     UDC_EPSTS_OUT_DATA) && !dev->stall) {
> +		if (list_empty(&ep->queue)) {
> +			dev_err(&dev->pdev->dev, "%s: ZLP", __func__);

Please provide a useful error message here.

> +	for (i = 0; i < PCH_UDC_EP_NUM; i++) {
> +		ep = &dev->ep[i];
> +		pch_udc_clear_ep_status(ep, 0x1F0006F0);

This magic number needs a #define.


Overall, the code cleanliness looks OK once the above are addressed.
I am not qualified to review the USB gadget interfaces, but the general
structure appears to be sane.

-andy
--
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