From: Sam Ravnborg <sam@ravnborg.org> To: "Guido Günther" <agx@sigxcpu.org> Cc: David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Shawn Guo <shawnguo@kernel.org>, Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix Kernel Team <kernel@pengutronix.de>, Fabio Estevam <festevam@gmail.com>, NXP Linux Team <linux-imx@nxp.com>, Andrzej Hajda <a.hajda@samsung.com>, Neil Armstrong <narmstrong@baylibre.com>, Laurent Pinchart <Laurent.pinchart@ideasonboard.com>, Jonas Karlman <jonas@kwiboo.se>, Jernej Skrabec <jernej.skrabec@siol.net>, Lee Jones <lee.jones@linaro.org>, dri-devel@lists.freedesktop.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Robert Chiras <robert.chiras@nxp.com> Subject: Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support Date: Fri, 26 Jul 2019 12:08:17 +0200 [thread overview] Message-ID: <20190726100817.GB9754@ravnborg.org> (raw) In-Reply-To: <3158f4f8c97c21f98c394e5631d74bc60d796522.1563983037.git.agx@sigxcpu.org> Hi Guido. Following some trivial comments. As for the overall design I already commented on that in the binding. (bridge versus display controller) That it can work on top of mxsfb is a good indication that it is a bridge but I just do not see the full picture. In general the code looked clean and neat. On Wed, Jul 24, 2019 at 05:52:26PM +0200, Guido Günther wrote: > This adds initial support for the NWL MIPI DSI Host controller found on > i.MX8 SoCs. > > It adds support for the i.MX8MQ but the same IP can be found on > e.g. the i.MX8QXP. > > It has been tested on the Librem 5 devkit using mxsfb. Looking at mxsfb I wonder hw this was done, as there seems to be no bridge support in mxsfb. Using a patched version of mxsfb? > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 4934fcf5a6f8..904a9eb3a20a 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -16,4 +16,5 @@ obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o > obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o > +obj-y += imx-nwl/ obj-$(ONFIG_DRM_IMX_NWL_DSI) += imx-nwl/? So we do not visit the directory unless required. > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx-nwl/Makefile > @@ -0,0 +1,2 @@ > +imx-nwl-objs := nwl-drv.o nwl-dsi.o The preferred syntax is imx-nwl-y := nwl-drv.o nwl-dsi.o See for example Makefile for mxsfb. Consider to introduce header-test-y += nwl-drv.h nwl-dsi.h So we at build time check that the headers are self-contained. (they include what they need). > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_of.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_print.h> > +#include <drm/drm_probe_helper.h> > +#include <linux/clk-provider.h> > +#include <linux/clk.h> > +#include <linux/component.h> > +#include <linux/gpio/consumer.h> > +#include <linux/irq.h> > +#include <linux/mfd/syscon.h> > +#include <linux/mfd/syscon/imx8mq-iomuxc-gpr.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/phy/phy.h> > +#include <linux/regmap.h> > +#include <linux/sys_soc.h> > +#include <video/videomode.h> > + > +#include "nwl-drv.h" > +#include "nwl-dsi.h" The most typical order of include files are: #include <linux/*> #include <video/*> #include <drm/*> #include "" With the empty lines in-between each block. And sorted like is already done here. This in general for all the files for this driver. > + > +static bool > +imx_nwl_dsi_bridge_mode_fixup(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode) > +{ > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge); > + struct device *dev = dsi->dev; > + union phy_configure_opts new_cfg; > + unsigned long phy_ref_rate; > + int ret; > + > + ret = nwl_dsi_get_dphy_params(dsi, adjusted_mode, &new_cfg); > + if (ret < 0) > + return ret; > + > + /* > + * If hs clock is unchanged, we're all good - all parameters are > + * derived from it atm. > + */ > + if (new_cfg.mipi_dphy.hs_clk_rate == dsi->phy_cfg.mipi_dphy.hs_clk_rate) > + return true; > + > + phy_ref_rate = clk_get_rate(dsi->phy_ref_clk); > + DRM_DEV_DEBUG_DRIVER(dev, "PHY at ref rate: %lu\n", phy_ref_rate); > + if (ret < 0) { > + DRM_DEV_ERROR(dsi->dev, > + "Cannot setup PHY for mode: %ux%u @%d Hz\n", > + adjusted_mode->hdisplay, adjusted_mode->vdisplay, > + adjusted_mode->clock); > + DRM_DEV_ERROR(dsi->dev, "PHY ref clk: %lu, bit clk: %lu\n", > + phy_ref_rate, new_cfg.mipi_dphy.hs_clk_rate); > + } else { > + /* Save the new desired phy config */ > + memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg)); > + } > + > + /* LCDIF + NWL needs active high sync */ > + adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); > + adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); > + > + drm_display_mode_to_videomode(adjusted_mode, &dsi->vm); Hmm, the videomode is just another representation of data already included in display_mode. And, as a personal itch, I consider videomode as something that belongs in the old fb drivers, and not drm drivers. But that may be me only. Sam
WARNING: multiple messages have this Message-ID (diff)
From: Sam Ravnborg <sam@ravnborg.org> To: "Guido Günther" <agx@sigxcpu.org> Cc: Mark Rutland <mark.rutland@arm.com>, devicetree@vger.kernel.org, Jernej Skrabec <jernej.skrabec@siol.net>, Pengutronix Kernel Team <kernel@pengutronix.de>, Neil Armstrong <narmstrong@baylibre.com>, David Airlie <airlied@linux.ie>, Fabio Estevam <festevam@gmail.com>, Sascha Hauer <s.hauer@pengutronix.de>, Jonas Karlman <jonas@kwiboo.se>, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Andrzej Hajda <a.hajda@samsung.com>, Rob Herring <robh+dt@kernel.org>, NXP Linux Team <linux-imx@nxp.com>, Daniel Vetter <daniel@ffwll.ch>, Robert Chiras <robert.chiras@nxp.com>, Lee Jones <lee.jones@linaro.org>, Shawn Guo <shawnguo@kernel.org>, linux-arm-kernel@lists.infradead.org, Laurent Pinchart <Laurent.pinchart@ideasonboard.com> Subject: Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support Date: Fri, 26 Jul 2019 12:08:17 +0200 [thread overview] Message-ID: <20190726100817.GB9754@ravnborg.org> (raw) In-Reply-To: <3158f4f8c97c21f98c394e5631d74bc60d796522.1563983037.git.agx@sigxcpu.org> Hi Guido. Following some trivial comments. As for the overall design I already commented on that in the binding. (bridge versus display controller) That it can work on top of mxsfb is a good indication that it is a bridge but I just do not see the full picture. In general the code looked clean and neat. On Wed, Jul 24, 2019 at 05:52:26PM +0200, Guido Günther wrote: > This adds initial support for the NWL MIPI DSI Host controller found on > i.MX8 SoCs. > > It adds support for the i.MX8MQ but the same IP can be found on > e.g. the i.MX8QXP. > > It has been tested on the Librem 5 devkit using mxsfb. Looking at mxsfb I wonder hw this was done, as there seems to be no bridge support in mxsfb. Using a patched version of mxsfb? > diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile > index 4934fcf5a6f8..904a9eb3a20a 100644 > --- a/drivers/gpu/drm/bridge/Makefile > +++ b/drivers/gpu/drm/bridge/Makefile > @@ -16,4 +16,5 @@ obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/ > obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/ > obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o > obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o > +obj-y += imx-nwl/ obj-$(ONFIG_DRM_IMX_NWL_DSI) += imx-nwl/? So we do not visit the directory unless required. > --- /dev/null > +++ b/drivers/gpu/drm/bridge/imx-nwl/Makefile > @@ -0,0 +1,2 @@ > +imx-nwl-objs := nwl-drv.o nwl-dsi.o The preferred syntax is imx-nwl-y := nwl-drv.o nwl-dsi.o See for example Makefile for mxsfb. Consider to introduce header-test-y += nwl-drv.h nwl-dsi.h So we at build time check that the headers are self-contained. (they include what they need). > + > +#include <drm/drm_atomic_helper.h> > +#include <drm/drm_of.h> > +#include <drm/drm_panel.h> > +#include <drm/drm_print.h> > +#include <drm/drm_probe_helper.h> > +#include <linux/clk-provider.h> > +#include <linux/clk.h> > +#include <linux/component.h> > +#include <linux/gpio/consumer.h> > +#include <linux/irq.h> > +#include <linux/mfd/syscon.h> > +#include <linux/mfd/syscon/imx8mq-iomuxc-gpr.h> > +#include <linux/module.h> > +#include <linux/of.h> > +#include <linux/of_platform.h> > +#include <linux/phy/phy.h> > +#include <linux/regmap.h> > +#include <linux/sys_soc.h> > +#include <video/videomode.h> > + > +#include "nwl-drv.h" > +#include "nwl-dsi.h" The most typical order of include files are: #include <linux/*> #include <video/*> #include <drm/*> #include "" With the empty lines in-between each block. And sorted like is already done here. This in general for all the files for this driver. > + > +static bool > +imx_nwl_dsi_bridge_mode_fixup(struct drm_bridge *bridge, > + const struct drm_display_mode *mode, > + struct drm_display_mode *adjusted_mode) > +{ > + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge); > + struct device *dev = dsi->dev; > + union phy_configure_opts new_cfg; > + unsigned long phy_ref_rate; > + int ret; > + > + ret = nwl_dsi_get_dphy_params(dsi, adjusted_mode, &new_cfg); > + if (ret < 0) > + return ret; > + > + /* > + * If hs clock is unchanged, we're all good - all parameters are > + * derived from it atm. > + */ > + if (new_cfg.mipi_dphy.hs_clk_rate == dsi->phy_cfg.mipi_dphy.hs_clk_rate) > + return true; > + > + phy_ref_rate = clk_get_rate(dsi->phy_ref_clk); > + DRM_DEV_DEBUG_DRIVER(dev, "PHY at ref rate: %lu\n", phy_ref_rate); > + if (ret < 0) { > + DRM_DEV_ERROR(dsi->dev, > + "Cannot setup PHY for mode: %ux%u @%d Hz\n", > + adjusted_mode->hdisplay, adjusted_mode->vdisplay, > + adjusted_mode->clock); > + DRM_DEV_ERROR(dsi->dev, "PHY ref clk: %lu, bit clk: %lu\n", > + phy_ref_rate, new_cfg.mipi_dphy.hs_clk_rate); > + } else { > + /* Save the new desired phy config */ > + memcpy(&dsi->phy_cfg, &new_cfg, sizeof(new_cfg)); > + } > + > + /* LCDIF + NWL needs active high sync */ > + adjusted_mode->flags |= (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC); > + adjusted_mode->flags &= ~(DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC); > + > + drm_display_mode_to_videomode(adjusted_mode, &dsi->vm); Hmm, the videomode is just another representation of data already included in display_mode. And, as a personal itch, I consider videomode as something that belongs in the old fb drivers, and not drm drivers. But that may be me only. Sam _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-07-26 10:08 UTC|newest] Thread overview: 55+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-07-24 15:52 [PATCH 0/3] drm: bridge: Add NWL MIPI DSI host controller support Guido Günther 2019-07-24 15:52 ` Guido Günther 2019-07-24 15:52 ` Guido Günther 2019-07-24 15:52 ` [PATCH 1/3] arm64: imx8mq: add imx8mq iomux-gpr field defines Guido Günther 2019-07-24 15:52 ` Guido Günther 2019-07-24 15:52 ` Guido Günther 2019-07-24 15:52 ` [PATCH 2/3] dt-bindings: display/bridge: Add binding for IMX NWL mipi dsi host controller Guido Günther 2019-07-24 15:52 ` Guido Günther 2019-07-24 15:52 ` Guido Günther 2019-07-26 9:23 ` Sam Ravnborg 2019-07-26 9:23 ` Sam Ravnborg 2019-07-26 9:23 ` Sam Ravnborg 2019-07-26 11:05 ` Guido Günther 2019-07-26 11:05 ` Guido Günther 2019-07-27 1:59 ` Laurent Pinchart 2019-07-27 1:59 ` Laurent Pinchart 2019-07-27 1:57 ` Laurent Pinchart 2019-07-27 1:57 ` Laurent Pinchart 2019-07-27 1:57 ` Laurent Pinchart 2019-07-31 14:37 ` Guido Günther 2019-07-31 14:37 ` Guido Günther 2019-07-24 15:52 ` [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support Guido Günther 2019-07-24 15:52 ` Guido Günther 2019-07-24 15:52 ` Guido Günther 2019-07-26 10:08 ` Sam Ravnborg [this message] 2019-07-26 10:08 ` Sam Ravnborg 2019-07-26 20:01 ` Fabio Estevam 2019-07-26 20:01 ` Fabio Estevam 2019-07-26 20:01 ` Fabio Estevam 2019-07-27 2:04 ` Laurent Pinchart 2019-07-27 2:04 ` Laurent Pinchart 2019-07-27 2:04 ` Laurent Pinchart 2019-07-31 14:38 ` Guido Günther 2019-07-31 14:38 ` Guido Günther 2019-07-31 14:38 ` Guido Günther 2019-07-31 14:35 ` Guido Günther 2019-07-31 14:35 ` Guido Günther 2019-07-31 14:35 ` Guido Günther 2019-07-31 14:43 ` Fabio Estevam 2019-07-31 14:43 ` Fabio Estevam 2019-07-31 14:43 ` Fabio Estevam 2019-07-31 16:40 ` Jernej Škrabec 2019-07-31 16:40 ` Jernej Škrabec 2019-07-31 16:40 ` Jernej Škrabec 2019-07-31 17:40 ` Fabio Estevam 2019-07-31 17:40 ` Fabio Estevam 2019-07-31 17:40 ` Fabio Estevam 2019-07-31 16:54 ` Guido Günther 2019-07-31 16:54 ` Guido Günther 2019-07-31 16:54 ` Guido Günther 2019-07-27 2:47 ` Laurent Pinchart 2019-07-27 2:47 ` Laurent Pinchart 2019-07-27 2:47 ` Laurent Pinchart 2019-08-09 16:25 ` Guido Günther 2019-08-09 16:25 ` Guido Günther
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190726100817.GB9754@ravnborg.org \ --to=sam@ravnborg.org \ --cc=Laurent.pinchart@ideasonboard.com \ --cc=a.hajda@samsung.com \ --cc=agx@sigxcpu.org \ --cc=airlied@linux.ie \ --cc=daniel@ffwll.ch \ --cc=devicetree@vger.kernel.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=festevam@gmail.com \ --cc=jernej.skrabec@siol.net \ --cc=jonas@kwiboo.se \ --cc=kernel@pengutronix.de \ --cc=lee.jones@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-imx@nxp.com \ --cc=linux-kernel@vger.kernel.org \ --cc=mark.rutland@arm.com \ --cc=narmstrong@baylibre.com \ --cc=robert.chiras@nxp.com \ --cc=robh+dt@kernel.org \ --cc=s.hauer@pengutronix.de \ --cc=shawnguo@kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.