Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
From:   Guillaume Ranquet <granquet@baylibre.com>
Date:   Thu, 30 Sep 2021 11:30:16 -0400
Message-ID: <CABnWg9th7ZFqa48jKcjqs4u+27t663H-zD7yn3oFRMEsGzbMGg@mail.gmail.com>
Subject: Re: [PATCH v1 4/4] drm/mediatek: add mt8195 hdmi TX support
To:     Chun-Kuang Hu <chunkuang.hu@kernel.org>
Cc:     Philipp Zabel <p.zabel@pengutronix.de>,
        David Airlie <airlied@linux.ie>,
        Daniel Vetter <daniel@ffwll.ch>,
        Matthias Brugger <matthias.bgg@gmail.com>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@lists.infradead.org>,
        Linux ARM <linux-arm-kernel@lists.infradead.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Chun-Kuang.

Thank you for your input.
I have tried to find commonalities between the two drivers but I didn't
find enough shared code to warrant that architecture.
I'll have another look, especially now that I'm more familiar with the
driver.

Regarding 2, I have removed as much functionalities as I could from the
original vendor tree (like hdcp, hdr, cec...) to keep only HDMI audio and
video TX.

There might be some more things to remove, but I'm no expert in the domain
and I'm working without access to mediatek datasheets and documentation.

Though, at this stage I could split the patch in two with video first and
then audio.

I will try to work something out for a V2.

Thx,
Guillaume.

Quoting Chun-Kuang Hu (2021-09-30 15:36:42)
> Hi, Guillaume:
>
> This is a big patch, and I'm not familiar with this driver, so the
> review process would be long. So I tell you about how I review this
> patch, and if you could process according to my way, the process would
> be more short.
>
> 1. Find the common part of all hdmi driver.
> Even though mt8195 hdmi has many difference with other mediatek soc
> hdmi driver, I would like to find the common part and have just one
> copy of the common part. I expect there would three file finally:
>
> mtk_hdmi.c               (the common part)
> mtk_hdmi_mt8173.c (each soc special part)
> mtk_hdmi_mt8195.c (each soc special part)
>
> But this would be difficult in this stage, so you could temporarily
> have these three file:
>
> mtk_hdmi_common.c (the common part)
> mtk_hdmi.c                 (each soc special part)
> mtk_hdmi_mt8195.c   (each soc special part)
>
> When review is almost done, then change the file name as I wish.
>
> 2. The first patch has only basic function, separate advance function
> to another patch.
> When comparing mt8195 hdmi driver with other hdmi driver, if mt8195
> hdmi driver has some function that other hdmi does not have, I would
> think that function is advance function and should be separate to
> another patch.
>
> If you follow this way, I think the review process would be short.
> Because this patch is big, I would just review partial part each time.
>
> Regards,
> Chun-Kuang.
>
>
> Guillaume Ranquet <granquet@baylibre.com> =E6=96=BC 2021=E5=B9=B49=E6=9C=
=8829=E6=97=A5 =E9=80=B1=E4=B8=89 =E4=B8=8B=E5=8D=885:47=E5=AF=AB=E9=81=93=
=EF=BC=9A
> >
> > Add basic hdmi TX support for the mediatek mt8195 SoCs
> >
> > Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> > ---
> >  drivers/gpu/drm/mediatek/Kconfig              |   10 +
> >  drivers/gpu/drm/mediatek/Makefile             |    4 +-
> >  drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c    | 2293 +++++++++++++++++
> >  drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.h    |  128 +
> >  .../gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.c    |  530 ++++
> >  .../gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.h    |   20 +
> >  .../gpu/drm/mediatek/mtk_mt8195_hdmi_regs.h   |  329 +++
> >  7 files changed, 3313 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.h
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.c
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.h
> >  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_regs.h
> >
> > diff --git a/drivers/gpu/drm/mediatek/Kconfig b/drivers/gpu/drm/mediate=
k/Kconfig
> > index 2976d21e9a34a..517d065f0511b 100644
> > --- a/drivers/gpu/drm/mediatek/Kconfig
> > +++ b/drivers/gpu/drm/mediatek/Kconfig
> > @@ -28,3 +28,13 @@ config DRM_MEDIATEK_HDMI
> >         select PHY_MTK_HDMI
> >         help
> >           DRM/KMS HDMI driver for Mediatek SoCs
> > +
> > +config DRM_MEDIATEK_HDMI_MT8195_SUSPEND_LOW_POWER
> > +       tristate "DRM HDMI SUSPEND LOW POWER Support for Mediatek mt819=
5 SoCs"
> > +       depends on DRM_MEDIATEK_HDMI
> > +       help
> > +         DRM/KMS HDMI SUSPEND_LOW_POWER for Mediatek SoCs.
> > +         Choose this option if you want to disable/enable
> > +         clock and power domain when platform enter suspend,
> > +         and this config depends on DRM_MEDIATEK_HDMI.
> > +
> > diff --git a/drivers/gpu/drm/mediatek/Makefile b/drivers/gpu/drm/mediat=
ek/Makefile
> > index 29098d7c8307c..736f0816083d0 100644
> > --- a/drivers/gpu/drm/mediatek/Makefile
> > +++ b/drivers/gpu/drm/mediatek/Makefile
> > @@ -18,6 +18,8 @@ obj-$(CONFIG_DRM_MEDIATEK) +=3D mediatek-drm.o
> >
> >  mediatek-drm-hdmi-objs :=3D mtk_cec.o \
> >                           mtk_hdmi.o \
> > -                         mtk_hdmi_ddc.o
> > +                         mtk_hdmi_ddc.o \
> > +                         mtk_mt8195_hdmi.o \
> > +                         mtk_mt8195_hdmi_ddc.o \
> >
> >  obj-$(CONFIG_DRM_MEDIATEK_HDMI) +=3D mediatek-drm-hdmi.o
> > diff --git a/drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c b/drivers/gpu/d=
rm/mediatek/mtk_mt8195_hdmi.c
> > new file mode 100644
> > index 0000000000000..46c7c8af524ac
> > --- /dev/null
> >