Re: [RFC PATCH V1 6/6] platform: mtk-isp: Add Mediatek FD driver

[Tomasz Figa <tfiga@chromium.org>]

Hi Jerry,

On Tue, Apr 23, 2019 at 06:45:05PM +0800, Jerry-ch Chen wrote:
> From: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> 
> This patch adds the driver of Face Detection (FD) unit in
> Mediatek camera system, providing face detection function.
> 
> The mtk-isp directory will contain drivers for multiple IP
> blocks found in Mediatek ISP system. It will include ISP Pass 1
> driver (CAM), sensor interface driver, DIP driver and face
> detection driver.
> 

Thanks for the patch.

First of all a general comment about the design:

My understanding is that this is a relatively straightforward
memory-to-memory device that reads a video frame and detects faces on it.
Such devices should be implemented as normal V4L2 memory-to-memory devices,
with contexts (instances; pipes) represented by v4l2_fh.

Also, please replace the META_OUTPUT queue with proper V4L2 controls, as I
don't think there is anything that we couldn't model using controls here.

The end result should be a V4L2 m2m driver (using the m2m helpers), where
you get a new context (instance; pipe) whenever you open the video node,
similar to codecs, video processors (like MTK MDP) and so on.

Also please see my comments inline.

> Signed-off-by: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> ---
>  drivers/media/platform/mtk-isp/Makefile       |   16 +
>  drivers/media/platform/mtk-isp/fd/Makefile    |   25 +
>  .../media/platform/mtk-isp/fd/mtk_fd-dev.c    |  754 +++++++++++
>  .../media/platform/mtk-isp/fd/mtk_fd-dev.h    |  315 +++++
>  drivers/media/platform/mtk-isp/fd/mtk_fd-hw.h |  158 +++
>  .../media/platform/mtk-isp/fd/mtk_fd-smem.c   |  322 +++++
>  .../media/platform/mtk-isp/fd/mtk_fd-smem.h   |   39 +
>  .../media/platform/mtk-isp/fd/mtk_fd-v4l2.c   | 1171 +++++++++++++++++
>  drivers/media/platform/mtk-isp/fd/mtk_fd_40.c |  555 ++++++++

This is a small driver. Please just put all the code in one file. (Except
the smem stuff, which should go away.)

>  9 files changed, 3355 insertions(+)
>  create mode 100644 drivers/media/platform/mtk-isp/Makefile
>  create mode 100644 drivers/media/platform/mtk-isp/fd/Makefile
>  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd-dev.c
>  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd-dev.h
>  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd-hw.h
>  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd-smem.c
>  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd-smem.h
>  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd-v4l2.c
>  create mode 100644 drivers/media/platform/mtk-isp/fd/mtk_fd_40.c
> 
> diff --git a/drivers/media/platform/mtk-isp/Makefile b/drivers/media/platform/mtk-isp/Makefile
> new file mode 100644
> index 000000000000..5e3a9aa7f8b2
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/Makefile
> @@ -0,0 +1,16 @@
> +#
> +# Copyright (C) 2018 MediaTek Inc.
> +#
> +# 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.
> +#
> +
> +ifeq ($(CONFIG_VIDEO_MEDIATEK_FD_SUPPORT),y)

There is no value in having "SUPPORT" in the Kconfig symbol name. It just
makes it unnecessarily long.

> +obj-y += fd/
> +endif

You can just add this directly in drivers/media/platform/Makefile. No need
for this intermediate file.

Also, the driver should be compilable as a module too.

> diff --git a/drivers/media/platform/mtk-isp/fd/Makefile b/drivers/media/platform/mtk-isp/fd/Makefile
> new file mode 100644
> index 000000000000..f2b64cf53da9
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/fd/Makefile
> @@ -0,0 +1,25 @@
> +#
> +# Copyright (C) 2018 MediaTek Inc.
> +#
> +# 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.
> +#
> +$(info $(srctree))
> +ccflags-y += -I$(srctree)/drivers/media/platform/mtk-mdp3
> +
> +obj-y += mtk_fd_40.o
> +obj-y += mtk_fd-v4l2.o
> +
> +# To provide alloc context managing memory shared
> +# between CPU and camera coprocessor
> +obj-y += mtk_fd-smem.o
> +
> +# Utilits to provide frame-based streaming model
> +# with v4l2 user interfaces
> +obj-y += mtk_fd-dev.o

This wouldn't work if the driver is compiled as a module.
Please use something like if you have more than 1 object.

	mtk-fd-objs += list of .o objects

	obj-$(CONFIG_VIDEO_MEDIATEK_FD) += mtk-fd.o

Otherwise just use the last line directly.

> diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd-dev.c b/drivers/media/platform/mtk-isp/fd/mtk_fd-dev.c
> new file mode 100644
> index 000000000000..207e5d20ad46
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd-dev.c
> @@ -0,0 +1,754 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Frederic Chen <frederic.chen@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.
> + */

No need for this text if there is SPDX.

[snip]
> diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd-dev.h b/drivers/media/platform/mtk-isp/fd/mtk_fd-dev.h
> new file mode 100644
> index 000000000000..c13627f2bac4
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd-dev.h
> @@ -0,0 +1,315 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2018 MediaTek Inc.
> + * Author: Frederic Chen <frederic.chen@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 _MTK_FD_DEV_H_
> +#define _MTK_FD_DEV_H_
> +
> +#include <linux/types.h>
> +#include <linux/platform_device.h>
> +#include <media/v4l2-device.h>
> +#include <media/videobuf2-v4l2.h>
> +
> +#include "mtk_fd-hw.h"
> +#include "mtk_fd-smem.h"
> +
> +#define MTK_FD_PIPE_ID_STREAM_0				0
> +#define MTK_FD_PIPE_ID_STREAM_1				1
> +#define MTK_FD_PIPE_ID_TOTAL_NUM			2
> +
> +#define MTK_FD_VIDEO_NODE_ID_YUV_OUT			0
> +#define MTK_FD_VIDEO_NODE_ID_CONFIG_OUT			1
> +#define MTK_FD_VIDEO_NODE_ID_OUT_TOTAL_NUM		2
> +#define MTK_FD_VIDEO_NODE_ID_CAPTURE			2
> +#define MTK_FD_VIDEO_NODE_ID_CAPTURE_TOTAL_NUM		1
> +#define MTK_FD_VIDEO_NODE_ID_TOTAL_NUM \
> +	(MTK_FD_VIDEO_NODE_ID_OUT_TOTAL_NUM + \
> +	MTK_FD_VIDEO_NODE_ID_CAPTURE_TOTAL_NUM)
> +
> +#define MTK_FD_VIDEO_NODE_ID_NO_MASTER			-1
> +
> +#define MTK_FD_OUTPUT_MIN_WIDTH				2U
> +#define MTK_FD_OUTPUT_MIN_HEIGHT			2U
> +#define MTK_FD_OUTPUT_MAX_WIDTH				5376U
> +#define MTK_FD_OUTPUT_MAX_HEIGHT			4032U
> +#define MTK_FD_CAPTURE_MIN_WIDTH			2U
> +#define MTK_FD_CAPTURE_MIN_HEIGHT			2U
> +#define MTK_FD_CAPTURE_MAX_WIDTH			5376U
> +#define MTK_FD_CAPTURE_MAX_HEIGHT			4032U
> +
> +#define MTK_FD_PIPE_MEDIA_MODEL_NAME	"MTK-FD-V4L2"
> +#define MTK_FD_PIPE_NAME_STREAM_0	MTK_FD_PIPE_MEDIA_MODEL_NAME
> +#define MTK_FD_PIPE_NAME_STREAM_1	"MTK-FD-V4L2-STREAM-1"
> +
> +#define MTK_FD_DEV_META_BUF_DEFAULT_SIZE		(1110 * 1024)
> +
> +/*
> + * Supported format and the information used for
> + * size calculation
> + */
> +struct mtk_fd_dev_meta_format {
> +	u32 dataformat;
> +	u32 max_buffer_size;
> +	u8 flags;
> +};
> +
> +/* MDP part private format definitation */
> +struct mtk_fd_dev_mdp_format {
> +	u32 pixelformat;
> +	u32 mdp_color;
> +	u32 colorspace;
> +	u8 depth[VIDEO_MAX_PLANES];
> +	u8 row_depth[VIDEO_MAX_PLANES];
> +	u8 num_planes;
> +	u8 walign;
> +	u8 halign;
> +	u8 salign;
> +	u32 flags;
> +};
> +
> +struct mtk_fd_dev_format {
> +	union {
> +		struct mtk_fd_dev_meta_format meta;
> +		struct mtk_fd_dev_mdp_format img;
> +	} fmt;
> +};

This looks like a copy/paste from the DIP driver. Please merge the 3
structures above into 1 as suggested in review of that driver.

[snip]
> diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd-hw.h b/drivers/media/platform/mtk-isp/fd/mtk_fd-hw.h
> new file mode 100644
> index 000000000000..40e09d66c479
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd-hw.h
> @@ -0,0 +1,158 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + * Copyright (C) 2015 MediaTek Inc.
> + *
> + * 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 __MTK_FD_HW_H__
> +#define __MTK_FD_HW_H__
> +
> +#include <linux/io.h>
> +#define SIG_ERESTARTSYS 512
> +
> +#define FD_WR32(v, a) \
> +do { \
> +	__raw_writel((v), (void __force __iomem *)((a))); \
> +	mb(); /* ensure written */ \
> +} while (0)
> +
> +#define FD_RD32(addr) ioread32((void *)addr)
> +
> +#define FD_INT_EN		0x15c
> +#define FD_INT			0x168
> +#define FD_RESULT		0x178
> +#define FD_IRQ_MASK		0x001
> +
> +#define RS_BUF_SIZE_MAX		2288788
> +#define VA_OFFSET		0xffff000000000000
> +
> +#define MTK_FD_MAX_NO		1024
> +#define MAX_FACE_SEL_NUM	(MTK_FD_MAX_NO + 2)
> +
> +/* The max number of face sizes could be detected, for feature scaling */
> +#define FACE_SIZE_NUM_MAX	14
> +
> +/* FACE_SIZE_NUM_MAX + 1, first scale for input image W/H */
> +#define FD_SCALE_NUM		15
> +
> +/* Number of Learning data sets */
> +#define LEARNDATA_NUM		18
> +
> +#define mtk_fd_us_to_jiffies(us) \
> +	((((unsigned long)(us) / 1000) * HZ + 512) >> 10)
> +

Uhm, looking at the arbitrary numbers involved in this computation I'm
afraid to even ask what this macro is expected to do.

Judging by the name, why not just use usecs_to_jiffies()?

[snip]
> diff --git a/drivers/media/platform/mtk-isp/fd/mtk_fd-smem.h b/drivers/media/platform/mtk-isp/fd/mtk_fd-smem.h
> new file mode 100644
> index 000000000000..758a4ab68ec2
> --- /dev/null
> +++ b/drivers/media/platform/mtk-isp/fd/mtk_fd-smem.h
> @@ -0,0 +1,39 @@
[snip]
> +
> +struct mtk_fd_smem_dev {
> +	struct device dev;
> +	struct sg_table sgt;
> +	struct page **smem_pages;
> +	int num_smem_pages;
> +	phys_addr_t smem_base;
> +	dma_addr_t smem_dma_base;
> +	int smem_size;
> +};
> +
> +phys_addr_t mtk_fd_smem_iova_to_phys(struct mtk_fd_smem_dev *smem_dev,
> +				     dma_addr_t iova);
> +int mtk_fd_smem_alloc_dev_init(struct mtk_fd_smem_dev *smem_dev,
> +			       struct device *default_alloc_dev);
> +void mtk_fd_smem_alloc_dev_release(struct mtk_fd_smem_dev *smem_dev);
> +

Please remove this custom smem thing as we should just use dma_alloc_*()
from the right struct device attached to the right reserved memory pool.

[snip]
> +static int mtk_fd_videoc_enum_fmt(struct file *file, void *fh,
> +				  struct v4l2_fmtdesc *f)

It's "vidioc".

> +{
> +	struct mtk_fd_video_device *node = mtk_fd_file_to_node(file);
> +
> +	if (f->index > node->desc->num_fmts ||
> +	    f->type != node->dev_q.vbq.type)

No need to check the type.

> +		return -EINVAL;
> +
> +	strscpy(f->description, node->desc->description,
> +		sizeof(f->description));
> +
> +	f->pixelformat = node->desc->fmts[f->index].fmt.img.pixelformat;
> +	f->flags = 0;
> +
> +	return 0;
> +}
> +
> +static int mtk_fd_meta_enum_format(struct file *file,
> +				   void *fh, struct v4l2_fmtdesc *f)

Please name the functions consistently. Above it has the vidioc prefix (with
typo) and enum_fmt, but here it doesn't have a prefix and is enum_format.

> +{
> +	struct mtk_fd_video_device *node = mtk_fd_file_to_node(file);
> +
> +	if (f->index > 0 || f->type != node->dev_q.vbq.type)

There is no need to check the type, as the core should already check it for
you.

> +		return -EINVAL;
> +
> +	strscpy(f->description, node->desc->description,
> +		sizeof(f->description));
> +
> +	f->pixelformat = node->vdev_fmt.fmt.meta.dataformat;

Also set flags to 0.

> +
> +	return 0;
> +}
> +
> +static int mtk_fd_videoc_g_meta_fmt(struct file *file,
> +				    void *fh, struct v4l2_format *f)
> +{
> +	struct mtk_fd_video_device *node = mtk_fd_file_to_node(file);

The Linux coding style requires 1 blank line between variable declarations
and code.

> +	*f = node->vdev_fmt;
> +
> +	return 0;
> +}
> +
> +static int
> +mtk_fd_vidioc_subscribe_event(struct v4l2_fh *fh,
> +			      const struct v4l2_event_subscription *sub)
> +{
> +	switch (sub->type) {
> +	case V4L2_EVENT_CTRL:
> +		return v4l2_ctrl_subscribe_event(fh, sub);
> +	default:
> +		return -EINVAL;
> +	}
> +}

This driver doesn't seem to support any controls, so there is no point in
supporting the above event.

[snip]
> +static void mtk_fd_node_to_v4l2(struct mtk_fd_pipe *fd_pipe,
> +				u32 idx,
> +				 struct video_device *vdev,
> +				 struct v4l2_format *f)
> +{
> +	struct mtk_fd_video_device *node = &fd_pipe->nodes[idx];
> +
> +	vdev->ioctl_ops = node->desc->ops;
> +	vdev->device_caps = V4L2_CAP_STREAMING | node->desc->cap;
> +	f->type = node->desc->buf_type;
> +	mtk_fd_pipe_load_default_fmt(fd_pipe, node, f);
> +}

This function is only called once, is very short and has a very misleading
name (this kind of name is used for functions that convert things).
Just move the code back to the caller.

> +
> +int mtk_fd_dev_media_register(struct device *dev,
> +			      struct media_device *media_dev,
> +			      const char *model)
> +{
> +	int ret = 0;
> +
> +	media_dev->dev = dev;
> +	dev_dbg(dev, "setup media_dev.dev: %p\n",
> +		media_dev->dev);

I don't think these logs every second line are useful even for debugging.
Please remove.

> +
> +	strlcpy(media_dev->model, model,
> +		sizeof(media_dev->model));

No need to pass model here as an argument. Just write the string here
directly.

> +	dev_dbg(dev, "setup media_dev.model: %s\n",
> +		media_dev->model);
> +
> +	snprintf(media_dev->bus_info, sizeof(media_dev->bus_info),
> +		 "platform:%s", dev_name(dev));
> +	dev_dbg(dev, "setup media_dev.bus_info: %s\n",
> +		media_dev->bus_info);
> +
> +	media_dev->hw_revision = 0;
> +	dev_dbg(dev, "setup media_dev.hw_revision: %d\n",
> +		media_dev->hw_revision);

No need to explicitly initialize to 0.

> +
> +	media_dev->ops = &mtk_fd_media_req_ops;
> +
> +	dev_dbg(dev, "media_device_init: media_dev:%p\n",
> +		media_dev);
> +	media_device_init(media_dev);
> +
> +	pr_debug("Register media device: %s, %p",
> +		 media_dev->model,
> +		media_dev);
> +
> +	ret = media_device_register(media_dev);
> +
> +	if (ret) {
> +		dev_err(dev, "failed to register media device (%d)\n", ret);
> +		goto fail_media_dev;
> +	}
> +	return 0;
> +
> +fail_media_dev:
> +	media_device_unregister(media_dev);

We jump here even if media_device_register() failed. Unregistering something
that wasn't registered doesn't sound like a good idea.

> +	media_device_cleanup(media_dev);
> +
> +	return ret;
> +}

There isn't much happening in this function. Perhaps just move the code back
to the caller?

> +
> +int mtk_fd_dev_v4l2_register(struct device *dev,
> +			     struct media_device *media_dev,
> +			     struct v4l2_device *v4l2_dev)
> +{
> +	int ret = 0;

Add a blank line between variable declarations and code.

> +	/* Set up v4l2 device */
> +	v4l2_dev->mdev = media_dev;
> +	dev_dbg(dev, "setup v4l2_dev->mdev: %p",
> +		v4l2_dev->mdev);

Please clean up such debugging messages, it makes it much harder to review
the code.

> +	v4l2_dev->ctrl_handler = NULL;

No need for explicit NULL assignments, as the structure was zero-filled
already.

> +	dev_dbg(dev, "setup v4l2_dev->ctrl_handler: %p",
> +		v4l2_dev->ctrl_handler);
> +
> +	pr_debug("Register v4l2 device: %p",
> +		 v4l2_dev);

dev_dbg()? But I would still just remove it.

> +
> +	ret = v4l2_device_register(dev, v4l2_dev);
> +
> +	if (ret) {
> +		dev_err(dev, "failed to register V4L2 device (%d)\n", ret);
> +		goto fail_v4l2_dev;
> +	}
> +
> +	return 0;
> +
> +fail_v4l2_dev:
> +	media_device_unregister(media_dev);
> +	media_device_cleanup(media_dev);
> +
> +	return ret;
> +}
> +

There isn't much happening in this function. Perhaps just move the code back
to the caller?

> +int mtk_fd_pipe_v4l2_register(struct mtk_fd_pipe *fd_pipe,
> +			      struct media_device *media_dev,
> +			      struct v4l2_device *v4l2_dev)
> +{
> +	int i, ret;
> +
> +	/* Initialize miscellaneous variables */
> +	fd_pipe->streaming = 0;
> +
> +	/* Initialize subdev media entity */
> +	fd_pipe->subdev_pads = kcalloc(fd_pipe->num_nodes,
> +				       sizeof(*fd_pipe->subdev_pads),
> +					GFP_KERNEL);
> +	if (!fd_pipe->subdev_pads) {
> +		ret = -ENOMEM;
> +		goto fail_subdev_pads;

There isn't anything to clean up at this point, so just return.

> +	}
> +
> +	ret = media_entity_pads_init(&fd_pipe->subdev.entity,
> +				     fd_pipe->num_nodes,
> +				     fd_pipe->subdev_pads);
> +	if (ret) {
> +		dev_err(&fd_pipe->fd_dev->pdev->dev,
> +			"failed initialize subdev media entity (%d)\n", ret);
> +		goto fail_media_entity;

Please name the labels after the next cleanup step to happen after jumping
to it.

> +	}
> +
> +	/* Initialize subdev */
> +	v4l2_subdev_init(&fd_pipe->subdev, &mtk_fd_subdev_ops);
> +
> +	fd_pipe->subdev.entity.function =
> +		MEDIA_ENT_F_PROC_VIDEO_PIXEL_FORMATTER;
> +
> +	fd_pipe->subdev.entity.ops = &mtk_fd_media_ops;
> +
> +	for (i = 0; i < fd_pipe->num_nodes; i++) {
> +		struct mtk_fd_video_device_desc *desc =
> +			fd_pipe->nodes[i].desc;
> +
> +		fd_pipe->subdev_pads[i].flags =
> +			V4L2_TYPE_IS_OUTPUT(desc->buf_type) ?
> +			MEDIA_PAD_FL_SINK : MEDIA_PAD_FL_SOURCE;
> +	}
> +
> +	fd_pipe->subdev.flags =
> +		V4L2_SUBDEV_FL_HAS_DEVNODE | V4L2_SUBDEV_FL_HAS_EVENTS;
> +	snprintf(fd_pipe->subdev.name, sizeof(fd_pipe->subdev.name),
> +		 "%s", fd_pipe->desc->name);
> +	v4l2_set_subdevdata(&fd_pipe->subdev, fd_pipe);
> +	fd_pipe->subdev.ctrl_handler = NULL;
> +	fd_pipe->subdev.internal_ops = &mtk_fd_subdev_internal_ops;

The above code registers a subdev, so it sounds like it could be a separate
function.

> +
> +	dev_dbg(&fd_pipe->fd_dev->pdev->dev,
> +		"register subdev: %s, ctrl_handler %p\n",
> +		 fd_pipe->subdev.name, fd_pipe->subdev.ctrl_handler);
> +	ret = v4l2_device_register_subdev(&fd_pipe->fd_dev->v4l2_dev,
> +					  &fd_pipe->subdev);
> +	if (ret) {
> +		dev_err(&fd_pipe->fd_dev->pdev->dev,
> +			"failed initialize subdev (%d)\n", ret);
> +		goto fail_subdev;
> +	}
> +
> +	ret = v4l2_device_register_subdev_nodes(&fd_pipe->fd_dev->v4l2_dev);
> +	if (ret) {
> +		dev_err(&fd_pipe->fd_dev->pdev->dev,
> +			"failed to register subdevs (%d)\n", ret);
> +		goto fail_subdevs;
> +	}

This isn't per-pipe, but global for the whole v4l2_device. It should be
called after all subdevs are fully initialized, to expose the device nodes
to the userspace atomically.

> +
> +	/* Create video nodes and links */
> +	for (i = 0; i < fd_pipe->num_nodes; i++) {

Please move the contents of the loop into a separate function that handles
one node.

> +		struct mtk_fd_video_device *node = &fd_pipe->nodes[i];
> +		struct video_device *vdev = &node->vdev;
> +		struct vb2_queue *vbq = &node->dev_q.vbq;
> +		struct mtk_fd_video_device_desc *desc = node->desc;
> +		u32 flags;
> +
> +		/* Initialize miscellaneous variables */
> +		mutex_init(&node->dev_q.lock);

Please just use the video_device lock for vb2 queues too. It simplifies the
overall driver locking and doesn't really have any practical performance
difference.

> +
> +		/* Initialize formats to default values */
> +		mtk_fd_node_to_v4l2(fd_pipe, i, vdev, &node->vdev_fmt);
> +
> +		/* Initialize media entities */
> +		ret = media_entity_pads_init(&vdev->entity, 1, &node->vdev_pad);
> +		if (ret) {
> +			dev_err(&fd_pipe->fd_dev->pdev->dev,

This kind of long chains of pointer dereferences signal a problem in the
design of driver structures and/or function arguments.

I'd suggest passing fd_dev to this function and also storing dev instead of
pdev inside fd_dev.

> +				"failed initialize media entity (%d)\n", ret);
> +			goto fail_vdev_media_entity;
> +		}
> +
> +		node->vdev_pad.flags = V4L2_TYPE_IS_OUTPUT(desc->buf_type) ?
> +			MEDIA_PAD_FL_SOURCE : MEDIA_PAD_FL_SINK;
> +		vdev->entity.ops = NULL;
> +
> +		/* Initialize vbq */
> +		vbq->type = node->vdev_fmt.type;
> +		vbq->io_modes = VB2_MMAP | VB2_DMABUF;
> +		vbq->ops = &mtk_fd_vb2_ops;
> +		vbq->mem_ops = &vb2_dma_contig_memops;
> +		vbq->supports_requests = true;
> +		vbq->buf_struct_size = sizeof(struct mtk_fd_dev_buffer);
> +		vbq->timestamp_flags = V4L2_BUF_FLAG_TIMESTAMP_MONOTONIC;

This is a mem2mem device so the timestamps should be copied from OUTPUT to
CAPTURE. Please set the flag appropriately.

> +		vbq->min_buffers_needed = 0;
> +		/* Put the process hub sub device in the vb2 private data*/
> +		vbq->drv_priv = fd_pipe;
> +		vbq->lock = &node->dev_q.lock;
> +		ret = vb2_queue_init(vbq);
> +		if (ret) {
> +			dev_err(&fd_pipe->fd_dev->pdev->dev,
> +				"failed to initialize video queue (%d)\n", ret);
> +			goto fail_vdev;
> +		}
> +
> +		/* Initialize vdev */
> +		snprintf(vdev->name, sizeof(vdev->name), "%s %s",
> +			 fd_pipe->desc->name,
> +			 node->desc->name);
> +		vdev->release = video_device_release_empty;
> +		vdev->fops = &mtk_fd_v4l2_fops;
> +		vdev->lock = &node->dev_q.lock;

Aha, so it's in fact the same lock. Please move it to the "node" struct
then.

> +		vdev->ctrl_handler = NULL;
> +		vdev->v4l2_dev = &fd_pipe->fd_dev->v4l2_dev;
> +		vdev->queue = &node->dev_q.vbq;
> +		vdev->vfl_dir = V4L2_TYPE_IS_OUTPUT(desc->buf_type) ?
> +			VFL_DIR_TX : VFL_DIR_RX;
> +		video_set_drvdata(vdev, fd_pipe);
> +		pr_debug("register vdev: %s\n", vdev->name);

dev_dbg()?

> +		ret = video_register_device(vdev, VFL_TYPE_GRABBER, -1);
> +		if (ret) {
> +			dev_err(&fd_pipe->fd_dev->pdev->dev,
> +				"failed to register video device (%d)\n", ret);
> +			goto fail_vdev;
> +		}
> +
> +		/* Create link between video node and the subdev pad */
> +		flags = 0;
> +		if (desc->dynamic)
> +			flags |= MEDIA_LNK_FL_DYNAMIC;
> +		if (node->enabled)
> +			flags |= MEDIA_LNK_FL_ENABLED;
> +		if (node->immutable)
> +			flags |= MEDIA_LNK_FL_IMMUTABLE;

Wouldn't all the nodes be always ENABLED and IMMUTABLE and not DYNAMIC for
this driver?

> +
> +		if (V4L2_TYPE_IS_OUTPUT(desc->buf_type))
> +			ret = media_create_pad_link(&vdev->entity, 0,
> +						    &fd_pipe->subdev.entity,
> +						    i, flags);
> +		else
> +			ret = media_create_pad_link(&fd_pipe->subdev.entity,
> +						    i, &vdev->entity, 0,
> +						    flags);
> +

No need for this blank line.

> +		if (ret)
> +			goto fail_link;
> +	}
> +
> +	return 0;
> +
> +	for (; i >= 0; i--) {
> +fail_link:
> +		video_unregister_device(&fd_pipe->nodes[i].vdev);
> +fail_vdev:
> +		vb2_queue_release(&fd_pipe->nodes[i].dev_q.vbq);
> +		media_entity_cleanup(&fd_pipe->nodes[i].vdev.entity);
> +fail_vdev_media_entity:
> +		mutex_destroy(&fd_pipe->nodes[i].dev_q.lock);
> +	}
> +fail_subdevs:
> +	v4l2_device_unregister_subdev(&fd_pipe->subdev);
> +fail_subdev:
> +	media_entity_cleanup(&fd_pipe->subdev.entity);
> +fail_media_entity:
> +	kfree(fd_pipe->subdev_pads);
> +fail_subdev_pads:
> +	v4l2_device_unregister(&fd_pipe->fd_dev->v4l2_dev);

We haven't registered the v4l2_device in this function.

> +	pr_err("fail_v4l2_dev: media_device_unregister and clenaup:%p",
> +	       &fd_pipe->fd_dev->mdev);

Error messages should be printed at the place of the failure.

> +	media_device_unregister(&fd_pipe->fd_dev->mdev);
> +	media_device_cleanup(&fd_pipe->fd_dev->mdev);

We haven't registered or initialized media_device in this function.

> +
> +	return ret;
> +}
> +
> +int mtk_fd_pipe_v4l2_unregister(struct mtk_fd_pipe *fd_pipe)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < fd_pipe->num_nodes; i++) {
> +		video_unregister_device(&fd_pipe->nodes[i].vdev);
> +		vb2_queue_release(&fd_pipe->nodes[i].dev_q.vbq);
> +		media_entity_cleanup(&fd_pipe->nodes[i].vdev.entity);
> +		mutex_destroy(&fd_pipe->nodes[i].dev_q.lock);
> +	}
> +
> +	v4l2_device_unregister_subdev(&fd_pipe->subdev);
> +	media_entity_cleanup(&fd_pipe->subdev.entity);
> +	kfree(fd_pipe->subdev_pads);
> +	v4l2_device_unregister(&fd_pipe->fd_dev->v4l2_dev);
> +	media_device_unregister(&fd_pipe->fd_dev->mdev);
> +	media_device_cleanup(&fd_pipe->fd_dev->mdev);

Please make this consistent with the registration functions. For each
registration function there should be a matching unregister function that
cleans up only whatever was registered in that function.

> +
> +	return 0;
> +}
> +
> +void mtk_fd_v4l2_buffer_done(struct vb2_buffer *vb,
> +			     enum vb2_buffer_state state)
> +{
> +	struct mtk_fd_pipe *fd_pipe;
> +	struct mtk_fd_video_device *node;
> +
> +	fd_pipe = vb2_get_drv_priv(vb->vb2_queue);
> +	node = mtk_fd_vbq_to_node(vb->vb2_queue);
> +	dev_dbg(&fd_pipe->fd_dev->pdev->dev,
> +		"%s:%s: return buf, idx(%d), state(%d)\n",
> +		fd_pipe->desc->name, node->desc->name,
> +		vb->index, state);
> +	vb2_buffer_done(vb, state);
> +}

No need for this function. Just call vb2_buffer_done() directly from the
caller. (I already mentioned this in MTK DIP driver review. Please
coordinate with other driver owners and make sure that similar comments are
addressed in all drivers...)

> +
> +/********************************************
> + * MTK FD V4L2 Settings *
> + ********************************************/
> +
> +static const struct v4l2_ioctl_ops mtk_fd_v4l2_video_out_ioctl_ops = {
> +	.vidioc_querycap = mtk_fd_videoc_querycap,
> +	.vidioc_enum_framesizes = mtk_fd_videoc_enum_framesizes,
> +	.vidioc_enum_fmt_vid_cap_mplane = mtk_fd_videoc_enum_fmt,
> +	.vidioc_g_fmt_vid_cap_mplane = mtk_fd_videoc_g_fmt,
> +	.vidioc_s_fmt_vid_cap_mplane = mtk_fd_videoc_s_fmt,
> +	.vidioc_try_fmt_vid_cap_mplane = mtk_fd_videoc_try_fmt,

No need for *cap* ops if this is only for an OUTPUT device.

> +	.vidioc_enum_fmt_vid_out_mplane = mtk_fd_videoc_enum_fmt,
> +	.vidioc_g_fmt_vid_out_mplane = mtk_fd_videoc_g_fmt,
> +	.vidioc_s_fmt_vid_out_mplane = mtk_fd_videoc_s_fmt,
> +	.vidioc_try_fmt_vid_out_mplane = mtk_fd_videoc_try_fmt,
> +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> +	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> +	.vidioc_querybuf = vb2_ioctl_querybuf,
> +	.vidioc_qbuf = vb2_ioctl_qbuf,
> +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
> +	.vidioc_streamon = vb2_ioctl_streamon,
> +	.vidioc_streamoff = vb2_ioctl_streamoff,
> +	.vidioc_expbuf = vb2_ioctl_expbuf,
> +	.vidioc_subscribe_event = mtk_fd_vidioc_subscribe_event,
> +	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> +
> +};
> +
> +static const struct v4l2_ioctl_ops mtk_fd_v4l2_video_cap_ioctl_ops = {
> +	.vidioc_querycap = mtk_fd_videoc_querycap,
> +	.vidioc_enum_framesizes = mtk_fd_videoc_enum_framesizes,
> +	.vidioc_enum_fmt_vid_cap_mplane = mtk_fd_videoc_enum_fmt,
> +	.vidioc_g_fmt_vid_cap_mplane = mtk_fd_videoc_g_fmt,
> +	.vidioc_s_fmt_vid_cap_mplane = mtk_fd_videoc_s_fmt,
> +	.vidioc_try_fmt_vid_cap_mplane = mtk_fd_videoc_try_fmt,
> +	.vidioc_enum_fmt_vid_out_mplane = mtk_fd_videoc_enum_fmt,
> +	.vidioc_g_fmt_vid_out_mplane = mtk_fd_videoc_g_fmt,
> +	.vidioc_s_fmt_vid_out_mplane = mtk_fd_videoc_s_fmt,
> +	.vidioc_try_fmt_vid_out_mplane = mtk_fd_videoc_try_fmt,
> +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> +	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> +	.vidioc_querybuf = vb2_ioctl_querybuf,
> +	.vidioc_qbuf = vb2_ioctl_qbuf,
> +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
> +	.vidioc_streamon = vb2_ioctl_streamon,
> +	.vidioc_streamoff = vb2_ioctl_streamoff,
> +	.vidioc_expbuf = vb2_ioctl_expbuf,
> +	.vidioc_subscribe_event = mtk_fd_vidioc_subscribe_event,
> +	.vidioc_unsubscribe_event = v4l2_event_unsubscribe,
> +
> +};

This structure is unused.

> +
> +static const struct v4l2_ioctl_ops mtk_fd_v4l2_meta_out_ioctl_ops = {
> +	.vidioc_querycap = mtk_fd_videoc_querycap,
> +
> +	.vidioc_enum_fmt_meta_cap = mtk_fd_meta_enum_format,
> +	.vidioc_g_fmt_meta_cap = mtk_fd_videoc_g_meta_fmt,
> +	.vidioc_s_fmt_meta_cap = mtk_fd_videoc_g_meta_fmt,
> +	.vidioc_try_fmt_meta_cap = mtk_fd_videoc_g_meta_fmt,
> +
> +	.vidioc_enum_fmt_meta_out = mtk_fd_meta_enum_format,
> +	.vidioc_g_fmt_meta_out = mtk_fd_videoc_g_meta_fmt,
> +	.vidioc_s_fmt_meta_out = mtk_fd_videoc_g_meta_fmt,
> +	.vidioc_try_fmt_meta_out = mtk_fd_videoc_g_meta_fmt,
> +
> +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> +	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> +	.vidioc_querybuf = vb2_ioctl_querybuf,
> +	.vidioc_qbuf = vb2_ioctl_qbuf,
> +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
> +	.vidioc_streamon = vb2_ioctl_streamon,
> +	.vidioc_streamoff = vb2_ioctl_streamoff,
> +	.vidioc_expbuf = vb2_ioctl_expbuf,
> +};
> +
> +static const struct v4l2_ioctl_ops mtk_fd_v4l2_meta_cap_ioctl_ops = {
> +	.vidioc_querycap = mtk_fd_videoc_querycap,
> +
> +	.vidioc_enum_fmt_meta_cap = mtk_fd_meta_enum_format,
> +	.vidioc_g_fmt_meta_cap = mtk_fd_videoc_g_meta_fmt,
> +	.vidioc_s_fmt_meta_cap = mtk_fd_videoc_g_meta_fmt,
> +	.vidioc_try_fmt_meta_cap = mtk_fd_videoc_g_meta_fmt,
> +
> +	.vidioc_enum_fmt_meta_out = mtk_fd_meta_enum_format,
> +	.vidioc_g_fmt_meta_out = mtk_fd_videoc_g_meta_fmt,
> +	.vidioc_s_fmt_meta_out = mtk_fd_videoc_g_meta_fmt,
> +	.vidioc_try_fmt_meta_out = mtk_fd_videoc_g_meta_fmt,
> +
> +	.vidioc_reqbufs = vb2_ioctl_reqbufs,
> +	.vidioc_create_bufs = vb2_ioctl_create_bufs,
> +	.vidioc_prepare_buf = vb2_ioctl_prepare_buf,
> +	.vidioc_querybuf = vb2_ioctl_querybuf,
> +	.vidioc_qbuf = vb2_ioctl_qbuf,
> +	.vidioc_dqbuf = vb2_ioctl_dqbuf,
> +	.vidioc_streamon = vb2_ioctl_streamon,
> +	.vidioc_streamoff = vb2_ioctl_streamoff,
> +	.vidioc_expbuf = vb2_ioctl_expbuf,
> +};

Aren't the 2 structures above identical? Should be merged if so.

[snip]
> +int mtk_fd_dev_v4l2_init(struct mtk_fd_dev *fd_dev)
> +{
> +	struct media_device *media_dev;
> +	struct v4l2_device *v4l2_dev;
> +	struct mtk_fd_smem_dev *smem_alloc_dev = &fd_dev->smem_alloc_dev;
> +	int i;
> +	int ret = 0;

Please don't initialize local variables unless that's needed by the logic.
It prevents the compiler from detecting missing assignments.

> +
> +	media_dev = &fd_dev->mdev;
> +	v4l2_dev = &fd_dev->v4l2_dev;

Just pass fd_dev to the functions below. No need to extract only some
fields.

> +
> +	ret = mtk_fd_dev_media_register(&fd_dev->pdev->dev,
> +					media_dev,
> +					 MTK_FD_PIPE_MEDIA_MODEL_NAME);

We should bail out on error.

> +
> +	ret = mtk_fd_dev_v4l2_register(&fd_dev->pdev->dev,
> +				       media_dev,
> +					v4l2_dev);

We should clean up the previous steps and bail out on error.

> +
> +	ret = mtk_fd_smem_alloc_dev_init(smem_alloc_dev, &fd_dev->pdev->dev);

Ditto.

> +
> +	for (i = 0; i < MTK_FD_PIPE_ID_TOTAL_NUM; i++) {
> +		ret = mtk_fd_pipe_init(&fd_dev->fd_pipe[i], fd_dev,
> +				       &pipe_settings[i],
> +					media_dev, v4l2_dev, smem_alloc_dev);
> +		if (ret) {
> +			dev_err(&fd_dev->pdev->dev,
> +				"%s: Pipe id(%d) init failed(%d)\n",
> +				fd_dev->fd_pipe[i].desc->name,
> +				i, ret);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +void mtk_fd_dev_v4l2_release(struct mtk_fd_dev *fd_dev)
> +{
> +	int i = 0;

No need for initialization.

> +
> +	if (fd_dev)

Why could it ever be NULL?

> +		for (i = 0; i < MTK_FD_PIPE_ID_TOTAL_NUM; i++)
> +			mtk_fd_pipe_release(&fd_dev->fd_pipe[i]);
> +
> +	mtk_fd_smem_alloc_dev_release(&fd_dev->smem_alloc_dev);
> +}
> +

[snip]

> +static int mtk_fd_probe(struct platform_device *pdev)
> +{
> +	struct mtk_fd_dev *fd_dev;
> +	struct mtk_fd_hw *fd_hw;
> +	struct device_node *node;
> +	struct platform_device *larb_pdev;
> +	int irq_num;
> +	int ret;
> +
> +	fd_dev = devm_kzalloc(&pdev->dev, sizeof(*fd_dev), GFP_KERNEL);
> +

nit: No need for this blank line, because the if below is directly related.

> +	if (!fd_dev)
> +		return -ENOMEM;
> +
> +	dev_set_drvdata(&pdev->dev, fd_dev);
> +	fd_hw = &fd_dev->fd_hw;
> +
> +	if (!fd_hw) {

How is this possible for a struct member?

> +		dev_err(&pdev->dev, "Unable to allocate fd_hw\n");
> +		return -ENOMEM;
> +	}
> +
> +	fd_dev->pdev = pdev;
> +
> +	irq_num = irq_of_parse_and_map(pdev->dev.of_node, FD_IRQ_IDX);

We should use platform_get_irq() here instead, because the IRQs were already
parsed for us when the platform core created the platform_device.

> +	ret = request_irq(irq_num, (irq_handler_t)mtk_fd_irq,
> +			  IRQF_TRIGGER_NONE, FD_DRVNAME, fd_hw);

It should be a device name, not driver name. One would normally use
dev_name() here.

Also devm_request_irq() should simplify the cleanup.

> +	if (ret) {
> +		dev_dbg(&pdev->dev, "%s request_irq fail, irq=%d\n",
> +			__func__, irq_num);

This is an error, so dev_err().

> +		return ret;
> +	}
> +	dev_dbg(&pdev->dev, "irq_num=%d\n", irq_num);

That's probably not very useful.

> +
> +	node = of_parse_phandle(pdev->dev.of_node, "mediatek,larb", 0);
> +	if (!node) {
> +		dev_err(&pdev->dev, "no mediatek, larb found");
> +		return -EINVAL;
> +	}
> +	larb_pdev = of_find_device_by_node(node);
> +	if (!larb_pdev) {
> +		dev_err(&pdev->dev, "no mediatek, larb device found");
> +		return -EINVAL;
> +	}
> +	fd_hw->larb_dev = &larb_pdev->dev;
> +

LARBs are handled automatically by the IOMMU driver, no need to do anything
with them explicitly anymore.

> +	node = pdev->dev.of_node;
> +	if (!node) {
> +		dev_err(&pdev->dev, "find fd node failed!!!\n");
> +		return -ENODEV;
> +	}
> +
> +	fd_hw->fd_base = of_iomap(node, 0);

One would normally use platform_get_resource() and devm_ioremap_resource()
here.

> +
> +	if (!fd_hw->fd_base) {
> +		dev_err(&pdev->dev, "unable to map fd node!!!\n");
> +		return -ENODEV;
> +	}
> +
> +	dev_dbg(&pdev->dev, "fd_hw->fd_base: %lx\n",
> +		(unsigned long)fd_hw->fd_base);

Not very useful either.

> +
> +	fd_hw->fd_clk = devm_clk_get(&pdev->dev, "FD_CLK_IMG_FD");

Clock names should be lowercase and name just inputs of the IP block, so
simply "fd", should be enough.

> +	if (IS_ERR(fd_hw->fd_clk)) {
> +		dev_err(&pdev->dev, "cannot get FD_CLK_IMG_FD clock\n");
> +		return PTR_ERR(fd_hw->fd_clk);
> +	}
> +
> +	pm_runtime_enable(&pdev->dev);
> +	atomic_set(&fd_hw->fd_user_cnt, 0);
> +	init_waitqueue_head(&fd_hw->wq);
> +	mutex_init(&fd_hw->fd_hw_lock);
> +	fd_hw->fd_irq_result = 0;
> +
> +	ret = mtk_fd_dev_v4l2_init(fd_dev);
> +	if (ret)
> +		dev_err(&pdev->dev, "v4l2 init failed: %d\n", ret);

We should clean up and return the error code, not 0.

> +
> +	dev_info(&pdev->dev, "Mediatek Camera FD driver probe.\n");
> +
> +	return 0;
> +}
> +
> +static int mtk_fd_remove(struct platform_device *pdev)
> +{
> +	int irq_i4;
> +	struct mtk_fd_dev *fd_dev = dev_get_drvdata(&pdev->dev);
> +
> +	if (fd_dev) {
> +		mtk_fd_dev_v4l2_release(fd_dev);
> +	} else {

This is impossible.

> +		dev_err(&pdev->dev, "Can't find fd driver data\n");
> +		return -EINVAL;
> +	}
> +
> +	mutex_destroy(&fd_dev->fd_hw.fd_hw_lock);
> +	pm_runtime_disable(&pdev->dev);
> +
> +	irq_i4 = platform_get_irq(pdev, 0);
> +	free_irq(irq_i4, NULL);
> +	kfree(fd_dev);

fd_dev was allocated using devm_kzalloc(), no need to free it explicitly.

> +
> +	return 0;
> +}
> +
> +static int mtk_fd_suspend(struct device *dev)
> +{
> +	struct mtk_fd_dev *fd_dev;
> +	int ret;
> +
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	fd_dev = dev_get_drvdata(dev);
> +
> +	if (atomic_read(&fd_dev->fd_hw.fd_user_cnt) > 0) {
> +		ret = pm_runtime_put_sync(fd_dev->fd_hw.larb_dev);
> +		clk_disable_unprepare(fd_dev->fd_hw.fd_clk);
> +		return ret;
> +	}

This isn't going to work, because the hardware may be still processing a
frame at this point. You need a way to ensure that the hardware goes idle
here first and then in resume, you need to make the hardware continue when
it left before suspend.

> +	return 0;
> +}
> +
> +static int mtk_fd_resume(struct device *dev)
> +{
> +	struct mtk_fd_dev *fd_dev;
> +	int ret;
> +
> +	if (pm_runtime_suspended(dev))
> +		return 0;
> +
> +	fd_dev = dev_get_drvdata(dev);
> +
> +	if (atomic_read(&fd_dev->fd_hw.fd_user_cnt) > 0) {
> +		ret = pm_runtime_get_sync(fd_dev->fd_hw.larb_dev);
> +		if (ret) {
> +			dev_dbg(&fd_dev->pdev->dev, "open larb clk failed\n");
> +			return ret;
> +		}
> +
> +		ret = clk_prepare_enable(fd_dev->fd_hw.fd_clk);
> +		if (ret) {
> +			dev_dbg(&fd_dev->pdev->dev, "open fd clk failed\n");
> +			return ret;
> +		}
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops mtk_fd_pm_ops = {
> +	SET_SYSTEM_SLEEP_PM_OPS(mtk_fd_suspend, mtk_fd_resume)
> +	SET_RUNTIME_PM_OPS(mtk_fd_suspend, mtk_fd_resume, NULL)
> +};
> +
> +static const struct of_device_id mtk_fd_of_ids[] = {
> +	{ .compatible = "mediatek,mt8183-fd", },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_fd_of_ids);
> +
> +static struct platform_driver mtk_fd_driver = {
> +	.probe   = mtk_fd_probe,
> +	.remove  = mtk_fd_remove,
> +	.driver  = {
> +		.name  = FD_DRVNAME,

Please just set the name explicitly here and remove the macro.

> +		.of_match_table = mtk_fd_of_ids,

Please use of_match_ptr().

> +		.pm = &mtk_fd_pm_ops,
> +	}
> +};
> +module_platform_driver(mtk_fd_driver);
> +
> +MODULE_DESCRIPTION("Mediatek FD driver");
> +MODULE_LICENSE("GPL");

GPL v2

> -- 
> 2.18.0
>