Message-ID: <7cfeeb34-6f5c-4c7c-9a60-dcb35cdd458b@kernel.org>
Date: Wed, 17 Jul 2024 15:16:56 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: 20220614094956 created <yelian.wang@...iatek.com>,
 Mauro Carvalho Chehab <mchehab@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Matthias Brugger <matthias.bgg@...il.com>,
 AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: linux-media@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-mediatek@...ts.infradead.org,
 Project_Global_Chrome_Upstream_Group@...iatek.com, yaya.chang@...iatek.com,
 teddy.chen@...iatek.com, hidenorik@...omium.org
Subject: Re: [PATCH 3/3] media: mediatek: add MT8188 AIE driver

On 17/07/2024 14:41, 20220614094956 created wrote:
> From: Yelian Wang <yelian.wang@...iatek.com>
> 
> Add a V4L2 sub-device driver for MT8188 AIE.
> 
> Signed-off-by: Yelian Wang <yelian.wang@...iatek.com>
> ---
>  drivers/media/platform/mediatek/Kconfig       |    1 +
>  drivers/media/platform/mediatek/Makefile      |    1 +
>  drivers/media/platform/mediatek/aie/Kconfig   |   13 +
>  drivers/media/platform/mediatek/aie/Makefile  |    5 +
>  drivers/media/platform/mediatek/aie/mtk_aie.h | 1012 +++++
>  .../media/platform/mediatek/aie/mtk_aie_53.c  | 2031 +++++++++
>  .../media/platform/mediatek/aie/mtk_aie_drv.c | 3613 +++++++++++++++++
>  7 files changed, 6676 insertions(+)
>  create mode 100644 drivers/media/platform/mediatek/aie/Kconfig
>  create mode 100644 drivers/media/platform/mediatek/aie/Makefile
>  create mode 100644 drivers/media/platform/mediatek/aie/mtk_aie.h
>  create mode 100644 drivers/media/platform/mediatek/aie/mtk_aie_53.c
>  create mode 100644 drivers/media/platform/mediatek/aie/mtk_aie_drv.c
> 
> diff --git a/drivers/media/platform/mediatek/Kconfig b/drivers/media/platform/mediatek/Kconfig
> index 84104e2cd024..987c754a76b7 100644
> --- a/drivers/media/platform/mediatek/Kconfig
> +++ b/drivers/media/platform/mediatek/Kconfig
> @@ -7,3 +7,4 @@ source "drivers/media/platform/mediatek/mdp/Kconfig"
>  source "drivers/media/platform/mediatek/vcodec/Kconfig"
>  source "drivers/media/platform/mediatek/vpu/Kconfig"
>  source "drivers/media/platform/mediatek/mdp3/Kconfig"
> +source "drivers/media/platform/mediatek/aie/Kconfig"
> diff --git a/drivers/media/platform/mediatek/Makefile b/drivers/media/platform/mediatek/Makefile
> index 38e6ba917fe5..db8e6d09de4e 100644
> --- a/drivers/media/platform/mediatek/Makefile
> +++ b/drivers/media/platform/mediatek/Makefile
> @@ -4,3 +4,4 @@ obj-y += mdp/
>  obj-y += vcodec/
>  obj-y += vpu/
>  obj-y += mdp3/
> +obj-y += aie/
> diff --git a/drivers/media/platform/mediatek/aie/Kconfig b/drivers/media/platform/mediatek/aie/Kconfig
> new file mode 100644
> index 000000000000..f2fe4bfcdd6a
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/aie/Kconfig
> @@ -0,0 +1,13 @@
> +config VIDEO_MTK_AIE
> +	tristate "MediaTek AI engine function"
> +	depends on OF
> +	select V4L2_MEM2MEM_DEV
> +	select VIDEOBUF2_DMA_CONTIG
> +	select MEDIA_CONTROLLER_REQUEST_API
> +	help
> +		Support the AI engine (AIE) feature

Fix indentation. See every other file...

> +
> +		AIE driver is a V4L2 memory-to-memory device driver which
> +		provides hardware accelerated face detection function,
> +		it can detect different sizes of faces in a raw image.
> +
> diff --git a/drivers/media/platform/mediatek/aie/Makefile b/drivers/media/platform/mediatek/aie/Makefile
> new file mode 100644
> index 000000000000..615fb8de8f3e
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/aie/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +mtk-aie-y += mtk_aie_53.o
> +mtk-aie-y += mtk_aie_drv.o
> +
> +obj-$(CONFIG_VIDEO_MTK_AIE) += mtk-aie.o
> diff --git a/drivers/media/platform/mediatek/aie/mtk_aie.h b/drivers/media/platform/mediatek/aie/mtk_aie.h
> new file mode 100644
> index 000000000000..0a2af547b56b
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/aie/mtk_aie.h
> @@ -0,0 +1,1012 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2020 MediaTek Inc.
> + * Author: Fish Wu <fish.wu@...iatek.com>
> + */
> +
> +#ifndef __MTK_AIE_H__
> +#define __MTK_AIE_H__
> +
> +#include <linux/completion.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/types.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-ctrls.h>
> +#include <media/v4l2-device.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +// AIE 3.X
> +
> +#ifdef CONFIG_INTERCONNECT_MTK_EXTENSION

Nope, there is no such stuff.

> +#include "mtk-interconnect.h"
> +#endif
> +
> +// AIE 3.X
> +#define MTK_AIE_OPP_SET 1
> +#define MTK_AIE_CLK_LEVEL_CNT 4
> +
> +#define FD_VERSION 1946050
> +#define ATTR_VERSION 1929401

What do these mean? Document you magic constants.



...

> +
> +#define FLD_SH_IN_BASE_ADDR_0 0x760
> +#define FLD_SH_IN_BASE_ADDR_1 0x764
> +#define FLD_SH_IN_BASE_ADDR_2 0x768
> +#define FLD_SH_IN_BASE_ADDR_3 0x76C
> +#define FLD_SH_IN_BASE_ADDR_4 0x770
> +#define FLD_SH_IN_BASE_ADDR_5 0x774
> +#define FLD_SH_IN_BASE_ADDR_6 0x778
> +#define FLD_SH_IN_BASE_ADDR_7 0x77C
> +#define FLD_SH_IN_BASE_ADDR_8 0x780
> +#define FLD_SH_IN_BASE_ADDR_9 0x784
> +#define FLD_SH_IN_BASE_ADDR_10 0x788
> +#define FLD_SH_IN_BASE_ADDR_11 0x78C
> +#define FLD_SH_IN_BASE_ADDR_12 0x790
> +#define FLD_SH_IN_BASE_ADDR_13 0x794
> +#define FLD_SH_IN_BASE_ADDR_14 0x798
> +#define FLD_SH_IN_BASE_ADDR_15 0x79C
> +#define FLD_SH_IN_BASE_ADDR_16 0x7A0
> +#define FLD_SH_IN_BASE_ADDR_17 0x7A4
> +#define FLD_SH_IN_BASE_ADDR_18 0x7A8
> +#define FLD_SH_IN_BASE_ADDR_19 0x7AC
> +#define FLD_SH_IN_BASE_ADDR_20 0x7B0
> +#define FLD_SH_IN_BASE_ADDR_21 0x7B4
> +#define FLD_SH_IN_BASE_ADDR_22 0x7B8
> +#define FLD_SH_IN_BASE_ADDR_23 0x7BC
> +#define FLD_SH_IN_BASE_ADDR_24 0x7C0
> +#define FLD_SH_IN_BASE_ADDR_25 0x7C4
> +#define FLD_SH_IN_BASE_ADDR_26 0x7C8
> +#define FLD_SH_IN_BASE_ADDR_27 0x7CC
> +#define FLD_SH_IN_BASE_ADDR_28 0x7D0
> +#define FLD_SH_IN_BASE_ADDR_29 0x7D4
> +
> +#define FLD_SH_IN_SIZE_0 0x7D8
> +#define FLD_SH_IN_STRIDE_0 0x7DC
> +#define FLD_TR_OUT_BASE_ADDR_0 0x7E0
> +#define FLD_TR_OUT_SIZE_0 0x7E4
> +#define FLD_TR_OUT_STRIDE_0 0x7E8
> +#define FLD_PP_OUT_BASE_ADDR_0 0x7EC
> +#define FLD_PP_OUT_SIZE_0 0x7F0
> +#define FLD_PP_OUT_STRIDE_0 0x7F4
> +#define FLD_SPARE 0x7F8
> +
> +#define FLD_BASE_ADDR_FACE_0_7_MSB 0x7FC
> +#define FLD_BASE_ADDR_FACE_8_14_MSB 0x800
> +
> +#define FLD_PL_IN_BASE_ADDR_0_0_7_MSB 0x804
> +#define FLD_PL_IN_BASE_ADDR_0_8_15_MSB 0x808
> +#define FLD_PL_IN_BASE_ADDR_0_16_23_MSB 0x80C
> +#define FLD_PL_IN_BASE_ADDR_0_24_29_MSB 0x810
> +
> +#define FLD_PL_IN_BASE_ADDR_1_0_7_MSB 0x814
> +#define FLD_PL_IN_BASE_ADDR_1_8_15_MSB 0x818
> +#define FLD_PL_IN_BASE_ADDR_1_16_23_MSB 0x81C
> +#define FLD_PL_IN_BASE_ADDR_1_24_29_MSB 0x820
> +
> +#define FLD_PL_IN_BASE_ADDR_2_0_7_MSB 0x824
> +#define FLD_PL_IN_BASE_ADDR_2_8_15_MSB 0x828
> +#define FLD_PL_IN_BASE_ADDR_2_16_23_MSB 0x82C
> +#define FLD_PL_IN_BASE_ADDR_2_24_29_MSB 0x830
> +
> +#define FLD_PL_IN_BASE_ADDR_3_0_7_MSB 0x834
> +#define FLD_PL_IN_BASE_ADDR_3_8_15_MSB 0x838
> +#define FLD_PL_IN_BASE_ADDR_3_16_23_MSB 0x83C
> +#define FLD_PL_IN_BASE_ADDR_3_24_29_MSB 0x840
> +
> +#define FLD_SH_IN_BASE_ADDR_0_7_MSB 0x844
> +#define FLD_SH_IN_BASE_ADDR_8_15_MSB 0x848
> +#define FLD_SH_IN_BASE_ADDR_16_23_MSB 0x84C
> +#define FLD_SH_IN_BASE_ADDR_24_29_MSB 0x850
> +
> +#define FLD_BS_IN_BASE_ADDR_0_7_MSB 0x8d4
> +#define FLD_BS_IN_BASE_ADDR_8_15_MSB 0x8d8
> +
> +#define FLD_TR_OUT_BASE_ADDR_0_MSB 0x854
> +#define FLD_PP_OUT_BASE_ADDR_0_MSB 0x858


All this coding style is unreadable.

> +
> +#define FLD_BS_IN_BASE_ADDR_00 0x85C
> +#define FLD_BS_IN_BASE_ADDR_01 0x860
> +#define FLD_BS_IN_BASE_ADDR_02 0x864
> +#define FLD_BS_IN_BASE_ADDR_03 0x868
> +#define FLD_BS_IN_BASE_ADDR_04 0x86C
> +#define FLD_BS_IN_BASE_ADDR_05 0x870
> +#define FLD_BS_IN_BASE_ADDR_06 0x874
> +#define FLD_BS_IN_BASE_ADDR_07 0x878
> +#define FLD_BS_IN_BASE_ADDR_08 0x87C
> +#define FLD_BS_IN_BASE_ADDR_09 0x880
> +#define FLD_BS_IN_BASE_ADDR_10 0x884
> +#define FLD_BS_IN_BASE_ADDR_11 0x888
> +#define FLD_BS_IN_BASE_ADDR_12 0x88C
> +#define FLD_BS_IN_BASE_ADDR_13 0x890
> +#define FLD_BS_IN_BASE_ADDR_14 0x894
> +#define FLD_BS_BIAS 0x8E4
> +#define FLD_CV_FM_RANGE_0 0x8E8
> +#define FLD_CV_FM_RANGE_1 0x8EC
> +#define FLD_CV_PM_RANGE_0 0x8F0
> +#define FLD_CV_PM_RANGE_1 0x8F4
> +#define FLD_BS_RANGE_0 0x8F8
> +#define FLD_BS_RANGE_1 0x8FC
> +
> +#define MTK_FD_OUTPUT_MIN_WIDTH 16U
> +#define MTK_FD_OUTPUT_MIN_HEIGHT 16U
> +#define MTK_FD_OUTPUT_MAX_WIDTH 4096U
> +#define MTK_FD_OUTPUT_MAX_HEIGHT 4096U
> +
> +#define MTK_FD_HW_TIMEOUT_IN_MSEC 2000
> +#define MAX_FACE_NUM 1024
> +#define RLT_NUM 48
> +#define GENDER_OUT 32
> +
> +#define RACE_RST_X_NUM 4
> +#define RACE_RST_Y_NUM 64
> +#define GENDER_RST_X_NUM 2
> +#define GENDER_RST_Y_NUM 64
> +#define MRACE_RST_NUM 4
> +#define MGENDER_RST_NUM 2
> +#define MAGE_RST_NUM 2
> +#define MINDIAN_RST_NUM 2
> +
> +// AIE 3.X
> +#define FLD_FOREST 14
> +#define FLD_POINT 500
> +
> +#define FLD_STEP_NUM 6
> +#define FLD_MAX_FRAME 15
> +
> +#define FLD_STEP_BLINK 0
> +#define FLD_STEP_CV 1
> +#define FLD_STEP_FP 2
> +#define FLD_STEP_LEAF 3
> +#define FLD_STEP_KM02 4
> +#define FLD_STEP_KM13 5
> +
> +#define FLD_BLINK_WEIGHT_FOREST14_SIZE 6416
> +#define FLD_CV_SIZE 19392
> +#define FLD_FP_SIZE 80160
> +#define FLD_LEAFNODE_SIZE 4608000
> +#define FLD_TREE_KM02_SIZE 120000
> +#define FLD_TREE_KM13_SIZE 120000
> +#define FLD_OUTPUT_SIZE 112
> +
> +#define FLD_CUR_LANDMARK 11
> +
> +#define RESULT_SIZE (RLT_NUM * MAX_FACE_NUM)
> +
> +struct aie_static_info_element {
> +	unsigned int fd_wdma_size[OUTPUT_WDMA_WRA_NUM];
> +	unsigned int out_xsize_plus_1;
> +	unsigned int out_height;
> +	unsigned int out_ysize_plus_1_stride2;
> +	unsigned int out_stride;
> +	unsigned int out_stride_stride2;
> +	unsigned int out_width;
> +	unsigned int img_width;
> +	unsigned int img_height;
> +	unsigned int stride2_out_width;
> +	unsigned int stride2_out_height;
> +	unsigned int out_xsize_plus_1_stride2;
> +	unsigned int input_xsize_plus_1;
> +};
> +
> +struct aie_static_info {
> +	struct aie_static_info_element inf_elm[FD_LOOP_NUM];
> +};
> +
> +enum aie_state { STATE_NA = 0x0, STATE_INIT = 0x1, STATE_OPEN = 0x2 };

That's some odd wrapping. Please follow Linux Coding Style/.


> +
> +// AIE 3.1
> +enum aie_mode { FDMODE = 0, ATTRIBUTEMODE = 1, FLDMODE = 2};
> +
> +enum aie_format {
> +	FMT_NA = 0,
> +	FMT_YUV_2P = 1,
> +	FMT_YVU_2P = 2,
> +	FMT_YUYV = 3,
> +	FMT_YVYU = 4,
> +	FMT_UYVY = 5,
> +	FMT_VYUY = 6,
> +	FMT_MONO = 7,
> +	// AIE 3.X
> +	FMT_YUV420_2P = 8,
> +	FMT_YUV420_1P = 9
> +};
> +
> +enum aie_input_degree {
> +	DEGREE_0 = 0,
> +	DEGREE_90 = 1,
> +	DEGREE_270 = 2,
> +	DEGREE_180 = 3
> +};
> +
> +/* align v4l2 user space interface */
> +struct FDRESULT {
> +	u16 anchor_x0[MAX_FACE_NUM];
> +	u16 anchor_x1[MAX_FACE_NUM];
> +	u16 anchor_y0[MAX_FACE_NUM];
> +	u16 anchor_y1[MAX_FACE_NUM];
> +	signed short rop_landmark_score0[MAX_FACE_NUM];
> +	signed short rop_landmark_score1[MAX_FACE_NUM];
> +	signed short rop_landmark_score2[MAX_FACE_NUM];
> +	signed short anchor_score[MAX_FACE_NUM];
> +	signed short rip_landmark_score0[MAX_FACE_NUM];
> +	signed short rip_landmark_score1[MAX_FACE_NUM];
> +	signed short rip_landmark_score2[MAX_FACE_NUM];
> +	signed short rip_landmark_score3[MAX_FACE_NUM];
> +	signed short rip_landmark_score4[MAX_FACE_NUM];
> +	signed short rip_landmark_score5[MAX_FACE_NUM];
> +	signed short rip_landmark_score6[MAX_FACE_NUM];
> +	u16 face_result_index[MAX_FACE_NUM];
> +	u16 anchor_index[MAX_FACE_NUM];
> +	u32 fd_partial_result;
> +};
> +
> +struct fd_result {
> +	u16 fd_pyramid0_num;
> +	u16 fd_pyramid1_num;
> +	u16 fd_pyramid2_num;
> +	u16 fd_total_num;
> +	struct FDRESULT PYRAMID0_RESULT;


Nope. Drop all this downstream junk style.

This applies everywhere - there is no C type or member declaration
(except pre-processor) which is upper case. Nowhere.

The fact you send such code means you push downstream code, which is not
how you upstream stuff.

Drop entire driver and start from scratch from decent and recent driver
which has already fixed all this weirdness.


...

> +
> +struct mtk_aie_dev {
> +	struct device *dev;
> +	struct mtk_aie_ctx *ctx;
> +	struct v4l2_m2m_dev *m2m_dev;
> +	struct device *larb;
> +	struct aie_para *base_para;
> +	struct aie_attr_para *attr_para;
> +	struct aie_fd_dma_para *dma_para;
> +
> +	// AIE 3.X
> +	struct aie_fd_fld_para *fld_para;
> +#ifdef CONFIG_INTERCONNECT_MTK_EXTENSION

WTF?

> +	struct mtk_aie_dvfs *dvfs_info;
> +	struct mtk_aie_qos *qos_info;
> +#endif
> +
> +	struct aie_enq_info *aie_cfg;
> +	struct workqueue_struct *frame_done_wq;
> +	void __iomem *fd_base;
> +	const struct mtk_aie_variant *variant;
> +
> +	/* Input Buffer Pointer */
> +	struct imem_buf_info rs_cfg_data;
> +	struct imem_buf_info fd_cfg_data;
> +	struct imem_buf_info yuv2rgb_cfg_data;
> +	/* HW Output Buffer Pointer */
> +	struct imem_buf_info rs_output_hw;
> +	struct imem_buf_info fd_dma_hw;
> +	struct imem_buf_info fd_dma_result_hw;
> +	struct imem_buf_info fd_kernel_hw;
> +	struct imem_buf_info fd_attr_dma_hw;
> +	struct aie_static_info st_info;
> +
> +	struct aie_reg_cfg reg_cfg;
> +
> +	// AIE 3.X
> +	/* fld fw buffer */
> +	struct media_device mdev;
> +	struct video_device vfd;
> +	struct aie_clocks aie_clk;
> +	struct v4l2_device v4l2_dev;
> +
> +	/* Lock for V4L2 operations */
> +	struct mutex vfd_lock;
> +	/* Lock for device operations */
> +	struct mutex dev_lock;
> +	/* Lock for performance optimization */
> +	struct mutex fd_lock;
> +	struct imem_buf_info fd_fld_step_data;
> +	struct imem_buf_info fd_fld_out_hw;
> +
> +	int irq;
> +	struct completion fd_job_finished;
> +	struct delayed_work job_timeout_work;
> +
> +	/* DRAM Buffer Size */
> +	unsigned int fd_rs_cfg_size;
> +	unsigned int fd_fd_cfg_size;
> +	unsigned int fd_yuv2rgb_cfg_size;
> +	unsigned int attr_fd_cfg_size;
> +	unsigned int attr_yuv2rgb_cfg_size;
> +
> +	/* HW Output Buffer Size */
> +	unsigned int rs_pym_out_size[PYM_NUM];
> +	unsigned int fd_dma_max_size;
> +	unsigned int fd_dma_rst_max_size;
> +	unsigned int fd_fd_kernel_size;
> +	unsigned int fd_attr_kernel_size;
> +	unsigned int fd_attr_dma_max_size;
> +	unsigned int fd_attr_dma_rst_max_size;
> +	// AIE 3.X
> +	/* fld size */
> +	unsigned int fld_step_size;
> +	unsigned int fld_out_size;
> +
> +	wait_queue_head_t flushing_waitq;
> +	atomic_t num_composing;
> +	struct mtk_aie_req_work req_work;
> +	unsigned int fd_state;
> +	unsigned int fd_mem_size;
> +	u32 fd_stream_count;


Amount of all these structures and their members is HUGE. Many look like
duplicated, but tricky to say.

Uptreaming this stuff will take many iterations, so start from dividing
your work into proper reasonable and logical chunks.

....


> +/**************************************************************************/
> +/*                   C L A S S    D E C L A R A T I O N                   */


What class?

> +/**************************************************************************/
> +
> +void aie_reset(struct mtk_aie_dev *fd);
> +int aie_init(struct mtk_aie_dev *fd, struct v4l2_ctrl_aie_init *user_init);
> +void aie_uninit(struct mtk_aie_dev *fd);
> +void aie_prepare(struct mtk_aie_dev *fd, struct aie_enq_info *aie_cfg);
> +void aie_execute(struct mtk_aie_dev *fd, struct aie_enq_info *aie_cfg);
> +void aie_irqhandle(struct mtk_aie_dev *fd);
> +void aie_get_fd_result(struct mtk_aie_dev *fd, struct aie_enq_info *aie_cfg);
> +void aie_get_attr_result(struct mtk_aie_dev *fd, struct aie_enq_info *aie_cfg);
> +void aie_get_fld_result(struct mtk_aie_dev *fd, struct aie_enq_info *aie_cfg);
> +#endif /*__MTK_AIE_H__*/
> diff --git a/drivers/media/platform/mediatek/aie/mtk_aie_53.c b/drivers/media/platform/mediatek/aie/mtk_aie_53.c
> new file mode 100644
> index 000000000000..a976b23c5576
> --- /dev/null
> +++ b/drivers/media/platform/mediatek/aie/mtk_aie_53.c
> @@ -0,0 +1,2031 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 MediaTek Inc.
> + * Author: Fish Wu <fish.wu@...iatek.com>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/iopoll.h>
> +#include <linux/jiffies.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/remoteproc.h>
> +#include <linux/types.h>
> +#include <linux/version.h>
> +#include <linux/mtk_aie_v4l2_controls.h>
> +
> +#include <media/v4l2-event.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mem2mem.h>
> +#include <media/videobuf2-core.h>
> +#include <media/videobuf2-dma-contig.h>
> +
> +#include <soc/mediatek/smi.h>
> +
> +#include "mtk_aie.h"
> +
> +static const struct mtk_aie_variant *mtk_aie_get_variant(struct device *dev);
> +
> +static struct clk_bulk_data aie_clks[] = {

Why this is file-scope not device-scope?

> +	{ .id = "IMG_IPE" },
> +	{ .id = "IPE_FDVT" },
> +	{ .id = "IPE_TOP" },
> +	{ .id = "IPE_SMI_LARB12" },
> +};
> +
> +#ifdef CONFIG_DEBUG_FS
> +#define AIE_DUMP_BUFFER_SIZE (1 * 1024 * 1024)
> +#define AIE_DEBUG_RAW_DATA_SIZE (1 * 640 * 480)
> +static unsigned char srcrawdata[AIE_DEBUG_RAW_DATA_SIZE];
> +static unsigned char dstrawdata[AIE_DEBUG_RAW_DATA_SIZE];
> +static struct debugfs_blob_wrapper srcdata, dstdata;

Oh my... No, drop all these.

> +#endif
> +
> +#ifdef CONFIG_INTERCONNECT_MTK_EXTENSION

WTF?

> +#define AIE_QOS_MAX 4
> +#define AIE_QOS_RA_IDX 0
> +#define AIE_QOS_RB_IDX 1
> +#define AIE_QOS_WA_IDX 2
> +#define AIE_QOS_WB_IDX 3
> +#define AIE_READ_AVG_BW 213
> +#define AIE_WRITE_AVG_BW 145
> +#endif
> +
> +#define V4L2_CID_MTK_AIE_MAX 2


> +
> +	opp_num = regulator_count_voltages(fd->dvfs_info->reg);
> +	of_for_each_phandle(&it, ret, fd->dvfs_info->dev->of_node, "operating-points-v2", NULL, 0) {
> +		if (!it.node) {
> +			dev_err(fd->dvfs_info->dev, "of_node_get fail\n");
> +			return ret;
> +		}
> +		np = of_node_get(it.node);
> +
> +		do {
> +			child_np = of_get_next_available_child(np, child_np);
> +			if (child_np) {
> +				of_property_read_u64(child_np, "opp-hz", &freq);
> +				of_property_read_u32(child_np, "opp-microvolt", &volt);

Whaaaaat?

Sorry, that's not how you use OPP code.

> +				if (freq == 0 || volt == 0) {
> +					dev_err(fd->dvfs_info->dev,
> +						"%s: [ERROR] parsing zero freq/volt(%d/%d) at idx(%d)\n",
> +						__func__,
> +						freq,
> +						volt,
> +						idx
> +					);
> +					continue;
> +				}
> +				fd->dvfs_info->clklv[opp_idx][idx] = freq;
> +				fd->dvfs_info->voltlv[opp_idx][idx] = volt;
> +				dev_dbg(fd->dvfs_info->dev,
> +					"[%s] opp=%d, idx=%d, clk=%d volt=%d\n",
> +					__func__,
> +					opp_idx,
> +					idx,
> +					fd->dvfs_info->clklv[opp_idx][idx],
> +					fd->dvfs_info->voltlv[opp_idx][idx]
> +				);
> +				idx++;
> +			}
> +		} while (child_np);
> +		fd->dvfs_info->clklv_num[opp_idx] = idx;
> +		fd->dvfs_info->clklv_target[opp_idx] = fd->dvfs_info->clklv[opp_idx][0];
> +		fd->dvfs_info->clklv_idx[opp_idx] = 0;
> +		idx = 0;
> +		opp_idx++;
> +		of_node_put(np);
> +	}
> +	fd->dvfs_info->cur_volt = 0;
> +
> +	return 0;
> +}

...

to err_free_dev;
> +	}
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	ret = v4l2_m2m_register_media_controller(m2m_dev, vfd, MEDIA_ENT_F_PROC_VIDEO_STATISTICS);
> +	if (ret) {
> +		dev_err(dev, "Failed to init mem2mem media controller\n");
> +		goto err_unreg_video;
> +	}
> +#endif
> +	return 0;
> +#ifdef CONFIG_MEDIA_CONTROLLER

What? Ifdef in error path?

> +err_unreg_video:
> +	video_unregister_device(vfd);
> +#endif
> +err_free_dev:
> +	return ret;
> +}
> +
> +static int mtk_aie_dev_v4l2_init(struct mtk_aie_dev *fd)
> +{
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	struct media_device *mdev = &fd->mdev;
> +#endif
> +	struct device *dev = fd->dev;
> +	int ret = -EINVAL;
> +
> +	ret = v4l2_device_register(dev, &fd->v4l2_dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register v4l2 device\n");
> +		return ret;
> +	}
> +
> +	fd->m2m_dev = v4l2_m2m_init(&fd_m2m_ops);
> +	if (IS_ERR(fd->m2m_dev)) {
> +		dev_err(dev, "Failed to init mem2mem device\n");
> +		ret = PTR_ERR(fd->m2m_dev);
> +		goto err_unreg_v4l2_dev;
> +	}
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	mdev->dev = dev;
> +	strscpy(mdev->model, dev_driver_string(dev), sizeof(mdev->model));
> +	media_device_init(mdev);
> +	mdev->ops = &fd_m2m_media_ops;
> +	fd->v4l2_dev.mdev = mdev;
> +#endif
> +
> +	ret = mtk_aie_video_device_register(fd);
> +	if (ret)
> +		goto err_cleanup_mdev;
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER

Why do you have so many ifdefs?

> +	ret = media_device_register(mdev);
> +	if (ret) {
> +		dev_err(dev, "Failed to register mem2mem media device\n");
> +		goto err_unreg_vdev;
> +	}
> +#endif
> +	return 0;
> +
> +#ifdef CONFIG_MEDIA_CONTROLLER

No, that's unreadable code.

> +err_unreg_vdev:
> +	v4l2_m2m_unregister_media_controller(fd->m2m_dev);
> +	video_unregister_device(&fd->vfd);
> +#endif
> +err_cleanup_mdev:
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	media_device_cleanup(mdev);
> +#endif
> +	v4l2_m2m_release(fd->m2m_dev);
> +err_unreg_v4l2_dev:
> +	v4l2_device_unregister(&fd->v4l2_dev);
> +	return ret;
> +}
> +
> +static void mtk_aie_video_device_unregister(struct mtk_aie_dev *fd)
> +{
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	v4l2_m2m_unregister_media_controller(fd->m2m_dev);
> +#endif
> +	video_unregister_device(&fd->vfd);
> +#ifdef CONFIG_MEDIA_CONTROLLER
> +	media_device_cleanup(&fd->mdev);
> +#endif
> +	v4l2_m2m_release(fd->m2m_dev);
> +	v4l2_device_unregister(&fd->v4l2_dev);
> +}
> +

...

> +
> +static int mtk_aie_probe(struct platform_device *pdev)
> +{
> +	struct mtk_aie_dev *fd = NULL;

Why do you assign it?

> +	struct device *dev = &pdev->dev;
> +

Drop

> +	int irq = -1;

What is this style?

> +	int ret = -EINVAL;

And here?

> +
> +	fd = devm_kzalloc(&pdev->dev, sizeof(struct mtk_aie_dev), GFP_KERNEL);

sizeof(*)

Don't send downstream junk code. There is no reason why reviewers have
to correct the same mistakes which were already corrected.

> +	if (!fd)
> +		return -ENOMEM;
> +
> +	fd->variant = mtk_aie_get_variant(dev);
> +	if (!fd->variant)
> +		return -ENODEV;
> +
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(34));
> +	if (ret) {
> +		dev_err(dev, "%s: No suitable DMA available\n", __func__);
> +		return ret;
> +	}
> +
> +	dev_set_drvdata(dev, fd);
> +	fd->dev = dev;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		dev_err(dev, "Failed to get irq by platform: %d\n", irq);
> +		return irq;
> +	}
> +
> +	ret = devm_request_irq(dev,
> +			       irq,
> +			       mtk_aie_irq,
> +			       IRQF_SHARED,
> +			       dev_driver_string(dev),
> +			       fd
> +		);

That's an unreadable crap.

> +	if (ret) {
> +		dev_err(dev, "Failed to request irq\n");
> +		return ret;
> +	}
> +	fd->irq = irq;
> +
> +	fd->fd_base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(fd->fd_base)) {
> +		dev_err(dev, "Failed to get fd reg base\n");
> +		return PTR_ERR(fd->fd_base);
> +	}
> +
> +	fd->aie_clk.clk_num = ARRAY_SIZE(aie_clks);
> +	fd->aie_clk.clks = aie_clks;
> +	ret = devm_clk_bulk_get(&pdev->dev, fd->aie_clk.clk_num, fd->aie_clk.clks);
> +	if (ret) {
> +		dev_err(dev, "failed to get raw clock:%d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mtk_aie_debugfs_init(fd);
> +	if (ret)
> +		goto err_free;
> +
> +	ret = mtk_aie_resource_init(fd);
> +	if (ret)
> +		goto err_free;

Why do you disable runtime PM? This does not make sense.

> +	pm_runtime_enable(dev);
> +	ret = mtk_aie_dev_v4l2_init(fd);
> +	if (ret)
> +		goto err_free;
> +	dev_info(dev, "AIE : Success to %s\n", __func__);

No, drop. Srsly.

> +
> +	return 0;
> +
> +err_free:
> +	pm_runtime_disable(&pdev->dev);
> +	mtk_aie_resource_free(pdev);
> +
> +	return ret;
> +}
> +
> +static void mtk_aie_remove(struct platform_device *pdev)
> +{
> +	struct mtk_aie_dev *fd = dev_get_drvdata(&pdev->dev);
> +
> +	mtk_aie_video_device_unregister(fd);
> +	pm_runtime_disable(&pdev->dev);
> +	mtk_aie_debugfs_free(pdev);
> +	mtk_aie_resource_free(pdev);
> +
> +#ifdef CONFIG_DEBUG_FS
> +	kfree(fd->tr_info.dump_buffer);
> +#endif
> +}
> +
> +static int mtk_aie_suspend(struct device *dev)
> +{
> +	struct mtk_aie_dev *fd = dev_get_drvdata(dev);
> +	int ret = -EINVAL, num = 0;
> +
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	num = atomic_read(&fd->num_composing);
> +	dev_info(dev, "%s: suspend aie job start, num(%d)\n", __func__, num);

NAK, drop.

> +
> +	ret = wait_event_timeout(fd->flushing_waitq,
> +				 !(num = atomic_read(&fd->num_composing)),
> +				 msecs_to_jiffies(MTK_FD_HW_TIMEOUT_IN_MSEC)
> +		);
> +	if (!ret && num) {
> +		dev_dbg(dev,
> +			"%s: flushing aie job timeout num %d\n",
> +			__func__,
> +			num
> +		);
> +
> +		return -EBUSY;
> +	}
> +
> +	dev_info(dev, "%s: suspend aie job end num(%d)\n", __func__, num);

NAK

> +
> +	ret = pm_runtime_force_suspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int mtk_aie_resume(struct device *dev)
> +{
> +	int ret = -EINVAL;
> +
> +	dev_info(dev, "%s: resume aie job start)\n", __func__);

Omg, you keep going...


> +
> +static struct platform_driver mtk_aie_driver = {
> +	.probe = mtk_aie_probe,
> +	.remove = mtk_aie_remove,
> +	.driver = {
> +			.name = "mtk-aie-5.3",

Messed indentation

> +			.of_match_table = of_match_ptr(mtk_aie_of_ids),

Test your code. You have warnings here - drop of_match_ptr.


I am not going to review the rest. Sorry, this code is very poor quality
and does not meet criteria of sending upstream.

Do not, really, do not throw downstream code at the community.

Best regards,
Krzysztof

Powered by blists - more mailing lists