From: Enric Balletbo Serra <eballetbo@gmail.com>
To: Moudy Ho <moudy.ho@mediatek.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
Rob Herring <robh+dt@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Hans Verkuil <hverkuil-cisco@xs4all.nl>,
Jernej Skrabec <jernej.skrabec@siol.net>,
Maoguang Meng <maoguang.meng@mediatek.com>,
Krzysztof Kozlowski <krzk@kernel.org>,
daoyuan huang <daoyuan.huang@mediatek.com>,
Ping-Hsun Wu <ping-hsun.wu@mediatek.com>,
Geert Uytterhoeven <geert+renesas@glider.be>,
Rob Landley <rob@landley.net>,
Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org,
tfiga@chromium.org, drinkcat@chromium.org, acourbot@chromium.org,
pihsun@chromium.org, menghui.lin@mediatek.com,
sj.huang@mediatek.com, ben.lok@mediatek.com,
randy.wu@mediatek.com, srv_heupstream@mediatek.com,
frederic.chen@mediatek.com
Subject: Re: [RFC PATCH V0 01/10] media: mtk-mdp3: Add Mediatek MDP Driver
Date: Fri, 2 Jul 2021 14:00:28 +0200 [thread overview]
Message-ID: <CAFqH_51+wQ=MxOKAfjA7nBspjcqfsRPy0oxg3iripuYJQ3HEmA@mail.gmail.com> (raw)
In-Reply-To: <20210623073549.24170-1-moudy.ho@mediatek.com>
Hi Mody Ho,
Thank you for your patch. Some comments below.
Missatge de Moudy Ho <moudy.ho@mediatek.com> del dia dc., 23 de juny
2021 a les 9:36:
>
> From: mtk18742 <moudy.ho@mediatek.com>
>
Please fix your git configuration, I assume this should be something
like Moudy Ho <moudy.ho@mediatek.com>. Note that the author and the
Signed-off must match.
> Add MDP driver for MT8183
>
In general please use a more verbose commit messages, you know what
MDP means (I probably also know, but not everyone knows what MDP
means) It is a good practice to when you introduce a new driver to
explain well what is and for what is used.
The first question that I have here is why current MDP driver (that
supports other Mediatek chips like the MT8173) can't be extended to
support MT8183. How different is this MDP driver and why can't share
the code.
This single patch is huge, and is unlikely that anyone spend time
reviewing it just for that. I'd recommend to split it and introduce
step-by-step, functionality by functionally. I also have the
impression that you're just reimplementing things that are actually
upstream or not taking advantage of what is already upstream. i.e the
access to the mmsys registers. Please use current kernel
drivers/interfaces API instead of reimplementing it.
I'd strongly recommend to look at current Mediatek mdp driver and see
if you can add support for MT8183 there, and if not, looks at it as
reference.
Best regards,
Enric
> Signed-off-by: Moudy Ho <moudy.ho@mediatek.com>
> ---
> drivers/media/platform/Kconfig | 17 +
> drivers/media/platform/Makefile | 2 +
> drivers/media/platform/mtk-mdp3/Makefile | 9 +
> drivers/media/platform/mtk-mdp3/isp_reg.h | 37 +
> .../media/platform/mtk-mdp3/mdp-platform.h | 58 +
> .../media/platform/mtk-mdp3/mdp_reg_ccorr.h | 75 +
> .../media/platform/mtk-mdp3/mdp_reg_rdma.h | 206 +++
> drivers/media/platform/mtk-mdp3/mdp_reg_rsz.h | 109 ++
> .../media/platform/mtk-mdp3/mdp_reg_wdma.h | 125 ++
> .../media/platform/mtk-mdp3/mdp_reg_wrot.h | 115 ++
> .../media/platform/mtk-mdp3/mmsys_config.h | 188 +++
> drivers/media/platform/mtk-mdp3/mmsys_mutex.h | 35 +
> .../media/platform/mtk-mdp3/mmsys_reg_base.h | 38 +
> drivers/media/platform/mtk-mdp3/mtk-img-ipi.h | 282 ++++
> .../media/platform/mtk-mdp3/mtk-mdp3-cmdq.c | 521 +++++++
> .../media/platform/mtk-mdp3/mtk-mdp3-cmdq.h | 54 +
> .../media/platform/mtk-mdp3/mtk-mdp3-comp.c | 1329 +++++++++++++++++
> .../media/platform/mtk-mdp3/mtk-mdp3-comp.h | 155 ++
> .../media/platform/mtk-mdp3/mtk-mdp3-core.c | 282 ++++
> .../media/platform/mtk-mdp3/mtk-mdp3-core.h | 86 ++
> .../media/platform/mtk-mdp3/mtk-mdp3-debug.c | 973 ++++++++++++
> .../media/platform/mtk-mdp3/mtk-mdp3-debug.h | 39 +
> .../media/platform/mtk-mdp3/mtk-mdp3-m2m.c | 804 ++++++++++
> .../media/platform/mtk-mdp3/mtk-mdp3-m2m.h | 42 +
> .../media/platform/mtk-mdp3/mtk-mdp3-regs.c | 748 ++++++++++
> .../media/platform/mtk-mdp3/mtk-mdp3-regs.h | 373 +++++
> .../media/platform/mtk-mdp3/mtk-mdp3-vpu.c | 313 ++++
> .../media/platform/mtk-mdp3/mtk-mdp3-vpu.h | 79 +
> 28 files changed, 7094 insertions(+)
> create mode 100644 drivers/media/platform/mtk-mdp3/Makefile
> create mode 100644 drivers/media/platform/mtk-mdp3/isp_reg.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mdp-platform.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_ccorr.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_rdma.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_rsz.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_wdma.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mdp_reg_wrot.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mmsys_config.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mmsys_mutex.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mmsys_reg_base.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-img-ipi.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-cmdq.c
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-cmdq.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-comp.c
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-comp.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-core.c
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-core.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-debug.c
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-debug.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-m2m.c
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-m2m.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-regs.c
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-regs.h
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-vpu.c
> create mode 100644 drivers/media/platform/mtk-mdp3/mtk-mdp3-vpu.h
[snip]
> --
> 2.18.0
>
prev parent reply other threads:[~2021-07-02 12:00 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-23 7:35 [RFC PATCH V0 01/10] media: mtk-mdp3: Add Mediatek MDP Driver Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 02/10] media: mtk-mdp3: fix redundant process done caused KE Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 03/10] media: mtk-mdp3: revise suspend strategy Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 04/10] media: mtk-mdp3: add error handling in error return Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 05/10] media: mtk-mdp3: remove unnecessary Null check Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 06/10] media: mtk-mdp3: move clock on to precise place Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 07/10] media: mtk-mdp3: Fix unpaired settings Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 08/10] media: mtk-mdp3: remove illegal device node usage Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 09/10] media: mtk-mdp3: revise error handling about get/probe MDP3 Moudy Ho
2021-06-23 7:35 ` [RFC PATCH V0 10/10] media: mtk-mdp3: Adjust related settings for 5.13-rc1 Moudy Ho
2021-06-23 10:41 ` [RFC PATCH V0 01/10] media: mtk-mdp3: Add Mediatek MDP Driver Chun-Kuang Hu
2021-07-02 12:00 ` Enric Balletbo Serra [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAFqH_51+wQ=MxOKAfjA7nBspjcqfsRPy0oxg3iripuYJQ3HEmA@mail.gmail.com' \
--to=eballetbo@gmail.com \
--cc=acourbot@chromium.org \
--cc=ben.lok@mediatek.com \
--cc=daoyuan.huang@mediatek.com \
--cc=devicetree@vger.kernel.org \
--cc=drinkcat@chromium.org \
--cc=frederic.chen@mediatek.com \
--cc=geert+renesas@glider.be \
--cc=hverkuil-cisco@xs4all.nl \
--cc=jernej.skrabec@siol.net \
--cc=krzk@kernel.org \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=maoguang.meng@mediatek.com \
--cc=matthias.bgg@gmail.com \
--cc=mchehab@kernel.org \
--cc=menghui.lin@mediatek.com \
--cc=moudy.ho@mediatek.com \
--cc=pihsun@chromium.org \
--cc=ping-hsun.wu@mediatek.com \
--cc=randy.wu@mediatek.com \
--cc=rob@landley.net \
--cc=robh+dt@kernel.org \
--cc=sj.huang@mediatek.com \
--cc=srv_heupstream@mediatek.com \
--cc=tfiga@chromium.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).