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

CK Hu (胡俊光)
2024-12-16T07:39:10Z

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@collabora.com>
> ---

[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

> +};
> +