Hello Lucas,
On Fri, 26 Aug 2022 21:24:24 +0200
Lucas Stach <l.stach@xxxxxxxxxxxxxx> wrote:
> This IP block is found in the HDMI subsystem of the i.MX8MP SoC. It has a
> full timing generator and can switch between different video sources. On
> the i.MX8MP however the only supported source is the LCDIF.
Reading this sentence I had assumed that the i.MX8MP does only support
the LCDIF as an input to the PVI, but after having read the reference
manual it does not seem to have such a limitation. Do you mean that
"this driver only supports the LCDIF as an input"?
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx8mp-hdmi-pvi.c
> @@ -0,0 +1,201 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +/*
> + * Copyright (C) 2022 Pengutronix, Lucas Stach <kernel@xxxxxxxxxxxxxx>
> + */
> +
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_crtc.h>
> +#include <linux/module.h>
> +#include <linux/of_device.h>
> +#include <linux/of_graph.h>
> +#include <linux/pm_runtime.h>
> +
> +#define HTX_PVI_CTL 0x0
> +#define PVI_CTL_OP_VSYNC_POL BIT(18)
> +#define PVI_CTL_OP_HSYNC_POL BIT(17)
> +#define PVI_CTL_OP_DE_POL BIT(16)
> +#define PVI_CTL_INP_VSYNC_POL BIT(14)
> +#define PVI_CTL_INP_HSYNC_POL BIT(13)
> +#define PVI_CTL_INP_DE_POL BIT(12)
> +#define PVI_CTL_INPUT_LCDIF BIT(2)
According to the reference manual there is actually a 2-bit field here:
HTX_PVI_MOD, using bits 2:1, and whose "LCDIF" value is 0b10. Thus while
it obviously won't change the resulting code, it seems more correct to
define this as (2 << 1).
> +static void imx_hdmi_pvi_bridge_enable(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state)
> +{
> + struct drm_atomic_state *state = bridge_state->base.state;
> + struct imx_hdmi_pvi *pvi = to_imx_hdmi_pvi(bridge);
> + struct drm_connector_state *conn_state;
> + const struct drm_display_mode *mode;
> + struct drm_crtc_state *crtc_state;
> + struct drm_connector *connector;
> + u32 bus_flags, val;
> +
> + connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> + conn_state = drm_atomic_get_new_connector_state(state, connector);
> + crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> +
> + if (WARN_ON(pm_runtime_resume_and_get(pvi->dev)))
> + return;
> +
> + mode = &crtc_state->adjusted_mode;
> +
> + val = PVI_CTL_INPUT_LCDIF;
> +
> + if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> + val |= PVI_CTL_OP_VSYNC_POL | PVI_CTL_INP_VSYNC_POL;
> +
> + if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> + val |= PVI_CTL_OP_HSYNC_POL | PVI_CTL_INP_HSYNC_POL;
> +
> + if (pvi->next_bridge->timings)
> + bus_flags = pvi->next_bridge->timings->input_bus_flags;
> + else if (bridge_state)
> + bus_flags = bridge_state->input_bus_cfg.flags;
> +
> + if (bus_flags & DRM_BUS_FLAG_DE_HIGH)
> + val |= PVI_CTL_OP_DE_POL | PVI_CTL_INP_DE_POL;
> +
> + writel(val, pvi->regs + HTX_PVI_CTL);
> + val |= PVI_CTL_EN;
> + writel(val, pvi->regs + HTX_PVI_CTL);
I guess I'm missing something here: why can't one just write the
register once, with the enable bit set? I tried removing the first
writel() and everything seems to work just the same.
> +static void imx_hdmi_pvi_bridge_disable(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state)
> +{
> + struct imx_hdmi_pvi *pvi = to_imx_hdmi_pvi(bridge);
> +
> + writel(0x0, pvi->regs + HTX_PVI_CTL);
A very minor nit: why not simply writel(0, ...)?
With these fixed:
Reviewed-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
And definitely:
Tested-by: Luca Ceresoli <luca.ceresoli@xxxxxxxxxxx>
--
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
