Re: [PATCH v4] USB device driver of Topcliff PCH

["Michał Nazarewicz" <m.nazarewicz@samsung.com>]

On Fri, 24 Sep 2010 14:12:21 +0200, Masayuki Ohtake <masa-korg@dsn.okisemi.com> wrote:
> Hi Maurus and Michal.
>
> Thank you for your comments.
>
> This patch was modified.
>   Add MODULE_AUTHOR()
>   Modify struct pch_udc_stp_dma_desc.
>   Delete union pch_udc_setup_data.
>   Modify as only one "unlock"
>   Modify while loop.
>   Modify loop count.
>   Refactored function
>    -pch_udc_free_dma_chain()
>    -pch_udc_create_dma_chain()
>  and so on

As Maurus commented before, this is not a proper commit message.  Describe
what the patch does.  All other comments should go after the "---" marker.


> diff --git a/drivers/usb/gadget/pch_udc.c b/drivers/usb/gadget/pch_udc.c
> +static inline void pch_udc_writel(struct pch_udc_dev *dev,
> +				    unsigned long val, unsigned long reg)
> +{
> +	iowrite32(val, dev->base_addr + reg);
> +}

Empty line please.

> +static inline void pch_udc_bit_set(struct pch_udc_dev *dev,
> +				     unsigned long reg,
> +				     unsigned long bitmask)
> +{
> +	pch_udc_writel((dev), pch_udc_readl((dev), (reg)) | (bitmask), (reg));
> +}
> +static inline void pch_udc_bit_clr(struct pch_udc_dev *dev,
> +				     unsigned long reg,
> +				     unsigned long bitmask)
> +{
> +	pch_udc_writel((dev), pch_udc_readl((dev), (reg)) & ~(bitmask), (reg));
> +}

Definitely too many parenthesis here.  It's no longer a macro so parens around
arguments are not needed and only clutter the code.

> +
> +static inline u32 pch_udc_ep_readl(struct pch_udc_ep *ep, unsigned long reg)
> +{
> +	return ioread32(ep->dev->base_addr + ep->offset_addr + reg);
> +}
> +
> +static inline void pch_udc_ep_writel(struct pch_udc_ep *ep,
> +				    unsigned long val, unsigned long reg)
> +{
> +	iowrite32(val, ep->dev->base_addr + ep->offset_addr + reg);
> +}

Again, empty line please.

> +static inline void pch_udc_ep_bit_set(struct pch_udc_ep *ep,
> +				     unsigned long reg,
> +				     unsigned long bitmask)
> +{
> +	pch_udc_ep_writel((ep), pch_udc_ep_readl((ep), (reg)) | (bitmask),
> +			  (reg));
> +}
> +static inline void pch_udc_ep_bit_clr(struct pch_udc_ep *ep,
> +				     unsigned long reg,
> +				     unsigned long bitmask)
> +{
> +	pch_udc_ep_writel((ep), pch_udc_ep_readl((ep), (reg)) & ~(bitmask),
> +			  (reg));
> +}

And again, too many parenthesis.

> +static void process_zlp(struct pch_udc_ep *ep, struct pch_udc_request *req)
> +{
> +	struct pch_udc_dev	*dev = ep->dev;
> +
> +	dev_dbg(&dev->pdev->dev, "%s: enter  ep%d%s", __func__, ep->num,
> +		(ep->in ? "in" : "out"));
> +	/* IN zlp's are handled by hardware */
> +	complete_req(ep, req, 0);
> +
> +	/* if set_config or set_intf is waiting for ack by zlp
> +	 * then set CSR_DONE
> +	 */
> +	if (dev->set_cfg_not_acked) {
> +		dev_dbg(&dev->pdev->dev, "%s: process_zlp: csr done", __func__);
> +		pch_udc_set_csr_done(dev);
> +		dev->set_cfg_not_acked = 0;
> +	}
> +	/* setup command is ACK'ed now by zlp */
> +	if ((!dev->stall) && (dev->waiting_zlp_ack)) {

Useless parenthesis.

> +		/* clear NAK by writing CNAK in EP0_IN */
> +		pch_udc_ep_clear_nak(&(dev->ep[UDC_EP0IN_IDX]));
> +		dev->waiting_zlp_ack = 0;
> +	}
> +}


> +/**
> + * pch_udc_alloc_request() - This function allocates request structure.
> + *				It iscalled by gadget driver

"is called" -- typo.

> + * @usbep:	Reference to the USB endpoint structure
> + * @gfp:	Flag to be used while allocating memory
> + * Returns
> + *	NULL:			Failure
> + *	Allocated address:	Success
> + */


Other then those trivial mistakes, I haven't noticed any obvious
problems so I guess "Acked-by" me.

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