Re: [PATCH 7/7] staging: kpc2000: fix incorrect code comment in core.c

[Dan Carpenter]

On Tue, Jun 04, 2019 at 12:29:16AM +0200, Simon Sandström wrote:
> Step 11 was removed from kp2000_pcie_probe in a previous commit but the
> comment was not changed to reflect this, so do it now.
> 
> Signed-off-by: Simon Sandström <simon@xxxxxxxxxx>
> ---
>  drivers/staging/kpc2000/kpc2000/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/kpc2000/kpc2000/core.c b/drivers/staging/kpc2000/kpc2000/core.c
> index 2d8d188624f7..cd3876f1ce17 100644
> --- a/drivers/staging/kpc2000/kpc2000/core.c
> +++ b/drivers/staging/kpc2000/kpc2000/core.c
> @@ -501,7 +501,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
>  		goto out10;
>  
>  	/*
> -	 * Step 12: Enable IRQs in HW
> +	 * Step 11: Enable IRQs in HW

I don't have a problem with this patch but for the future these numbers
don't add any value.  And the numbered out labels are sort of ugly.  The
label name should say what the label does just like a function name says
what the function does.  Really a lot of these comments in the probe
function are very obvious and don't add information (delete them).


   491          /*
   492           * Step 9: Setup sysfs attributes
   493           */
   494          err = sysfs_create_files(&(pdev->dev.kobj), kp_attr_list);

The comment is probably less informative than the code.

   495          if (err) {
   496                  dev_err(&pdev->dev, "Failed to add sysfs files: %d\n", err);
   497                  goto out9;

What does goto out9 do?

   498          }
   499  
   500          /*
   501           * Step 10: Probe cores
   502           */
   503          err = kp2000_probe_cores(pcard);
   504          if (err)
   505                  goto out10;

Hopefully, goto out10 deletes the sysfs files but we don't know because
the label doesn't give any clues away.  We have to search for it and
then come back.

   506  
   507          /*
   508           * Step 12: Enable IRQs in HW
   509           */
   510          writel(KPC_DMA_CARD_IRQ_ENABLE | KPC_DMA_CARD_USER_INTERRUPT_MODE,
   511                 pcard->dma_common_regs);
   512  
   513          dev_dbg(&pcard->pdev->dev, "kp2000_pcie_probe() complete!\n");
   514          mutex_unlock(&pcard->sem);
   515          return 0;
   516  
   517  out10:

err_remove_sysfs:

   518          sysfs_remove_files(&(pdev->dev.kobj), kp_attr_list);
   519  out9:

err_free_irq:

   520          free_irq(pcard->pdev->irq, pcard);
   521  out8b:

err_disable_msi:

   522          pci_disable_msi(pcard->pdev);
   523  out8a:
   524  out7:
   525  out6:

err_unmap_dma:

   526          iounmap(pcard->dma_bar_base);
   527          pci_release_region(pdev, DMA_BAR);
   528          pcard->dma_bar_base = NULL;
   529  out5:

err_unmap_regs:

   530          iounmap(pcard->regs_bar_base);
   531          pci_release_region(pdev, REG_BAR);
   532          pcard->regs_bar_base = NULL;

Something like that is way more useful because then you don't have to
scroll back and forth because new the label names are useful.

regards,
dan carpenter