On Thu, 2008-08-28 at 17:04 +0200, Arnd Bergmann wrote:
> On Thursday 28 August 2008, Li Yang wrote:
> > 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:
>
> > +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.
>
> > +#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
> ...
> > +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.
>
> > +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.
>
> > +
> > + 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
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
