Re: [RFC, V2, 09/11] media: platform: Add Mediatek ISP P1 device driver

[Jungo Lin <jungo.lin@mediatek.com>]

Hi, Drew:


On Fri, 2019-05-24 at 15:19 -0600, Drew Davenport wrote:
> Hi Jungo,
> 
> On Fri, May 10, 2019 at 09:58:04AM +0800, Jungo Lin wrote:
> > This patch adds the Mediatek ISP P1 HW control device driver.
> > It handles the ISP HW configuration, provides interrupt handling and
> > initializes the V4L2 device nodes and other functions.
> 
> A few comments inline.
> 

Appreciate your feedback on this patch set firstly.

> > 
> > Signed-off-by: Jungo Lin <jungo.lin@mediatek.com>
> > ---
> >  .../mtk-isp/isp_50/cam/mtk_cam-regs.h         |  149 ++
> >  .../platform/mtk-isp/isp_50/cam/mtk_cam.c     | 1206 +++++++++++++++++
> >  .../platform/mtk-isp/isp_50/cam/mtk_cam.h     |  300 ++++
> >  3 files changed, 1655 insertions(+)
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c
> >  create mode 100644 drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h
> > 
> > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h
> > new file mode 100644
> > index 000000000000..342f0e0e9837
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam-regs.h
> > @@ -0,0 +1,149 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Ryan Yu <ryan.yu@mediatek.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#ifndef _CAM_REGS_H
> > +#define _CAM_REGS_H
> > +
> > +/* TG Bit Mask */
> > +#define VFDATA_EN_BIT	BIT(0)
> > +#define CMOS_EN_BIT	BIT(0)
> > +
> > +/* normal signal bit */
> > +#define VS_INT_ST	BIT(0)
> > +#define HW_PASS1_DON_ST	BIT(11)
> > +#define SOF_INT_ST	BIT(12)
> > +#define SW_PASS1_DON_ST	BIT(30)
> > +
> > +/* err status bit */
> > +#define TG_ERR_ST	BIT(4)
> > +#define TG_GBERR_ST	BIT(5)
> > +#define CQ_CODE_ERR_ST	BIT(6)
> > +#define CQ_APB_ERR_ST	BIT(7)
> > +#define CQ_VS_ERR_ST	BIT(8)
> > +#define AMX_ERR_ST	BIT(15)
> > +#define RMX_ERR_ST	BIT(16)
> > +#define BMX_ERR_ST	BIT(17)
> > +#define RRZO_ERR_ST	BIT(18)
> > +#define AFO_ERR_ST	BIT(19)
> > +#define IMGO_ERR_ST	BIT(20)
> > +#define AAO_ERR_ST	BIT(21)
> > +#define PSO_ERR_ST	BIT(22)
> > +#define LCSO_ERR_ST	BIT(23)
> > +#define BNR_ERR_ST	BIT(24)
> > +#define LSCI_ERR_ST	BIT(25)
> > +#define DMA_ERR_ST	BIT(29)
> > +
> > +/* CAM DMA done status */
> > +#define FLKO_DONE_ST	BIT(4)
> > +#define AFO_DONE_ST	BIT(5)
> > +#define AAO_DONE_ST	BIT(7)
> > +#define PSO_DONE_ST	BIT(14)
> 
> Please align the values using tabs here and elsewhere.
> 

Ok, we will revise this coding style issue in next patch.

> > +
> > +/* IRQ signal mask */
> > +#define INT_ST_MASK_CAM	( \
> > +			VS_INT_ST |\
> > +			SOF_INT_ST |\
> > +			HW_PASS1_DON_ST |\
> > +			SW_PASS1_DON_ST)
> > +
> > +/* IRQ Warning Mask */
> > +#define INT_ST_MASK_CAM_WARN	(\
> > +				RRZO_ERR_ST |\
> > +				AFO_ERR_ST |\
> > +				IMGO_ERR_ST |\
> > +				AAO_ERR_ST |\
> > +				PSO_ERR_ST | \
> > +				LCSO_ERR_ST |\
> > +				BNR_ERR_ST |\
> > +				LSCI_ERR_ST)
> > +
> > +/* IRQ Error Mask */
> > +#define INT_ST_MASK_CAM_ERR	(\
> > +				TG_ERR_ST |\
> > +				TG_GBERR_ST |\
> > +				CQ_CODE_ERR_ST |\
> > +				CQ_APB_ERR_ST |\
> > +				CQ_VS_ERR_ST |\
> > +				BNR_ERR_ST |\
> > +				RMX_ERR_ST |\
> > +				BMX_ERR_ST |\
> > +				BNR_ERR_ST |\
> > +				LSCI_ERR_ST |\
> > +				DMA_ERR_ST)
> > +
> > +/* IRQ Signal Log Mask */
> > +#define INT_ST_LOG_MASK_CAM	(\
> > +				SOF_INT_ST |\
> > +				SW_PASS1_DON_ST |\
> > +				VS_INT_ST |\
> > +				TG_ERR_ST |\
> > +				TG_GBERR_ST |\
> > +				RRZO_ERR_ST |\
> > +				AFO_ERR_ST |\
> > +				IMGO_ERR_ST |\
> > +				AAO_ERR_ST |\
> > +				DMA_ERR_ST)
> > +
> > +/* DMA Event Notification Mask */
> > +#define DMA_ST_MASK_CAM	(\
> > +			AFO_DONE_ST |\
> > +			AAO_DONE_ST |\
> > +			PSO_DONE_ST |\
> > +			FLKO_DONE_ST)
> > +
> > +/* Status check */
> > +#define REG_CTL_EN		0x0004
> > +#define REG_CTL_DMA_EN		0x0008
> > +#define REG_CTL_FMT_SEL		0x0010
> > +#define REG_CTL_EN2		0x0018
> > +#define REG_CTL_RAW_INT_EN	0x0020
> > +#define REG_CTL_RAW_INT_STAT	0x0024
> > +#define REG_CTL_RAW_INT2_STAT	0x0034
> > +#define REG_CTL_RAW_INT3_STAT	0x00c4
> > +#define REG_CTL_TWIN_STAT	0x0050
> > +
> > +#define REG_TG_SEN_MODE		0x0230
> > +#define REG_TG_SEN_GRAB_PIX	0x0238
> > +#define REG_TG_SEN_GRAB_LIN	0x023c
> > +#define REG_TG_VF_CON		0x0234
> > +#define REG_TG_SUB_PERIOD	0x02a4
> > +
> > +#define REG_IMGO_BASE_ADDR	0x1020
> > +#define REG_RRZO_BASE_ADDR	0x1050
> > +
> > +/* Error status log */
> > +#define REG_IMGO_ERR_STAT	0x1360
> > +#define REG_RRZO_ERR_STAT	0x1364
> > +#define REG_AAO_ERR_STAT	0x1368
> > +#define REG_AFO_ERR_STAT	0x136c
> > +#define REG_LCSO_ERR_STAT	0x1370
> > +#define REG_UFEO_ERR_STAT	0x1374
> > +#define REG_PDO_ERR_STAT	0x1378
> > +#define REG_BPCI_ERR_STAT	0x137c
> > +#define REG_LSCI_ERR_STAT	0x1384
> > +#define REG_PDI_ERR_STAT	0x138c
> > +#define REG_LMVO_ERR_STAT	0x1390
> > +#define REG_FLKO_ERR_STAT	0x1394
> > +#define REG_PSO_ERR_STAT	0x13a0
> > +
> > +/* ISP command */
> > +#define REG_CQ_THR0_BASEADDR	0x0198
> > +#define REG_HW_FRAME_NUM	0x13b8
> > +
> > +/* META */
> > +#define REG_META0_VB2_INDEX	0x14dc
> > +#define REG_META1_VB2_INDEX	0x151c
> > +
> > +#endif	/* _CAM_REGS_H */
> > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c
> > new file mode 100644
> > index 000000000000..fc874ec8f7f0
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.c
> > @@ -0,0 +1,1206 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Ryan Yu <ryan.yu@mediatek.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/atomic.h>
> > +#include <linux/cdev.h>
> > +#include <linux/compat.h>
> > +#include <linux/fs.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/kernel.h>
> > +#include <linux/ktime.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/of_address.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/mtk_scp.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/remoteproc.h>
> > +#include <linux/sched/clock.h>
> > +#include <linux/spinlock.h>
> > +#include <linux/types.h>
> > +#include <linux/videodev2.h>
> > +#include <linux/vmalloc.h>
> > +
> > +#include "mtk_cam.h"
> > +#include "mtk_cam-regs.h"
> > +#include "mtk_cam-smem.h"
> > +
> > +static const struct of_device_id mtk_isp_of_ids[] = {
> > +	{.compatible = "mediatek,mt8183-camisp",},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_isp_of_ids);
> > +
> > +/* list of clocks required by isp cam */
> > +static const char * const mtk_isp_clks[] = {
> > +	"CAMSYS_CAM_CGPDN", "CAMSYS_CAMTG_CGPDN"
> > +};
> > +
> > +static void isp_dump_dma_status(struct isp_device *isp_dev)
> > +{
> > +	dev_err(isp_dev->dev,
> > +		"IMGO:0x%x, RRZO:0x%x, AAO=0x%x, AFO=0x%x, LMVO=0x%x\n",
> > +		readl(isp_dev->regs + REG_IMGO_ERR_STAT),
> > +		readl(isp_dev->regs + REG_RRZO_ERR_STAT),
> > +		readl(isp_dev->regs + REG_AAO_ERR_STAT),
> > +		readl(isp_dev->regs + REG_AFO_ERR_STAT),
> > +		readl(isp_dev->regs + REG_LMVO_ERR_STAT));
> > +	dev_err(isp_dev->dev,
> > +		"LCSO=0x%x, PSO=0x%x, FLKO=0x%x, BPCI:0x%x, LSCI=0x%x\n",
> > +		readl(isp_dev->regs + REG_LCSO_ERR_STAT),
> > +		readl(isp_dev->regs + REG_PSO_ERR_STAT),
> > +		readl(isp_dev->regs + REG_FLKO_ERR_STAT),
> > +		readl(isp_dev->regs + REG_BPCI_ERR_STAT),
> > +		readl(isp_dev->regs + REG_LSCI_ERR_STAT));
> > +}
> > +
> > +static void mtk_isp_notify(struct mtk_isp_p1_ctx *isp_ctx,
> > +			   unsigned int request_fd,
> > +			   unsigned int frame_seq_no,
> > +			   struct list_head *list_buf,
> > +			   enum vb2_buffer_state state)
> > +{
> > +	struct isp_p1_device *p1_dev = p1_ctx_to_dev(isp_ctx);
> > +	struct device *dev = &p1_dev->pdev->dev;
> > +	struct mtk_cam_dev_finish_param fram_param;
> > +
> > +	fram_param.list_buf = list_buf;
> > +	fram_param.request_fd = request_fd;
> > +	fram_param.frame_seq_no = frame_seq_no;
> > +	fram_param.state = state;
> > +	dev_dbg(dev, "request fd:%d frame_seq_no:%d\n",
> > +		fram_param.request_fd,
> > +		fram_param.frame_seq_no);
> > +	mtk_cam_dev_job_finish(p1_dev->cam_dev, &fram_param);
> > +}
> > +
> > +static void isp_deque_frame(struct isp_p1_device *p1_dev,
> > +			    unsigned int node_id, int vb2_index,
> > +			    int frame_seq_no)
> > +{
> > +	struct mtk_cam_dev *cam_dev = p1_dev->cam_dev;
> > +	struct device *dev = &p1_dev->pdev->dev;
> > +	struct vb2_queue *vb2_queue = &cam_dev->mem2mem2_nodes[node_id].vbq;
> > +	struct vb2_buffer *vb;
> > +	struct vb2_v4l2_buffer *vbb;
> > +
> > +	if (!cam_dev->mem2mem2_nodes[node_id].enabled)
> > +		return;
> > +
> > +	mutex_lock(vb2_queue->lock);
> > +	list_for_each_entry(vb, &vb2_queue->queued_list, queued_entry) {
> > +		vbb = to_vb2_v4l2_buffer(vb);
> > +		if (vbb->request_fd < 0 &&
> > +		    vb->index == vb2_index &&
> > +		    vb->state == VB2_BUF_STATE_ACTIVE) {
> > +			dev_dbg(dev, "%s:%d:%d", __func__, node_id, vb2_index);
> > +			vbb->vb2_buf.timestamp = ktime_get_ns();
> > +			vbb->sequence = frame_seq_no;
> > +			vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> > +		}
> > +	}
> > +	mutex_unlock(vb2_queue->lock);
> > +}
> > +
> > +static void isp_deque_request_frame(struct isp_p1_device *p1_dev,
> > +				    int frame_seq_no)
> > +{
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	struct device *dev = &p1_dev->pdev->dev;
> > +	struct mtk_isp_queue_job *framejob, *tmp;
> > +	struct isp_queue *p1_enqueue_list = &isp_ctx->p1_enqueue_list;
> > +
> > +	/* Match dequeue work and enqueue frame */
> > +	spin_lock(&p1_enqueue_list->lock);
> > +	list_for_each_entry_safe(framejob, tmp, &p1_enqueue_list->queue,
> > +				 list_entry) {
> > +		dev_dbg(dev,
> > +			"%s frame_seq_no:%d, target frame_seq_no:%d\n",
> > +			__func__,
> > +			framejob->frame_seq_no, frame_seq_no);
> > +		/* Match by the en-queued request number */
> > +		if (framejob->frame_seq_no == frame_seq_no) {
> > +			/* Pass to user space */
> > +			mtk_isp_notify(isp_ctx,
> > +				       framejob->request_fd,
> > +				       framejob->frame_seq_no,
> > +				       &framejob->list_buf,
> > +				       VB2_BUF_STATE_DONE);
> > +			atomic_dec(&p1_enqueue_list->queue_cnt);
> > +			dev_dbg(dev,
> > +				"frame_seq_no:%d is done, queue_cnt:%d\n",
> > +				framejob->frame_seq_no,
> > +				atomic_read(&p1_enqueue_list->queue_cnt));
> > +
> > +			/* remove only when frame ready */
> > +			list_del(&framejob->list_entry);
> > +			kfree(framejob);
> > +			break;
> > +		} else if (framejob->frame_seq_no < frame_seq_no) {
> > +			/* Pass to user space for frame drop */
> > +			mtk_isp_notify(isp_ctx,
> > +				       framejob->request_fd,
> > +				       framejob->frame_seq_no,
> > +				       &framejob->list_buf,
> > +				       VB2_BUF_STATE_ERROR);
> > +			atomic_dec(&p1_enqueue_list->queue_cnt);
> > +			dev_dbg(dev,
> > +				"frame_seq_no:%d drop, queue_cnt:%d\n",
> > +				framejob->frame_seq_no,
> > +				atomic_read(&p1_enqueue_list->queue_cnt));
> > +
> > +			/* remove only drop frame */
> > +			list_del(&framejob->list_entry);
> > +			kfree(framejob);
> > +		}
> > +	}
> > +	spin_unlock(&p1_enqueue_list->lock);
> > +}
> > +
> > +static int isp_deque_work(void *data)
> > +{
> > +	struct isp_p1_device *p1_dev = (struct isp_p1_device *)data;
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	struct device *dev = &p1_dev->pdev->dev;
> > +	struct mtk_cam_dev *cam_dev = p1_dev->cam_dev;
> > +	struct mtk_cam_dev_stat_event_data event_data;
> > +	atomic_t *irq_data_end = &isp_ctx->irq_data_end;
> > +	atomic_t *irq_data_start = &isp_ctx->irq_data_start;
> > +	unsigned long flags;
> > +	int ret, i;
> > +
> > +	while (1) {
> > +		ret = wait_event_interruptible(isp_ctx->isp_deque_thread.wq,
> > +					       (atomic_read(irq_data_end) !=
> > +					       atomic_read(irq_data_start)) ||
> > +					       kthread_should_stop());
> > +
> > +		if (kthread_should_stop())
> > +			break;
> > +
> > +		if (ret == ERESTARTSYS) {
> > +			dev_err(dev, "interrupted by a signal!\n");
> > +			continue;
> > +		}
> > +
> > +		spin_lock_irqsave(&isp_ctx->irq_dequeue_lock, flags);
> > +		i = atomic_read(&isp_ctx->irq_data_start);
> > +		memcpy(&event_data, &isp_ctx->irq_event_datas[i],
> > +		       sizeof(event_data));
> > +		memset(&isp_ctx->irq_event_datas[i], 0x00, sizeof(event_data));
> > +		atomic_set(&isp_ctx->irq_data_start, ++i & 0x3);
> > +		spin_unlock_irqrestore(&isp_ctx->irq_dequeue_lock, flags);
> > +
> > +		if (event_data.irq_status_mask & VS_INT_ST) {
> > +			/* Notify specific HW events to user space */
> > +			mtk_cam_dev_event_frame_sync(cam_dev,
> > +						     event_data.frame_seq_no);
> > +			dev_dbg(dev,
> > +				"event IRQ:0x%x DMA:0x%x is sent\n",
> > +				event_data.irq_status_mask,
> > +				event_data.dma_status_mask);
> > +		}
> > +
> > +		if (event_data.dma_status_mask & AAO_DONE_ST) {
> > +			isp_deque_frame(p1_dev,
> > +					MTK_CAM_P1_META_OUT_0,
> > +					event_data.meta0_vb2_index,
> > +					event_data.frame_seq_no);
> > +		}
> > +
> > +		if (event_data.irq_status_mask & SW_PASS1_DON_ST) {
> > +			isp_deque_frame(p1_dev,
> > +					MTK_CAM_P1_META_OUT_0,
> > +					event_data.meta0_vb2_index,
> > +					event_data.frame_seq_no);
> > +
> > +			isp_deque_request_frame(p1_dev,
> > +						event_data.frame_seq_no);
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int irq_handle_sof(struct isp_device *isp_dev,
> > +			  dma_addr_t base_addr,
> > +			  unsigned int frame_num)
> > +{
> > +	unsigned int cq_addr_index;
> > +	struct isp_p1_device *p1_dev = get_p1_device(isp_dev->dev);
> > +	int cq_num = atomic_read(&p1_dev->isp_ctx.composed_frame_id);
> > +
> > +	if (cq_num > frame_num) {
> > +		cq_addr_index = frame_num % CQ_BUFFER_COUNT;
> > +
> > +		writel(base_addr +
> > +			(dma_addr_t)(CQ_ADDRESS_OFFSET * cq_addr_index),
> > +			isp_dev->regs + REG_CQ_THR0_BASEADDR);
> > +		dev_dbg(isp_dev->dev,
> > +			"SOF_INT_ST, update next, cq_num:%d, frame_num:%d cq_addr:%d",
> > +			cq_num, frame_num, cq_addr_index);
> > +	} else {
> > +		dev_dbg(isp_dev->dev,
> > +			"SOF_INT_ST, wait next, cq_num:%d, frame_num:%d",
> > +			cq_num, frame_num);
> > +	}
> > +
> > +	isp_dev->sof_count += 1;
> > +
> > +	return cq_num;
> > +}
> > +
> > +static int irq_handle_notify_event(struct isp_device *isp_dev,
> > +				   unsigned int irqstatus,
> > +				   unsigned int dmastatus)
> > +{
> > +	struct isp_p1_device *p1_dev = get_p1_device(isp_dev->dev);
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	unsigned long flags;
> > +	int i;
> > +
> > +	spin_lock_irqsave(&isp_ctx->irq_dequeue_lock, flags);
> > +	i = atomic_read(&isp_ctx->irq_data_end);
> > +	isp_ctx->irq_event_datas[i].frame_seq_no = isp_dev->current_frame;
> > +	isp_ctx->irq_event_datas[i].meta0_vb2_index = isp_dev->meta0_vb2_index;
> > +	isp_ctx->irq_event_datas[i].meta1_vb2_index = isp_dev->meta1_vb2_index;
> > +	isp_ctx->irq_event_datas[i].irq_status_mask |=
> > +		(irqstatus & INT_ST_MASK_CAM);
> > +	isp_ctx->irq_event_datas[i].dma_status_mask |=
> > +		(dmastatus & DMA_ST_MASK_CAM);
> > +	atomic_set(&isp_ctx->irq_data_end, ++i & 0x3);
> > +	spin_unlock_irqrestore(&isp_ctx->irq_dequeue_lock, flags);
> > +
> > +	wake_up_interruptible(&isp_ctx->isp_deque_thread.wq);
> > +
> > +	dev_dbg(isp_dev->dev,
> > +		"%s IRQ:0x%x DMA:0x%x seq:%d idx0:%d idx1:%d\n",
> > +		__func__,
> > +		(irqstatus & INT_ST_MASK_CAM),
> > +		(dmastatus & DMA_ST_MASK_CAM),
> > +		isp_dev->current_frame,
> > +		isp_dev->meta0_vb2_index,
> > +		isp_dev->meta1_vb2_index);
> > +
> > +	return 0;
> > +}
> > +
> > +irqreturn_t isp_irq_cam(int irq, void *data)
> > +{
> > +	struct isp_device *isp_dev = (struct isp_device *)data;
> > +	struct isp_p1_device *p1_dev = get_p1_device(isp_dev->dev);
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	struct device *dev = isp_dev->dev;
> > +	unsigned int cardinalnum, cq_num, hw_frame_num;
> > +	unsigned int meta0_vb2_index, meta1_vb2_index;
> > +	unsigned int irqstatus, errstatus, warnstatus, dmastatus;
> > +	unsigned long flags;
> > +
> > +	/* Check the streaming is off or not */
> > +	if (!p1_dev->cam_dev->streaming)
> > +		return IRQ_HANDLED;
> > +
> > +	cardinalnum = isp_dev->isp_hw_module - ISP_CAM_A_IDX;
> > +	cq_num = 0;
> > +
> > +	spin_lock_irqsave(&isp_dev->spinlock_irq, flags);
> > +	irqstatus = readl(isp_dev->regs + REG_CTL_RAW_INT_STAT);
> > +	dmastatus = readl(isp_dev->regs + REG_CTL_RAW_INT2_STAT);
> > +	hw_frame_num = readl(isp_dev->regs + REG_HW_FRAME_NUM);
> > +	meta0_vb2_index = readl(isp_dev->regs + REG_META0_VB2_INDEX);
> > +	meta1_vb2_index = readl(isp_dev->regs + REG_META1_VB2_INDEX);
> > +	spin_unlock_irqrestore(&isp_dev->spinlock_irq, flags);
> > +
> > +	/* Ignore unnecessary IRQ */
> > +	if (irqstatus == 0)
> > +		return IRQ_HANDLED;
> > +
> > +	errstatus = irqstatus & INT_ST_MASK_CAM_ERR;
> > +	warnstatus = irqstatus & INT_ST_MASK_CAM_WARN;
> > +	irqstatus = irqstatus & INT_ST_MASK_CAM;
> > +
> > +	/* sof , done order check . */
> > +	spin_lock_irqsave(&isp_dev->spinlock_irq, flags);
> > +	if ((irqstatus & HW_PASS1_DON_ST) && (irqstatus & SOF_INT_ST)) {
> > +		dev_warn(dev,
> > +			 "isp sof_don block, sof_cnt:%d\n",
> > +			 isp_dev->sof_count);
> > +
> > +		/* Notify IRQ event and enqueue ready frame */
> > +		irq_handle_notify_event(isp_dev, irqstatus, dmastatus);
> > +		isp_dev->current_frame = hw_frame_num;
> > +		isp_dev->meta0_vb2_index = meta0_vb2_index;
> > +		isp_dev->meta1_vb2_index = meta1_vb2_index;
> > +	} else {
> > +		if (irqstatus & SOF_INT_ST) {
> > +			isp_dev->current_frame = hw_frame_num;
> > +			isp_dev->meta0_vb2_index = meta0_vb2_index;
> > +			isp_dev->meta1_vb2_index = meta1_vb2_index;
> > +		}
> > +
> > +		if ((irqstatus & INT_ST_MASK_CAM) ||
> > +		    (dmastatus & DMA_ST_MASK_CAM))
> > +			irq_handle_notify_event(isp_dev, irqstatus, dmastatus);
> > +	}
> > +	spin_unlock_irqrestore(&isp_dev->spinlock_irq, flags);
> > +
> > +	if (irqstatus & SOF_INT_ST)
> > +		cq_num = irq_handle_sof(isp_dev, isp_ctx->scp_mem_iova,
> > +					hw_frame_num);
> > +
> > +	if (irqstatus & SW_PASS1_DON_ST) {
> > +		int num = atomic_dec_return(&isp_ctx->composing_frame);
> > +
> > +		dev_dbg(dev, "SW_PASS1_DON_ST queued frame:%d\n", num);
> > +		/* Notify TX thread to send if TX frame is blocked */
> > +		wake_up_interruptible
> > +				(&isp_ctx->composer_tx_thread.wq);
> > +	}
> > +
> > +	/* check ISP error status */
> > +	if (errstatus) {
> > +		dev_err(dev,
> > +			"raw_int_err:0x%x/0x%x/0x%x\n",
> > +			irqstatus, warnstatus, errstatus);
> > +
> > +		/* show DMA errors in detail */
> > +		if (errstatus & DMA_ERR_ST)
> > +			isp_dump_dma_status(isp_dev);
> > +	}
> > +
> > +	if (irqstatus & INT_ST_LOG_MASK_CAM)
> > +		dev_dbg(dev, IRQ_STAT_STR,
> > +			'A' + cardinalnum,
> > +			isp_dev->sof_count,
> > +			irqstatus,
> > +			dmastatus,
> > +			hw_frame_num,
> > +			cq_num);
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int enable_sys_clock(struct isp_p1_device *p1_dev)
> > +{
> > +	struct device *dev = &p1_dev->pdev->dev;
> > +	int ret;
> > +
> > +	dev_info(dev, "- %s dev id:%d\n", __func__, dev->id);
> > +
> > +	ret = clk_bulk_prepare_enable(p1_dev->isp_clk.num_clks,
> > +				      p1_dev->isp_clk.clk_list);
> > +	if (ret < 0)
> > +		goto clk_err;
> > +	return 0;
> > +clk_err:
> > +	dev_err(dev, "cannot pre-en isp_cam clock:%d\n", ret);
> > +	clk_bulk_disable_unprepare(p1_dev->isp_clk.num_clks,
> > +				   p1_dev->isp_clk.clk_list);
> > +	return ret;
> > +}
> > +
> > +static void disable_sys_clock(struct isp_p1_device *p1_dev)
> > +{
> > +	struct device *dev = &p1_dev->pdev->dev;
> > +
> > +	dev_info(dev, "- %s dev id:%d\n", __func__, dev->id);
> > +	clk_bulk_disable_unprepare(p1_dev->isp_clk.num_clks,
> > +				   p1_dev->isp_clk.clk_list);
> > +}
> > +
> > +static int mtk_isp_probe(struct platform_device *pdev)
> > +{
> > +	struct isp_p1_device *p1_dev;
> > +	struct mtk_isp_p1_ctx *isp_ctx;
> > +	struct isp_device *isp_dev;
> > +	struct device *dev = &pdev->dev;
> > +	struct resource *res;
> > +	int ret;
> > +	unsigned int i;
> > +
> > +	/* Allocate context */
> > +	p1_dev = devm_kzalloc(dev, sizeof(*p1_dev), GFP_KERNEL);
> > +	if (!p1_dev)
> > +		return -ENOMEM;
> > +
> > +	dev_set_drvdata(dev, p1_dev);
> > +	isp_ctx = &p1_dev->isp_ctx;
> > +	p1_dev->pdev = pdev;
> > +
> > +	p1_dev->isp_devs =
> > +		devm_kzalloc(dev,
> > +			     sizeof(struct isp_device) * ISP_DEV_NODE_NUM,
> > +			     GFP_KERNEL);
> > +	if (!p1_dev->isp_devs)
> > +		return -ENOMEM;
> > +
> > +	p1_dev->cam_dev =
> > +		devm_kzalloc(dev, sizeof(struct mtk_cam_dev), GFP_KERNEL);
> > +	if (!p1_dev->isp_devs)
> > +		return -ENOMEM;
> > +
> > +	/* iomap registers */
> > +	for (i = ISP_CAMSYS_CONFIG_IDX; i < ISP_DEV_NODE_NUM; i++) {
> > +		isp_dev = &p1_dev->isp_devs[i];
> > +		isp_dev->isp_hw_module = i;
> > +		isp_dev->dev = dev;
> > +		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
> > +		isp_dev->regs = devm_ioremap_resource(dev, res);
> > +
> > +		dev_info(dev, "cam%u, map_addr=0x%lx\n",
> > +			 i, (unsigned long)isp_dev->regs);
> > +
> > +		if (!isp_dev->regs)
> > +			return PTR_ERR(isp_dev->regs);
> > +
> > +		/* support IRQ from ISP_CAM_A_IDX */
> > +		if (i >= ISP_CAM_A_IDX) {
> > +			/* reg & interrupts index is shifted with 1  */
> > +			isp_dev->irq = platform_get_irq(pdev, i - 1);
> > +			if (isp_dev->irq > 0) {
> > +				ret = devm_request_irq(dev, isp_dev->irq,
> > +						       isp_irq_cam,
> > +						       IRQF_SHARED,
> > +						       dev_driver_string(dev),
> > +						       (void *)isp_dev);
> > +				if (ret) {
> > +					dev_err(dev,
> > +						"req_irq fail, dev:%s irq=%d\n",
> > +						dev->of_node->name,
> > +						isp_dev->irq);
> > +					return ret;
> > +				}
> > +				dev_info(dev, "Registered irq=%d, ISR:%s\n",
> > +					 isp_dev->irq, dev_driver_string(dev));
> > +			}
> > +		}
> > +		spin_lock_init(&isp_dev->spinlock_irq);
> > +	}
> > +
> > +	p1_dev->isp_clk.num_clks = ARRAY_SIZE(mtk_isp_clks);
> > +	p1_dev->isp_clk.clk_list =
> > +		devm_kcalloc(dev,
> > +			     p1_dev->isp_clk.num_clks,
> > +			     sizeof(*p1_dev->isp_clk.clk_list),
> > +			     GFP_KERNEL);
> > +	if (!p1_dev->isp_clk.clk_list)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < p1_dev->isp_clk.num_clks; ++i)
> > +		p1_dev->isp_clk.clk_list->id = mtk_isp_clks[i];
> > +
> > +	ret = devm_clk_bulk_get(dev,
> > +				p1_dev->isp_clk.num_clks,
> > +				p1_dev->isp_clk.clk_list);
> > +	if (ret) {
> > +		dev_err(dev, "cannot get isp cam clock:%d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	/* Initialize reserved DMA memory */
> > +	ret = mtk_cam_reserved_memory_init(p1_dev);
> > +	if (ret) {
> > +		dev_err(dev, "failed to configure DMA memory\n");
> > +		return ret;
> > +	}
> > +
> > +	/* Initialize the v4l2 common part */
> > +	ret = mtk_cam_dev_init(pdev, p1_dev->cam_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	spin_lock_init(&isp_ctx->p1_enqueue_list.lock);
> > +	atomic_set(&p1_dev->isp_ctx.isp_user_cnt, 0);
> > +	pm_runtime_enable(dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_isp_remove(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	struct isp_p1_device *p1_dev = dev_get_drvdata(dev);
> > +
> > +	pm_runtime_disable(dev);
> > +	mtk_cam_dev_release(pdev, p1_dev->cam_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_isp_suspend(struct device *dev)
> > +{
> > +	struct isp_p1_device *p1_dev = get_p1_device(dev);
> > +	struct isp_device *isp_dev;
> > +	unsigned int reg_val;
> > +	int usercount, module;
> > +
> > +	module = p1_dev->isp_ctx.isp_hw_module;
> > +	usercount = atomic_read(&p1_dev->isp_ctx.isp_user_cnt);
> > +
> > +	dev_dbg(dev, "- %s:%d\n", __func__, usercount);
> > +
> > +	/* If no user count, no further action */
> > +	if (!usercount)
> > +		return 0;
> > +
> > +	isp_dev = &p1_dev->isp_devs[module];
> > +	reg_val = readl(isp_dev->regs + REG_TG_VF_CON);
> > +	if (reg_val & VFDATA_EN_BIT) {
> > +		dev_dbg(dev, "Cam:%d suspend, disable VF\n", module);
> > +		/* disable VF */
> > +		writel((reg_val & (~VFDATA_EN_BIT)),
> > +		       isp_dev->regs + REG_TG_VF_CON);
> > +		/*
> > +		 * After VF enable, The TG frame count will be reset to 0;
> > +		 */
> > +		reg_val = readl(isp_dev->regs + REG_TG_SEN_MODE);
> > +		writel((reg_val & (~CMOS_EN_BIT)),
> > +		       isp_dev->regs +  + REG_TG_SEN_MODE);
> > +	}
> > +
> > +	disable_sys_clock(p1_dev);
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_isp_resume(struct device *dev)
> > +{
> > +	struct isp_p1_device *p1_dev = get_p1_device(dev);
> > +	struct isp_device *isp_dev;
> > +	unsigned int reg_val;
> > +	int module, usercount;
> > +
> > +	module = p1_dev->isp_ctx.isp_hw_module;
> > +	usercount = atomic_read(&p1_dev->isp_ctx.isp_user_cnt);
> > +
> > +	dev_dbg(dev, "- %s:%d\n", __func__, usercount);
> > +
> > +	/* If no user count, no further action */
> > +	if (!usercount)
> > +		return 0;
> > +
> > +	enable_sys_clock(p1_dev);
> > +
> > +	/* V4L2 stream-on phase & restore HW stream-on status */
> > +	if (p1_dev->cam_dev->streaming) {
> > +		isp_dev = &p1_dev->isp_devs[module];
> > +		dev_dbg(dev, "Cam:%d resume,enable VF\n", module);
> > +		/* Enable CMOS */
> > +		reg_val = readl(isp_dev->regs + REG_TG_SEN_MODE);
> > +		writel((reg_val | CMOS_EN_BIT),
> > +		       isp_dev->regs + REG_TG_SEN_MODE);
> > +		/* Enable VF */
> > +		reg_val = readl(isp_dev->regs + REG_TG_VF_CON);
> > +		writel((reg_val | VFDATA_EN_BIT),
> > +		       isp_dev->regs + REG_TG_VF_CON);
> > +	}
> > +	return 0;
> > +}
> > +
> > +static int isp_setup_scp_rproc(struct isp_p1_device *p1_dev)
> > +{
> > +	phandle rproc_phandle;
> > +	struct device *dev = &p1_dev->pdev->dev;
> > +	int ret;
> > +
> > +	p1_dev->scp_pdev = scp_get_pdev(p1_dev->pdev);
> > +	if (!p1_dev->scp_pdev) {
> > +		dev_err(dev, "Failed to get scp device\n");
> > +		return -ENODEV;
> > +	}
> > +	ret = of_property_read_u32(dev->of_node, "mediatek,scp",
> > +				   &rproc_phandle);
> > +	if (ret) {
> > +		dev_err(dev, "fail to get rproc_phandle:%d\n", ret);
> > +		return -EINVAL;
> > +	}
> > +
> > +	p1_dev->rproc_handle = rproc_get_by_phandle(rproc_phandle);
> > +	dev_dbg(dev, "p1 rproc_phandle: 0x%pK\n\n",
> > +		p1_dev->rproc_handle);
> > +	if (!p1_dev->rproc_handle) {
> > +		dev_err(dev, "fail to get rproc_handle\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = rproc_boot(p1_dev->rproc_handle);
> > +	if (ret < 0) {
> > +		/*
> > +		 * Return 0 if downloading firmware successfully,
> > +		 * otherwise it is failed
> > +		 */
> > +		return -ENODEV;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int isp_init_context(struct isp_p1_device *p1_dev)
> > +{
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	struct device *dev = &p1_dev->pdev->dev;
> > +	unsigned int i;
> > +
> > +	dev_dbg(dev, "init irq work thread\n");
> > +	if (!isp_ctx->isp_deque_thread.thread) {
> > +		mutex_init(&isp_ctx->composer_tx_lock);
> > +		init_waitqueue_head(&isp_ctx->isp_deque_thread.wq);
> > +		isp_ctx->isp_deque_thread.thread =
> > +			kthread_run(isp_deque_work, (void *)p1_dev,
> > +				    "isp_deque_work");
> > +		if (IS_ERR(isp_ctx->isp_deque_thread.thread)) {
> > +			dev_err(dev, "unable to alloc kthread\n");
> > +			isp_ctx->isp_deque_thread.thread = NULL;
> > +			return -ENOMEM;
> > +		}
> > +	}
> > +	spin_lock_init(&isp_ctx->irq_dequeue_lock);
> > +
> > +	INIT_LIST_HEAD(&isp_ctx->p1_enqueue_list.queue);
> > +	atomic_set(&isp_ctx->p1_enqueue_list.queue_cnt, 0);
> > +
> > +	for (i = 0; i < ISP_DEV_NODE_NUM; i++)
> > +		spin_lock_init(&p1_dev->isp_devs[i].spinlock_irq);
> > +
> > +	spin_lock_init(&isp_ctx->p1_enqueue_list.lock);
> > +	spin_lock_init(&isp_ctx->composer_txlist.lock);
> > +
> > +	atomic_set(&isp_ctx->irq_data_end, 0);
> > +	atomic_set(&isp_ctx->irq_data_start, 0);
> > +	return 0;
> > +}
> > +
> > +static int isp_uninit_context(struct isp_p1_device *p1_dev)
> > +{
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	struct mtk_isp_queue_job *framejob, *tmp_framejob;
> > +
> > +	spin_lock_irq(&isp_ctx->p1_enqueue_list.lock);
> > +	list_for_each_entry_safe(framejob, tmp_framejob,
> > +				 &isp_ctx->p1_enqueue_list.queue, list_entry) {
> > +		list_del(&framejob->list_entry);
> > +		kfree(framejob);
> > +	}
> > +	spin_unlock_irq(&isp_ctx->p1_enqueue_list.lock);
> > +
> > +	atomic_set(&isp_ctx->isp_user_cnt, 0);
> > +
> > +	if (!IS_ERR(isp_ctx->isp_deque_thread.thread)) {
> > +		kthread_stop(isp_ctx->isp_deque_thread.thread);
> > +		wake_up_interruptible(&isp_ctx->isp_deque_thread.wq);
> > +		isp_ctx->isp_deque_thread.thread = NULL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static unsigned int get_enable_dma_ports(struct mtk_cam_dev *cam_dev)
> 
> I think s/enable/enabled would be a clearer name for both the function
> and local variable.
> 

Ok, we will revised this "enabled" working in next patch.

> > +{
> > +	unsigned int enable_dma_ports, i;
> > +
> > +	/* Get the enabled meta DMA ports */
> > +	enable_dma_ports = 0;
> > +	for (i = 0; i < cam_dev->dev_node_num; i++) {
> > +		if (cam_dev->mem2mem2_nodes[i].enabled)
> > +			enable_dma_ports |=
> > +				cam_dev->mem2mem2_nodes[i].desc.dma_port;
> > +	}
> > +	dev_dbg(&cam_dev->pdev->dev, "%s enable_dma_ports:0x%x",
> > +		__func__, enable_dma_ports);
> > +
> > +	return enable_dma_ports;
> > +}
> > +
> > +/* Utility functions */
> > +static unsigned int get_sensor_pixel_id(unsigned int fmt)
> > +{
> > +	switch (fmt) {
> > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> > +	case MEDIA_BUS_FMT_SBGGR14_1X14:
> > +		return raw_pxl_id_b;
> > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> > +	case MEDIA_BUS_FMT_SGBRG14_1X14:
> > +		return raw_pxl_id_gb;
> > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> > +	case MEDIA_BUS_FMT_SGRBG14_1X14:
> > +		return raw_pxl_id_gr;
> > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +	case MEDIA_BUS_FMT_SRGGB14_1X14:
> > +		return raw_pxl_id_r;
> > +	default:
> > +		return raw_pxl_id_b;
> > +	}
> > +}
> > +
> > +static unsigned int get_sensor_fmt(unsigned int fmt)
> > +{
> > +	switch (fmt) {
> > +	case MEDIA_BUS_FMT_SBGGR8_1X8:
> > +	case MEDIA_BUS_FMT_SGBRG8_1X8:
> > +	case MEDIA_BUS_FMT_SGRBG8_1X8:
> > +	case MEDIA_BUS_FMT_SRGGB8_1X8:
> > +		return img_fmt_bayer8;
> > +	case MEDIA_BUS_FMT_SBGGR10_1X10:
> > +	case MEDIA_BUS_FMT_SGBRG10_1X10:
> > +	case MEDIA_BUS_FMT_SGRBG10_1X10:
> > +	case MEDIA_BUS_FMT_SRGGB10_1X10:
> > +		return img_fmt_bayer10;
> > +	case MEDIA_BUS_FMT_SBGGR12_1X12:
> > +	case MEDIA_BUS_FMT_SGBRG12_1X12:
> > +	case MEDIA_BUS_FMT_SGRBG12_1X12:
> > +	case MEDIA_BUS_FMT_SRGGB12_1X12:
> > +		return img_fmt_bayer12;
> > +	case MEDIA_BUS_FMT_SBGGR14_1X14:
> > +	case MEDIA_BUS_FMT_SGBRG14_1X14:
> > +	case MEDIA_BUS_FMT_SGRBG14_1X14:
> > +	case MEDIA_BUS_FMT_SRGGB14_1X14:
> > +		return img_fmt_bayer14;
> > +	default:
> > +		return img_fmt_unknown;
> > +	}
> > +}
> > +
> > +static unsigned int get_img_fmt(unsigned int fourcc)
> > +{
> > +	switch (fourcc) {
> > +	case V4L2_PIX_FMT_MTISP_B8:
> > +		return img_fmt_bayer8;
> > +	case V4L2_PIX_FMT_MTISP_F8:
> > +		return img_fmt_fg_bayer8;
> > +	case V4L2_PIX_FMT_MTISP_B10:
> > +		return img_fmt_bayer10;
> > +	case V4L2_PIX_FMT_MTISP_F10:
> > +		return img_fmt_fg_bayer10;
> > +	case V4L2_PIX_FMT_MTISP_B12:
> > +		return img_fmt_bayer12;
> > +	case V4L2_PIX_FMT_MTISP_F12:
> > +		return img_fmt_fg_bayer12;
> > +	case V4L2_PIX_FMT_MTISP_B14:
> > +		return img_fmt_bayer14;
> > +	case V4L2_PIX_FMT_MTISP_F14:
> > +		return img_fmt_fg_bayer14;
> > +	default:
> > +		return img_fmt_unknown;
> > +	}
> > +}
> > +
> > +static unsigned int get_pixel_byte(unsigned int fourcc)
> > +{
> > +	switch (fourcc) {
> > +	case V4L2_PIX_FMT_MTISP_B8:
> > +	case V4L2_PIX_FMT_MTISP_F8:
> > +		return 8;
> > +	case V4L2_PIX_FMT_MTISP_B10:
> > +	case V4L2_PIX_FMT_MTISP_F10:
> > +		return 10;
> > +	case V4L2_PIX_FMT_MTISP_B12:
> > +	case V4L2_PIX_FMT_MTISP_F12:
> > +		return 12;
> > +	case V4L2_PIX_FMT_MTISP_B14:
> > +	case V4L2_PIX_FMT_MTISP_F14:
> > +		return 14;
> > +	case V4L2_PIX_FMT_MTISP_U8:
> > +	case V4L2_PIX_FMT_MTISP_U10:
> > +	case V4L2_PIX_FMT_MTISP_U12:
> > +	case V4L2_PIX_FMT_MTISP_U14:
> > +		return 16;
> > +	default:
> > +		return 10;
> > +	}
> > +}
> > +
> > +static void composer_deinit_done_cb(void *data)
> > +{
> > +	struct isp_p1_device *p1_dev = p1_ctx_to_dev(data);
> > +
> > +	disable_sys_clock(p1_dev);
> > +	/* Notify PM */
> > +	pm_runtime_put_sync(&p1_dev->pdev->dev);
> > +}
> > +
> > +/* ISP P1 interface functions */
> > +int mtk_isp_open(struct device *dev)
> > +{
> > +	struct isp_p1_device *p1_dev = get_p1_device(dev);
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	s32 usercount = atomic_inc_return(&isp_ctx->isp_user_cnt);
> > +	int ret;
> > +
> > +	dev_dbg(dev, "%s usercount=%d\n", __func__, usercount);
> > +
> > +	if (usercount == 1) {
> > +		ret = isp_setup_scp_rproc(p1_dev);
> > +		if (ret)
> > +			goto scp_err;
> > +
> > +		/* ISP HW INIT */
> > +		isp_ctx->isp_hw_module = ISP_CAM_B_IDX;
> > +		/* Use pure RAW as default HW path */
> > +		isp_ctx->isp_raw_path = ISP_PURE_RAW_PATH;
> > +		/* Check enabled DMAs which is configured by media setup */
> > +		isp_ctx->enable_dma_ports =
> > +			get_enable_dma_ports(p1_dev->cam_dev);
> > +
> > +		if (!isp_ctx->enable_dma_ports) {
> > +			dev_dbg(dev, "No DMAs are enabled\n");
> > +			ret = -EINVAL;
> > +			goto scp_err;
> > +		}
> > +
> > +		pm_runtime_get_sync(dev);
> > +
> > +		ret = isp_init_context(p1_dev);
> > +		if (ret)
> > +			goto ctx_err;
> > +		ret = isp_composer_init(isp_ctx);
> > +		if (ret)
> > +			goto composer_err;
> > +		ret = isp_composer_hw_init(isp_ctx);
> > +		if (ret)
> > +			goto composer_err;
> > +
> > +		isp_composer_meta_config(&p1_dev->isp_ctx,
> > +					 isp_ctx->enable_dma_ports);
> > +	}
> > +
> > +	return 0;
> > +composer_err:
> > +	isp_uninit_context(p1_dev);
> > +ctx_err:
> > +	pm_runtime_put_sync(dev);
> > +scp_err:
> > +	atomic_dec_return(&isp_ctx->isp_user_cnt);
> > +	return ret;
> > +}
> > +
> > +int mtk_isp_release(struct device *dev)
> > +{
> > +	struct isp_p1_device *p1_dev = get_p1_device(dev);
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +
> > +	if (atomic_dec_and_test(&p1_dev->isp_ctx.isp_user_cnt)) {
> > +		isp_composer_hw_deinit(isp_ctx, composer_deinit_done_cb);
> > +		isp_uninit_context(p1_dev);
> > +	}
> > +
> > +	dev_dbg(dev, "%s usercount=%d\n", __func__,
> > +		atomic_read(&p1_dev->isp_ctx.isp_user_cnt));
> > +
> > +	return 0;
> > +}
> > +
> > +int mtk_isp_config(struct device *dev)
> > +{
> > +	struct isp_p1_device *p1_dev = get_p1_device(dev);
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	struct p1_config_param config_param;
> > +	struct mtk_cam_dev *cam_dev = p1_dev->cam_dev;
> > +	struct v4l2_subdev_format sd_format;
> > +	unsigned int sd_width, sd_height;
> > +	unsigned int enable_dma_ports, idx;
> > +	int ret;
> > +
> > +	p1_dev->isp_devs[isp_ctx->isp_hw_module].current_frame = 0;
> > +	p1_dev->isp_devs[isp_ctx->isp_hw_module].sof_count = 0;
> > +
> > +	isp_ctx->frame_seq_no = 1;
> > +	atomic_set(&isp_ctx->composed_frame_id, 0);
> > +
> > +	/* Get the enabled DMA ports */
> > +	enable_dma_ports = isp_ctx->enable_dma_ports;
> > +	dev_dbg(dev, "%s enable_dma_ports:0x%x", __func__, enable_dma_ports);
> > +
> > +	/* sensor config */
> > +	sd_format.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> > +	ret = v4l2_subdev_call(cam_dev->sensor,
> > +			       pad, get_fmt, NULL, &sd_format);
> > +
> > +	if (ret) {
> > +		dev_dbg(dev, "sensor:%s g_fmt on failed:%d\n",
> > +			cam_dev->sensor->entity.name, ret);
> > +		return -EPERM;
> > +	}
> > +
> > +	dev_dbg(dev,
> > +		"sensor get_fmt ret=%d, w=%d, h=%d, code=0x%x, field=%d, color=%d\n",
> > +		ret, sd_format.format.width, sd_format.format.height,
> > +		sd_format.format.code, sd_format.format.field,
> > +		sd_format.format.colorspace);
> > +
> > +	config_param.cfg_in_param.continuous = 0x1;
> > +	config_param.cfg_in_param.subsample = 0x0;
> > +	/* fix to one pixel mode in default */
> > +	config_param.cfg_in_param.pixel_mode = one_pixel_mode;
> > +	/* support normal pattern in default */
> > +	config_param.cfg_in_param.data_pattern = 0x0;
> > +
> > +	config_param.cfg_in_param.crop.left = 0x0;
> > +	config_param.cfg_in_param.crop.top = 0x0;
> > +
> > +	config_param.cfg_in_param.raw_pixel_id =
> > +		get_sensor_pixel_id(sd_format.format.code);
> > +	config_param.cfg_in_param.img_fmt =
> > +		get_sensor_fmt(sd_format.format.code);
> > +	config_param.cfg_in_param.crop.width = sd_format.format.width;
> > +	config_param.cfg_in_param.crop.height = sd_format.format.height;
> > +	sd_width = sd_format.format.width;
> > +	sd_height = sd_format.format.height;
> > +
> > +	idx = MTK_CAM_P1_MAIN_STREAM_OUT;
> 
> The idx variable is unnecessary. Just use MTK_CAM_P1_... to index into
> mem2mem2_nodes directly here and below.
> 

Ok, we will remove idx variable and use const value to index.

> > +	if ((enable_dma_ports & R_IMGO) == R_IMGO) {
> > +		struct v4l2_format *imgo_fmt =
> > +			&p1_dev->cam_dev->mem2mem2_nodes[idx].vdev_fmt;
> > +
> > +		config_param.cfg_main_param.pure_raw = isp_ctx->isp_raw_path;
> > +		config_param.cfg_main_param.pure_raw_pack = 1;
> > +		config_param.cfg_main_param.bypass = 0;
> > +
> > +		config_param.cfg_main_param.output.img_fmt =
> > +			get_img_fmt(imgo_fmt->fmt.pix_mp.pixelformat);
> > +		config_param.cfg_main_param.output.pixel_byte =
> > +			get_pixel_byte(imgo_fmt->fmt.pix_mp.pixelformat);
> > +		config_param.cfg_main_param.output.size.w =
> > +			imgo_fmt->fmt.pix_mp.width;
> > +		config_param.cfg_main_param.output.size.h =
> > +			imgo_fmt->fmt.pix_mp.height;
> > +
> > +		config_param.cfg_main_param.output.size.stride =
> > +			imgo_fmt->fmt.pix_mp.plane_fmt[0].bytesperline;
> > +		config_param.cfg_main_param.output.size.xsize =
> > +			imgo_fmt->fmt.pix_mp.plane_fmt[0].bytesperline;
> > +
> > +		config_param.cfg_main_param.output.crop.left = 0x0;
> > +		config_param.cfg_main_param.output.crop.top = 0x0;
> > +
> > +		config_param.cfg_main_param.output.crop.width = sd_width;
> > +		config_param.cfg_main_param.output.crop.height = sd_height;
> > +
> > +		WARN_ONCE(imgo_fmt->fmt.pix_mp.width > sd_width ||
> > +			  imgo_fmt->fmt.pix_mp.height > sd_height,
> > +			  "img out:%d:%d in:%d:%d",
> > +			  imgo_fmt->fmt.pix_mp.width,
> > +			  imgo_fmt->fmt.pix_mp.height,
> > +			  sd_width,
> > +			  sd_height);
> > +
> > +		dev_dbg(dev,
> > +			"imgo pixel_byte:%d img_fmt:0x%x raw:%d\n",
> > +			config_param.cfg_main_param.output.pixel_byte,
> > +			config_param.cfg_main_param.output.img_fmt,
> > +			config_param.cfg_main_param.pure_raw);
> > +		dev_dbg(dev,
> > +			"imgo param:size=%0dx%0d, stride:%d,xsize:%d,crop=%0dx%0d\n",
> > +			config_param.cfg_main_param.output.size.w,
> > +			config_param.cfg_main_param.output.size.h,
> > +			config_param.cfg_main_param.output.size.stride,
> > +			config_param.cfg_main_param.output.size.xsize,
> > +			config_param.cfg_main_param.output.crop.width,
> > +			config_param.cfg_main_param.output.crop.height);
> > +	} else {
> > +		config_param.cfg_main_param.bypass = 1;
> > +	}
> > +
> > +	idx = MTK_CAM_P1_PACKED_BIN_OUT;
> > +	if ((enable_dma_ports & R_RRZO) == R_RRZO) {
> > +		struct v4l2_format *rrzo_fmt =
> > +			&p1_dev->cam_dev->mem2mem2_nodes[idx].vdev_fmt;
> > +
> > +		config_param.cfg_resize_param.bypass = 0;
> > +		config_param.cfg_resize_param.output.img_fmt =
> > +			get_img_fmt(rrzo_fmt->fmt.pix_mp.pixelformat);
> > +		config_param.cfg_resize_param.output.pixel_byte =
> > +			get_pixel_byte(rrzo_fmt->fmt.pix_mp.pixelformat);
> > +		config_param.cfg_resize_param.output.size.w =
> > +			rrzo_fmt->fmt.pix_mp.width;
> > +		config_param.cfg_resize_param.output.size.h =
> > +			rrzo_fmt->fmt.pix_mp.height;
> > +		config_param.cfg_resize_param.output.size.stride =
> > +			rrzo_fmt->fmt.pix_mp.plane_fmt[0].bytesperline;
> > +		config_param.cfg_resize_param.output.size.xsize =
> > +			rrzo_fmt->fmt.pix_mp.plane_fmt[0].bytesperline;
> > +
> > +		config_param.cfg_resize_param.output.crop.left = 0x0;
> > +		config_param.cfg_resize_param.output.crop.top = 0x0;
> > +		config_param.cfg_resize_param.output.crop.width = sd_width;
> > +		config_param.cfg_resize_param.output.crop.height = sd_height;
> > +
> > +		WARN_ONCE(rrzo_fmt->fmt.pix_mp.width > sd_width ||
> > +			  rrzo_fmt->fmt.pix_mp.height > sd_height,
> > +			  "rrz out:%d:%d in:%d:%d",
> > +			  rrzo_fmt->fmt.pix_mp.width,
> > +			  rrzo_fmt->fmt.pix_mp.height,
> > +			  sd_width,
> > +			  sd_height);
> > +
> > +		dev_dbg(dev, "rrzo pixel_byte:%d img_fmt:0x%x\n",
> > +			config_param.cfg_resize_param.output.pixel_byte,
> > +			config_param.cfg_resize_param.output.img_fmt);
> > +		dev_dbg(dev,
> > +			"rrzo param:size=%0dx%0d,stride:%d,xsize:%d,crop=%0dx%0d\n",
> > +			config_param.cfg_resize_param.output.size.w,
> > +			config_param.cfg_resize_param.output.size.h,
> > +			config_param.cfg_resize_param.output.size.stride,
> > +			config_param.cfg_resize_param.output.size.xsize,
> > +			config_param.cfg_resize_param.output.crop.width,
> > +			config_param.cfg_resize_param.output.crop.height);
> > +	} else {
> > +		config_param.cfg_resize_param.bypass = 1;
> > +	}
> > +
> > +	/* Configure meta DMAs info. */
> > +	config_param.cfg_meta_param.enabled_meta_dmas = enable_dma_ports;
> > +
> > +	isp_composer_hw_config(isp_ctx, &config_param);
> > +
> > +	dev_dbg(dev, "%s done\n", __func__);
> > +	return 0;
> > +}
> > +
> > +int mtk_isp_enqueue(struct device *dev,
> > +		    unsigned int dma_port,
> > +		    struct mtk_cam_dev_buffer *buffer)
> > +{
> > +	struct isp_p1_device *p1_dev = get_p1_device(dev);
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	struct mtk_isp_scp_p1_cmd frameparams;
> > +
> > +	memset(&frameparams, 0, sizeof(frameparams));
> > +
> > +	frameparams.cmd_id = ISP_CMD_ENQUEUE_META;
> > +	frameparams.meta_frame.enabled_dma = dma_port;
> > +	frameparams.meta_frame.vb_index = buffer->vbb.vb2_buf.index;
> > +	frameparams.meta_frame.meta_addr.iova = buffer->daddr;
> > +	frameparams.meta_frame.meta_addr.scp_addr = buffer->scp_addr;
> > +
> > +	isp_composer_enqueue(isp_ctx, &frameparams, SCP_ISP_CMD);
> > +
> > +	return 0;
> > +}
> > +
> > +int mtk_isp_req_enqueue(struct device *dev,
> > +			struct mtk_cam_dev_start_param *frameparamsbase)
> > +{
> > +	struct isp_p1_device *p1_dev = get_p1_device(dev);
> > +	struct mtk_isp_p1_ctx *isp_ctx = &p1_dev->isp_ctx;
> > +	struct p1_frame_param frameparams;
> > +	struct mtk_isp_queue_job *framejob;
> > +	struct mtk_cam_dev_buffer **bundle_buffers;
> > +	unsigned int i, idx;
> > +
> > +	framejob = kzalloc(sizeof(*framejob), GFP_ATOMIC);
> > +	memset(framejob, 0, sizeof(*framejob));
> > +	memset(&frameparams, 0, sizeof(frameparams));
> > +	INIT_LIST_HEAD(&framejob->list_buf);
> > +
> > +	bundle_buffers = &frameparamsbase->buffers[0];
> > +	frameparams.frame_seq_no = isp_ctx->frame_seq_no++;
> > +	frameparams.sof_idx =
> > +		p1_dev->isp_devs[isp_ctx->isp_hw_module].sof_count;
> > +	framejob->request_fd = frameparamsbase->request_fd;
> > +	framejob->frame_seq_no = frameparams.frame_seq_no;
> > +
> > +	idx = MTK_CAM_P1_META_IN_0;
> > +	if (bundle_buffers[idx]) {
> > +		frameparams.tuning_addr.iova =
> > +			bundle_buffers[idx]->daddr;
> > +		frameparams.tuning_addr.scp_addr =
> > +			bundle_buffers[idx]->scp_addr;
> > +		list_add_tail(&bundle_buffers[idx]->list,
> > +			      &framejob->list_buf);
> > +	}
> > +
> > +	/* Image output */
> > +	idx = MTK_CAM_P1_MAIN_STREAM_OUT;
> > +	if (bundle_buffers[idx]) {
> > +		frameparams.img_dma_buffers[0].buffer.iova =
> > +			bundle_buffers[idx]->daddr;
> > +		frameparams.img_dma_buffers[0].buffer.scp_addr =
> > +			bundle_buffers[idx]->scp_addr;
> > +		dev_dbg(dev, "main stream address iova:0x%x\n",
> > +			frameparams.img_dma_buffers[0].buffer.iova);
> > +		list_add_tail(&bundle_buffers[idx]->list,
> > +			      &framejob->list_buf);
> > +	}
> > +
> > +	/* Resize output */
> > +	idx = MTK_CAM_P1_PACKED_BIN_OUT;
> > +	if (bundle_buffers[idx]) {
> > +		frameparams.img_dma_buffers[1].buffer.iova =
> > +			bundle_buffers[idx]->daddr;
> > +		frameparams.img_dma_buffers[1].buffer.scp_addr =
> > +			bundle_buffers[idx]->scp_addr;
> > +		dev_dbg(dev, "packed out address iova:0x%x\n",
> > +			frameparams.img_dma_buffers[1].buffer.iova);
> > +		list_add_tail(&bundle_buffers[idx]->list,
> > +			      &framejob->list_buf);
> > +	}
> > +
> > +	/* Meta output DMAs */
> > +	for (i = 0; i < MAX_META_DMA_NODES; i++) {
> > +		idx = MTK_CAM_P1_META_OUT_0 + i;
> > +		if (bundle_buffers[idx]) {
> > +			frameparams.meta_addrs[i].iova =
> > +			  bundle_buffers[idx]->daddr;
> > +			frameparams.meta_addrs[i].scp_addr =
> > +			  bundle_buffers[idx]->scp_addr;
> > +			list_add_tail(&bundle_buffers[idx]->list,
> > +				      &framejob->list_buf);
> > +		} else {
> > +			frameparams.meta_addrs[i].iova = 0;
> > +			frameparams.meta_addrs[i].scp_addr = 0;
> > +		}
> > +	}
> > +
> > +	spin_lock(&isp_ctx->p1_enqueue_list.lock);
> > +	list_add_tail(&framejob->list_entry, &isp_ctx->p1_enqueue_list.queue);
> > +	atomic_inc(&isp_ctx->p1_enqueue_list.queue_cnt);
> > +	spin_unlock(&isp_ctx->p1_enqueue_list.lock);
> > +
> > +	isp_composer_enqueue(isp_ctx, &frameparams, SCP_ISP_FRAME);
> > +	dev_dbg(dev, "request fd:%d frame_seq_no:%d is queued cnt:%d\n",
> > +		frameparamsbase->request_fd,
> > +		frameparams.frame_seq_no,
> > +		atomic_read(&isp_ctx->p1_enqueue_list.queue_cnt));
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct dev_pm_ops mtk_isp_pm_ops = {
> > +	SET_SYSTEM_SLEEP_PM_OPS(mtk_isp_suspend, mtk_isp_resume)
> > +	SET_RUNTIME_PM_OPS(mtk_isp_suspend, mtk_isp_resume, NULL)
> > +};
> > +
> > +static struct platform_driver mtk_isp_driver = {
> > +	.probe   = mtk_isp_probe,
> > +	.remove  = mtk_isp_remove,
> > +	.driver  = {
> > +		.name  = "mtk-cam",
> > +		.of_match_table = of_match_ptr(mtk_isp_of_ids),
> > +		.pm     = &mtk_isp_pm_ops,
> > +	}
> > +};
> > +
> > +module_platform_driver(mtk_isp_driver);
> > +
> > +MODULE_DESCRIPTION("Camera ISP driver");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h
> > new file mode 100644
> > index 000000000000..6cf8bb4ba93a
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/isp_50/cam/mtk_cam.h
> > @@ -0,0 +1,300 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (c) 2018 MediaTek Inc.
> > + * Author: Ryan Yu <ryan.yu@mediatek.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __CAMERA_ISP_H
> > +#define __CAMERA_ISP_H
> > +
> > +#include <linux/cdev.h>
> > +#include <linux/clk.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/ioctl.h>
> > +#include <linux/irqreturn.h>
> > +#include <linux/miscdevice.h>
> > +#include <linux/pm_qos.h>
> > +#include <linux/scatterlist.h>
> > +
> > +#include "mtk_cam-dev.h"
> > +#include "mtk_cam-scp.h"
> > +
> > +#define CAM_A_MAX_WIDTH		3328U
> > +#define CAM_A_MAX_HEIGHT	2496U
> > +#define CAM_B_MAX_WIDTH		5376U
> > +#define CAM_B_MAX_HEIGHT	4032U
> > +
> > +#define CAM_MIN_WIDTH		80U
> > +#define CAM_MIN_HEIGHT		60U
> > +
> > +#define IMG_MAX_WIDTH		CAM_B_MAX_WIDTH
> > +#define IMG_MAX_HEIGHT		CAM_B_MAX_HEIGHT
> > +#define IMG_MIN_WIDTH		CAM_MIN_WIDTH
> > +#define IMG_MIN_HEIGHT		CAM_MIN_HEIGHT
> > +
> > +#define RRZ_MAX_WIDTH		CAM_B_MAX_WIDTH
> > +#define RRZ_MAX_HEIGHT		CAM_B_MAX_HEIGHT
> > +#define RRZ_MIN_WIDTH		CAM_MIN_WIDTH
> > +#define RRZ_MIN_HEIGHT		CAM_MIN_HEIGHT
> > +
> > +#define R_IMGO		BIT(0)
> > +#define R_RRZO		BIT(1)
> > +#define R_AAO		BIT(3)
> > +#define R_AFO		BIT(4)
> > +#define R_LCSO		BIT(5)
> > +#define R_PDO		BIT(6)
> > +#define R_LMVO		BIT(7)
> > +#define R_FLKO		BIT(8)
> > +#define R_RSSO		BIT(9)
> > +#define R_PSO		BIT(10)
> > +
> > +#define ISP_COMPOSING_MAX_NUM		4
> > +#define ISP_FRAME_COMPOSING_MAX_NUM	3
> > +
> > +#define IRQ_DATA_BUF_SIZE		4
> > +#define COMPOSRE_EVENT_BUF_SIZE		4
> > +
> > +#define CQ_ADDRESS_OFFSET		0x640
> > +#define CQ_BUFFER_COUNT			3
> 
> Please align macro values using tabs.
> 
> > +
> > +#define IRQ_STAT_STR "cam%c, SOF_%d irq(0x%x), " \
> > +			"dma(0x%x), frame_num(%d)/cq_num(%d)\n"
> > +
> > +/*
> > + * In order with the sequence of device nodes defined in dtsi rule,
> > + * one hardware module should be mapping to one node.
> > + */
> > +enum isp_dev_node_enum {
> > +	ISP_CAMSYS_CONFIG_IDX = 0,
> > +	ISP_CAM_UNI_IDX,
> > +	ISP_CAM_A_IDX,
> > +	ISP_CAM_B_IDX,
> > +	ISP_DEV_NODE_NUM
> > +};
> > +
> > +/* Image RAW path for ISP P1 module. */
> > +enum isp_raw_path_enum {
> > +	ISP_PROCESS_RAW_PATH = 0,
> > +	ISP_PURE_RAW_PATH
> > +};
> > +
> > +enum {
> > +	img_fmt_unknown		= 0x0000,
> > +	img_fmt_raw_start	= 0x2200,
> > +	img_fmt_bayer8		= img_fmt_raw_start,
> > +	img_fmt_bayer10,
> > +	img_fmt_bayer12,
> > +	img_fmt_bayer14,
> > +	img_fmt_fg_bayer8,
> > +	img_fmt_fg_bayer10,
> > +	img_fmt_fg_bayer12,
> > +	img_fmt_fg_bayer14,
> > +};
> > +
> > +enum {
> > +	raw_pxl_id_b   = 0,
> > +	raw_pxl_id_gb,
> > +	raw_pxl_id_gr,
> > +	raw_pxl_id_r
> > +};
> > +
> > +enum {
> > +	default_pixel_mode = 0,
> > +	one_pixel_mode,
> > +	two_pixel_mode,
> > +	four_pixel_mode,
> > +	pixel_mode_num,
> > +};
> > +
> > +enum mtk_isp_scp_ipi_type {
> > +	SCP_ISP_CMD = 0,
> > +	SCP_ISP_FRAME,
> > +};
> > +
> > +struct isp_queue {
> > +	struct list_head queue;
> > +	atomic_t queue_cnt;
> > +	spinlock_t lock; /* queue attributes protection */
> > +};
> > +
> > +struct isp_thread {
> > +	struct task_struct *thread;
> > +	wait_queue_head_t wq;
> > +};
> > +
> > +struct mtk_isp_queue_work {
> > +	union {
> > +		struct mtk_isp_scp_p1_cmd cmd;
> > +		struct p1_frame_param frameparams;
> > +	};
> > +	struct list_head list_entry;
> > +	enum mtk_isp_scp_ipi_type type;
> > +};
> > +
> > +struct mtk_cam_dev_stat_event_data {
> > +	__u32 frame_seq_no;
> > +	__u32 meta0_vb2_index;
> > +	__u32 meta1_vb2_index;
> > +	__u32 irq_status_mask;
> > +	__u32 dma_status_mask;
> > +};
> > +
> > +struct mtk_isp_queue_job {
> > +	struct list_head list_entry;
> > +	struct list_head list_buf;
> > +	unsigned int request_fd;
> > +	unsigned int frame_seq_no;
> > +};
> > +
> > +struct isp_clk_struct {
> > +	int num_clks;
> > +	struct clk_bulk_data *clk_list;
> > +};
> > +
> > +struct isp_device {
> > +	struct device *dev;
> > +	void __iomem *regs;
> > +	int irq;
> > +	spinlock_t spinlock_irq; /* ISP reg setting integrity */
> > +	unsigned int current_frame;
> > +	unsigned int meta0_vb2_index;
> > +	unsigned int meta1_vb2_index;
> > +	u8 sof_count;
> > +	u8 isp_hw_module;
> > +};
> > +
> > +struct mtk_isp_p1_ctx {
> > +	struct isp_queue composer_txlist;
> > +	struct isp_thread composer_tx_thread;
> > +	atomic_t cmd_queued;
> > +	struct mutex composer_tx_lock; /* isp composer work protection */
> > +
> > +	struct isp_thread composer_rx_thread;
> > +	struct mtk_isp_scp_p1_cmd composer_evts[COMPOSRE_EVENT_BUF_SIZE];
> > +	atomic_t composer_evts_start;
> > +	atomic_t composer_evts_end;
> > +	spinlock_t composer_evts_lock; /* SCP events protection */
> > +	/* increase after ipi */
> > +	atomic_t ipi_occupied;
> > +	/* increase after frame enqueue */
> > +	atomic_t composing_frame;
> > +	/* current composed frame id */
> > +	atomic_t composed_frame_id;
> > +
> > +	struct isp_queue p1_enqueue_list;
> > +
> > +	struct isp_thread isp_deque_thread;
> > +	struct mtk_cam_dev_stat_event_data irq_event_datas[IRQ_DATA_BUF_SIZE];
> > +	atomic_t irq_data_start;
> > +	atomic_t irq_data_end;
> > +	spinlock_t irq_dequeue_lock; /* ISP frame dequeuq protection */
> > +
> > +	dma_addr_t scp_mem_pa;
> > +	dma_addr_t scp_mem_iova;
> > +	struct sg_table sgtable;
> > +
> > +	/* increase after open, decrease when close */
> > +	atomic_t isp_user_cnt;
> > +	/* frame sequence number, increase per en-queue*/
> > +	int frame_seq_no;
> > +	unsigned int isp_hw_module;
> > +	unsigned int isp_raw_path;
> > +	unsigned int enable_dma_ports;
> > +
> > +	void (*composer_deinit_donecb)(void *isp_ctx);
> > +
> > +	struct list_head list;
> > +};
> > +
> > +struct isp_p1_device {
> > +	struct platform_device *pdev;
> > +
> > +	/* for SCP driver  */
> > +	struct platform_device *scp_pdev;
> > +	struct rproc *rproc_handle;
> > +
> > +	struct mtk_isp_p1_ctx isp_ctx;
> > +	struct isp_clk_struct isp_clk;
> 
> What's the benefit of having mtk_isp_p1_ctx and isp_clk_struct in
> separate structs? They are only ever used in isp_p1_device.
> 

Ok, we will merge isp_clk_struct into mtk_isp_p1_ctx in next patch.

> > +	struct mtk_cam_dev *cam_dev;
> > +	struct isp_device *isp_devs;
> 
> Similarly, why are these allocated at runtime rather then just members
> of the struct?
> 

Ok, we will revise new isp_p1_device structure as below list:

struct isp_p1_device {
	struct platform_device *pdev;
	struct platform_device *scp_pdev;
	struct rproc *rproc_handle;
	struct mtk_isp_p1_ctx isp_ctx;
	struct mtk_cam_dev cam_dev;
	struct isp_device isp_devs[ISP_DEV_NODE_NUM];
};


> In mtk_isp_probe the struct isp_p1_device is allocated, and immediately
> afterwards the struct mtk_cam_dev and struct isp_devices are. There will
> only ever be ISP_DEV_NODE_NUM isp_devices.
> 
> Could this be changed to:
> 	struct mtk_cam_dev cam_dev;
> 	struct isp_device isp_devs[ISP_DEV_NODE_NUM];
> ?
> 

Yes, thanks for your suggestion as above.

> > +};
> > +
> > +static inline struct isp_p1_device *
> > +p1_ctx_to_dev(const struct mtk_isp_p1_ctx *__p1_ctx)
> > +{
> > +	return container_of(__p1_ctx, struct isp_p1_device, isp_ctx);
> > +}
> > +
> > +static inline struct isp_p1_device *get_p1_device(struct device *dev)
> > +{
> > +	return ((struct isp_p1_device *)dev_get_drvdata(dev));
> > +}
> > +
> > +int isp_composer_init(struct mtk_isp_p1_ctx *isp_ctx);
> > +int isp_composer_hw_init(struct mtk_isp_p1_ctx *isp_ctx);
> > +void isp_composer_meta_config(struct mtk_isp_p1_ctx *isp_ctx,
> > +			      unsigned int dma);
> > +void isp_composer_hw_config(struct mtk_isp_p1_ctx *isp_ctx,
> > +			    struct p1_config_param *config_param);
> > +void isp_composer_stream(struct mtk_isp_p1_ctx *isp_ctx, int on);
> > +void isp_composer_hw_deinit(struct mtk_isp_p1_ctx *isp_ctx,
> > +			    void (*donecb)(void *data));
> > +void isp_composer_enqueue(struct mtk_isp_p1_ctx *isp_ctx,
> > +			  void *data,
> > +			  enum mtk_isp_scp_ipi_type type);
> 
> These functions are declared here, but implemented in mtk_cam-scp.c.
> Can the funtion declarations be moved to mtk_cam-scp.h?
> 

Ok, we will revise these function declared in next patch.

Best regards,


Jungo 

> > +
> > +/**
> > + * mtk_isp_open - open isp driver and initialize related resources.
> > + *
> > + * @dev:	isp device.
> > + *
> > + */
> > +int mtk_isp_open(struct device *dev);
> > +
> > +/**
> > + * mtk_isp_release - release isp driver and related resources.
> > + *
> > + * @dev:	isp device.
> > + *
> > + */
> > +int mtk_isp_release(struct device *dev);
> > +
> > +/**
> > + * mtk_isp_config - output image & meta data configuration.
> > + *
> > + * @dev:	isp device.
> > + *
> > + */
> > +int mtk_isp_config(struct device *dev);
> > +
> > +/**
> > + * mtk_isp_req_enqueue - enqueue a frame bundle (per-frame basis) to ISP driver.
> > + *
> > + * @dev:	isp device.
> > + * @frameparamsbase: pointer to &struct mtk_cam_dev_start_param.
> > + *
> > + */
> > +int mtk_isp_req_enqueue(struct device *dev,
> > +			struct mtk_cam_dev_start_param *frameparamsbase);
> > +
> > +/**
> > + * mtk_isp_enqueue - enqueue a single frame to ISP driver
> > + * for non-per-frame DMA.
> > + *
> > + * @dev:	isp device.
> > + * @buffer: pointer to &struct mtk_cam_dev_buffer.
> > + *
> > + */
> > +int mtk_isp_enqueue(struct device *dev,
> > +		    unsigned int dma_idx,
> > +		    struct mtk_cam_dev_buffer *buffer);
> > +#endif /*__CAMERA_ISP_H*/
> > -- 
> > 2.18.0
> > 
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek