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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
