On Tuesday, July 18, 2023 5:31 PM Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> wrote:
>
> Hi,
Hi,
>
> Am Dienstag, 18. Juli 2023, 11:00:25 CEST schrieb Ying Liu:
> > On Tuesday, July 18, 2023 3:49 PM Alexander Stein
> <alexander.stein@xxxxx-
> group.com> wrote:
> > > Hi,
> >
> > Hi,
> >
> > > thanks for the patch.
> >
> > Thanks for your review.
> >
> > > 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@xxxxxxx>
> > > > ---
> > > >
> > > > 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?
> >
> > Maybe, it's better to keep using DRM_DEV_ERROR to achieve consistent
> > error log format across the driver which is implemented in
> drm_dev_printk().
> > I see other DRM drivers do the same.
>
> I see your point. On the other hand the benefit of dev_err_probe() is that the
> message of deferred probe can be seen in
> /sys/kernel/debug/devices_deferred.
> Your check against EPROBE_DEFER will hide the message if something is not
> correct.
>
> Maybe a to be introduced DRM_DEV_ERROR_PROBE might be useful.
Maybe. I assume this is not a must-have for this series - maybe someone may
come along to introduce it in future.
Regards,
Liu Ying
>
> Best regards,
> Alexander
>
> > > > + 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?
> >
> > Ditto.
> >
> > > > + 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?
> >
> > Ditto.
> >
> > > > + 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?
> >
> > Ditto.
> >
> > Regards,
> > Liu Ying
> >
> > > 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@xxxxxxx>");
> > > > +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.t/
> %2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cefbbc19ba3414f89448308
> db8771b5df%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638252
> 694676247773%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdat
> a=BCL6xs5i8jZZDFC2ONKMv5%2B1o0mpuD0ocxC3BnUa2To%3D&reserved=0
> > > q-
> > >
> group.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cc42417f9a9484
> > >
> 3ead2b808db876380d3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> > > %7C638252633665634690%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLj
> > >
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> > >
> 7C%7C&sdata=GUWOZgHFFp0nKImw2aIAsaqMv9KtgI6%2BD%2BaOdDhJ%2B
> > > tU%3D&reserved=0
>
>
> --
> 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.t/
> q-
> group.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cefbbc19ba3414
> f89448308db8771b5df%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C638252694676247773%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&sdata=%2FCtQd5%2BYCDCjOKmsbiO2LSeE1hqQsrmhepmSGalydhc%3
> D&reserved=0
>
