Re: [PATCH] USB device driver of Topcliff PCH

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

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@dsn.okisemi.com> 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--