[3/8] drm/loongson: Allow attach drm bridge driver by calling lsdc_output_init()

  Move the sharable subroutine into lsdc_output.c and refactor.

Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/loongson/Makefile      |   1 +
 drivers/gpu/drm/loongson/lsdc_drv.h    |  17 -
 drivers/gpu/drm/loongson/lsdc_output.c | 640 +++++++++++++++++++++++++
 drivers/gpu/drm/loongson/lsdc_output.h |  52 +-
 4 files changed, 691 insertions(+), 19 deletions(-)
 create mode 100644 drivers/gpu/drm/loongson/lsdc_output.c
  

On Sun, 29 Oct 2023 at 21:46, Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>
> Move the sharable subroutine into lsdc_output.c and refactor.
>
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/loongson/Makefile      |   1 +
>  drivers/gpu/drm/loongson/lsdc_drv.h    |  17 -
>  drivers/gpu/drm/loongson/lsdc_output.c | 640 +++++++++++++++++++++++++
>  drivers/gpu/drm/loongson/lsdc_output.h |  52 +-

This diffstat doesn't match patch description.

>  4 files changed, 691 insertions(+), 19 deletions(-)
>  create mode 100644 drivers/gpu/drm/loongson/lsdc_output.c
>
> diff --git a/drivers/gpu/drm/loongson/Makefile b/drivers/gpu/drm/loongson/Makefile
> index 1459d19b2c90..393709e686aa 100644
> --- a/drivers/gpu/drm/loongson/Makefile
> +++ b/drivers/gpu/drm/loongson/Makefile
> @@ -9,6 +9,7 @@ loongson-y := \
>         lsdc_gfxpll.o \
>         lsdc_i2c.o \
>         lsdc_irq.o \
> +       lsdc_output.o \
>         lsdc_output_7a1000.o \
>         lsdc_output_7a2000.o \
>         lsdc_plane.o \
> diff --git a/drivers/gpu/drm/loongson/lsdc_drv.h b/drivers/gpu/drm/loongson/lsdc_drv.h
> index 335953c988d1..46ba9b88a30d 100644
> --- a/drivers/gpu/drm/loongson/lsdc_drv.h
> +++ b/drivers/gpu/drm/loongson/lsdc_drv.h
> @@ -175,23 +175,6 @@ struct lsdc_cursor {
>         struct lsdc_device *ldev;
>  };
>
> -struct lsdc_output {
> -       struct drm_encoder encoder;
> -       struct drm_connector connector;
> -};
> -
> -static inline struct lsdc_output *
> -connector_to_lsdc_output(struct drm_connector *connector)
> -{
> -       return container_of(connector, struct lsdc_output, connector);
> -}
> -
> -static inline struct lsdc_output *
> -encoder_to_lsdc_output(struct drm_encoder *encoder)
> -{
> -       return container_of(encoder, struct lsdc_output, encoder);
> -}
> -
>  struct lsdc_display_pipe {
>         struct lsdc_crtc crtc;
>         struct lsdc_primary primary;
> diff --git a/drivers/gpu/drm/loongson/lsdc_output.c b/drivers/gpu/drm/loongson/lsdc_output.c
> new file mode 100644
> index 000000000000..8262c3f91ebe
> --- /dev/null
> +++ b/drivers/gpu/drm/loongson/lsdc_output.c
> @@ -0,0 +1,640 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2023 Loongson Technology Corporation Limited
> + */
> +
> +#include <linux/delay.h>
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_debugfs.h>
> +#include <drm/drm_edid.h>
> +#include <drm/drm_probe_helper.h>
> +
> +#include "lsdc_drv.h"
> +#include "lsdc_output.h"
> +
> +/* This file contain shared subroutines for the output part */
> +
> +/* Usable for generic DVO, VGA and buitl-in HDMI connector */
> +
> +static int lsdc_connector_get_modes(struct drm_connector *connector)
> +{
> +       unsigned int num = 0;
> +       struct edid *edid;
> +
> +       if (connector->ddc) {
> +               edid = drm_get_edid(connector, connector->ddc);
> +               if (edid) {
> +                       drm_connector_update_edid_property(connector, edid);
> +                       num = drm_add_edid_modes(connector, edid);
> +                       kfree(edid);
> +               }
> +
> +               return num;

drm_connector_helper_get_modes_from_ddc()

> +       }
> +
> +       num = drm_add_modes_noedid(connector, 1920, 1200);
> +
> +       drm_set_preferred_mode(connector, 1024, 768);
> +
> +       return num;
> +}
> +
> +static struct drm_encoder *
> +lsdc_connector_get_best_encoder(struct drm_connector *connector,
> +                               struct drm_atomic_state *state)
> +{
> +       struct lsdc_output *output = connector_to_lsdc_output(connector);
> +
> +       return &output->encoder;

Do you really need it? Does drm_connector_get_single_encoder() work
for your case?

> +}
> +
> +const struct drm_connector_helper_funcs lsdc_connector_helper_funcs = {
> +       .atomic_best_encoder = lsdc_connector_get_best_encoder,
> +       .get_modes = lsdc_connector_get_modes,
> +};
> +
> +static enum drm_connector_status
> +lsdc_connector_detect(struct drm_connector *connector, bool force)
> +{
> +       struct i2c_adapter *ddc = connector->ddc;
> +
> +       if (ddc) {
> +               if (drm_probe_ddc(ddc))
> +                       return connector_status_connected;
> +
> +               return connector_status_disconnected;
> +       }
> +
> +       return connector_status_unknown;
> +}
> +
> +const struct drm_connector_funcs lsdc_connector_funcs = {
> +       .detect = lsdc_connector_detect,
> +       .fill_modes = drm_helper_probe_single_connector_modes,
> +       .destroy = drm_connector_cleanup,
> +       .reset = drm_atomic_helper_connector_reset,
> +       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state
> +};
> +
> +/* debugfs */
> +
> +#define LSDC_HDMI_REG(i, reg) {                               \
> +       .name = __stringify_1(LSDC_HDMI##i##_##reg##_REG),    \
> +       .offset = LSDC_HDMI##i##_##reg##_REG,                 \
> +}
> +
> +static int lsdc_hdmi_regs_show(struct seq_file *m, void *data)
> +{
> +       struct drm_info_node *node = (struct drm_info_node *)m->private;
> +       struct drm_device *ddev = node->minor->dev;
> +       struct lsdc_device *ldev = to_lsdc(ddev);
> +       const struct lsdc_reg32 *preg;
> +
> +       preg = (const struct lsdc_reg32 *)node->info_ent->data;
> +
> +       while (preg->name) {
> +               u32 offset = preg->offset;
> +
> +               seq_printf(m, "%s (0x%04x): 0x%08x\n",
> +                          preg->name, offset, lsdc_rreg32(ldev, offset));
> +               ++preg;
> +       }
> +
> +       return 0;
> +}
> +
> +/* LSDC built-in HDMI encoder, connected with display pipe 0 */
> +
> +static const struct lsdc_reg32 lsdc_hdmi_regs_pipe0[] = {
> +       LSDC_HDMI_REG(0, ZONE),
> +       LSDC_HDMI_REG(0, INTF_CTRL),
> +       LSDC_HDMI_REG(0, PHY_CTRL),
> +       LSDC_HDMI_REG(0, PHY_PLL),
> +       LSDC_HDMI_REG(0, AVI_INFO_CRTL),
> +       LSDC_HDMI_REG(0, PHY_CAL),
> +       LSDC_HDMI_REG(0, AUDIO_PLL_LO),
> +       LSDC_HDMI_REG(0, AUDIO_PLL_HI),
> +       {NULL, 0},  /* MUST be {NULL, 0} terminated */
> +};
> +
> +static const struct drm_info_list lsdc_pipe0_hdmi_debugfs_files[] = {
> +       { "regs", lsdc_hdmi_regs_show, 0, (void *)lsdc_hdmi_regs_pipe0 },
> +};
> +
> +static enum drm_connector_status
> +lsdc_pipe0_hdmi_connector_detect(struct drm_connector *connector, bool force)
> +{
> +       struct lsdc_device *ldev = to_lsdc(connector->dev);
> +       u32 val;
> +
> +       val = lsdc_rreg32(ldev, LSDC_HDMI_HPD_STATUS_REG);
> +
> +       if (val & HDMI0_HPD_FLAG)
> +               return connector_status_connected;
> +
> +       return connector_status_disconnected;
> +}
> +
> +static void lsdc_pipe0_hdmi_late_register(struct drm_connector *connector,
> +                                         struct dentry *root)
> +{
> +       struct drm_device *ddev = connector->dev;
> +       struct drm_minor *minor = ddev->primary;
> +
> +       drm_debugfs_create_files(lsdc_pipe0_hdmi_debugfs_files,
> +                                ARRAY_SIZE(lsdc_pipe0_hdmi_debugfs_files),
> +                                root, minor);
> +}
> +
> +const struct drm_connector_funcs lsdc_pipe0_hdmi_connector_funcs = {
> +       .detect = lsdc_pipe0_hdmi_connector_detect,
> +       .fill_modes = drm_helper_probe_single_connector_modes,
> +       .destroy = drm_connector_cleanup,
> +       .reset = drm_atomic_helper_connector_reset,
> +       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +       .debugfs_init = lsdc_pipe0_hdmi_late_register,
> +};
> +
> +/* LSDC built-in HDMI connector, connected with display pipe 1 */
> +
> +static enum drm_connector_status
> +lsdc_pipe1_hdmi_connector_detect(struct drm_connector *connector, bool force)
> +{
> +       struct lsdc_device *ldev = to_lsdc(connector->dev);
> +       u32 val;
> +
> +       val = lsdc_rreg32(ldev, LSDC_HDMI_HPD_STATUS_REG);
> +
> +       if (val & HDMI1_HPD_FLAG)
> +               return connector_status_connected;
> +
> +       return connector_status_disconnected;
> +}
> +
> +static const struct lsdc_reg32 lsdc_pipe1_hdmi_encoder_regs[] = {
> +       LSDC_HDMI_REG(1, ZONE),
> +       LSDC_HDMI_REG(1, INTF_CTRL),
> +       LSDC_HDMI_REG(1, PHY_CTRL),
> +       LSDC_HDMI_REG(1, PHY_PLL),
> +       LSDC_HDMI_REG(1, AVI_INFO_CRTL),
> +       LSDC_HDMI_REG(1, PHY_CAL),
> +       LSDC_HDMI_REG(1, AUDIO_PLL_LO),
> +       LSDC_HDMI_REG(1, AUDIO_PLL_HI),
> +       {NULL, 0},  /* MUST be {NULL, 0} terminated */
> +};
> +
> +static const struct drm_info_list lsdc_pipe1_hdmi_debugfs_files[] = {
> +       { "regs", lsdc_hdmi_regs_show, 0, (void *)lsdc_pipe1_hdmi_encoder_regs },
> +};
> +
> +static void lsdc_pipe1_hdmi_late_register(struct drm_connector *connector,
> +                                         struct dentry *root)
> +{
> +       struct drm_device *ddev = connector->dev;
> +       struct drm_minor *minor = ddev->primary;
> +
> +       drm_debugfs_create_files(lsdc_pipe1_hdmi_debugfs_files,
> +                                ARRAY_SIZE(lsdc_pipe1_hdmi_debugfs_files),
> +                                root, minor);
> +}
> +
> +const struct drm_connector_funcs lsdc_pipe1_hdmi_connector_funcs = {
> +       .detect = lsdc_pipe1_hdmi_connector_detect,
> +       .fill_modes = drm_helper_probe_single_connector_modes,
> +       .destroy = drm_connector_cleanup,
> +       .reset = drm_atomic_helper_connector_reset,
> +       .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +       .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +       .debugfs_init = lsdc_pipe1_hdmi_late_register,
> +};
> +
> +/*
> + *  Fout = M * in_khz
> + *
> + *  M = (4 * LF) / (IDF * ODF)
> + *
> + *  IDF: Input Division Factor
> + *  ODF: Output Division Factor
> + *   LF: Loop Factor
> + *    M: Required Mult
> + *
> + *  +--------------------------------------------------------+
> + *  |       in_khz      | M  | IDF | LF | ODF |   Fout(Mhz)  |
> + *  |-------------------+----+-----+----+-----+--------------|
> + *  |  170000 ~ 340000  | 10 | 16  | 40 |  1  | 1700 ~ 3400  |
> + *  |   85000 ~ 170000  | 10 |  8  | 40 |  2  |  850 ~ 1700  |
> + *  |   42500 ~  85000  | 10 |  4  | 40 |  4  |  425 ~ 850   |
> + *  |   21250 ~  42500  | 10 |  2  | 40 |  8  | 212.5 ~ 425  |
> + *  |   20000 ~  21250  | 10 |  1  | 40 | 16  |  200 ~ 212.5 |
> + *  +--------------------------------------------------------+
> + */
> +static void lsdc_hdmi_phy_pll_config(struct drm_device *ddev,
> +                                    int in_khz,
> +                                    unsigned int pipe)
> +{
> +       struct lsdc_device *ldev = to_lsdc(ddev);
> +       int count = 0;
> +       u32 val;
> +
> +       /* Firstly, disable phy pll */
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_PHY_PLL_REG, pipe, 0x0);
> +
> +       /*
> +        * Most of time, loongson HDMI require M = 10
> +        * for example, 10 = (4 * 40) / (8 * 2)
> +        * here, write "1" to the ODF will get "2"
> +        */
> +
> +       if (in_khz >= 170000)
> +               val = (16 << HDMI_PLL_IDF_SHIFT) |
> +                     (40 << HDMI_PLL_LF_SHIFT) |
> +                     (0 << HDMI_PLL_ODF_SHIFT);
> +       else if (in_khz >= 85000)
> +               val = (8 << HDMI_PLL_IDF_SHIFT) |
> +                     (40 << HDMI_PLL_LF_SHIFT) |
> +                     (1 << HDMI_PLL_ODF_SHIFT);
> +       else if (in_khz >= 42500)
> +               val = (4 << HDMI_PLL_IDF_SHIFT) |
> +                     (40 << HDMI_PLL_LF_SHIFT) |
> +                     (2 << HDMI_PLL_ODF_SHIFT);
> +       else if (in_khz >= 21250)
> +               val = (2 << HDMI_PLL_IDF_SHIFT) |
> +                     (40 << HDMI_PLL_LF_SHIFT) |
> +                     (3 << HDMI_PLL_ODF_SHIFT);
> +       else
> +               val = (1 << HDMI_PLL_IDF_SHIFT) |
> +                     (40 << HDMI_PLL_LF_SHIFT) |
> +                     (4 << HDMI_PLL_ODF_SHIFT);
> +
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_PHY_PLL_REG, pipe, val);
> +
> +       val |= HDMI_PLL_ENABLE;
> +
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_PHY_PLL_REG, pipe, val);
> +
> +       udelay(2);
> +
> +       drm_dbg(ddev, "Input frequency of HDMI-%u: %d kHz\n", pipe, in_khz);
> +
> +       /* Wait hdmi phy pll lock */
> +       do {
> +               val = lsdc_pipe_rreg32(ldev, LSDC_HDMI0_PHY_PLL_REG, pipe);
> +
> +               if (val & HDMI_PLL_LOCKED) {
> +                       drm_dbg(ddev, "Setting HDMI-%u PLL take %d cycles\n",
> +                               pipe, count);
> +                       break;
> +               }
> +               ++count;
> +       } while (count < 1000);
> +
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_PHY_CAL_REG, pipe, 0x0f000ff0);
> +
> +       if (count >= 1000)
> +               drm_err(ddev, "Setting HDMI-%u PLL failed\n", pipe);
> +}
> +
> +static int lsdc_hdmi_phy_set_avi_infoframe(struct drm_encoder *encoder,
> +                                          struct drm_connector *connector,
> +                                          struct drm_display_mode *mode,
> +                                          unsigned int index)
> +{
> +       struct drm_device *ddev = encoder->dev;
> +       struct lsdc_device *ldev = to_lsdc(ddev);
> +       struct hdmi_avi_infoframe infoframe;
> +       u8 buffer[HDMI_INFOFRAME_SIZE(AVI)];
> +       unsigned char *ptr = &buffer[HDMI_INFOFRAME_HEADER_SIZE];
> +       unsigned int content0, content1, content2, content3;
> +       int err;
> +
> +       err = drm_hdmi_avi_infoframe_from_display_mode(&infoframe,
> +                                                      connector,
> +                                                      mode);
> +       if (err < 0) {
> +               drm_err(ddev, "failed to setup AVI infoframe: %d\n", err);
> +               return err;
> +       }
> +
> +       /* Fixed infoframe configuration not linked to the mode */
> +       infoframe.colorspace = HDMI_COLORSPACE_RGB;
> +       infoframe.quantization_range = HDMI_QUANTIZATION_RANGE_DEFAULT;
> +       infoframe.colorimetry = HDMI_COLORIMETRY_NONE;
> +
> +       err = hdmi_avi_infoframe_pack(&infoframe, buffer, sizeof(buffer));
> +       if (err < 0) {
> +               drm_err(ddev, "failed to pack AVI infoframe: %d\n", err);
> +                       return err;
> +       }
> +
> +       content0 = *(unsigned int *)ptr;
> +       content1 = *(ptr + 4);
> +       content2 = *(unsigned int *)(ptr + 5);
> +       content3 = *(unsigned int *)(ptr + 9);
> +
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_AVI_CONTENT0, index, content0);
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_AVI_CONTENT1, index, content1);
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_AVI_CONTENT2, index, content2);
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_AVI_CONTENT3, index, content3);
> +
> +       lsdc_pipe_wreg32(ldev, LSDC_HDMI0_AVI_INFO_CRTL_REG, index,
> +                        AVI_PKT_ENABLE | AVI_PKT_UPDATE);
> +
> +       drm_dbg(ddev, "Update HDMI-%u avi infoframe\n", index);
> +
> +       return 0;
> +}
> +
> +/* Built-in HDMI encoder funcs on display pipe 0 */
> +
> +static void lsdc_pipe0_hdmi_encoder_reset(struct drm_encoder *encoder)
> +{
> +       struct drm_device *ddev = encoder->dev;
> +       struct lsdc_device *ldev = to_lsdc(ddev);
> +       u32 val;
> +
> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
> +       lsdc_wreg32(ldev, LSDC_CRTC0_DVO_CONF_REG, val);
> +
> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
> +       lsdc_ureg32_clr(ldev, LSDC_HDMI0_INTF_CTRL_REG, HW_I2C_EN);
> +
> +       /* Help the HDMI phy get out of reset state */
> +       lsdc_wreg32(ldev, LSDC_HDMI0_PHY_CTRL_REG, HDMI_PHY_RESET_N);
> +
> +       drm_dbg(ddev, "%s reset\n", encoder->name);
> +
> +       mdelay(20);
> +}
> +
> +const struct drm_encoder_funcs lsdc_pipe0_hdmi_encoder_funcs = {
> +       .reset = lsdc_pipe0_hdmi_encoder_reset,
> +       .destroy = drm_encoder_cleanup,
> +};
> +
> +/* Built-in HDMI encoder funcs on display pipe 1 */

All pipe 1 code looks like a pipe0, just the registers were changed.
Could you please refactor that to use a single instance of all
functions and pass pipe id through the data struct?
Then you can use macros to determine whether to use pipe 0 or pipe 1 register.

> +
> +static void lsdc_pipe1_hdmi_encoder_reset(struct drm_encoder *encoder)
> +{
> +       struct drm_device *ddev = encoder->dev;
> +       struct lsdc_device *ldev = to_lsdc(ddev);
> +       u32 val;
> +
> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
> +       lsdc_wreg32(ldev, LSDC_CRTC1_DVO_CONF_REG, val);
> +
> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
> +       lsdc_ureg32_clr(ldev, LSDC_HDMI1_INTF_CTRL_REG, HW_I2C_EN);
> +
> +       /* Help the HDMI phy get out of reset state */
> +       lsdc_wreg32(ldev, LSDC_HDMI1_PHY_CTRL_REG, HDMI_PHY_RESET_N);
> +
> +       drm_dbg(ddev, "%s reset\n", encoder->name);
> +
> +       mdelay(20);
> +}
> +
> +const struct drm_encoder_funcs lsdc_pipe1_hdmi_encoder_funcs = {
> +       .reset = lsdc_pipe1_hdmi_encoder_reset,
> +       .destroy = drm_encoder_cleanup,
> +};
> +
> +/* Built-in DVO encoder helper funcs */
> +
> +static void lsdc_dvo_atomic_disable(struct drm_encoder *encoder,
> +                                   struct drm_atomic_state *state)
> +{
> +}
> +
> +static void lsdc_dvo_atomic_enable(struct drm_encoder *encoder,
> +                                  struct drm_atomic_state *state)
> +{
> +}
> +
> +static void lsdc_dvo_atomic_modeset(struct drm_encoder *encoder,
> +                                   struct drm_crtc_state *crtc_state,
> +                                   struct drm_connector_state *conn_state)
> +{
> +}

You do not need these empty callbacks, do you?

> +
> +const struct drm_encoder_helper_funcs lsdc_encoder_helper_funcs = {
> +       .atomic_disable = lsdc_dvo_atomic_disable,
> +       .atomic_enable = lsdc_dvo_atomic_enable,
> +       .atomic_mode_set = lsdc_dvo_atomic_modeset,
> +};
> +
> +/* Built-in HDMI encoder helper funcs on display pipe 0 */
> +
> +static void lsdc_pipe0_hdmi_atomic_disable(struct drm_encoder *encoder,
> +                                          struct drm_atomic_state *state)
> +{
> +       struct lsdc_device *ldev = to_lsdc(encoder->dev);
> +
> +       /* Disable the HDMI PHY */
> +       lsdc_ureg32_clr(ldev, LSDC_HDMI0_PHY_CTRL_REG, HDMI_PHY_EN);
> +
> +       /* Disable the HDMI interface */
> +       lsdc_ureg32_clr(ldev, LSDC_HDMI0_INTF_CTRL_REG, HDMI_INTERFACE_EN);
> +}
> +
> +static void lsdc_pipe0_hdmi_atomic_enable(struct drm_encoder *encoder,
> +                                         struct drm_atomic_state *state)
> +{
> +       struct lsdc_device *ldev = to_lsdc(encoder->dev);
> +       u32 val;
> +
> +       /* datasheet say it should larger than 48 */
> +       val = 64 << HDMI_H_ZONE_IDLE_SHIFT | 64 << HDMI_V_ZONE_IDLE_SHIFT;
> +       lsdc_wreg32(ldev, LSDC_HDMI0_ZONE_REG, val);
> +
> +       val = HDMI_PHY_TERM_STATUS |
> +             HDMI_PHY_TERM_DET_EN |
> +             HDMI_PHY_TERM_H_EN |
> +             HDMI_PHY_TERM_L_EN |
> +             HDMI_PHY_RESET_N |
> +             HDMI_PHY_EN;
> +       lsdc_wreg32(ldev, LSDC_HDMI0_PHY_CTRL_REG, val);
> +
> +       udelay(2);
> +
> +       val = HDMI_CTL_PERIOD_MODE |
> +             HDMI_AUDIO_EN |
> +             HDMI_PACKET_EN |
> +             HDMI_INTERFACE_EN |
> +             (8 << HDMI_VIDEO_PREAMBLE_SHIFT);
> +       lsdc_wreg32(ldev, LSDC_HDMI0_INTF_CTRL_REG, val);
> +}
> +
> +static void lsdc_pipe0_hdmi_atomic_modeset(struct drm_encoder *encoder,
> +                                          struct drm_crtc_state *crtc_state,
> +                                          struct drm_connector_state *conn_state)
> +{
> +       struct lsdc_output *output = encoder_to_lsdc_output(encoder);
> +       struct drm_device *ddev = encoder->dev;
> +       struct drm_display_mode *mode = &crtc_state->mode;
> +
> +       lsdc_hdmi_phy_pll_config(ddev, mode->clock, 0);
> +
> +       lsdc_hdmi_phy_set_avi_infoframe(encoder, &output->connector, mode, 0);
> +
> +       drm_dbg(ddev, "%s modeset finished\n", encoder->name);
> +}
> +
> +const struct drm_encoder_helper_funcs lsdc_pipe0_hdmi_encoder_helper_funcs = {
> +       .atomic_disable = lsdc_pipe0_hdmi_atomic_disable,
> +       .atomic_enable = lsdc_pipe0_hdmi_atomic_enable,
> +       .atomic_mode_set = lsdc_pipe0_hdmi_atomic_modeset,
> +};
> +
> +/* Built-in HDMI encoder helper funcs on display pipe 1 */
> +
> +static void lsdc_pipe1_hdmi_atomic_disable(struct drm_encoder *encoder,
> +                                          struct drm_atomic_state *state)
> +{
> +       struct lsdc_device *ldev = to_lsdc(encoder->dev);
> +
> +       /* Disable the HDMI PHY */
> +       lsdc_ureg32_clr(ldev, LSDC_HDMI1_PHY_CTRL_REG, HDMI_PHY_EN);
> +
> +       /* Disable the HDMI interface */
> +       lsdc_ureg32_clr(ldev, LSDC_HDMI1_INTF_CTRL_REG, HDMI_INTERFACE_EN);
> +}
> +
> +static void lsdc_pipe1_hdmi_atomic_enable(struct drm_encoder *encoder,
> +                                         struct drm_atomic_state *state)
> +{
> +       struct lsdc_device *ldev = to_lsdc(encoder->dev);
> +       u32 val;
> +
> +       /* datasheet say it should larger than 48 */
> +       val = 64 << HDMI_H_ZONE_IDLE_SHIFT | 64 << HDMI_V_ZONE_IDLE_SHIFT;
> +       lsdc_wreg32(ldev, LSDC_HDMI1_ZONE_REG, val);
> +
> +       val = HDMI_PHY_TERM_STATUS |
> +             HDMI_PHY_TERM_DET_EN |
> +             HDMI_PHY_TERM_H_EN |
> +             HDMI_PHY_TERM_L_EN |
> +             HDMI_PHY_RESET_N |
> +             HDMI_PHY_EN;
> +       lsdc_wreg32(ldev, LSDC_HDMI1_PHY_CTRL_REG, val);
> +
> +       udelay(2);
> +
> +       val = HDMI_CTL_PERIOD_MODE |
> +             HDMI_AUDIO_EN |
> +             HDMI_PACKET_EN |
> +             HDMI_INTERFACE_EN |
> +             (8 << HDMI_VIDEO_PREAMBLE_SHIFT);
> +       lsdc_wreg32(ldev, LSDC_HDMI1_INTF_CTRL_REG, val);
> +}
> +
> +static void lsdc_pipe1_hdmi_atomic_modeset(struct drm_encoder *encoder,
> +                                          struct drm_crtc_state *crtc_state,
> +                                          struct drm_connector_state *conn_state)
> +{
> +       struct lsdc_output *output = encoder_to_lsdc_output(encoder);
> +       struct drm_device *ddev = encoder->dev;
> +       struct drm_display_mode *mode = &crtc_state->mode;
> +
> +       lsdc_hdmi_phy_pll_config(ddev, mode->clock, 1);
> +
> +       lsdc_hdmi_phy_set_avi_infoframe(encoder, &output->connector, mode, 1);
> +
> +       drm_dbg(ddev, "%s modeset finished\n", encoder->name);
> +}
> +
> +const struct drm_encoder_helper_funcs lsdc_pipe1_hdmi_encoder_helper_funcs = {
> +       .atomic_disable = lsdc_pipe1_hdmi_atomic_disable,
> +       .atomic_enable = lsdc_pipe1_hdmi_atomic_enable,
> +       .atomic_mode_set = lsdc_pipe1_hdmi_atomic_modeset,
> +};
> +
> +int lsdc_encoder_init(struct drm_device *ddev,
> +                     struct lsdc_output *output,
> +                     unsigned int pipe)
> +{
> +       const struct lsdc_output_desc *descp = output->descp;
> +       struct drm_encoder *encoder = &output->encoder;
> +       int ret;
> +
> +       ret = drm_encoder_init(ddev,
> +                              encoder,
> +                              descp->encoder_funcs,
> +                              descp->encoder_type,
> +                              descp->name);
> +       if (ret)
> +               return ret;
> +
> +       encoder->possible_crtcs = BIT(pipe);
> +
> +       drm_encoder_helper_add(encoder, descp->encoder_helper_funcs);
> +
> +       return 0;
> +}
> +
> +int lsdc_connector_init(struct drm_device *ddev,
> +                       struct lsdc_output *output,
> +                       struct i2c_adapter *ddc,
> +                       unsigned int pipe)
> +{
> +       const struct lsdc_output_desc *descp = output->descp;
> +       struct drm_connector *connector = &output->connector;
> +       int ret;
> +
> +       ret = drm_connector_init_with_ddc(ddev,
> +                                         connector,
> +                                         descp->connector_funcs,
> +                                         descp->connector_type,
> +                                         ddc);
> +       if (ret)
> +               return ret;
> +
> +       drm_connector_helper_add(connector, descp->connector_helper_funcs);
> +
> +       drm_connector_attach_encoder(connector, &output->encoder);
> +
> +       connector->polled = DRM_CONNECTOR_POLL_CONNECT |
> +                           DRM_CONNECTOR_POLL_DISCONNECT;
> +
> +       connector->interlace_allowed = 0;
> +       connector->doublescan_allowed = 0;
> +
> +       drm_info(ddev, "DisplayPipe-%u has %s\n", pipe, descp->name);
> +
> +       return 0;
> +}
> +
> +/*
> + * A common, sharable subroutine for the initialization of output part.
> + * If there is external non-transparent display bridge chip on the display
> + * pipe, we will attach it. Otherwise, the output is simple, we will just
> + * initial a connector for it.
> + */
> +int lsdc_output_init(struct drm_device *ddev,
> +                    struct lsdc_display_pipe *dispipe,
> +                    struct i2c_adapter *ddc,
> +                    unsigned int pipe)
> +{
> +       struct lsdc_output *output = &dispipe->output;
> +       int ret;
> +
> +       ret = lsdc_encoder_init(ddev, output, pipe);
> +       if (ret)
> +               return ret;
> +
> +       if (output->bridge) {
> +               ret = drm_bridge_attach(&output->encoder, output->bridge,
> +                                       NULL, 0);

Since you are creating the connector manually, you should pass
DRM_BRIDGE_ATTACH_NO_CONNECTOR here.

> +               if (ret) {
> +                       drm_err(ddev, "Attach display bridge failed\n");
> +                       ret = lsdc_connector_init(ddev, output, ddc, pipe);
> +               }
> +       } else {
> +               ret = lsdc_connector_init(ddev, output, ddc, pipe);
> +       }
> +
> +       return ret;
> +}
> diff --git a/drivers/gpu/drm/loongson/lsdc_output.h b/drivers/gpu/drm/loongson/lsdc_output.h
> index 097789051a1d..a37a72687bdf 100644
> --- a/drivers/gpu/drm/loongson/lsdc_output.h
> +++ b/drivers/gpu/drm/loongson/lsdc_output.h
> @@ -6,16 +6,64 @@
>  #ifndef __LSDC_OUTPUT_H__
>  #define __LSDC_OUTPUT_H__
>
> -#include "lsdc_drv.h"
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_connector.h>
> +#include <drm/drm_encoder.h>
> +
> +struct lsdc_output_desc {
> +       u32 pipe;
> +       u32 encoder_type;
> +       u32 connector_type;
> +       const struct drm_encoder_funcs *encoder_funcs;
> +       const struct drm_encoder_helper_funcs *encoder_helper_funcs;
> +       const struct drm_connector_funcs *connector_funcs;
> +       const struct drm_connector_helper_funcs *connector_helper_funcs;
> +       const char name[32];
> +};
> +
> +struct lsdc_output {
> +       struct drm_encoder encoder;
> +       struct drm_connector connector;
> +       struct drm_bridge *bridge;
> +       const struct lsdc_output_desc *descp;
> +};
> +
> +static inline struct lsdc_output *
> +connector_to_lsdc_output(struct drm_connector *connector)
> +{
> +       return container_of(connector, struct lsdc_output, connector);
> +}
> +
> +static inline struct lsdc_output *
> +encoder_to_lsdc_output(struct drm_encoder *encoder)
> +{
> +       return container_of(encoder, struct lsdc_output, encoder);
> +}
> +
> +extern const struct drm_connector_funcs lsdc_connector_funcs;
> +extern const struct drm_connector_funcs lsdc_pipe0_hdmi_connector_funcs;
> +extern const struct drm_connector_funcs lsdc_pipe1_hdmi_connector_funcs;
> +extern const struct drm_connector_helper_funcs lsdc_connector_helper_funcs;
> +
> +extern const struct drm_encoder_funcs lsdc_pipe0_hdmi_encoder_funcs;
> +extern const struct drm_encoder_funcs lsdc_pipe1_hdmi_encoder_funcs;
> +extern const struct drm_encoder_helper_funcs lsdc_encoder_helper_funcs;
> +extern const struct drm_encoder_helper_funcs lsdc_pipe0_hdmi_encoder_helper_funcs;
> +extern const struct drm_encoder_helper_funcs lsdc_pipe1_hdmi_encoder_helper_funcs;
>
>  int ls7a1000_output_init(struct drm_device *ddev,
>                          struct lsdc_display_pipe *dispipe,
>                          struct i2c_adapter *ddc,
>                          unsigned int index);
>
> -int ls7a2000_output_init(struct drm_device *ldev,
> +int ls7a2000_output_init(struct drm_device *ddev,
>                          struct lsdc_display_pipe *dispipe,
>                          struct i2c_adapter *ddc,
>                          unsigned int index);
>
> +int lsdc_output_init(struct drm_device *ddev,
> +                    struct lsdc_display_pipe *dispipe,
> +                    struct i2c_adapter *ddc,
> +                    unsigned int pipe);
> +
>  #endif
> --
> 2.34.1
>
  

Hi Sui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.6-rc7 next-20231027]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/drm-loongson-Introduce-a-minimal-support-for-Loongson-VBIOS/20231030-034730
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231029194607.379459-4-suijingfeng%40loongson.cn
patch subject: [PATCH 3/8] drm/loongson: Allow attach drm bridge driver by calling lsdc_output_init()
config: loongarch-randconfig-002-20231030 (https://download.01.org/0day-ci/archive/20231030/202310300738.zcudNQfj-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231030/202310300738.zcudNQfj-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202310300738.zcudNQfj-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/loongson/lsdc_output.c:555:5: warning: no previous prototype for 'lsdc_encoder_init' [-Wmissing-prototypes]
     555 | int lsdc_encoder_init(struct drm_device *ddev,
         |     ^~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/loongson/lsdc_output.c:578:5: warning: no previous prototype for 'lsdc_connector_init' [-Wmissing-prototypes]
     578 | int lsdc_connector_init(struct drm_device *ddev,
         |     ^~~~~~~~~~~~~~~~~~~


vim +/lsdc_encoder_init +555 drivers/gpu/drm/loongson/lsdc_output.c

   554	
 > 555	int lsdc_encoder_init(struct drm_device *ddev,
   556			      struct lsdc_output *output,
   557			      unsigned int pipe)
   558	{
   559		const struct lsdc_output_desc *descp = output->descp;
   560		struct drm_encoder *encoder = &output->encoder;
   561		int ret;
   562	
   563		ret = drm_encoder_init(ddev,
   564				       encoder,
   565				       descp->encoder_funcs,
   566				       descp->encoder_type,
   567				       descp->name);
   568		if (ret)
   569			return ret;
   570	
   571		encoder->possible_crtcs = BIT(pipe);
   572	
   573		drm_encoder_helper_add(encoder, descp->encoder_helper_funcs);
   574	
   575		return 0;
   576	}
   577	
 > 578	int lsdc_connector_init(struct drm_device *ddev,
   579				struct lsdc_output *output,
   580				struct i2c_adapter *ddc,
   581				unsigned int pipe)
   582	{
   583		const struct lsdc_output_desc *descp = output->descp;
   584		struct drm_connector *connector = &output->connector;
   585		int ret;
   586	
   587		ret = drm_connector_init_with_ddc(ddev,
   588						  connector,
   589						  descp->connector_funcs,
   590						  descp->connector_type,
   591						  ddc);
   592		if (ret)
   593			return ret;
   594	
   595		drm_connector_helper_add(connector, descp->connector_helper_funcs);
   596	
   597		drm_connector_attach_encoder(connector, &output->encoder);
   598	
   599		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
   600				    DRM_CONNECTOR_POLL_DISCONNECT;
   601	
   602		connector->interlace_allowed = 0;
   603		connector->doublescan_allowed = 0;
   604	
   605		drm_info(ddev, "DisplayPipe-%u has %s\n", pipe, descp->name);
   606	
   607		return 0;
   608	}
   609
  

Hi,


On 2023/10/30 07:10, Dmitry Baryshkov wrote:
>> +
>> +/* Built-in HDMI encoder funcs on display pipe 0 */
>> +
>> +static void lsdc_pipe0_hdmi_encoder_reset(struct drm_encoder *encoder)
>> +{
>> +       struct drm_device *ddev = encoder->dev;
>> +       struct lsdc_device *ldev = to_lsdc(ddev);
>> +       u32 val;
>> +
>> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
>> +       lsdc_wreg32(ldev, LSDC_CRTC0_DVO_CONF_REG, val);
>> +
>> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
>> +       lsdc_ureg32_clr(ldev, LSDC_HDMI0_INTF_CTRL_REG, HW_I2C_EN);
>> +
>> +       /* Help the HDMI phy get out of reset state */
>> +       lsdc_wreg32(ldev, LSDC_HDMI0_PHY_CTRL_REG, HDMI_PHY_RESET_N);
>> +
>> +       drm_dbg(ddev, "%s reset\n", encoder->name);
>> +
>> +       mdelay(20);
>> +}
>> +
>> +const struct drm_encoder_funcs lsdc_pipe0_hdmi_encoder_funcs = {
>> +       .reset = lsdc_pipe0_hdmi_encoder_reset,
>> +       .destroy = drm_encoder_cleanup,
>> +};
>> +
>> +/* Built-in HDMI encoder funcs on display pipe 1 */
> All pipe 1 code looks like a pipe0, just the registers were changed.
> Could you please refactor that to use a single instance of all
> functions and pass pipe id through the data struct?
> Then you can use macros to determine whether to use pipe 0 or pipe 1 register.
>

Yes, you are right. But please allow me to explain something.

In the past, Thomas told me to untangle it, despite this idea lead to duplicated code(or pattern).
but at the long run, this pay off.

Because the method of passing pipe id will introduce the "if and else" side effects.
But my functions have no if and else.


```
if (pipe == 0) {
     ...
} else if (pipe == 1) {
     ...
}
```

Using the C program language's Macro(#define XXX) to generate code is not fun to me.
Because every time you want to change it, It needs my brains to thinking it twice. Maybe
more than twice.

1) It needs my brains to replace the macros manually each time I saw the code.

2) When I want to change(alter) the prototype, I need to worry about all of the instances.
    sometimes it is not symmetry. The DVO port and HDMI phy itself is symmetry, but the
    external display bridge connected with them are not symmetry. So, there are some registers
    located at the domain of this display controller side should change according to the
    different type of external display bridge.

3) Code duplication is actually less harmful than unmaintainable.
    macros are abstract, as noob level programmer, we completely drop the idea of abstract.
    Bad abstract means design failure, this is what we are most afraid of.
    Generally, we would like divide the whole into small cases, handle them one by one.
    It is actually to review and understand.

4) From the viewpoint of the hardware, the display output hardware suffer from changes.
    Because users always want *new* display interface. The need of the users are also varies.
    Personally, I think macros are best for the symmetry case, while the output part of a
    display pipe always suffer from change.

>> +
>> +static void lsdc_pipe1_hdmi_encoder_reset(struct drm_encoder *encoder)
>> +{
>> +       struct drm_device *ddev = encoder->dev;
>> +       struct lsdc_device *ldev = to_lsdc(ddev);
>> +       u32 val;
>> +
>> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
>> +       lsdc_wreg32(ldev, LSDC_CRTC1_DVO_CONF_REG, val);
>> +
>> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
>> +       lsdc_ureg32_clr(ldev, LSDC_HDMI1_INTF_CTRL_REG, HW_I2C_EN);
>> +
>> +       /* Help the HDMI phy get out of reset state */
>> +       lsdc_wreg32(ldev, LSDC_HDMI1_PHY_CTRL_REG, HDMI_PHY_RESET_N);
>> +
>> +       drm_dbg(ddev, "%s reset\n", encoder->name);
>> +
>> +       mdelay(20);
>> +}
>> +
>> +const struct drm_encoder_funcs lsdc_pipe1_hdmi_encoder_funcs = {
>> +       .reset = lsdc_pipe1_hdmi_encoder_reset,
>> +       .destroy = drm_encoder_cleanup,
>> +};
  

Hi,

On 2023/10/30 12:13, Sui Jingfeng wrote:
> handle them one by one.
>    It is actually to review and understand. 


Handle the cases one by one.

It is actually more easier to review the untangled code.
It is also more easier to edit and maintain.
  

Hi,


On 2023/10/30 07:10, Dmitry Baryshkov wrote:
>> +
>> +static void lsdc_pipe1_hdmi_encoder_reset(struct drm_encoder *encoder)
>> +{
>> +       struct drm_device *ddev = encoder->dev;
>> +       struct lsdc_device *ldev = to_lsdc(ddev);
>> +       u32 val;
>> +
>> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
>> +       lsdc_wreg32(ldev, LSDC_CRTC1_DVO_CONF_REG, val);
>> +
>> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
>> +       lsdc_ureg32_clr(ldev, LSDC_HDMI1_INTF_CTRL_REG, HW_I2C_EN);
>> +
>> +       /* Help the HDMI phy get out of reset state */
>> +       lsdc_wreg32(ldev, LSDC_HDMI1_PHY_CTRL_REG, HDMI_PHY_RESET_N);
>> +
>> +       drm_dbg(ddev, "%s reset\n", encoder->name);
>> +
>> +       mdelay(20);
>> +}
>> +
>> +const struct drm_encoder_funcs lsdc_pipe1_hdmi_encoder_funcs = {
>> +       .reset = lsdc_pipe1_hdmi_encoder_reset,
>> +       .destroy = drm_encoder_cleanup,
>> +};
>> +
>> +/* Built-in DVO encoder helper funcs */
>> +
>> +static void lsdc_dvo_atomic_disable(struct drm_encoder *encoder,
>> +                                   struct drm_atomic_state *state)
>> +{
>> +}
>> +
>> +static void lsdc_dvo_atomic_enable(struct drm_encoder *encoder,
>> +                                  struct drm_atomic_state *state)
>> +{
>> +}
>> +
>> +static void lsdc_dvo_atomic_modeset(struct drm_encoder *encoder,
>> +                                   struct drm_crtc_state *crtc_state,
>> +                                   struct drm_connector_state *conn_state)
>> +{
>> +}
> You do not need these empty callbacks, do you?


Thanks a lot for the sharpen eyes.

Yes. I do not need these empty callbacks.
I have some program experience with Linux-4.19 kernel.
I clearly remember that these empty callbacks was need when I add lt8619 display bridge driver.
I have just remove it and tested, it seems that those is not need anymore.

Thanks a lot for review, this problem will be fixed at the next version.
  

On Mon, 30 Oct 2023 at 06:13, Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>
> Hi,
>
>
> On 2023/10/30 07:10, Dmitry Baryshkov wrote:
> >> +
> >> +/* Built-in HDMI encoder funcs on display pipe 0 */
> >> +
> >> +static void lsdc_pipe0_hdmi_encoder_reset(struct drm_encoder *encoder)
> >> +{
> >> +       struct drm_device *ddev = encoder->dev;
> >> +       struct lsdc_device *ldev = to_lsdc(ddev);
> >> +       u32 val;
> >> +
> >> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
> >> +       lsdc_wreg32(ldev, LSDC_CRTC0_DVO_CONF_REG, val);
> >> +
> >> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
> >> +       lsdc_ureg32_clr(ldev, LSDC_HDMI0_INTF_CTRL_REG, HW_I2C_EN);
> >> +
> >> +       /* Help the HDMI phy get out of reset state */
> >> +       lsdc_wreg32(ldev, LSDC_HDMI0_PHY_CTRL_REG, HDMI_PHY_RESET_N);
> >> +
> >> +       drm_dbg(ddev, "%s reset\n", encoder->name);
> >> +
> >> +       mdelay(20);
> >> +}
> >> +
> >> +const struct drm_encoder_funcs lsdc_pipe0_hdmi_encoder_funcs = {
> >> +       .reset = lsdc_pipe0_hdmi_encoder_reset,
> >> +       .destroy = drm_encoder_cleanup,
> >> +};
> >> +
> >> +/* Built-in HDMI encoder funcs on display pipe 1 */
> > All pipe 1 code looks like a pipe0, just the registers were changed.
> > Could you please refactor that to use a single instance of all
> > functions and pass pipe id through the data struct?
> > Then you can use macros to determine whether to use pipe 0 or pipe 1 register.
> >
>
> Yes, you are right. But please allow me to explain something.
>
> In the past, Thomas told me to untangle it, despite this idea lead to duplicated code(or pattern).
> but at the long run, this pay off.
>
> Because the method of passing pipe id will introduce the "if and else" side effects.
> But my functions have no if and else.
>
>
> ```
> if (pipe == 0) {
>      ...
> } else if (pipe == 1) {
>      ...
> }
> ```

I was thinking about something easier:

static void lsdc_pipe_hdmi_encoder_reset(struct drm_encoder *encoder)
{
   ....
   lsdc_wreg32(ldev, LSDC_CRTCn_DVO_CONF_REG(ldev->pipe_id), val);
   ...
};

So, no ifs, just define per-pipe registers.


>
> Using the C program language's Macro(#define XXX) to generate code is not fun to me.
> Because every time you want to change it, It needs my brains to thinking it twice. Maybe
> more than twice.
>
> 1) It needs my brains to replace the macros manually each time I saw the code.
>
> 2) When I want to change(alter) the prototype, I need to worry about all of the instances.
>     sometimes it is not symmetry. The DVO port and HDMI phy itself is symmetry, but the
>     external display bridge connected with them are not symmetry. So, there are some registers
>     located at the domain of this display controller side should change according to the
>     different type of external display bridge.
>
> 3) Code duplication is actually less harmful than unmaintainable.
>     macros are abstract, as noob level programmer, we completely drop the idea of abstract.
>     Bad abstract means design failure, this is what we are most afraid of.
>     Generally, we would like divide the whole into small cases, handle them one by one.
>     It is actually to review and understand.

Code duplication is both harmful and unmaintainable. It is _very_hard_
to spot the difference between pipe0 and pipe1. And it is _very_easy_
to patch just one instance of these functions leaving the issue in the
second one. So, I'd say, all the c&p functions are a no-go.

>
> 4) From the viewpoint of the hardware, the display output hardware suffer from changes.
>     Because users always want *new* display interface. The need of the users are also varies.
>     Personally, I think macros are best for the symmetry case, while the output part of a
>     display pipe always suffer from change.
>
> >> +
> >> +static void lsdc_pipe1_hdmi_encoder_reset(struct drm_encoder *encoder)
> >> +{
> >> +       struct drm_device *ddev = encoder->dev;
> >> +       struct lsdc_device *ldev = to_lsdc(ddev);
> >> +       u32 val;
> >> +
> >> +       val = PHY_CLOCK_POL | PHY_CLOCK_EN | PHY_DATA_EN;
> >> +       lsdc_wreg32(ldev, LSDC_CRTC1_DVO_CONF_REG, val);
> >> +
> >> +       /* Using built-in GPIO emulated I2C instead of the hardware I2C */
> >> +       lsdc_ureg32_clr(ldev, LSDC_HDMI1_INTF_CTRL_REG, HW_I2C_EN);
> >> +
> >> +       /* Help the HDMI phy get out of reset state */
> >> +       lsdc_wreg32(ldev, LSDC_HDMI1_PHY_CTRL_REG, HDMI_PHY_RESET_N);
> >> +
> >> +       drm_dbg(ddev, "%s reset\n", encoder->name);
> >> +
> >> +       mdelay(20);
> >> +}
> >> +
> >> +const struct drm_encoder_funcs lsdc_pipe1_hdmi_encoder_funcs = {
> >> +       .reset = lsdc_pipe1_hdmi_encoder_reset,
> >> +       .destroy = drm_encoder_cleanup,
> >> +};
>
  

Hi Sui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.6 next-20231031]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/drm-loongson-Introduce-a-minimal-support-for-Loongson-VBIOS/20231030-034730
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231029194607.379459-4-suijingfeng%40loongson.cn
patch subject: [PATCH 3/8] drm/loongson: Allow attach drm bridge driver by calling lsdc_output_init()
config: x86_64-randconfig-001-20231101 (https://download.01.org/0day-ci/archive/20231101/202311011055.5xna6j7t-lkp@intel.com/config)
compiler: gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231101/202311011055.5xna6j7t-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311011055.5xna6j7t-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/loongson/lsdc_output.c:555:5: warning: no previous declaration for 'lsdc_encoder_init' [-Wmissing-declarations]
    int lsdc_encoder_init(struct drm_device *ddev,
        ^~~~~~~~~~~~~~~~~
>> drivers/gpu/drm/loongson/lsdc_output.c:578:5: warning: no previous declaration for 'lsdc_connector_init' [-Wmissing-declarations]
    int lsdc_connector_init(struct drm_device *ddev,
        ^~~~~~~~~~~~~~~~~~~


vim +/lsdc_encoder_init +555 drivers/gpu/drm/loongson/lsdc_output.c

   554	
 > 555	int lsdc_encoder_init(struct drm_device *ddev,
   556			      struct lsdc_output *output,
   557			      unsigned int pipe)
   558	{
   559		const struct lsdc_output_desc *descp = output->descp;
   560		struct drm_encoder *encoder = &output->encoder;
   561		int ret;
   562	
   563		ret = drm_encoder_init(ddev,
   564				       encoder,
   565				       descp->encoder_funcs,
   566				       descp->encoder_type,
   567				       descp->name);
   568		if (ret)
   569			return ret;
   570	
   571		encoder->possible_crtcs = BIT(pipe);
   572	
   573		drm_encoder_helper_add(encoder, descp->encoder_helper_funcs);
   574	
   575		return 0;
   576	}
   577	
 > 578	int lsdc_connector_init(struct drm_device *ddev,
   579				struct lsdc_output *output,
   580				struct i2c_adapter *ddc,
   581				unsigned int pipe)
   582	{
   583		const struct lsdc_output_desc *descp = output->descp;
   584		struct drm_connector *connector = &output->connector;
   585		int ret;
   586	
   587		ret = drm_connector_init_with_ddc(ddev,
   588						  connector,
   589						  descp->connector_funcs,
   590						  descp->connector_type,
   591						  ddc);
   592		if (ret)
   593			return ret;
   594	
   595		drm_connector_helper_add(connector, descp->connector_helper_funcs);
   596	
   597		drm_connector_attach_encoder(connector, &output->encoder);
   598	
   599		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
   600				    DRM_CONNECTOR_POLL_DISCONNECT;
   601	
   602		connector->interlace_allowed = 0;
   603		connector->doublescan_allowed = 0;
   604	
   605		drm_info(ddev, "DisplayPipe-%u has %s\n", pipe, descp->name);
   606	
   607		return 0;
   608	}
   609
  

Hi Sui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.6 next-20231103]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/drm-loongson-Introduce-a-minimal-support-for-Loongson-VBIOS/20231030-034730
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231029194607.379459-4-suijingfeng%40loongson.cn
patch subject: [PATCH 3/8] drm/loongson: Allow attach drm bridge driver by calling lsdc_output_init()
config: x86_64-randconfig-122-20231102 (https://download.01.org/0day-ci/archive/20231104/202311041536.7w9xBsF9-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231104/202311041536.7w9xBsF9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311041536.7w9xBsF9-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/loongson/lsdc_output.c:555:5: sparse: sparse: symbol 'lsdc_encoder_init' was not declared. Should it be static?
>> drivers/gpu/drm/loongson/lsdc_output.c:578:5: sparse: sparse: symbol 'lsdc_connector_init' was not declared. Should it be static?
  

Hi Sui,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master v6.6 next-20231103]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Sui-Jingfeng/drm-loongson-Introduce-a-minimal-support-for-Loongson-VBIOS/20231030-034730
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20231029194607.379459-4-suijingfeng%40loongson.cn
patch subject: [PATCH 3/8] drm/loongson: Allow attach drm bridge driver by calling lsdc_output_init()
config: x86_64-randconfig-122-20231102 (https://download.01.org/0day-ci/archive/20231106/202311060159.WME8x93V-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231106/202311060159.WME8x93V-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311060159.WME8x93V-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/gpu/drm/loongson/lsdc_output.c:555:5: sparse: sparse: symbol 'lsdc_encoder_init' was not declared. Should it be static?
>> drivers/gpu/drm/loongson/lsdc_output.c:578:5: sparse: sparse: symbol 'lsdc_connector_init' was not declared. Should it be static?

vim +/lsdc_encoder_init +555 drivers/gpu/drm/loongson/lsdc_output.c

   554	
 > 555	int lsdc_encoder_init(struct drm_device *ddev,
   556			      struct lsdc_output *output,
   557			      unsigned int pipe)
   558	{
   559		const struct lsdc_output_desc *descp = output->descp;
   560		struct drm_encoder *encoder = &output->encoder;
   561		int ret;
   562	
   563		ret = drm_encoder_init(ddev,
   564				       encoder,
   565				       descp->encoder_funcs,
   566				       descp->encoder_type,
   567				       descp->name);
   568		if (ret)
   569			return ret;
   570	
   571		encoder->possible_crtcs = BIT(pipe);
   572	
   573		drm_encoder_helper_add(encoder, descp->encoder_helper_funcs);
   574	
   575		return 0;
   576	}
   577	
 > 578	int lsdc_connector_init(struct drm_device *ddev,
   579				struct lsdc_output *output,
   580				struct i2c_adapter *ddc,
   581				unsigned int pipe)
   582	{
   583		const struct lsdc_output_desc *descp = output->descp;
   584		struct drm_connector *connector = &output->connector;
   585		int ret;
   586	
   587		ret = drm_connector_init_with_ddc(ddev,
   588						  connector,
   589						  descp->connector_funcs,
   590						  descp->connector_type,
   591						  ddc);
   592		if (ret)
   593			return ret;
   594	
   595		drm_connector_helper_add(connector, descp->connector_helper_funcs);
   596	
   597		drm_connector_attach_encoder(connector, &output->encoder);
   598	
   599		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
   600				    DRM_CONNECTOR_POLL_DISCONNECT;
   601	
   602		connector->interlace_allowed = 0;
   603		connector->doublescan_allowed = 0;
   604	
   605		drm_info(ddev, "DisplayPipe-%u has %s\n", pipe, descp->name);
   606	
   607		return 0;
   608	}
   609