These two patches fixes issues pointed out by Dan in a previous
staging/kpc2000 patch thread: many comments in kp2000_pcie_probe just
repeats the code and the current label names doesn't add any information
and makes it hard to follow the code.
Rename all labels and remove the comments that just repeats the code.
Simon Sandström (2):
staging: kpc2000: improve label names in kp2000_pcie_probe
staging: kpc2000: remove unnecessary comments in kp2000_pcie_probe
drivers/staging/kpc2000/kpc2000/core.c | 80 ++++++++------------------
1 file changed, 25 insertions(+), 55 deletions(-)
--
2.20.1
Much of the code comments in kp2000_pcie_probe just repeats the code and
does not add any additional information. Delete them and make sure that
comments still left in the function all use the same style.
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/kpc2000/kpc2000/core.c | 38 ++++----------------------
1 file changed, 5 insertions(+), 33 deletions(-)
diff --git a/drivers/staging/kpc2000/kpc2000/core.c b/drivers/staging/kpc2000/kpc2000/core.c
index ee6b9be7127d..7ec54b672c20 100644
--- a/drivers/staging/kpc2000/kpc2000/core.c
+++ b/drivers/staging/kpc2000/kpc2000/core.c
@@ -311,18 +311,12 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
unsigned long dma_bar_phys_len;
u16 regval;
- /*
- * Step 1: Allocate a struct for the pcard
- */
pcard = kzalloc(sizeof(*pcard), GFP_KERNEL);
if (!pcard)
return -ENOMEM;
dev_dbg(&pdev->dev, "probe: allocated struct kp2000_device @ %p\n",
pcard);
- /*
- * Step 2: Initialize trivial pcard elements
- */
err = ida_simple_get(&card_num_ida, 1, INT_MAX, GFP_KERNEL);
if (err < 0) {
dev_err(&pdev->dev, "probe: failed to get card number (%d)\n",
@@ -338,9 +332,6 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
pcard->pdev = pdev;
pci_set_drvdata(pdev, pcard);
- /*
- * Step 3: Enable PCI device
- */
err = pci_enable_device(pcard->pdev);
if (err) {
dev_err(&pcard->pdev->dev,
@@ -349,9 +340,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
goto err_remove_ida;
}
- /*
- * Step 4: Setup the Register BAR
- */
+ // Setup the Register BAR
reg_bar_phys_addr = pci_resource_start(pcard->pdev, REG_BAR);
reg_bar_phys_len = pci_resource_len(pcard->pdev, REG_BAR);
@@ -381,9 +370,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
reg_bar_phys_len - 1;
pcard->regs_base_resource.flags = IORESOURCE_MEM;
- /*
- * Step 5: Setup the DMA BAR
- */
+ // Setup the DMA BAR
dma_bar_phys_addr = pci_resource_start(pcard->pdev, DMA_BAR);
dma_bar_phys_len = pci_resource_len(pcard->pdev, DMA_BAR);
@@ -415,9 +402,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
dma_bar_phys_len - 1;
pcard->dma_base_resource.flags = IORESOURCE_MEM;
- /*
- * Step 6: System Regs
- */
+ // Read System Regs
pcard->sysinfo_regs_base = pcard->regs_bar_base;
err = read_system_regs(pcard);
if (err)
@@ -427,11 +412,9 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
writeq(0xFFFFFFFFFFFFFFFF,
pcard->sysinfo_regs_base + REG_INTERRUPT_MASK);
- /*
- * Step 7: Configure PCI thingies
- */
// let the card master PCIe
pci_set_master(pcard->pdev);
+
// enable IO and mem if not already done
pci_read_config_word(pcard->pdev, PCI_COMMAND, ®val);
regval |= (PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
@@ -466,9 +449,6 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
dev_dbg(&pcard->pdev->dev,
"Using DMA mask %0llx\n", dma_get_mask(PCARD_TO_DEV(pcard)));
- /*
- * Step 8: Configure IRQs
- */
err = pci_enable_msi(pcard->pdev);
if (err < 0)
goto err_unmap_dma;
@@ -481,25 +461,17 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
goto err_disable_msi;
}
- /*
- * Step 9: Setup sysfs attributes
- */
err = sysfs_create_files(&pdev->dev.kobj, kp_attr_list);
if (err) {
dev_err(&pdev->dev, "Failed to add sysfs files: %d\n", err);
goto err_free_irq;
}
- /*
- * Step 10: Probe cores
- */
err = kp2000_probe_cores(pcard);
if (err)
goto err_remove_sysfs;
- /*
- * Step 11: Enable IRQs in HW
- */
+ // Enable IRQs in HW
writel(KPC_DMA_CARD_IRQ_ENABLE | KPC_DMA_CARD_USER_INTERRUPT_MODE,
pcard->dma_common_regs);
--
2.20.1
On Mon, Jun 10, 2019 at 10:05:35PM +0200, Simon Sandstr?m wrote:
> @@ -349,9 +340,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
> goto err_remove_ida;
> }
>
> - /*
> - * Step 4: Setup the Register BAR
> - */
> + // Setup the Register BAR
Greg, are we moving the C++ style comments? Linus is fine with them. I
don't like them but whatever...
> reg_bar_phys_addr = pci_resource_start(pcard->pdev, REG_BAR);
> reg_bar_phys_len = pci_resource_len(pcard->pdev, REG_BAR);
>
regards,
dan carpenter
On Wed, Jun 12, 2019 at 10:39:36AM +0300, Dan Carpenter wrote:
> On Mon, Jun 10, 2019 at 10:05:35PM +0200, Simon Sandstr?m wrote:
> > @@ -349,9 +340,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
> > goto err_remove_ida;
> > }
> >
> > - /*
> > - * Step 4: Setup the Register BAR
> > - */
> > + // Setup the Register BAR
>
> Greg, are we moving the C++ style comments? Linus is fine with them. I
> don't like them but whatever...
I don't like them either. I'm only "ok" with them on the very first
line of the file. Linus chose // to make it "stand out" from the normal
flow of the file, which is fine for an SPDX line. So putting these in
here like this is not ok to me.
thanks,
greg k-h
On 12/06, Greg KH wrote:
> On Wed, Jun 12, 2019 at 10:39:36AM +0300, Dan Carpenter wrote:
> > On Mon, Jun 10, 2019 at 10:05:35PM +0200, Simon Sandström wrote:
> > > @@ -349,9 +340,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
> > > goto err_remove_ida;
> > > }
> > >
> > > - /*
> > > - * Step 4: Setup the Register BAR
> > > - */
> > > + // Setup the Register BAR
> >
> > Greg, are we moving the C++ style comments? Linus is fine with them. I
> > don't like them but whatever...
>
> I don't like them either. I'm only "ok" with them on the very first
> line of the file. Linus chose // to make it "stand out" from the normal
> flow of the file, which is fine for an SPDX line. So putting these in
> here like this is not ok to me.
>
> thanks,
>
> greg k-h
I changed them to C++ style so that they would match the other comments
in the file, which are C++ style, but I guess that it should have been
done the other way around with the C++ style changed to C style.
Good to know. I'll change them back and send a v2.
- Simon
Use self-explanatory label names instead of the generic numbered ones,
to make it easier to follow and understand the code.
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/kpc2000/kpc2000/core.c | 42 ++++++++++++--------------
1 file changed, 20 insertions(+), 22 deletions(-)
diff --git a/drivers/staging/kpc2000/kpc2000/core.c b/drivers/staging/kpc2000/kpc2000/core.c
index 9b9b29ac90c5..ee6b9be7127d 100644
--- a/drivers/staging/kpc2000/kpc2000/core.c
+++ b/drivers/staging/kpc2000/kpc2000/core.c
@@ -327,7 +327,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
if (err < 0) {
dev_err(&pdev->dev, "probe: failed to get card number (%d)\n",
err);
- goto out2;
+ goto err_free_pcard;
}
pcard->card_num = err;
scnprintf(pcard->name, 16, "kpcard%u", pcard->card_num);
@@ -346,7 +346,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
dev_err(&pcard->pdev->dev,
"probe: failed to enable PCIE2000 PCIe device (%d)\n",
err);
- goto out3;
+ goto err_remove_ida;
}
/*
@@ -360,7 +360,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
dev_err(&pcard->pdev->dev,
"probe: REG_BAR could not remap memory to virtual space\n");
err = -ENODEV;
- goto out4;
+ goto err_disable_device;
}
dev_dbg(&pcard->pdev->dev,
"probe: REG_BAR virt hardware address start [%p]\n",
@@ -373,7 +373,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
"probe: failed to acquire PCI region (%d)\n",
err);
err = -ENODEV;
- goto out4;
+ goto err_disable_device;
}
pcard->regs_base_resource.start = reg_bar_phys_addr;
@@ -393,7 +393,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
dev_err(&pcard->pdev->dev,
"probe: DMA_BAR could not remap memory to virtual space\n");
err = -ENODEV;
- goto out5;
+ goto err_unmap_regs;
}
dev_dbg(&pcard->pdev->dev,
"probe: DMA_BAR virt hardware address start [%p]\n",
@@ -407,7 +407,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
dev_err(&pcard->pdev->dev,
"probe: failed to acquire PCI region (%d)\n", err);
err = -ENODEV;
- goto out5;
+ goto err_unmap_regs;
}
pcard->dma_base_resource.start = dma_bar_phys_addr;
@@ -421,7 +421,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
pcard->sysinfo_regs_base = pcard->regs_bar_base;
err = read_system_regs(pcard);
if (err)
- goto out6;
+ goto err_unmap_dma;
// Disable all "user" interrupts because they're not used yet.
writeq(0xFFFFFFFFFFFFFFFF,
@@ -461,7 +461,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
if (err) {
dev_err(&pcard->pdev->dev,
"CANNOT use DMA mask %0llx\n", DMA_BIT_MASK(64));
- goto out7;
+ goto err_unmap_dma;
}
dev_dbg(&pcard->pdev->dev,
"Using DMA mask %0llx\n", dma_get_mask(PCARD_TO_DEV(pcard)));
@@ -471,14 +471,14 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
*/
err = pci_enable_msi(pcard->pdev);
if (err < 0)
- goto out8a;
+ goto err_unmap_dma;
rv = request_irq(pcard->pdev->irq, kp2000_irq_handler, IRQF_SHARED,
pcard->name, pcard);
if (rv) {
dev_err(&pcard->pdev->dev,
"%s: failed to request_irq: %d\n", __func__, rv);
- goto out8b;
+ goto err_disable_msi;
}
/*
@@ -487,7 +487,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
err = sysfs_create_files(&pdev->dev.kobj, kp_attr_list);
if (err) {
dev_err(&pdev->dev, "Failed to add sysfs files: %d\n", err);
- goto out9;
+ goto err_free_irq;
}
/*
@@ -495,7 +495,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
*/
err = kp2000_probe_cores(pcard);
if (err)
- goto out10;
+ goto err_remove_sysfs;
/*
* Step 11: Enable IRQs in HW
@@ -506,28 +506,26 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
mutex_unlock(&pcard->sem);
return 0;
-out10:
+err_remove_sysfs:
sysfs_remove_files(&pdev->dev.kobj, kp_attr_list);
-out9:
+err_free_irq:
free_irq(pcard->pdev->irq, pcard);
-out8b:
+err_disable_msi:
pci_disable_msi(pcard->pdev);
-out8a:
-out7:
-out6:
+err_unmap_dma:
iounmap(pcard->dma_bar_base);
pci_release_region(pdev, DMA_BAR);
pcard->dma_bar_base = NULL;
-out5:
+err_unmap_regs:
iounmap(pcard->regs_bar_base);
pci_release_region(pdev, REG_BAR);
pcard->regs_bar_base = NULL;
-out4:
+err_disable_device:
pci_disable_device(pcard->pdev);
-out3:
+err_remove_ida:
mutex_unlock(&pcard->sem);
ida_simple_remove(&card_num_ida, pcard->card_num);
-out2:
+err_free_pcard:
kfree(pcard);
return err;
}
--
2.20.1
These two patches fixes issues pointed out by Dan in a previous
staging/kpc2000 patch thread: many comments in kp2000_pcie_probe just
repeats the code and the current label names doesn't add any information
and makes it hard to follow the code.
Rename all labels and remove the comments that just repeats the code.
Version 2:
- Don't convert C style comments to C++ style
Simon Sandström (2):
staging: kpc2000: improve label names in kp2000_pcie_probe
staging: kpc2000: remove unnecessary comments in kp2000_pcie_probe
drivers/staging/kpc2000/kpc2000/core.c | 80 ++++++++------------------
1 file changed, 25 insertions(+), 55 deletions(-)
--
2.20.1
Much of the code comments in kp2000_pcie_probe just repeats the code and
does not add any additional information. Delete them and make sure that
comments still left in the function all use the same style.
Signed-off-by: Simon Sandström <[email protected]>
---
drivers/staging/kpc2000/kpc2000/core.c | 38 ++++----------------------
1 file changed, 5 insertions(+), 33 deletions(-)
diff --git a/drivers/staging/kpc2000/kpc2000/core.c b/drivers/staging/kpc2000/kpc2000/core.c
index ee6b9be7127d..6a5999e8ff4e 100644
--- a/drivers/staging/kpc2000/kpc2000/core.c
+++ b/drivers/staging/kpc2000/kpc2000/core.c
@@ -311,18 +311,12 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
unsigned long dma_bar_phys_len;
u16 regval;
- /*
- * Step 1: Allocate a struct for the pcard
- */
pcard = kzalloc(sizeof(*pcard), GFP_KERNEL);
if (!pcard)
return -ENOMEM;
dev_dbg(&pdev->dev, "probe: allocated struct kp2000_device @ %p\n",
pcard);
- /*
- * Step 2: Initialize trivial pcard elements
- */
err = ida_simple_get(&card_num_ida, 1, INT_MAX, GFP_KERNEL);
if (err < 0) {
dev_err(&pdev->dev, "probe: failed to get card number (%d)\n",
@@ -338,9 +332,6 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
pcard->pdev = pdev;
pci_set_drvdata(pdev, pcard);
- /*
- * Step 3: Enable PCI device
- */
err = pci_enable_device(pcard->pdev);
if (err) {
dev_err(&pcard->pdev->dev,
@@ -349,9 +340,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
goto err_remove_ida;
}
- /*
- * Step 4: Setup the Register BAR
- */
+ /* Setup the Register BAR */
reg_bar_phys_addr = pci_resource_start(pcard->pdev, REG_BAR);
reg_bar_phys_len = pci_resource_len(pcard->pdev, REG_BAR);
@@ -381,9 +370,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
reg_bar_phys_len - 1;
pcard->regs_base_resource.flags = IORESOURCE_MEM;
- /*
- * Step 5: Setup the DMA BAR
- */
+ /* Setup the DMA BAR */
dma_bar_phys_addr = pci_resource_start(pcard->pdev, DMA_BAR);
dma_bar_phys_len = pci_resource_len(pcard->pdev, DMA_BAR);
@@ -415,9 +402,7 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
dma_bar_phys_len - 1;
pcard->dma_base_resource.flags = IORESOURCE_MEM;
- /*
- * Step 6: System Regs
- */
+ /* Read System Regs */
pcard->sysinfo_regs_base = pcard->regs_bar_base;
err = read_system_regs(pcard);
if (err)
@@ -427,11 +412,9 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
writeq(0xFFFFFFFFFFFFFFFF,
pcard->sysinfo_regs_base + REG_INTERRUPT_MASK);
- /*
- * Step 7: Configure PCI thingies
- */
// let the card master PCIe
pci_set_master(pcard->pdev);
+
// enable IO and mem if not already done
pci_read_config_word(pcard->pdev, PCI_COMMAND, ®val);
regval |= (PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
@@ -466,9 +449,6 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
dev_dbg(&pcard->pdev->dev,
"Using DMA mask %0llx\n", dma_get_mask(PCARD_TO_DEV(pcard)));
- /*
- * Step 8: Configure IRQs
- */
err = pci_enable_msi(pcard->pdev);
if (err < 0)
goto err_unmap_dma;
@@ -481,25 +461,17 @@ static int kp2000_pcie_probe(struct pci_dev *pdev,
goto err_disable_msi;
}
- /*
- * Step 9: Setup sysfs attributes
- */
err = sysfs_create_files(&pdev->dev.kobj, kp_attr_list);
if (err) {
dev_err(&pdev->dev, "Failed to add sysfs files: %d\n", err);
goto err_free_irq;
}
- /*
- * Step 10: Probe cores
- */
err = kp2000_probe_cores(pcard);
if (err)
goto err_remove_sysfs;
- /*
- * Step 11: Enable IRQs in HW
- */
+ /* Enable IRQs in HW */
writel(KPC_DMA_CARD_IRQ_ENABLE | KPC_DMA_CARD_USER_INTERRUPT_MODE,
pcard->dma_common_regs);
--
2.20.1