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

["CK Hu (胡俊光)" <ck.hu@mediatek.com>]

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]

> +static void mtk_hdmi_unregister_audio_driver(void *data)
> +{
> +       platform_device_unregister(data);
> +}
> +
> +static int mtk_hdmi_register_audio_driver(struct device *dev)
> +{
> +       struct mtk_hdmi *hdmi = dev_get_drvdata(dev);
> +       struct hdmi_audio_param *aud_param = &hdmi->aud_param;
> +       struct hdmi_codec_pdata codec_data = {
> +               .i2s = 1,
> +               .max_i2s_channels = 2,
> +               .data = hdmi,
> +               .ops = hdmi->conf->ver_conf->codec_ops
> +       };
> +       int ret;
> +
> +       aud_param->aud_codec = HDMI_AUDIO_CODING_TYPE_PCM;
> +       aud_param->aud_sample_size = HDMI_AUDIO_SAMPLE_SIZE_16;
> +       aud_param->aud_input_type = HDMI_AUD_INPUT_I2S;
> +       aud_param->aud_i2s_fmt = HDMI_I2S_MODE_I2S_24BIT;
> +       aud_param->aud_mclk = HDMI_AUD_MCLK_128FS;
> +       aud_param->aud_input_chan_type = HDMI_AUD_CHAN_TYPE_2_0;

You squash mtk_hdmi_outout_init() into this function.
I'm not sure this is necessary or not.
If it's necessary, I would like this modification to be a separate patch and it's easier to review.
I want this patch is simply moving common code.

And this patch remove hdmi->csp and it is really not necessary.
Let removing hdmi->csp be a clean up patch.

> +
> +       hdmi->audio_pdev = platform_device_register_data(dev,
> +                                                        HDMI_CODEC_DRV_NAME,
> +                                                        PLATFORM_DEVID_AUTO,
> +                                                        &codec_data,
> +                                                        sizeof(codec_data));
> +       if (IS_ERR(hdmi->audio_pdev))
> +               return PTR_ERR(hdmi->audio_pdev);
> +
> +       ret = devm_add_action_or_reset(dev, mtk_hdmi_unregister_audio_driver,
> +                                      hdmi->audio_pdev);

This adding action or reset is other than moving common function.
So separate this to a refinement patch.

Regards,
CK

> +       if (ret) {
> +               platform_device_unregister(hdmi->audio_pdev);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +