Hi Guido,
Thanks for your work on this driver!
On Wed, Jul 24, 2019 at 12:52 PM Guido Günther <agx@xxxxxxxxxxx> wrote:
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx-nwl/Kconfig
> @@ -0,0 +1,15 @@
> +config DRM_IMX_NWL_DSI
> + tristate "Support for Northwest Logic MIPI DSI Host controller"
> + depends on DRM && (ARCH_MXC || ARCH_MULTIPLATFORM || COMPILE_TEST)
This IP could potentially be found on other SoCs, so no need to make
it depend on ARCH_MXC.
> +#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>
I did not find gpio AP used in this driver.
> +static void imx_nwl_dsi_set_clocks(struct imx_nwl_dsi *dsi, bool enable)
Better make it to return 'int' instead...
> +{
> + struct device *dev = dsi->dev;
> + const char *id;
> + struct clk *clk;
> + unsigned long new_rate, cur_rate;
> + bool enabled;
> + size_t i;
> + int ret;
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "%sabling platform clocks",
Please remove the letter 's' from 'sabling'.
> + enable ? "en" : "dis");
> + ret = clk_prepare_enable(clk);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to enable clock %s",
> + id);
and propagate the error in case of clk_prepare_enable() failure.
> + }
> + dsi->clk_config[i].enabled = true;
> + cur_rate = clk_get_rate(clk);
> + DRM_DEV_DEBUG_DRIVER(
> + dev, "Enabled %s clk (rate: req=%lu act=%lu)\n",
> + id, new_rate, cur_rate);
> + } else if (enabled) {
> + clk_disable_unprepare(clk);
> + dsi->clk_config[i].enabled = false;
> + DRM_DEV_DEBUG_DRIVER(dev, "Disabled %s clk\n", id);
> + }
> + }
> +}
> +
> +static void imx_nwl_dsi_enable(struct imx_nwl_dsi *dsi)
Same here. Please return 'int' instead.
> +{
> + struct device *dev = dsi->dev;
> + int ret;
> +
> + imx_nwl_dsi_set_clocks(dsi, true);
> +
> + ret = dsi->pdata->poweron(dsi);
> + if (ret < 0)
> + DRM_DEV_ERROR(dev, "Failed to power on DSI (%d)\n", ret);
If the power domain failed to turn on, it is better to propagate the error.
> + 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) {
This check looks wrong. At this point ret is always 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 */
Would this still work if DCSS is used instead?
> + 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);
> + drm_mode_debug_printmodeline(adjusted_mode);
> +
> + return ret == 0;
At this point ret is always 0.
> +static void imx_nwl_dsi_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> + struct imx_nwl_dsi *dsi = bridge_to_dsi(bridge);
> +
> + if (dsi->dpms_mode == DRM_MODE_DPMS_ON)
> + return;
> +
> + imx_nwl_select_input_source(dsi);
This function is i.MX8M specific, so better protect it to run only for
the i.MX8M variant.
> + pm_runtime_get_sync(dsi->dev);
> + imx_nwl_dsi_enable(dsi);
> + nwl_dsi_enable(dsi);
Please check the error and propagate in the case of failure.
> + dsi->dpms_mode = DRM_MODE_DPMS_ON;
> +}
> +
> + dsi->csr = syscon_regmap_lookup_by_phandle(np, "csr");
> + if (IS_ERR(dsi->csr) && dsi->pdata->ext_regs & IMX_REG_CSR) {
> + ret = PTR_ERR(dsi->csr);
> + DRM_DEV_ERROR(dsi->dev, "Failed to get CSR regmap: %d\n",
In this function (and globally in the driver) there is a mix of
DRM_DEV_ERROR() and dev_err().
Can we just pick one of the two and use it consistently?
Not sure what is the norm in drm code, but IMHO dev_err() looks prettier :-)
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + base = devm_ioremap_resource(dsi->dev, res);
Could use devm_platform_ioremap_resource(), which makes it simpler.
> +err_cleanup:
> + devm_free_irq(dev, dsi->irq, dsi);
No need to call devm_free_irq() here. The devm functions do not need
to be freed on probe.
> diff --git a/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c
> new file mode 100644
> index 000000000000..0e1463af162f
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx-nwl/nwl-dsi.c
> @@ -0,0 +1,745 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * NWL DSI host driver
> + *
> + * Copyright (C) 2017 NXP
> + * Copyright (C) 2019 Purism SPC
> + */
> +
> +#include <asm/unaligned.h>
Is this asm header required?
> +/*
> + * DSI Video mode
> + */
Single line comment would suffice.
> +#define VIDEO_MODE_BURST_MODE_WITH_SYNC_PULSES 0
> +#define VIDEO_MODE_NON_BURST_MODE_WITH_SYNC_EVENTS BIT(0)
> +#define VIDEO_MODE_BURST_MODE BIT(1)
> +
> +/*
> + * DPI color coding
> + */
Ditto.
> +#define DPI_16_BIT_565_PACKED 0
> +#define DPI_16_BIT_565_ALIGNED 1
> +#define DPI_16_BIT_565_SHIFTED 2
> +#define DPI_18_BIT_PACKED 3
> +#define DPI_18_BIT_ALIGNED 4
> +#define DPI_24_BIT 5
> +
> +/*
> + * DPI Pixel format
> + */
Ditto.
> +#define PIXEL_FORMAT_16 0
> +#define PIXEL_FORMAT_18 BIT(0)
> +#define PIXEL_FORMAT_18L BIT(1)
> +#define PIXEL_FORMAT_24 (BIT(0) | BIT(1))
> +
> +enum transfer_direction { DSI_PACKET_SEND, DSI_PACKET_RECEIVE };
> +
> +struct mipi_dsi_transfer {
> + const struct mipi_dsi_msg *msg;
> + struct mipi_dsi_packet packet;
> + struct completion completed;
> +
> + int status; /* status of transmission */
> + enum transfer_direction direction;
> + bool need_bta;
> + u8 cmd;
> + u16 rx_word_count;
> + size_t tx_len; /* bytes sent */
> + size_t rx_len; /* bytes received */
> +};
The comments here are kind of obvious, so I would just remove them.
> +static inline int nwl_dsi_write(struct imx_nwl_dsi *dsi, unsigned int reg,
inline can be dropped.
> + u32 val)
> +{
> + int ret;
> +
> + ret = regmap_write(dsi->regmap, reg, val);
> + if (ret < 0)
> + DRM_DEV_ERROR(dsi->dev,
> + "Failed to write NWL DSI reg 0x%x: %d\n", reg,
> + ret);
> + return ret;
> +}
> +
> +static inline u32 nwl_dsi_read(struct imx_nwl_dsi *dsi, u32 reg)
Same here.
> +{
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(dsi->regmap, reg, &val);
> + if (ret < 0)
> + DRM_DEV_ERROR(dsi->dev, "Failed to read NWL DSI reg 0x%x: %d\n",
> + reg, ret);
> +
> + return val;
> +}
It seems that we could simply use regmap_read/write() directly instead
of these functions.
> +int nwl_dsi_get_dphy_params(struct imx_nwl_dsi *dsi,
> + const struct drm_display_mode *mode,
> + union phy_configure_opts *phy_opts)
> +{
> + unsigned long rate;
> +
> + if (dsi->lanes < 1 || dsi->lanes > 4)
> + return -EINVAL;
> +
> + /*
> + * So far the DPHY spec minimal timings work for both mixel
> + * dphy and nwl dsi host
> + */
> + phy_mipi_dphy_get_default_config(
> + mode->crtc_clock * 1000,
> + mipi_dsi_pixel_format_to_bpp(dsi->format), dsi->lanes,
> + &phy_opts->mipi_dphy);
> + rate = clk_get_rate(dsi->tx_esc_clk);
> + DRM_DEV_DEBUG_DRIVER(dsi->dev, "LP clk is @%lu Hz\n", rate);
> + phy_opts->mipi_dphy.lp_clk_rate = rate;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(nwl_dsi_get_dphy_params);
Does it really need to be exported? Why can't it be placed inside
nwl-drv.c and be made static?
> +/**
/* is enough
> + * ui2bc - UI time periods to byte clock cycles
> + */
> +static u32 ui2bc(struct imx_nwl_dsi *dsi, unsigned long long ui)
> +{
> + int bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +
> + return DIV_ROUND_UP(ui * dsi->lanes, dsi->vm.pixelclock * bpp);
> +}
> +
> +#define USEC_PER_SEC 1000000L
This definition already exists in include/linux/time64.h. No need to
redefine it.
> +static int nwl_dsi_enable_tx_clock(struct imx_nwl_dsi *dsi)
> +{
> + struct device *dev = dsi->dev;
> + int ret;
> +
> + ret = clk_prepare_enable(dsi->tx_esc_clk);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to enable tx_esc clk: %d\n", ret);
> + return ret;
> + }
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "Enabled tx_esc clk @%lu Hz\n",
> + clk_get_rate(dsi->tx_esc_clk));
> + return 0;
> +}
Do we really need this function? It looks like it would be simpler
just to call clk_prepare_enable() directly.
> +
> +static int nwl_dsi_enable_rx_clock(struct imx_nwl_dsi *dsi)
> +{
> + struct device *dev = dsi->dev;
> + int ret;
> +
> + ret = clk_prepare_enable(dsi->rx_esc_clk);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to enable rx_esc clk: %d\n", ret);
> + return ret;
> + }
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "Enabled rx_esc clk @%lu Hz\n",
> + clk_get_rate(dsi->rx_esc_clk));
> + return 0;
> +}
Same here.
> +static ssize_t nwl_dsi_host_transfer(struct mipi_dsi_host *dsi_host,
> + const struct mipi_dsi_msg *msg)
> +{
> + struct imx_nwl_dsi *dsi =
> + container_of(dsi_host, struct imx_nwl_dsi, dsi_host);
> + struct mipi_dsi_transfer xfer;
> + ssize_t ret = 0;
> +
> + /* Create packet to be sent */
> + dsi->xfer = &xfer;
> + ret = mipi_dsi_create_packet(&xfer.packet, msg);
> + if (ret < 0) {
> + dsi->xfer = NULL;
> + return ret;
> + }
> +
> + if ((msg->type & MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM ||
> + msg->type & MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM ||
> + msg->type & MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM ||
> + msg->type & MIPI_DSI_DCS_READ) &&
> + msg->rx_len > 0 && msg->rx_buf != NULL)
> + xfer.direction = DSI_PACKET_RECEIVE;
> + else
> + xfer.direction = DSI_PACKET_SEND;
> +
> + xfer.need_bta = (xfer.direction == DSI_PACKET_RECEIVE);
> + xfer.need_bta |= (msg->flags & MIPI_DSI_MSG_REQ_ACK) ? 1 : 0;
> + xfer.msg = msg;
> + xfer.status = -ETIMEDOUT;
> + xfer.rx_word_count = 0;
> + xfer.rx_len = 0;
> + xfer.cmd = 0x00;
> + if (msg->tx_len > 0)
> + xfer.cmd = ((u8 *)(msg->tx_buf))[0];
> + init_completion(&xfer.completed);
> +
> + nwl_dsi_enable_rx_clock(dsi);
This may fail, so better check the error.
ret = clk_prepare_enable()
if (ret < 0)
return ret;
> +irqreturn_t nwl_dsi_irq_handler(int irq, void *data)
> +{
> + u32 irq_status;
> + struct imx_nwl_dsi *dsi = data;
> +
> + irq_status = nwl_dsi_read(dsi, IRQ_STATUS);
> +
> + if (irq_status & TX_PKT_DONE || irq_status & RX_PKT_HDR_RCVD ||
> + irq_status & RX_PKT_PAYLOAD_DATA_RCVD)
> + nwl_dsi_finish_transmission(dsi, irq_status);
> +
> + return IRQ_HANDLED;
> +}
> +EXPORT_SYMBOL_GPL(nwl_dsi_irq_handler);
What about placing this function inside nwl-drv.c and make it static?
> +
> +int nwl_dsi_enable(struct imx_nwl_dsi *dsi)
> +{
> + struct device *dev = dsi->dev;
> + union phy_configure_opts *phy_cfg = &dsi->phy_cfg;
> + int ret;
> +
> + if (!dsi->lanes) {
> + DRM_DEV_ERROR(dev, "Need DSI lanes: %d\n", dsi->lanes);
> + return -EINVAL;
> + }
> +
> + ret = phy_init(dsi->phy);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to init DSI phy: %d\n", ret);
> + return ret;
> + }
> +
> + ret = phy_configure(dsi->phy, phy_cfg);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to configure DSI phy: %d\n", ret);
> + return ret;
> + }
> +
> + ret = nwl_dsi_enable_tx_clock(dsi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to enable tx clock: %d\n", ret);
> + return ret;
> + }
> +
> + ret = nwl_dsi_config_host(dsi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to set up DSI: %d", ret);
> + return ret;
> + }
> +
> + ret = nwl_dsi_config_dpi(dsi);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to set up DPI: %d", ret);
> + return ret;
> + }
> +
> + ret = phy_power_on(dsi->phy);
> + if (ret < 0) {
> + DRM_DEV_ERROR(dev, "Failed to power on DPHY (%d)\n", ret);
> + return ret;
> + }
> +
> + nwl_dsi_init_interrupts(dsi);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(nwl_dsi_enable);
Same here.
> +
> +int nwl_dsi_disable(struct imx_nwl_dsi *dsi)
> +{
> + struct device *dev = dsi->dev;
> +
> + DRM_DEV_DEBUG_DRIVER(dev, "Disabling clocks and phy\n");
> +
> + phy_power_off(dsi->phy);
> + phy_exit(dsi->phy);
> +
> + /* Disabling the clock before the phy breaks enabling dsi again */
> + clk_disable_unprepare(dsi->tx_esc_clk);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(nwl_dsi_disable);
Same here.
