вт, 3 лист. 2026 р. о 08:54 Mikko Perttunen <[email protected]> пише:
>
> On Friday, October 30, 2026 12:29 AM 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 the bus.
> >
> > Suggested-by: Mikko Perttunen <[email protected]> # DC command transfer
> > Signed-off-by: Svyatoslav Ryhel <[email protected]>
> > ---
> > drivers/gpu/drm/tegra/Makefile | 1 +
> > drivers/gpu/drm/tegra/dbi.c | 632 +++++++++++++++++++++++++++++++++
> > drivers/gpu/drm/tegra/dc.c | 4 +
> > drivers/gpu/drm/tegra/dc.h | 3 +
> > 4 files changed, 640 insertions(+)
> > create mode 100644 drivers/gpu/drm/tegra/dbi.c
> >
> > diff --git a/drivers/gpu/drm/tegra/Makefile b/drivers/gpu/drm/tegra/Makefile
> > index e399b40d64a1d..ae7fccb3d6421 100644
> > --- a/drivers/gpu/drm/tegra/Makefile
> > +++ b/drivers/gpu/drm/tegra/Makefile
> > @@ -18,6 +18,7 @@ tegra-drm-y := \
> > hdmi.o \
> > mipi-phy.o \
> > dsi.o \
> > + dbi.o \
> > sor.o \
> > dpaux.o \
> > gr2d.o \
> > diff --git a/drivers/gpu/drm/tegra/dbi.c b/drivers/gpu/drm/tegra/dbi.c
> > new file mode 100644
> > index 0000000000000..e129be0f21da5
> > --- /dev/null
> > +++ b/drivers/gpu/drm/tegra/dbi.c
> > @@ -0,0 +1,632 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nvidia Tegra20/30 MIPI-DBI Type B host driver
> > + *
> > + * Copyright (C) 2026 Svyatoslav Ryhel <[email protected]>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/cleanup.h>
> > +#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 <linux/string.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_mipi_dbi_bus.h>
> > +#include <drm/drm_of.h>
> > +
> > +#include <video/mipi_display.h>
> > +
> > +#include "dc.h"
> > +
> > +enum tegra_dbi_dc_signal {
> > + TEGRA_DBI_COMMAND,
> > + TEGRA_DBI_DATA
> > +};
> > +
> > +#define TEGRA_DBI_DATA_PINS_MAX 8
> > +#define TEGRA_DBI_INTERFACE_MAX 2
> > +
> > +struct reg_entry {
> > + u32 offset;
> > + u32 value;
> > +};
> > +
> > +static const 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 },
> > +};
>
> Instead of this table with unused repeated zero values, I think it would
> be cleaner to have just the register offsets:
>
> static const u32 tegra_dbi_pin_regs[] = {
> DC_COM_PIN_OUTPUT_ENABLE(0),
> // ...
> };
>
It has 2 components since I use memcpy in the probe, but I can switch
to using a loop to copy.
> > +
> > +struct tegra_dbi_priv {
> > + struct device *dev;
> > + struct tegra_dc *dc;
> > +
> > + struct mipi_dbi dbi;
> > + struct mipi_dbi_host dbi_host;
> > +
> > + struct drm_bridge bridge;
> > + struct drm_bridge *next_bridge;
> > +
> > + const struct tegra_dbi_interface *index;
>
> 'index' implies an integer, can you rename this to 'interface'.
>
Noted
> > + struct reg_entry com_pin_backup[ARRAY_SIZE(com_pin_output)];
> > +
> > + u32 bus_width;
> > + u32 reg;
> > + bool active;
> > +};
> > +
> > +struct pin {
> > + u32 reg;
> > + u32 pos;
> > +};
>
> Can you rename this to struct tegra_dbi_pin?
>
Sure
> > +
> > +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 = { DC_COM_PIN_OUTPUT_DATA(3), PIN_LDC },
> > + .rw = { DC_COM_PIN_OUTPUT_DATA(1), PIN_LSC0 },
> > + .cs = { DC_COM_PIN_OUTPUT_DATA(3), PIN_LCSN },
> > + .data = {
> > + { DC_COM_PIN_OUTPUT_DATA(0), PIN_LD0 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), PIN_LD1 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), PIN_LD2 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), PIN_LD3 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), PIN_LD4 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), PIN_LD5 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), PIN_LD6 },
> > + { DC_COM_PIN_OUTPUT_DATA(0), PIN_LD7 },
> > + },
> > + },
> > + /* Nvidia Secondary 8-bit CPU interface configuration */
> > + {
> > + .dc = { DC_COM_PIN_OUTPUT_DATA(2), PIN_LVP0 },
> > + .rw = { DC_COM_PIN_OUTPUT_DATA(1), PIN_LSC1 },
> > + .cs = { DC_COM_PIN_OUTPUT_DATA(2), PIN_LM0 },
> > + .data = {
> > + { DC_COM_PIN_OUTPUT_DATA(2), PIN_LHP1 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), PIN_LM1 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), PIN_LDI },
> > + { DC_COM_PIN_OUTPUT_DATA(2), PIN_LPP },
> > + { DC_COM_PIN_OUTPUT_DATA(1), PIN_LPW0 },
> > + { DC_COM_PIN_OUTPUT_DATA(1), PIN_LPW2 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), PIN_LVP1 },
> > + { DC_COM_PIN_OUTPUT_DATA(2), PIN_LHP0 },
> > + },
> > + },
> > +};
>
> If we include the information needed by init_primary/init_secondary in
> the tegra_dbi_interface tables as well, we can get rid of the 'reg'
> field.
>
Noted
> > +
> > +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 *dbi_host_to_tegra_dbi(struct
> > mipi_dbi_host *host)
> > +{
> > + return container_of(host, struct tegra_dbi_priv, dbi_host);
> > +}
> > +
> > +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,
> > bool value)
> > +{
> > + u32 val = BIT(pin->pos + 1); /* unmask is 1 bit above position */
> > +
> > + if (value)
> > + val |= BIT(pin->pos);
> > +
> > + tegra_dc_writel(dc, val, pin->reg);
> > +}
> > +
> > +static void tegra_dbi_write(struct tegra_dbi_priv *priv,
> > + enum tegra_dbi_dc_signal signal, u8 value)
> > +{
> > + struct tegra_dc *dc = priv->dc;
> > + const struct tegra_dbi_interface *idx = priv->index;
> > +
> > + tegra_dbi_pin_set(dc, &idx->dc, signal);
> > +
> > + 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], field_get(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 int tegra_dbi_host_attach(struct mipi_dbi_host *host, struct
> > mipi_dbi_device *dbidev)
> > +{
> > + struct tegra_dbi_priv *priv = dbi_host_to_tegra_dbi(host);
> > + struct device *dev = priv->dev;
> > + struct drm_bridge *bridge;
> > + struct device_node *ep;
> > + int ret;
> > +
> > + bridge = of_drm_get_bridge_by_endpoint(dev_of_node(dev), 1, -1);
> > + if (IS_ERR(bridge))
> > + return PTR_ERR(bridge);
> > +
> > + bridge->type = DRM_MODE_CONNECTOR_DPI;
> > + priv->next_bridge = bridge;
> > +
> > + /* get input ep (port0/endpoint0) */
> > + ret = -EINVAL;
> > + ep = of_graph_get_endpoint_by_regs(dev_of_node(dev), 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;
>
> I think this would look a bit cleaner like ..
>
> priv->bus_width = 24;
>
> ep = of_graph_get_endpoint_by_regs(dev_of_node(dev), 0, 0);
> if (ep) {
> of_property_read_u32(ep, "bus-width", &priv->bus_width);
> of_node_put(ep);
> }
>
Yes, I have already adjusted in this way locally for v6. Thanks.
> > +
> > + return devm_drm_bridge_add(dev, &priv->bridge);
> > +}
> > +
> > +static void tegra_dbi_host_detach(struct mipi_dbi_host *host, struct
> > mipi_dbi_device *dbidev)
> > +{
> > + struct tegra_dbi_priv *priv = dbi_host_to_tegra_dbi(host);
> > +
> > + drm_bridge_remove(&priv->bridge);
>
> The above calls devm_drm_bridge_add, so I believe drm_bridge_remove will
> happen automatically.
>
Yes, I have switched to devm_drm_bridge_add but forgot to remove the other part.
> > + drm_bridge_put(priv->next_bridge);
> > +}
> > +
> > +static const struct mipi_dbi_host_ops tegra_dbi_host_ops = {
> > + .attach = tegra_dbi_host_attach,
> > + .detach = tegra_dbi_host_detach,
> > +};
> > +
> > +static void tegra_dbi_init_primary_interface(struct tegra_dbi_priv *priv)
> > +{
> > + struct tegra_dc *dc = priv->dc;
> > + u32 value;
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_ENABLE(3));
> > + value &= ~(BIT(PIN_LCSN) | BIT(PIN_LDC));
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_ENABLE(3));
> > +
> > + /* Set LSC0/LCSN/LDC output polarity HIGH */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_POLARITY(1));
> > + value &= ~BIT(PIN_LSC0);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_POLARITY(1));
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_POLARITY(3));
> > + value &= ~(BIT(PIN_LCSN) | BIT(PIN_LDC));
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_POLARITY(3));
> > +
> > + value = PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LD0) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LD1) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LD2) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LD3) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LD4) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LD5) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LD6) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LD7);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(0));
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(3));
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LSC0);
> > + value |= PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LSC0);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(3));
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(6));
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LCSN);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LDC);
> > + value |= PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LCSN) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LDC);
> > + 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;
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_ENABLE(2));
> > + value &= ~(BIT(PIN_LDI) | BIT(PIN_LPP));
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_ENABLE(2));
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(3));
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LPW0);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LPW2);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LSC1);
> > + value |= PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LPW0) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LPW2) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LSC1);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(3));
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(4));
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LHP0);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LHP1);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LVP0);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LVP1);
> > + value |= PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LHP0) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LHP1) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LVP0) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LVP1);
> > + tegra_dc_writel(dc, value, DC_COM_PIN_OUTPUT_SELECT(4));
> > +
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(5));
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LM0);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LM1);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LDI);
> > + value &= ~PIN_OUTPUT_SELECT_MASK(PIN_OUTPUT_SELECT_LPP);
> > + value |= PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LM0) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LM1) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LDI) |
> > + PIN_OUTPUT_SELECT_OUT(PIN_OUTPUT_SELECT_LPP);
> > + 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 drm_bridge *next_bridge = priv->next_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);
> > +
> > + 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);
> > +
> > + /* Init cycle 1: WRITE_MEMORY_START, set LCD_PCLK and COMMAND */
>
> Extra space here before '/*'
>
Noted
> > + tegra_dc_writel(dc, MIPI_DCS_WRITE_MEMORY_START,
> > DC_DISP_SPI_INIT_SEQ_DATA_A);
> > + 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, BIT(12) | BIT(14), DC_DISP_SPI_INIT_SEQ_DATA_D);
> > +
> > + 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:
>
> Why the default case? My assumption would be that if the bus width is
> not something that's expected, that's an error.
>
Makes sense.
> > + 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++)
> > + priv->com_pin_backup[i].value = tegra_dc_readl(dc,
> > + priv->com_pin_backup[i].offset);
> > +
> > + /* Unshadow to set up command and data pins */
> > + tegra_dc_writel(dc, WRITE_MUX | READ_MUX, DC_CMD_STATE_ACCESS);
> > +
> > + if (priv->reg)
> > + tegra_dbi_init_secondary_interface(priv);
> > + else
> > + tegra_dbi_init_primary_interface(priv);
> > +
> > + priv->active = true;
> > +
> > + /*
> > + * DBI requires that the next bridge be enabled here in order to
> > + * send DCS commands, since the interface pins will subsequently be
> > + * reconfigured for video data.
> > + */
> > + if (next_bridge->funcs && next_bridge->funcs->atomic_enable)
> > + next_bridge->funcs->atomic_enable(next_bridge, state);
>
> Instead of hijacking the flow, could we move the panel init commands
> into the prepare callback, and deinit into the unprepare callback?
> Command mode would then stay active until this function (host-side
> atomic_enable) is called, and again become active at the end of
> atomic_disable. AFAICT that should allow for a normal lifecycle.
>
Current framework does this panel prepare() > bridge pre_enable() >
bridge enable() > panel enable(). Bridge must set up DBI before panel
is prep or enabled and then reconfigure to RGB afterward. Only
possible quirk to do so is to use prepare_prev_first flag in the
panel but this is not a panel issue this is a bridge quirk and it
should not be addressed in the panel making it potentially
incompatible with other DBI bridges.
> > +
> > + tegra_dbi_pin_set(dc, &priv->index->cs, 0);
> > +
> > + priv->active = false;
> > +
> > + tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> > +
> > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> > + tegra_dc_writel(dc, priv->com_pin_backup[i].value,
> > + priv->com_pin_backup[i].offset);
> > +
> > + tegra_dc_commit(dc);
> > +
> > + /* Set LDC pin to V Pulse 1 */
> > + value = tegra_dc_readl(dc, DC_COM_PIN_OUTPUT_SELECT(6));
> > + value |= PIN_OUTPUT_SELECT_F4(PIN_OUTPUT_SELECT_LDC);
> > + 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);
> > +}
> > +
> > +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 drm_bridge *next_bridge = priv->next_bridge;
> > + struct tegra_dc *dc = priv->dc;
> > +
> > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> > + priv->com_pin_backup[i].value = tegra_dc_readl(dc,
> > + priv->com_pin_backup[i].offset);
> > +
> > + tegra_dc_writel(dc, WRITE_MUX, DC_CMD_STATE_ACCESS);
> > +
> > + if (priv->reg)
> > + tegra_dbi_init_secondary_interface(priv);
> > + else
> > + tegra_dbi_init_primary_interface(priv);
> > +
> > + priv->active = true;
> > +
> > + /*
> > + * DBI requires that the next bridge be disabled here in order to
> > + * send DCS commands, since the interface pins will subsequently be
> > + * reconfigured for video data.
> > + */
> > + if (next_bridge->funcs && next_bridge->funcs->atomic_disable)
> > + next_bridge->funcs->atomic_disable(next_bridge, state);
> > +
> > + priv->active = false;
> > +
> > + tegra_dc_writel(dc, 0, DC_CMD_STATE_ACCESS);
> > +
> > + for (int i = 0; i < ARRAY_SIZE(com_pin_output); i++)
> > + tegra_dc_writel(dc, priv->com_pin_backup[i].value,
> > + priv->com_pin_backup[i].offset);
> > +
> > + tegra_dc_commit(dc);
> > +}
> > +
> > +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->next_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;
>
> Is there a source for the 800x800 limit?
>
This is more a sane assumption.
Tegra TRM and MIPI DBI Standard do not provide limitations, but a
480x800 panel would require approx 768 KB of on-die SRAM and
integrating it into display module would be too costly. If this is not
desirable, I can remove mode check.
> > +
> > + 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 device *dev = &pdev->dev;
> > + struct mipi_dbi_device *dbidev;
> > + struct tegra_dbi_priv *priv;
> > + struct device_node *np;
> > + 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 */
>
> The comment seems unnecessary now, as the code below will error out
> (correctly) if the property is not present.
>
Noted
> > + ret = device_property_read_u32(dev, "reg", &priv->reg);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "Failed to get reg\n");
> > +
> > + if (priv->reg != 0 && priv->reg != 1)
> > + return dev_err_probe(dev, -EINVAL, "Wrong reg value: %d\n",
> > priv->reg);
> > +
> > + priv->index = &tegra_dbi_idx[priv->reg];
> > + memcpy(priv->com_pin_backup, com_pin_output, sizeof(com_pin_output));
> > +
> > + /* Initialize MIPI DBI interface */
> > + mutex_init(&priv->dbi.cmdlock);
> > + priv->dbi.command = tegra_dbi_dbi_command;
> > +
> > + priv->dbi_host.dev = dev;
> > + priv->dbi_host.dbi = &priv->dbi;
> > + priv->dbi_host.ops = &tegra_dbi_host_ops;
> > +
> > + priv->bridge.of_node = dev_of_node(dev);
> > + priv->bridge.timings = &default_tegra_dbi_timings;
> > +
> > + np = of_graph_get_remote_node(dev_of_node(dev), 1, -1);
> > + if (!np)
> > + return -EINVAL;
> > +
> > + dbidev = devm_mipi_dbi_device_register(dev, &priv->dbi_host, np);
> > + of_node_put(np);
> > + if (IS_ERR(dbidev))
> > + return dev_err_probe(dev, PTR_ERR(dbidev),
> > + "Failed to register DBI device\n");
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver tegra_dbi_driver = {
> > + .driver.name = "tegra-dbi",
> > + .probe = tegra_dbi_probe,
> > +};
>
> I don't see the need for the separate driver and probe. You should be
> able to just use the tegra-dc platform device. If there's some issue,
> please point it out.
>
Ok, I will do my best to remove this.
> > +
> > +static void tegra_dbi_release(void *pdev)
> > +{
> > + platform_device_unregister(pdev);
> > + platform_driver_unregister(&tegra_dbi_driver);
> > +}
> > +
> > +int devm_tegra_dc_dbi_init(struct device *dev)
>
> No need to have the devm_ prefix, for driver internal initialization
> functions it's expected that they can set up devm actions on the device.
>
Ok, noted.
> > +{
> > + struct device_node *dbi_np __free(device_node) =
> > + of_get_available_child_by_name(dev_of_node(dev), "dbi");
> > + struct platform_device_info dbi_info = { 0 };
> > + struct platform_device *dbi;
> > + int ret;
> > +
> > + /* Missing node means that DBI is not used, continue */
> > + if (!dbi_np)
> > + return 0;
> > +
> > + ret = platform_driver_register(&tegra_dbi_driver);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "Failed to register DBI driver\n");
> > +
> > + dbi_info.parent = dev;
> > + dbi_info.name = "tegra-dbi";
> > + dbi_info.id = PLATFORM_DEVID_AUTO;
> > + dbi_info.fwnode = of_fwnode_handle(dbi_np);
> > + dbi_info.of_node_reused = true;
> > +
> > + dbi = platform_device_register_full(&dbi_info);
> > + if (IS_ERR(dbi)) {
> > + platform_driver_unregister(&tegra_dbi_driver);
> > + return dev_err_probe(dev, PTR_ERR(dbi),
> > + "Failed to create DBI device\n");
> > + }
> > +
> > + ret = devm_add_action_or_reset(dev, tegra_dbi_release, dbi);
> > + if (ret)
> > + return ret;
> > +
> > + return 0;
> > +}
> > diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
> > index 5bbf2c028edd8..768cf82776632 100644
> > --- a/drivers/gpu/drm/tegra/dc.c
> > +++ b/drivers/gpu/drm/tegra/dc.c
> > @@ -3382,6 +3382,10 @@ static int tegra_dc_probe(struct platform_device
> > *pdev)
> > }
> > }
> >
> > + err = devm_tegra_dc_dbi_init(&pdev->dev);
> > + if (err)
> > + return dev_err_probe(&pdev->dev, err, "failed to init DBI\n");
> > +
> > 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 2b87bb675d4cf..3df49422a2bd1 100644
> > --- a/drivers/gpu/drm/tegra/dc.h
> > +++ b/drivers/gpu/drm/tegra/dc.h
> > @@ -177,6 +177,9 @@ void tegra_dc_rgb_remove(struct tegra_dc *dc);
> > int tegra_dc_rgb_init(struct drm_device *drm, struct tegra_dc *dc);
> > int tegra_dc_rgb_exit(struct tegra_dc *dc);
> >
> > +/* from dbi.c */
> > +int devm_tegra_dc_dbi_init(struct device *dev);
> > +
> > #define DC_CMD_GENERAL_INCR_SYNCPT 0x000
> >
> > #define DC_CMD_GENERAL_INCR_SYNCPT_CNTRL 0x001
> > --
> > 2.53.0
> >
> >
>
>
>
>
