Re: [PATCH 3/3] drm/bridge: Add NWL MIPI DSI host controller support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux