[[RFC]DPU PATCH 0/2] Add suppport for sn65dsi86 bridge chip

[[RFC]DPU PATCH 0/2] Add suppport for sn65dsi86 bridge chip

[Sandeep Panda]

This patchset replaces the obsolete gpio config APIs
with gpiod APIs.

This patchset captures the bindings for bridge driver
in more detailed manner.

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.

Current patchset is yet tested on an actual sn65dsi86 bridge HW.

Sandeep Panda (2):
  dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings
  drm/bridge: add support for sn65dsi86 bridge driver

 .../bindings/display/bridge/ti,sn65dsi86.txt       |   75 ++
 drivers/gpu/drm/bridge/ti-sn65dsi86.c              | 1019 ++++++++++++++++++++
 2 files changed, 1094 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c

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

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[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 <spanda@codeaurora.org>
---
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 1019 +++++++++++++++++++++++++++++++++
 1 file changed, 1019 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c

diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
new file mode 100644
index 0000000..93aa1ad
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -0,0 +1,1019 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#define pr_fmt(fmt) "%s: " fmt, __func__
+
+#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/interrupt.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/of_irq.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>
+
+#define SN_DEVICE_REV_REG 0x08
+
+/* Link Training specific registers */
+#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
+
+struct sn65dsi86_reg_cfg {
+	u8 reg;
+	u8 val;
+	int sleep_in_ms;
+};
+
+struct sn65dsi86_video_cfg {
+	u32 h_active;
+	u32 h_front_porch;
+	u32 h_pulse_width;
+	u32 h_back_porch;
+	bool h_polarity;
+	u32 v_active;
+	u32 v_front_porch;
+	u32 v_pulse_width;
+	u32 v_back_porch;
+	bool v_polarity;
+	u32 pclk_khz;
+	u32 num_of_lanes;
+};
+
+struct sn65dsi86_gpios {
+	struct gpio_desc *irq_gpio;
+	struct gpio_desc *enable_gpio;
+	struct gpio_desc *aux_i2c_sda;
+	struct gpio_desc *aux_i2c_scl;
+	struct gpio_desc *edp_bias_en;
+	struct gpio_desc *edp_bklt_en;
+	struct gpio_desc *edp_bklt_ctrl;
+};
+
+struct sn65dsi86 {
+	struct device *dev;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+
+	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
+
+	u8 i2c_addr;
+	int irq;
+
+	struct sn65dsi86_gpios gpios;
+
+	unsigned int num_supplies;
+	struct regulator_bulk_data *supplies;
+
+	struct i2c_client *i2c_client;
+
+	enum drm_connector_status connector_status;
+	bool power_on;
+
+	bool is_pluggable;
+	u32 num_of_modes;
+	struct list_head mode_list;
+	struct edid *edid;
+
+	struct drm_display_mode curr_mode;
+	struct sn65dsi86_video_cfg video_cfg;
+};
+
+struct sn65dsi86_reg_cfg cfg_proto_0_init[] = {
+	{SN_REFCLK_FREQ_REG, 0x02, 0x0},
+	{SN_DSI_LANES_REG, 0x26, 0x14},
+	{SN_DSIA_CLK_FREQ_REG, 0x7B, 0x0},
+	{SN_ENH_FRAME_REG, 0x05, 0x0},
+	{SN_SSC_CONFIG_REG, 0x30, 0x0},
+	{SN_DATARATE_CONFIG_REG, 0x80, 0x0},
+	{SN_PLL_ENABLE_REG, 0x01, 0x0},
+	{SN_SCRAMBLE_CONFIG_REG, 0x00, 0x0},
+	{SN_AUX_WDATA0_REG, 0x01, 0x0},
+	{SN_AUX_ADDR_19_16_REG, 0x00, 0x0},
+	{SN_AUX_ADDR_15_8_REG, 0x01, 0x0},
+	{SN_AUX_ADDR_7_0_REG, 0x0A, 0x0},
+	{SN_AUX_LENGTH_REG, 0x01, 0x0},
+	{SN_AUX_CMD_REG, 0x81, 0x14},
+	{SN_ML_TX_MODE_REG, 0x0A, 0x14},
+	{SN_PAGE_SELECT_REG, 0x14, 0x0},
+	{SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, 0x70, 0x0},
+	{SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG, 0x08, 0x0},
+	{SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, 0xA0, 0x0},
+	{SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG, 0x05, 0x0},
+	{SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, 0x20, 0x0},
+	{SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG, 0x80, 0x0},
+	{SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG, 0x0A, 0x0},
+	{SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG, 0x80, 0x0},
+	{SN_CHA_HORIZONTAL_BACK_PORCH_REG, 0x50, 0x0},
+	{SN_CHA_VERTICAL_BACK_PORCH_REG, 0x1B, 0x0},
+	{SN_CHA_HORIZONTAL_FRONT_PORCH_REG, 0x30, 0x0},
+	{SN_CHA_VERTICAL_FRONT_PORCH_REG, 0x03, 0x0},
+	{SN_DATA_FORMAT_REG, 0x00, 0x0},
+	{SN_COLOR_BAR_CONFIG_REG, 0x00, 0x14},
+	{SN_ENH_FRAME_REG, 0x0D, 0x0},
+};
+
+static int sn65dsi86_write(struct sn65dsi86 *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) {
+		pr_err("i2c write failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int sn65dsi86_read(struct sn65dsi86 *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) {
+		pr_err("i2c read failed\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int sn65dsi86_write_array(struct sn65dsi86 *pdata,
+	struct sn65dsi86_reg_cfg *cfg, int size)
+{
+	int ret = 0;
+	int i;
+
+	size = size / sizeof(struct sn65dsi86_reg_cfg);
+	for (i = 0; i < size; i++) {
+		ret = sn65dsi86_write(pdata, cfg[i].reg, cfg[i].val);
+
+		if (ret != 0) {
+			pr_err("reg writes failed. Last write %02X to %02X\n",
+				cfg[i].val, cfg[i].reg);
+			goto w_regs_fail;
+		}
+
+		if (cfg[i].sleep_in_ms)
+			msleep(cfg[i].sleep_in_ms);
+	}
+
+w_regs_fail:
+	if (ret != 0)
+		pr_err("exiting with ret = %d after %d writes\n", ret, i);
+
+	return ret;
+}
+
+static void sn65dsi86_gpio_configure(struct sn65dsi86 *pdata, bool on)
+{
+	int value;
+
+	value = on ? 1: 0;
+
+	gpiod_set_value(pdata->gpios.enable_gpio, value);
+	gpiod_set_value(pdata->gpios.aux_i2c_sda, value);
+	gpiod_set_value(pdata->gpios.aux_i2c_scl, value);
+	gpiod_set_value(pdata->gpios.edp_bias_en, value);
+	gpiod_set_value(pdata->gpios.edp_bklt_en, value);
+	gpiod_set_value(pdata->gpios.irq_gpio, value);
+	gpiod_set_value(pdata->gpios.edp_bklt_ctrl, value);
+
+	pr_debug("config to: %d\n", value);
+}
+
+static void sn65dsi86_power_ctrl(struct sn65dsi86 *pdata, bool enable)
+{
+	if (!pdata)
+		return;
+
+	if (!pdata->power_on && enable) {
+		sn65dsi86_gpio_configure(pdata, true);
+
+		if (regulator_bulk_enable(pdata->num_supplies,
+						pdata->supplies)) {
+			pr_err("bridge regulator enable failed\n");
+			return;
+		}
+		pdata->power_on = true;
+	} else if (pdata->power_on && !enable) {
+		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
+
+		sn65dsi86_gpio_configure(pdata, false);
+		pdata->power_on = false;
+	} else {
+		pr_debug("unnecessary call to power control\n");
+	}
+}
+
+/* Connector funcs */
+static struct sn65dsi86 *connector_to_sn65dsi86(struct drm_connector *connector)
+{
+	return container_of(connector, struct sn65dsi86, connector);
+}
+
+static int sn65dsi86_send_aux_cmd(struct sn65dsi86 *pdata,
+				  u8 cmd, u8 addr, u8 length, int w_data)
+{
+	u8 read = 0;
+	int retry_cnt = 10;
+
+	sn65dsi86_write(pdata, SN_AUX_CMD_REG, (cmd << 4));
+	sn65dsi86_write(pdata, SN_AUX_ADDR_7_0_REG, addr);
+	sn65dsi86_write(pdata, SN_AUX_LENGTH_REG, length);
+	if (w_data >= 0)
+		sn65dsi86_write(pdata, SN_AUX_WDATA0_REG, (u8)w_data);
+
+	/* set SEND bit */
+	sn65dsi86_read(pdata, SN_AUX_CMD_REG, &read, 1);
+	read |= BIT(0);
+	sn65dsi86_write(pdata, SN_AUX_CMD_REG, read);
+
+	/* poll for bridge to ack SEND bit */
+	while (retry_cnt) {
+		sn65dsi86_read(pdata, SN_AUX_CMD_REG, &read, 0x1);
+		if (!(read & BIT(0)))
+			break;
+		retry_cnt--;
+		udelay(1000);
+	}
+
+	if (!retry_cnt) {
+		pr_err("aux_cmd transfer failed\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+
+static int sn65dsi86_read_edid(struct sn65dsi86 *pdata, u8 *buf)
+{
+	int i = 0;
+	u8 addr = SN_AUX_RDATA0_REG;
+	u8 *data = buf;
+
+	if (!data)
+		return -ENOMEM;
+
+	if (sn65dsi86_send_aux_cmd(pdata, 0x4, 0x50, 0x0, -1) ||
+		sn65dsi86_send_aux_cmd(pdata, 0x4, 0x50, 0x01, 0x0) ||
+		sn65dsi86_send_aux_cmd(pdata, 0x5, 0x50, 0x0, -1) ||
+		sn65dsi86_send_aux_cmd(pdata, 0x5, 0x50, 0x10, -1))
+		goto error;
+
+	for (i = 0; i < 16; i++) {
+		if (sn65dsi86_read(pdata, addr, data, 0x1))
+			goto error;
+		addr++;
+		data++;
+	}
+
+	return 0;
+error:
+	pr_err("edid read over i2c failed\n");
+	return -EINVAL;
+}
+
+static int sn65dsi86_read_edid_block(struct sn65dsi86 *pdata,
+			       u8 *buf, unsigned int block)
+{
+	if (block == 0) {
+		if (sn65dsi86_read_edid(pdata, buf))
+			goto error;
+	} else if (block == 1) {
+		/* move segment pointer */
+		if (sn65dsi86_send_aux_cmd(pdata, 0x4, 0x30, 0x0, -1) ||
+			sn65dsi86_send_aux_cmd(pdata, 0x4, 0x30, 0x01, 0x1) ||
+			sn65dsi86_send_aux_cmd(pdata, 0x0, 0x30, 0x00, -1))
+			goto error;
+		else
+			if (sn65dsi86_read_edid(pdata, buf))
+				goto error;
+	} else {
+		pr_debug("unsupported edid block\n");
+		goto error;
+	}
+
+	return 0;
+error:
+	pr_err("edid block read failed\n");
+	return -EINVAL;
+}
+
+static int sn65dsi86_get_edid_block(void *data, u8 *buf, unsigned int block,
+				  size_t len)
+{
+	struct sn65dsi86 *pdata = data;
+	int ret = 0;
+
+	pr_debug("get edid block: block=%d, len=%d\n", block, (int)len);
+
+	if (len > 128 || block > 1)
+		return -EINVAL;
+
+	ret = sn65dsi86_read_edid_block(pdata, buf, block);
+	if (ret) {
+		pr_err("edid read failed for block: %d ret: %d\n", block, ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sn65dsi86_connector_get_modes(struct drm_connector *connector)
+{
+	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
+	struct drm_display_mode *mode, *m;
+
+	if (pdata->edid)
+		return drm_add_edid_modes(connector, pdata->edid);
+
+	sn65dsi86_power_ctrl(pdata, true);
+	if (pdata->is_pluggable) {
+		pdata->edid = drm_do_get_edid(connector,
+				sn65dsi86_get_edid_block, pdata);
+
+		drm_mode_connector_update_edid_property(connector, pdata->edid);
+		pdata->num_of_modes = drm_add_edid_modes(connector,
+								pdata->edid);
+	}
+
+	if (!pdata->is_pluggable || !pdata->num_of_modes) {
+		/*
+		 * if device does not support HPD or due to some reason
+		 * EDID read failed then fall back to mode_list which is
+		 * already parsed from dt if any.
+		 */
+		list_for_each_entry(mode, &pdata->mode_list, head) {
+			m = drm_mode_duplicate(connector->dev, mode);
+			if (!m) {
+				pr_err("failed to get mode %dx%d\n",
+					mode->hdisplay, mode->vdisplay);
+				break;
+			}
+			drm_mode_probed_add(connector, m);
+		}
+	}
+
+	sn65dsi86_power_ctrl(pdata, false);
+	return pdata->num_of_modes;
+}
+
+static enum drm_mode_status
+sn65dsi86_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
+	struct drm_display_mode *m;
+
+	if (pdata->edid)
+		return MODE_OK;
+
+	if (!pdata->is_pluggable) {
+		list_for_each_entry(m, &pdata->mode_list, head) {
+			if (m->hdisplay == mode->hdisplay &&
+				m->vdisplay == mode->vdisplay)
+				return MODE_OK;
+		}
+	}
+
+	return MODE_BAD;
+}
+
+static struct drm_connector_helper_funcs sn65dsi86_connector_helper_funcs = {
+	.get_modes = sn65dsi86_connector_get_modes,
+	.mode_valid = sn65dsi86_connector_mode_valid,
+};
+
+static enum drm_connector_status
+sn65dsi86_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
+
+	pdata->connector_status = pdata->power_on ?
+		connector_status_connected : connector_status_disconnected;
+
+	return pdata->connector_status;
+}
+
+static const struct drm_connector_funcs sn65dsi86_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = sn65dsi86_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 sn65dsi86 *bridge_to_sn65dsi86(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct sn65dsi86, bridge);
+}
+
+static int sn65dsi86_read_device_rev(struct sn65dsi86 *pdata)
+{
+	u8 rev = 0;
+	int ret = 0;
+
+	ret = sn65dsi86_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
+
+	if (!ret) {
+		if (rev == 0x2) {
+			pr_info("SN65DSI86 revision id: 0x%x\n", rev);
+		} else {
+			pr_warn("SN65DSI86 revision id mismatch\n");
+			ret = -EINVAL;
+		}
+	}
+
+	return ret;
+}
+
+static irqreturn_t sn65dsi86_irq_thread_handler(int irq, void *dev_id)
+{
+	return IRQ_HANDLED;
+}
+
+static const char * const sn65dsi86_supply_names[] = {
+	"vccio",
+	"vcca",
+	"vccs"
+};
+
+static int sn65dsi86_init_regulators(struct sn65dsi86 *pdata)
+{
+	const char * const *supply_names;
+	unsigned int i;
+	int ret;
+
+	supply_names = sn65dsi86_supply_names;
+	pdata->num_supplies = ARRAY_SIZE(sn65dsi86_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 = supply_names[i];
+
+	ret = devm_regulator_bulk_get(pdata->dev,
+			pdata->num_supplies, pdata->supplies);
+	if (ret)
+		return ret;
+
+	return regulator_bulk_enable(pdata->num_supplies, pdata->supplies);
+}
+
+static int sn65dsi86_bridge_attach(struct drm_bridge *bridge)
+{
+	struct mipi_dsi_host *host;
+	struct mipi_dsi_device *dsi;
+	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "sn65dsi86",
+						   .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,
+				 &sn65dsi86_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,
+				 &sn65dsi86_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) {
+		pr_err("failed to find dsi host\n");
+		return -ENODEV;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		pr_err("failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		goto err_dsi_device;
+	}
+
+	/* 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) {
+		pr_err("failed to attach dsi to host\n");
+		goto err_dsi_attach;
+	}
+
+	pdata->dsi = dsi;
+
+	pr_debug("bridge attached\n");
+
+	return 0;
+
+err_dsi_attach:
+	mipi_dsi_device_unregister(dsi);
+err_dsi_device:
+	return ret;
+}
+
+static void sn65dsi86_set_video_cfg(struct sn65dsi86 *pdata,
+	struct drm_display_mode *mode,
+	struct sn65dsi86_video_cfg *video_cfg)
+{
+	video_cfg->h_active = mode->hdisplay;
+	video_cfg->v_active = mode->vdisplay;
+	video_cfg->h_front_porch = mode->hsync_start - mode->hdisplay;
+	video_cfg->v_front_porch = mode->vsync_start - mode->vdisplay;
+	video_cfg->h_back_porch = mode->htotal - mode->hsync_end;
+	video_cfg->v_back_porch = mode->vtotal - mode->vsync_end;
+	video_cfg->h_pulse_width = mode->hsync_end - mode->hsync_start;
+	video_cfg->v_pulse_width = mode->vsync_end - mode->vsync_start;
+	video_cfg->pclk_khz = mode->clock;
+
+	video_cfg->h_polarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
+	video_cfg->v_polarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
+
+	/* setting to 4 lanes always for now */
+	video_cfg->num_of_lanes = 4;
+
+	pr_debug("video=h[%d,%d,%d,%d] v[%d,%d,%d,%d] pclk=%d lane=%d\n",
+		video_cfg->h_active, video_cfg->h_front_porch,
+		video_cfg->h_pulse_width, video_cfg->h_back_porch,
+		video_cfg->v_active, video_cfg->v_front_porch,
+		video_cfg->v_pulse_width, video_cfg->v_back_porch,
+		video_cfg->pclk_khz, video_cfg->num_of_lanes);
+}
+
+static void sn65dsi86_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj_mode)
+{
+	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
+	struct sn65dsi86_video_cfg *video_cfg = &pdata->video_cfg;
+	int ret = 0;
+
+	pr_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);
+
+	memset(video_cfg, 0, sizeof(struct sn65dsi86_video_cfg));
+	sn65dsi86_set_video_cfg(pdata, adj_mode, video_cfg);
+
+	if (video_cfg->num_of_lanes != pdata->dsi->lanes) {
+		mipi_dsi_detach(pdata->dsi);
+		pdata->dsi->lanes = video_cfg->num_of_lanes;
+		ret = mipi_dsi_attach(pdata->dsi);
+		if (ret)
+			pr_err("failed to change host lanes\n");
+	}
+}
+
+static void sn65dsi86_bridge_disable(struct drm_bridge *bridge)
+{
+	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
+
+	sn65dsi86_power_ctrl(pdata, false);
+	pdata->connector_status =  connector_status_disconnected;
+}
+
+static void sn65dsi86_bridge_enable(struct drm_bridge *bridge)
+{
+	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
+
+	sn65dsi86_power_ctrl(pdata, true);
+	pdata->connector_status =  connector_status_connected;
+	sn65dsi86_write_array(pdata, cfg_proto_0_init,
+			      sizeof(cfg_proto_0_init));
+}
+
+static const struct drm_bridge_funcs sn65dsi86_bridge_funcs = {
+	.attach = sn65dsi86_bridge_attach,
+	.enable = sn65dsi86_bridge_enable,
+	.disable = sn65dsi86_bridge_disable,
+	.mode_set = sn65dsi86_bridge_mode_set,
+};
+
+static int sn65dsi86_parse_dt_modes(struct device_node *np,
+					struct list_head *head,
+					u32 *num_of_modes)
+{
+	int rc = 0;
+	struct drm_display_mode *mode;
+	u32 mode_count = 0;
+	struct device_node *node = NULL;
+	struct device_node *root_node = NULL;
+	u32 h_front_porch, h_pulse_width, h_back_porch;
+	u32 v_front_porch, v_pulse_width, v_back_porch;
+	bool h_active_high, v_active_high;
+	u32 flags = 0;
+
+	root_node = of_get_child_by_name(np, "sn,custom-modes");
+	if (!root_node) {
+		root_node = of_parse_phandle(np, "sn,custom-modes", 0);
+		if (!root_node) {
+			pr_info("No modes present for sn,custom-modes");
+			goto end;
+		}
+	}
+
+	for_each_child_of_node(root_node, node) {
+		rc = 0;
+		mode = kzalloc(sizeof(*mode), GFP_KERNEL);
+		if (!mode) {
+			rc =  -ENOMEM;
+			goto end;
+		}
+
+		of_property_read_u32(node, "sn,mode-h-active",
+						&mode->hdisplay);
+
+		of_property_read_u32(node, "sn,mode-h-front-porch",
+						&h_front_porch);
+		of_property_read_u32(node, "sn,mode-h-pulse-width",
+						&h_pulse_width);
+
+		of_property_read_u32(node, "sn,mode-h-back-porch",
+						&h_back_porch);
+
+		h_active_high = of_property_read_bool(node,
+						"sn,mode-h-active-high");
+
+		of_property_read_u32(node, "sn,mode-v-active",
+						&mode->vdisplay);
+		of_property_read_u32(node, "sn,mode-v-front-porch",
+						&v_front_porch);
+
+		of_property_read_u32(node, "sn,mode-v-pulse-width",
+						&v_pulse_width);
+		of_property_read_u32(node, "sn,mode-v-back-porch",
+						&v_back_porch);
+		v_active_high = of_property_read_bool(node,
+						"sn,mode-v-active-high");
+
+		of_property_read_u32(node, "sn,mode-refresh-rate",
+						&mode->vrefresh);
+
+		of_property_read_u32(node, "sn,mode-clock-in-khz",
+						&mode->clock);
+
+		mode->hsync_start = mode->hdisplay + h_front_porch;
+		mode->hsync_end = mode->hsync_start + h_pulse_width;
+		mode->htotal = mode->hsync_end + h_back_porch;
+		mode->vsync_start = mode->vdisplay + v_front_porch;
+		mode->vsync_end = mode->vsync_start + v_pulse_width;
+		mode->vtotal = mode->vsync_end + v_back_porch;
+
+		if (!mode->htotal || !mode->vtotal) {
+			rc = -EINVAL;
+			goto fail;
+		}
+
+		if (h_active_high)
+			flags |= DRM_MODE_FLAG_PHSYNC;
+		else
+			flags |= DRM_MODE_FLAG_NHSYNC;
+		if (v_active_high)
+			flags |= DRM_MODE_FLAG_PVSYNC;
+		else
+			flags |= DRM_MODE_FLAG_NVSYNC;
+		mode->flags = flags;
+
+		if (!rc) {
+			mode_count++;
+			list_add_tail(&mode->head, head);
+		}
+
+		drm_mode_set_name(mode);
+
+		pr_debug("mode[%s] h[%d,%d,%d,%d] v[%d,%d,%d,%d] %d %x %dkHZ\n",
+			mode->name, mode->hdisplay, mode->hsync_start,
+			mode->hsync_end, mode->htotal, mode->vdisplay,
+			mode->vsync_start, mode->vsync_end, mode->vtotal,
+			mode->vrefresh, mode->flags, mode->clock);
+fail:
+		if (rc) {
+			kfree(mode);
+			continue;
+		}
+	}
+
+	if (num_of_modes)
+		*num_of_modes = mode_count;
+
+end:
+	return rc;
+}
+
+static int sn65dsi86_parse_gpios(struct device_node *np,
+					struct sn65dsi86 *pdata)
+{
+	int ret = 0;
+
+	if (!pdata || !pdata->dev)
+		return -EINVAL;
+
+	pdata->gpios.enable_gpio = devm_gpiod_get(pdata->dev, "enable",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->gpios.enable_gpio)) {
+		pr_err("failed to get enable gpio from DT\n");
+		ret = PTR_ERR(pdata->gpios.enable_gpio);
+		goto exit;
+	}
+
+	pdata->gpios.aux_i2c_scl = devm_gpiod_get(pdata->dev, "aux-scl",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->gpios.aux_i2c_scl)) {
+		pr_err("failed to get aux scl gpio from DT\n");
+		ret = PTR_ERR(pdata->gpios.aux_i2c_scl);
+		goto exit;
+	}
+
+	pdata->gpios.aux_i2c_sda = devm_gpiod_get(pdata->dev, "aux-sda",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->gpios.aux_i2c_sda)) {
+		pr_err("failed to get aux sda gpio from DT\n");
+		ret = PTR_ERR(pdata->gpios.aux_i2c_sda);
+		goto exit;
+	}
+
+	pdata->gpios.edp_bias_en = devm_gpiod_get(pdata->dev, "bias-en",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->gpios.edp_bias_en)) {
+		pr_err("failed to get bias en gpio from DT\n");
+		ret = PTR_ERR(pdata->gpios.edp_bias_en);
+		goto exit;
+	}
+
+	pdata->gpios.edp_bklt_en = devm_gpiod_get(pdata->dev, "bklt-en",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->gpios.edp_bklt_en)) {
+		pr_err("failed to get bklt en gpio from DT\n");
+		ret = PTR_ERR(pdata->gpios.edp_bklt_en);
+		goto exit;
+	}
+
+	pdata->gpios.irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
+						  GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->gpios.irq_gpio))
+		pr_err("failed to get irq gpio from DT\n");
+
+	pdata->gpios.edp_bklt_ctrl = devm_gpiod_get_optional(pdata->dev,
+					"bklt-ctrl", GPIOD_OUT_HIGH);
+	if (IS_ERR(pdata->gpios.edp_bklt_ctrl))
+		pr_err("failed to get bklt ctrl gpio from DT\n");
+
+exit:
+	return ret;
+}
+
+static int sn65dsi86_parse_dt(struct device *dev, struct sn65dsi86 *pdata)
+{
+	struct device_node *np = dev->of_node;
+	struct device_node *end_node;
+	int ret = 0;
+
+	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
+	if (!end_node) {
+		pr_err("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) {
+		pr_err("remote node not found\n");
+		return -ENODEV;
+	}
+	of_node_put(pdata->host_node);
+
+	ret = sn65dsi86_parse_gpios(np, pdata);
+
+	pdata->is_pluggable = of_property_read_bool(np, "sn,is-pluggable");
+	pr_debug("is_pluggable = %d\n", pdata->is_pluggable);
+	if (!pdata->is_pluggable) {
+		INIT_LIST_HEAD(&pdata->mode_list);
+		sn65dsi86_parse_dt_modes(np,
+			&pdata->mode_list, &pdata->num_of_modes);
+	}
+
+	return ret;
+}
+
+static int sn65dsi86_probe(struct i2c_client *client,
+	 const struct i2c_device_id *id)
+{
+	struct sn65dsi86 *pdata;
+	int ret = 0;
+	struct drm_display_mode *mode, *n;
+
+	if (!client || !client->dev.of_node) {
+		pr_err("invalid input\n");
+		return -EINVAL;
+	}
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		pr_err("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	pdata = devm_kzalloc(&client->dev,
+		sizeof(struct sn65dsi86), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->power_on = false;
+	pdata->is_pluggable = false;
+	pdata->connector_status = connector_status_disconnected;
+	pdata->dev = &client->dev;
+	pdata->i2c_client = client;
+	pr_debug("I2C address is %x\n", client->addr);
+
+	ret = sn65dsi86_parse_dt(&client->dev, pdata);
+	if (ret) {
+		pr_err("failed to parse device tree\n");
+		goto err_dt_parse;
+	}
+
+	sn65dsi86_gpio_configure(pdata, true);
+
+	ret = sn65dsi86_init_regulators(pdata);
+	if (ret) {
+		pr_err("failed to enable regulators\n");
+		goto err_gpio_config;
+	}
+
+	ret = sn65dsi86_read_device_rev(pdata);
+	if (ret) {
+		pr_err("failed to read chip rev\n");
+		goto err_gpio_config;
+	} else {
+		pr_debug("bridge chip enabled successfully\n");
+		pdata->power_on = true;
+	}
+
+	pdata->irq = gpiod_to_irq(pdata->gpios.irq_gpio);
+	ret = request_threaded_irq(pdata->irq, NULL,
+			sn65dsi86_irq_thread_handler,
+			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+			"sn65dsi86", pdata);
+
+	i2c_set_clientdata(client, pdata);
+	dev_set_drvdata(&client->dev, pdata);
+
+	pdata->bridge.funcs = &sn65dsi86_bridge_funcs;
+	pdata->bridge.of_node = client->dev.of_node;
+
+	drm_bridge_add(&pdata->bridge);
+
+	return ret;
+
+err_gpio_config:
+	sn65dsi86_gpio_configure(pdata, false);
+err_dt_parse:
+	if (!pdata->is_pluggable) {
+		list_for_each_entry_safe(mode, n, &pdata->mode_list, head) {
+			list_del(&mode->head);
+			kfree(mode);
+		}
+		pdata->num_of_modes = 0;
+	}
+	devm_kfree(&client->dev, pdata);
+	return ret;
+}
+
+static int sn65dsi86_remove(struct i2c_client *client)
+{
+	int ret = -EINVAL;
+	struct sn65dsi86 *pdata = i2c_get_clientdata(client);
+	struct drm_display_mode *mode, *n;
+
+	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);
+
+	sn65dsi86_gpio_configure(pdata, false);
+
+	if (!pdata->is_pluggable) {
+		list_for_each_entry_safe(mode, n, &pdata->mode_list, head) {
+			list_del(&mode->head);
+			kfree(mode);
+		}
+	}
+
+	devm_kfree(&client->dev, pdata);
+
+end:
+	return ret;
+}
+
+static struct i2c_device_id sn65dsi86_id[] = {
+	{ "ti,sn65dsi86", 0},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, sn65dsi86_id);
+
+static const struct of_device_id sn65dsi86_match_table[] = {
+	{.compatible = "ti,sn65dsi86"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, sn65dsi86_match_table);
+
+static struct i2c_driver sn65dsi86_driver = {
+	.driver = {
+		.name = "sn65dsi86",
+		.owner = THIS_MODULE,
+		.of_match_table = sn65dsi86_match_table,
+	},
+	.probe = sn65dsi86_probe,
+	.remove = sn65dsi86_remove,
+	.id_table = sn65dsi86_id,
+};
+
+module_i2c_driver(sn65dsi86_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
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sandeep Panda]

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

Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
---
 .../bindings/display/bridge/ti,sn65dsi86.txt       | 75 ++++++++++++++++++++++
 1 file changed, 75 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
new file mode 100644
index 0000000..cbd2f0e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
@@ -0,0 +1,75 @@
+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 specification for EDP_BRIJ_EN pin
+- aux-sda-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SDA pin of AUX channel
+- aux-scl-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SCL pin of AUX channel
+- bias-en-gpios: OF device-tree gpio specification for EN_PP3300_DX_EDP pin to
+			enable 3.3V supply to eDP connector
+- bklt-en-gpios: OF device-tree gpio specification for AP_EDP_BKLTEN pin
+
+- vccio-supply: A 1.8V supply that powers up the PHY.
+- vcca-supply: A 1.2V supply that powers up the Controller.
+- vccs-supply: A 3.3V supply that power the eDP connector
+
+Optional properties:
+
+- irq-gpios: OF device-tree gpio specification for interrupt pin
+- bklt-ctrl-gpios: OF device-tree gpio specification for EDP_BKLTCTL pin
+
+- sn,is-pluggable: boolean property to specify if HPD supported or not
+- sn,custom-modes: OF device-tree specifiction to add support for custom modes
+
+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>;
+	aux-sda-gpios = <&msmgpio 34 GPIO_ACTIVE_HIGH>;
+	aux-scl-gpios = <&msmgpio 35 GPIO_ACTIVE_HIGH>;
+	bias-en-gpios = <&msmgpio 36 GPIO_ACTIVE_HIGH>;
+	bklt-en-gpios = <&msmgpio 37 GPIO_ACTIVE_HIGH>;
+
+	vccio-supply = <&pm8916_l17>;
+	vcca-supply = <&pm8916_l6>;
+	vccs-supply = <&pm8916_l7>;
+
+	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>;
+			};
+		};
+	};
+}
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sean Paul]

On Fri, Apr 13, 2018 at 1:23 AM Sandeep Panda <spanda@codeaurora.org> wrote:

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

> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
> ---
>   .../bindings/display/bridge/ti,sn65dsi86.txt       | 75
++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
>   create mode 100644
Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt

> diff --git
a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> new file mode 100644
> index 0000000..cbd2f0e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> @@ -0,0 +1,75 @@
> +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 specification for EDP_BRIJ_EN pin
> +- aux-sda-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SDA
pin of AUX channel
> +- aux-scl-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SCL
pin of AUX channel
> +- bias-en-gpios: OF device-tree gpio specification for EN_PP3300_DX_EDP
pin to


PP3300?


> +                       enable 3.3V supply to eDP connector
> +- bklt-en-gpios: OF device-tree gpio specification for AP_EDP_BKLTEN pin


I don't see any of these pins listed in the SN65DSI86 datasheet. Can you
please use the actual pin names?


> +
> +- vccio-supply: A 1.8V supply that powers up the PHY.
> +- vcca-supply: A 1.2V supply that powers up the Controller.
> +- vccs-supply: A 3.3V supply that power the eDP connector
> +
> +Optional properties:
> +
> +- irq-gpios: OF device-tree gpio specification for interrupt pin
> +- bklt-ctrl-gpios: OF device-tree gpio specification for EDP_BKLTCTL pin


Same comment here, this isn't listed in the datasheet


> +
> +- sn,is-pluggable: boolean property to specify if HPD supported or not


Could you infer this from whether there's a panel present in the dts?


> +- sn,custom-modes: OF device-tree specifiction to add support for custom
modes


Please drop custom-modes, it doesn't belong on the bridge.

> +
> +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>;
> +       aux-sda-gpios = <&msmgpio 34 GPIO_ACTIVE_HIGH>;
> +       aux-scl-gpios = <&msmgpio 35 GPIO_ACTIVE_HIGH>;
> +       bias-en-gpios = <&msmgpio 36 GPIO_ACTIVE_HIGH>;
> +       bklt-en-gpios = <&msmgpio 37 GPIO_ACTIVE_HIGH>;
> +
> +       vccio-supply = <&pm8916_l17>;
> +       vcca-supply = <&pm8916_l6>;
> +       vccs-supply = <&pm8916_l7>;
> +
> +       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>;
> +                       };
> +               };
> +       };
> +}
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sean Paul]

On Fri, Apr 13, 2018 at 10:53:00AM +0530, Sandeep Panda wrote:
> 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 <spanda@codeaurora.org>

Hi Sandeep,
Thank you for your patch!

> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 1019 +++++++++++++++++++++++++++++++++
>  1 file changed, 1019 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
> 
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> new file mode 100644
> index 0000000..93aa1ad
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -0,0 +1,1019 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#define pr_fmt(fmt) "%s: " fmt, __func__

Instead of using pr_* for logging, please use the DRM_* variants. Since there
is unlikely to be more than one of these bridge drivers in a system, you won't
need to use the DRM_DEV_* versions.

> +
> +#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/interrupt.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_graph.h>
> +#include <linux/of_irq.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>
> +
> +#define SN_DEVICE_REV_REG 0x08
> +
> +/* Link Training specific registers */
> +#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

It'd be nice if you could tab-align the values.

> +
> +struct sn65dsi86_reg_cfg {
> +	u8 reg;
> +	u8 val;
> +	int sleep_in_ms;
> +};
> +
> +struct sn65dsi86_video_cfg {
> +	u32 h_active;
> +	u32 h_front_porch;
> +	u32 h_pulse_width;
> +	u32 h_back_porch;
> +	bool h_polarity;
> +	u32 v_active;
> +	u32 v_front_porch;
> +	u32 v_pulse_width;
> +	u32 v_back_porch;
> +	bool v_polarity;
> +	u32 pclk_khz;
> +	u32 num_of_lanes;
> +};

All of these can be derived from drm_display_mode except for num_of_lanes which
is hardcoded. So let's just use drm_display_mode directly.

> +
> +struct sn65dsi86_gpios {
> +	struct gpio_desc *irq_gpio;
> +	struct gpio_desc *enable_gpio;
> +	struct gpio_desc *aux_i2c_sda;
> +	struct gpio_desc *aux_i2c_scl;
> +	struct gpio_desc *edp_bias_en;
> +	struct gpio_desc *edp_bklt_en;
> +	struct gpio_desc *edp_bklt_ctrl;
> +};

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.

> +
> +struct sn65dsi86 {

I will have fits trying to type this name. Could you please use something
simple, like sn_bridge? Same comment applies to all of the function names.

> +	struct device *dev;
> +	struct drm_bridge bridge;
> +	struct drm_connector connector;
> +
> +	struct device_node *host_node;
> +	struct mipi_dsi_device *dsi;
> +
> +	u8 i2c_addr;

Unused

> +	int irq;
> +
> +	struct sn65dsi86_gpios gpios;
> +
> +	unsigned int num_supplies;
> +	struct regulator_bulk_data *supplies;
> +
> +	struct i2c_client *i2c_client;
> +
> +	enum drm_connector_status connector_status;

You never use the value of this, you just assign it. So you can remove this.

> +	bool power_on;
> +
> +	bool is_pluggable;

As I mentioned in the dt review, you can determine this via panel. You should
also take this into account in detect().

> +	u32 num_of_modes;
> +	struct list_head mode_list;
> +	struct edid *edid;
> +
> +	struct drm_display_mode curr_mode;

This is not used anywhere

> +	struct sn65dsi86_video_cfg video_cfg;
> +};
> +
> +struct sn65dsi86_reg_cfg cfg_proto_0_init[] = {
> +	{SN_REFCLK_FREQ_REG, 0x02, 0x0},
> +	{SN_DSI_LANES_REG, 0x26, 0x14},
> +	{SN_DSIA_CLK_FREQ_REG, 0x7B, 0x0},
> +	{SN_ENH_FRAME_REG, 0x05, 0x0},
> +	{SN_SSC_CONFIG_REG, 0x30, 0x0},
> +	{SN_DATARATE_CONFIG_REG, 0x80, 0x0},
> +	{SN_PLL_ENABLE_REG, 0x01, 0x0},
> +	{SN_SCRAMBLE_CONFIG_REG, 0x00, 0x0},
> +	{SN_AUX_WDATA0_REG, 0x01, 0x0},
> +	{SN_AUX_ADDR_19_16_REG, 0x00, 0x0},
> +	{SN_AUX_ADDR_15_8_REG, 0x01, 0x0},
> +	{SN_AUX_ADDR_7_0_REG, 0x0A, 0x0},
> +	{SN_AUX_LENGTH_REG, 0x01, 0x0},
> +	{SN_AUX_CMD_REG, 0x81, 0x14},
> +	{SN_ML_TX_MODE_REG, 0x0A, 0x14},
> +	{SN_PAGE_SELECT_REG, 0x14, 0x0},
> +	{SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, 0x70, 0x0},
> +	{SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG, 0x08, 0x0},
> +	{SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, 0xA0, 0x0},
> +	{SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG, 0x05, 0x0},
> +	{SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, 0x20, 0x0},
> +	{SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG, 0x80, 0x0},
> +	{SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG, 0x0A, 0x0},
> +	{SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG, 0x80, 0x0},
> +	{SN_CHA_HORIZONTAL_BACK_PORCH_REG, 0x50, 0x0},
> +	{SN_CHA_VERTICAL_BACK_PORCH_REG, 0x1B, 0x0},
> +	{SN_CHA_HORIZONTAL_FRONT_PORCH_REG, 0x30, 0x0},
> +	{SN_CHA_VERTICAL_FRONT_PORCH_REG, 0x03, 0x0},
> +	{SN_DATA_FORMAT_REG, 0x00, 0x0},
> +	{SN_COLOR_BAR_CONFIG_REG, 0x00, 0x14},
> +	{SN_ENH_FRAME_REG, 0x0D, 0x0},
> +};

This should be programmed using the display parameters set out by mode/panel, as
opposed to just dumped in a table.

> +
> +static int sn65dsi86_write(struct sn65dsi86 *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) {
> +		pr_err("i2c write failed\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sn65dsi86_read(struct sn65dsi86 *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) {
> +		pr_err("i2c read failed\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sn65dsi86_write_array(struct sn65dsi86 *pdata,
> +	struct sn65dsi86_reg_cfg *cfg, int size)
> +{
> +	int ret = 0;
> +	int i;
> +
> +	size = size / sizeof(struct sn65dsi86_reg_cfg);
> +	for (i = 0; i < size; i++) {
> +		ret = sn65dsi86_write(pdata, cfg[i].reg, cfg[i].val);
> +
> +		if (ret != 0) {
> +			pr_err("reg writes failed. Last write %02X to %02X\n",
> +				cfg[i].val, cfg[i].reg);
> +			goto w_regs_fail;
> +		}
> +
> +		if (cfg[i].sleep_in_ms)
> +			msleep(cfg[i].sleep_in_ms);
> +	}
> +
> +w_regs_fail:
> +	if (ret != 0)
> +		pr_err("exiting with ret = %d after %d writes\n", ret, i);
> +
> +	return ret;
> +}
> +
> +static void sn65dsi86_gpio_configure(struct sn65dsi86 *pdata, bool on)
> +{
> +	int value;
> +
> +	value = on ? 1: 0;
> +
> +	gpiod_set_value(pdata->gpios.enable_gpio, value);
> +	gpiod_set_value(pdata->gpios.aux_i2c_sda, value);
> +	gpiod_set_value(pdata->gpios.aux_i2c_scl, value);
> +	gpiod_set_value(pdata->gpios.edp_bias_en, value);
> +	gpiod_set_value(pdata->gpios.edp_bklt_en, value);
> +	gpiod_set_value(pdata->gpios.irq_gpio, value);
> +	gpiod_set_value(pdata->gpios.edp_bklt_ctrl, value);
> +
> +	pr_debug("config to: %d\n", value);
> +}
> +
> +static void sn65dsi86_power_ctrl(struct sn65dsi86 *pdata, bool enable)
> +{
> +	if (!pdata)
> +		return;
> +
> +	if (!pdata->power_on && enable) {
> +		sn65dsi86_gpio_configure(pdata, true);
> +
> +		if (regulator_bulk_enable(pdata->num_supplies,
> +						pdata->supplies)) {
> +			pr_err("bridge regulator enable failed\n");
> +			return;
> +		}
> +		pdata->power_on = true;
> +	} else if (pdata->power_on && !enable) {
> +		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
> +
> +		sn65dsi86_gpio_configure(pdata, false);
> +		pdata->power_on = false;
> +	} else {
> +		pr_debug("unnecessary call to power control\n");
> +	}
> +}
> +
> +/* Connector funcs */
> +static struct sn65dsi86 *connector_to_sn65dsi86(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct sn65dsi86, connector);
> +}
> +
> +static int sn65dsi86_send_aux_cmd(struct sn65dsi86 *pdata,
> +				  u8 cmd, u8 addr, u8 length, int w_data)
> +{
> +	u8 read = 0;
> +	int retry_cnt = 10;
> +
> +	sn65dsi86_write(pdata, SN_AUX_CMD_REG, (cmd << 4));
> +	sn65dsi86_write(pdata, SN_AUX_ADDR_7_0_REG, addr);
> +	sn65dsi86_write(pdata, SN_AUX_LENGTH_REG, length);
> +	if (w_data >= 0)
> +		sn65dsi86_write(pdata, SN_AUX_WDATA0_REG, (u8)w_data);
> +
> +	/* set SEND bit */
> +	sn65dsi86_read(pdata, SN_AUX_CMD_REG, &read, 1);
> +	read |= BIT(0);
> +	sn65dsi86_write(pdata, SN_AUX_CMD_REG, read);
> +
> +	/* poll for bridge to ack SEND bit */
> +	while (retry_cnt) {
> +		sn65dsi86_read(pdata, SN_AUX_CMD_REG, &read, 0x1);
> +		if (!(read & BIT(0)))
> +			break;
> +		retry_cnt--;
> +		udelay(1000);
> +	}
> +
> +	if (!retry_cnt) {
> +		pr_err("aux_cmd transfer failed\n");
> +		return -EINVAL;
> +	}
> +	return 0;
> +}
> +
> +static int sn65dsi86_read_edid(struct sn65dsi86 *pdata, u8 *buf)
> +{
> +	int i = 0;
> +	u8 addr = SN_AUX_RDATA0_REG;
> +	u8 *data = buf;
> +
> +	if (!data)
> +		return -ENOMEM;
> +
> +	if (sn65dsi86_send_aux_cmd(pdata, 0x4, 0x50, 0x0, -1) ||
> +		sn65dsi86_send_aux_cmd(pdata, 0x4, 0x50, 0x01, 0x0) ||
> +		sn65dsi86_send_aux_cmd(pdata, 0x5, 0x50, 0x0, -1) ||
> +		sn65dsi86_send_aux_cmd(pdata, 0x5, 0x50, 0x10, -1))
> +		goto error;

Instead of hand-crafting this, you should implement an i2c_adapter. You can then
use that in drm_get_edid() to read the edid via the core.

See the comment about drm_do_get_edid():
 * drivers must make all reasonable efforts to expose it as an I2C
 * adapter and use drm_get_edid() instead of abusing this function.

> +
> +	for (i = 0; i < 16; i++) {
> +		if (sn65dsi86_read(pdata, addr, data, 0x1))
> +			goto error;
> +		addr++;
> +		data++;
> +	}
> +
> +	return 0;
> +error:
> +	pr_err("edid read over i2c failed\n");
> +	return -EINVAL;
> +}
> +
> +static int sn65dsi86_read_edid_block(struct sn65dsi86 *pdata,
> +			       u8 *buf, unsigned int block)
> +{
> +	if (block == 0) {
> +		if (sn65dsi86_read_edid(pdata, buf))
> +			goto error;
> +	} else if (block == 1) {
> +		/* move segment pointer */
> +		if (sn65dsi86_send_aux_cmd(pdata, 0x4, 0x30, 0x0, -1) ||
> +			sn65dsi86_send_aux_cmd(pdata, 0x4, 0x30, 0x01, 0x1) ||
> +			sn65dsi86_send_aux_cmd(pdata, 0x0, 0x30, 0x00, -1))
> +			goto error;
> +		else
> +			if (sn65dsi86_read_edid(pdata, buf))
> +				goto error;
> +	} else {
> +		pr_debug("unsupported edid block\n");
> +		goto error;
> +	}
> +
> +	return 0;
> +error:
> +	pr_err("edid block read failed\n");
> +	return -EINVAL;
> +}
> +
> +static int sn65dsi86_get_edid_block(void *data, u8 *buf, unsigned int block,
> +				  size_t len)
> +{
> +	struct sn65dsi86 *pdata = data;
> +	int ret = 0;
> +
> +	pr_debug("get edid block: block=%d, len=%d\n", block, (int)len);
> +
> +	if (len > 128 || block > 1)
> +		return -EINVAL;
> +
> +	ret = sn65dsi86_read_edid_block(pdata, buf, block);
> +	if (ret) {
> +		pr_err("edid read failed for block: %d ret: %d\n", block, ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int sn65dsi86_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
> +	struct drm_display_mode *mode, *m;
> +
> +	if (pdata->edid)
> +		return drm_add_edid_modes(connector, pdata->edid);

What if the edid changes? You'll never update it.

> +
> +	sn65dsi86_power_ctrl(pdata, true);
> +	if (pdata->is_pluggable) {

Here, call the panel func if a panel is connected, or probe the edid.

> +		pdata->edid = drm_do_get_edid(connector,
> +				sn65dsi86_get_edid_block, pdata);

Are you leaking memory here?

> +
> +		drm_mode_connector_update_edid_property(connector, pdata->edid);
> +		pdata->num_of_modes = drm_add_edid_modes(connector,
> +								pdata->edid);

You should be able to remove num_of_modes and mode_list once you remove the dt
custom modes. Make sure you incorporate a call to drm_panel_get_modes() here.

> +	}
> +
> +	if (!pdata->is_pluggable || !pdata->num_of_modes) {
> +		/*
> +		 * if device does not support HPD or due to some reason
> +		 * EDID read failed then fall back to mode_list which is
> +		 * already parsed from dt if any.
> +		 */
> +		list_for_each_entry(mode, &pdata->mode_list, head) {
> +			m = drm_mode_duplicate(connector->dev, mode);
> +			if (!m) {
> +				pr_err("failed to get mode %dx%d\n",
> +					mode->hdisplay, mode->vdisplay);
> +				break;
> +			}
> +			drm_mode_probed_add(connector, m);
> +		}
> +	}
> +
> +	sn65dsi86_power_ctrl(pdata, false);

What if power was already on?

> +	return pdata->num_of_modes;
> +}
> +
> +static enum drm_mode_status
> +sn65dsi86_connector_mode_valid(struct drm_connector *connector,
> +			     struct drm_display_mode *mode)
> +{
> +	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
> +	struct drm_display_mode *m;
> +
> +	if (pdata->edid)
> +		return MODE_OK;

Careful here, userspace could add an invalid mode and this would say it's
correct. Instead of doing this, and the check below, just check the given mode
against the max hardware limitations listed in the datasheet (4k60). I assume
this will also depend on how many dsi channels we have coming in.

> +
> +	if (!pdata->is_pluggable) {
> +		list_for_each_entry(m, &pdata->mode_list, head) {
> +			if (m->hdisplay == mode->hdisplay &&
> +				m->vdisplay == mode->vdisplay)
> +				return MODE_OK;
> +		}
> +	}
> +
> +	return MODE_BAD;
> +}
> +
> +static struct drm_connector_helper_funcs sn65dsi86_connector_helper_funcs = {
> +	.get_modes = sn65dsi86_connector_get_modes,
> +	.mode_valid = sn65dsi86_connector_mode_valid,
> +};
> +
> +static enum drm_connector_status
> +sn65dsi86_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
> +
> +	pdata->connector_status = pdata->power_on ?
> +		connector_status_connected : connector_status_disconnected;

It's possible detect() will be called while the device is off, in that case
this will return the incorrect value. Is there some reason you can't just detect
whether there's something connected?

> +
> +	return pdata->connector_status;
> +}
> +
> +static const struct drm_connector_funcs sn65dsi86_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = sn65dsi86_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 sn65dsi86 *bridge_to_sn65dsi86(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct sn65dsi86, bridge);
> +}
> +
> +static int sn65dsi86_read_device_rev(struct sn65dsi86 *pdata)
> +{
> +	u8 rev = 0;
> +	int ret = 0;
> +
> +	ret = sn65dsi86_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
> +

extra line

> +	if (!ret) {

In this case, flip the condition so you can avoid indenting everything:

        if (ret)
                return ret;

> +		if (rev == 0x2) {

What's 0x2? Can you please #define this?

> +			pr_info("SN65DSI86 revision id: 0x%x\n", rev);

While I personally appreciate info messages, others not so much. So downgrade
this to DRM_DEBUG_KMS()

> +		} else {
> +			pr_warn("SN65DSI86 revision id mismatch\n");
> +			ret = -EINVAL;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static irqreturn_t sn65dsi86_irq_thread_handler(int irq, void *dev_id)
> +{
> +	return IRQ_HANDLED;
> +}

What's the point of even registering this? Just leave it all out if it's unused.
If there's a need in the future, it can easily be added back in.

> +
> +static const char * const sn65dsi86_supply_names[] = {
> +	"vccio",
> +	"vcca",
> +	"vccs"
> +};
> +
> +static int sn65dsi86_init_regulators(struct sn65dsi86 *pdata)
> +{
> +	const char * const *supply_names;
> +	unsigned int i;
> +	int ret;
> +
> +	supply_names = sn65dsi86_supply_names;

This local isn't necessary.

> +	pdata->num_supplies = ARRAY_SIZE(sn65dsi86_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 = supply_names[i];
> +
> +	ret = devm_regulator_bulk_get(pdata->dev,
> +			pdata->num_supplies, pdata->supplies);
> +	if (ret)
> +		return ret;
> +
> +	return regulator_bulk_enable(pdata->num_supplies, pdata->supplies);
> +}
> +
> +static int sn65dsi86_bridge_attach(struct drm_bridge *bridge)
> +{
> +	struct mipi_dsi_host *host;
> +	struct mipi_dsi_device *dsi;
> +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
> +	int ret;
> +	const struct mipi_dsi_device_info info = { .type = "sn65dsi86",
> +						   .channel = 0,
> +						   .node = NULL,
> +						 };
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found");
> +		return -ENODEV;
> +	}
> +
> +	/* HPD not supported */
> +	pdata->connector.polled = 0;
> +

You'll need to refactor the below to accommodate panels. If you're not planning
on supporting hotplug, you should probably remove all of the connector-related
stuff from this driver, since you will always be using a panel driver.

> +	ret = drm_connector_init(bridge->dev, &pdata->connector,
> +				 &sn65dsi86_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,
> +				 &sn65dsi86_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) {
> +		pr_err("failed to find dsi host\n");
> +		return -ENODEV;
> +	}
> +
> +	dsi = mipi_dsi_device_register_full(host, &info);
> +	if (IS_ERR(dsi)) {
> +		pr_err("failed to create dsi device\n");
> +		ret = PTR_ERR(dsi);
> +		goto err_dsi_device;
> +	}
> +
> +	/* setting to 4 lanes always for now */
> +	dsi->lanes = 4;

Perhaps add a TODO for this?

> +	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) {
> +		pr_err("failed to attach dsi to host\n");
> +		goto err_dsi_attach;
> +	}
> +
> +	pdata->dsi = dsi;
> +
> +	pr_debug("bridge attached\n");
> +
> +	return 0;
> +
> +err_dsi_attach:
> +	mipi_dsi_device_unregister(dsi);
> +err_dsi_device:
> +	return ret;
> +}
> +
> +static void sn65dsi86_set_video_cfg(struct sn65dsi86 *pdata,
> +	struct drm_display_mode *mode,
> +	struct sn65dsi86_video_cfg *video_cfg)
> +{
> +	video_cfg->h_active = mode->hdisplay;
> +	video_cfg->v_active = mode->vdisplay;
> +	video_cfg->h_front_porch = mode->hsync_start - mode->hdisplay;
> +	video_cfg->v_front_porch = mode->vsync_start - mode->vdisplay;
> +	video_cfg->h_back_porch = mode->htotal - mode->hsync_end;
> +	video_cfg->v_back_porch = mode->vtotal - mode->vsync_end;
> +	video_cfg->h_pulse_width = mode->hsync_end - mode->hsync_start;
> +	video_cfg->v_pulse_width = mode->vsync_end - mode->vsync_start;
> +	video_cfg->pclk_khz = mode->clock;
> +
> +	video_cfg->h_polarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
> +	video_cfg->v_polarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
> +
> +	/* setting to 4 lanes always for now */
> +	video_cfg->num_of_lanes = 4;
> +
> +	pr_debug("video=h[%d,%d,%d,%d] v[%d,%d,%d,%d] pclk=%d lane=%d\n",
> +		video_cfg->h_active, video_cfg->h_front_porch,
> +		video_cfg->h_pulse_width, video_cfg->h_back_porch,
> +		video_cfg->v_active, video_cfg->v_front_porch,
> +		video_cfg->v_pulse_width, video_cfg->v_back_porch,
> +		video_cfg->pclk_khz, video_cfg->num_of_lanes);
> +}

As mentioned above, this needs to be removed.

> +
> +static void sn65dsi86_bridge_mode_set(struct drm_bridge *bridge,
> +				    struct drm_display_mode *mode,
> +				    struct drm_display_mode *adj_mode)
> +{
> +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
> +	struct sn65dsi86_video_cfg *video_cfg = &pdata->video_cfg;
> +	int ret = 0;
> +
> +	pr_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);
> +
> +	memset(video_cfg, 0, sizeof(struct sn65dsi86_video_cfg));
> +	sn65dsi86_set_video_cfg(pdata, adj_mode, video_cfg);
> +
> +	if (video_cfg->num_of_lanes != pdata->dsi->lanes) {
> +		mipi_dsi_detach(pdata->dsi);
> +		pdata->dsi->lanes = video_cfg->num_of_lanes;
> +		ret = mipi_dsi_attach(pdata->dsi);

Hmm, if this is configurable is there no better way of doing this than detaching
and attaching?

> +		if (ret)
> +			pr_err("failed to change host lanes\n");
> +	}
> +}
> +
> +static void sn65dsi86_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
> +
> +	sn65dsi86_power_ctrl(pdata, false);
> +	pdata->connector_status =  connector_status_disconnected;
> +}
> +
> +static void sn65dsi86_bridge_enable(struct drm_bridge *bridge)
> +{
> +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
> +
> +	sn65dsi86_power_ctrl(pdata, true);
> +	pdata->connector_status =  connector_status_connected;
> +	sn65dsi86_write_array(pdata, cfg_proto_0_init,
> +			      sizeof(cfg_proto_0_init));

As mentioned above, most these should likely be set in mode_set() using the
values set in the current mode.

Sean

> +}
> +
> +static const struct drm_bridge_funcs sn65dsi86_bridge_funcs = {
> +	.attach = sn65dsi86_bridge_attach,
> +	.enable = sn65dsi86_bridge_enable,
> +	.disable = sn65dsi86_bridge_disable,
> +	.mode_set = sn65dsi86_bridge_mode_set,
> +};
> +
> +static int sn65dsi86_parse_dt_modes(struct device_node *np,
> +					struct list_head *head,
> +					u32 *num_of_modes)
> +{
> +	int rc = 0;
> +	struct drm_display_mode *mode;
> +	u32 mode_count = 0;
> +	struct device_node *node = NULL;
> +	struct device_node *root_node = NULL;
> +	u32 h_front_porch, h_pulse_width, h_back_porch;
> +	u32 v_front_porch, v_pulse_width, v_back_porch;
> +	bool h_active_high, v_active_high;
> +	u32 flags = 0;
> +
> +	root_node = of_get_child_by_name(np, "sn,custom-modes");
> +	if (!root_node) {
> +		root_node = of_parse_phandle(np, "sn,custom-modes", 0);
> +		if (!root_node) {
> +			pr_info("No modes present for sn,custom-modes");
> +			goto end;
> +		}
> +	}
> +
> +	for_each_child_of_node(root_node, node) {
> +		rc = 0;
> +		mode = kzalloc(sizeof(*mode), GFP_KERNEL);
> +		if (!mode) {
> +			rc =  -ENOMEM;
> +			goto end;
> +		}
> +
> +		of_property_read_u32(node, "sn,mode-h-active",
> +						&mode->hdisplay);
> +
> +		of_property_read_u32(node, "sn,mode-h-front-porch",
> +						&h_front_porch);
> +		of_property_read_u32(node, "sn,mode-h-pulse-width",
> +						&h_pulse_width);
> +
> +		of_property_read_u32(node, "sn,mode-h-back-porch",
> +						&h_back_porch);
> +
> +		h_active_high = of_property_read_bool(node,
> +						"sn,mode-h-active-high");
> +
> +		of_property_read_u32(node, "sn,mode-v-active",
> +						&mode->vdisplay);
> +		of_property_read_u32(node, "sn,mode-v-front-porch",
> +						&v_front_porch);
> +
> +		of_property_read_u32(node, "sn,mode-v-pulse-width",
> +						&v_pulse_width);
> +		of_property_read_u32(node, "sn,mode-v-back-porch",
> +						&v_back_porch);
> +		v_active_high = of_property_read_bool(node,
> +						"sn,mode-v-active-high");
> +
> +		of_property_read_u32(node, "sn,mode-refresh-rate",
> +						&mode->vrefresh);
> +
> +		of_property_read_u32(node, "sn,mode-clock-in-khz",
> +						&mode->clock);
> +
> +		mode->hsync_start = mode->hdisplay + h_front_porch;
> +		mode->hsync_end = mode->hsync_start + h_pulse_width;
> +		mode->htotal = mode->hsync_end + h_back_porch;
> +		mode->vsync_start = mode->vdisplay + v_front_porch;
> +		mode->vsync_end = mode->vsync_start + v_pulse_width;
> +		mode->vtotal = mode->vsync_end + v_back_porch;
> +
> +		if (!mode->htotal || !mode->vtotal) {
> +			rc = -EINVAL;
> +			goto fail;
> +		}
> +
> +		if (h_active_high)
> +			flags |= DRM_MODE_FLAG_PHSYNC;
> +		else
> +			flags |= DRM_MODE_FLAG_NHSYNC;
> +		if (v_active_high)
> +			flags |= DRM_MODE_FLAG_PVSYNC;
> +		else
> +			flags |= DRM_MODE_FLAG_NVSYNC;
> +		mode->flags = flags;
> +
> +		if (!rc) {
> +			mode_count++;
> +			list_add_tail(&mode->head, head);
> +		}
> +
> +		drm_mode_set_name(mode);
> +
> +		pr_debug("mode[%s] h[%d,%d,%d,%d] v[%d,%d,%d,%d] %d %x %dkHZ\n",
> +			mode->name, mode->hdisplay, mode->hsync_start,
> +			mode->hsync_end, mode->htotal, mode->vdisplay,
> +			mode->vsync_start, mode->vsync_end, mode->vtotal,
> +			mode->vrefresh, mode->flags, mode->clock);
> +fail:
> +		if (rc) {
> +			kfree(mode);
> +			continue;
> +		}
> +	}
> +
> +	if (num_of_modes)
> +		*num_of_modes = mode_count;
> +
> +end:
> +	return rc;
> +}
> +
> +static int sn65dsi86_parse_gpios(struct device_node *np,
> +					struct sn65dsi86 *pdata)
> +{
> +	int ret = 0;
> +
> +	if (!pdata || !pdata->dev)
> +		return -EINVAL;
> +
> +	pdata->gpios.enable_gpio = devm_gpiod_get(pdata->dev, "enable",
> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->gpios.enable_gpio)) {
> +		pr_err("failed to get enable gpio from DT\n");
> +		ret = PTR_ERR(pdata->gpios.enable_gpio);
> +		goto exit;
> +	}
> +
> +	pdata->gpios.aux_i2c_scl = devm_gpiod_get(pdata->dev, "aux-scl",
> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->gpios.aux_i2c_scl)) {
> +		pr_err("failed to get aux scl gpio from DT\n");
> +		ret = PTR_ERR(pdata->gpios.aux_i2c_scl);
> +		goto exit;
> +	}
> +
> +	pdata->gpios.aux_i2c_sda = devm_gpiod_get(pdata->dev, "aux-sda",
> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->gpios.aux_i2c_sda)) {
> +		pr_err("failed to get aux sda gpio from DT\n");
> +		ret = PTR_ERR(pdata->gpios.aux_i2c_sda);
> +		goto exit;
> +	}
> +
> +	pdata->gpios.edp_bias_en = devm_gpiod_get(pdata->dev, "bias-en",
> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->gpios.edp_bias_en)) {
> +		pr_err("failed to get bias en gpio from DT\n");
> +		ret = PTR_ERR(pdata->gpios.edp_bias_en);
> +		goto exit;
> +	}
> +
> +	pdata->gpios.edp_bklt_en = devm_gpiod_get(pdata->dev, "bklt-en",
> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->gpios.edp_bklt_en)) {
> +		pr_err("failed to get bklt en gpio from DT\n");
> +		ret = PTR_ERR(pdata->gpios.edp_bklt_en);
> +		goto exit;
> +	}
> +
> +	pdata->gpios.irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
> +						  GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->gpios.irq_gpio))
> +		pr_err("failed to get irq gpio from DT\n");
> +
> +	pdata->gpios.edp_bklt_ctrl = devm_gpiod_get_optional(pdata->dev,
> +					"bklt-ctrl", GPIOD_OUT_HIGH);
> +	if (IS_ERR(pdata->gpios.edp_bklt_ctrl))
> +		pr_err("failed to get bklt ctrl gpio from DT\n");
> +
> +exit:
> +	return ret;
> +}
> +
> +static int sn65dsi86_parse_dt(struct device *dev, struct sn65dsi86 *pdata)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct device_node *end_node;
> +	int ret = 0;
> +
> +	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
> +	if (!end_node) {
> +		pr_err("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) {
> +		pr_err("remote node not found\n");
> +		return -ENODEV;
> +	}
> +	of_node_put(pdata->host_node);
> +
> +	ret = sn65dsi86_parse_gpios(np, pdata);
> +
> +	pdata->is_pluggable = of_property_read_bool(np, "sn,is-pluggable");
> +	pr_debug("is_pluggable = %d\n", pdata->is_pluggable);
> +	if (!pdata->is_pluggable) {
> +		INIT_LIST_HEAD(&pdata->mode_list);
> +		sn65dsi86_parse_dt_modes(np,
> +			&pdata->mode_list, &pdata->num_of_modes);
> +	}
> +
> +	return ret;
> +}
> +
> +static int sn65dsi86_probe(struct i2c_client *client,
> +	 const struct i2c_device_id *id)
> +{
> +	struct sn65dsi86 *pdata;
> +	int ret = 0;
> +	struct drm_display_mode *mode, *n;
> +
> +	if (!client || !client->dev.of_node) {
> +		pr_err("invalid input\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		pr_err("device doesn't support I2C\n");
> +		return -ENODEV;
> +	}
> +
> +	pdata = devm_kzalloc(&client->dev,
> +		sizeof(struct sn65dsi86), GFP_KERNEL);
> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->power_on = false;
> +	pdata->is_pluggable = false;
> +	pdata->connector_status = connector_status_disconnected;
> +	pdata->dev = &client->dev;
> +	pdata->i2c_client = client;
> +	pr_debug("I2C address is %x\n", client->addr);
> +
> +	ret = sn65dsi86_parse_dt(&client->dev, pdata);
> +	if (ret) {
> +		pr_err("failed to parse device tree\n");
> +		goto err_dt_parse;
> +	}
> +
> +	sn65dsi86_gpio_configure(pdata, true);
> +
> +	ret = sn65dsi86_init_regulators(pdata);
> +	if (ret) {
> +		pr_err("failed to enable regulators\n");
> +		goto err_gpio_config;
> +	}
> +
> +	ret = sn65dsi86_read_device_rev(pdata);
> +	if (ret) {
> +		pr_err("failed to read chip rev\n");
> +		goto err_gpio_config;
> +	} else {
> +		pr_debug("bridge chip enabled successfully\n");
> +		pdata->power_on = true;
> +	}
> +
> +	pdata->irq = gpiod_to_irq(pdata->gpios.irq_gpio);
> +	ret = request_threaded_irq(pdata->irq, NULL,
> +			sn65dsi86_irq_thread_handler,
> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +			"sn65dsi86", pdata);
> +
> +	i2c_set_clientdata(client, pdata);
> +	dev_set_drvdata(&client->dev, pdata);
> +
> +	pdata->bridge.funcs = &sn65dsi86_bridge_funcs;
> +	pdata->bridge.of_node = client->dev.of_node;
> +
> +	drm_bridge_add(&pdata->bridge);
> +
> +	return ret;
> +
> +err_gpio_config:
> +	sn65dsi86_gpio_configure(pdata, false);
> +err_dt_parse:
> +	if (!pdata->is_pluggable) {
> +		list_for_each_entry_safe(mode, n, &pdata->mode_list, head) {
> +			list_del(&mode->head);
> +			kfree(mode);
> +		}
> +		pdata->num_of_modes = 0;
> +	}
> +	devm_kfree(&client->dev, pdata);
> +	return ret;
> +}
> +
> +static int sn65dsi86_remove(struct i2c_client *client)
> +{
> +	int ret = -EINVAL;
> +	struct sn65dsi86 *pdata = i2c_get_clientdata(client);
> +	struct drm_display_mode *mode, *n;
> +
> +	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);
> +
> +	sn65dsi86_gpio_configure(pdata, false);
> +
> +	if (!pdata->is_pluggable) {
> +		list_for_each_entry_safe(mode, n, &pdata->mode_list, head) {
> +			list_del(&mode->head);
> +			kfree(mode);
> +		}
> +	}
> +
> +	devm_kfree(&client->dev, pdata);
> +
> +end:
> +	return ret;
> +}
> +
> +static struct i2c_device_id sn65dsi86_id[] = {
> +	{ "ti,sn65dsi86", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, sn65dsi86_id);
> +
> +static const struct of_device_id sn65dsi86_match_table[] = {
> +	{.compatible = "ti,sn65dsi86"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, sn65dsi86_match_table);
> +
> +static struct i2c_driver sn65dsi86_driver = {
> +	.driver = {
> +		.name = "sn65dsi86",
> +		.owner = THIS_MODULE,
> +		.of_match_table = sn65dsi86_match_table,
> +	},
> +	.probe = sn65dsi86_probe,
> +	.remove = sn65dsi86_remove,
> +	.id_table = sn65dsi86_id,
> +};
> +
> +module_i2c_driver(sn65dsi86_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
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[spanda-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-04-14 00:59, Sean Paul wrote:
> On Fri, Apr 13, 2018 at 10:53:00AM +0530, Sandeep Panda wrote:
>> 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 <spanda@codeaurora.org>
> 
> Hi Sandeep,
> Thank you for your patch!
> 
>> ---
>>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 1019 
>> +++++++++++++++++++++++++++++++++
>>  1 file changed, 1019 insertions(+)
>>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> 
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
>> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> new file mode 100644
>> index 0000000..93aa1ad
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -0,0 +1,1019 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#define pr_fmt(fmt) "%s: " fmt, __func__
> 
> Instead of using pr_* for logging, please use the DRM_* variants. Since 
> there
> is unlikely to be more than one of these bridge drivers in a system, 
> you won't
> need to use the DRM_DEV_* versions.
> 
>> +
>> +#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/interrupt.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/of_irq.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>
>> +
>> +#define SN_DEVICE_REV_REG 0x08
>> +
>> +/* Link Training specific registers */
>> +#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
> 
> It'd be nice if you could tab-align the values.
> 
>> +
>> +struct sn65dsi86_reg_cfg {
>> +	u8 reg;
>> +	u8 val;
>> +	int sleep_in_ms;
>> +};
>> +
>> +struct sn65dsi86_video_cfg {
>> +	u32 h_active;
>> +	u32 h_front_porch;
>> +	u32 h_pulse_width;
>> +	u32 h_back_porch;
>> +	bool h_polarity;
>> +	u32 v_active;
>> +	u32 v_front_porch;
>> +	u32 v_pulse_width;
>> +	u32 v_back_porch;
>> +	bool v_polarity;
>> +	u32 pclk_khz;
>> +	u32 num_of_lanes;
>> +};
> 
> All of these can be derived from drm_display_mode except for 
> num_of_lanes which
> is hardcoded. So let's just use drm_display_mode directly.
> 
>> +
>> +struct sn65dsi86_gpios {
>> +	struct gpio_desc *irq_gpio;
>> +	struct gpio_desc *enable_gpio;
>> +	struct gpio_desc *aux_i2c_sda;
>> +	struct gpio_desc *aux_i2c_scl;
>> +	struct gpio_desc *edp_bias_en;
>> +	struct gpio_desc *edp_bklt_en;
>> +	struct gpio_desc *edp_bklt_ctrl;
>> +};
> 
> 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.
> 
>> +
>> +struct sn65dsi86 {
> 
> I will have fits trying to type this name. Could you please use 
> something
> simple, like sn_bridge? Same comment applies to all of the function 
> names.
> 
>> +	struct device *dev;
>> +	struct drm_bridge bridge;
>> +	struct drm_connector connector;
>> +
>> +	struct device_node *host_node;
>> +	struct mipi_dsi_device *dsi;
>> +
>> +	u8 i2c_addr;
> 
> Unused
> 
>> +	int irq;
>> +
>> +	struct sn65dsi86_gpios gpios;
>> +
>> +	unsigned int num_supplies;
>> +	struct regulator_bulk_data *supplies;
>> +
>> +	struct i2c_client *i2c_client;
>> +
>> +	enum drm_connector_status connector_status;
> 
> You never use the value of this, you just assign it. So you can remove 
> this.
> 
>> +	bool power_on;
>> +
>> +	bool is_pluggable;
> 
> As I mentioned in the dt review, you can determine this via panel. You 
> should
> also take this into account in detect().
> 
>> +	u32 num_of_modes;
>> +	struct list_head mode_list;
>> +	struct edid *edid;
>> +
>> +	struct drm_display_mode curr_mode;
> 
> This is not used anywhere
> 
>> +	struct sn65dsi86_video_cfg video_cfg;
>> +};
>> +
>> +struct sn65dsi86_reg_cfg cfg_proto_0_init[] = {
>> +	{SN_REFCLK_FREQ_REG, 0x02, 0x0},
>> +	{SN_DSI_LANES_REG, 0x26, 0x14},
>> +	{SN_DSIA_CLK_FREQ_REG, 0x7B, 0x0},
>> +	{SN_ENH_FRAME_REG, 0x05, 0x0},
>> +	{SN_SSC_CONFIG_REG, 0x30, 0x0},
>> +	{SN_DATARATE_CONFIG_REG, 0x80, 0x0},
>> +	{SN_PLL_ENABLE_REG, 0x01, 0x0},
>> +	{SN_SCRAMBLE_CONFIG_REG, 0x00, 0x0},
>> +	{SN_AUX_WDATA0_REG, 0x01, 0x0},
>> +	{SN_AUX_ADDR_19_16_REG, 0x00, 0x0},
>> +	{SN_AUX_ADDR_15_8_REG, 0x01, 0x0},
>> +	{SN_AUX_ADDR_7_0_REG, 0x0A, 0x0},
>> +	{SN_AUX_LENGTH_REG, 0x01, 0x0},
>> +	{SN_AUX_CMD_REG, 0x81, 0x14},
>> +	{SN_ML_TX_MODE_REG, 0x0A, 0x14},
>> +	{SN_PAGE_SELECT_REG, 0x14, 0x0},
>> +	{SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG, 0x70, 0x0},
>> +	{SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG, 0x08, 0x0},
>> +	{SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG, 0xA0, 0x0},
>> +	{SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG, 0x05, 0x0},
>> +	{SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG, 0x20, 0x0},
>> +	{SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG, 0x80, 0x0},
>> +	{SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG, 0x0A, 0x0},
>> +	{SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG, 0x80, 0x0},
>> +	{SN_CHA_HORIZONTAL_BACK_PORCH_REG, 0x50, 0x0},
>> +	{SN_CHA_VERTICAL_BACK_PORCH_REG, 0x1B, 0x0},
>> +	{SN_CHA_HORIZONTAL_FRONT_PORCH_REG, 0x30, 0x0},
>> +	{SN_CHA_VERTICAL_FRONT_PORCH_REG, 0x03, 0x0},
>> +	{SN_DATA_FORMAT_REG, 0x00, 0x0},
>> +	{SN_COLOR_BAR_CONFIG_REG, 0x00, 0x14},
>> +	{SN_ENH_FRAME_REG, 0x0D, 0x0},
>> +};
> 
> This should be programmed using the display parameters set out by 
> mode/panel, as
> opposed to just dumped in a table.
> 
>> +
>> +static int sn65dsi86_write(struct sn65dsi86 *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) {
>> +		pr_err("i2c write failed\n");
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int sn65dsi86_read(struct sn65dsi86 *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) {
>> +		pr_err("i2c read failed\n");
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int sn65dsi86_write_array(struct sn65dsi86 *pdata,
>> +	struct sn65dsi86_reg_cfg *cfg, int size)
>> +{
>> +	int ret = 0;
>> +	int i;
>> +
>> +	size = size / sizeof(struct sn65dsi86_reg_cfg);
>> +	for (i = 0; i < size; i++) {
>> +		ret = sn65dsi86_write(pdata, cfg[i].reg, cfg[i].val);
>> +
>> +		if (ret != 0) {
>> +			pr_err("reg writes failed. Last write %02X to %02X\n",
>> +				cfg[i].val, cfg[i].reg);
>> +			goto w_regs_fail;
>> +		}
>> +
>> +		if (cfg[i].sleep_in_ms)
>> +			msleep(cfg[i].sleep_in_ms);
>> +	}
>> +
>> +w_regs_fail:
>> +	if (ret != 0)
>> +		pr_err("exiting with ret = %d after %d writes\n", ret, i);
>> +
>> +	return ret;
>> +}
>> +
>> +static void sn65dsi86_gpio_configure(struct sn65dsi86 *pdata, bool 
>> on)
>> +{
>> +	int value;
>> +
>> +	value = on ? 1: 0;
>> +
>> +	gpiod_set_value(pdata->gpios.enable_gpio, value);
>> +	gpiod_set_value(pdata->gpios.aux_i2c_sda, value);
>> +	gpiod_set_value(pdata->gpios.aux_i2c_scl, value);
>> +	gpiod_set_value(pdata->gpios.edp_bias_en, value);
>> +	gpiod_set_value(pdata->gpios.edp_bklt_en, value);
>> +	gpiod_set_value(pdata->gpios.irq_gpio, value);
>> +	gpiod_set_value(pdata->gpios.edp_bklt_ctrl, value);
>> +
>> +	pr_debug("config to: %d\n", value);
>> +}
>> +
>> +static void sn65dsi86_power_ctrl(struct sn65dsi86 *pdata, bool 
>> enable)
>> +{
>> +	if (!pdata)
>> +		return;
>> +
>> +	if (!pdata->power_on && enable) {
>> +		sn65dsi86_gpio_configure(pdata, true);
>> +
>> +		if (regulator_bulk_enable(pdata->num_supplies,
>> +						pdata->supplies)) {
>> +			pr_err("bridge regulator enable failed\n");
>> +			return;
>> +		}
>> +		pdata->power_on = true;
>> +	} else if (pdata->power_on && !enable) {
>> +		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
>> +
>> +		sn65dsi86_gpio_configure(pdata, false);
>> +		pdata->power_on = false;
>> +	} else {
>> +		pr_debug("unnecessary call to power control\n");
>> +	}
>> +}
>> +
>> +/* Connector funcs */
>> +static struct sn65dsi86 *connector_to_sn65dsi86(struct drm_connector 
>> *connector)
>> +{
>> +	return container_of(connector, struct sn65dsi86, connector);
>> +}
>> +
>> +static int sn65dsi86_send_aux_cmd(struct sn65dsi86 *pdata,
>> +				  u8 cmd, u8 addr, u8 length, int w_data)
>> +{
>> +	u8 read = 0;
>> +	int retry_cnt = 10;
>> +
>> +	sn65dsi86_write(pdata, SN_AUX_CMD_REG, (cmd << 4));
>> +	sn65dsi86_write(pdata, SN_AUX_ADDR_7_0_REG, addr);
>> +	sn65dsi86_write(pdata, SN_AUX_LENGTH_REG, length);
>> +	if (w_data >= 0)
>> +		sn65dsi86_write(pdata, SN_AUX_WDATA0_REG, (u8)w_data);
>> +
>> +	/* set SEND bit */
>> +	sn65dsi86_read(pdata, SN_AUX_CMD_REG, &read, 1);
>> +	read |= BIT(0);
>> +	sn65dsi86_write(pdata, SN_AUX_CMD_REG, read);
>> +
>> +	/* poll for bridge to ack SEND bit */
>> +	while (retry_cnt) {
>> +		sn65dsi86_read(pdata, SN_AUX_CMD_REG, &read, 0x1);
>> +		if (!(read & BIT(0)))
>> +			break;
>> +		retry_cnt--;
>> +		udelay(1000);
>> +	}
>> +
>> +	if (!retry_cnt) {
>> +		pr_err("aux_cmd transfer failed\n");
>> +		return -EINVAL;
>> +	}
>> +	return 0;
>> +}
>> +
>> +static int sn65dsi86_read_edid(struct sn65dsi86 *pdata, u8 *buf)
>> +{
>> +	int i = 0;
>> +	u8 addr = SN_AUX_RDATA0_REG;
>> +	u8 *data = buf;
>> +
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	if (sn65dsi86_send_aux_cmd(pdata, 0x4, 0x50, 0x0, -1) ||
>> +		sn65dsi86_send_aux_cmd(pdata, 0x4, 0x50, 0x01, 0x0) ||
>> +		sn65dsi86_send_aux_cmd(pdata, 0x5, 0x50, 0x0, -1) ||
>> +		sn65dsi86_send_aux_cmd(pdata, 0x5, 0x50, 0x10, -1))
>> +		goto error;
> 
> Instead of hand-crafting this, you should implement an i2c_adapter. You 
> can then
> use that in drm_get_edid() to read the edid via the core.
> 
> See the comment about drm_do_get_edid():
>  * drivers must make all reasonable efforts to expose it as an I2C
>  * adapter and use drm_get_edid() instead of abusing this function.
> 
>> +
>> +	for (i = 0; i < 16; i++) {
>> +		if (sn65dsi86_read(pdata, addr, data, 0x1))
>> +			goto error;
>> +		addr++;
>> +		data++;
>> +	}
>> +
>> +	return 0;
>> +error:
>> +	pr_err("edid read over i2c failed\n");
>> +	return -EINVAL;
>> +}
>> +
>> +static int sn65dsi86_read_edid_block(struct sn65dsi86 *pdata,
>> +			       u8 *buf, unsigned int block)
>> +{
>> +	if (block == 0) {
>> +		if (sn65dsi86_read_edid(pdata, buf))
>> +			goto error;
>> +	} else if (block == 1) {
>> +		/* move segment pointer */
>> +		if (sn65dsi86_send_aux_cmd(pdata, 0x4, 0x30, 0x0, -1) ||
>> +			sn65dsi86_send_aux_cmd(pdata, 0x4, 0x30, 0x01, 0x1) ||
>> +			sn65dsi86_send_aux_cmd(pdata, 0x0, 0x30, 0x00, -1))
>> +			goto error;
>> +		else
>> +			if (sn65dsi86_read_edid(pdata, buf))
>> +				goto error;
>> +	} else {
>> +		pr_debug("unsupported edid block\n");
>> +		goto error;
>> +	}
>> +
>> +	return 0;
>> +error:
>> +	pr_err("edid block read failed\n");
>> +	return -EINVAL;
>> +}
>> +
>> +static int sn65dsi86_get_edid_block(void *data, u8 *buf, unsigned int 
>> block,
>> +				  size_t len)
>> +{
>> +	struct sn65dsi86 *pdata = data;
>> +	int ret = 0;
>> +
>> +	pr_debug("get edid block: block=%d, len=%d\n", block, (int)len);
>> +
>> +	if (len > 128 || block > 1)
>> +		return -EINVAL;
>> +
>> +	ret = sn65dsi86_read_edid_block(pdata, buf, block);
>> +	if (ret) {
>> +		pr_err("edid read failed for block: %d ret: %d\n", block, ret);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int sn65dsi86_connector_get_modes(struct drm_connector 
>> *connector)
>> +{
>> +	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
>> +	struct drm_display_mode *mode, *m;
>> +
>> +	if (pdata->edid)
>> +		return drm_add_edid_modes(connector, pdata->edid);
> 
> What if the edid changes? You'll never update it.
> 
>> +
>> +	sn65dsi86_power_ctrl(pdata, true);
>> +	if (pdata->is_pluggable) {
> 
> Here, call the panel func if a panel is connected, or probe the edid.
> 
>> +		pdata->edid = drm_do_get_edid(connector,
>> +				sn65dsi86_get_edid_block, pdata);
> 
> Are you leaking memory here?
> 
>> +
>> +		drm_mode_connector_update_edid_property(connector, pdata->edid);
>> +		pdata->num_of_modes = drm_add_edid_modes(connector,
>> +								pdata->edid);
> 
> You should be able to remove num_of_modes and mode_list once you remove 
> the dt
> custom modes. Make sure you incorporate a call to drm_panel_get_modes() 
> here.
> 
>> +	}
>> +
>> +	if (!pdata->is_pluggable || !pdata->num_of_modes) {
>> +		/*
>> +		 * if device does not support HPD or due to some reason
>> +		 * EDID read failed then fall back to mode_list which is
>> +		 * already parsed from dt if any.
>> +		 */
>> +		list_for_each_entry(mode, &pdata->mode_list, head) {
>> +			m = drm_mode_duplicate(connector->dev, mode);
>> +			if (!m) {
>> +				pr_err("failed to get mode %dx%d\n",
>> +					mode->hdisplay, mode->vdisplay);
>> +				break;
>> +			}
>> +			drm_mode_probed_add(connector, m);
>> +		}
>> +	}
>> +
>> +	sn65dsi86_power_ctrl(pdata, false);
> 
> What if power was already on?
> 
>> +	return pdata->num_of_modes;
>> +}
>> +
>> +static enum drm_mode_status
>> +sn65dsi86_connector_mode_valid(struct drm_connector *connector,
>> +			     struct drm_display_mode *mode)
>> +{
>> +	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
>> +	struct drm_display_mode *m;
>> +
>> +	if (pdata->edid)
>> +		return MODE_OK;
> 
> Careful here, userspace could add an invalid mode and this would say 
> it's
> correct. Instead of doing this, and the check below, just check the 
> given mode
> against the max hardware limitations listed in the datasheet (4k60). I 
> assume
> this will also depend on how many dsi channels we have coming in.
> 
>> +
>> +	if (!pdata->is_pluggable) {
>> +		list_for_each_entry(m, &pdata->mode_list, head) {
>> +			if (m->hdisplay == mode->hdisplay &&
>> +				m->vdisplay == mode->vdisplay)
>> +				return MODE_OK;
>> +		}
>> +	}
>> +
>> +	return MODE_BAD;
>> +}
>> +
>> +static struct drm_connector_helper_funcs 
>> sn65dsi86_connector_helper_funcs = {
>> +	.get_modes = sn65dsi86_connector_get_modes,
>> +	.mode_valid = sn65dsi86_connector_mode_valid,
>> +};
>> +
>> +static enum drm_connector_status
>> +sn65dsi86_connector_detect(struct drm_connector *connector, bool 
>> force)
>> +{
>> +	struct sn65dsi86 *pdata = connector_to_sn65dsi86(connector);
>> +
>> +	pdata->connector_status = pdata->power_on ?
>> +		connector_status_connected : connector_status_disconnected;
> 
> It's possible detect() will be called while the device is off, in that 
> case
> this will return the incorrect value. Is there some reason you can't 
> just detect
> whether there's something connected?
> 
>> +
>> +	return pdata->connector_status;
>> +}
>> +
>> +static const struct drm_connector_funcs sn65dsi86_connector_funcs = {
>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.detect = sn65dsi86_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 sn65dsi86 *bridge_to_sn65dsi86(struct drm_bridge 
>> *bridge)
>> +{
>> +	return container_of(bridge, struct sn65dsi86, bridge);
>> +}
>> +
>> +static int sn65dsi86_read_device_rev(struct sn65dsi86 *pdata)
>> +{
>> +	u8 rev = 0;
>> +	int ret = 0;
>> +
>> +	ret = sn65dsi86_read(pdata, SN_DEVICE_REV_REG, &rev, 1);
>> +
> 
> extra line
> 
>> +	if (!ret) {
> 
> In this case, flip the condition so you can avoid indenting everything:
> 
>         if (ret)
>                 return ret;
> 
>> +		if (rev == 0x2) {
> 
> What's 0x2? Can you please #define this?
> 
>> +			pr_info("SN65DSI86 revision id: 0x%x\n", rev);
> 
> While I personally appreciate info messages, others not so much. So 
> downgrade
> this to DRM_DEBUG_KMS()
> 
>> +		} else {
>> +			pr_warn("SN65DSI86 revision id mismatch\n");
>> +			ret = -EINVAL;
>> +		}
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static irqreturn_t sn65dsi86_irq_thread_handler(int irq, void 
>> *dev_id)
>> +{
>> +	return IRQ_HANDLED;
>> +}
> 
> What's the point of even registering this? Just leave it all out if 
> it's unused.
> If there's a need in the future, it can easily be added back in.
> 
>> +
>> +static const char * const sn65dsi86_supply_names[] = {
>> +	"vccio",
>> +	"vcca",
>> +	"vccs"
>> +};
>> +
>> +static int sn65dsi86_init_regulators(struct sn65dsi86 *pdata)
>> +{
>> +	const char * const *supply_names;
>> +	unsigned int i;
>> +	int ret;
>> +
>> +	supply_names = sn65dsi86_supply_names;
> 
> This local isn't necessary.
> 
>> +	pdata->num_supplies = ARRAY_SIZE(sn65dsi86_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 = supply_names[i];
>> +
>> +	ret = devm_regulator_bulk_get(pdata->dev,
>> +			pdata->num_supplies, pdata->supplies);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return regulator_bulk_enable(pdata->num_supplies, pdata->supplies);
>> +}
>> +
>> +static int sn65dsi86_bridge_attach(struct drm_bridge *bridge)
>> +{
>> +	struct mipi_dsi_host *host;
>> +	struct mipi_dsi_device *dsi;
>> +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
>> +	int ret;
>> +	const struct mipi_dsi_device_info info = { .type = "sn65dsi86",
>> +						   .channel = 0,
>> +						   .node = NULL,
>> +						 };
>> +
>> +	if (!bridge->encoder) {
>> +		DRM_ERROR("Parent encoder object not found");
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* HPD not supported */
>> +	pdata->connector.polled = 0;
>> +
> 
> You'll need to refactor the below to accommodate panels. If you're not 
> planning
> on supporting hotplug, you should probably remove all of the 
> connector-related
> stuff from this driver, since you will always be using a panel driver.
> 

Thanks for reviewing the patch in detail.

I have one doubt here. If we remove connector from bridge driver, then 
how will detect()
and get_modes() called. If you are suggesting to use panel func's 
detect() and get_mode()
then it might not work, because once upstream DSI driver sees an 
external bridge is connected
to DSI, then it does not create a connector of it own, it expects the 
external bridge
to create the connector node. I think here the external bridge has to 
create the connector
and when detect() and get_modes() call come to external bridge then it 
should query connected
panel's detect() and get_modes() API.

>> +	ret = drm_connector_init(bridge->dev, &pdata->connector,
>> +				 &sn65dsi86_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,
>> +				 &sn65dsi86_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) {
>> +		pr_err("failed to find dsi host\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	dsi = mipi_dsi_device_register_full(host, &info);
>> +	if (IS_ERR(dsi)) {
>> +		pr_err("failed to create dsi device\n");
>> +		ret = PTR_ERR(dsi);
>> +		goto err_dsi_device;
>> +	}
>> +
>> +	/* setting to 4 lanes always for now */
>> +	dsi->lanes = 4;
> 
> Perhaps add a TODO for this?
> 
>> +	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) {
>> +		pr_err("failed to attach dsi to host\n");
>> +		goto err_dsi_attach;
>> +	}
>> +
>> +	pdata->dsi = dsi;
>> +
>> +	pr_debug("bridge attached\n");
>> +
>> +	return 0;
>> +
>> +err_dsi_attach:
>> +	mipi_dsi_device_unregister(dsi);
>> +err_dsi_device:
>> +	return ret;
>> +}
>> +
>> +static void sn65dsi86_set_video_cfg(struct sn65dsi86 *pdata,
>> +	struct drm_display_mode *mode,
>> +	struct sn65dsi86_video_cfg *video_cfg)
>> +{
>> +	video_cfg->h_active = mode->hdisplay;
>> +	video_cfg->v_active = mode->vdisplay;
>> +	video_cfg->h_front_porch = mode->hsync_start - mode->hdisplay;
>> +	video_cfg->v_front_porch = mode->vsync_start - mode->vdisplay;
>> +	video_cfg->h_back_porch = mode->htotal - mode->hsync_end;
>> +	video_cfg->v_back_porch = mode->vtotal - mode->vsync_end;
>> +	video_cfg->h_pulse_width = mode->hsync_end - mode->hsync_start;
>> +	video_cfg->v_pulse_width = mode->vsync_end - mode->vsync_start;
>> +	video_cfg->pclk_khz = mode->clock;
>> +
>> +	video_cfg->h_polarity = !!(mode->flags & DRM_MODE_FLAG_PHSYNC);
>> +	video_cfg->v_polarity = !!(mode->flags & DRM_MODE_FLAG_PVSYNC);
>> +
>> +	/* setting to 4 lanes always for now */
>> +	video_cfg->num_of_lanes = 4;
>> +
>> +	pr_debug("video=h[%d,%d,%d,%d] v[%d,%d,%d,%d] pclk=%d lane=%d\n",
>> +		video_cfg->h_active, video_cfg->h_front_porch,
>> +		video_cfg->h_pulse_width, video_cfg->h_back_porch,
>> +		video_cfg->v_active, video_cfg->v_front_porch,
>> +		video_cfg->v_pulse_width, video_cfg->v_back_porch,
>> +		video_cfg->pclk_khz, video_cfg->num_of_lanes);
>> +}
> 
> As mentioned above, this needs to be removed.
> 
>> +
>> +static void sn65dsi86_bridge_mode_set(struct drm_bridge *bridge,
>> +				    struct drm_display_mode *mode,
>> +				    struct drm_display_mode *adj_mode)
>> +{
>> +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
>> +	struct sn65dsi86_video_cfg *video_cfg = &pdata->video_cfg;
>> +	int ret = 0;
>> +
>> +	pr_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);
>> +
>> +	memset(video_cfg, 0, sizeof(struct sn65dsi86_video_cfg));
>> +	sn65dsi86_set_video_cfg(pdata, adj_mode, video_cfg);
>> +
>> +	if (video_cfg->num_of_lanes != pdata->dsi->lanes) {
>> +		mipi_dsi_detach(pdata->dsi);
>> +		pdata->dsi->lanes = video_cfg->num_of_lanes;
>> +		ret = mipi_dsi_attach(pdata->dsi);
> 
> Hmm, if this is configurable is there no better way of doing this than 
> detaching
> and attaching?
> 
>> +		if (ret)
>> +			pr_err("failed to change host lanes\n");
>> +	}
>> +}
>> +
>> +static void sn65dsi86_bridge_disable(struct drm_bridge *bridge)
>> +{
>> +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
>> +
>> +	sn65dsi86_power_ctrl(pdata, false);
>> +	pdata->connector_status =  connector_status_disconnected;
>> +}
>> +
>> +static void sn65dsi86_bridge_enable(struct drm_bridge *bridge)
>> +{
>> +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
>> +
>> +	sn65dsi86_power_ctrl(pdata, true);
>> +	pdata->connector_status =  connector_status_connected;
>> +	sn65dsi86_write_array(pdata, cfg_proto_0_init,
>> +			      sizeof(cfg_proto_0_init));
> 
> As mentioned above, most these should likely be set in mode_set() using 
> the
> values set in the current mode.
> 
> Sean
> 
>> +}
>> +
>> +static const struct drm_bridge_funcs sn65dsi86_bridge_funcs = {
>> +	.attach = sn65dsi86_bridge_attach,
>> +	.enable = sn65dsi86_bridge_enable,
>> +	.disable = sn65dsi86_bridge_disable,
>> +	.mode_set = sn65dsi86_bridge_mode_set,
>> +};
>> +
>> +static int sn65dsi86_parse_dt_modes(struct device_node *np,
>> +					struct list_head *head,
>> +					u32 *num_of_modes)
>> +{
>> +	int rc = 0;
>> +	struct drm_display_mode *mode;
>> +	u32 mode_count = 0;
>> +	struct device_node *node = NULL;
>> +	struct device_node *root_node = NULL;
>> +	u32 h_front_porch, h_pulse_width, h_back_porch;
>> +	u32 v_front_porch, v_pulse_width, v_back_porch;
>> +	bool h_active_high, v_active_high;
>> +	u32 flags = 0;
>> +
>> +	root_node = of_get_child_by_name(np, "sn,custom-modes");
>> +	if (!root_node) {
>> +		root_node = of_parse_phandle(np, "sn,custom-modes", 0);
>> +		if (!root_node) {
>> +			pr_info("No modes present for sn,custom-modes");
>> +			goto end;
>> +		}
>> +	}
>> +
>> +	for_each_child_of_node(root_node, node) {
>> +		rc = 0;
>> +		mode = kzalloc(sizeof(*mode), GFP_KERNEL);
>> +		if (!mode) {
>> +			rc =  -ENOMEM;
>> +			goto end;
>> +		}
>> +
>> +		of_property_read_u32(node, "sn,mode-h-active",
>> +						&mode->hdisplay);
>> +
>> +		of_property_read_u32(node, "sn,mode-h-front-porch",
>> +						&h_front_porch);
>> +		of_property_read_u32(node, "sn,mode-h-pulse-width",
>> +						&h_pulse_width);
>> +
>> +		of_property_read_u32(node, "sn,mode-h-back-porch",
>> +						&h_back_porch);
>> +
>> +		h_active_high = of_property_read_bool(node,
>> +						"sn,mode-h-active-high");
>> +
>> +		of_property_read_u32(node, "sn,mode-v-active",
>> +						&mode->vdisplay);
>> +		of_property_read_u32(node, "sn,mode-v-front-porch",
>> +						&v_front_porch);
>> +
>> +		of_property_read_u32(node, "sn,mode-v-pulse-width",
>> +						&v_pulse_width);
>> +		of_property_read_u32(node, "sn,mode-v-back-porch",
>> +						&v_back_porch);
>> +		v_active_high = of_property_read_bool(node,
>> +						"sn,mode-v-active-high");
>> +
>> +		of_property_read_u32(node, "sn,mode-refresh-rate",
>> +						&mode->vrefresh);
>> +
>> +		of_property_read_u32(node, "sn,mode-clock-in-khz",
>> +						&mode->clock);
>> +
>> +		mode->hsync_start = mode->hdisplay + h_front_porch;
>> +		mode->hsync_end = mode->hsync_start + h_pulse_width;
>> +		mode->htotal = mode->hsync_end + h_back_porch;
>> +		mode->vsync_start = mode->vdisplay + v_front_porch;
>> +		mode->vsync_end = mode->vsync_start + v_pulse_width;
>> +		mode->vtotal = mode->vsync_end + v_back_porch;
>> +
>> +		if (!mode->htotal || !mode->vtotal) {
>> +			rc = -EINVAL;
>> +			goto fail;
>> +		}
>> +
>> +		if (h_active_high)
>> +			flags |= DRM_MODE_FLAG_PHSYNC;
>> +		else
>> +			flags |= DRM_MODE_FLAG_NHSYNC;
>> +		if (v_active_high)
>> +			flags |= DRM_MODE_FLAG_PVSYNC;
>> +		else
>> +			flags |= DRM_MODE_FLAG_NVSYNC;
>> +		mode->flags = flags;
>> +
>> +		if (!rc) {
>> +			mode_count++;
>> +			list_add_tail(&mode->head, head);
>> +		}
>> +
>> +		drm_mode_set_name(mode);
>> +
>> +		pr_debug("mode[%s] h[%d,%d,%d,%d] v[%d,%d,%d,%d] %d %x %dkHZ\n",
>> +			mode->name, mode->hdisplay, mode->hsync_start,
>> +			mode->hsync_end, mode->htotal, mode->vdisplay,
>> +			mode->vsync_start, mode->vsync_end, mode->vtotal,
>> +			mode->vrefresh, mode->flags, mode->clock);
>> +fail:
>> +		if (rc) {
>> +			kfree(mode);
>> +			continue;
>> +		}
>> +	}
>> +
>> +	if (num_of_modes)
>> +		*num_of_modes = mode_count;
>> +
>> +end:
>> +	return rc;
>> +}
>> +
>> +static int sn65dsi86_parse_gpios(struct device_node *np,
>> +					struct sn65dsi86 *pdata)
>> +{
>> +	int ret = 0;
>> +
>> +	if (!pdata || !pdata->dev)
>> +		return -EINVAL;
>> +
>> +	pdata->gpios.enable_gpio = devm_gpiod_get(pdata->dev, "enable",
>> +						  GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdata->gpios.enable_gpio)) {
>> +		pr_err("failed to get enable gpio from DT\n");
>> +		ret = PTR_ERR(pdata->gpios.enable_gpio);
>> +		goto exit;
>> +	}
>> +
>> +	pdata->gpios.aux_i2c_scl = devm_gpiod_get(pdata->dev, "aux-scl",
>> +						  GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdata->gpios.aux_i2c_scl)) {
>> +		pr_err("failed to get aux scl gpio from DT\n");
>> +		ret = PTR_ERR(pdata->gpios.aux_i2c_scl);
>> +		goto exit;
>> +	}
>> +
>> +	pdata->gpios.aux_i2c_sda = devm_gpiod_get(pdata->dev, "aux-sda",
>> +						  GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdata->gpios.aux_i2c_sda)) {
>> +		pr_err("failed to get aux sda gpio from DT\n");
>> +		ret = PTR_ERR(pdata->gpios.aux_i2c_sda);
>> +		goto exit;
>> +	}
>> +
>> +	pdata->gpios.edp_bias_en = devm_gpiod_get(pdata->dev, "bias-en",
>> +						  GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdata->gpios.edp_bias_en)) {
>> +		pr_err("failed to get bias en gpio from DT\n");
>> +		ret = PTR_ERR(pdata->gpios.edp_bias_en);
>> +		goto exit;
>> +	}
>> +
>> +	pdata->gpios.edp_bklt_en = devm_gpiod_get(pdata->dev, "bklt-en",
>> +						  GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdata->gpios.edp_bklt_en)) {
>> +		pr_err("failed to get bklt en gpio from DT\n");
>> +		ret = PTR_ERR(pdata->gpios.edp_bklt_en);
>> +		goto exit;
>> +	}
>> +
>> +	pdata->gpios.irq_gpio = devm_gpiod_get_optional(pdata->dev, "irq",
>> +						  GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdata->gpios.irq_gpio))
>> +		pr_err("failed to get irq gpio from DT\n");
>> +
>> +	pdata->gpios.edp_bklt_ctrl = devm_gpiod_get_optional(pdata->dev,
>> +					"bklt-ctrl", GPIOD_OUT_HIGH);
>> +	if (IS_ERR(pdata->gpios.edp_bklt_ctrl))
>> +		pr_err("failed to get bklt ctrl gpio from DT\n");
>> +
>> +exit:
>> +	return ret;
>> +}
>> +
>> +static int sn65dsi86_parse_dt(struct device *dev, struct sn65dsi86 
>> *pdata)
>> +{
>> +	struct device_node *np = dev->of_node;
>> +	struct device_node *end_node;
>> +	int ret = 0;
>> +
>> +	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
>> +	if (!end_node) {
>> +		pr_err("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) {
>> +		pr_err("remote node not found\n");
>> +		return -ENODEV;
>> +	}
>> +	of_node_put(pdata->host_node);
>> +
>> +	ret = sn65dsi86_parse_gpios(np, pdata);
>> +
>> +	pdata->is_pluggable = of_property_read_bool(np, "sn,is-pluggable");
>> +	pr_debug("is_pluggable = %d\n", pdata->is_pluggable);
>> +	if (!pdata->is_pluggable) {
>> +		INIT_LIST_HEAD(&pdata->mode_list);
>> +		sn65dsi86_parse_dt_modes(np,
>> +			&pdata->mode_list, &pdata->num_of_modes);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static int sn65dsi86_probe(struct i2c_client *client,
>> +	 const struct i2c_device_id *id)
>> +{
>> +	struct sn65dsi86 *pdata;
>> +	int ret = 0;
>> +	struct drm_display_mode *mode, *n;
>> +
>> +	if (!client || !client->dev.of_node) {
>> +		pr_err("invalid input\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> +		pr_err("device doesn't support I2C\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	pdata = devm_kzalloc(&client->dev,
>> +		sizeof(struct sn65dsi86), GFP_KERNEL);
>> +	if (!pdata)
>> +		return -ENOMEM;
>> +
>> +	pdata->power_on = false;
>> +	pdata->is_pluggable = false;
>> +	pdata->connector_status = connector_status_disconnected;
>> +	pdata->dev = &client->dev;
>> +	pdata->i2c_client = client;
>> +	pr_debug("I2C address is %x\n", client->addr);
>> +
>> +	ret = sn65dsi86_parse_dt(&client->dev, pdata);
>> +	if (ret) {
>> +		pr_err("failed to parse device tree\n");
>> +		goto err_dt_parse;
>> +	}
>> +
>> +	sn65dsi86_gpio_configure(pdata, true);
>> +
>> +	ret = sn65dsi86_init_regulators(pdata);
>> +	if (ret) {
>> +		pr_err("failed to enable regulators\n");
>> +		goto err_gpio_config;
>> +	}
>> +
>> +	ret = sn65dsi86_read_device_rev(pdata);
>> +	if (ret) {
>> +		pr_err("failed to read chip rev\n");
>> +		goto err_gpio_config;
>> +	} else {
>> +		pr_debug("bridge chip enabled successfully\n");
>> +		pdata->power_on = true;
>> +	}
>> +
>> +	pdata->irq = gpiod_to_irq(pdata->gpios.irq_gpio);
>> +	ret = request_threaded_irq(pdata->irq, NULL,
>> +			sn65dsi86_irq_thread_handler,
>> +			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +			"sn65dsi86", pdata);
>> +
>> +	i2c_set_clientdata(client, pdata);
>> +	dev_set_drvdata(&client->dev, pdata);
>> +
>> +	pdata->bridge.funcs = &sn65dsi86_bridge_funcs;
>> +	pdata->bridge.of_node = client->dev.of_node;
>> +
>> +	drm_bridge_add(&pdata->bridge);
>> +
>> +	return ret;
>> +
>> +err_gpio_config:
>> +	sn65dsi86_gpio_configure(pdata, false);
>> +err_dt_parse:
>> +	if (!pdata->is_pluggable) {
>> +		list_for_each_entry_safe(mode, n, &pdata->mode_list, head) {
>> +			list_del(&mode->head);
>> +			kfree(mode);
>> +		}
>> +		pdata->num_of_modes = 0;
>> +	}
>> +	devm_kfree(&client->dev, pdata);
>> +	return ret;
>> +}
>> +
>> +static int sn65dsi86_remove(struct i2c_client *client)
>> +{
>> +	int ret = -EINVAL;
>> +	struct sn65dsi86 *pdata = i2c_get_clientdata(client);
>> +	struct drm_display_mode *mode, *n;
>> +
>> +	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);
>> +
>> +	sn65dsi86_gpio_configure(pdata, false);
>> +
>> +	if (!pdata->is_pluggable) {
>> +		list_for_each_entry_safe(mode, n, &pdata->mode_list, head) {
>> +			list_del(&mode->head);
>> +			kfree(mode);
>> +		}
>> +	}
>> +
>> +	devm_kfree(&client->dev, pdata);
>> +
>> +end:
>> +	return ret;
>> +}
>> +
>> +static struct i2c_device_id sn65dsi86_id[] = {
>> +	{ "ti,sn65dsi86", 0},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, sn65dsi86_id);
>> +
>> +static const struct of_device_id sn65dsi86_match_table[] = {
>> +	{.compatible = "ti,sn65dsi86"},
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, sn65dsi86_match_table);
>> +
>> +static struct i2c_driver sn65dsi86_driver = {
>> +	.driver = {
>> +		.name = "sn65dsi86",
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = sn65dsi86_match_table,
>> +	},
>> +	.probe = sn65dsi86_probe,
>> +	.remove = sn65dsi86_remove,
>> +	.id_table = sn65dsi86_id,
>> +};
>> +
>> +module_i2c_driver(sn65dsi86_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
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Sean Paul]

On Mon, Apr 16, 2018 at 11:32:50AM +0530, spanda@codeaurora.org wrote:
> On 2018-04-14 00:59, Sean Paul wrote:
> > On Fri, Apr 13, 2018 at 10:53:00AM +0530, Sandeep Panda wrote:
> > > 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 <spanda@codeaurora.org>

<snip />

> > > +{
> > > +	struct mipi_dsi_host *host;
> > > +	struct mipi_dsi_device *dsi;
> > > +	struct sn65dsi86 *pdata = bridge_to_sn65dsi86(bridge);
> > > +	int ret;
> > > +	const struct mipi_dsi_device_info info = { .type = "sn65dsi86",
> > > +						   .channel = 0,
> > > +						   .node = NULL,
> > > +						 };
> > > +
> > > +	if (!bridge->encoder) {
> > > +		DRM_ERROR("Parent encoder object not found");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	/* HPD not supported */
> > > +	pdata->connector.polled = 0;
> > > +
> > 
> > You'll need to refactor the below to accommodate panels. If you're not
> > planning
> > on supporting hotplug, you should probably remove all of the
> > connector-related
> > stuff from this driver, since you will always be using a panel driver.
> > 
> 
> Thanks for reviewing the patch in detail.
> 
> I have one doubt here. If we remove connector from bridge driver, then how
> will detect()
> and get_modes() called. If you are suggesting to use panel func's detect()
> and get_mode()
> then it might not work, because once upstream DSI driver sees an external
> bridge is connected
> to DSI, then it does not create a connector of it own, it expects the
> external bridge
> to create the connector node. I think here the external bridge has to create
> the connector
> and when detect() and get_modes() call come to external bridge then it
> should query connected
> panel's detect() and get_modes() API.
> 

Right, thanks for setting me straight. You'll need to call the drm_panel_*
helper functions if the panel is present for the connector hooks.

Sean


<snip />

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Rob Herring]

On Fri, Apr 13, 2018 at 10:53:01AM +0530, Sandeep Panda wrote:
> Document the bindings used for the sn65dsi86 DSI to eDP bridge.
> 
> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
> ---
>  .../bindings/display/bridge/ti,sn65dsi86.txt       | 75 ++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> new file mode 100644
> index 0000000..cbd2f0e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> @@ -0,0 +1,75 @@
> +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 specification for EDP_BRIJ_EN pin
> +- aux-sda-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SDA pin of AUX channel
> +- aux-scl-gpios: OF device-tree gpio specification for EDP_BRIJ_I2C_SCL pin of AUX channel

What if I use I2C host on my device? If your board design uses bitbanged 
I2C, that is outside the scope of this binding and it should use 
i2c-gpio binding.

> +- bias-en-gpios: OF device-tree gpio specification for EN_PP3300_DX_EDP pin to
> +			enable 3.3V supply to eDP connector
> +- bklt-en-gpios: OF device-tree gpio specification for AP_EDP_BKLTEN 
pin
> +
> +- vccio-supply: A 1.8V supply that powers up the PHY.
> +- vcca-supply: A 1.2V supply that powers up the Controller.
> +- vccs-supply: A 3.3V supply that power the eDP connector
> +
> +Optional properties:
> +
> +- irq-gpios: OF device-tree gpio specification for interrupt pin

Use "interrupts" property instead.

> +- bklt-ctrl-gpios: OF device-tree gpio specification for EDP_BKLTCTL pin
> +
> +- sn,is-pluggable: boolean property to specify if HPD supported or not
> +- sn,custom-modes: OF device-tree specifiction to add support for custom modes

"sn" is not a vendor.

> +
> +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>;
> +	aux-sda-gpios = <&msmgpio 34 GPIO_ACTIVE_HIGH>;
> +	aux-scl-gpios = <&msmgpio 35 GPIO_ACTIVE_HIGH>;
> +	bias-en-gpios = <&msmgpio 36 GPIO_ACTIVE_HIGH>;
> +	bklt-en-gpios = <&msmgpio 37 GPIO_ACTIVE_HIGH>;
> +
> +	vccio-supply = <&pm8916_l17>;
> +	vcca-supply = <&pm8916_l6>;
> +	vccs-supply = <&pm8916_l7>;
> +
> +	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>;
> +			};
> +		};
> +	};
> +}
> -- 
> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno