Re: [PATCH v14 09/10] drm/mediatek: DP audio support for MT8195

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

Il 12/07/22 13:12, Bo-Chen Chen ha scritto:
> From: Guillaume Ranquet <granquet@baylibre.com>
> 
> This patch adds audio support to the DP driver for MT8195 with up to 8
> channels.
> 
> Signed-off-by: Guillaume Ranquet <granquet@baylibre.com>
> Signed-off-by: Bo-Chen Chen <rex-bc.chen@mediatek.com>
> ---
>   drivers/gpu/drm/mediatek/mtk_dp.c     | 723 ++++++++++++++++++++++++++
>   drivers/gpu/drm/mediatek/mtk_dp_reg.h |   2 +
>   2 files changed, 725 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_dp.c b/drivers/gpu/drm/mediatek/mtk_dp.c
> index 5ab646bd2bc4..fa7bb102a105 100644
> --- a/drivers/gpu/drm/mediatek/mtk_dp.c
> +++ b/drivers/gpu/drm/mediatek/mtk_dp.c

..snip..

> @@ -2082,6 +2693,104 @@ static void mtk_dp_debounce_timer(struct timer_list *t)
>   	mtk_dp->need_debounce = true;
>   }
>   
> +/*
> + * HDMI audio codec callbacks
> + */
> +static int mtk_dp_audio_hw_params(struct device *dev, void *data,
> +				  struct hdmi_codec_daifmt *daifmt,
> +				  struct hdmi_codec_params *params)
> +{
> +	struct mtk_dp *mtk_dp = dev_get_drvdata(dev);
> +	struct mtk_dp_audio_cfg cfg;
> +
> +	if (!mtk_dp->enabled) {
> +		pr_err("%s, DP is not ready!\n", __func__);

drm_err() here please.

> +		return -ENODEV;
> +	}
> +
> +	cfg.channels = params->cea.channels;
> +	cfg.sample_rate = params->sample_rate;
> +	cfg.word_length_bits = 24;
> +
> +	mtk_dp_audio_setup(mtk_dp, &cfg);
> +
> +	return 0;
> +}
> +

..snip..

> +
> +static int mtk_dp_register_audio_driver(struct device *dev)
> +{
> +	struct mtk_dp *mtk_dp = dev_get_drvdata(dev);
> +	struct hdmi_codec_pdata codec_data = {
> +		.ops = &mtk_dp_audio_codec_ops,
> +		.max_i2s_channels = 8,
> +		.i2s = 1,
> +		.data = mtk_dp,
> +	};
> +	struct platform_device *pdev;
> +
> +	pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
> +					     PLATFORM_DEVID_AUTO, &codec_data,
> +					     sizeof(codec_data));
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);

You're never unregistering this device, which is unacceptable.

Please add a platform device pointer in struct mtk_dp, so that this function
becomes, simply:

	mtk_dp->audio_pdev = platform_device_register_data(dev,
						HDMI_CODEC_DRV_NAME,
						PLATFORM_DEVID_AUTO, &codec_data,
						sizeof(codec_data));
	return PTR_ERR_OR_ZERO(mtk_dp->audio_pdev);

> +
> +	return 0;
> +}
> +
>   static int mtk_dp_probe(struct platform_device *pdev)
>   {
>   	struct mtk_dp *mtk_dp;
> @@ -2127,8 +2836,21 @@ static int mtk_dp_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, -EPROBE_DEFER,
>   				     "failed to request mediatek dptx irq\n");
>   
> +	mutex_init(&mtk_dp->edid_lock);
> +	mutex_init(&mtk_dp->eld_lock);
> +	mutex_init(&mtk_dp->update_plugged_status_lock);
> +
>   	platform_set_drvdata(pdev, mtk_dp);
>   
> +	if (!mtk_dp_is_edp(mtk_dp)) {
> +		ret = mtk_dp_register_audio_driver(dev);
> +		if (ret) {
> +			dev_err(dev, "Failed to register audio driver: %d\n",
> +				ret);
> +			return ret;
> +		}
> +	}
> +
>   	mtk_dp->phy_dev = platform_device_register_data(dev, "mediatek-dp-phy",
>   							PLATFORM_DEVID_AUTO,
>   							&mtk_dp->regs,
> @@ -2174,6 +2896,7 @@ static int mtk_dp_remove(struct platform_device *pdev)
>   
>   	platform_device_unregister(mtk_dp->phy_dev);

... and unregister it here:

	if (mtk_dp->audio_pdev)
		platform_device_unregister(mtk_dp->audio_pdev);

>   	mtk_dp_video_mute(mtk_dp, true);
> +	mtk_dp_audio_mute(mtk_dp, true);
>   	del_timer_sync(&mtk_dp->debounce_timer);
>   
>   	pm_runtime_disable(&pdev->dev);

Regards,
Angelo