Re: [PATCH v2 12/15] drm/mediatek: mtk_hdmi: Split driver and add common probe function

[CK Hu (胡俊光)]

Hi, Angelo:

On Thu, 2024-12-05 at 12:45 +0100, AngeloGioacchino Del Regno wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
> 
> 
> In preparation for adding a new driver for the HDMI TX v2 IP,
> split out the functions that will be common between the already
> present mtk_hdmi (v1) driver and the new one.
> 
> Since the probe flow for both drivers is 90% similar, add a common
> probe function that will be called from each driver's .probe()
> callback, avoiding lots of code duplication.
> 
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxx>
> ---

[snip]

> -struct mtk_hdmi {
> -       struct drm_bridge bridge;
> -       struct drm_bridge *next_bridge;
> -       struct drm_connector *curr_conn;/* current connector (only valid when 'enabled') */
> -       struct device *dev;
> -       const struct mtk_hdmi_conf *conf;
> -       struct phy *phy;
> -       struct device *cec_dev;
> -       struct i2c_adapter *ddc_adpt;
> -       struct clk *clk[MTK_HDMI_CLK_COUNT];
> -       struct drm_display_mode mode;
> -       bool dvi_mode;
> -       u32 min_clock;
> -       u32 max_clock;
> -       u32 max_hdisplay;
> -       u32 max_vdisplay;
> -       u32 ibias;
> -       u32 ibias_up;
> -       struct regmap *sys_regmap;
> -       unsigned int sys_offset;
> -       struct regmap *regs;
> -       enum hdmi_colorspace csp;
> -       struct hdmi_audio_param aud_param;
> -       bool audio_enable;
> -       bool powered;
> -       bool enabled;
> -       hdmi_codec_plugged_cb plugged_cb;
> -       struct device *codec_dev;
> -       struct mutex update_plugged_status_lock;
> };
> 
> +struct mtk_hdmi {
> +       struct drm_bridge bridge;
> +       struct device *dev;
> +       const struct mtk_hdmi_conf *conf;
> +       struct phy *phy;
> +       struct i2c_adapter *ddc_adpt;
> +       struct clk **clk;
> +       struct drm_display_mode mode;
> +       bool dvi_mode;
> +       struct regmap *sys_regmap;
> +       unsigned int sys_offset;
> +       struct regmap *regs;
> +
> +       bool powered;
> +       bool enabled;
> +       unsigned int irq;

You add something which is used in v2 but not in v1.
Move these to the v2 patch, or separate them to a precondition patch of v2.

> +       enum hdmi_hpd_state hpd;
> +
> +       /* Audio */
> +       struct platform_device *audio_pdev;
> +       struct hdmi_audio_param aud_param;
> +       bool audio_enable;
> +
> +       struct drm_connector *curr_conn;/* current connector (only valid when 'enabled') */
> +       struct mutex update_plugged_status_lock;
> +       struct device *cec_dev;
> +       struct device *codec_dev;
> +       hdmi_codec_plugged_cb plugged_cb;
> +       struct drm_bridge *next_bridge;

I don't know why you reorder these.
If it's necessary, separate these reorder to a refinement patch.

Regards,
CK

> +};
> +