Re: [PATCH v3 1/4] drm/mediatek: Move tz_disabled from mtk_hdmi_phy to mtk_hdmi driver

[Chun-Kuang Hu <chunkuang.hu@kernel.org>]

Chunfeng Yun <chunfeng.yun@mediatek.com> 於 2020年4月3日 週五 上午10:59寫道：
>
> On Thu, 2020-04-02 at 20:49 +0800, Chun-Kuang Hu wrote:
> > Hi, Matthias:
> >
> > Matthias Brugger <matthias.bgg@gmail.com> 於 2020年4月1日 週三 下午11:53寫道：
> > >
> > >
> > >
> > > On 01/04/2020 04:16, Chunfeng Yun wrote:
> > > > On Tue, 2020-03-31 at 23:57 +0800, Chun-Kuang Hu wrote:
> > > >> From: CK Hu <ck.hu@mediatek.com>
> > > >>
> > > >> tz_disabled is used to control mtk_hdmi output signal, but this variable
> > > >> is stored in mtk_hdmi_phy and mtk_hdmi_phy does not use it. So move
> > > >> tz_disabled to mtk_hdmi where it's used.
> > > >>
> > > >> Signed-off-by: CK Hu <ck.hu@mediatek.com>
> > > >> Signed-off-by: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> > > >> ---
> > > >>  drivers/gpu/drm/mediatek/mtk_hdmi.c           | 22 ++++++++++++++++---
> > > >>  drivers/gpu/drm/mediatek/mtk_hdmi_phy.h       |  1 -
> > > >>  .../gpu/drm/mediatek/mtk_mt2701_hdmi_phy.c    |  1 -
> > > >>  3 files changed, 19 insertions(+), 5 deletions(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi.c b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > > >> index 5e4a4dbda443..878433c09c9b 100644
> > > >> --- a/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > > >> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi.c
> > > >> @@ -144,11 +144,16 @@ struct hdmi_audio_param {
> > > >>      struct hdmi_codec_params codec_params;
> > > >>  };
> > > >>
> > > >> +struct mtk_hdmi_conf {
> > > >> +    bool tz_disabled;
> > > >> +};
> > > >> +
> > > >>  struct mtk_hdmi {
> > > >>      struct drm_bridge bridge;
> > > >>      struct drm_bridge *next_bridge;
> > > >>      struct drm_connector conn;
> > > >>      struct device *dev;
> > > >> +    const struct mtk_hdmi_conf *conf;
> > > >>      struct phy *phy;
> > > >>      struct device *cec_dev;
> > > >>      struct i2c_adapter *ddc_adpt;
> > > >> @@ -230,7 +235,6 @@ static void mtk_hdmi_hw_vid_black(struct mtk_hdmi *hdmi, bool black)
> > > >>  static void mtk_hdmi_hw_make_reg_writable(struct mtk_hdmi *hdmi, bool enable)
> > > >>  {
> > > >>      struct arm_smccc_res res;
> > > >> -    struct mtk_hdmi_phy *hdmi_phy = phy_get_drvdata(hdmi->phy);
> > > >>
> > > >>      /*
> > > >>       * MT8173 HDMI hardware has an output control bit to enable/disable HDMI
> > > >> @@ -238,7 +242,7 @@ static void mtk_hdmi_hw_make_reg_writable(struct mtk_hdmi *hdmi, bool enable)
> > > >>       * The ARM trusted firmware provides an API for the HDMI driver to set
> > > >>       * this control bit to enable HDMI output in supervisor mode.
> > > >>       */
> > > >> -    if (hdmi_phy->conf && hdmi_phy->conf->tz_disabled)
> > > >> +    if (hdmi->conf->tz_disabled)
> > >
> > > Wouldn't we need to check:
> > > if (hdmi->conf && hdmi->conf->tz_disabled)
> >
> > My design is: hdmi->conf would not be NULL.
> >
> > >
> > > >>              regmap_update_bits(hdmi->sys_regmap,
> > > >>                                 hdmi->sys_offset + HDMI_SYS_CFG20,
> > > >>                                 0x80008005, enable ? 0x80000005 : 0x8000);
> > > >> @@ -1688,6 +1692,7 @@ static int mtk_drm_hdmi_probe(struct platform_device *pdev)
> > > >>              return -ENOMEM;
> > > >>
> > > >>      hdmi->dev = dev;
> > > >> +    hdmi->conf = of_device_get_match_data(dev);
> > > >>
> > > >>      ret = mtk_hdmi_dt_parse_pdata(hdmi, pdev);
> > > >>      if (ret)
> > > >> @@ -1765,8 +1770,19 @@ static int mtk_hdmi_resume(struct device *dev)
> > > >>  static SIMPLE_DEV_PM_OPS(mtk_hdmi_pm_ops,
> > > >>                       mtk_hdmi_suspend, mtk_hdmi_resume);
> > > >>
> > > >> +static const struct mtk_hdmi_conf mtk_hdmi_conf_mt2701 = {
> > > >> +    .tz_disabled = true,
> > > >> +};
> > > >> +
> > > >> +static const struct mtk_hdmi_conf mtk_hdmi_conf_mt8173;
> > > >> +
> > > >>  static const struct of_device_id mtk_drm_hdmi_of_ids[] = {
> > > >> -    { .compatible = "mediatek,mt8173-hdmi", },
> > > >> +    { .compatible = "mediatek,mt2701-hdmi",
> > > >> +      .data = &mtk_hdmi_conf_mt2701,
> > > >> +    },
> > > >> +    { .compatible = "mediatek,mt8173-hdmi",
> > > >> +      .data = &mtk_hdmi_conf_mt8173,
> > >
> > > We don't have any data? Then we should set data to NULL instead.
> >
> > My design is data would not be NULL, so I need not to check whether it
> > is NULL in driver.
> But we don't need .data for mt8173, it's better to set it to NULL.

OK, in the view of reducing the code size, setting it to NULL would
make code size smaller.
I would modify this in next version.

Regards,
Chun-Kuang.

>
> >
> > Regards,
> > CK
> >
> > >
> > > Regards,
> > > Matthias
> > >
> > > >> +    },
> > > >>      {}
> > > >>  };
> > > >>
> > > >> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_phy.h b/drivers/gpu/drm/mediatek/mtk_hdmi_phy.h
> > > >> index 2d8b3182470d..fc1c2efd1128 100644
> > > >> --- a/drivers/gpu/drm/mediatek/mtk_hdmi_phy.h
> > > >> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_phy.h
> > > >> @@ -20,7 +20,6 @@
> > > >>  struct mtk_hdmi_phy;
> > > >>
> > > >>  struct mtk_hdmi_phy_conf {
> > > >> -    bool tz_disabled;
> > > >>      unsigned long flags;
> > > >>      const struct clk_ops *hdmi_phy_clk_ops;
> > > >>      void (*hdmi_phy_enable_tmds)(struct mtk_hdmi_phy *hdmi_phy);
> > > >> diff --git a/drivers/gpu/drm/mediatek/mtk_mt2701_hdmi_phy.c b/drivers/gpu/drm/mediatek/mtk_mt2701_hdmi_phy.c
> > > >> index d3cc4022e988..99fe05cd3598 100644
> > > >> --- a/drivers/gpu/drm/mediatek/mtk_mt2701_hdmi_phy.c
> > > >> +++ b/drivers/gpu/drm/mediatek/mtk_mt2701_hdmi_phy.c
> > > >> @@ -237,7 +237,6 @@ static void mtk_hdmi_phy_disable_tmds(struct mtk_hdmi_phy *hdmi_phy)
> > > >>  }
> > > >>
> > > >>  struct mtk_hdmi_phy_conf mtk_hdmi_phy_2701_conf = {
> > > >> -    .tz_disabled = true,
> > > >>      .flags = CLK_SET_RATE_GATE,
> > > >>      .hdmi_phy_clk_ops = &mtk_hdmi_phy_pll_ops,
> > > >>      .hdmi_phy_enable_tmds = mtk_hdmi_phy_enable_tmds,
> > > >
> > > > Reviewed-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > > >
>