media: atomisp: flatten pre_power_down logic and clean up staging code

PATCH In review Ruziev Miraly 2 participants 3 messages Jul 4 – Jul 5

Subsystems (4)

Media (V4L/DVB) Staging Intel MID (Mobile Internet Device) platform Staging - atomisp driver

lore

Share & tools

lore

Ruziev Miraly

author

2026-07-04 19:09 UTC #0 lore raw

PATCH v2 1/1 media: atomisp: flatten pre_power_down logic and clean up staging code

Refactor atomisp_mrfld_pre_power_down() by removing the redundant nested 'else' block, making the function structure linear and easier to follow. Keep both error logging branches intact to ensure consistency in hardware failure reporting.

In addition, fix minor checkpatch.pl style warnings in the same file regarding code alignment and hanging open parentheses.

Signed-off-by Ruziev Miraly <[email protected]>

---
Changes in v2:
- Fixed an issue from v1 where dev_err() was accidentally dropped.
- Kept both error logging branches for consistency with original behavior.
- Flattened the code structure by removing the nested 'else' block.
- Fixed checkpatch.pl style issues (hanging parenthesis and alignment).

---
 .../staging/media/atomisp/pci/atomisp_v4l2.c  | 39 +++++++++++--------
 1 file changed, 23 insertions(+), 16 deletions(-)

drivers/staging/media/atomisp/pci/atomisp_v4l2.c +23 ~~-16~~

index 2d72467eb..78cf09753 100644
@@ -439,14 +439,14 @@ static int atomisp_mrfld_pre_power_down(struct atomisp_device *isp)
 	struct pci_dev *pdev = to_pci_dev(isp->dev);
 	u32 irq;
 	unsigned long flags;
+	int ret = 0;
 
 	spin_lock_irqsave(&isp->lock, flags);
 
 	/*
 	 * MRFLD HAS requirement: cannot power off i-unit if
 	 * ISP has IRQ not serviced.
-	 * So, here we need to check if there is any pending
-	 * IRQ, if so, waiting for it to be served
+	 * Wait for pending IRQs to be served.
 	 */
 	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
 	irq &= BIT(INTR_IIR);

@@ -454,29 +454,36 @@ static int atomisp_mrfld_pre_power_down(struct atomisp_device *isp)
 
 	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
 	if (!(irq & BIT(INTR_IIR)))
-		goto done;
+		goto power_down;
 
+	/* Clear and check interrupt status registers */
 	atomisp_css2_hw_store_32(MRFLD_INTR_CLEAR_REG, 0xFFFFFFFF);
 	atomisp_load_uint32(MRFLD_INTR_STATUS_REG, &irq);
+
 	if (irq != 0) {
 		dev_err(isp->dev,
 			"%s: fail to clear isp interrupt status reg=0x%x\n",
 			__func__, irq);
-		spin_unlock_irqrestore(&isp->lock, flags);
-		return -EAGAIN;
+		ret = -EAGAIN;
+		goto unlock_exit;
 	}
+
 	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
 	irq &= BIT(INTR_IIR);
 	pci_write_config_dword(pdev, PCI_INTERRUPT_CTRL, irq);
 
 	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
-	if (!(irq & BIT(INTR_IIR))) {
-		atomisp_css2_hw_store_32(MRFLD_INTR_ENABLE_REG, 0x0);
-		goto done;
+	if (irq & BIT(INTR_IIR)) {
+		dev_err(isp->dev,
+			"%s: error in iunit interrupt. status reg=0x%x\n",
+			__func__, irq);
+		ret = -EAGAIN;
+		goto unlock_exit;
 	}
-	spin_unlock_irqrestore(&isp->lock, flags);
-	return -EAGAIN;
-done:
+
+	atomisp_css2_hw_store_32(MRFLD_INTR_ENABLE_REG, 0x0);
+
+power_down:
 	/*
 	 * MRFLD WORKAROUND:
 	 * before powering off IUNIT, clear the pending interrupts

@@ -490,9 +497,10 @@ static int atomisp_mrfld_pre_power_down(struct atomisp_device *isp)
 
 	atomisp_msi_irq_uninit(isp);
 	atomisp_freq_scaling(isp, ATOMISP_DFS_MODE_LOW, true);
-	spin_unlock_irqrestore(&isp->lock, flags);
 
-	return 0;
+unlock_exit:
+	spin_unlock_irqrestore(&isp->lock, flags);
+	return ret;
 }
 
 /*

@@ -839,7 +847,7 @@ static int atomisp_register_entities(struct atomisp_device *isp)
 	/* Register internal entities */
 	for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
 		ret = atomisp_mipi_csi2_register_entities(&isp->csi2_port[i],
-			&isp->v4l2_dev);
+							  &isp->v4l2_dev);
 		if (ret == 0)
 			continue;

@@ -847,8 +855,7 @@ static int atomisp_register_entities(struct atomisp_device *isp)
 		dev_err(isp->dev, "failed to register the CSI port: %d\n", i);
 		/* deregister all registered CSI ports */
 		while (i--)
-			atomisp_mipi_csi2_unregister_entities(
-			    &isp->csi2_port[i]);
+			atomisp_mipi_csi2_unregister_entities(&isp->csi2_port[i]);
 
 		goto csi_and_subdev_probe_failed;
 	}

Signature · 1 line

2.54.0

Andy Shevchenko

2026-07-05 06:46 UTC #1 lore raw

Quoted 3 lines · Ruziev Miraly

Refactor atomisp_mrfld_pre_power_down() by removing the redundant nested 'else' block, making the function structure linear and easier to follow.

I don't see this. There is no 'else' removal, what did I miss?

Quoted 5 lines

Keep both error logging branches intact to ensure consistency in hardware failure reporting.
In addition, fix minor checkpatch.pl style warnings in the same file regarding code alignment and hanging open parentheses.

These are a few things that probably need to be split to different patches.

...

drivers/staging/media/atomisp/pci/atomisp_v4l2.c static int atomisp_mrfld_pre_power_down(struct atomisp_device *isp) · line 442

Quoted 4 lines

drivers/staging/media/atomisp/pci/atomisp_v4l2.c +1 -0

 	struct pci_dev *pdev = to_pci_dev(isp->dev);
 	u32 irq;
 	unsigned long flags;
+	int ret = 0;

Don't do like this. It makes code harder to maintain and prone to mistakes.

drivers/staging/media/atomisp/pci/atomisp_v4l2.c static int atomisp_mrfld_pre_power_down(struct atomisp_device *isp) · line 450

Quoted 9 lines

drivers/staging/media/atomisp/pci/atomisp_v4l2.c +1 -2

 	spin_lock_irqsave(&isp->lock, flags);
 
 	/*
 	 * MRFLD HAS requirement: cannot power off i-unit if
 	 * ISP has IRQ not serviced.
-	 * So, here we need to check if there is any pending
-	 * IRQ, if so, waiting for it to be served
+	 * Wait for pending IRQs to be served.
 	 */

Hmm... Why is this wording changed?

Quoted 45 lines

drivers/staging/media/atomisp/pci/atomisp_v4l2.c +16 -9

 	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
 	if (!(irq & BIT(INTR_IIR)))
-		goto done;
+		goto power_down;
 
+	/* Clear and check interrupt status registers */
 	atomisp_css2_hw_store_32(MRFLD_INTR_CLEAR_REG, 0xFFFFFFFF);
 	atomisp_load_uint32(MRFLD_INTR_STATUS_REG, &irq);
+
 	if (irq != 0) {
 		dev_err(isp->dev,
 			"%s: fail to clear isp interrupt status reg=0x%x\n",
 			__func__, irq);
-		spin_unlock_irqrestore(&isp->lock, flags);
-		return -EAGAIN;
+		ret = -EAGAIN;
+		goto unlock_exit;
 	}
+
 	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
 	irq &= BIT(INTR_IIR);
 	pci_write_config_dword(pdev, PCI_INTERRUPT_CTRL, irq);
 
 	pci_read_config_dword(pdev, PCI_INTERRUPT_CTRL, &irq);
-	if (!(irq & BIT(INTR_IIR))) {
-		atomisp_css2_hw_store_32(MRFLD_INTR_ENABLE_REG, 0x0);
-		goto done;
+	if (irq & BIT(INTR_IIR)) {
+		dev_err(isp->dev,
+			"%s: error in iunit interrupt. status reg=0x%x\n",
+			__func__, irq);
+		ret = -EAGAIN;
+		goto unlock_exit;
 	}
-	spin_unlock_irqrestore(&isp->lock, flags);
-	return -EAGAIN;
-done:
+
+	atomisp_css2_hw_store_32(MRFLD_INTR_ENABLE_REG, 0x0);
+
+power_down:
 	/*
 	 * MRFLD WORKAROUND:
 	 * before powering off IUNIT, clear the pending interrupts

drivers/staging/media/atomisp/pci/atomisp_v4l2.c static int atomisp_mrfld_pre_power_down(struct atomisp_device *isp) · line 504

Quoted 10 lines

drivers/staging/media/atomisp/pci/atomisp_v4l2.c +3 -2

@@ -490,9 +497,10 @@ static int atomisp_mrfld_pre_power_down(struct atomisp_device *isp)
 
 	atomisp_msi_irq_uninit(isp);
 	atomisp_freq_scaling(isp, ATOMISP_DFS_MODE_LOW, true);
-	spin_unlock_irqrestore(&isp->lock, flags);
 
-	return 0;
+unlock_exit:
+	spin_unlock_irqrestore(&isp->lock, flags);
+	return ret;
 }

Are you sending an update on top of your v1?! You must use the same base for the each version of the patches. The incremental changes in Git means something different.

...

drivers/staging/media/atomisp/pci/atomisp_v4l2.c static int atomisp_register_entities(struct atomisp_device *isp) · line 852

Quoted 8 lines

drivers/staging/media/atomisp/pci/atomisp_v4l2.c +1 -1

@@ -839,7 +847,7 @@ static int atomisp_register_entities(struct atomisp_device *isp)
 	/* Register internal entities */
 	for (i = 0; i < ATOMISP_CAMERA_NR_PORTS; i++) {
 		ret = atomisp_mipi_csi2_register_entities(&isp->csi2_port[i],
-			&isp->v4l2_dev);
+							  &isp->v4l2_dev);
 		if (ret == 0)
 			continue;

This change is not related. Split it.

...

drivers/staging/media/atomisp/pci/atomisp_v4l2.c static int atomisp_register_entities(struct atomisp_device *isp) · line 861

Quoted 10 lines

drivers/staging/media/atomisp/pci/atomisp_v4l2.c +1 -2

@@ -847,8 +855,7 @@ static int atomisp_register_entities(struct atomisp_device *isp)
 		dev_err(isp->dev, "failed to register the CSI port: %d\n", i);
 		/* deregister all registered CSI ports */
 		while (i--)
-			atomisp_mipi_csi2_unregister_entities(
-			    &isp->csi2_port[i]);
+			atomisp_mipi_csi2_unregister_entities(&isp->csi2_port[i]);
 
 		goto csi_and_subdev_probe_failed;
 	}

As previous, should be in another patch.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2026-07-05 06:50 UTC #2 lore raw

In reply to Ruziev Miraly

Quoted 10 lines · Ruziev Miraly

Refactor atomisp_mrfld_pre_power_down() by removing the redundant nested 'else' block, making the function structure linear and easier to follow. Keep both error logging branches intact to ensure consistency in hardware failure reporting.
In addition, fix minor checkpatch.pl style warnings in the same file regarding code alignment and hanging open parentheses.
---
Changes in v2:

As I see this went completely wrong. Read my previous reply why. Please, slow down and take your time to study how the Git works and what is the process to upstream the changes and read related (in-kernel) documentation for that (also articles from lwn.net will be helpful).

On top of that, find a hardware to test your changes. It's usually quite cheap no-name Intel Bay Trail or Intel Cherry Trail tablets one may find.

Signature · 2 lines

With Best Regards,
Andy Shevchenko