Skip to contentSkip to conversation

DPU PATCH 0/4] Add suppport for sn65dsi86 bridge chip and Innolux 2k edp panel driver

RFC In review Sandeep Panda 6 participants 18 messages Apr 18, 2018 – Apr 26, 2018 4 messages not in the archive

Subsystems (7)
Device tree bindings DRM drivers DRM misc Arm Qualcomm mailing list +3 more subsystems
lore
Share & tools
lore

Sandeep Panda

author
#0 lore raw

Changelog:

v3 -> v4:
Current patchset separates out eDP panel specific resources from sn65dsi86 bridge driver and creates a separate driver for the innloux edp panel.

Now bridge driver check if any panel is attached or not to get the supported modes. If any panel is attached then query from panel driver to get the modes, or else probe the provided i2c adapter to read the modes from EDID.

Removed hardcoding of bridge init sequence. With this patchset bridge driver now will program the init sequence based on the current mode set by drm framework.

Current patchset is not tested on actual bridge chip/panel.

Sandeep Panda (4):
drm/bridge: add support for sn65dsi86 bridge driver
dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings drm/panel: add Innolux TV123WAM eDP panel driver
dt-bindings: Add Innolux TV123WAM panel bindings

 .../bindings/display/bridge/ti,sn65dsi86.txt       |  60 ++
 .../display/panel/innolux,edp-2k-panel.txt         |  21 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c              | 742 +++++++++++++++++++++
 drivers/gpu/drm/panel/panel-innolux-tv123wam.c     | 299 +++++++++
 4 files changed, 1122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
 create mode 100644 Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
 create mode 100644 drivers/gpu/drm/panel/panel-innolux-tv123wam.c
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

A message that is not in the archive (sent off-list or not yet received): 1524054002-17869-1-git-send-email-spanda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org

author
#2 lore raw

RFC 1/1 DPU PATCH 1/4] drm/bridge: add support for sn65dsi86 bridge driver

Add support for TI's sn65dsi86 dsi2edp bridge chip. The chip converts DSI transmitted signal to eDP signal, which is fed to the connected eDP panel.

This chip can be controlled via either i2c interface or dsi interface. Currently in driver all the control registers are being accessed through i2c interface only. Also as of now HPD support has not been added to bridge chip driver.

Signed-off-by Sandeep Panda <[email protected]>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 742 ++++++++++++++++++++++++++++++++++
 1 file changed, 742 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +742 -0
new file mode 100644
index 0000000..c798f2f
@@ -0,0 +1,742 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#define SN_BRIDGE_REVISION_ID 0x2
+
+/* Link Training specific registers */
+#define SN_DEVICE_REV_REG			0x08
+#define SN_REFCLK_FREQ_REG			0x0A
+#define SN_DSI_LANES_REG			0x10
+#define SN_DSIA_CLK_FREQ_REG			0x12
+#define SN_ENH_FRAME_REG			0x5A
+#define SN_SSC_CONFIG_REG			0x93
+#define SN_DATARATE_CONFIG_REG			0x94
+#define SN_PLL_ENABLE_REG			0x0D
+#define SN_SCRAMBLE_CONFIG_REG			0x95
+#define SN_AUX_WDATA0_REG			0x64
+#define SN_AUX_ADDR_19_16_REG			0x74
+#define SN_AUX_ADDR_15_8_REG			0x75
+#define SN_AUX_ADDR_7_0_REG			0x76
+#define SN_AUX_LENGTH_REG			0x77
+#define SN_AUX_CMD_REG				0x78
+#define SN_ML_TX_MODE_REG			0x96
+#define SN_PAGE_SELECT_REG			0xFF
+#define SN_AUX_RDATA0_REG			0x79
+/* video config specific registers */
+#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG	0x20
+#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG	0x21
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG	0x24
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG	0x25
+#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG	0x2C
+#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG	0x2D
+#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG	0x30
+#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG	0x31
+#define SN_CHA_HORIZONTAL_BACK_PORCH_REG	0x34
+#define SN_CHA_VERTICAL_BACK_PORCH_REG		0x36
+#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG	0x38
+#define SN_CHA_VERTICAL_FRONT_PORCH_REG		0x3A
+#define SN_DATA_FORMAT_REG			0x5B
+#define SN_COLOR_BAR_CONFIG_REG			0x3C
+
+#define DPPLL_CLK_SRC_REFCLK	0
+#define DPPLL_CLK_SRC_DSICLK	1
+#define MIN_DSI_CLK_FREQ_MHZ	40
+
+enum ti_sn_bridge_ref_clks {
+	SN_REF_CLK_12_MHZ = 0,
+	SN_REF_CLK_19_2_MHZ,
+	SN_REF_CLK_26_MHZ,
+	SN_REF_CLK_27_MHZ,
+	SN_REF_CLK_38_4_MHZ,
+	SN_REF_CLK_MAX,
+};
+
+struct ti_sn_bridge {
+	struct device *dev;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
+	/* handle to the connected eDP panel */
+	struct drm_panel *panel;
+	int irq;
+	struct gpio_desc *irq_gpio;
+	/* list of gpios needed to enable the bridge functionality */
+	struct gpio_descs *enable_gpios;
+	unsigned int num_supplies;
+	struct regulator_bulk_data *supplies;
+	struct i2c_client *i2c_client;
+	struct i2c_adapter *ddc;
+	bool power_on;
+	u32 num_modes;
+	struct drm_display_mode curr_mode;
+};
+
+static int ti_sn_bridge_write(struct ti_sn_bridge *pdata, u8 reg, u8 val)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	u8 buf[2] = {reg, val};
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = 0,
+		.len = 2,
+		.buf = buf,
+	};
+
+	if (i2c_transfer(client->adapter, &msg, 1) < 1) {
+		DRM_ERROR("i2c write failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ti_sn_bridge_read(struct ti_sn_bridge *pdata,
+					u8 reg, char *buf, u32 size)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	struct i2c_msg msg[2] = {
+		{
+			.addr = client->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = &reg,
+		},
+		{
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buf,
+		}
+	};
+
+	if (i2c_transfer(client->adapter, msg, 2) != 2) {
+		DRM_ERROR("i2c read failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ti_sn_bridge_gpio_configure(struct ti_sn_bridge *pdata, bool on)
+{
+	int value, i = 0, num = 0;
+	int *value_array = NULL;
+
+	if (!pdata)
+		return;
+
+	value = on ? 1 : 0;
+	num = pdata->enable_gpios->ndescs;
+	value_array = kmalloc(num * sizeof(int), GFP_KERNEL);
+	if (value_array) {
+		for (i = 0; i < num; i++)
+			value_array[i] = value;
+		gpiod_set_array_value(num, pdata->enable_gpios->desc,
+				      value_array);
+		kfree(value_array);
+	}
+
+	if (pdata->irq_gpio)
+		gpiod_set_value(pdata->irq_gpio, value);
+
+	DRM_DEBUG("config to: %d\n", value);
+}
+
+static void ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool enable)
+{
+	static int ref_count;
+
+	if (!pdata)
+		return;
+
+	if (enable)
+		ref_count++;
+	else
+		ref_count--;
+
+	if (enable && (ref_count == 1)) {
+		ti_sn_bridge_gpio_configure(pdata, true);
+
+		if (regulator_bulk_enable(pdata->num_supplies,
+						pdata->supplies)) {
+			DRM_ERROR("bridge regulator enable failed\n");
+			return;
+		}
+		pdata->power_on = true;
+	} else if (!enable && !ref_count) {
+		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
+
+		ti_sn_bridge_gpio_configure(pdata, false);
+		pdata->power_on = false;
+	} else {
+		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
+							enable, ref_count);
+	}
+}
+
+/* Connector funcs */
+static struct ti_sn_bridge *
+connector_to_ti_sn_bridge(struct drm_connector *connector)
+{
+	return container_of(connector, struct ti_sn_bridge, connector);
+}
+
+static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+	struct drm_panel *panel = pdata->panel;
+	struct edid *edid;
+
+	if (panel) {
+		DRM_DEBUG("get mode from connected drm_panel\n");
+		pdata->num_modes = drm_panel_get_modes(panel);
+	} else {
+		/* get from EDID */
+		if (!pdata->ddc)
+			return 0;
+		ti_sn_bridge_power_ctrl(pdata, true);
+		edid = drm_get_edid(connector, pdata->ddc);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			pdata->num_modes = drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			kfree(edid);
+		} else {
+			DRM_DEBUG("failed to get edid\n");
+		}
+		ti_sn_bridge_power_ctrl(pdata, false);
+	}
+
+	return pdata->num_modes;
+}
+
+static enum drm_mode_status
+ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	/* maximum supported resolution is 4K at 60 fps */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
+	.get_modes = ti_sn_bridge_connector_get_modes,
+	.mode_valid = ti_sn_bridge_connector_mode_valid,
+};
+
+static enum drm_connector_status
+ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+
+	/**
+	 * TODO: Currently if drm_panel is present, then always
+	 * return the status as connected. Need to add support to detect
+	 * device state for no panel(hot pluggable) scenarios.
+	 */
+	if (pdata->panel)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+}
+
+static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = ti_sn_bridge_connector_detect,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct ti_sn_bridge, bridge);
+}
+
+static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
+{
+	u8 rev = 0;
+	int ret = 0;
+
+	ret = ti_sn_bridge_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
+	if (ret)
+		goto exit;
+
+	if (rev == SN_BRIDGE_REVISION_ID) {
+		DRM_DEBUG("ti_sn_bridge revision id: 0x%x\n", rev);
+	} else {
+		DRM_ERROR("ti_sn_bridge revision id mismatch\n");
+		ret = -EINVAL;
+	}
+
+exit:
+	return ret;
+}
+
+static const char * const ti_sn_bridge_supply_names[] = {
+	"vccio",
+	"vcca",
+};
+
+static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
+{
+	unsigned int i;
+	int ret;
+
+	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
+
+	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
+				     sizeof(*pdata->supplies), GFP_KERNEL);
+	if (!pdata->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < pdata->num_supplies; i++)
+		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
+
+	ret = devm_regulator_bulk_get(pdata->dev,
+			pdata->num_supplies, pdata->supplies);
+
+	return ret;
+}
+
+static void ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
+{
+	struct device_node *panel_node, *port, *endpoint;
+	struct drm_panel *panel = NULL;
+
+	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
+	if (port) {
+		endpoint = of_get_child_by_name(port, "endpoint");
+		of_node_put(port);
+		if (!endpoint) {
+			dev_err(pdata->dev, "no output endpoint found\n");
+			goto error;
+		}
+
+		panel_node = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+		if (!panel_node) {
+			dev_err(pdata->dev, "no output node found\n");
+			goto error;
+		}
+
+		panel = of_drm_find_panel(panel_node);
+		of_node_put(panel_node);
+		if (!panel) {
+			dev_err(pdata->dev, "no panel node found\n");
+			goto error;
+		}
+	}
+
+	pdata->panel = panel;
+	drm_panel_attach(panel, &pdata->connector);
+
+	return;
+
+error:
+	pdata->panel = NULL;
+}
+
+static int ti_sn_bridge_attach(struct drm_bridge *bridge)
+{
+	struct mipi_dsi_host *host;
+	struct mipi_dsi_device *dsi;
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
+						   .channel = 0,
+						   .node = NULL,
+						 };
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	/* HPD not supported */
+	pdata->connector.polled = 0;
+
+	ret = drm_connector_init(bridge->dev, &pdata->connector,
+				 &ti_sn_bridge_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&pdata->connector,
+				 &ti_sn_bridge_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&pdata->connector, bridge->encoder);
+
+	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+	if (!host) {
+		DRM_ERROR("failed to find dsi host\n");
+		return -ENODEV;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		DRM_ERROR("failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		goto err_dsi_device;
+	}
+
+	/* TODO: setting to 4 lanes always for now */
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_ERROR("failed to attach dsi to host\n");
+		goto err_dsi_attach;
+	}
+
+	pdata->dsi = dsi;
+
+	DRM_DEBUG("bridge attached\n");
+	/* attach panel to bridge */
+	ti_sn_bridge_attach_panel(pdata);
+
+	return 0;
+
+err_dsi_attach:
+	mipi_dsi_device_unregister(dsi);
+err_dsi_device:
+	return ret;
+}
+
+static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj_mode)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
+		adj_mode->hdisplay, adj_mode->vdisplay,
+		adj_mode->vrefresh, adj_mode->clock);
+
+	drm_mode_copy(&pdata->curr_mode, adj_mode);
+}
+
+static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+
+	if (panel) {
+		drm_panel_disable(panel);
+		drm_panel_unprepare(panel);
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, false);
+}
+
+static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+	struct drm_display_mode *mode = &pdata->curr_mode;
+	u32 bit_rate_mhz, clk_freq_mhz;
+	u8 val = 0;
+
+	if (!pdata->num_modes)
+		return;
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+
+	if (panel)
+		drm_panel_prepare(panel);
+
+	/* CLK_SRC config */
+	ti_sn_bridge_write(pdata, SN_REFCLK_FREQ_REG,
+			(DPPLL_CLK_SRC_REFCLK | (SN_REF_CLK_19_2_MHZ << 0x1)));
+
+	/* DSI_A lane config */
+	ti_sn_bridge_read(pdata, SN_DSI_LANES_REG, &val, 0x1);
+	val |= ((0x4 - pdata->dsi->lanes) & 0x03) << 0x3;
+	ti_sn_bridge_write(pdata, SN_DSI_LANES_REG, val);
+
+	/* DP lane config */
+	ti_sn_bridge_read(pdata, SN_SSC_CONFIG_REG, &val, 0x1);
+	val |= ((pdata->dsi->lanes - 0x1) & 0x03) << 0x4;
+	ti_sn_bridge_write(pdata, SN_SSC_CONFIG_REG, val);
+
+	/* set dsi clk frequency value */
+	bit_rate_mhz = (mode->clock / 1000) *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+	/* for each increment in val, frequency increases by 5MHz */
+	val = (MIN_DSI_CLK_FREQ_MHZ / 0x5) +
+		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 0x5) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_DSIA_CLK_FREQ_REG, val);
+
+	ti_sn_bridge_write(pdata, SN_DATARATE_CONFIG_REG, 0x80); /* set HBR */
+
+	ti_sn_bridge_write(pdata, SN_PLL_ENABLE_REG, 0x1); /* Enable PLL */
+	usleep_range(10000, 10001);
+
+	/* DPCD Register 0x0010A in Sink */
+	ti_sn_bridge_write(pdata, SN_AUX_WDATA0_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_19_16_REG, 0x00);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_15_8_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_7_0_REG, 0x0A);
+	ti_sn_bridge_write(pdata, SN_AUX_LENGTH_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_CMD_REG, 0x81);
+	usleep_range(10000, 10001);
+
+	/* Semi auto link training mode */
+	ti_sn_bridge_write(pdata, SN_ML_TX_MODE_REG, 0x0A);
+	usleep_range(20000, 20001);
+
+	/* config video parameters */
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
+			mode->hdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
+			mode->vdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->htotal - mode->hsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->htotal - mode->hsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->vtotal - mode->vsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->vtotal - mode->vsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
+			(mode->hsync_start - mode->hdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_BACK_PORCH_REG,
+			(mode->vsync_start - mode->vdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
+			(mode->hsync_end - mode->hsync_start) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_FRONT_PORCH_REG,
+			(mode->vsync_end - mode->vsync_start) & 0xFF);
+	if (pdata->dsi->format == MIPI_DSI_FMT_RGB888)
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x00);
+	else
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x01);
+	usleep_range(10000, 10001);
+
+	/* enable video stream */
+	ti_sn_bridge_read(pdata, SN_ENH_FRAME_REG, &val, 0x1);
+	val |= BIT(3);
+	ti_sn_bridge_write(pdata, SN_ENH_FRAME_REG, val);
+
+	if (panel)
+		drm_panel_enable(panel);
+}
+
+static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
+	.attach = ti_sn_bridge_attach,
+	.enable = ti_sn_bridge_enable,
+	.disable = ti_sn_bridge_disable,
+	.mode_set = ti_sn_bridge_mode_set,
+};
+
+static int ti_sn_bridge_parse_gpios(struct ti_sn_bridge *pdata)
+{
+	int ret = 0;
+
+	if (!pdata || !pdata->dev)
+		return -EINVAL;
+
+	pdata->enable_gpios = devm_gpiod_get_array(pdata->dev,
+						"enable", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->enable_gpios)) {
+		DRM_ERROR("failed to get enable gpio from DT\n");
+		ret = PTR_ERR(pdata->enable_gpios);
+		goto exit;
+	}
+
+	pdata->irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->irq_gpio))
+		DRM_ERROR("failed to get irq gpio from DT\n");
+
+exit:
+	return ret;
+}
+
+static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
+{
+	struct device_node *np = pdata->dev->of_node;
+	struct device_node *end_node;
+
+	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
+	if (!end_node) {
+		DRM_ERROR("remote endpoint not found\n");
+		return -ENODEV;
+	}
+
+	pdata->host_node = of_graph_get_remote_port_parent(end_node);
+	of_node_put(end_node);
+	if (!pdata->host_node) {
+		DRM_ERROR("remote node not found\n");
+		return -ENODEV;
+	}
+	of_node_put(pdata->host_node);
+
+	return 0;
+}
+
+static int ti_sn_bridge_probe(struct i2c_client *client,
+	 const struct i2c_device_id *id)
+{
+	struct ti_sn_bridge *pdata;
+	struct device_node *ddc_node;
+	int ret = 0;
+
+	if (!client || !client->dev.of_node) {
+		DRM_ERROR("invalid input\n");
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		DRM_ERROR("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	pdata = devm_kzalloc(&client->dev,
+		sizeof(struct ti_sn_bridge), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->power_on = false;
+	pdata->dev = &client->dev;
+	pdata->i2c_client = client;
+	DRM_DEBUG("I2C address is %x\n", client->addr);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse device tree\n");
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_gpios(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse gpios from DT\n");
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_regulators(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse regulators\n");
+		goto err_dt_parse;
+	}
+
+	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+		if (!pdata->ddc) {
+			dev_dbg(pdata->dev, "failed to read ddc node\n");
+			return -EPROBE_DEFER;
+		}
+	} else {
+		dev_dbg(pdata->dev, "no ddc property found\n");
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+	ret = ti_sn_bridge_read_device_rev(pdata);
+	if (ret) {
+		DRM_ERROR("failed to read chip rev\n");
+		goto err_config;
+	} else {
+		DRM_DEBUG("bridge chip enabled successfully\n");
+	}
+	ti_sn_bridge_power_ctrl(pdata, false);
+
+	i2c_set_clientdata(client, pdata);
+	dev_set_drvdata(&client->dev, pdata);
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = client->dev.of_node;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return ret;
+
+err_config:
+	ti_sn_bridge_power_ctrl(pdata, false);
+err_dt_parse:
+	devm_kfree(&client->dev, pdata);
+	return ret;
+}
+
+static int ti_sn_bridge_remove(struct i2c_client *client)
+{
+	int ret = -EINVAL;
+	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
+
+	if (!pdata)
+		goto end;
+
+	mipi_dsi_detach(pdata->dsi);
+	mipi_dsi_device_unregister(pdata->dsi);
+
+	drm_bridge_remove(&pdata->bridge);
+
+	disable_irq(pdata->irq);
+	free_irq(pdata->irq, pdata);
+
+	ti_sn_bridge_gpio_configure(pdata, false);
+	i2c_put_adapter(pdata->ddc);
+
+	devm_kfree(&client->dev, pdata);
+
+end:
+	return ret;
+}
+
+static struct i2c_device_id ti_sn_bridge_id[] = {
+	{ "ti,sn65dsi86", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+
+static const struct of_device_id ti_sn_bridge_match_table[] = {
+	{.compatible = "ti,sn65dsi86"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
+
+static struct i2c_driver ti_sn_bridge_driver = {
+	.driver = {
+		.name = "ti_sn65dsi86",
+		.owner = THIS_MODULE,
+		.of_match_table = ti_sn_bridge_match_table,
+	},
+	.probe = ti_sn_bridge_probe,
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id,
+};
+
+module_i2c_driver(ti_sn_bridge_driver);
+MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
#3 lore raw

Re: [[RFC]DPU PATCH 1/4] drm/bridge: add support for sn65dsi86 bridge driver

Quoted 32 lines · Sandeep Panda

Add support for TI's sn65dsi86 dsi2edp bridge chip. The chip converts DSI transmitted signal to eDP signal, which is fed to the connected eDP panel.

This chip can be controlled via either i2c interface or dsi interface. Currently in driver all the control registers are being accessed through i2c interface only. Also as of now HPD support has not been added to bridge chip driver.

Signed-off-by Sandeep Panda <[email protected]>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 742 ++++++++++++++++++++++++++++++++++
 1 file changed, 742 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
new file mode 100644
index 0000000..c798f2f
@@ -0,0 +1,742 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 20

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>

I have a hard time believing that you need all of these includes. Off the top of my head, you could probably remove types, kernel, init, platform_device, delay, drmP, drm_atomic, drm_crtc_helper. You could probably trim it even further with the help of your compiler. These should also be in alphabetical order.

Quoted 53 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +53 -0
+
+#define SN_BRIDGE_REVISION_ID 0x2
+
+/* Link Training specific registers */
+#define SN_DEVICE_REV_REG			0x08
+#define SN_REFCLK_FREQ_REG			0x0A
+#define SN_DSI_LANES_REG			0x10
+#define SN_DSIA_CLK_FREQ_REG			0x12
+#define SN_ENH_FRAME_REG			0x5A
+#define SN_SSC_CONFIG_REG			0x93
+#define SN_DATARATE_CONFIG_REG			0x94
+#define SN_PLL_ENABLE_REG			0x0D
+#define SN_SCRAMBLE_CONFIG_REG			0x95
+#define SN_AUX_WDATA0_REG			0x64
+#define SN_AUX_ADDR_19_16_REG			0x74
+#define SN_AUX_ADDR_15_8_REG			0x75
+#define SN_AUX_ADDR_7_0_REG			0x76
+#define SN_AUX_LENGTH_REG			0x77
+#define SN_AUX_CMD_REG				0x78
+#define SN_ML_TX_MODE_REG			0x96
+#define SN_PAGE_SELECT_REG			0xFF
+#define SN_AUX_RDATA0_REG			0x79
+/* video config specific registers */
+#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG	0x20
+#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG	0x21
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG	0x24
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG	0x25
+#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG	0x2C
+#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG	0x2D
+#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG	0x30
+#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG	0x31
+#define SN_CHA_HORIZONTAL_BACK_PORCH_REG	0x34
+#define SN_CHA_VERTICAL_BACK_PORCH_REG		0x36
+#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG	0x38
+#define SN_CHA_VERTICAL_FRONT_PORCH_REG		0x3A
+#define SN_DATA_FORMAT_REG			0x5B
+#define SN_COLOR_BAR_CONFIG_REG			0x3C
+
+#define DPPLL_CLK_SRC_REFCLK	0
+#define DPPLL_CLK_SRC_DSICLK	1
+#define MIN_DSI_CLK_FREQ_MHZ	40
+
+enum ti_sn_bridge_ref_clks {
+	SN_REF_CLK_12_MHZ = 0,
+	SN_REF_CLK_19_2_MHZ,
+	SN_REF_CLK_26_MHZ,
+	SN_REF_CLK_27_MHZ,
+	SN_REF_CLK_38_4_MHZ,
+	SN_REF_CLK_MAX,
+};
+
+struct ti_sn_bridge {
+	struct device *dev;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 83

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
+	/* handle to the connected eDP panel */
+	struct drm_panel *panel;
+	int irq;
+	struct gpio_desc *irq_gpio;
+	/* list of gpios needed to enable the bridge functionality */
+	struct gpio_descs *enable_gpios;

Why are the enable gpios a list?

Quoted 460 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +460 -0
+	unsigned int num_supplies;
+	struct regulator_bulk_data *supplies;
+	struct i2c_client *i2c_client;
+	struct i2c_adapter *ddc;
+	bool power_on;
+	u32 num_modes;
+	struct drm_display_mode curr_mode;
+};
+
+static int ti_sn_bridge_write(struct ti_sn_bridge *pdata, u8 reg, u8 val)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	u8 buf[2] = {reg, val};
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = 0,
+		.len = 2,
+		.buf = buf,
+	};
+
+	if (i2c_transfer(client->adapter, &msg, 1) < 1) {
+		DRM_ERROR("i2c write failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ti_sn_bridge_read(struct ti_sn_bridge *pdata,
+					u8 reg, char *buf, u32 size)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	struct i2c_msg msg[2] = {
+		{
+			.addr = client->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = &reg,
+		},
+		{
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buf,
+		}
+	};
+
+	if (i2c_transfer(client->adapter, msg, 2) != 2) {
+		DRM_ERROR("i2c read failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ti_sn_bridge_gpio_configure(struct ti_sn_bridge *pdata, bool on)
+{
+	int value, i = 0, num = 0;
+	int *value_array = NULL;
+
+	if (!pdata)
+		return;
+
+	value = on ? 1 : 0;
+	num = pdata->enable_gpios->ndescs;
+	value_array = kmalloc(num * sizeof(int), GFP_KERNEL);
+	if (value_array) {
+		for (i = 0; i < num; i++)
+			value_array[i] = value;
+		gpiod_set_array_value(num, pdata->enable_gpios->desc,
+				      value_array);
+		kfree(value_array);
+	}
+
+	if (pdata->irq_gpio)
+		gpiod_set_value(pdata->irq_gpio, value);
+
+	DRM_DEBUG("config to: %d\n", value);
+}
+
+static void ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool enable)
+{
+	static int ref_count;
+
+	if (!pdata)
+		return;
+
+	if (enable)
+		ref_count++;
+	else
+		ref_count--;
+
+	if (enable && (ref_count == 1)) {
+		ti_sn_bridge_gpio_configure(pdata, true);
+
+		if (regulator_bulk_enable(pdata->num_supplies,
+						pdata->supplies)) {
+			DRM_ERROR("bridge regulator enable failed\n");
+			return;
+		}
+		pdata->power_on = true;
+	} else if (!enable && !ref_count) {
+		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
+
+		ti_sn_bridge_gpio_configure(pdata, false);
+		pdata->power_on = false;
+	} else {
+		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
+							enable, ref_count);
+	}
+}
+
+/* Connector funcs */
+static struct ti_sn_bridge *
+connector_to_ti_sn_bridge(struct drm_connector *connector)
+{
+	return container_of(connector, struct ti_sn_bridge, connector);
+}
+
+static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+	struct drm_panel *panel = pdata->panel;
+	struct edid *edid;
+
+	if (panel) {
+		DRM_DEBUG("get mode from connected drm_panel\n");
+		pdata->num_modes = drm_panel_get_modes(panel);
+	} else {
+		/* get from EDID */
+		if (!pdata->ddc)
+			return 0;
+		ti_sn_bridge_power_ctrl(pdata, true);
+		edid = drm_get_edid(connector, pdata->ddc);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			pdata->num_modes = drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			kfree(edid);
+		} else {
+			DRM_DEBUG("failed to get edid\n");
+		}
+		ti_sn_bridge_power_ctrl(pdata, false);
+	}
+
+	return pdata->num_modes;
+}
+
+static enum drm_mode_status
+ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	/* maximum supported resolution is 4K at 60 fps */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
+	.get_modes = ti_sn_bridge_connector_get_modes,
+	.mode_valid = ti_sn_bridge_connector_mode_valid,
+};
+
+static enum drm_connector_status
+ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+
+	/**
+	 * TODO: Currently if drm_panel is present, then always
+	 * return the status as connected. Need to add support to detect
+	 * device state for no panel(hot pluggable) scenarios.
+	 */
+	if (pdata->panel)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+}
+
+static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = ti_sn_bridge_connector_detect,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct ti_sn_bridge, bridge);
+}
+
+static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
+{
+	u8 rev = 0;
+	int ret = 0;
+
+	ret = ti_sn_bridge_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
+	if (ret)
+		goto exit;
+
+	if (rev == SN_BRIDGE_REVISION_ID) {
+		DRM_DEBUG("ti_sn_bridge revision id: 0x%x\n", rev);
+	} else {
+		DRM_ERROR("ti_sn_bridge revision id mismatch\n");
+		ret = -EINVAL;
+	}
+
+exit:
+	return ret;
+}
+
+static const char * const ti_sn_bridge_supply_names[] = {
+	"vccio",
+	"vcca",
+};
+
+static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
+{
+	unsigned int i;
+	int ret;
+
+	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
+
+	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
+				     sizeof(*pdata->supplies), GFP_KERNEL);
+	if (!pdata->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < pdata->num_supplies; i++)
+		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
+
+	ret = devm_regulator_bulk_get(pdata->dev,
+			pdata->num_supplies, pdata->supplies);
+
+	return ret;
+}
+
+static void ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
+{
+	struct device_node *panel_node, *port, *endpoint;
+	struct drm_panel *panel = NULL;
+
+	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
+	if (port) {
+		endpoint = of_get_child_by_name(port, "endpoint");
+		of_node_put(port);
+		if (!endpoint) {
+			dev_err(pdata->dev, "no output endpoint found\n");
+			goto error;
+		}
+
+		panel_node = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+		if (!panel_node) {
+			dev_err(pdata->dev, "no output node found\n");
+			goto error;
+		}
+
+		panel = of_drm_find_panel(panel_node);
+		of_node_put(panel_node);
+		if (!panel) {
+			dev_err(pdata->dev, "no panel node found\n");
+			goto error;
+		}
+	}
+
+	pdata->panel = panel;
+	drm_panel_attach(panel, &pdata->connector);
+
+	return;
+
+error:
+	pdata->panel = NULL;
+}
+
+static int ti_sn_bridge_attach(struct drm_bridge *bridge)
+{
+	struct mipi_dsi_host *host;
+	struct mipi_dsi_device *dsi;
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
+						   .channel = 0,
+						   .node = NULL,
+						 };
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	/* HPD not supported */
+	pdata->connector.polled = 0;
+
+	ret = drm_connector_init(bridge->dev, &pdata->connector,
+				 &ti_sn_bridge_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&pdata->connector,
+				 &ti_sn_bridge_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&pdata->connector, bridge->encoder);
+
+	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+	if (!host) {
+		DRM_ERROR("failed to find dsi host\n");
+		return -ENODEV;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		DRM_ERROR("failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		goto err_dsi_device;
+	}
+
+	/* TODO: setting to 4 lanes always for now */
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_ERROR("failed to attach dsi to host\n");
+		goto err_dsi_attach;
+	}
+
+	pdata->dsi = dsi;
+
+	DRM_DEBUG("bridge attached\n");
+	/* attach panel to bridge */
+	ti_sn_bridge_attach_panel(pdata);
+
+	return 0;
+
+err_dsi_attach:
+	mipi_dsi_device_unregister(dsi);
+err_dsi_device:
+	return ret;
+}
+
+static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj_mode)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
+		adj_mode->hdisplay, adj_mode->vdisplay,
+		adj_mode->vrefresh, adj_mode->clock);
+
+	drm_mode_copy(&pdata->curr_mode, adj_mode);
+}
+
+static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+
+	if (panel) {
+		drm_panel_disable(panel);
+		drm_panel_unprepare(panel);
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, false);
+}
+
+static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+	struct drm_display_mode *mode = &pdata->curr_mode;
+	u32 bit_rate_mhz, clk_freq_mhz;
+	u8 val = 0;
+
+	if (!pdata->num_modes)
+		return;
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+
+	if (panel)
+		drm_panel_prepare(panel);
+
+	/* CLK_SRC config */
+	ti_sn_bridge_write(pdata, SN_REFCLK_FREQ_REG,
+			(DPPLL_CLK_SRC_REFCLK | (SN_REF_CLK_19_2_MHZ << 0x1)));
+
+	/* DSI_A lane config */
+	ti_sn_bridge_read(pdata, SN_DSI_LANES_REG, &val, 0x1);
+	val |= ((0x4 - pdata->dsi->lanes) & 0x03) << 0x3;
+	ti_sn_bridge_write(pdata, SN_DSI_LANES_REG, val);
+
+	/* DP lane config */
+	ti_sn_bridge_read(pdata, SN_SSC_CONFIG_REG, &val, 0x1);
+	val |= ((pdata->dsi->lanes - 0x1) & 0x03) << 0x4;
+	ti_sn_bridge_write(pdata, SN_SSC_CONFIG_REG, val);
+
+	/* set dsi clk frequency value */
+	bit_rate_mhz = (mode->clock / 1000) *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+	/* for each increment in val, frequency increases by 5MHz */
+	val = (MIN_DSI_CLK_FREQ_MHZ / 0x5) +
+		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 0x5) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_DSIA_CLK_FREQ_REG, val);
+
+	ti_sn_bridge_write(pdata, SN_DATARATE_CONFIG_REG, 0x80); /* set HBR */
+
+	ti_sn_bridge_write(pdata, SN_PLL_ENABLE_REG, 0x1); /* Enable PLL */
+	usleep_range(10000, 10001);
+
+	/* DPCD Register 0x0010A in Sink */
+	ti_sn_bridge_write(pdata, SN_AUX_WDATA0_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_19_16_REG, 0x00);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_15_8_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_7_0_REG, 0x0A);
+	ti_sn_bridge_write(pdata, SN_AUX_LENGTH_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_CMD_REG, 0x81);
+	usleep_range(10000, 10001);
+
+	/* Semi auto link training mode */
+	ti_sn_bridge_write(pdata, SN_ML_TX_MODE_REG, 0x0A);
+	usleep_range(20000, 20001);
+
+	/* config video parameters */
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
+			mode->hdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
+			mode->vdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->htotal - mode->hsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->htotal - mode->hsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->vtotal - mode->vsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->vtotal - mode->vsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
+			(mode->hsync_start - mode->hdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_BACK_PORCH_REG,
+			(mode->vsync_start - mode->vdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
+			(mode->hsync_end - mode->hsync_start) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_FRONT_PORCH_REG,
+			(mode->vsync_end - mode->vsync_start) & 0xFF);
+	if (pdata->dsi->format == MIPI_DSI_FMT_RGB888)
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x00);
+	else

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 553

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x01);
+	usleep_range(10000, 10001);
+
+	/* enable video stream */
+	ti_sn_bridge_read(pdata, SN_ENH_FRAME_REG, &val, 0x1);
+	val |= BIT(3);
+	ti_sn_bridge_write(pdata, SN_ENH_FRAME_REG, val);
+
+	if (panel)
+		drm_panel_enable(panel);

As mentioned in the previous review, you should configure the mode in mode_set, not enable. This eliminates the need for curr_mode.

Quoted 144 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +144 -0
+}
+
+static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
+	.attach = ti_sn_bridge_attach,
+	.enable = ti_sn_bridge_enable,
+	.disable = ti_sn_bridge_disable,
+	.mode_set = ti_sn_bridge_mode_set,
+};
+
+static int ti_sn_bridge_parse_gpios(struct ti_sn_bridge *pdata)
+{
+	int ret = 0;
+
+	if (!pdata || !pdata->dev)
+		return -EINVAL;
+
+	pdata->enable_gpios = devm_gpiod_get_array(pdata->dev,
+						"enable", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->enable_gpios)) {
+		DRM_ERROR("failed to get enable gpio from DT\n");
+		ret = PTR_ERR(pdata->enable_gpios);
+		goto exit;
+	}
+
+	pdata->irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->irq_gpio))
+		DRM_ERROR("failed to get irq gpio from DT\n");
+
+exit:
+	return ret;
+}
+
+static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
+{
+	struct device_node *np = pdata->dev->of_node;
+	struct device_node *end_node;
+
+	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
+	if (!end_node) {
+		DRM_ERROR("remote endpoint not found\n");
+		return -ENODEV;
+	}
+
+	pdata->host_node = of_graph_get_remote_port_parent(end_node);
+	of_node_put(end_node);
+	if (!pdata->host_node) {
+		DRM_ERROR("remote node not found\n");
+		return -ENODEV;
+	}
+	of_node_put(pdata->host_node);
+
+	return 0;
+}
+
+static int ti_sn_bridge_probe(struct i2c_client *client,
+	 const struct i2c_device_id *id)
+{
+	struct ti_sn_bridge *pdata;
+	struct device_node *ddc_node;
+	int ret = 0;
+
+	if (!client || !client->dev.of_node) {
+		DRM_ERROR("invalid input\n");
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		DRM_ERROR("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	pdata = devm_kzalloc(&client->dev,
+		sizeof(struct ti_sn_bridge), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->power_on = false;
+	pdata->dev = &client->dev;
+	pdata->i2c_client = client;
+	DRM_DEBUG("I2C address is %x\n", client->addr);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse device tree\n");
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_gpios(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse gpios from DT\n");
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_regulators(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse regulators\n");
+		goto err_dt_parse;
+	}
+
+	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+		if (!pdata->ddc) {
+			dev_dbg(pdata->dev, "failed to read ddc node\n");
+			return -EPROBE_DEFER;
+		}
+	} else {
+		dev_dbg(pdata->dev, "no ddc property found\n");
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+	ret = ti_sn_bridge_read_device_rev(pdata);
+	if (ret) {
+		DRM_ERROR("failed to read chip rev\n");
+		goto err_config;
+	} else {
+		DRM_DEBUG("bridge chip enabled successfully\n");
+	}
+	ti_sn_bridge_power_ctrl(pdata, false);
+
+	i2c_set_clientdata(client, pdata);
+	dev_set_drvdata(&client->dev, pdata);
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = client->dev.of_node;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return ret;
+
+err_config:
+	ti_sn_bridge_power_ctrl(pdata, false);
+err_dt_parse:
+	devm_kfree(&client->dev, pdata);
+	return ret;
+}
+
+static int ti_sn_bridge_remove(struct i2c_client *client)
+{
+	int ret = -EINVAL;
+	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
+

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 707

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	if (!pdata)
+		goto end;
+
+	mipi_dsi_detach(pdata->dsi);
+	mipi_dsi_device_unregister(pdata->dsi);
+
+	drm_bridge_remove(&pdata->bridge);
+
+	disable_irq(pdata->irq);
+	free_irq(pdata->irq, pdata);

This is uninitialized.

Quoted 39 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +35 -0
+
+	ti_sn_bridge_gpio_configure(pdata, false);
+	i2c_put_adapter(pdata->ddc);
+
+	devm_kfree(&client->dev, pdata);
+
+end:
+	return ret;
+}
+
+static struct i2c_device_id ti_sn_bridge_id[] = {
+	{ "ti,sn65dsi86", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+
+static const struct of_device_id ti_sn_bridge_match_table[] = {
+	{.compatible = "ti,sn65dsi86"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
+
+static struct i2c_driver ti_sn_bridge_driver = {
+	.driver = {
+		.name = "ti_sn65dsi86",
+		.owner = THIS_MODULE,
+		.of_match_table = ti_sn_bridge_match_table,
+	},
+	.probe = ti_sn_bridge_probe,
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id,
+};
+
+module_i2c_driver(ti_sn_bridge_driver);
+MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#4 lore raw

Re: [[RFC]DPU PATCH 1/4] drm/bridge: add support for sn65dsi86 bridge driver

Quoted 41 lines · Sean Paul
Quoted 40 lines · Sandeep Panda

Add support for TI's sn65dsi86 dsi2edp bridge chip. The chip converts DSI transmitted signal to eDP signal, which is fed to the connected eDP panel.

This chip can be controlled via either i2c interface or dsi interface. Currently in driver all the control registers are being accessed through i2c interface only. Also as of now HPD support has not been added to bridge chip driver.

Signed-off-by Sandeep Panda <[email protected]>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 742 
++++++++++++++++++++++++++++++++++
 1 file changed, 742 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +17 -0
new file mode 100644
index 0000000..c798f2f
@@ -0,0 +1,742 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
Quoted 9 lines

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 20

Quoted 3 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +3 -0
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>

I have a hard time believing that you need all of these includes. Off the top of my head, you could probably remove types, kernel, init, platform_device, delay, drmP, drm_atomic, drm_crtc_helper. You could probably trim it even further with the help of your compiler. These should also be in alphabetical order.

I will take care of alphabetical order and generic kernel header file inclusion. But the header files of drmP, drm_atomic, drm_crtc_helper are required here.

drmP, drm_atomic, drm_crtc_helper all these are needed because of the drm api that we using in this driver for modes, connectors and bridges. the drm header files i have already checked with compiler.

Quoted 544 lines
Quoted 53 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +53 -0
+
+#define SN_BRIDGE_REVISION_ID 0x2
+
+/* Link Training specific registers */
+#define SN_DEVICE_REV_REG			0x08
+#define SN_REFCLK_FREQ_REG			0x0A
+#define SN_DSI_LANES_REG			0x10
+#define SN_DSIA_CLK_FREQ_REG			0x12
+#define SN_ENH_FRAME_REG			0x5A
+#define SN_SSC_CONFIG_REG			0x93
+#define SN_DATARATE_CONFIG_REG			0x94
+#define SN_PLL_ENABLE_REG			0x0D
+#define SN_SCRAMBLE_CONFIG_REG			0x95
+#define SN_AUX_WDATA0_REG			0x64
+#define SN_AUX_ADDR_19_16_REG			0x74
+#define SN_AUX_ADDR_15_8_REG			0x75
+#define SN_AUX_ADDR_7_0_REG			0x76
+#define SN_AUX_LENGTH_REG			0x77
+#define SN_AUX_CMD_REG				0x78
+#define SN_ML_TX_MODE_REG			0x96
+#define SN_PAGE_SELECT_REG			0xFF
+#define SN_AUX_RDATA0_REG			0x79
+/* video config specific registers */
+#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG	0x20
+#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG	0x21
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG	0x24
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG	0x25
+#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG	0x2C
+#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG	0x2D
+#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG	0x30
+#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG	0x31
+#define SN_CHA_HORIZONTAL_BACK_PORCH_REG	0x34
+#define SN_CHA_VERTICAL_BACK_PORCH_REG		0x36
+#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG	0x38
+#define SN_CHA_VERTICAL_FRONT_PORCH_REG		0x3A
+#define SN_DATA_FORMAT_REG			0x5B
+#define SN_COLOR_BAR_CONFIG_REG			0x3C
+
+#define DPPLL_CLK_SRC_REFCLK	0
+#define DPPLL_CLK_SRC_DSICLK	1
+#define MIN_DSI_CLK_FREQ_MHZ	40
+
+enum ti_sn_bridge_ref_clks {
+	SN_REF_CLK_12_MHZ = 0,
+	SN_REF_CLK_19_2_MHZ,
+	SN_REF_CLK_26_MHZ,
+	SN_REF_CLK_27_MHZ,
+	SN_REF_CLK_38_4_MHZ,
+	SN_REF_CLK_MAX,
+};
+
+struct ti_sn_bridge {
+	struct device *dev;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 83

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
+	/* handle to the connected eDP panel */
+	struct drm_panel *panel;
+	int irq;
+	struct gpio_desc *irq_gpio;
+	/* list of gpios needed to enable the bridge functionality */
+	struct gpio_descs *enable_gpios;

Why are the enable gpios a list?

Quoted 478 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	unsigned int num_supplies;
+	struct regulator_bulk_data *supplies;
+	struct i2c_client *i2c_client;
+	struct i2c_adapter *ddc;
+	bool power_on;
+	u32 num_modes;
+	struct drm_display_mode curr_mode;
+};
+
+static int ti_sn_bridge_write(struct ti_sn_bridge *pdata, u8 reg, u8

val)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +46 -0
+{
+	struct i2c_client *client = pdata->i2c_client;
+	u8 buf[2] = {reg, val};
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = 0,
+		.len = 2,
+		.buf = buf,
+	};
+
+	if (i2c_transfer(client->adapter, &msg, 1) < 1) {
+		DRM_ERROR("i2c write failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ti_sn_bridge_read(struct ti_sn_bridge *pdata,
+					u8 reg, char *buf, u32 size)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	struct i2c_msg msg[2] = {
+		{
+			.addr = client->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = &reg,
+		},
+		{
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buf,
+		}
+	};
+
+	if (i2c_transfer(client->adapter, msg, 2) != 2) {
+		DRM_ERROR("i2c read failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ti_sn_bridge_gpio_configure(struct ti_sn_bridge *pdata,

bool on)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +25 -0
+{
+	int value, i = 0, num = 0;
+	int *value_array = NULL;
+
+	if (!pdata)
+		return;
+
+	value = on ? 1 : 0;
+	num = pdata->enable_gpios->ndescs;
+	value_array = kmalloc(num * sizeof(int), GFP_KERNEL);
+	if (value_array) {
+		for (i = 0; i < num; i++)
+			value_array[i] = value;
+		gpiod_set_array_value(num, pdata->enable_gpios->desc,
+				      value_array);
+		kfree(value_array);
+	}
+
+	if (pdata->irq_gpio)
+		gpiod_set_value(pdata->irq_gpio, value);
+
+	DRM_DEBUG("config to: %d\n", value);
+}
+
+static void ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool

enable)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +39 -0
+{
+	static int ref_count;
+
+	if (!pdata)
+		return;
+
+	if (enable)
+		ref_count++;
+	else
+		ref_count--;
+
+	if (enable && (ref_count == 1)) {
+		ti_sn_bridge_gpio_configure(pdata, true);
+
+		if (regulator_bulk_enable(pdata->num_supplies,
+						pdata->supplies)) {
+			DRM_ERROR("bridge regulator enable failed\n");
+			return;
+		}
+		pdata->power_on = true;
+	} else if (!enable && !ref_count) {
+		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
+
+		ti_sn_bridge_gpio_configure(pdata, false);
+		pdata->power_on = false;
+	} else {
+		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
+							enable, ref_count);
+	}
+}
+
+/* Connector funcs */
+static struct ti_sn_bridge *
+connector_to_ti_sn_bridge(struct drm_connector *connector)
+{
+	return container_of(connector, struct ti_sn_bridge, connector);
+}
+
+static int ti_sn_bridge_connector_get_modes(struct drm_connector

*connector)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +41 -0
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+	struct drm_panel *panel = pdata->panel;
+	struct edid *edid;
+
+	if (panel) {
+		DRM_DEBUG("get mode from connected drm_panel\n");
+		pdata->num_modes = drm_panel_get_modes(panel);
+	} else {
+		/* get from EDID */
+		if (!pdata->ddc)
+			return 0;
+		ti_sn_bridge_power_ctrl(pdata, true);
+		edid = drm_get_edid(connector, pdata->ddc);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			pdata->num_modes = drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			kfree(edid);
+		} else {
+			DRM_DEBUG("failed to get edid\n");
+		}
+		ti_sn_bridge_power_ctrl(pdata, false);
+	}
+
+	return pdata->num_modes;
+}
+
+static enum drm_mode_status
+ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	/* maximum supported resolution is 4K at 60 fps */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static struct drm_connector_helper_funcs

ti_sn_bridge_connector_helper_funcs = {

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+	.get_modes = ti_sn_bridge_connector_get_modes,
+	.mode_valid = ti_sn_bridge_connector_mode_valid,
+};
+
+static enum drm_connector_status
+ti_sn_bridge_connector_detect(struct drm_connector *connector, bool

force)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +15 -0
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+
+	/**
+	 * TODO: Currently if drm_panel is present, then always
+	 * return the status as connected. Need to add support to detect
+	 * device state for no panel(hot pluggable) scenarios.
+	 */
+	if (pdata->panel)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+}
+
+static const struct drm_connector_funcs ti_sn_bridge_connector_funcs

= {

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +5 -0
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = ti_sn_bridge_connector_detect,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state =

drm_atomic_helper_connector_duplicate_state,

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +4 -0
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge

*bridge)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +118 -0
+{
+	return container_of(bridge, struct ti_sn_bridge, bridge);
+}
+
+static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
+{
+	u8 rev = 0;
+	int ret = 0;
+
+	ret = ti_sn_bridge_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
+	if (ret)
+		goto exit;
+
+	if (rev == SN_BRIDGE_REVISION_ID) {
+		DRM_DEBUG("ti_sn_bridge revision id: 0x%x\n", rev);
+	} else {
+		DRM_ERROR("ti_sn_bridge revision id mismatch\n");
+		ret = -EINVAL;
+	}
+
+exit:
+	return ret;
+}
+
+static const char * const ti_sn_bridge_supply_names[] = {
+	"vccio",
+	"vcca",
+};
+
+static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
+{
+	unsigned int i;
+	int ret;
+
+	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
+
+	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
+				     sizeof(*pdata->supplies), GFP_KERNEL);
+	if (!pdata->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < pdata->num_supplies; i++)
+		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
+
+	ret = devm_regulator_bulk_get(pdata->dev,
+			pdata->num_supplies, pdata->supplies);
+
+	return ret;
+}
+
+static void ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
+{
+	struct device_node *panel_node, *port, *endpoint;
+	struct drm_panel *panel = NULL;
+
+	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
+	if (port) {
+		endpoint = of_get_child_by_name(port, "endpoint");
+		of_node_put(port);
+		if (!endpoint) {
+			dev_err(pdata->dev, "no output endpoint found\n");
+			goto error;
+		}
+
+		panel_node = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+		if (!panel_node) {
+			dev_err(pdata->dev, "no output node found\n");
+			goto error;
+		}
+
+		panel = of_drm_find_panel(panel_node);
+		of_node_put(panel_node);
+		if (!panel) {
+			dev_err(pdata->dev, "no panel node found\n");
+			goto error;
+		}
+	}
+
+	pdata->panel = panel;
+	drm_panel_attach(panel, &pdata->connector);
+
+	return;
+
+error:
+	pdata->panel = NULL;
+}
+
+static int ti_sn_bridge_attach(struct drm_bridge *bridge)
+{
+	struct mipi_dsi_host *host;
+	struct mipi_dsi_device *dsi;
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
+						   .channel = 0,
+						   .node = NULL,
+						 };
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	/* HPD not supported */
+	pdata->connector.polled = 0;
+
+	ret = drm_connector_init(bridge->dev, &pdata->connector,
+				 &ti_sn_bridge_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&pdata->connector,
+				 &ti_sn_bridge_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&pdata->connector,

bridge->encoder);

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +18 -0
+
+	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+	if (!host) {
+		DRM_ERROR("failed to find dsi host\n");
+		return -ENODEV;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		DRM_ERROR("failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		goto err_dsi_device;
+	}
+
+	/* TODO: setting to 4 lanes always for now */
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |

MIPI_DSI_MODE_VIDEO_SYNC_PULSE |

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +29 -0
+			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_ERROR("failed to attach dsi to host\n");
+		goto err_dsi_attach;
+	}
+
+	pdata->dsi = dsi;
+
+	DRM_DEBUG("bridge attached\n");
+	/* attach panel to bridge */
+	ti_sn_bridge_attach_panel(pdata);
+
+	return 0;
+
+err_dsi_attach:
+	mipi_dsi_device_unregister(dsi);
+err_dsi_device:
+	return ret;
+}
+
+static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj_mode)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d,

clock=%d\n",

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +59 -0
+		adj_mode->hdisplay, adj_mode->vdisplay,
+		adj_mode->vrefresh, adj_mode->clock);
+
+	drm_mode_copy(&pdata->curr_mode, adj_mode);
+}
+
+static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+
+	if (panel) {
+		drm_panel_disable(panel);
+		drm_panel_unprepare(panel);
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, false);
+}
+
+static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+	struct drm_display_mode *mode = &pdata->curr_mode;
+	u32 bit_rate_mhz, clk_freq_mhz;
+	u8 val = 0;
+
+	if (!pdata->num_modes)
+		return;
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+
+	if (panel)
+		drm_panel_prepare(panel);
+
+	/* CLK_SRC config */
+	ti_sn_bridge_write(pdata, SN_REFCLK_FREQ_REG,
+			(DPPLL_CLK_SRC_REFCLK | (SN_REF_CLK_19_2_MHZ << 0x1)));
+
+	/* DSI_A lane config */
+	ti_sn_bridge_read(pdata, SN_DSI_LANES_REG, &val, 0x1);
+	val |= ((0x4 - pdata->dsi->lanes) & 0x03) << 0x3;
+	ti_sn_bridge_write(pdata, SN_DSI_LANES_REG, val);
+
+	/* DP lane config */
+	ti_sn_bridge_read(pdata, SN_SSC_CONFIG_REG, &val, 0x1);
+	val |= ((pdata->dsi->lanes - 0x1) & 0x03) << 0x4;
+	ti_sn_bridge_write(pdata, SN_SSC_CONFIG_REG, val);
+
+	/* set dsi clk frequency value */
+	bit_rate_mhz = (mode->clock / 1000) *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+	/* for each increment in val, frequency increases by 5MHz */
+	val = (MIN_DSI_CLK_FREQ_MHZ / 0x5) +
+		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 0x5) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_DSIA_CLK_FREQ_REG, val);
+
+	ti_sn_bridge_write(pdata, SN_DATARATE_CONFIG_REG, 0x80); /* set HBR

*/

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +50 -0
+
+	ti_sn_bridge_write(pdata, SN_PLL_ENABLE_REG, 0x1); /* Enable PLL */
+	usleep_range(10000, 10001);
+
+	/* DPCD Register 0x0010A in Sink */
+	ti_sn_bridge_write(pdata, SN_AUX_WDATA0_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_19_16_REG, 0x00);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_15_8_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_7_0_REG, 0x0A);
+	ti_sn_bridge_write(pdata, SN_AUX_LENGTH_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_CMD_REG, 0x81);
+	usleep_range(10000, 10001);
+
+	/* Semi auto link training mode */
+	ti_sn_bridge_write(pdata, SN_ML_TX_MODE_REG, 0x0A);
+	usleep_range(20000, 20001);
+
+	/* config video parameters */
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
+			mode->hdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
+			mode->vdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->htotal - mode->hsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->htotal - mode->hsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->vtotal - mode->vsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->vtotal - mode->vsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
+			(mode->hsync_start - mode->hdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_BACK_PORCH_REG,
+			(mode->vsync_start - mode->vdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
+			(mode->hsync_end - mode->hsync_start) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_FRONT_PORCH_REG,
+			(mode->vsync_end - mode->vsync_start) & 0xFF);
+	if (pdata->dsi->format == MIPI_DSI_FMT_RGB888)
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x00);
+	else
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x01);
+	usleep_range(10000, 10001);
+
+	/* enable video stream */
+	ti_sn_bridge_read(pdata, SN_ENH_FRAME_REG, &val, 0x1);
Quoted 8 lines

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 553

Quoted 5 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +5 -0
+	val |= BIT(3);
+	ti_sn_bridge_write(pdata, SN_ENH_FRAME_REG, val);
+
+	if (panel)
+		drm_panel_enable(panel);

As mentioned in the previous review, you should configure the mode in mode_set, not enable. This eliminates the need for curr_mode.

If we move this to mode_set then it does not match with the datasheet init sequence provided in page 71. First mode_set is called in commit which will set the current mode. after that when bridge enable gets called gpio and regulator enable and then the bridge registers needs to be programmed based on current mode.

Quoted 149 lines
Quoted 149 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +148 -0
+}
+
+static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
+	.attach = ti_sn_bridge_attach,
+	.enable = ti_sn_bridge_enable,
+	.disable = ti_sn_bridge_disable,
+	.mode_set = ti_sn_bridge_mode_set,
+};
+
+static int ti_sn_bridge_parse_gpios(struct ti_sn_bridge *pdata)
+{
+	int ret = 0;
+
+	if (!pdata || !pdata->dev)
+		return -EINVAL;
+
+	pdata->enable_gpios = devm_gpiod_get_array(pdata->dev,
+						"enable", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->enable_gpios)) {
+		DRM_ERROR("failed to get enable gpio from DT\n");
+		ret = PTR_ERR(pdata->enable_gpios);
+		goto exit;
+	}
+
+	pdata->irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->irq_gpio))
+		DRM_ERROR("failed to get irq gpio from DT\n");
+
+exit:
+	return ret;
+}
+
+static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
+{
+	struct device_node *np = pdata->dev->of_node;
+	struct device_node *end_node;
+
+	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
+	if (!end_node) {
+		DRM_ERROR("remote endpoint not found\n");
+		return -ENODEV;
+	}
+
+	pdata->host_node = of_graph_get_remote_port_parent(end_node);
+	of_node_put(end_node);
+	if (!pdata->host_node) {
+		DRM_ERROR("remote node not found\n");
+		return -ENODEV;
+	}
+	of_node_put(pdata->host_node);
+
+	return 0;
+}
+
+static int ti_sn_bridge_probe(struct i2c_client *client,
+	 const struct i2c_device_id *id)
+{
+	struct ti_sn_bridge *pdata;
+	struct device_node *ddc_node;
+	int ret = 0;
+
+	if (!client || !client->dev.of_node) {
+		DRM_ERROR("invalid input\n");
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		DRM_ERROR("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	pdata = devm_kzalloc(&client->dev,
+		sizeof(struct ti_sn_bridge), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->power_on = false;
+	pdata->dev = &client->dev;
+	pdata->i2c_client = client;
+	DRM_DEBUG("I2C address is %x\n", client->addr);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse device tree\n");
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_gpios(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse gpios from DT\n");
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_regulators(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse regulators\n");
+		goto err_dt_parse;
+	}
+
+	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+		if (!pdata->ddc) {
+			dev_dbg(pdata->dev, "failed to read ddc node\n");
+			return -EPROBE_DEFER;
+		}
+	} else {
+		dev_dbg(pdata->dev, "no ddc property found\n");
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+	ret = ti_sn_bridge_read_device_rev(pdata);
+	if (ret) {
+		DRM_ERROR("failed to read chip rev\n");
+		goto err_config;
+	} else {
+		DRM_DEBUG("bridge chip enabled successfully\n");
+	}
+	ti_sn_bridge_power_ctrl(pdata, false);
+
+	i2c_set_clientdata(client, pdata);
+	dev_set_drvdata(&client->dev, pdata);
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = client->dev.of_node;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return ret;
+
+err_config:
+	ti_sn_bridge_power_ctrl(pdata, false);
+err_dt_parse:
+	devm_kfree(&client->dev, pdata);
+	return ret;
+}
+
+static int ti_sn_bridge_remove(struct i2c_client *client)
+{
+	int ret = -EINVAL;
+	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
+
+	if (!pdata)
+		goto end;
+
+	mipi_dsi_detach(pdata->dsi);
Quoted 8 lines

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 707

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+	mipi_dsi_device_unregister(pdata->dsi);
+
+	drm_bridge_remove(&pdata->bridge);
+
+	disable_irq(pdata->irq);
+	free_irq(pdata->irq, pdata);

This is uninitialized.

since irq is anyways not used in this driver yet so i will remove this part.

Quoted 32 lines
Quoted 32 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +31 -0
+
+	ti_sn_bridge_gpio_configure(pdata, false);
+	i2c_put_adapter(pdata->ddc);
+
+	devm_kfree(&client->dev, pdata);
+
+end:
+	return ret;
+}
+
+static struct i2c_device_id ti_sn_bridge_id[] = {
+	{ "ti,sn65dsi86", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+
+static const struct of_device_id ti_sn_bridge_match_table[] = {
+	{.compatible = "ti,sn65dsi86"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
+
+static struct i2c_driver ti_sn_bridge_driver = {
+	.driver = {
+		.name = "ti_sn65dsi86",
+		.owner = THIS_MODULE,
+		.of_match_table = ti_sn_bridge_match_table,
+	},
+	.probe = ti_sn_bridge_probe,
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id,
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +4 -1
+};
+
+module_i2c_driver(ti_sn_bridge_driver);
+MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

author
#5 lore raw

Re: [[RFC]DPU PATCH 1/4] drm/bridge: add support for sn65dsi86 bridge driver

In reply to Sean Paul
Quoted 107 lines · Sean Paul
Quoted 33 lines · Sandeep Panda

Add support for TI's sn65dsi86 dsi2edp bridge chip. The chip converts DSI transmitted signal to eDP signal, which is fed to the connected eDP panel.

This chip can be controlled via either i2c interface or dsi interface. Currently in driver all the control registers are being accessed through i2c interface only. Also as of now HPD support has not been added to bridge chip driver.

Signed-off-by Sandeep Panda <[email protected]>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 742 
++++++++++++++++++++++++++++++++++
 1 file changed, 742 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
new file mode 100644
index 0000000..c798f2f
@@ -0,0 +1,742 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 20

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>

I have a hard time believing that you need all of these includes. Off the top of my head, you could probably remove types, kernel, init, platform_device, delay, drmP, drm_atomic, drm_crtc_helper. You could probably trim it even further with the help of your compiler. These should also be in alphabetical order.

Quoted 57 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +57 -0
+
+#define SN_BRIDGE_REVISION_ID 0x2
+
+/* Link Training specific registers */
+#define SN_DEVICE_REV_REG			0x08
+#define SN_REFCLK_FREQ_REG			0x0A
+#define SN_DSI_LANES_REG			0x10
+#define SN_DSIA_CLK_FREQ_REG			0x12
+#define SN_ENH_FRAME_REG			0x5A
+#define SN_SSC_CONFIG_REG			0x93
+#define SN_DATARATE_CONFIG_REG			0x94
+#define SN_PLL_ENABLE_REG			0x0D
+#define SN_SCRAMBLE_CONFIG_REG			0x95
+#define SN_AUX_WDATA0_REG			0x64
+#define SN_AUX_ADDR_19_16_REG			0x74
+#define SN_AUX_ADDR_15_8_REG			0x75
+#define SN_AUX_ADDR_7_0_REG			0x76
+#define SN_AUX_LENGTH_REG			0x77
+#define SN_AUX_CMD_REG				0x78
+#define SN_ML_TX_MODE_REG			0x96
+#define SN_PAGE_SELECT_REG			0xFF
+#define SN_AUX_RDATA0_REG			0x79
+/* video config specific registers */
+#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG	0x20
+#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG	0x21
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG	0x24
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG	0x25
+#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG	0x2C
+#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG	0x2D
+#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG	0x30
+#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG	0x31
+#define SN_CHA_HORIZONTAL_BACK_PORCH_REG	0x34
+#define SN_CHA_VERTICAL_BACK_PORCH_REG		0x36
+#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG	0x38
+#define SN_CHA_VERTICAL_FRONT_PORCH_REG		0x3A
+#define SN_DATA_FORMAT_REG			0x5B
+#define SN_COLOR_BAR_CONFIG_REG			0x3C
+
+#define DPPLL_CLK_SRC_REFCLK	0
+#define DPPLL_CLK_SRC_DSICLK	1
+#define MIN_DSI_CLK_FREQ_MHZ	40
+
+enum ti_sn_bridge_ref_clks {
+	SN_REF_CLK_12_MHZ = 0,
+	SN_REF_CLK_19_2_MHZ,
+	SN_REF_CLK_26_MHZ,
+	SN_REF_CLK_27_MHZ,
+	SN_REF_CLK_38_4_MHZ,
+	SN_REF_CLK_MAX,
+};
+
+struct ti_sn_bridge {
+	struct device *dev;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
Quoted 8 lines

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 83

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+	/* handle to the connected eDP panel */
+	struct drm_panel *panel;
+	int irq;
+	struct gpio_desc *irq_gpio;
+	/* list of gpios needed to enable the bridge functionality */
+	struct gpio_descs *enable_gpios;

Why are the enable gpios a list?

This as per the review comment in previous patchset
"I see IRQ and EN in the datasheet, but not the others. It seems like the aux_* and edp_* gpios are always equal to en. So if you actually need them, don't specify a new struct, just add irq_gpio to the main struct and add an array of en_gpios to handle the rest."

I see in schematic 2 more gpios are needed to enable aux_channel communication through the ddc. So i have put an array of enable gpios. Based on dt it will dynamically parse one or more gpios.

Quoted 676 lines
Quoted 474 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	unsigned int num_supplies;
+	struct regulator_bulk_data *supplies;
+	struct i2c_client *i2c_client;
+	struct i2c_adapter *ddc;
+	bool power_on;
+	u32 num_modes;
+	struct drm_display_mode curr_mode;
+};
+
+static int ti_sn_bridge_write(struct ti_sn_bridge *pdata, u8 reg, u8

val)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +46 -0
+{
+	struct i2c_client *client = pdata->i2c_client;
+	u8 buf[2] = {reg, val};
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = 0,
+		.len = 2,
+		.buf = buf,
+	};
+
+	if (i2c_transfer(client->adapter, &msg, 1) < 1) {
+		DRM_ERROR("i2c write failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ti_sn_bridge_read(struct ti_sn_bridge *pdata,
+					u8 reg, char *buf, u32 size)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	struct i2c_msg msg[2] = {
+		{
+			.addr = client->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = &reg,
+		},
+		{
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buf,
+		}
+	};
+
+	if (i2c_transfer(client->adapter, msg, 2) != 2) {
+		DRM_ERROR("i2c read failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ti_sn_bridge_gpio_configure(struct ti_sn_bridge *pdata,

bool on)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +25 -0
+{
+	int value, i = 0, num = 0;
+	int *value_array = NULL;
+
+	if (!pdata)
+		return;
+
+	value = on ? 1 : 0;
+	num = pdata->enable_gpios->ndescs;
+	value_array = kmalloc(num * sizeof(int), GFP_KERNEL);
+	if (value_array) {
+		for (i = 0; i < num; i++)
+			value_array[i] = value;
+		gpiod_set_array_value(num, pdata->enable_gpios->desc,
+				      value_array);
+		kfree(value_array);
+	}
+
+	if (pdata->irq_gpio)
+		gpiod_set_value(pdata->irq_gpio, value);
+
+	DRM_DEBUG("config to: %d\n", value);
+}
+
+static void ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool

enable)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +39 -0
+{
+	static int ref_count;
+
+	if (!pdata)
+		return;
+
+	if (enable)
+		ref_count++;
+	else
+		ref_count--;
+
+	if (enable && (ref_count == 1)) {
+		ti_sn_bridge_gpio_configure(pdata, true);
+
+		if (regulator_bulk_enable(pdata->num_supplies,
+						pdata->supplies)) {
+			DRM_ERROR("bridge regulator enable failed\n");
+			return;
+		}
+		pdata->power_on = true;
+	} else if (!enable && !ref_count) {
+		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
+
+		ti_sn_bridge_gpio_configure(pdata, false);
+		pdata->power_on = false;
+	} else {
+		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
+							enable, ref_count);
+	}
+}
+
+/* Connector funcs */
+static struct ti_sn_bridge *
+connector_to_ti_sn_bridge(struct drm_connector *connector)
+{
+	return container_of(connector, struct ti_sn_bridge, connector);
+}
+
+static int ti_sn_bridge_connector_get_modes(struct drm_connector

*connector)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +41 -0
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+	struct drm_panel *panel = pdata->panel;
+	struct edid *edid;
+
+	if (panel) {
+		DRM_DEBUG("get mode from connected drm_panel\n");
+		pdata->num_modes = drm_panel_get_modes(panel);
+	} else {
+		/* get from EDID */
+		if (!pdata->ddc)
+			return 0;
+		ti_sn_bridge_power_ctrl(pdata, true);
+		edid = drm_get_edid(connector, pdata->ddc);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			pdata->num_modes = drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			kfree(edid);
+		} else {
+			DRM_DEBUG("failed to get edid\n");
+		}
+		ti_sn_bridge_power_ctrl(pdata, false);
+	}
+
+	return pdata->num_modes;
+}
+
+static enum drm_mode_status
+ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	/* maximum supported resolution is 4K at 60 fps */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static struct drm_connector_helper_funcs

ti_sn_bridge_connector_helper_funcs = {

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+	.get_modes = ti_sn_bridge_connector_get_modes,
+	.mode_valid = ti_sn_bridge_connector_mode_valid,
+};
+
+static enum drm_connector_status
+ti_sn_bridge_connector_detect(struct drm_connector *connector, bool

force)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +15 -0
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+
+	/**
+	 * TODO: Currently if drm_panel is present, then always
+	 * return the status as connected. Need to add support to detect
+	 * device state for no panel(hot pluggable) scenarios.
+	 */
+	if (pdata->panel)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+}
+
+static const struct drm_connector_funcs ti_sn_bridge_connector_funcs

= {

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +5 -0
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = ti_sn_bridge_connector_detect,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state =

drm_atomic_helper_connector_duplicate_state,

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +4 -0
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge

*bridge)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +118 -0
+{
+	return container_of(bridge, struct ti_sn_bridge, bridge);
+}
+
+static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
+{
+	u8 rev = 0;
+	int ret = 0;
+
+	ret = ti_sn_bridge_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
+	if (ret)
+		goto exit;
+
+	if (rev == SN_BRIDGE_REVISION_ID) {
+		DRM_DEBUG("ti_sn_bridge revision id: 0x%x\n", rev);
+	} else {
+		DRM_ERROR("ti_sn_bridge revision id mismatch\n");
+		ret = -EINVAL;
+	}
+
+exit:
+	return ret;
+}
+
+static const char * const ti_sn_bridge_supply_names[] = {
+	"vccio",
+	"vcca",
+};
+
+static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
+{
+	unsigned int i;
+	int ret;
+
+	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
+
+	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
+				     sizeof(*pdata->supplies), GFP_KERNEL);
+	if (!pdata->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < pdata->num_supplies; i++)
+		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
+
+	ret = devm_regulator_bulk_get(pdata->dev,
+			pdata->num_supplies, pdata->supplies);
+
+	return ret;
+}
+
+static void ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
+{
+	struct device_node *panel_node, *port, *endpoint;
+	struct drm_panel *panel = NULL;
+
+	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
+	if (port) {
+		endpoint = of_get_child_by_name(port, "endpoint");
+		of_node_put(port);
+		if (!endpoint) {
+			dev_err(pdata->dev, "no output endpoint found\n");
+			goto error;
+		}
+
+		panel_node = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+		if (!panel_node) {
+			dev_err(pdata->dev, "no output node found\n");
+			goto error;
+		}
+
+		panel = of_drm_find_panel(panel_node);
+		of_node_put(panel_node);
+		if (!panel) {
+			dev_err(pdata->dev, "no panel node found\n");
+			goto error;
+		}
+	}
+
+	pdata->panel = panel;
+	drm_panel_attach(panel, &pdata->connector);
+
+	return;
+
+error:
+	pdata->panel = NULL;
+}
+
+static int ti_sn_bridge_attach(struct drm_bridge *bridge)
+{
+	struct mipi_dsi_host *host;
+	struct mipi_dsi_device *dsi;
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
+						   .channel = 0,
+						   .node = NULL,
+						 };
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	/* HPD not supported */
+	pdata->connector.polled = 0;
+
+	ret = drm_connector_init(bridge->dev, &pdata->connector,
+				 &ti_sn_bridge_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&pdata->connector,
+				 &ti_sn_bridge_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&pdata->connector,

bridge->encoder);

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +18 -0
+
+	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+	if (!host) {
+		DRM_ERROR("failed to find dsi host\n");
+		return -ENODEV;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		DRM_ERROR("failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		goto err_dsi_device;
+	}
+
+	/* TODO: setting to 4 lanes always for now */
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |

MIPI_DSI_MODE_VIDEO_SYNC_PULSE |

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +29 -0
+			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_ERROR("failed to attach dsi to host\n");
+		goto err_dsi_attach;
+	}
+
+	pdata->dsi = dsi;
+
+	DRM_DEBUG("bridge attached\n");
+	/* attach panel to bridge */
+	ti_sn_bridge_attach_panel(pdata);
+
+	return 0;
+
+err_dsi_attach:
+	mipi_dsi_device_unregister(dsi);
+err_dsi_device:
+	return ret;
+}
+
+static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj_mode)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d,

clock=%d\n",

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +59 -0
+		adj_mode->hdisplay, adj_mode->vdisplay,
+		adj_mode->vrefresh, adj_mode->clock);
+
+	drm_mode_copy(&pdata->curr_mode, adj_mode);
+}
+
+static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+
+	if (panel) {
+		drm_panel_disable(panel);
+		drm_panel_unprepare(panel);
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, false);
+}
+
+static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+	struct drm_display_mode *mode = &pdata->curr_mode;
+	u32 bit_rate_mhz, clk_freq_mhz;
+	u8 val = 0;
+
+	if (!pdata->num_modes)
+		return;
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+
+	if (panel)
+		drm_panel_prepare(panel);
+
+	/* CLK_SRC config */
+	ti_sn_bridge_write(pdata, SN_REFCLK_FREQ_REG,
+			(DPPLL_CLK_SRC_REFCLK | (SN_REF_CLK_19_2_MHZ << 0x1)));
+
+	/* DSI_A lane config */
+	ti_sn_bridge_read(pdata, SN_DSI_LANES_REG, &val, 0x1);
+	val |= ((0x4 - pdata->dsi->lanes) & 0x03) << 0x3;
+	ti_sn_bridge_write(pdata, SN_DSI_LANES_REG, val);
+
+	/* DP lane config */
+	ti_sn_bridge_read(pdata, SN_SSC_CONFIG_REG, &val, 0x1);
+	val |= ((pdata->dsi->lanes - 0x1) & 0x03) << 0x4;
+	ti_sn_bridge_write(pdata, SN_SSC_CONFIG_REG, val);
+
+	/* set dsi clk frequency value */
+	bit_rate_mhz = (mode->clock / 1000) *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+	/* for each increment in val, frequency increases by 5MHz */
+	val = (MIN_DSI_CLK_FREQ_MHZ / 0x5) +
+		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 0x5) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_DSIA_CLK_FREQ_REG, val);
+
+	ti_sn_bridge_write(pdata, SN_DATARATE_CONFIG_REG, 0x80); /* set HBR

*/

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +45 -0
+
+	ti_sn_bridge_write(pdata, SN_PLL_ENABLE_REG, 0x1); /* Enable PLL */
+	usleep_range(10000, 10001);
+
+	/* DPCD Register 0x0010A in Sink */
+	ti_sn_bridge_write(pdata, SN_AUX_WDATA0_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_19_16_REG, 0x00);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_15_8_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_7_0_REG, 0x0A);
+	ti_sn_bridge_write(pdata, SN_AUX_LENGTH_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_CMD_REG, 0x81);
+	usleep_range(10000, 10001);
+
+	/* Semi auto link training mode */
+	ti_sn_bridge_write(pdata, SN_ML_TX_MODE_REG, 0x0A);
+	usleep_range(20000, 20001);
+
+	/* config video parameters */
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
+			mode->hdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
+			mode->vdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->htotal - mode->hsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->htotal - mode->hsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->vtotal - mode->vsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->vtotal - mode->vsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
+			(mode->hsync_start - mode->hdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_BACK_PORCH_REG,
+			(mode->vsync_start - mode->vdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
+			(mode->hsync_end - mode->hsync_start) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_FRONT_PORCH_REG,
+			(mode->vsync_end - mode->vsync_start) & 0xFF);
+	if (pdata->dsi->format == MIPI_DSI_FMT_RGB888)
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x00);
+	else

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 553

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x01);
+	usleep_range(10000, 10001);
+
+	/* enable video stream */
+	ti_sn_bridge_read(pdata, SN_ENH_FRAME_REG, &val, 0x1);
+	val |= BIT(3);
+	ti_sn_bridge_write(pdata, SN_ENH_FRAME_REG, val);
+
+	if (panel)
+		drm_panel_enable(panel);

As mentioned in the previous review, you should configure the mode in mode_set, not enable. This eliminates the need for curr_mode.

Quoted 144 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +144 -0
+}
+
+static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
+	.attach = ti_sn_bridge_attach,
+	.enable = ti_sn_bridge_enable,
+	.disable = ti_sn_bridge_disable,
+	.mode_set = ti_sn_bridge_mode_set,
+};
+
+static int ti_sn_bridge_parse_gpios(struct ti_sn_bridge *pdata)
+{
+	int ret = 0;
+
+	if (!pdata || !pdata->dev)
+		return -EINVAL;
+
+	pdata->enable_gpios = devm_gpiod_get_array(pdata->dev,
+						"enable", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->enable_gpios)) {
+		DRM_ERROR("failed to get enable gpio from DT\n");
+		ret = PTR_ERR(pdata->enable_gpios);
+		goto exit;
+	}
+
+	pdata->irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->irq_gpio))
+		DRM_ERROR("failed to get irq gpio from DT\n");
+
+exit:
+	return ret;
+}
+
+static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
+{
+	struct device_node *np = pdata->dev->of_node;
+	struct device_node *end_node;
+
+	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
+	if (!end_node) {
+		DRM_ERROR("remote endpoint not found\n");
+		return -ENODEV;
+	}
+
+	pdata->host_node = of_graph_get_remote_port_parent(end_node);
+	of_node_put(end_node);
+	if (!pdata->host_node) {
+		DRM_ERROR("remote node not found\n");
+		return -ENODEV;
+	}
+	of_node_put(pdata->host_node);
+
+	return 0;
+}
+
+static int ti_sn_bridge_probe(struct i2c_client *client,
+	 const struct i2c_device_id *id)
+{
+	struct ti_sn_bridge *pdata;
+	struct device_node *ddc_node;
+	int ret = 0;
+
+	if (!client || !client->dev.of_node) {
+		DRM_ERROR("invalid input\n");
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		DRM_ERROR("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	pdata = devm_kzalloc(&client->dev,
+		sizeof(struct ti_sn_bridge), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->power_on = false;
+	pdata->dev = &client->dev;
+	pdata->i2c_client = client;
+	DRM_DEBUG("I2C address is %x\n", client->addr);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse device tree\n");
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_gpios(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse gpios from DT\n");
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_regulators(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse regulators\n");
+		goto err_dt_parse;
+	}
+
+	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+		if (!pdata->ddc) {
+			dev_dbg(pdata->dev, "failed to read ddc node\n");
+			return -EPROBE_DEFER;
+		}
+	} else {
+		dev_dbg(pdata->dev, "no ddc property found\n");
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+	ret = ti_sn_bridge_read_device_rev(pdata);
+	if (ret) {
+		DRM_ERROR("failed to read chip rev\n");
+		goto err_config;
+	} else {
+		DRM_DEBUG("bridge chip enabled successfully\n");
+	}
+	ti_sn_bridge_power_ctrl(pdata, false);
+
+	i2c_set_clientdata(client, pdata);
+	dev_set_drvdata(&client->dev, pdata);
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = client->dev.of_node;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return ret;
+
+err_config:
+	ti_sn_bridge_power_ctrl(pdata, false);
+err_dt_parse:
+	devm_kfree(&client->dev, pdata);
+	return ret;
+}
+
+static int ti_sn_bridge_remove(struct i2c_client *client)
+{
+	int ret = -EINVAL;
+	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
+

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 707

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	if (!pdata)
+		goto end;
+
+	mipi_dsi_detach(pdata->dsi);
+	mipi_dsi_device_unregister(pdata->dsi);
+
+	drm_bridge_remove(&pdata->bridge);
+
+	disable_irq(pdata->irq);
+	free_irq(pdata->irq, pdata);

This is uninitialized.

Quoted 31 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +31 -0
+
+	ti_sn_bridge_gpio_configure(pdata, false);
+	i2c_put_adapter(pdata->ddc);
+
+	devm_kfree(&client->dev, pdata);
+
+end:
+	return ret;
+}
+
+static struct i2c_device_id ti_sn_bridge_id[] = {
+	{ "ti,sn65dsi86", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+
+static const struct of_device_id ti_sn_bridge_match_table[] = {
+	{.compatible = "ti,sn65dsi86"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
+
+static struct i2c_driver ti_sn_bridge_driver = {
+	.driver = {
+		.name = "ti_sn65dsi86",
+		.owner = THIS_MODULE,
+		.of_match_table = ti_sn_bridge_match_table,
+	},
+	.probe = ti_sn_bridge_probe,
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id,
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +4 -1
+};
+
+module_i2c_driver(ti_sn_bridge_driver);
+MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

A message that is not in the archive (sent off-list or not yet received): 1524054002-17869-2-git-send-email-spanda-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org

#7 lore raw

Re: [[RFC]DPU PATCH 1/4] drm/bridge: add support for sn65dsi86 bridge driver

Quoted 22 lines · Sandeep Panda

Add support for TI's sn65dsi86 dsi2edp bridge chip. The chip converts DSI transmitted signal to eDP signal, which is fed to the connected eDP panel.

This chip can be controlled via either i2c interface or dsi interface. Currently in driver all the control registers are being accessed through i2c interface only. Also as of now HPD support has not been added to bridge chip driver.

Signed-off-by Sandeep Panda <[email protected]>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 742 ++++++++++++++++++++++++++++++++++
 1 file changed, 742 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +0 -0
new file mode 100644
index 0000000..c798f2f

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 1

Quoted 1 line
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +1 -0
@@ -0,0 +1,742 @@
+/* SPDX-License-Identifier: GPL-2.0 */

There should be a copyright here.

Quoted 134 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +134 -0
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#define SN_BRIDGE_REVISION_ID 0x2
+
+/* Link Training specific registers */
+#define SN_DEVICE_REV_REG			0x08
+#define SN_REFCLK_FREQ_REG			0x0A
+#define SN_DSI_LANES_REG			0x10
+#define SN_DSIA_CLK_FREQ_REG			0x12
+#define SN_ENH_FRAME_REG			0x5A
+#define SN_SSC_CONFIG_REG			0x93
+#define SN_DATARATE_CONFIG_REG			0x94
+#define SN_PLL_ENABLE_REG			0x0D
+#define SN_SCRAMBLE_CONFIG_REG			0x95
+#define SN_AUX_WDATA0_REG			0x64
+#define SN_AUX_ADDR_19_16_REG			0x74
+#define SN_AUX_ADDR_15_8_REG			0x75
+#define SN_AUX_ADDR_7_0_REG			0x76
+#define SN_AUX_LENGTH_REG			0x77
+#define SN_AUX_CMD_REG				0x78
+#define SN_ML_TX_MODE_REG			0x96
+#define SN_PAGE_SELECT_REG			0xFF
+#define SN_AUX_RDATA0_REG			0x79
+/* video config specific registers */
+#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG	0x20
+#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG	0x21
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG	0x24
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG	0x25
+#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG	0x2C
+#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG	0x2D
+#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG	0x30
+#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG	0x31
+#define SN_CHA_HORIZONTAL_BACK_PORCH_REG	0x34
+#define SN_CHA_VERTICAL_BACK_PORCH_REG		0x36
+#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG	0x38
+#define SN_CHA_VERTICAL_FRONT_PORCH_REG		0x3A
+#define SN_DATA_FORMAT_REG			0x5B
+#define SN_COLOR_BAR_CONFIG_REG			0x3C
+
+#define DPPLL_CLK_SRC_REFCLK	0
+#define DPPLL_CLK_SRC_DSICLK	1
+#define MIN_DSI_CLK_FREQ_MHZ	40
+
+enum ti_sn_bridge_ref_clks {
+	SN_REF_CLK_12_MHZ = 0,
+	SN_REF_CLK_19_2_MHZ,
+	SN_REF_CLK_26_MHZ,
+	SN_REF_CLK_27_MHZ,
+	SN_REF_CLK_38_4_MHZ,
+	SN_REF_CLK_MAX,
+};
+
+struct ti_sn_bridge {
+	struct device *dev;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
+	/* handle to the connected eDP panel */
+	struct drm_panel *panel;
+	int irq;
+	struct gpio_desc *irq_gpio;
+	/* list of gpios needed to enable the bridge functionality */
+	struct gpio_descs *enable_gpios;
+	unsigned int num_supplies;
+	struct regulator_bulk_data *supplies;
+	struct i2c_client *i2c_client;
+	struct i2c_adapter *ddc;
+	bool power_on;
+	u32 num_modes;
+	struct drm_display_mode curr_mode;
+};
+
+static int ti_sn_bridge_write(struct ti_sn_bridge *pdata, u8 reg, u8 val)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	u8 buf[2] = {reg, val};
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = 0,
+		.len = 2,
+		.buf = buf,
+	};
+
+	if (i2c_transfer(client->adapter, &msg, 1) < 1) {
+		DRM_ERROR("i2c write failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ti_sn_bridge_read(struct ti_sn_bridge *pdata,
+					u8 reg, char *buf, u32 size)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	struct i2c_msg msg[2] = {
+		{
+			.addr = client->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = &reg,
+		},
+		{
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buf,
+		}
+	};
+
+	if (i2c_transfer(client->adapter, msg, 2) != 2) {
+		DRM_ERROR("i2c read failed\n");
+		return -EIO;
+	}
+

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 145

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	return 0;
+}
+
+static void ti_sn_bridge_gpio_configure(struct ti_sn_bridge *pdata, bool on)
+{
+	int value, i = 0, num = 0;
+	int *value_array = NULL;
+
+	if (!pdata)
+		return;

pdata should always be valid here.

Quoted 13 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +13 -0
+	value = on ? 1 : 0;
+	num = pdata->enable_gpios->ndescs;
+	value_array = kmalloc(num * sizeof(int), GFP_KERNEL);
+	if (value_array) {
+		for (i = 0; i < num; i++)
+			value_array[i] = value;
+		gpiod_set_array_value(num, pdata->enable_gpios->desc,
+				      value_array);
+		kfree(value_array);
+	}
+
+	if (pdata->irq_gpio)
+		gpiod_set_value(pdata->irq_gpio, value);

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 169

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+
+	DRM_DEBUG("config to: %d\n", value);
+}
+
+static void ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool enable)
+{
+	static int ref_count;
+
+	if (!pdata)
+		return;

pdata should always be valid here.

Quoted 111 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +111 -0
+	if (enable)
+		ref_count++;
+	else
+		ref_count--;
+
+	if (enable && (ref_count == 1)) {
+		ti_sn_bridge_gpio_configure(pdata, true);
+
+		if (regulator_bulk_enable(pdata->num_supplies,
+						pdata->supplies)) {
+			DRM_ERROR("bridge regulator enable failed\n");
+			return;
+		}
+		pdata->power_on = true;
+	} else if (!enable && !ref_count) {
+		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
+
+		ti_sn_bridge_gpio_configure(pdata, false);
+		pdata->power_on = false;
+	} else {
+		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
+							enable, ref_count);
+	}
+}
+
+/* Connector funcs */
+static struct ti_sn_bridge *
+connector_to_ti_sn_bridge(struct drm_connector *connector)
+{
+	return container_of(connector, struct ti_sn_bridge, connector);
+}
+
+static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+	struct drm_panel *panel = pdata->panel;
+	struct edid *edid;
+
+	if (panel) {
+		DRM_DEBUG("get mode from connected drm_panel\n");
+		pdata->num_modes = drm_panel_get_modes(panel);
+	} else {
+		/* get from EDID */
+		if (!pdata->ddc)
+			return 0;
+		ti_sn_bridge_power_ctrl(pdata, true);
+		edid = drm_get_edid(connector, pdata->ddc);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			pdata->num_modes = drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			kfree(edid);
+		} else {
+			DRM_DEBUG("failed to get edid\n");
+		}
+		ti_sn_bridge_power_ctrl(pdata, false);
+	}
+
+	return pdata->num_modes;
+}
+
+static enum drm_mode_status
+ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	/* maximum supported resolution is 4K at 60 fps */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
+	.get_modes = ti_sn_bridge_connector_get_modes,
+	.mode_valid = ti_sn_bridge_connector_mode_valid,
+};
+
+static enum drm_connector_status
+ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+
+	/**
+	 * TODO: Currently if drm_panel is present, then always
+	 * return the status as connected. Need to add support to detect
+	 * device state for no panel(hot pluggable) scenarios.
+	 */
+	if (pdata->panel)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+}
+
+static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = ti_sn_bridge_connector_detect,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct ti_sn_bridge, bridge);
+}
+
+static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
+{
+	u8 rev = 0;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 291

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	int ret = 0;
+
+	ret = ti_sn_bridge_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
+	if (ret)
+		goto exit;
+
+	if (rev == SN_BRIDGE_REVISION_ID) {
+		DRM_DEBUG("ti_sn_bridge revision id: 0x%x\n", rev);
+	} else {
+		DRM_ERROR("ti_sn_bridge revision id mismatch\n");

I find it useful to print what the mismatch is so you can debug more effectively.

Quoted 21 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +21 -0
+		ret = -EINVAL;
+	}
+
+exit:
+	return ret;
+}
+
+static const char * const ti_sn_bridge_supply_names[] = {
+	"vccio",
+	"vcca",
+};
+
+static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
+{
+	unsigned int i;
+	int ret;
+
+	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
+
+	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
+				     sizeof(*pdata->supplies), GFP_KERNEL);

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 322

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	if (!pdata->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < pdata->num_supplies; i++)
+		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
+
+	ret = devm_regulator_bulk_get(pdata->dev,
+			pdata->num_supplies, pdata->supplies);
+
+	return ret;

Nit: you can combine the above two lines and get rid of 'ret'.

Quoted 28 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +28 -0
+}
+
+static void ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
+{
+	struct device_node *panel_node, *port, *endpoint;
+	struct drm_panel *panel = NULL;
+
+	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
+	if (port) {
+		endpoint = of_get_child_by_name(port, "endpoint");
+		of_node_put(port);
+		if (!endpoint) {
+			dev_err(pdata->dev, "no output endpoint found\n");
+			goto error;
+		}
+
+		panel_node = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+		if (!panel_node) {
+			dev_err(pdata->dev, "no output node found\n");
+			goto error;
+		}
+
+		panel = of_drm_find_panel(panel_node);
+		of_node_put(panel_node);
+		if (!panel) {
+			dev_err(pdata->dev, "no panel node found\n");
+			goto error;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 360

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+		}
+	}
+
+	pdata->panel = panel;
+	drm_panel_attach(panel, &pdata->connector);
+
+	return;
+
+error:
+	pdata->panel = NULL;

Is this needed? isn't pdata->panel already null when we come in? If so you can just return the error cases and remove the label.

Quoted 5 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +5 -0
+}
+
+static int ti_sn_bridge_attach(struct drm_bridge *bridge)
+{
+	struct mipi_dsi_host *host;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 375

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	struct mipi_dsi_device *dsi;
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
+						   .channel = 0,
+						   .node = NULL,
+						 };
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");

Missing \n on the end of the string.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 384

Quoted 9 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +9 -0
+		return -ENODEV;
+	}
+
+	/* HPD not supported */
+	pdata->connector.polled = 0;
+
+	ret = drm_connector_init(bridge->dev, &pdata->connector,
+				 &ti_sn_bridge_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);

The actual #define is CONNNECTOR_eDP?

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 396

Quoted 12 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +12 -0
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&pdata->connector,
+				 &ti_sn_bridge_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&pdata->connector, bridge->encoder);
+
+	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+	if (!host) {
+		DRM_ERROR("failed to find dsi host\n");

There isn't any tear down that needs to happen here?

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 404

Quoted 8 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +8 -0
+		return -ENODEV;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		DRM_ERROR("failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		goto err_dsi_device;

Above you return directly but here you goto a return. You should use the same consistent approach in both cases.

Quoted 154 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +154 -0
+	}
+
+	/* TODO: setting to 4 lanes always for now */
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_ERROR("failed to attach dsi to host\n");
+		goto err_dsi_attach;
+	}
+
+	pdata->dsi = dsi;
+
+	DRM_DEBUG("bridge attached\n");
+	/* attach panel to bridge */
+	ti_sn_bridge_attach_panel(pdata);
+
+	return 0;
+
+err_dsi_attach:
+	mipi_dsi_device_unregister(dsi);
+err_dsi_device:
+	return ret;
+}
+
+static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj_mode)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
+		adj_mode->hdisplay, adj_mode->vdisplay,
+		adj_mode->vrefresh, adj_mode->clock);
+
+	drm_mode_copy(&pdata->curr_mode, adj_mode);
+}
+
+static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+
+	if (panel) {
+		drm_panel_disable(panel);
+		drm_panel_unprepare(panel);
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, false);
+}
+
+static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+	struct drm_display_mode *mode = &pdata->curr_mode;
+	u32 bit_rate_mhz, clk_freq_mhz;
+	u8 val = 0;
+
+	if (!pdata->num_modes)
+		return;
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+
+	if (panel)
+		drm_panel_prepare(panel);
+
+	/* CLK_SRC config */
+	ti_sn_bridge_write(pdata, SN_REFCLK_FREQ_REG,
+			(DPPLL_CLK_SRC_REFCLK | (SN_REF_CLK_19_2_MHZ << 0x1)));
+
+	/* DSI_A lane config */
+	ti_sn_bridge_read(pdata, SN_DSI_LANES_REG, &val, 0x1);
+	val |= ((0x4 - pdata->dsi->lanes) & 0x03) << 0x3;
+	ti_sn_bridge_write(pdata, SN_DSI_LANES_REG, val);
+
+	/* DP lane config */
+	ti_sn_bridge_read(pdata, SN_SSC_CONFIG_REG, &val, 0x1);
+	val |= ((pdata->dsi->lanes - 0x1) & 0x03) << 0x4;
+	ti_sn_bridge_write(pdata, SN_SSC_CONFIG_REG, val);
+
+	/* set dsi clk frequency value */
+	bit_rate_mhz = (mode->clock / 1000) *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+	/* for each increment in val, frequency increases by 5MHz */
+	val = (MIN_DSI_CLK_FREQ_MHZ / 0x5) +
+		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 0x5) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_DSIA_CLK_FREQ_REG, val);
+
+	ti_sn_bridge_write(pdata, SN_DATARATE_CONFIG_REG, 0x80); /* set HBR */
+
+	ti_sn_bridge_write(pdata, SN_PLL_ENABLE_REG, 0x1); /* Enable PLL */
+	usleep_range(10000, 10001);
+
+	/* DPCD Register 0x0010A in Sink */
+	ti_sn_bridge_write(pdata, SN_AUX_WDATA0_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_19_16_REG, 0x00);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_15_8_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_7_0_REG, 0x0A);
+	ti_sn_bridge_write(pdata, SN_AUX_LENGTH_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_CMD_REG, 0x81);
+	usleep_range(10000, 10001);
+
+	/* Semi auto link training mode */
+	ti_sn_bridge_write(pdata, SN_ML_TX_MODE_REG, 0x0A);
+	usleep_range(20000, 20001);
+
+	/* config video parameters */
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
+			mode->hdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
+			mode->vdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->htotal - mode->hsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->htotal - mode->hsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->vtotal - mode->vsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->vtotal - mode->vsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
+			(mode->hsync_start - mode->hdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_BACK_PORCH_REG,
+			(mode->vsync_start - mode->vdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
+			(mode->hsync_end - mode->hsync_start) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_FRONT_PORCH_REG,
+			(mode->vsync_end - mode->vsync_start) & 0xFF);
+	if (pdata->dsi->format == MIPI_DSI_FMT_RGB888)
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x00);
+	else
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x01);
+	usleep_range(10000, 10001);
+
+	/* enable video stream */
+	ti_sn_bridge_read(pdata, SN_ENH_FRAME_REG, &val, 0x1);
+	val |= BIT(3);
+	ti_sn_bridge_write(pdata, SN_ENH_FRAME_REG, val);
+
+	if (panel)
+		drm_panel_enable(panel);
+}
+
+static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
+	.attach = ti_sn_bridge_attach,
+	.enable = ti_sn_bridge_enable,

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 568

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	.disable = ti_sn_bridge_disable,
+	.mode_set = ti_sn_bridge_mode_set,
+};
+
+static int ti_sn_bridge_parse_gpios(struct ti_sn_bridge *pdata)
+{
+	int ret = 0;
+
+	if (!pdata || !pdata->dev)
+		return -EINVAL;

Both of these are known to be valid at this point.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 581

Quoted 12 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +12 -0
+	pdata->enable_gpios = devm_gpiod_get_array(pdata->dev,
+						"enable", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->enable_gpios)) {
+		DRM_ERROR("failed to get enable gpio from DT\n");
+		ret = PTR_ERR(pdata->enable_gpios);
+		goto exit;
+	}
+
+	pdata->irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->irq_gpio))
+		DRM_ERROR("failed to get irq gpio from DT\n");

You need to set ret to PTR_ERR(pdata->irq_gpio) here.

Quoted 25 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +25 -0
+
+exit:
+	return ret;
+}
+
+static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
+{
+	struct device_node *np = pdata->dev->of_node;
+	struct device_node *end_node;
+
+	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
+	if (!end_node) {
+		DRM_ERROR("remote endpoint not found\n");
+		return -ENODEV;
+	}
+
+	pdata->host_node = of_graph_get_remote_port_parent(end_node);
+	of_node_put(end_node);
+	if (!pdata->host_node) {
+		DRM_ERROR("remote node not found\n");
+		return -ENODEV;
+	}
+	of_node_put(pdata->host_node);
+
+	return 0;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 616

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+}
+
+static int ti_sn_bridge_probe(struct i2c_client *client,
+	 const struct i2c_device_id *id)
+{
+	struct ti_sn_bridge *pdata;
+	struct device_node *ddc_node;
+	int ret = 0;
+
+	if (!client || !client->dev.of_node) {

I doubt the i2c probe would send you a NULL value for client and you are matching on a DT device, so there is a pretty solid argument that client->dev.of_node will be valid as well.

Quoted 12 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +12 -0
+		DRM_ERROR("invalid input\n");
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		DRM_ERROR("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	pdata = devm_kzalloc(&client->dev,
+		sizeof(struct ti_sn_bridge), GFP_KERNEL);
+	if (!pdata)

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 638

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+		return -ENOMEM;
+
+	pdata->power_on = false;
+	pdata->dev = &client->dev;
+	pdata->i2c_client = client;
+	DRM_DEBUG("I2C address is %x\n", client->addr);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse device tree\n");

ti_sn_bridge_parse_dsi_host prints a error message on every failure point - you don't need another one here.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 644

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_gpios(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse gpios from DT\n");

Same - all the paths are covered in the child function.

Quoted 15 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +15 -0
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_regulators(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse regulators\n");
+		goto err_dt_parse;
+	}
+
+	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+		if (!pdata->ddc) {
+			dev_dbg(pdata->dev, "failed to read ddc node\n");

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 669

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+			return -EPROBE_DEFER;
+		}
+	} else {
+		dev_dbg(pdata->dev, "no ddc property found\n");
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+	ret = ti_sn_bridge_read_device_rev(pdata);
+	if (ret) {
+		DRM_ERROR("failed to read chip rev\n");

ti_sn_bridge_read_device_rev has you covered with the error messages.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 673

Quoted 4 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +4 -0
+		goto err_config;
+	} else {
+		DRM_DEBUG("bridge chip enabled successfully\n");
+	}

And also the debug messages for whatever its worth.

Quoted 16 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +16 -0
+	ti_sn_bridge_power_ctrl(pdata, false);
+
+	i2c_set_clientdata(client, pdata);
+	dev_set_drvdata(&client->dev, pdata);
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = client->dev.of_node;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return ret;
+
+err_config:
+	ti_sn_bridge_power_ctrl(pdata, false);
+err_dt_parse:
+	devm_kfree(&client->dev, pdata);

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 699

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	return ret;
+}
+
+static int ti_sn_bridge_remove(struct i2c_client *client)
+{
+	int ret = -EINVAL;
+	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
+
+	if (!pdata)
+		goto end;

pdata is very unlikely to be null here, but as long as this is a light weight check, I don't think it hurts too much.

Quoted 11 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +11 -0
+
+	mipi_dsi_detach(pdata->dsi);
+	mipi_dsi_device_unregister(pdata->dsi);
+
+	drm_bridge_remove(&pdata->bridge);
+
+	disable_irq(pdata->irq);
+	free_irq(pdata->irq, pdata);
+
+	ti_sn_bridge_gpio_configure(pdata, false);
+	i2c_put_adapter(pdata->ddc);

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 720

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+
+	devm_kfree(&client->dev, pdata);
+
+end:
+	return ret;
+}
+
+static struct i2c_device_id ti_sn_bridge_id[] = {
+	{ "ti,sn65dsi86", 0},
+	{}

Missing a comma on the end of the empty entry.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 726

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+};
+MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+
+static const struct of_device_id ti_sn_bridge_match_table[] = {
+	{.compatible = "ti,sn65dsi86"},
+	{}

Same.

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+};
+MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
+
+static struct i2c_driver ti_sn_bridge_driver = {
+	.driver = {
+		.name = "ti_sn65dsi86",

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 742

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+		.owner = THIS_MODULE,
+		.of_match_table = ti_sn_bridge_match_table,
+	},
+	.probe = ti_sn_bridge_probe,
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id,
+};
+
+module_i2c_driver(ti_sn_bridge_driver);
+MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");

Looks like you are missing the module license here.

Quoted 8 lines
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 6 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

A message that is not in the archive (sent off-list or not yet received): 20180419151438.GA18652-9PYrDHPZ2Orvke4nUoYGnHL1okKdlPRT@public.gmane.org

author
#9 lore raw

Re: [[RFC]DPU PATCH 1/4] drm/bridge: add support for sn65dsi86 bridge driver

Quoted 402 lines · Jordan Crouse
Quoted 23 lines · Sandeep Panda

Add support for TI's sn65dsi86 dsi2edp bridge chip. The chip converts DSI transmitted signal to eDP signal, which is fed to the connected eDP panel.

This chip can be controlled via either i2c interface or dsi interface. Currently in driver all the control registers are being accessed through i2c interface only. Also as of now HPD support has not been added to bridge chip driver.

Signed-off-by Sandeep Panda <[email protected]>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 742 
++++++++++++++++++++++++++++++++++
 1 file changed, 742 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +0 -0
new file mode 100644
index 0000000..c798f2f

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 1

Quoted 1 line
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +1 -0
@@ -0,0 +1,742 @@
+/* SPDX-License-Identifier: GPL-2.0 */

There should be a copyright here.

Quoted 140 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +92 -0
+
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/gpio.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+
+#define SN_BRIDGE_REVISION_ID 0x2
+
+/* Link Training specific registers */
+#define SN_DEVICE_REV_REG			0x08
+#define SN_REFCLK_FREQ_REG			0x0A
+#define SN_DSI_LANES_REG			0x10
+#define SN_DSIA_CLK_FREQ_REG			0x12
+#define SN_ENH_FRAME_REG			0x5A
+#define SN_SSC_CONFIG_REG			0x93
+#define SN_DATARATE_CONFIG_REG			0x94
+#define SN_PLL_ENABLE_REG			0x0D
+#define SN_SCRAMBLE_CONFIG_REG			0x95
+#define SN_AUX_WDATA0_REG			0x64
+#define SN_AUX_ADDR_19_16_REG			0x74
+#define SN_AUX_ADDR_15_8_REG			0x75
+#define SN_AUX_ADDR_7_0_REG			0x76
+#define SN_AUX_LENGTH_REG			0x77
+#define SN_AUX_CMD_REG				0x78
+#define SN_ML_TX_MODE_REG			0x96
+#define SN_PAGE_SELECT_REG			0xFF
+#define SN_AUX_RDATA0_REG			0x79
+/* video config specific registers */
+#define SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG	0x20
+#define SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG	0x21
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG	0x24
+#define SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG	0x25
+#define SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG	0x2C
+#define SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG	0x2D
+#define SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG	0x30
+#define SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG	0x31
+#define SN_CHA_HORIZONTAL_BACK_PORCH_REG	0x34
+#define SN_CHA_VERTICAL_BACK_PORCH_REG		0x36
+#define SN_CHA_HORIZONTAL_FRONT_PORCH_REG	0x38
+#define SN_CHA_VERTICAL_FRONT_PORCH_REG		0x3A
+#define SN_DATA_FORMAT_REG			0x5B
+#define SN_COLOR_BAR_CONFIG_REG			0x3C
+
+#define DPPLL_CLK_SRC_REFCLK	0
+#define DPPLL_CLK_SRC_DSICLK	1
+#define MIN_DSI_CLK_FREQ_MHZ	40
+
+enum ti_sn_bridge_ref_clks {
+	SN_REF_CLK_12_MHZ = 0,
+	SN_REF_CLK_19_2_MHZ,
+	SN_REF_CLK_26_MHZ,
+	SN_REF_CLK_27_MHZ,
+	SN_REF_CLK_38_4_MHZ,
+	SN_REF_CLK_MAX,
+};
+
+struct ti_sn_bridge {
+	struct device *dev;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
+	/* handle to the connected eDP panel */
+	struct drm_panel *panel;
+	int irq;
+	struct gpio_desc *irq_gpio;
+	/* list of gpios needed to enable the bridge functionality */
+	struct gpio_descs *enable_gpios;
+	unsigned int num_supplies;
+	struct regulator_bulk_data *supplies;
+	struct i2c_client *i2c_client;
+	struct i2c_adapter *ddc;
+	bool power_on;
+	u32 num_modes;
+	struct drm_display_mode curr_mode;
+};
+
+static int ti_sn_bridge_write(struct ti_sn_bridge *pdata, u8 reg, u8

val)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +46 -0
+{
+	struct i2c_client *client = pdata->i2c_client;
+	u8 buf[2] = {reg, val};
+	struct i2c_msg msg = {
+		.addr = client->addr,
+		.flags = 0,
+		.len = 2,
+		.buf = buf,
+	};
+
+	if (i2c_transfer(client->adapter, &msg, 1) < 1) {
+		DRM_ERROR("i2c write failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ti_sn_bridge_read(struct ti_sn_bridge *pdata,
+					u8 reg, char *buf, u32 size)
+{
+	struct i2c_client *client = pdata->i2c_client;
+	struct i2c_msg msg[2] = {
+		{
+			.addr = client->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = &reg,
+		},
+		{
+			.addr = client->addr,
+			.flags = I2C_M_RD,
+			.len = size,
+			.buf = buf,
+		}
+	};
+
+	if (i2c_transfer(client->adapter, msg, 2) != 2) {
+		DRM_ERROR("i2c read failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ti_sn_bridge_gpio_configure(struct ti_sn_bridge *pdata,

bool on)

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 145

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+{
+	int value, i = 0, num = 0;
+	int *value_array = NULL;
+
+	if (!pdata)
+		return;

pdata should always be valid here.

Quoted 19 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +18 -0
+	value = on ? 1 : 0;
+	num = pdata->enable_gpios->ndescs;
+	value_array = kmalloc(num * sizeof(int), GFP_KERNEL);
+	if (value_array) {
+		for (i = 0; i < num; i++)
+			value_array[i] = value;
+		gpiod_set_array_value(num, pdata->enable_gpios->desc,
+				      value_array);
+		kfree(value_array);
+	}
+
+	if (pdata->irq_gpio)
+		gpiod_set_value(pdata->irq_gpio, value);
+
+	DRM_DEBUG("config to: %d\n", value);
+}
+
+static void ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool

enable)

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 169

Quoted 5 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +5 -0
+{
+	static int ref_count;
+
+	if (!pdata)
+		return;

pdata should always be valid here.

Quoted 117 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +33 -0
+	if (enable)
+		ref_count++;
+	else
+		ref_count--;
+
+	if (enable && (ref_count == 1)) {
+		ti_sn_bridge_gpio_configure(pdata, true);
+
+		if (regulator_bulk_enable(pdata->num_supplies,
+						pdata->supplies)) {
+			DRM_ERROR("bridge regulator enable failed\n");
+			return;
+		}
+		pdata->power_on = true;
+	} else if (!enable && !ref_count) {
+		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
+
+		ti_sn_bridge_gpio_configure(pdata, false);
+		pdata->power_on = false;
+	} else {
+		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
+							enable, ref_count);
+	}
+}
+
+/* Connector funcs */
+static struct ti_sn_bridge *
+connector_to_ti_sn_bridge(struct drm_connector *connector)
+{
+	return container_of(connector, struct ti_sn_bridge, connector);
+}
+
+static int ti_sn_bridge_connector_get_modes(struct drm_connector

*connector)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +41 -0
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+	struct drm_panel *panel = pdata->panel;
+	struct edid *edid;
+
+	if (panel) {
+		DRM_DEBUG("get mode from connected drm_panel\n");
+		pdata->num_modes = drm_panel_get_modes(panel);
+	} else {
+		/* get from EDID */
+		if (!pdata->ddc)
+			return 0;
+		ti_sn_bridge_power_ctrl(pdata, true);
+		edid = drm_get_edid(connector, pdata->ddc);
+		if (edid) {
+			drm_mode_connector_update_edid_property(connector,
+								edid);
+			pdata->num_modes = drm_add_edid_modes(connector, edid);
+			drm_edid_to_eld(connector, edid);
+			kfree(edid);
+		} else {
+			DRM_DEBUG("failed to get edid\n");
+		}
+		ti_sn_bridge_power_ctrl(pdata, false);
+	}
+
+	return pdata->num_modes;
+}
+
+static enum drm_mode_status
+ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	/* maximum supported resolution is 4K at 60 fps */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static struct drm_connector_helper_funcs

ti_sn_bridge_connector_helper_funcs = {

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+	.get_modes = ti_sn_bridge_connector_get_modes,
+	.mode_valid = ti_sn_bridge_connector_mode_valid,
+};
+
+static enum drm_connector_status
+ti_sn_bridge_connector_detect(struct drm_connector *connector, bool

force)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +15 -0
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+
+	/**
+	 * TODO: Currently if drm_panel is present, then always
+	 * return the status as connected. Need to add support to detect
+	 * device state for no panel(hot pluggable) scenarios.
+	 */
+	if (pdata->panel)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
+}
+
+static const struct drm_connector_funcs ti_sn_bridge_connector_funcs

= {

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +5 -0
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = ti_sn_bridge_connector_detect,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state =

drm_atomic_helper_connector_duplicate_state,

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +4 -0
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge

*bridge)

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +7 -0
+{
+	return container_of(bridge, struct ti_sn_bridge, bridge);
+}
+
+static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
+{
+	u8 rev = 0;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 291

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	int ret = 0;
+
+	ret = ti_sn_bridge_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
+	if (ret)
+		goto exit;
+
+	if (rev == SN_BRIDGE_REVISION_ID) {
+		DRM_DEBUG("ti_sn_bridge revision id: 0x%x\n", rev);
+	} else {
+		DRM_ERROR("ti_sn_bridge revision id mismatch\n");

I find it useful to print what the mismatch is so you can debug more effectively.

Quoted 21 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +21 -0
+		ret = -EINVAL;
+	}
+
+exit:
+	return ret;
+}
+
+static const char * const ti_sn_bridge_supply_names[] = {
+	"vccio",
+	"vcca",
+};
+
+static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
+{
+	unsigned int i;
+	int ret;
+
+	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
+
+	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
+				     sizeof(*pdata->supplies), GFP_KERNEL);

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 322

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	if (!pdata->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < pdata->num_supplies; i++)
+		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
+
+	ret = devm_regulator_bulk_get(pdata->dev,
+			pdata->num_supplies, pdata->supplies);
+
+	return ret;

Nit: you can combine the above two lines and get rid of 'ret'.

Quoted 33 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +33 -0
+}
+
+static void ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
+{
+	struct device_node *panel_node, *port, *endpoint;
+	struct drm_panel *panel = NULL;
+
+	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
+	if (port) {
+		endpoint = of_get_child_by_name(port, "endpoint");
+		of_node_put(port);
+		if (!endpoint) {
+			dev_err(pdata->dev, "no output endpoint found\n");
+			goto error;
+		}
+
+		panel_node = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+		if (!panel_node) {
+			dev_err(pdata->dev, "no output node found\n");
+			goto error;
+		}
+
+		panel = of_drm_find_panel(panel_node);
+		of_node_put(panel_node);
+		if (!panel) {
+			dev_err(pdata->dev, "no panel node found\n");
+			goto error;
+		}
+	}
+
+	pdata->panel = panel;
+	drm_panel_attach(panel, &pdata->connector);
Quoted 8 lines

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 360

Quoted 5 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +5 -0
+
+	return;
+
+error:
+	pdata->panel = NULL;

Is this needed? isn't pdata->panel already null when we come in? If so you can just return the error cases and remove the label.

Just making sure it is NULL, not some uninitialized value, since this will be used through out the driver to detect if a panel is connected or not and HDP needs to be supported or not.

Quoted 19 lines
Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +5 -0
+}
+
+static int ti_sn_bridge_attach(struct drm_bridge *bridge)
+{
+	struct mipi_dsi_host *host;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 375

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	struct mipi_dsi_device *dsi;
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
+						   .channel = 0,
+						   .node = NULL,
+						 };
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");

Missing \n on the end of the string.

Quoted 11 lines

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 384

Quoted 9 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +9 -0
+		return -ENODEV;
+	}
+
+	/* HPD not supported */
+	pdata->connector.polled = 0;
+
+	ret = drm_connector_init(bridge->dev, &pdata->connector,
+				 &ti_sn_bridge_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);

The actual #define is CONNNECTOR_eDP?

yes.

Quoted 397 lines
Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +8 -0
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&pdata->connector,
+				 &ti_sn_bridge_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&pdata->connector,

bridge->encoder);

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 396

Quoted 4 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +4 -0
+
+	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+	if (!host) {
+		DRM_ERROR("failed to find dsi host\n");

There isn't any tear down that needs to happen here?

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 404

Quoted 8 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +8 -0
+		return -ENODEV;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		DRM_ERROR("failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		goto err_dsi_device;

Above you return directly but here you goto a return. You should use the same consistent approach in both cases.

Quoted 157 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+	}
+
+	/* TODO: setting to 4 lanes always for now */
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO |

MIPI_DSI_MODE_VIDEO_SYNC_PULSE |

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +29 -0
+			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_ERROR("failed to attach dsi to host\n");
+		goto err_dsi_attach;
+	}
+
+	pdata->dsi = dsi;
+
+	DRM_DEBUG("bridge attached\n");
+	/* attach panel to bridge */
+	ti_sn_bridge_attach_panel(pdata);
+
+	return 0;
+
+err_dsi_attach:
+	mipi_dsi_device_unregister(dsi);
+err_dsi_device:
+	return ret;
+}
+
+static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj_mode)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d,

clock=%d\n",

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +59 -0
+		adj_mode->hdisplay, adj_mode->vdisplay,
+		adj_mode->vrefresh, adj_mode->clock);
+
+	drm_mode_copy(&pdata->curr_mode, adj_mode);
+}
+
+static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+
+	if (panel) {
+		drm_panel_disable(panel);
+		drm_panel_unprepare(panel);
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, false);
+}
+
+static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+	struct drm_display_mode *mode = &pdata->curr_mode;
+	u32 bit_rate_mhz, clk_freq_mhz;
+	u8 val = 0;
+
+	if (!pdata->num_modes)
+		return;
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+
+	if (panel)
+		drm_panel_prepare(panel);
+
+	/* CLK_SRC config */
+	ti_sn_bridge_write(pdata, SN_REFCLK_FREQ_REG,
+			(DPPLL_CLK_SRC_REFCLK | (SN_REF_CLK_19_2_MHZ << 0x1)));
+
+	/* DSI_A lane config */
+	ti_sn_bridge_read(pdata, SN_DSI_LANES_REG, &val, 0x1);
+	val |= ((0x4 - pdata->dsi->lanes) & 0x03) << 0x3;
+	ti_sn_bridge_write(pdata, SN_DSI_LANES_REG, val);
+
+	/* DP lane config */
+	ti_sn_bridge_read(pdata, SN_SSC_CONFIG_REG, &val, 0x1);
+	val |= ((pdata->dsi->lanes - 0x1) & 0x03) << 0x4;
+	ti_sn_bridge_write(pdata, SN_SSC_CONFIG_REG, val);
+
+	/* set dsi clk frequency value */
+	bit_rate_mhz = (mode->clock / 1000) *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+	/* for each increment in val, frequency increases by 5MHz */
+	val = (MIN_DSI_CLK_FREQ_MHZ / 0x5) +
+		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 0x5) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_DSIA_CLK_FREQ_REG, val);
+
+	ti_sn_bridge_write(pdata, SN_DATARATE_CONFIG_REG, 0x80); /* set HBR

*/

drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +60 -0
+
+	ti_sn_bridge_write(pdata, SN_PLL_ENABLE_REG, 0x1); /* Enable PLL */
+	usleep_range(10000, 10001);
+
+	/* DPCD Register 0x0010A in Sink */
+	ti_sn_bridge_write(pdata, SN_AUX_WDATA0_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_19_16_REG, 0x00);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_15_8_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_ADDR_7_0_REG, 0x0A);
+	ti_sn_bridge_write(pdata, SN_AUX_LENGTH_REG, 0x01);
+	ti_sn_bridge_write(pdata, SN_AUX_CMD_REG, 0x81);
+	usleep_range(10000, 10001);
+
+	/* Semi auto link training mode */
+	ti_sn_bridge_write(pdata, SN_ML_TX_MODE_REG, 0x0A);
+	usleep_range(20000, 20001);
+
+	/* config video parameters */
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
+			mode->hdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
+			mode->vdisplay & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
+			(mode->hdisplay >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->htotal - mode->hsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->htotal - mode->hsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->vtotal - mode->vsync_end) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->vtotal - mode->vsync_end) >> 0x8) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
+			(mode->hsync_start - mode->hdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_BACK_PORCH_REG,
+			(mode->vsync_start - mode->vdisplay) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
+			(mode->hsync_end - mode->hsync_start) & 0xFF);
+	ti_sn_bridge_write(pdata, SN_CHA_VERTICAL_FRONT_PORCH_REG,
+			(mode->vsync_end - mode->vsync_start) & 0xFF);
+	if (pdata->dsi->format == MIPI_DSI_FMT_RGB888)
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x00);
+	else
+		ti_sn_bridge_write(pdata, SN_DATA_FORMAT_REG, 0x01);
+	usleep_range(10000, 10001);
+
+	/* enable video stream */
+	ti_sn_bridge_read(pdata, SN_ENH_FRAME_REG, &val, 0x1);
+	val |= BIT(3);
+	ti_sn_bridge_write(pdata, SN_ENH_FRAME_REG, val);
+
+	if (panel)
+		drm_panel_enable(panel);
+}
+
+static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
+	.attach = ti_sn_bridge_attach,
+	.enable = ti_sn_bridge_enable,

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 568

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	.disable = ti_sn_bridge_disable,
+	.mode_set = ti_sn_bridge_mode_set,
+};
+
+static int ti_sn_bridge_parse_gpios(struct ti_sn_bridge *pdata)
+{
+	int ret = 0;
+
+	if (!pdata || !pdata->dev)
+		return -EINVAL;

Both of these are known to be valid at this point.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 581

Quoted 12 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +12 -0
+	pdata->enable_gpios = devm_gpiod_get_array(pdata->dev,
+						"enable", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->enable_gpios)) {
+		DRM_ERROR("failed to get enable gpio from DT\n");
+		ret = PTR_ERR(pdata->enable_gpios);
+		goto exit;
+	}
+
+	pdata->irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
+						  GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->irq_gpio))
+		DRM_ERROR("failed to get irq gpio from DT\n");

You need to set ret to PTR_ERR(pdata->irq_gpio) here.

Quoted 25 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +25 -0
+
+exit:
+	return ret;
+}
+
+static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
+{
+	struct device_node *np = pdata->dev->of_node;
+	struct device_node *end_node;
+
+	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
+	if (!end_node) {
+		DRM_ERROR("remote endpoint not found\n");
+		return -ENODEV;
+	}
+
+	pdata->host_node = of_graph_get_remote_port_parent(end_node);
+	of_node_put(end_node);
+	if (!pdata->host_node) {
+		DRM_ERROR("remote node not found\n");
+		return -ENODEV;
+	}
+	of_node_put(pdata->host_node);
+
+	return 0;

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 616

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+}
+
+static int ti_sn_bridge_probe(struct i2c_client *client,
+	 const struct i2c_device_id *id)
+{
+	struct ti_sn_bridge *pdata;
+	struct device_node *ddc_node;
+	int ret = 0;
+
+	if (!client || !client->dev.of_node) {

I doubt the i2c probe would send you a NULL value for client and you are matching on a DT device, so there is a pretty solid argument that client->dev.of_node will be valid as well.

Quoted 12 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +12 -0
+		DRM_ERROR("invalid input\n");
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		DRM_ERROR("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	pdata = devm_kzalloc(&client->dev,
+		sizeof(struct ti_sn_bridge), GFP_KERNEL);
+	if (!pdata)

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 638

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+		return -ENOMEM;
+
+	pdata->power_on = false;
+	pdata->dev = &client->dev;
+	pdata->i2c_client = client;
+	DRM_DEBUG("I2C address is %x\n", client->addr);
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse device tree\n");

ti_sn_bridge_parse_dsi_host prints a error message on every failure point - you don't need another one here.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 644

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_gpios(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse gpios from DT\n");

Same - all the paths are covered in the child function.

Quoted 15 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +15 -0
+		goto err_dt_parse;
+	}
+
+	ret = ti_sn_bridge_parse_regulators(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse regulators\n");
+		goto err_dt_parse;
+	}
+
+	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+		if (!pdata->ddc) {
+			dev_dbg(pdata->dev, "failed to read ddc node\n");

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 669

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+			return -EPROBE_DEFER;
+		}
+	} else {
+		dev_dbg(pdata->dev, "no ddc property found\n");
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+	ret = ti_sn_bridge_read_device_rev(pdata);
+	if (ret) {
+		DRM_ERROR("failed to read chip rev\n");

ti_sn_bridge_read_device_rev has you covered with the error messages.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 673

Quoted 4 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +4 -0
+		goto err_config;
+	} else {
+		DRM_DEBUG("bridge chip enabled successfully\n");
+	}

And also the debug messages for whatever its worth.

Quoted 16 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +16 -0
+	ti_sn_bridge_power_ctrl(pdata, false);
+
+	i2c_set_clientdata(client, pdata);
+	dev_set_drvdata(&client->dev, pdata);
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = client->dev.of_node;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return ret;
+
+err_config:
+	ti_sn_bridge_power_ctrl(pdata, false);
+err_dt_parse:
+	devm_kfree(&client->dev, pdata);

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 699

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+	return ret;
+}
+
+static int ti_sn_bridge_remove(struct i2c_client *client)
+{
+	int ret = -EINVAL;
+	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
+
+	if (!pdata)
+		goto end;

pdata is very unlikely to be null here, but as long as this is a light weight check, I don't think it hurts too much.

Quoted 11 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +11 -0
+
+	mipi_dsi_detach(pdata->dsi);
+	mipi_dsi_device_unregister(pdata->dsi);
+
+	drm_bridge_remove(&pdata->bridge);
+
+	disable_irq(pdata->irq);
+	free_irq(pdata->irq, pdata);
+
+	ti_sn_bridge_gpio_configure(pdata, false);
+	i2c_put_adapter(pdata->ddc);

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 720

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+
+	devm_kfree(&client->dev, pdata);
+
+end:
+	return ret;
+}
+
+static struct i2c_device_id ti_sn_bridge_id[] = {
+	{ "ti,sn65dsi86", 0},
+	{}

Missing a comma on the end of the empty entry.

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 726

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+};
+MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+
+static const struct of_device_id ti_sn_bridge_match_table[] = {
+	{.compatible = "ti,sn65dsi86"},
+	{}

Same.

Quoted 10 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +10 -0
+};
+MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
+
+static struct i2c_driver ti_sn_bridge_driver = {
+	.driver = {
+		.name = "ti_sn65dsi86",
+		.owner = THIS_MODULE,
+		.of_match_table = ti_sn_bridge_match_table,
+	},
+	.probe = ti_sn_bridge_probe,
Quoted 17 lines

drivers/gpu/drm/bridge/ti-sn65dsi86.c line 742

Quoted 6 lines
drivers/gpu/drm/bridge/ti-sn65dsi86.c new file +6 -0
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id,
+};
+
+module_i2c_driver(ti_sn_bridge_driver);
+MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");

Looks like you are missing the module license here.

Quoted 8 lines

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

author
#10 lore raw

RFC 1/1 DPU PATCH 2/4] dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings

In reply to a message not in the archive

Document the bindings used for the sn65dsi86 DSI to eDP bridge.

Signed-off-by Sandeep Panda <[email protected]>
---
 .../bindings/display/bridge/ti,sn65dsi86.txt       | 60 ++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +60 -0
new file mode 100644
index 0000000..9e2612e
@@ -0,0 +1,60 @@
+SN65DSI86 DSI to eDP bridge chip
+--------------------------------
+
+This is the binding for Texas Instruments SN65DSI86 bridge.
+
+Required properties:
+- compatible: Must be "ti,sn65dsi86"
+- reg: i2c address of the chip, 0x2d as per datasheet
+- enable-gpios: OF device-tree gpio specifications for bridge_en pin
+
+- vccio-supply: A 1.8V supply that powers up the digital IOs.
+- vcca-supply: A 1.2V supply that powers up the analog circuits.
+
+Optional properties:
+
+- irq-gpios: OF device-tree gpio specification for interrupt pin
+
+Required nodes:
+
+This device has two video ports. Their connections are modelled using the
+OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for DSI input
+- Video port 1 for eDP output
+
+Example
+-------
+
+edp-bridge@2d {
+	compatible = "ti,sn65dsi86";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x2d>;
+
+	enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>;
+
+	vccio-supply = <&pm8916_l17>;
+	vcca-supply = <&pm8916_l6>;
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			edp_bridge_in: endpoint {
+				remote-endpoint = <&dsi_out>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			edp_bridge_out: endpoint {
+				remote-endpoint = <&edp_panel_in>;
+			};
+		};
+	};
+}
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
#11 lore raw

Re: [[RFC]DPU PATCH 2/4] dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings

Quoted 2 lines · Sandeep Panda

Document the bindings used for the sn65dsi86 DSI to eDP bridge.

Please add a changelog to your patches so reviewers know what has changed between patch versions.

Quoted 12 lines
Signed-off-by Sandeep Panda <[email protected]>
---
 .../bindings/display/bridge/ti,sn65dsi86.txt       | 60 ++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +0 -0
new file mode 100644
index 0000000..9e2612e

Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt line 9

Quoted 9 lines
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +9 -0
@@ -0,0 +1,60 @@
+SN65DSI86 DSI to eDP bridge chip
+--------------------------------
+
+This is the binding for Texas Instruments SN65DSI86 bridge.
+
+Required properties:
+- compatible: Must be "ti,sn65dsi86"
+- reg: i2c address of the chip, 0x2d as per datasheet
+- enable-gpios: OF device-tree gpio specifications for bridge_en pin

The datasheet only has one enable pin, why gpios?

Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt line 16

Quoted 7 lines
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +7 -0
+
+- vccio-supply: A 1.8V supply that powers up the digital IOs.
+- vcca-supply: A 1.2V supply that powers up the analog circuits.
+
+Optional properties:
+
+- irq-gpios: OF device-tree gpio specification for interrupt pin

From the last review, Rob said, 'Use "interrupts" property instead.'

You didn't provide responses to prior review comments, so it's unclear whether you've intentionally or unintentionally ignored him :)

Sean

Quoted 48 lines
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +44 -0
+
+Required nodes:
+
+This device has two video ports. Their connections are modelled using the
+OF graph bindings specified in Documentation/devicetree/bindings/graph.txt.
+
+- Video port 0 for DSI input
+- Video port 1 for eDP output
+
+Example
+-------
+
+edp-bridge@2d {
+	compatible = "ti,sn65dsi86";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x2d>;
+
+	enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>;
+
+	vccio-supply = <&pm8916_l17>;
+	vcca-supply = <&pm8916_l6>;
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			edp_bridge_in: endpoint {
+				remote-endpoint = <&dsi_out>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			edp_bridge_out: endpoint {
+				remote-endpoint = <&edp_panel_in>;
+			};
+		};
+	};
+}
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#12 lore raw

Re: [[RFC]DPU PATCH 2/4] dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings

Quoted 6 lines · Sean Paul
Quoted 2 lines · Sandeep Panda

Document the bindings used for the sn65dsi86 DSI to eDP bridge.

Please add a changelog to your patches so reviewers know what has changed between patch versions.

I will update the change log in the commit text of the patches.

Quoted 19 lines
Quoted 19 lines
Signed-off-by Sandeep Panda <[email protected]>
---
 .../bindings/display/bridge/ti,sn65dsi86.txt       | 60 
++++++++++++++++++++++
 1 file changed, 60 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt

(quoted diff) +0 -0
new file mode 100644
index 0000000..9e2612e

b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt

Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +3 -0
@@ -0,0 +1,60 @@
+SN65DSI86 DSI to eDP bridge chip
+--------------------------------
+
Quoted 8 lines

Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt line 9

Quoted 6 lines
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +6 -0
+This is the binding for Texas Instruments SN65DSI86 bridge.
+
+Required properties:
+- compatible: Must be "ti,sn65dsi86"
+- reg: i2c address of the chip, 0x2d as per datasheet
+- enable-gpios: OF device-tree gpio specifications for bridge_en pin

The datasheet only has one enable pin, why gpios?

This is the convention followed by other bridge drivers also.
https://www.kernel.org/doc/Documentation/devicetree/bindings/gpio/gpio.txt
please the example mentioned here,
Example of a node using GPIOs:

	node {
		enable-gpios = <&qe_pio_e 18 GPIO_ACTIVE_HIGH>;
	};
Quoted 11 lines

Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt line 16

Quoted 8 lines
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +7 -0
+
+- vccio-supply: A 1.8V supply that powers up the digital IOs.
+- vcca-supply: A 1.2V supply that powers up the analog circuits.
+
+Optional properties:
+
+- irq-gpios: OF device-tree gpio specification for interrupt pin

From the last review, Rob said, 'Use "interrupts" property instead.'

Quoted 2 lines

You didn't provide responses to prior review comments, so it's unclear whether you've intentionally or unintentionally ignored him :)

Sorry missed it completely while addressing other review comments.

Quoted 45 lines

Sean

Quoted 42 lines
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +4 -0
+
+Required nodes:
+
+This device has two video ports. Their connections are modelled using

the
+OF graph bindings specified in
Documentation/devicetree/bindings/graph.txt.

Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +35 -0
+
+- Video port 0 for DSI input
+- Video port 1 for eDP output
+
+Example
+-------
+
+edp-bridge@2d {
+	compatible = "ti,sn65dsi86";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	reg = <0x2d>;
+
+	enable-gpios = <&msmgpio 33 GPIO_ACTIVE_HIGH>;
+
+	vccio-supply = <&pm8916_l17>;
+	vcca-supply = <&pm8916_l6>;
+
+	ports {
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		port@0 {
+			reg = <0>;
+
+			edp_bridge_in: endpoint {
+				remote-endpoint = <&dsi_out>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+
+			edp_bridge_out: endpoint {
+				remote-endpoint = <&edp_panel_in>;
Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt new file +4 -1
+			};
+		};
+	};
+}
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

author
#13 lore raw

RFC 1/1 DPU PATCH 3/4] drm/panel: add Innolux TV123WAM eDP panel driver

In reply to a message not in the archive

Add support for Innolux TV123WAM 12.3" panel which is an eDP panel.

Signed-off-by Sandeep Panda <[email protected]>
---
 drivers/gpu/drm/panel/panel-innolux-tv123wam.c | 299 +++++++++++++++++++++++++
 1 file changed, 299 insertions(+)
 create mode 100644 drivers/gpu/drm/panel/panel-innolux-tv123wam.c
drivers/gpu/drm/panel/panel-innolux-tv123wam.c new file +299 -0
new file mode 100644
index 0000000..f149fa9
@@ -0,0 +1,299 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drmP.h>
+#include <drm/drm_panel.h>
+#include <video/display_timing.h>
+#include <video/videomode.h>
+
+struct panel_edp_2k_desc {
+	const struct drm_display_mode *modes;
+	unsigned int num_modes;
+	struct {
+		unsigned int width;
+		unsigned int height;
+	} size;
+	u32 bpc;
+	u32 bus_flags;
+};
+
+struct panel_edp_2k_data {
+	struct drm_panel base;
+	bool enabled;
+	bool prepared;
+
+	const struct panel_edp_2k_desc *desc;
+
+	struct regulator *supply;
+	struct gpio_desc *enable_gpio;
+	struct backlight_device *backlight;
+};
+
+static const struct drm_display_mode innolux_edp_2k_mode = {
+	.clock = 206016,
+	.hdisplay = 2160,
+	.hsync_start = 2160 + 80,
+	.hsync_end = 2160 + 80 + 48,
+	.htotal = 2160 + 80 + 48 + 32,
+	.vdisplay = 1440,
+	.vsync_start = 1440 + 27,
+	.vsync_end = 1440 + 27 + 3,
+	.vtotal = 1440 + 27 + 3 + 10,
+	.vrefresh = 60,
+	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
+};
+
+static const struct panel_edp_2k_desc innolux_edp_2k = {
+	.modes = &innolux_edp_2k_mode,
+	.num_modes = 1,
+	.size = {
+		.width = 223,
+		.height = 125,
+	},
+	.bpc = 8,
+	.bus_flags = DRM_BUS_FLAG_PIXDATA_POSEDGE,
+};
+
+static const struct of_device_id platform_of_match[] = {
+	{
+		.compatible = "innolux, edp_2k_panel",
+		.data = &innolux_edp_2k,
+	},
+};
+
+static inline struct panel_edp_2k_data *
+to_panel_edp_2k_data(struct drm_panel *panel)
+{
+	return container_of(panel, struct panel_edp_2k_data, base);
+}
+
+static int panel_edp_2k_disable(struct drm_panel *panel)
+{
+	struct panel_edp_2k_data *pdata = to_panel_edp_2k_data(panel);
+
+	if (!pdata->enabled)
+		return 0;
+
+	if (pdata->backlight) {
+		pdata->backlight->props.power = FB_BLANK_POWERDOWN;
+		pdata->backlight->props.state |= BL_CORE_FBBLANK;
+		backlight_update_status(pdata->backlight);
+	}
+
+	pdata->enabled = false;
+
+	return 0;
+}
+
+static int panel_edp_2k_unprepare(struct drm_panel *panel)
+{
+	struct panel_edp_2k_data *pdata = to_panel_edp_2k_data(panel);
+
+	if (!pdata->prepared)
+		return 0;
+
+	if (pdata->enable_gpio)
+		gpiod_set_value_cansleep(pdata->enable_gpio, 0);
+
+	regulator_disable(pdata->supply);
+
+	pdata->prepared = false;
+
+	return 0;
+}
+
+static int panel_edp_2k_prepare(struct drm_panel *panel)
+{
+	struct panel_edp_2k_data *pdata = to_panel_edp_2k_data(panel);
+	int ret;
+
+	if (pdata->prepared)
+		return 0;
+
+	ret = regulator_enable(pdata->supply);
+	if (ret < 0) {
+		dev_err(panel->dev, "failed to enable supply: %d\n", ret);
+		return ret;
+	}
+
+	if (pdata->enable_gpio)
+		gpiod_set_value_cansleep(pdata->enable_gpio, 1);
+
+	pdata->prepared = true;
+
+	return 0;
+}
+
+static int panel_edp_2k_enable(struct drm_panel *panel)
+{
+	struct panel_edp_2k_data *pdata = to_panel_edp_2k_data(panel);
+
+	if (pdata->enabled)
+		return 0;
+
+	if (pdata->backlight) {
+		pdata->backlight->props.state &= ~BL_CORE_FBBLANK;
+		pdata->backlight->props.power = FB_BLANK_UNBLANK;
+		backlight_update_status(pdata->backlight);
+	}
+
+	pdata->enabled = true;
+
+	return 0;
+}
+
+static int panel_edp_2k_get_modes(struct drm_panel *panel)
+{
+	struct panel_edp_2k_data *pdata = to_panel_edp_2k_data(panel);
+	struct drm_connector *connector = pdata->base.connector;
+	struct drm_device *drm = pdata->base.drm;
+	struct drm_display_mode *mode;
+	unsigned int i, num = 0;
+
+	if (!pdata->desc || !connector || !drm)
+		return 0;
+
+	for (i = 0; i < pdata->desc->num_modes; i++) {
+		const struct drm_display_mode *m = &pdata->desc->modes[i];
+
+		mode = drm_mode_duplicate(drm, m);
+		if (!mode) {
+			dev_err(drm->dev, "failed to add mode %ux%u@%u\n",
+				m->hdisplay, m->vdisplay, m->vrefresh);
+			continue;
+		}
+
+		mode->type |= DRM_MODE_TYPE_DRIVER;
+
+		if (pdata->desc->num_modes == 1)
+			mode->type |= DRM_MODE_TYPE_PREFERRED;
+
+		drm_mode_set_name(mode);
+
+		drm_mode_probed_add(connector, mode);
+		num++;
+	}
+
+	connector->display_info.bpc = pdata->desc->bpc;
+	connector->display_info.width_mm = pdata->desc->size.width;
+	connector->display_info.height_mm = pdata->desc->size.height;
+	connector->display_info.bus_flags = pdata->desc->bus_flags;
+
+	return num;
+}
+
+static const struct drm_panel_funcs panel_edp_2k_funcs = {
+	.disable = panel_edp_2k_disable,
+	.unprepare = panel_edp_2k_unprepare,
+	.prepare = panel_edp_2k_prepare,
+	.enable = panel_edp_2k_enable,
+	.get_modes = panel_edp_2k_get_modes,
+};
+
+static int panel_edp_2k_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *id;
+	struct device_node *backlight;
+	struct panel_edp_2k_data *pdata;
+	int ret;
+
+	id = of_match_node(platform_of_match, pdev->dev.of_node);
+	if (!id)
+		return -ENODEV;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->enabled = false;
+	pdata->desc = id->data;
+
+	pdata->supply = devm_regulator_get(dev, "power");
+	if (IS_ERR(pdata->supply))
+		return PTR_ERR(pdata->supply);
+
+	pdata->enable_gpio = devm_gpiod_get(dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->enable_gpio)) {
+		ret = PTR_ERR(pdata->enable_gpio);
+		dev_err(dev, "failed to request GPIO: %d\n", ret);
+		return ret;
+	}
+
+	backlight = of_parse_phandle(dev->of_node, "backlight", 0);
+	if (backlight) {
+		pdata->backlight = of_find_backlight_by_node(backlight);
+		of_node_put(backlight);
+
+		if (!pdata->backlight)
+			return -EPROBE_DEFER;
+	}
+
+	drm_panel_init(&pdata->base);
+	pdata->base.dev = dev;
+	pdata->base.funcs = &panel_edp_2k_funcs;
+
+	ret = drm_panel_add(&pdata->base);
+	if (ret < 0)
+		goto free_backlight;
+
+	dev_set_drvdata(dev, pdata);
+
+	return 0;
+
+free_backlight:
+	if (pdata->backlight)
+		put_device(&pdata->backlight->dev);
+
+	return ret;
+}
+
+static int panel_edp_2k_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct panel_edp_2k_data *pdata = dev_get_drvdata(dev);
+
+	drm_panel_detach(&pdata->base);
+	drm_panel_remove(&pdata->base);
+
+	panel_edp_2k_disable(&pdata->base);
+	panel_edp_2k_unprepare(&pdata->base);
+
+	if (pdata->backlight)
+		put_device(&pdata->backlight->dev);
+
+	return 0;
+}
+
+static struct platform_driver panel_edp_2k_driver = {
+	.driver = {
+		.name = "innolux-2k-edp-panel",
+		.of_match_table = platform_of_match,
+	},
+	.probe = panel_edp_2k_probe,
+	.remove = panel_edp_2k_remove,
+};
+
+static int __init panel_edp_2k_init(void)
+{
+	int ret;
+
+	ret = platform_driver_register(&panel_edp_2k_driver);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+module_init(panel_edp_2k_init);
+
+static void __exit panel_edp_2k_exit(void)
+{
+	platform_driver_unregister(&panel_edp_2k_driver);
+}
+module_exit(panel_edp_2k_exit);
+
+MODULE_DESCRIPTION("Innolux 2k eDP panel driver");
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
author
#14 lore raw

RFC 1/1 DPU PATCH 4/4] dt-bindings: Add Innolux TV123WAM panel bindings

In reply to a message not in the archive

The Innolux TV123WAM is a 12.3" eDP display panel
with 2160x1440 resolution.

Signed-off-by Sandeep Panda <[email protected]>
---
 .../bindings/display/panel/innolux,edp-2k-panel.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +21 -0
new file mode 100644
index 0000000..19f271c
@@ -0,0 +1,21 @@
+Innolux TV123WAM 12.3 inch eDP display panel
+
+Required properties:
+- power-supply: regulator to provide the supply voltage
+- enable-gpios: GPIO pin to enable or disable the panel
+
+Optional properties:
+- backlight: phandle of the backlight device attached to the panel
+
+Example:
+
+	edp_panel: edp_panel {
+		compatible = "innolux, edp_2k_panel";
+		reg = <0>;
+
+		enable-gpios = <&msmgpio 32 GPIO_ACTIVE_HIGH>;
+
+		power-supply = <&pm8916_l8>;
+
+		backlight = <&backlight>;
+	};
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
maintainer
#15 lore raw

Re: [[RFC]DPU PATCH 4/4] dt-bindings: Add Innolux TV123WAM panel bindings

Quoted 2 lines · Sandeep Panda

The Innolux TV123WAM is a 12.3" eDP display panel
with 2160x1440 resolution.

Why don't you just submit this for upstream?

Quoted 13 lines
Signed-off-by Sandeep Panda <[email protected]>
---
 .../bindings/display/panel/innolux,edp-2k-panel.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +0 -0
new file mode 100644
index 0000000..19f271c

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 5

Quoted 5 lines
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +5 -0
@@ -0,0 +1,21 @@
+Innolux TV123WAM 12.3 inch eDP display panel
+
+Required properties:
+- power-supply: regulator to provide the supply voltage
+- enable-gpios: GPIO pin to enable or disable the panel

Need to state the active state.

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 12

Quoted 7 lines
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +7 -0
+
+Optional properties:
+- backlight: phandle of the backlight device attached to the panel
+
+Example:
+
+	edp_panel: edp_panel {

panel {

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 13

Quoted 1 line
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +1 -0
+		compatible = "innolux, edp_2k_panel";

spurious space ^

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 14

Quoted 1 line
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +1 -0
+		reg = <0>;

Not documented. Should be dropped.

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 19

Quoted 5 lines
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +5 -0
+
+		enable-gpios = <&msmgpio 32 GPIO_ACTIVE_HIGH>;
+
+		power-supply = <&pm8916_l8>;
+

Remove the extra blank lines.

Quoted 10 lines
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +2 -0
+		backlight = <&backlight>;
+	};
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#16 lore raw

Re: [[RFC]DPU PATCH 4/4] dt-bindings: Add Innolux TV123WAM panel bindings

Quoted 5 lines · Rob Herring
Quoted 2 lines · Sandeep Panda

The Innolux TV123WAM is a 12.3" eDP display panel
with 2160x1440 resolution.

Why don't you just submit this for upstream?

Sean Paul has suggested to reuse simple panel driver instead of having a new driver altogether. So all these dt-bindings wont be needed.

Quoted 57 lines
Quoted 17 lines
Signed-off-by Sandeep Panda <[email protected]>
---
 .../bindings/display/panel/innolux,edp-2k-panel.txt | 21 
+++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt

(quoted diff) +0 -0
new file mode 100644
index 0000000..19f271c

b/Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 5

Quoted 5 lines
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +5 -0
@@ -0,0 +1,21 @@
+Innolux TV123WAM 12.3 inch eDP display panel
+
+Required properties:
+- power-supply: regulator to provide the supply voltage
+- enable-gpios: GPIO pin to enable or disable the panel

Need to state the active state.

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 12

Quoted 7 lines
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +7 -0
+
+Optional properties:
+- backlight: phandle of the backlight device attached to the panel
+
+Example:
+
+	edp_panel: edp_panel {

panel {

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 13

Quoted 1 line
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +1 -0
+		compatible = "innolux, edp_2k_panel";

spurious space ^

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 14

Quoted 1 line
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +1 -0
+		reg = <0>;

Not documented. Should be dropped.

Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt line 19

Quoted 5 lines
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +5 -0
+
+		enable-gpios = <&msmgpio 32 GPIO_ACTIVE_HIGH>;
+
+		power-supply = <&pm8916_l8>;
+

Remove the extra blank lines.

Quoted 6 lines
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt new file +2 -1
+		backlight = <&backlight>;
+	};
--

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

Quoted 4 lines
Quoted 4 lines

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

A message that is not in the archive (sent off-list or not yet received): cb59510635f0e7c010588a4603ae3f0f-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org

#18 lore raw

Re: [[RFC]DPU PATCH 4/4] dt-bindings: Add Innolux TV123WAM panel bindings

Quoted 4 lines · On Tue, Apr 24, 2018 at 9:16 PM, <[email protected]> wrote:
Quoted 3 lines · Rob Herring
Quoted 1 line · Sandeep Panda
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines

The Innolux TV123WAM is a 12.3" eDP display panel
with 2160x1440 resolution.

Why don't you just submit this for upstream?

Sean Paul has suggested to reuse simple panel driver instead of having a new driver altogether. So all these dt-bindings wont be needed.

Yes it will unless there is already a binding for this panel. The "simple panel" is not a binding, only a driver which matches on dozens of different panels.
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

#19 lore raw
In reply to Sandeep Panda
Quoted 3 lines · Sandeep Panda

Changelog:

v3 -> v4:

I didn't really bother to do a thorough review given that there are obvious mistakes and ignored feedback, it's a waste of time, tbh. Please go through the previous reviews and resend a more polished version.

Sean

Quoted 32 lines

Current patchset separates out eDP panel specific resources from sn65dsi86 bridge driver and creates a separate driver for the innloux edp panel.

Now bridge driver check if any panel is attached or not to get the supported modes. If any panel is attached then query from panel driver to get the modes, or else probe the provided i2c adapter to read the modes from EDID.

Removed hardcoding of bridge init sequence. With this patchset bridge driver now will program the init sequence based on the current mode set by drm framework.

Current patchset is not tested on actual bridge chip/panel.

Sandeep Panda (4):
drm/bridge: add support for sn65dsi86 bridge driver
dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings drm/panel: add Innolux TV123WAM eDP panel driver
dt-bindings: Add Innolux TV123WAM panel bindings

 .../bindings/display/bridge/ti,sn65dsi86.txt       |  60 ++
 .../display/panel/innolux,edp-2k-panel.txt         |  21 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c              | 742 +++++++++++++++++++++
 drivers/gpu/drm/panel/panel-innolux-tv123wam.c     | 299 +++++++++
 4 files changed, 1122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
 create mode 100644 Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
 create mode 100644 drivers/gpu/drm/panel/panel-innolux-tv123wam.c
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
author
#20 lore raw
Quoted 8 lines · Sean Paul
Quoted 3 lines · Sandeep Panda

Changelog:

v3 -> v4:

I didn't really bother to do a thorough review given that there are obvious mistakes and ignored feedback, it's a waste of time, tbh. Please go through the previous reviews and resend a more polished version.

I have replied to your review comments. Please review the edp driver, so that i can address all the comments together while uploading next patchset.

Quoted 38 lines

Sean

Quoted 35 lines

Current patchset separates out eDP panel specific resources from sn65dsi86 bridge driver and creates a separate driver for the innloux edp panel.

Now bridge driver check if any panel is attached or not to get the supported modes. If any panel is attached then query from panel driver to get the modes, or else probe the provided i2c adapter to read the modes from EDID.

Removed hardcoding of bridge init sequence. With this patchset bridge driver now will program the init sequence based on the current mode set by drm framework.

Current patchset is not tested on actual bridge chip/panel.

Sandeep Panda (4):
drm/bridge: add support for sn65dsi86 bridge driver
dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings drm/panel: add Innolux TV123WAM eDP panel driver
dt-bindings: Add Innolux TV123WAM panel bindings

 .../bindings/display/bridge/ti,sn65dsi86.txt       |  60 ++
 .../display/panel/innolux,edp-2k-panel.txt         |  21 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c              | 742 
+++++++++++++++++++++
 drivers/gpu/drm/panel/panel-innolux-tv123wam.c     | 299 +++++++++
 4 files changed, 1122 insertions(+)
 create mode 100644 
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
 create mode 100644 
Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
 create mode 100644 drivers/gpu/drm/panel/panel-innolux-tv123wam.c

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno

#21 lore raw
In reply to Sandeep Panda
Quoted 6 lines · Sandeep Panda

Changelog:

v3 -> v4:
Current patchset separates out eDP panel specific resources from sn65dsi86 bridge driver and creates a separate driver for the innloux edp panel.

Quoted 8 lines

Now bridge driver check if any panel is attached or not to get the supported modes. If any panel is attached then query from panel driver to get the modes, or else probe the provided i2c adapter to read the modes from EDID.

Removed hardcoding of bridge init sequence. With this patchset bridge driver now will program the init sequence based on the current mode set by drm framework.

Current patchset is not tested on actual bridge chip/panel.

One more thing, could you use proper prefixes in subject:
- no double braces,
- patch version,
- no DPU,
- RFC up to you, but in case of driver which you want to eventually merge it seems unnecessary.

So subject should look more or less like:
[PATCH v4 0/4] Add suppport for sn65dsi86 bridge chip and Innolux 2k edp panel driver

Regards
Andrzej

Quoted 7 lines

Sandeep Panda (4):
drm/bridge: add support for sn65dsi86 bridge driver
dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings drm/panel: add Innolux TV123WAM eDP panel driver
dt-bindings: Add Innolux TV123WAM panel bindings

Quoted 10 lines
 .../bindings/display/bridge/ti,sn65dsi86.txt       |  60 ++
 .../display/panel/innolux,edp-2k-panel.txt         |  21 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c              | 742 +++++++++++++++++++++
 drivers/gpu/drm/panel/panel-innolux-tv123wam.c     | 299 +++++++++
 4 files changed, 1122 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
 create mode 100644 Documentation/devicetree/bindings/display/panel/innolux,edp-2k-panel.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
 create mode 100644 drivers/gpu/drm/panel/panel-innolux-tv123wam.c

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel