Re: [PATCH v7 4/7] media: platform: amd: isp4 subdev and firmware loading handling added

["Du, Bin" <bin.du@amd.com>]

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@kerneltoast.com>
>>>> Signed-off-by: Sultan Alsawaf <sultan@kerneltoast.com>
>>>> Co-developed-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>>> Signed-off-by: Svetoslav Stoilov <Svetoslav.Stoilov@amd.com>
>>>> Signed-off-by: Bin Du <Bin.Du@amd.com>
>>>> Tested-by: Alexey Zagorodnikov <xglooom@gmail.com>
>>>> ---
>>>>   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@amd.com>
>>>> 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