Re: [PATCH 12/13] media: qcom: camss: Add CSID Gen3 support for sm8550

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 12/08/2024 15:41, Depeng Shao wrote:
+	writel(1, csid->base + CSID_TOP_IRQ_CLEAR);
+	writel(1, csid->base + CSID_IRQ_CMD);

CSID_IRQ_CMD bit(0) = CMD_CLEAR

+	writel(1, csid->base + CSID_TOP_IRQ_MASK);
+
+	for (i = 0; i < MSM_CSID_MAX_SRC_STREAMS; i++)
+		if (csid->phy.en_vc & BIT(i)) {
+			writel(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+						csid->base + CSID_BUF_DONE_IRQ_CLEAR);
+			writel(0x1 << IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);

CSID_IRQ_CMD bit(0) = CMD_CLEAR

and again here.

+			writel(BIT(BUF_DONE_IRQ_STATUS_RDI_OFFSET + i),
+						csid->base + CSID_BUF_DONE_IRQ_MASK);
+		}

re: previous comments

1. Please define bits so that'd be

#define CSID_IRQ_CMD_CLEAR	BIT(0)
writel(CSID_IRQ_CMD_CLEAR, csid->base + CSID_IRQ_CMD);

There's no value in circumscribing the meaning of bitfields in upstream code, we just make our own lives easier by having self-documenting code.

TL;DR please name your bits - a blanket statement for the series.

2. And as mentioned above, you don't need to execute that clear n times in a loop. Just do it once at the top of the routine.

---
bod




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux