Re: [RFC PATCH 2/2] drm/mediatek: add mtk8195 hdmi display driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Guillaume:

On Mon, 2021-11-08 at 01:08 +0100, Guillaume Ranquet wrote:
> Signed-off-by: Guillaume Ranquet <granquet@xxxxxxxxxxxx>
> Change-Id: I6399dec26cfe56a338c2ca96989cb7cbd14e292b
> ---
>  drivers/gpu/drm/mediatek/Kconfig              |    9 +
>  drivers/gpu/drm/mediatek/Makefile             |    2 +
>  drivers/gpu/drm/mediatek/mtk_hdmi_common.c    |  219 +-
>  drivers/gpu/drm/mediatek/mtk_hdmi_common.h    |   24 +-
>  drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c    | 1835
> +++++++++++++++++
>  drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.h    |   26 +
>  .../gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.c    |  530 +++++
>  .../gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.h    |   20 +
>  .../gpu/drm/mediatek/mtk_mt8195_hdmi_regs.h   |  329 +++
>  9 files changed, 2932 insertions(+), 62 deletions(-)
>  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.c
>  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi.h
>  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.c
>  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_ddc.h
>  create mode 100644 drivers/gpu/drm/mediatek/mtk_mt8195_hdmi_regs.h
> 
> diff --git a/drivers/gpu/drm/mediatek/Kconfig
> b/drivers/gpu/drm/mediatek/Kconfig
> index 0df75bceb74e..76cc402cbd75 100644
> --- a/drivers/gpu/drm/mediatek/Kconfig
> +++ b/drivers/gpu/drm/mediatek/Kconfig
> @@ -31,3 +31,12 @@ config DRM_MEDIATEK_HDMI
>  	help
>  	  DRM/KMS HDMI driver for Mediatek SoCs
>  
> +config DRM_MEDIATEK_HDMI_MT8195_SUSPEND_LOW_POWER
> +	tristate "DRM HDMI SUSPEND LOW POWER Support for Mediatek
> mt8195 SoCs"
> +	depends on DRM_MEDIATEK_HDMI
> +	help
> +	  DRM/KMS HDMI SUSPEND_LOW_POWER for Mediatek SoCs.
> +	  Choose this option if you want to disable/enable
> +	  clock and power domain when platform enter suspend,
> +	  and this config depends on DRM_MEDIATEK_HDMI.
> +
> diff --git a/drivers/gpu/drm/mediatek/Makefile
> b/drivers/gpu/drm/mediatek/Makefile
> index 107b6fbbdbf7..477c0648643c 100644
> --- a/drivers/gpu/drm/mediatek/Makefile
> +++ b/drivers/gpu/drm/mediatek/Makefile
> @@ -26,6 +26,8 @@ mediatek-drm-hdmi-objs := mtk_cec.o \
>  			  mtk_hdmi.o \
>  			  mtk_hdmi_ddc.o \
>  			  mtk_hdmi_common.o \
> +			  mtk_mt8195_hdmi.o \
> +			  mtk_mt8195_hdmi_ddc.o \
>  
>  obj-$(CONFIG_DRM_MEDIATEK_HDMI) += mediatek-drm-hdmi.o
>  
> diff --git a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
> b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
> index 3c38a3e73920..19083be0978a 100644
> --- a/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
> +++ b/drivers/gpu/drm/mediatek/mtk_hdmi_common.c
> @@ -1,5 +1,15 @@
>  #include "mtk_hdmi_common.h"
>  
> +const char *const
> mtk_hdmi_clk_names_mt8195[MTK_MT8195_HDMI_CLK_COUNT] = {
> +	[MTK_MT8195_HDMI_CLK_UNIVPLL_D6D4] = "univpll_d6_d4",
> +	[MTK_MT8195_HDMI_CLK_MSDCPLL_D2] = "msdcpll_d2",
> +	[MTK_MT8195_HDMI_CLK_HDMI_APB_SEL] = "hdmi_apb_sel",
> +	[MTK_MT8195_HDMI_UNIVPLL_D4D8] = "univpll_d4_d8",
> +	[MTK_MT8195_HDIM_HDCP_SEL] = "hdcp_sel",
> +	[MTK_MT8195_HDMI_HDCP_24M_SEL] = "hdcp24_sel",
> +	[MTK_MT8195_HDMI_VPP_SPLIT_HDMI] = "split_hdmi",
> +};
> +
>  const char * const
> mtk_hdmi_clk_names_mt8183[MTK_MT8183_HDMI_CLK_COUNT] = {
>  	[MTK_MT8183_HDMI_CLK_HDMI_PIXEL] = "pixel",
>  	[MTK_MT8183_HDMI_CLK_HDMI_PLL] = "pll",
> @@ -138,6 +148,18 @@ int mtk_hdmi_setup_avi_infoframe(struct mtk_hdmi
> *hdmi, u8 *buffer, size_t bufsz
>  		return err;
>  	}
>  
> +	if(hdmi->conf && hdmi->conf->is_mt8195) {
> +		frame.colorimetry = hdmi->colorimtery;
> +		//no need, since we cannot support other extended
> colorimetry?
> +		if (frame.colorimetry == HDMI_COLORIMETRY_EXTENDED)
> +			frame.extended_colorimetry = hdmi-
> >extended_colorimetry;

Separate colorimetry related part to an independent patch and use hdmi-
>conf->colorimetry_support instead of hdmi->conf->is_mt8195.

> +
> +		/* quantiation range:limited or full */
> +		if (frame.colorspace == HDMI_COLORSPACE_RGB)
> +			frame.quantization_range = hdmi-
> >quantization_range;
> +		else
> +			frame.ycc_quantization_range = hdmi-
> >ycc_quantization_range;
> +	}
>  	err = hdmi_avi_infoframe_pack(&frame, buffer, bufsz);
>  
>  	if (err < 0) {
> @@ -155,6 +177,11 @@ void mtk_hdmi_send_infoframe(struct mtk_hdmi
> *hdmi, u8 *buffer_spd, size_t bufsz
>  	mtk_hdmi_setup_spd_infoframe(hdmi, buffer_spd, bufsz_spd,
> "mediatek", "On-chip HDMI");
>  }
>  
> +static struct mtk_hdmi_ddc *hdmi_ddc_ctx_from_mtk_hdmi(struct
> mtk_hdmi *hdmi)
> +{
> +	return container_of(hdmi->ddc_adpt, struct mtk_hdmi_ddc, adap);
> +}
> +
>  int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi *hdmi,
>  				   struct platform_device *pdev, const
> char *const *clk_names, size_t num_clocks)
>  {
> @@ -173,39 +200,41 @@ int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi
> *hdmi,
>  		return ret;
>  	}
>  
> -	/* The CEC module handles HDMI hotplug detection */
> -	cec_np = of_get_compatible_child(np->parent, "mediatek,mt8173-
> cec");
> -	if (!cec_np) {
> -		dev_err(dev, "Failed to find CEC node\n");
> -		return -EINVAL;
> -	}
> +	if (!hdmi->conf || !hdmi->conf->is_mt8195) {
> +		/* The CEC module handles HDMI hotplug detection */

Separate CEC hoplug related part to an independent patch.

> +		cec_np = of_get_compatible_child(np->parent,
> "mediatek,mt8173-cec");
> +		if (!cec_np) {
> +			dev_err(dev, "Failed to find CEC node\n");
> +			return -EINVAL;
> +		}
>  
> -	cec_pdev = of_find_device_by_node(cec_np);
> -	if (!cec_pdev) {
> -		dev_err(hdmi->dev, "Waiting for CEC device %pOF\n",
> -				cec_np);
> +		cec_pdev = of_find_device_by_node(cec_np);
> +		if (!cec_pdev) {
> +			dev_err(hdmi->dev, "Waiting for CEC device
> %pOF\n",
> +					cec_np);
> +			of_node_put(cec_np);
> +			return -EPROBE_DEFER;
> +		}
>  		of_node_put(cec_np);
> -		return -EPROBE_DEFER;
> -	}
> -	of_node_put(cec_np);
> -	hdmi->cec_dev = &cec_pdev->dev;
> -	/*
> -	 * The mediatek,syscon-hdmi property contains a phandle link to
> the
> -	 * MMSYS_CONFIG device and the register offset of the
> HDMI_SYS_CFG
> -	 * registers it contains.
> -	 */
> -	regmap = syscon_regmap_lookup_by_phandle(np, "mediatek,syscon-
> hdmi");
> -	ret = of_property_read_u32_index(np, "mediatek,syscon-hdmi", 1,
> -			&hdmi->sys_offset);
> -	if (IS_ERR(regmap))
> -		ret = PTR_ERR(regmap);
> -	if (ret) {
> -		dev_err(dev,
> -				"Failed to get system configuration
> registers: %d\n",
> -				ret);
> -		goto put_device;
> +		hdmi->cec_dev = &cec_pdev->dev;
> +		/*
> +		 * The mediatek,syscon-hdmi property contains a phandle
> link to the
> +		 * MMSYS_CONFIG device and the register offset of the
> HDMI_SYS_CFG

Separate HDMI_SYS_CFG related part to an independent patch.

> +		 * registers it contains.
> +		 */
> +		regmap = syscon_regmap_lookup_by_phandle(np,
> "mediatek,syscon-hdmi");
> +		ret = of_property_read_u32_index(np, "mediatek,syscon-
> hdmi", 1,
> +				&hdmi->sys_offset);
> +		if (IS_ERR(regmap))
> +			ret = PTR_ERR(regmap);
> +		if (ret) {
> +			dev_err(dev,
> +					"Failed to get system
> configuration registers: %d\n",
> +					ret);
> +			goto put_device;
> +		}
> +		hdmi->sys_regmap = regmap;
>  	}
> -	hdmi->sys_regmap = regmap;
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!mem) {
> @@ -219,20 +248,22 @@ int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi
> *hdmi,
>  		goto put_device;
>  	}
>  
> -	remote = of_graph_get_remote_node(np, 1, 0);
> -	if (!remote) {
> -		ret = -EINVAL;
> -		goto put_device;
> -	}
> -
> -	if (!of_device_is_compatible(remote, "hdmi-connector")) {
> -		hdmi->next_bridge = of_drm_find_bridge(remote);
> -		if (!hdmi->next_bridge) {
> -			dev_err(dev, "Waiting for external bridge\n");
> -			of_node_put(remote);
> -			ret = -EPROBE_DEFER;
> +	if (!hdmi->conf || !hdmi->conf->is_mt8195) {
> +		remote = of_graph_get_remote_node(np, 1, 0);
> +		if (!remote) {
> +			ret = -EINVAL;
>  			goto put_device;
>  		}
> +
> +		if (!of_device_is_compatible(remote, "hdmi-connector")) 
> {
> +			hdmi->next_bridge = of_drm_find_bridge(remote);
> +			if (!hdmi->next_bridge) {
> +				dev_err(dev, "Waiting for external
> bridge\n");
> +				of_node_put(remote);
> +				ret = -EPROBE_DEFER;
> +				goto put_device;
> +			}
> +		}
>  	}
>  
>  	i2c_np = of_parse_phandle(pdev->dev.of_node, "ddc-i2c-bus", 0);
> @@ -251,9 +282,16 @@ int mtk_hdmi_dt_parse_pdata(struct mtk_hdmi
> *hdmi,
>  		goto put_device;
>  	}
>  
> +	//TODO: rework this... this is ugly
> +	if (hdmi->conf && hdmi->conf->is_mt8195) {
> +		ddc = hdmi_ddc_ctx_from_mtk_hdmi(hdmi);
> +		ddc->regs = hdmi->regs;
> +	}

Separate ddc related part to an independent patch.

> +
>  	return 0;
>  put_device:
> -	put_device(hdmi->cec_dev);
> +	if (!hdmi->conf || !hdmi->conf->is_mt8195)
> +		put_device(hdmi->cec_dev);
>  	return ret;
>  }
>  
> @@ -265,7 +303,10 @@ static int mtk_hdmi_register_audio_driver(struct
> device *dev)
>  		.data = hdmi,
>  	};
>  
> -	set_hdmi_codec_pdata_mt8183(&codec_data);
> +	if(hdmi->conf && hdmi->conf->is_mt8195)
> +		set_hdmi_codec_pdata_mt8195(&codec_data);
> +	else
> +		set_hdmi_codec_pdata_mt8183(&codec_data);

set_hdmi_codec_pdata_mt8195() and set_hdmi_codec_pdata_mt8183() has the
same statement:

	codec_data->ops = &mtk_hdmi_audio_codec_ops;
	codec_data->max_i2s_channels = 2;
	codec_data->i2s = 1;

I think you could move these three lines out of that function, and set
ops as

	codec_data->ops = hdmi->conf->audio_codec_ops;

>  
>  	pdev = platform_device_register_data(dev, HDMI_CODEC_DRV_NAME,
>  					     PLATFORM_DEVID_AUTO,
> &codec_data,
> @@ -297,15 +338,23 @@ int mtk_drm_hdmi_probe(struct platform_device
> *pdev)
>  		return ret;
>  	}
>  
> -	ret = mtk_hdmi_dt_parse_pdata(hdmi, pdev,
> mtk_hdmi_clk_names_mt8183, ARRAY_SIZE(mtk_hdmi_clk_names_mt8183));
> +	if(hdmi->conf && hdmi->conf->is_mt8195)
> +		ret = mtk_hdmi_dt_parse_pdata(hdmi, pdev,
> mtk_hdmi_clk_names_mt8195, ARRAY_SIZE(mtk_hdmi_clk_names_mt8195));
> +	else
> +		ret = mtk_hdmi_dt_parse_pdata(hdmi, pdev,
> mtk_hdmi_clk_names_mt8183, ARRAY_SIZE(mtk_hdmi_clk_names_mt8183));
>  
>  	if (ret)
>  		return ret;
>  
>  	platform_set_drvdata(pdev, hdmi);
>  
> -	mtk_hdmi_output_init_mt8183(hdmi);
> -	hdmi->bridge.funcs = &mtk_mt8183_hdmi_bridge_funcs;
> +	if (hdmi->conf && hdmi->conf->is_mt8195) {
> +		mtk_hdmi_output_init_mt8195(hdmi);
> +		hdmi->bridge.funcs = &mtk_mt8195_hdmi_bridge_funcs;
> +	} else {
> +		mtk_hdmi_output_init_mt8183(hdmi);
> +		hdmi->bridge.funcs = &mtk_mt8183_hdmi_bridge_funcs;
> +	}

mtk_hdmi_output_init_mt8195() and mtk_hdmi_output_init_mt8183() has
common code, try to merge the common part.

I review partially here. And I would keep on reviewing the rest.

Regards,
CK

>  
>  	hdmi->bridge.of_node = pdev->dev.of_node;
>  	drm_bridge_add(&hdmi->bridge);
> @@ -324,7 +373,10 @@ int mtk_drm_hdmi_remove(struct platform_device
> *pdev)
>  
>  	drm_bridge_remove(&hdmi->bridge);
>  
> -	mtk_hdmi_clk_disable_mt8183(hdmi);
> +	if(hdmi->conf && hdmi->conf->is_mt8195)
> +		mtk_hdmi_clk_disable_mt8195(hdmi);
> +	else
> +		mtk_hdmi_clk_disable_mt8183(hdmi);
>  	i2c_put_adapter(hdmi->ddc_adpt);
>  
>  	return 0;
> @@ -339,6 +391,13 @@ static const struct mtk_hdmi_conf
> mtk_hdmi_conf_mt8167 = {
>  	.cea_modes_only = true,
>  };
>  
> 




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux