Re: [PATCH v4 7/7] drm/tegra: Add support for 8-bit CPU interface
From: Svyatoslav Ryhel
Date: Fri Oct 23 2026 - 04:27:22 EST
пт, 23 жовт. 2026 р. о 10:53 Mikko Perttunen <mperttunen@xxxxxxxxxx> пише:
>
> On Thursday, October 22, 2026 10:22 PM Svyatoslav Ryhel wrote:
> > The display controller in Tegra20/30 SoCs features an 8-bit SPI interface
> > that closely resembles the MIPI DBI Type B protocol and is referred to as
> > '8-bit CPU'. Each display controller provides two such interfaces, which
> > can be used to send MIPI DCS commands to initialize and control the panel
> > while image data is transmitted via 16/18/24-line RGB.
> >
> > The '8-bit CPU', although being the DC interface, is modeled using a
> > bridge framework for flexibility and convenience. This implementation does
> > not change the existing codebase besides registering an interface and can
> > be enabled/disabled without losing other Tegra DRM functionality.
> >
> > Signed-off-by: Svyatoslav Ryhel <clamor95@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/tegra/Kconfig | 7 +
> > drivers/gpu/drm/tegra/Makefile | 3 +
> > drivers/gpu/drm/tegra/dbi.c | 536 +++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/tegra/dc.c | 29 ++
> > drivers/gpu/drm/tegra/dc.h | 1 +
> > drivers/gpu/drm/tegra/trace.c | 3 +
> > 6 files changed, 579 insertions(+)
> > create mode 100644 drivers/gpu/drm/tegra/dbi.c
> >
> > diff --git a/drivers/gpu/drm/tegra/Kconfig b/drivers/gpu/drm/tegra/Kconfig
> > index 8a3b16aac5d68..ce3a424398713 100644
> > --- a/drivers/gpu/drm/tegra/Kconfig
> > +++ b/drivers/gpu/drm/tegra/Kconfig
> > @@ -30,6 +30,13 @@ config DRM_TEGRA
> >
> > if DRM_TEGRA
> >
> > +config DRM_TEGRA_DBI
> > + tristate "Enable MIPI DBI type B communication protocol for Tegra 20/30"
> > + help
> > + Tegra20 and Tegra30 feature 8 bit CPU driver panel control
> > + protocol (similar to MIPI DBI Type B). This option allows use
> > + it to set up and control compatible panel.
> > +
> > config DRM_TEGRA_DEBUG
> > bool "NVIDIA Tegra DRM debug support"
> > help
> > diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
> > index e399b40d64a1d..d67e53248a082 100644
> > --- a/drivers/gpu/drm/tegra/Makefile
> > +++ b/drivers/gpu/drm/tegra/Makefile
> > @@ -33,3 +33,6 @@ tegra-drm-y += trace.o
> > tegra-drm-$(CONFIG_DRM_FBDEV_EMULATION) += fbdev.o
> >
> > obj-$(CONFIG_DRM_TEGRA) += tegra-drm.o
> > +
> > +obj-$(CONFIG_DRM_TEGRA_DBI) += tegra-dbi.o
> > +tegra-dbi-y := dbi.o
> > diff --git a/drivers/gpu/drm/tegra/dbi.c b/drivers/gpu/drm/tegra/dbi.c
> > new file mode 100644
> > index 0000000000000..7bd90d32053f2
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tegra/dbi.c
> > @@ -0,0 +1,536 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/kernel.h>
> > +#include <linux/media-bus-format.h>
> > +#include <linux/module.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/property.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include <drm/drm_atomic_helper.h>
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_drv.h>
> > +#include <drm/drm_mipi_dbi.h>
> > +#include <drm/drm_of.h>
> > +#include <drm/drm_panel.h>
> > +
> > +#include <video/mipi_display.h>
> > +
> > +#include "dc.h"
> > +
> > +#define TEGRA_DBI_COMMAND 0
> > +#define TEGRA_DBI_DATA 1
>
> I'd use an enum for COMMAND/DATA -- as is it's not quite obvious these
> are alternative values. It would also give a proper typing for the
> parameter to dbi_write.
>
Noted
> > +#define TEGRA_DBI_DATA_PINS_MAX 8
> > +#define TEGRA_DBI_INTERFACE_MAX 2
> > +
> > +struct tegra_dbi_output {
> > + struct drm_panel *panel;
> > + struct drm_bridge *bridge;
> > +};
> > +
> > +struct tegra_dbi_priv {
> > + struct mipi_dbi dbi; /* must be first */
> > + struct device *dev;
> > + struct tegra_dc *dc;
> > +
> > + struct drm_bridge bridge;
> > + struct tegra_dbi_output output;
> > +
> > + const struct tegra_dbi_interface *index;
> > +
> > + u32 bus_width;
> > + u32 idx;
> > + bool active;
> > +};
> > +
> > +struct reg_entry {
> > + u32 offset;
> > + u32 value;
> > +};
> > +
> > +/*
> > + * This is used to backup and restore COM pin state during
> > + * tegra_dbi_atomic_enable/disable.
> > + */
> > +static struct reg_entry com_pin_output[] = {
> > + { DC_COM_PIN_OUTPUT_ENABLE(0), 0 },
> > + { DC_COM_PIN_OUTPUT_ENABLE(1), 0 },
> > + { DC_COM_PIN_OUTPUT_ENABLE(2), 0 },
> > + { DC_COM_PIN_OUTPUT_ENABLE(3), 0 },
> > + { DC_COM_PIN_OUTPUT_POLARITY(0), 0 },
> > + { DC_COM_PIN_OUTPUT_POLARITY(1), 0 },
> > + { DC_COM_PIN_OUTPUT_POLARITY(2), 0 },
> > + { DC_COM_PIN_OUTPUT_POLARITY(3), 0 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), 0 },
> > + { DC_COM_PIN_OUTPUT_DATA(1), 0 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), 0 },
> > + { DC_COM_PIN_OUTPUT_DATA(3), 0 },
> > + { DC_COM_PIN_OUTPUT_SELECT(0), 0 },
> > + { DC_COM_PIN_OUTPUT_SELECT(1), 0 },
> > + { DC_COM_PIN_OUTPUT_SELECT(2), 0 },
> > + { DC_COM_PIN_OUTPUT_SELECT(3), 0 },
> > + { DC_COM_PIN_OUTPUT_SELECT(4), 0 },
> > + { DC_COM_PIN_OUTPUT_SELECT(5), 0 },
> > + { DC_COM_PIN_OUTPUT_SELECT(6), 0 },
> > +};
> > +
> > +struct pin {
> > + u32 reg;
> > + u32 pos;
> > +};
> > +
> > +struct tegra_dbi_interface {
> > + struct pin dc;
> > + struct pin rw;
> > + struct pin cs;
> > + struct pin data[TEGRA_DBI_DATA_PINS_MAX];
> > +};
> > +
> > +static const struct tegra_dbi_interface tegra_dbi_idx[TEGRA_DBI_INTERFACE_MAX] = {
> > + /*
> > + * Nvidia Primary 8-bit CPU interface configuration:
> > + * DC = LCD_DC0/LDC; RW = LCD_PCLK/LSC0; CS = LCD_CS0_N/LCSN
> > + * DATA = LD0 - LD7
> > + */
> > + {
> > + .dc = { DC_COM_PIN_OUTPUT_DATA(3), 6 },
> > + .rw = { DC_COM_PIN_OUTPUT_DATA(1), 24 },
> > + .cs = { DC_COM_PIN_OUTPUT_DATA(3), 4 },
> > + .data = {
> > + { DC_COM_PIN_OUTPUT_DATA(0), 0 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), 2 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), 4 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), 6 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), 8 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), 10 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), 12 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), 14 },
>
> I'd like to see defines added for the bit offsets (in dc.h).
> Alternatively perhaps we could have an enum tegra_dc_pin with
> TEGRA_DC_PIN_... variants in the same order as the pins are in the
> various registers (I assume they are in the same order in all
> registers). Then we could refer to those enumeration values here
> and compute the register and bit from it dynamically.
>
That should be possible. I will include those in header rafactoring.
Thierry did not like my enums, so those will be definitions.
> > + },
> > + },
> > + /*
> > + * Nvidia Secondary 8-bit CPU interface configuration:
> > + * DC = LCD_DC1/LVP0; RW = LCD_WR_N/LSC1; CS = LCD_CS1_N/LM0
> > + * DATA0 = LCD_D18/LHP1 DATA4 = LCD_PWR0/LPW0
> > + * DATA1 = LCD_M1/LM1 DATA5 = LCD_PWR2/LPW2
> > + * DATA2 = LCD_D22/LDI DATA6 = LCD_D20/LVP1
> > + * DATA3 = LCD_D23/LPP DATA7 = LCD_D21/LHP0
> > + */
> > + {
> > + .dc = { DC_COM_PIN_OUTPUT_DATA(2), 8 },
> > + .rw = { DC_COM_PIN_OUTPUT_DATA(1), 26 },
> > + .cs = { DC_COM_PIN_OUTPUT_DATA(3), 16 },
> > + .data = {
> > + { DC_COM_PIN_OUTPUT_DATA(2), 2 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), 18 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), 20 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), 22 },
> > + { DC_COM_PIN_OUTPUT_DATA(1), 16 },
> > + { DC_COM_PIN_OUTPUT_DATA(1), 20 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), 10 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), 0 },
> > + },
> > + },
> > +};
> > +
> > +static inline struct tegra_dbi_priv *mipi_dbi_to_tegra_dbi(struct mipi_dbi *dbi)
> > +{
> > + return container_of(dbi, struct tegra_dbi_priv, dbi);
> > +}
> > +
> > +static inline struct tegra_dbi_priv *bridge_to_tegra_dbi(struct drm_bridge *bridge)
> > +{
> > + return container_of(bridge, struct tegra_dbi_priv, bridge);
> > +}
> > +
> > +static void tegra_dbi_pin_set(struct tegra_dc *dc, const struct pin *pin, int value)
>
> I would change this to 'bool value', and and have '!!(BIT(i) & value)'
> for more descriptive types.
>
I had similar idea but ended up making it look like gpio helper. Maybe
FIELD_GET macro would work here instead of !!
> > +{
> > + u32 val;
> > +
> > + val = tegra_dc_readl(dc, pin->reg);
>
> You don't need to read the register value -- the purpose of the unmask
> bit is that only the data bits that are unmasked in the write are
> modified.
>
I was cautious not to accidentally set wrong pins. I will test if
plain writing works and remove read if it is not needed.
> > + val |= BIT(pin->pos + 1); /* unmask is 1 bit above position */
> > + if (value)
> > + val |= BIT(pin->pos);
> > + else
> > + val &= ~BIT(pin->pos);
> > + tegra_dc_writel(dc, val, pin->reg);
> > +}
> > +
> > +static void tegra_dbi_write(struct tegra_dbi_priv *priv, u8 type, u8 value)
> > +{
> > + struct tegra_dc *dc = priv->dc;
> > + const struct tegra_dbi_interface *idx = priv->index;
> > +
> > + tegra_dbi_pin_set(dc, &idx->dc, type);
> > +
> > + tegra_dbi_pin_set(dc, &idx->cs, 0);
> > + tegra_dbi_pin_set(dc, &idx->rw, 0);
> > +
> > + for (int i = 0; i < TEGRA_DBI_DATA_PINS_MAX; i++)
> > + tegra_dbi_pin_set(dc, &idx->data[i], BIT(i) & value);
> > +
> > + tegra_dbi_pin_set(dc, &idx->cs, 1);
> > + tegra_dbi_pin_set(dc, &idx->rw, 1);
> > +
> > + udelay(10);
> > +}
> > +
> > +static int tegra_dbi_dbi_command(struct mipi_dbi *dbi, u8 *cmd, u8 *param, size_t num)
> > +{
> > + struct tegra_dbi_priv *priv = mipi_dbi_to_tegra_dbi(dbi);
> > + u8 command = *cmd;
> > +
> > + /* Blocks all commands when interface is not active */
> > + if (!priv->active)
> > + return -ENODEV;
> > +
> > + tegra_dbi_write(priv, TEGRA_DBI_COMMAND, command);
> > +
> > + for (int i = 0; i < num; i++)
> > + tegra_dbi_write(priv, TEGRA_DBI_DATA, param[i]);
> > +
> > + return 0;
> > +}
> > +
> > +static void tegra_dbi_init_primary_interface(struct tegra_dbi_priv *priv)
> > +{
> > + struct tegra_dc *dc = priv->dc;
> > + u32 value;
> > +
> > + /* Set LCSN and LDC to output enable */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_ENABLE(3));
> > + value &= ~(BIT(4) | BIT(6));
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_ENABLE(3));
>
> We should use defines, or something like the enum tegra_dc_pin thing to
> calculate the parameter to DC_COM_PIN_OUTPUT/SELECT and the bit offset.
>
Macro to make mask of the pin. BIT already does this, but I can wrap
it into smth. Since I will add pin definitions this will look like
BIT(PEN_LCSN) end so on. If you have preferred naming or ideas, let me
know.
> > +
> > + /* Set LSC0/LCSN/LDC output polarity HIGH */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_POLARITY(1));
> > + value &= ~BIT(24);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_POLARITY(1));
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_POLARITY(3));
> > + value &= ~(BIT(4) | BIT(6));
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_POLARITY(3));
> > +
> > + /* Select output for pins LD0 - LD7 */
> > + tegra_dc_writel(dc, 0x11111111, DC_COM_PIN_OUTPUT_SELECT(0));
> > +
> > + /* Select output for pins LSC0/LCSN/LDC */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(3));
> > + value &= ~0x70000;
>
> Please write this as (0x7 << 16) for clarity, similarly below.
>
I will convert this into macro.
> > + value |= BIT(16);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(3));
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(6));
> > + value &= ~0x7700;
> > + value |= BIT(12) | BIT(8);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(6));
> > +}
> > +
> > +static void tegra_dbi_init_secondary_interface(struct tegra_dbi_priv *priv)
> > +{
> > + struct tegra_dc *dc = priv->dc;
> > + u32 value;
> > +
> > + /* Set LPP/LDI to output enable */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_ENABLE(2));
> > + value &= ~(BIT(20) | BIT(22));
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_ENABLE(2));
> > +
> > + /* Select output for pins LSC1/LPW2/LPW0 */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(3));
> > + value &= ~0x700707;
> > + value |= BIT(20) | BIT(8) | BIT(0);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(3));
> > +
> > + /* Select output for pins LVP1/LVP0/LHP1/LHP0 */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(4));
> > + value &= ~0x770077;
> > + value |= BIT(20) | BIT(16) | BIT(4) | BIT(0);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(4));
> > +
> > + /* Select output for pins LPP/LDI/LM1/LM0 */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(5));
> > + value &= ~0x7777;
> > + value |= BIT(12) | BIT(8) | BIT(4) | BIT(0);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(5));
> > +}
> > +
> > +static void tegra_dbi_atomic_enable(struct drm_bridge *bridge,
> > + struct drm_atomic_commit *state)
> > +{
> > + struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge);
> > + struct tegra_dc *dc = priv->dc;
> > + u32 value;
> > +
> > + value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
> > + value &= ~DISP_CTRL_MODE_MASK;
> > + tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
> > +
> > + /* Disable all options except cursor if enabled */
> > + value = tegra_dc_readl(dc, DC_DISP_DISP_WIN_OPTIONS);
> > + value &= CURSOR_ENABLE;
> > + tegra_dc_writel(dc, value, DC_DISP_DISP_WIN_OPTIONS);
>
> Just 'tegra_dc_writel(dc, CURSOR_ENABLE, DC_DISP_DISP_WIN_OPTIONS);'
>
If CURSOR_ENABLE is not set, it will be enabled unconditionally. Not good.
> > +
> > + tegra_dc_commit(dc);
> > +
> > + tegra_dc_writel(dc, V_PULSE1_ENABLE, DC_DISP_DISP_SIGNAL_OPTIONS0);
> > + tegra_dc_writel(dc, PULSE_POLARITY_LOW, DC_DISP_V_PULSE1_CONTROL);
> > +
> > + tegra_dc_writel(dc, PULSE_END(1), DC_DISP_V_PULSE1_POSITION_A);
> > + tegra_dc_writel(dc, 0, DC_DISP_V_PULSE1_POSITION_B);
> > + tegra_dc_writel(dc, 0, DC_DISP_V_PULSE1_POSITION_C);
> > +
> > + value = 1 << FRAME_INIT_SEQ_CYCLES_SHIFT |
> > + DC_SIGNAL_VPULSE1 << INIT_SEQ_DC_SIGNAL_SHIFT |
> > + INIT_SEQUENCE_MODE_PLCD | SEND_INIT_SEQUENCE;
> > + tegra_dc_writel(dc, value, DC_DISP_INIT_SEQ_CONTROL);
> > +
> > + /* Generic sequence for DBI command panels */
> > + tegra_dc_writel(dc, 0x2c, DC_DISP_SPI_INIT_SEQ_DATA_A);
>
> MIPI_DCS_WRITE_MEMORY_START
>
Noted
> > + tegra_dc_writel(dc, 0x0, DC_DISP_SPI_INIT_SEQ_DATA_B);
> > + tegra_dc_writel(dc, 0x0, DC_DISP_SPI_INIT_SEQ_DATA_C);
> > + tegra_dc_writel(dc, 0x5000, DC_DISP_SPI_INIT_SEQ_DATA_D);
>
> (1 << 12) | (1 << 14), and comment above
>
> /* Init cycle 1: WRITE_MEMORY_START, set LCD_PCLK and COMMAND */
>
> or something in that vein.
>
Noted
> > +
> > + value = SC1_H_QUALIFIER(SC_H_QUALIFIER_NONE) |
> > + SC0_V_QUALIFIER(SC_V_QUALIFIER_VACTIVE) |
> > + SC0_H_QUALIFIER(SC_H_QUALIFIER_HACTIVE);
> > + tegra_dc_writel(dc, value, DC_DISP_SHIFT_CLOCK_OPTIONS);
> > +
> > + value = tegra_dc_readl(dc, DC_DISP_DISP_COLOR_CONTROL);
> > + value &= ~BASE_COLOR_SIZE_MASK;
> > +
> > + switch (priv->bus_width) {
> > + case 16:
> > + value |= BASE_COLOR_SIZE_565;
> > + break;
> > + case 18:
> > + value |= BASE_COLOR_SIZE_666;
> > + break;
> > + case 24:
> > + default:
> > + value |= BASE_COLOR_SIZE_888;
> > + break;
> > + }
> > +
> > + tegra_dc_writel(dc, value, DC_DISP_DISP_COLOR_CONTROL);
> > +
> > + /*
> > + * Backup registers before unshadowing since actual register values
> > + * may differ and their state backed up now is the correct one.
> > + */
> > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> > + com_pin_output[i].value = tegra_dc_readl(dc, com_pin_output[i].offset);
> > +
> > + /* Unshadow to set up command and data pins */
> > + tegra_dc_writel(dc, WRITE_MUX, DC_CMD_STATE_ACCESS);
> > +
> > + if (priv->idx)
> > + tegra_dbi_init_secondary_interface(priv);
> > + else
> > + tegra_dbi_init_primary_interface(priv);
> > +
> > + priv->active = true;
> > +
> > + if (priv->output.panel)
> > + drm_panel_enable(priv->output.panel);
> > +
> > + tegra_dbi_pin_set(dc, &priv->index->cs, 0);
> > +
> > + priv->active = false;
> > +
> > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> > + tegra_dc_writel(dc, com_pin_output[i].value, com_pin_output[i].offset);
> > +
> > + /* Set LDC pin to V Pulse 1 */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(6));
> > + value |= LDC_OUTPUT_SELECT_V_PULSE1;
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(6));
> > +
> > + value = tegra_dc_readl(dc, DC_CMD_DISPLAY_COMMAND);
> > + value &= ~DISP_CTRL_MODE_MASK;
> > + value |= DISP_CTRL_MODE_C_DISPLAY;
> > + tegra_dc_writel(dc, value, DC_CMD_DISPLAY_COMMAND);
> > +
> > + tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> > +}
> > +
> > +static void tegra_dbi_atomic_disable(struct drm_bridge *bridge,
> > + struct drm_atomic_commit *state)
> > +{
> > + struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge);
> > + struct tegra_dc *dc = priv->dc;
> > +
> > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> > + com_pin_output[i].value = tegra_dc_readl(dc, com_pin_output[i].offset);
> > +
> > + tegra_dc_writel(dc, WRITE_MUX, DC_CMD_STATE_ACCESS);
> > +
> > + if (priv->idx)
> > + tegra_dbi_init_secondary_interface(priv);
> > + else
> > + tegra_dbi_init_primary_interface(priv);
> > +
> > + priv->active = true;
> > +
> > + if (priv->output.panel)
> > + drm_panel_disable(priv->output.panel);
> > +
> > + priv->active = false;
> > +
> > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> > + tegra_dc_writel(dc, com_pin_output[i].value, com_pin_output[i].offset);
> > +
> > + tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> > +}
> > +
> > +static int tegra_dbi_atomic_check(struct drm_bridge *bridge,
> > + struct drm_bridge_state *bridge_state,
> > + struct drm_crtc_state *crtc_state,
> > + struct drm_connector_state *conn_state)
> > +{
> > + struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
> > +
> > + /* Set safe default bus flags if panel does not specify them */
> > + if (!bridge_state->input_bus_cfg.flags)
> > + bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> > +
> > + /* Default to positive sync */
> > + if (!(adjusted_mode->flags &
> > + (DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NHSYNC)))
> > + adjusted_mode->flags |= DRM_MODE_FLAG_PHSYNC;
> > +
> > + if (!(adjusted_mode->flags &
> > + (DRM_MODE_FLAG_PVSYNC | DRM_MODE_FLAG_NVSYNC)))
> > + adjusted_mode->flags |= DRM_MODE_FLAG_PVSYNC;
> > +
> > + return 0;
> > +}
> > +
> > +static int tegra_dbi_attach(struct drm_bridge *bridge,
> > + struct drm_encoder *encoder,
> > + enum drm_bridge_attach_flags flags)
> > +{
> > + struct tegra_dbi_priv *priv = bridge_to_tegra_dbi(bridge);
> > + struct device *dev = priv->dev;
> > +
> > + priv->dc = dev_get_drvdata(dev->parent);
> > + if (!priv->dc)
> > + return -ENODEV;
> > +
> > + return drm_bridge_attach(bridge->encoder, priv->output.bridge, bridge,
> > + flags);
> > +}
> > +
> > +static enum drm_mode_status tegra_dbi_mode_valid(struct drm_bridge *bridge,
> > + const struct drm_display_info *info,
> > + const struct drm_display_mode *mode)
> > +{
> > + if (mode->hdisplay > 800)
> > + return MODE_H_ILLEGAL;
> > +
> > + if (mode->vdisplay > 800)
> > + return MODE_V_ILLEGAL;
> > +
> > + return MODE_OK;
> > +}
> > +
> > +static const struct drm_bridge_funcs tegra_dbi_funcs = {
> > + .attach = tegra_dbi_attach,
> > + .mode_valid = tegra_dbi_mode_valid,
> > +
> > + .atomic_enable = tegra_dbi_atomic_enable,
> > + .atomic_disable = tegra_dbi_atomic_disable,
> > + .atomic_check = tegra_dbi_atomic_check,
> > +
> > + .atomic_create_state = drm_atomic_helper_bridge_create_state,
> > + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > +};
> > +
> > +static const struct drm_bridge_timings default_tegra_dbi_timings = {
> > + .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE |
> > + DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
> > + DRM_BUS_FLAG_DE_HIGH,
> > +};
> > +
> > +static int tegra_dbi_probe(struct platform_device *pdev)
> > +{
> > + struct tegra_dbi_priv *priv;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *np = dev->of_node;
> > + struct drm_bridge *bridge;
> > + struct drm_panel *panel;
> > + struct device_node *ep;
> > + int ret;
> > +
> > + priv = devm_drm_bridge_alloc(dev, struct tegra_dbi_priv, bridge,
> > + &tegra_dbi_funcs);
> > + if (IS_ERR(priv))
> > + return PTR_ERR(priv);
> > +
> > + priv->dev = dev;
> > +
> > + /* Default to primaty */
>
> primary
>
> > + device_property_read_u32(dev, "nvidia,index", &priv->idx);
>
> Could we use the standard 'reg' property instead of this?
>
OMG, YES!
> > + if (priv->idx)
> > + priv->index = &tegra_dbi_idx[1];
> > + else
> > + priv->index = &tegra_dbi_idx[0];
> > +
> > + /* Initialize MIPI DBI interface */
> > + mutex_init(&priv->dbi.cmdlock);
> > + priv->dbi.command = tegra_dbi_dbi_command;
> > +
> > + ret = devm_of_platform_populate(dev);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to probe children\n");
> > +
> > + ret = drm_of_find_panel_or_bridge(np, 1, 0, &panel, &bridge);
> > + if (ret)
> > + return ret;
> > +
> > + if (panel) {
> > + bridge = devm_drm_panel_bridge_add_typed(dev, panel,
> > + DRM_MODE_CONNECTOR_DPI);
> > + if (IS_ERR(bridge))
> > + return PTR_ERR(bridge);
> > + }
> > +
> > + priv->output.bridge = bridge;
> > + priv->output.panel = panel;
> > +
> > + /* get input ep (port0/endpoint0) */
> > + ret = -EINVAL;
> > + ep = of_graph_get_endpoint_by_regs(np, 0, 0);
> > + if (ep) {
> > + ret = of_property_read_u32(ep, "bus-width", &priv->bus_width);
> > + of_node_put(ep);
> > + }
> > +
> > + if (ret)
> > + priv->bus_width = 24;
> > +
> > + priv->bridge.of_node = np;
> > + priv->bridge.timings = &default_tegra_dbi_timings;
> > +
> > + ret = devm_drm_bridge_add(dev, &priv->bridge);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to add bridge\n");
> > +
> > + platform_set_drvdata(pdev, priv);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver tegra_dbi_driver = {
> > + .driver.name = "tegra-dbi",
> > + .probe = tegra_dbi_probe,
> > +};
> > +module_platform_driver(tegra_dbi_driver);
> > +
> > +MODULE_ALIAS("platform:tegra-dbi");
> > +MODULE_AUTHOR("Svyatoslav Ryhel <clamor95@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Nvidia Tegra20/30 MIPI-DBI Type B driver");
> > +MODULE_LICENSE("GPL");
>
> No separate platform driver or device. If a separate struct device is
> needed for some technical purpose, we need to create a minimal separate
> virtual device, but unless there is a strong reason to have that we
> should be fine with just the dc device.
>
> Thank you
> Mikko
>
ATM, I use bridge framework to describe interface and I am attaching
it to RGB since it basically uses RGB to transfer video data. It works
pretty nice so far.
You propose to replicate how RGB works just for DBI?
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index 5c67928bcabfa..8f12d30cd2d8b 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -223,6 +223,7 @@ void tegra_dc_commit(struct tegra_dc *dc)
> > tegra_dc_writel(dc, GENERAL_ACT_REQ << 8, DC_CMD_STATE_CONTROL);
> > tegra_dc_writel(dc, GENERAL_ACT_REQ, DC_CMD_STATE_CONTROL);
> > }
> > +EXPORT_SYMBOL_GPL(tegra_dc_commit);
> >
> > static inline u32 compute_dda_inc(unsigned int in, unsigned int out, bool v,
> > unsigned int bpp)
> > @@ -3279,9 +3280,15 @@ static int tegra_dc_init_opp_table(struct tegra_dc *dc)
> > return 0;
> > }
> >
> > +static void tegra_dc_release_dbi_pdev(void *pdev)
> > +{
> > + platform_device_unregister(pdev);
> > +}
> > +
> > static int tegra_dc_probe(struct platform_device *pdev)
> > {
> > u64 dma_mask = dma_get_mask(pdev->dev.parent);
> > + struct device_node *dbi_np;
> > struct tegra_dc *dc;
> > int err;
> >
> > @@ -3382,6 +3389,28 @@ static int tegra_dc_probe(struct platform_device *pdev)
> > }
> > }
> >
> > + dbi_np = of_get_available_child_by_name(pdev->dev.of_node, "dbi");
> > + if (dbi_np) {
> > + struct platform_device_info dbi_info = {
> > + .parent = &pdev->dev,
> > + .name = "tegra-dbi",
> > + .id = PLATFORM_DEVID_AUTO,
> > + .fwnode = of_fwnode_handle(dbi_np),
> > + .of_node_reused = true,
> > + };
> > +
> > + dc->dbi = platform_device_register_full(&dbi_info);
> > + of_node_put(dbi_np);
> > + if (IS_ERR(dc->dbi))
> > + return dev_err_probe(&pdev->dev, PTR_ERR(dc->dbi),
> > + "Failed to create DBI device\n");
> > +
> > + err = devm_add_action_or_reset(&pdev->dev, tegra_dc_release_dbi_pdev,
> > + dc->dbi);
> > + if (err)
> > + return err;
> > + }
> > +
> > err = tegra_dc_rgb_probe(dc);
> > if (err < 0 && err != -ENODEV)
> > return dev_err_probe(&pdev->dev, err,
> > diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
> > index 7005df32f485f..27dbee57f8f72 100644
> > --- a/drivers/gpu/drm/tegra/dc.h
> > +++ b/drivers/gpu/drm/tegra/dc.h
> > @@ -95,6 +95,7 @@ struct tegra_dc {
> >
> > struct tegra_output *rgb;
> > struct tegra_pmc *pmc;
> > + struct platform_device *dbi;
> >
> > struct tegra_dc_stats stats;
> > struct list_head list;
> > diff --git a/drivers/gpu/drm/tegra/trace.c b/drivers/gpu/drm/tegra/trace.c
> > index 006f65c72a344..3aeb3825e2100 100644
> > --- a/drivers/gpu/drm/tegra/trace.c
> > +++ b/drivers/gpu/drm/tegra/trace.c
> > @@ -1,2 +1,5 @@
> > #define CREATE_TRACE_POINTS
> > #include "trace.h"
> > +
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(dc_readl);
> > +EXPORT_TRACEPOINT_SYMBOL_GPL(dc_writel);
> > --
> > 2.53.0
> >
> >
>
>
>
>