On Thu, 2008-08-28 at 17:04 +0200, Arnd Bergmann wrote:

On Thursday 28 August 2008, Li Yang wrote:

quoted

Some of Freescale SoC chips have a QE or CPM co-processor which
supports full speed USB.  The driver adds device mode support
of both QE and CPM USB controller to Linux USB gadget.  The
driver is tested with MPC8360 and MPC8272, and should work with
other models having QE/CPM given minor tweaks.

Looks pretty good, just a few comments on the driver:
  

quoted

+config USB_GADGET_FSL_QE
+	boolean "Freescale QE/CPM USB Device Controller"
+	depends on FSL_SOC && (QUICC_ENGINE || CPM)
+	help
+	   Some of Freescale PowerPC processors have a Full Speed
+	   QE/CPM2 USB controller, which support device mode with 4
+	   programmable endpoints. This driver supports the
+	   controller in the MPC8360 and MPC8272, and should work with
+	   controllers having QE or CPM2, given minor tweaks.
+
+	   Say "y" to link the driver statically, or "m" to build a
+	   dynamically linked module called "fsl_qe_udc" and force all
+	   gadget drivers to also be dynamically linked.
+
+config USB_FSL_QE
+	tristate
+	depends on USB_GADGET_FSL_QE
+	default USB_GADGET
+	select USB_GADGET_SELECTED

Why do you need the two config options, not just one?

This is common for udc drivers.  I guess this measure is used to make
the selection of udc drivers a choice list while still make it possible
to compiled as module.

quoted

+#ifdef CONFIG_CPM2
+#include <asm/cpm.h>
+
+#define qe_muram_addr cpm_muram_addr
+#define qe_muram_offset cpm_muram_offset
+#define qe_muram_alloc cpm_muram_alloc
+#define qe_muram_free cpm_muram_free
+#endif

...

quoted

+static int qe_ep_cmd_restarttx(struct qe_ep *ep)
+{
+	u8 ep_num;
+#ifdef CONFIG_CPM2
+	u32 command;
+	u8 opcode;
+
+	ep_num = ep->epnum << CPM_USB_EP_SHIFT;
+	command = CPM_USB_RESTART_TX | (u32)ep_num;
+	opcode = CPM_USB_RESTART_TX_OPCODE;
+	cpm_command(command, opcode);
+#else
+	ep_num = ep->epnum;
+	qe_issue_cmd(QE_USB_RESTART_TX, QE_CR_SUBBLOCK_USB, ep_num, 0);
+#endif
+	return 0;
+}

This part doesn't look good, you should try to avoid hardcoding
the specific type of chip (QE or CPM2) here. AFAICT, you can build
a multiplatform kernel that supports both QE and CPM2, but your code
here would be broken in that case if you try to run it on QE.

Ok.

quoted

+static void setup_received_handle(struct qe_udc *udc,
+					struct usb_ctrlrequest *setup);
+static int qe_ep_rxframe_handle(struct qe_ep *ep);
+static void ep0_req_complete(struct qe_udc *udc, struct qe_req *req);

Better try to avoid static forward declarations by reordering your
functions in call order. That is the common coding style and makes
drivers easier to read when you're used to it.

quoted

+
+	tasklet_schedule(&udc->rx_tasklet);

Not a problem, but an observation: Most new code uses work queues instead
of tasklets these days, which gives you more predictable real time
latencies.
If you don't have a specific reason to prefer a tasklet, just use
a workqueue here.

Is this truly a trend?  Work queue is more flexible but it has higher
latency.  Why are work queues preferred?

- Leo