Re: [PATCH] USB device driver of Topcliff PCH

["Masayuki Ohtake" <masa-korg@dsn.okisemi.com>]

Hi Michal

My reply comments are included in the following.

Thanks Ohtake

Date: Wed, 08 Sep 2010 03:58:04 +0200
From: "Micha? Nazarewicz" <m.nazarewicz@samsung.com>
>Not sure why I'm on the "To" list, but here are a few of my comments:

[masa]
Your address was got the following scripts as maintainer.
"./scripts/get_maintainer.pl"
Whom should I submit patch to?


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

[masa]
This will be modified.


>> + * @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.

[masa]
This will be modified as
"static void pch_udc_write_csr(unsigned long val, unsigned long addr)"


>> +{
>> + 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.

[masa]
This will be modified.


>> +
>> + 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)?

[masa]
This will be modified.

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

[masa]
This will be modified.

>> +/**
>> + * 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.

[masa]
This will be modified.


>> +/**
>> + * 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;
>> +}

[masa]
This will be modified.


>> +/**
>> + * 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.

[masa]
This will be modified.


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

[masa]
This will be modified.

>> + } 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.

[masa]
This will be modified.


>> +/**
>> + * 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_*"?

[masa]
This will be modified.


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

[masa] ???

>> +/**
>> + * 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.

[masa]
This will be modified.

>> + /* next request */
>> + req = list_entry(ep->queue.next, struct pch_udc_request, queue);
>> + if (req && !req->dma_going) {
>
>Same here.

[masa]
This will be modified.

>> + pr_debug("%s: Set request: req=%p req->td_data=%p",
>> + __func__, req, req->td_data);
>> + if (req->td_data) {
>
>Same eher.

[masa]
This will be modified.

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

[masa]
This will be modified.

>> +static void pch_udc_complete_transfer(struct pch_udc_ep *ep)
>
>Same as with the function above.

[masa]
This will be modified.

>> +/**
>> + * 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.

[masa]
This will be modified.

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

[masa]
This will be deleted.


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

[masa]
This will be deleted.


>> +/**
>> + * 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.

[masa]
This will be modified as
"void pch_udc_svc_enum_interrupt(struct pch_udc_dev *dev)"


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

[masa]
This will be modified.

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

[masa]
This will be deleted.

>> + /* 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.

[masa]
This will be deleted.

>> +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);

[masa]
This will be modeified.

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

[masa]
This will be deleted.

>> +/**
>> + * 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 - ...

[masa]
This will be modeified.

>> + * @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.

[masa]
This will be modeified.


>> +/**
>> + * 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 ",".

[masa]
This will be modeified.

>> +
>
>Needless empty line.
>
>> +struct pch_udc_dev {

[masa]
This will be modeified.

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