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