Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Alexander Stein <alexander.stein@ew.tq-group.com>]

Hi,

thanks for the patch.

Am Montag, 17. Juli 2023, 08:18:31 CEST schrieb Liu Ying:
> Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> controller and a Synopsys Designware MIPI DPHY.  Some configurations
> and extensions to them are controlled by i.MX93 media blk-ctrl.
> 
> Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/gpu/drm/bridge/imx/Kconfig          |  10 +
>  drivers/gpu/drm/bridge/imx/Makefile         |   1 +
>  drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 934 ++++++++++++++++++++
>  3 files changed, 945 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> 
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> b/drivers/gpu/drm/bridge/imx/Kconfig index 9fae28db6aa7..5182298c7182
> 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -49,4 +49,14 @@ config DRM_IMX8QXP_PIXEL_LINK_TO_DPI
>  	  Choose this to enable pixel link to display pixel 
interface(PXL2DPI)
>  	  found in Freescale i.MX8qxp processor.
> 
> +config DRM_IMX93_MIPI_DSI
> +	tristate "Freescale i.MX93 specific extensions for Synopsys DW MIPI 
DSI"
> +	depends on OF
> +	depends on COMMON_CLK
> +	select DRM_DW_MIPI_DSI
> +	select GENERIC_PHY_MIPI_DPHY
> +	help
> +	  Choose this to enable MIPI DSI controller found in Freescale 
i.MX93
> +	  processor.
> +
>  endif # ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/gpu/drm/bridge/imx/Makefile
> b/drivers/gpu/drm/bridge/imx/Makefile index 8e2ebf3399a1..2b0c2e44aa1b
> 100644
> --- a/drivers/gpu/drm/bridge/imx/Makefile
> +++ b/drivers/gpu/drm/bridge/imx/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
>  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o
>  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
>  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o
> +obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o
> diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c new file mode 100644
> index 000000000000..77f59e3407a0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c

> [snip]

> +static int imx93_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct imx93_dsi *dsi;
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->regmap = syscon_regmap_lookup_by_phandle(np, "fsl,media-blk-
ctrl");
> +	if (IS_ERR(dsi->regmap)) {
> +		ret = PTR_ERR(dsi->regmap);
> +		DRM_DEV_ERROR(dev, "failed to get block ctrl regmap: 
%d\n", ret);

Could you use dev_err_probe here instead?

> +		return ret;
> +	}
> +
> +	dsi->clk_pixel = devm_clk_get(dev, "pix");
> +	if (IS_ERR(dsi->clk_pixel)) {
> +		ret = PTR_ERR(dsi->clk_pixel);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to get pixel clock: 
%d\n", ret);

Could you use dev_err_probe here instead?

> +		return ret;
> +	}
> +
> +	dsi->clk_cfg = devm_clk_get(dev, "phy_cfg");
> +	if (IS_ERR(dsi->clk_cfg)) {
> +		ret = PTR_ERR(dsi->clk_cfg);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to get phy cfg clock: 
%d\n", ret);
> +		return ret;
> +	}
> +
> +	dsi->clk_ref = devm_clk_get(dev, "phy_ref");
> +	if (IS_ERR(dsi->clk_ref)) {
> +		ret = PTR_ERR(dsi->clk_ref);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to get phy ref clock: 
%d\n", ret);

Could you use dev_err_probe here instead?

> +		return ret;
> +	}
> +
> +	dsi->ref_clk_rate = clk_get_rate(dsi->clk_ref);
> +	if (dsi->ref_clk_rate < REF_CLK_RATE_MIN ||
> +	    dsi->ref_clk_rate > REF_CLK_RATE_MAX) {
> +		DRM_DEV_ERROR(dev, "invalid phy ref clock rate %lu\n",
> +			      dsi->ref_clk_rate);
> +		return -EINVAL;
> +	}
> +	DRM_DEV_DEBUG_DRIVER(dev, "phy ref clock rate: %lu\n", dsi-
>ref_clk_rate);
> +
> +	dsi->dev = dev;
> +	dsi->pdata.max_data_lanes = 4;
> +	dsi->pdata.mode_valid = imx93_dsi_mode_valid;
> +	dsi->pdata.mode_fixup = imx93_dsi_mode_fixup;
> +	dsi->pdata.get_input_bus_fmts = imx93_dsi_get_input_bus_fmts;
> +	dsi->pdata.phy_ops = &imx93_dsi_phy_ops;
> +	dsi->pdata.host_ops = &imx93_dsi_host_ops;
> +	dsi->pdata.priv_data = dsi;
> +	platform_set_drvdata(pdev, dsi);
> +
> +	dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> +	if (IS_ERR(dsi->dmd)) {
> +		ret = PTR_ERR(dsi->dmd);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to probe dw_mipi_dsi: 
%d\n", ret);

Could you use dev_err_probe here instead?

Best regards,
Alexander

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void imx93_dsi_remove(struct platform_device *pdev)
> +{
> +	struct imx93_dsi *dsi = platform_get_drvdata(pdev);
> +
> +	dw_mipi_dsi_remove(dsi->dmd);
> +}
> +
> +static const struct of_device_id imx93_dsi_dt_ids[] = {
> +	{ .compatible = "fsl,imx93-mipi-dsi", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx93_dsi_dt_ids);
> +
> +static struct platform_driver imx93_dsi_driver = {
> +	.probe	= imx93_dsi_probe,
> +	.remove_new = imx93_dsi_remove,
> +	.driver	= {
> +		.of_match_table = imx93_dsi_dt_ids,
> +		.name = "imx93_mipi_dsi",
> +	},
> +};
> +module_platform_driver(imx93_dsi_driver);
> +
> +MODULE_DESCRIPTION("Freescale i.MX93 MIPI DSI driver");
> +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
> +MODULE_LICENSE("GPL");


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/