Re: [PATCH v7 4/7] media: platform: amd: isp4 subdev and firmware loading handling added
From: Du, Bin
Date: Thu Jan 15 2026 - 05:36:28 EST
On 1/15/2026 5:03 AM, Sakari Ailus wrote:
Hi Sultan,
On Tue, Jan 06, 2026 at 11:33:53PM -0800, Sultan Alsawaf wrote:
Hi Sakari,
On Mon, Dec 22, 2025 at 12:11:11PM +0200, Sakari Ailus wrote:
Hi Bin,
On Tue, Dec 16, 2025 at 05:13:23PM +0800, Bin Du wrote:
Isp4 sub-device is implementing v4l2 sub-device interface. It has one
capture video node, and supports only preview stream. It manages firmware
states, stream configuration. Add interrupt handling and notification for
isp firmware to isp-subdevice.
Co-developed-by: Sultan Alsawaf <sultan@xxxxxxxxxxxxxxx>
Signed-off-by: Sultan Alsawaf <sultan@xxxxxxxxxxxxxxx>
Co-developed-by: Svetoslav Stoilov <Svetoslav.Stoilov@xxxxxxx>
Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@xxxxxxx>
Signed-off-by: Bin Du <Bin.Du@xxxxxxx>
Tested-by: Alexey Zagorodnikov <xglooom@xxxxxxxxx>
---
MAINTAINERS | 2 +
drivers/media/platform/amd/isp4/Makefile | 3 +-
drivers/media/platform/amd/isp4/isp4.c | 99 +-
drivers/media/platform/amd/isp4/isp4.h | 7 +-
drivers/media/platform/amd/isp4/isp4_subdev.c | 975 ++++++++++++++++++
drivers/media/platform/amd/isp4/isp4_subdev.h | 124 +++
6 files changed, 1202 insertions(+), 8 deletions(-)
create mode 100644 drivers/media/platform/amd/isp4/isp4_subdev.c
create mode 100644 drivers/media/platform/amd/isp4/isp4_subdev.h
diff --git a/MAINTAINERS b/MAINTAINERS
index cccae369c876..48ffc8bbdcee 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1149,6 +1149,8 @@ F: drivers/media/platform/amd/isp4/isp4_fw_cmd_resp.h
F: drivers/media/platform/amd/isp4/isp4_hw_reg.h
F: drivers/media/platform/amd/isp4/isp4_interface.c
F: drivers/media/platform/amd/isp4/isp4_interface.h
+F: drivers/media/platform/amd/isp4/isp4_subdev.c
+F: drivers/media/platform/amd/isp4/isp4_subdev.h
AMD KFD
M: Felix Kuehling <Felix.Kuehling@xxxxxxx>
diff --git a/drivers/media/platform/amd/isp4/Makefile b/drivers/media/platform/amd/isp4/Makefile
index a2a5bf98e912..6d4e6d6ac7f5 100644
--- a/drivers/media/platform/amd/isp4/Makefile
+++ b/drivers/media/platform/amd/isp4/Makefile
@@ -4,4 +4,5 @@
obj-$(CONFIG_AMD_ISP4) += amd_capture.o
amd_capture-objs := isp4.o \
- isp4_interface.o
+ isp4_interface.o \
+ isp4_subdev.o
diff --git a/drivers/media/platform/amd/isp4/isp4.c b/drivers/media/platform/amd/isp4/isp4.c
index ad95e7f89189..bcd7cad32afd 100644
--- a/drivers/media/platform/amd/isp4/isp4.c
+++ b/drivers/media/platform/amd/isp4/isp4.c
[snip]
static irqreturn_t isp4_irq_handler(int irq, void *arg)
{
+ struct isp4_subdev *isp_subdev = arg;
+ u32 intr_ack = 0, intr_en = 0, intr_status;
+ int seen = 0;
Is int appropriate here? Should this be u32 or u64?
ffs() is just a macro alias for __builtin_ffs(). The parameter and return value
of __builtin_ffs() are both int.
Ack, sounds reasonable.
Thanks for confirming, Sakari.
[snip]
unsigned int, please.
As mentioned above, ffs() takes an int and returns an int.
The parentheses around ffs() appear redundant.
The parentheses are there because it's an assignment. Without them:
drivers/media/platform/amd/isp4/isp4.c: In function ‘isp4_irq_handler’:
drivers/media/platform/amd/isp4/isp4.c:106:21: warning: suggest parentheses around assignment used as truth value [-Wparentheses]
106 | for (int i; i = ffs(seen); seen = (seen >> i) << i)
| ^
The increment could probably be expressed as seen &= ~BIT(i).
Yes it can be, but it adds several more instructions before the loop body,
without any improvement to the loop body (the sarx in the loop body is replaced
by andn). The right shift trick is faster and this is a hot path (IRQ handler).
Fine by me, it won't make much difference in practice either way.
...
Yes, both options work for me either, my idea is to keep it as is and add the following comment.
/*
* The operation `(seen >> i) << i` is logically equivalent to
* `seen &= ~BIT(i)`, with fewer instructions after compilation.
*/
[snip]
--
Regards,
Bin