Re: [PATCH v4 2/7] media: platform: amd: low level support for isp4 firmware

[Du, Bin]

Many thanks Sultan for the careful review.

On 9/22/2025 4:31 AM, Sultan Alsawaf wrote:
> Hi Bin,
>
> On Thu, Sep 11, 2025 at 06:08:42PM +0800, Bin Du wrote:
> > Low level functions for accessing the registers and mapping to their
> > ranges. This change also includes register definitions for ring buffer
> > used to communicate with ISP Firmware. Ring buffer is the communication
> > interface between driver and ISP Firmware. Command and responses are
> > exchanged through the ring buffer.
> >
> > 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>
>
> [snip]
>
> > +++ b/drivers/media/platform/amd/isp4/Makefile
> > @@ -3,4 +3,4 @@
> >   # Copyright (C) 2025 Advanced Micro Devices, Inc.
> >   
> >   obj-$(CONFIG_AMD_ISP4) += amd_capture.o
> > -amd_capture-objs := isp4.o
> > +amd_capture-objs := isp4.o	\
>
> Remove this hunk since there are no new objects added in this patch.

Yes, you are correct, will remove it in the next version

> > +++ b/drivers/media/platform/amd/isp4/isp4_hw_reg.h
> > @@ -0,0 +1,125 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + * Copyright (C) 2025 Advanced Micro Devices, Inc.
> > + */
> > +
> > +#ifndef _ISP4_HW_REG_H_
> > +#define _ISP4_HW_REG_H_
> > +
> > +#include <linux/io.h>
> > +#include <linux/types.h>
>
> Remove redundant linux/types.h include, as it is included by linux/io.h.

Sure, will remove it in the next version

> > +
> > +#define ISP_SOFT_RESET			0x62000
>
> [snip]
>
> > +
> > +#endif
>
> Add /* _ISP4_HW_REG_H_ */

Sure, will add it in the next version and also check all other header files.

> > --
> > 2.34.1
>
> Sultan

--
Regards,
Bin