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

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

[Sandeep Panda]

Changes in current patchset:
 - Separate panel driver change from dt-binding documentation.
 - Add the properties from simple-panel binding that are applicable
   to this edp panel dt-binding.
 - Fix the warnings in bridge driver, reported by Kbuild test robot.

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

 .../bindings/display/bridge/ti,sn65dsi86.txt       |  76 +++
 .../bindings/display/panel/innolux,tv123wam.txt    |  20 +
 drivers/gpu/drm/bridge/Kconfig                     |   9 +
 drivers/gpu/drm/bridge/Makefile                    |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c              | 725 +++++++++++++++++++++
 drivers/gpu/drm/panel/panel-simple.c               |  27 +
 6 files changed, 858 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
 create mode 100644 Documentation/devicetree/bindings/display/panel/innolux,tv123wam.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

[PATCH v5 1/4] 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.

Changes in v1:
 - Split the dt-bindings and the driver support into separate patches
   (Andrzej Hajda).
 - Use of gpiod APIs to parse and configure gpios instead of obsolete ones
   (Andrzej Hajda).
 - Use macros to define the register offsets (Andrzej Hajda).

Changes in v2:
 - Separate out edp panel specific HW resource handling from bridge
   driver and create a separate edp panel drivers to handle panel
   specific mode information and HW resources (Sean Paul).
 - Replace pr_* APIs to DRM_* APIs to log error or debug information
   (Sean Paul).
 - Remove some of the unnecessary structure/variable from driver (Sean
   Paul).
 - Rename the function and structure prefix "sn65dsi86" to "ti_sn_bridge"
   (Sean Paul / Rob Herring).
 - Remove most of the hard-coding and modified the bridge init sequence
   based on current mode (Sean Paul).
 - Remove the existing function to retrieve the EDID data and
   implemented this as an i2c_adapter and use drm_get_edid() (Sean Paul).
 - Remove the dummy irq handler implementation, will add back the
   proper irq handling later (Sean Paul).
 - Capture the required enable gpios in a single array based on dt entry
   instead of having individual descriptor for each gpio (Sean Paul).

Changes in v3:
 - Remove usage of irq_gpio and replace it as "interrupts" property (Rob
   Herring).
 - Remove the unnecessary header file inclusions (Sean Paul).
 - Rearrange the header files in alphabetical order (Sean Paul).
 - Use regmap interface to perform i2c transactions.
 - Update Copyright/License field and address other review comments
   (Jordan Crouse).

Changes in v4:
 - Update License/Copyright (Sean Paul).
 - Add Kconfig and Makefile changes (Sean Paul).
 - Drop i2c gpio handling from this bridge driver, since i2c sda/scl gpios
   will be handled by i2c master.
 - Remove unnecessary goto statements (Sean Paul).
 - Add mutex lock to power_ctrl API to avoid race conditions (Sean
   Paul).
 - Add support to parse reference clk frequency from dt(optional).
 - Update the bridge chip enable/disable sequence.

Changes in v5:
 - Fixed Kbuild test service reported warnings.

Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
---
 drivers/gpu/drm/bridge/Kconfig        |   9 +
 drivers/gpu/drm/bridge/Makefile       |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c | 725 ++++++++++++++++++++++++++++++++++
 3 files changed, 735 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 3b99d5a..8153150 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -108,6 +108,15 @@ config DRM_TI_TFP410
 	---help---
 	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
 
+config DRM_TI_SN65DSI86
+	tristate "TI SN65DSI86 DSI to eDP bridge"
+	depends on OF
+	select DRM_KMS_HELPER
+	select REGMAP_I2C
+	select DRM_PANEL
+	---help---
+	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
+
 source "drivers/gpu/drm/bridge/analogix/Kconfig"
 
 source "drivers/gpu/drm/bridge/adv7511/Kconfig"
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index 373eb28..3711be8 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -12,4 +12,5 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
 obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
 obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
 obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
+obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
 obj-y += synopsys/
diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
new file mode 100644
index 0000000..019c7cd
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
@@ -0,0 +1,725 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
+ */
+
+#include <drm/drmP.h>
+#include <drm/drm_atomic.h>
+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_panel.h>
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/of_gpio.h>
+#include <linux/of_graph.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+
+#define SN_BRIDGE_REVISION_ID 0x2
+
+/* Link Training specific registers */
+#define SN_DEVICE_REV_REG			0x08
+#define SN_HPD_DISABLE_REG			0x5C
+#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
+/* 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 MIN_DSI_CLK_FREQ_MHZ	40
+#define SN_DEFAULT_REF_CLK_KHZ	19200
+
+/* fudge factor required to account for 8b/10b encoding */
+#define DP_CLK_FUDGE_NUM	10
+#define DP_CLK_FUDGE_DEN	8
+
+#define DPPLL_CLK_SRC_REFCLK	0
+#define DPPLL_CLK_SRC_DSICLK	1
+
+#define SN_DSIA_REFCLK_OFFSET	1
+#define SN_DSIA_LANE_OFFSET	3
+#define SN_DP_LANE_OFFSET	4
+#define SN_DP_DATA_RATE_OFFSET	5
+#define SN_TIMING_HIGH_OFFSET	8
+
+#define SN_ENABLE_VID_STREAM_BIT	BIT(3)
+#define SN_DSIA_NUM_LANES_BITS		(BIT(4) | BIT(3))
+#define SN_DP_NUM_LANES_BITS		(BIT(5) | BIT(4))
+#define SN_DP_DATA_RATE_BITS		(BIT(7) | BIT(6) | BIT(5))
+#define SN_HPD_DISABLE_BIT		BIT(0)
+
+struct ti_sn_bridge {
+	struct device *dev;
+	struct regmap *regmap;
+	struct drm_bridge bridge;
+	struct drm_connector connector;
+	struct device_node *host_node;
+	struct mipi_dsi_device *dsi;
+	unsigned int refclk_khz;
+	struct drm_panel *panel;
+	struct gpio_desc *enable_gpio;
+	unsigned int num_supplies;
+	struct regulator_bulk_data *supplies;
+	struct i2c_adapter *ddc;
+	unsigned int num_modes;
+	struct drm_display_mode curr_mode;
+	struct mutex lock;
+	unsigned int ctrl_ref_count;
+};
+
+static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
+	{ .range_min = 0, .range_max = 0xff },
+};
+
+static const struct regmap_access_table ti_sn_bridge_volatile_table = {
+	.yes_ranges = ti_sn_bridge_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges),
+};
+
+static const struct regmap_config ti_sn_bridge_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 8,
+	.volatile_table = &ti_sn_bridge_volatile_table,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool enable)
+{
+	int ret = 0;
+
+	mutex_lock(&pdata->lock);
+	if (enable)
+		pdata->ctrl_ref_count++;
+	else
+		pdata->ctrl_ref_count--;
+
+	if (enable && (pdata->ctrl_ref_count == 1)) {
+		ret = regulator_bulk_enable(pdata->num_supplies,
+						 pdata->supplies);
+		if (ret) {
+			DRM_ERROR("bridge regulator enable failed\n");
+			goto exit;
+		}
+
+		gpiod_set_value(pdata->enable_gpio, 1);
+	} else if (!enable && !pdata->ctrl_ref_count) {
+		gpiod_set_value(pdata->enable_gpio, 0);
+
+		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
+	} else {
+		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
+						enable, pdata->ctrl_ref_count);
+	}
+
+exit:
+	mutex_unlock(&pdata->lock);
+	return ret;
+}
+
+/* Connector funcs */
+static struct ti_sn_bridge *
+connector_to_ti_sn_bridge(struct drm_connector *connector)
+{
+	return container_of(connector, struct ti_sn_bridge, connector);
+}
+
+static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+	struct drm_panel *panel = pdata->panel;
+	struct edid *edid;
+
+	if (panel) {
+		DRM_DEBUG("get mode from connected drm_panel\n");
+		pdata->num_modes = drm_panel_get_modes(panel);
+		return pdata->num_modes;
+	}
+
+	/* get from EDID */
+	if (!pdata->ddc)
+		return 0;
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+	edid = drm_get_edid(connector, pdata->ddc);
+	ti_sn_bridge_power_ctrl(pdata, false);
+	if (!edid)
+		return 0;
+
+	drm_mode_connector_update_edid_property(connector, edid);
+	pdata->num_modes = drm_add_edid_modes(connector, edid);
+	kfree(edid);
+
+	return pdata->num_modes;
+}
+
+static enum drm_mode_status
+ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
+			     struct drm_display_mode *mode)
+{
+	/* maximum supported resolution is 4K at 60 fps */
+	if (mode->clock > 594000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
+static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
+	.get_modes = ti_sn_bridge_connector_get_modes,
+	.mode_valid = ti_sn_bridge_connector_mode_valid,
+};
+
+static enum drm_connector_status
+ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
+{
+	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
+
+	/**
+	 * TODO: Currently if drm_panel is present, then always
+	 * return the status as connected. Need to add support to detect
+	 * device state for no panel(hot pluggable) scenarios.
+	 */
+	if (pdata->panel)
+		return connector_status_connected;
+	else
+		return connector_status_unknown;
+}
+
+static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.detect = ti_sn_bridge_connector_detect,
+	.destroy = drm_connector_cleanup,
+	.reset = drm_atomic_helper_connector_reset,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct ti_sn_bridge, bridge);
+}
+
+static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
+{
+	unsigned int rev = 0;
+	int ret = 0;
+
+	ret = regmap_read(pdata->regmap, SN_DEVICE_REV_REG, &rev);
+	if (ret)
+		return ret;
+
+	if (rev != SN_BRIDGE_REVISION_ID) {
+		DRM_ERROR("ti_sn_bridge revision id: 0x%x mismatch\n", rev);
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static const char * const ti_sn_bridge_supply_names[] = {
+	"vcca",
+	"vcc",
+	"vccio",
+	"vpll",
+};
+
+static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
+{
+	unsigned int i;
+
+	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
+
+	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
+				     sizeof(*pdata->supplies), GFP_KERNEL);
+	if (!pdata->supplies)
+		return -ENOMEM;
+
+	for (i = 0; i < pdata->num_supplies; i++)
+		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
+
+	return devm_regulator_bulk_get(pdata->dev,
+				pdata->num_supplies, pdata->supplies);
+}
+
+static int ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
+{
+	struct device_node *panel_node, *port, *endpoint;
+
+	pdata->panel = NULL;
+	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
+	if (port) {
+		endpoint = of_get_child_by_name(port, "endpoint");
+		of_node_put(port);
+		if (!endpoint) {
+			DRM_ERROR("no output endpoint found\n");
+			return -EINVAL;
+		}
+
+		panel_node = of_graph_get_remote_port_parent(endpoint);
+		of_node_put(endpoint);
+		if (!panel_node) {
+			DRM_ERROR("no output node found\n");
+			return -EINVAL;
+		}
+
+		pdata->panel = of_drm_find_panel(panel_node);
+		of_node_put(panel_node);
+		if (!pdata->panel) {
+			DRM_ERROR("no panel node found\n");
+			return -EINVAL;
+		}
+		drm_panel_attach(pdata->panel, &pdata->connector);
+		DRM_DEBUG("panel attached\n");
+	}
+
+	return 0;
+}
+
+static int ti_sn_bridge_attach(struct drm_bridge *bridge)
+{
+	struct mipi_dsi_host *host;
+	struct mipi_dsi_device *dsi;
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	int ret;
+	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
+						   .channel = 0,
+						   .node = NULL,
+						 };
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found\n");
+		return -ENODEV;
+	}
+
+	/* HPD not supported */
+	pdata->connector.polled = 0;
+
+	ret = drm_connector_init(bridge->dev, &pdata->connector,
+				 &ti_sn_bridge_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_connector_helper_add(&pdata->connector,
+				 &ti_sn_bridge_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&pdata->connector, bridge->encoder);
+
+	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
+	if (!host) {
+		DRM_ERROR("failed to find dsi host\n");
+		return -ENODEV;
+	}
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi)) {
+		DRM_ERROR("failed to create dsi device\n");
+		ret = PTR_ERR(dsi);
+		return ret;
+	}
+
+	/* TODO: setting to 4 lanes always for now */
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
+			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		DRM_ERROR("failed to attach dsi to host\n");
+		mipi_dsi_device_unregister(dsi);
+		return ret;
+	}
+
+	pdata->dsi = dsi;
+
+	DRM_DEBUG("bridge attached\n");
+	/* attach panel to bridge */
+	ti_sn_bridge_attach_panel(pdata);
+
+	return 0;
+}
+
+static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
+				    struct drm_display_mode *mode,
+				    struct drm_display_mode *adj_mode)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
+		adj_mode->hdisplay, adj_mode->vdisplay,
+		adj_mode->vrefresh, adj_mode->clock);
+
+	drm_mode_copy(&pdata->curr_mode, adj_mode);
+}
+
+static void ti_sn_bridge_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+
+	if (panel) {
+		drm_panel_disable(panel);
+		drm_panel_unprepare(panel);
+	}
+
+	/* disable video stream */
+	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
+			SN_ENABLE_VID_STREAM_BIT, 0);
+	/* semi auto link training mode OFF */
+	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
+	/* disable DP PLL */
+	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
+}
+
+/* reference clk frequencies supported by the bridge in KHz */
+static u32 ti_sn_bridge_ref_clk_table[] = {
+	12000,
+	19200,
+	26000,
+	27000,
+	38400,
+};
+
+static void ti_sn_bridge_set_refclk(struct ti_sn_bridge *pdata)
+{
+	int i = 0;
+
+	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_ref_clk_table); i++)
+		if (ti_sn_bridge_ref_clk_table[i] == pdata->refclk_khz)
+			break;
+	regmap_write(pdata->regmap, SN_REFCLK_FREQ_REG,
+		(DPPLL_CLK_SRC_REFCLK | (i << SN_DSIA_REFCLK_OFFSET)));
+}
+
+struct dp_data_rate {
+	unsigned int bit_val;
+	unsigned int dp_rate;
+};
+
+/* dp data rate supported by the bridge Mbps */
+static struct dp_data_rate ti_sn_bridge_dp_rate_table[] = {
+	{1, 1620},
+	{2, 2160},
+	{3, 2430},
+	{4, 2700},
+	{5, 3240},
+	{6, 4320},
+	{7, 5400},
+};
+
+static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
+{
+	unsigned int bit_rate_mhz, clk_freq_mhz, dp_rate_mhz;
+	unsigned int val = 0, i = 0;
+	struct drm_display_mode *mode = &pdata->curr_mode;
+
+	/* set DSIA clk frequency */
+	bit_rate_mhz = (mode->clock / 1000) *
+			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
+	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
+
+	/* for each increment in val, frequency increases by 5MHz */
+	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
+		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
+	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
+
+	/* set DP data rate */
+	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * DP_CLK_FUDGE_NUM) /
+							DP_CLK_FUDGE_DEN;
+	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_table); i++)
+		if (ti_sn_bridge_dp_rate_table[i].dp_rate > dp_rate_mhz)
+			break;
+	if (i == ARRAY_SIZE(ti_sn_bridge_dp_rate_table))
+		i--; /* set to maximum possible */
+
+	val = ti_sn_bridge_dp_rate_table[i].bit_val << SN_DP_DATA_RATE_OFFSET;
+	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
+						SN_DP_DATA_RATE_BITS, val);
+}
+
+static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata)
+{
+	struct drm_display_mode *mode = &pdata->curr_mode;
+
+	regmap_write(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
+			mode->hdisplay & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
+			(mode->hdisplay >> SN_TIMING_HIGH_OFFSET) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
+			mode->vdisplay & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
+			(mode->vdisplay >> SN_TIMING_HIGH_OFFSET) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->hsync_end - mode->hsync_start) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->hsync_end - mode->hsync_start) >>
+			 SN_TIMING_HIGH_OFFSET) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
+			(mode->vsync_end - mode->vsync_start) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
+			((mode->vsync_end - mode->vsync_start) >>
+			 SN_TIMING_HIGH_OFFSET) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
+			(mode->htotal - mode->hsync_end) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_VERTICAL_BACK_PORCH_REG,
+			(mode->vtotal - mode->vsync_end) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
+			(mode->hsync_start - mode->hdisplay) & 0xFF);
+	regmap_write(pdata->regmap, SN_CHA_VERTICAL_FRONT_PORCH_REG,
+			(mode->vsync_start - mode->vdisplay) & 0xFF);
+	usleep_range(10000, 10050); /* 10ms delay recommended by spec */
+}
+
+static void ti_sn_bridge_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+	struct drm_panel *panel = pdata->panel;
+	unsigned int val = 0;
+
+	if (panel) {
+		drm_panel_prepare(panel);
+		/* in case drm_panel is connected then HPD is not supported */
+		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
+				   SN_HPD_DISABLE_BIT, SN_HPD_DISABLE_BIT);
+	}
+
+	/* DSI_A lane config */
+	val = (4 - pdata->dsi->lanes) << SN_DSIA_LANE_OFFSET;
+	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
+					SN_DSIA_NUM_LANES_BITS, val);
+
+	/* DP lane config */
+	val = (pdata->dsi->lanes - 1) << SN_DP_LANE_OFFSET;
+	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG,
+					SN_DP_NUM_LANES_BITS, val);
+
+	/* set dsi/dp clk frequency value */
+	ti_sn_bridge_set_dsi_dp_rate(pdata);
+
+	/* enable DP PLL */
+	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
+	usleep_range(10000, 10050); /* 10ms delay recommended by spec */
+
+	/**
+	 * The SN65DSI86 only supports ASSR Display Authentication method and
+	 * this method is enabled by default. An eDP panel must support this
+	 * authentication method. We need to enable this method in the eDP panel
+	 * at DisplayPort address 0x0010A prior to link training.
+	 */
+	regmap_write(pdata->regmap, SN_AUX_WDATA0_REG, 0x01);
+	regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, 0x00);
+	regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG, 0x01);
+	regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, 0x0A);
+	regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, 0x01);
+	regmap_write(pdata->regmap, SN_AUX_CMD_REG, 0x81);
+	usleep_range(10000, 10050); /* 10ms delay recommended by spec */
+
+	/* Semi auto link training mode */
+	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0x0A);
+	msleep(20); /* 20ms delay recommended by spec */
+
+	/* config video parameters */
+	ti_sn_bridge_set_video_timings(pdata);
+
+	/* enable video stream */
+	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
+			   SN_ENABLE_VID_STREAM_BIT, SN_ENABLE_VID_STREAM_BIT);
+
+	if (panel)
+		drm_panel_enable(panel);
+}
+
+static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+
+	/* configure bridge CLK_SRC and ref_clk */
+	ti_sn_bridge_set_refclk(pdata);
+}
+
+static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
+{
+	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
+
+	ti_sn_bridge_power_ctrl(pdata, false);
+}
+
+static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
+	.attach = ti_sn_bridge_attach,
+	.pre_enable = ti_sn_bridge_pre_enable,
+	.enable = ti_sn_bridge_enable,
+	.disable = ti_sn_bridge_disable,
+	.post_disable = ti_sn_bridge_post_disable,
+	.mode_set = ti_sn_bridge_mode_set,
+};
+
+static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
+{
+	struct device_node *np = pdata->dev->of_node;
+	struct device_node *end_node;
+
+	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
+	if (!end_node) {
+		DRM_ERROR("remote endpoint not found\n");
+		return -ENODEV;
+	}
+
+	pdata->host_node = of_graph_get_remote_port_parent(end_node);
+	of_node_put(end_node);
+	if (!pdata->host_node) {
+		DRM_ERROR("remote node not found\n");
+		return -ENODEV;
+	}
+	of_node_put(pdata->host_node);
+
+	return 0;
+}
+
+static int ti_sn_bridge_probe(struct i2c_client *client,
+	 const struct i2c_device_id *id)
+{
+	struct ti_sn_bridge *pdata;
+	struct device_node *ddc_node;
+	int ret = 0;
+
+	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
+		DRM_ERROR("device doesn't support I2C\n");
+		return -ENODEV;
+	}
+
+	pdata = devm_kzalloc(&client->dev,
+		sizeof(struct ti_sn_bridge), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	pdata->dev = &client->dev;
+	DRM_DEBUG("I2C address is %x\n", client->addr);
+
+	pdata->regmap = devm_regmap_init_i2c(client,
+				&ti_sn_bridge_regmap_config);
+	if (IS_ERR(pdata->regmap))
+		return PTR_ERR(pdata->regmap);
+
+	pdata->enable_gpio = devm_gpiod_get(pdata->dev,
+					    "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(pdata->enable_gpio)) {
+		DRM_ERROR("failed to get enable gpio from DT\n");
+		ret = PTR_ERR(pdata->enable_gpio);
+		return ret;
+	}
+
+	ret = ti_sn_bridge_parse_regulators(pdata);
+	if (ret) {
+		DRM_ERROR("failed to parse regulators\n");
+		return ret;
+	}
+
+	ret = of_property_read_u32(pdata->dev->of_node,
+			"refclk-freq-khz", &pdata->refclk_khz);
+	if (ret)
+		pdata->refclk_khz = SN_DEFAULT_REF_CLK_KHZ;
+
+	ret = ti_sn_bridge_parse_dsi_host(pdata);
+	if (ret)
+		return ret;
+
+	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+		if (!pdata->ddc) {
+			dev_dbg(pdata->dev, "failed to read ddc node\n");
+			return -EPROBE_DEFER;
+		}
+	} else {
+		dev_dbg(pdata->dev, "no ddc property found\n");
+	}
+
+	ti_sn_bridge_power_ctrl(pdata, true);
+	ret = ti_sn_bridge_read_device_rev(pdata);
+	ti_sn_bridge_power_ctrl(pdata, false);
+	if (ret)
+		return ret;
+
+	i2c_set_clientdata(client, pdata);
+	mutex_init(&pdata->lock);
+
+	pdata->bridge.funcs = &ti_sn_bridge_funcs;
+	pdata->bridge.of_node = client->dev.of_node;
+
+	drm_bridge_add(&pdata->bridge);
+
+	DRM_DEBUG("bridge device registered successfully\n");
+
+	return 0;
+}
+
+static int ti_sn_bridge_remove(struct i2c_client *client)
+{
+	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
+
+	if (!pdata)
+		return -EINVAL;
+
+	mipi_dsi_detach(pdata->dsi);
+	mipi_dsi_device_unregister(pdata->dsi);
+
+	drm_bridge_remove(&pdata->bridge);
+	i2c_put_adapter(pdata->ddc);
+
+	return 0;
+}
+
+static struct i2c_device_id ti_sn_bridge_id[] = {
+	{ "ti,sn65dsi86", 0},
+	{},
+};
+MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
+
+static const struct of_device_id ti_sn_bridge_match_table[] = {
+	{.compatible = "ti,sn65dsi86"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
+
+static struct i2c_driver ti_sn_bridge_driver = {
+	.driver = {
+		.name = "ti_sn65dsi86",
+		.of_match_table = ti_sn_bridge_match_table,
+	},
+	.probe = ti_sn_bridge_probe,
+	.remove = ti_sn_bridge_remove,
+	.id_table = ti_sn_bridge_id,
+};
+
+module_i2c_driver(ti_sn_bridge_driver);
+MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
+MODULE_LICENSE("GPL v2");
-- 
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: [PATCH v5 1/4] drm/bridge: add support for sn65dsi86 bridge driver

[Sean Paul]

On Wed, May 02, 2018 at 10:01:59AM +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.
> 
> Changes in v1:
>  - Split the dt-bindings and the driver support into separate patches
>    (Andrzej Hajda).
>  - Use of gpiod APIs to parse and configure gpios instead of obsolete ones
>    (Andrzej Hajda).
>  - Use macros to define the register offsets (Andrzej Hajda).
> 
> Changes in v2:
>  - Separate out edp panel specific HW resource handling from bridge
>    driver and create a separate edp panel drivers to handle panel
>    specific mode information and HW resources (Sean Paul).
>  - Replace pr_* APIs to DRM_* APIs to log error or debug information
>    (Sean Paul).
>  - Remove some of the unnecessary structure/variable from driver (Sean
>    Paul).
>  - Rename the function and structure prefix "sn65dsi86" to "ti_sn_bridge"
>    (Sean Paul / Rob Herring).
>  - Remove most of the hard-coding and modified the bridge init sequence
>    based on current mode (Sean Paul).
>  - Remove the existing function to retrieve the EDID data and
>    implemented this as an i2c_adapter and use drm_get_edid() (Sean Paul).
>  - Remove the dummy irq handler implementation, will add back the
>    proper irq handling later (Sean Paul).
>  - Capture the required enable gpios in a single array based on dt entry
>    instead of having individual descriptor for each gpio (Sean Paul).
> 
> Changes in v3:
>  - Remove usage of irq_gpio and replace it as "interrupts" property (Rob
>    Herring).
>  - Remove the unnecessary header file inclusions (Sean Paul).
>  - Rearrange the header files in alphabetical order (Sean Paul).
>  - Use regmap interface to perform i2c transactions.
>  - Update Copyright/License field and address other review comments
>    (Jordan Crouse).
> 
> Changes in v4:
>  - Update License/Copyright (Sean Paul).
>  - Add Kconfig and Makefile changes (Sean Paul).
>  - Drop i2c gpio handling from this bridge driver, since i2c sda/scl gpios
>    will be handled by i2c master.
>  - Remove unnecessary goto statements (Sean Paul).
>  - Add mutex lock to power_ctrl API to avoid race conditions (Sean
>    Paul).
>  - Add support to parse reference clk frequency from dt(optional).
>  - Update the bridge chip enable/disable sequence.

It seems like you also added 2 new supply names, as well as remove the
configurable gpios?

> 
> Changes in v5:
>  - Fixed Kbuild test service reported warnings.
> 
> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
> ---
>  drivers/gpu/drm/bridge/Kconfig        |   9 +
>  drivers/gpu/drm/bridge/Makefile       |   1 +
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 725 ++++++++++++++++++++++++++++++++++
>  3 files changed, 735 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
> 
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
> index 3b99d5a..8153150 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -108,6 +108,15 @@ config DRM_TI_TFP410
>  	---help---
>  	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
>  
> +config DRM_TI_SN65DSI86
> +	tristate "TI SN65DSI86 DSI to eDP bridge"
> +	depends on OF
> +	select DRM_KMS_HELPER
> +	select REGMAP_I2C
> +	select DRM_PANEL
> +	---help---
> +	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
> +
>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>  
>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
> diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
> index 373eb28..3711be8 100644
> --- a/drivers/gpu/drm/bridge/Makefile
> +++ b/drivers/gpu/drm/bridge/Makefile
> @@ -12,4 +12,5 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
> +obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>  obj-y += synopsys/
> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> new file mode 100644
> index 0000000..019c7cd
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
> @@ -0,0 +1,725 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <drm/drmP.h>
> +#include <drm/drm_atomic.h>
> +#include <drm/drm_atomic_helper.h>
> +#include <drm/drm_crtc_helper.h>
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_panel.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_graph.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define SN_BRIDGE_REVISION_ID 0x2
> +
> +/* Link Training specific registers */
> +#define SN_DEVICE_REV_REG			0x08
> +#define SN_HPD_DISABLE_REG			0x5C
> +#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
> +/* 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 MIN_DSI_CLK_FREQ_MHZ	40
> +#define SN_DEFAULT_REF_CLK_KHZ	19200
> +
> +/* fudge factor required to account for 8b/10b encoding */
> +#define DP_CLK_FUDGE_NUM	10
> +#define DP_CLK_FUDGE_DEN	8
> +
> +#define DPPLL_CLK_SRC_REFCLK	0
> +#define DPPLL_CLK_SRC_DSICLK	1
> +
> +#define SN_DSIA_REFCLK_OFFSET	1
> +#define SN_DSIA_LANE_OFFSET	3
> +#define SN_DP_LANE_OFFSET	4
> +#define SN_DP_DATA_RATE_OFFSET	5
> +#define SN_TIMING_HIGH_OFFSET	8
> +
> +#define SN_ENABLE_VID_STREAM_BIT	BIT(3)
> +#define SN_DSIA_NUM_LANES_BITS		(BIT(4) | BIT(3))
> +#define SN_DP_NUM_LANES_BITS		(BIT(5) | BIT(4))
> +#define SN_DP_DATA_RATE_BITS		(BIT(7) | BIT(6) | BIT(5))
> +#define SN_HPD_DISABLE_BIT		BIT(0)
> +
> +struct ti_sn_bridge {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct drm_bridge bridge;
> +	struct drm_connector connector;
> +	struct device_node *host_node;
> +	struct mipi_dsi_device *dsi;
> +	unsigned int refclk_khz;
> +	struct drm_panel *panel;
> +	struct gpio_desc *enable_gpio;
> +	unsigned int num_supplies;
> +	struct regulator_bulk_data *supplies;
> +	struct i2c_adapter *ddc;
> +	unsigned int num_modes;

You only use this in one function, it should be a local.

> +	struct drm_display_mode curr_mode;
> +	struct mutex lock;
> +	unsigned int ctrl_ref_count;
> +};
> +
> +static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> +	{ .range_min = 0, .range_max = 0xff },
> +};
> +
> +static const struct regmap_access_table ti_sn_bridge_volatile_table = {
> +	.yes_ranges = ti_sn_bridge_volatile_ranges,
> +	.n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges),
> +};
> +
> +static const struct regmap_config ti_sn_bridge_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.volatile_table = &ti_sn_bridge_volatile_table,
> +	.cache_type = REGCACHE_NONE,
> +};
> +
> +static int ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool enable)
> +{
> +	int ret = 0;
> +
> +	mutex_lock(&pdata->lock);
> +	if (enable)
> +		pdata->ctrl_ref_count++;
> +	else
> +		pdata->ctrl_ref_count--;

I think you should use a kref instead of rolling your own ref_count. You can
handle release by calling kref_put_mutex(), which will handle the reference and
the lock. On the acquire side, you can use kref_get_unless_zero which will be
fast if the reference is already active.

> +
> +	if (enable && (pdata->ctrl_ref_count == 1)) {
> +		ret = regulator_bulk_enable(pdata->num_supplies,
> +						 pdata->supplies);
> +		if (ret) {
> +			DRM_ERROR("bridge regulator enable failed\n");

print ret on failure

> +			goto exit;
> +		}
> +
> +		gpiod_set_value(pdata->enable_gpio, 1);
> +	} else if (!enable && !pdata->ctrl_ref_count) {
> +		gpiod_set_value(pdata->enable_gpio, 0);
> +
> +		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
> +	} else {
> +		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
> +						enable, pdata->ctrl_ref_count);

No need for this.

> +	}
> +
> +exit:
> +	mutex_unlock(&pdata->lock);
> +	return ret;
> +}
> +
> +/* Connector funcs */
> +static struct ti_sn_bridge *
> +connector_to_ti_sn_bridge(struct drm_connector *connector)
> +{
> +	return container_of(connector, struct ti_sn_bridge, connector);
> +}
> +
> +static int ti_sn_bridge_connector_get_modes(struct drm_connector *connector)
> +{
> +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> +	struct drm_panel *panel = pdata->panel;
> +	struct edid *edid;
> +
> +	if (panel) {
> +		DRM_DEBUG("get mode from connected drm_panel\n");
> +		pdata->num_modes = drm_panel_get_modes(panel);
> +		return pdata->num_modes;
> +	}
> +
> +	/* get from EDID */

This comment isn't useful.

> +	if (!pdata->ddc)
> +		return 0;
> +
> +	ti_sn_bridge_power_ctrl(pdata, true);
> +	edid = drm_get_edid(connector, pdata->ddc);
> +	ti_sn_bridge_power_ctrl(pdata, false);
> +	if (!edid)
> +		return 0;
> +
> +	drm_mode_connector_update_edid_property(connector, edid);
> +	pdata->num_modes = drm_add_edid_modes(connector, edid);
> +	kfree(edid);
> +
> +	return pdata->num_modes;
> +}
> +
> +static enum drm_mode_status
> +ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
> +			     struct drm_display_mode *mode)
> +{
> +	/* maximum supported resolution is 4K at 60 fps */
> +	if (mode->clock > 594000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
> +static struct drm_connector_helper_funcs ti_sn_bridge_connector_helper_funcs = {
> +	.get_modes = ti_sn_bridge_connector_get_modes,
> +	.mode_valid = ti_sn_bridge_connector_mode_valid,
> +};
> +
> +static enum drm_connector_status
> +ti_sn_bridge_connector_detect(struct drm_connector *connector, bool force)
> +{
> +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
> +
> +	/**
> +	 * TODO: Currently if drm_panel is present, then always
> +	 * return the status as connected. Need to add support to detect
> +	 * device state for no panel(hot pluggable) scenarios.
> +	 */
> +	if (pdata->panel)
> +		return connector_status_connected;
> +	else
> +		return connector_status_unknown;
> +}
> +
> +static const struct drm_connector_funcs ti_sn_bridge_connector_funcs = {
> +	.fill_modes = drm_helper_probe_single_connector_modes,
> +	.detect = ti_sn_bridge_connector_detect,
> +	.destroy = drm_connector_cleanup,
> +	.reset = drm_atomic_helper_connector_reset,
> +	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> +};
> +
> +static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge *bridge)
> +{
> +	return container_of(bridge, struct ti_sn_bridge, bridge);
> +}
> +
> +static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
> +{
> +	unsigned int rev = 0;
> +	int ret = 0;
> +
> +	ret = regmap_read(pdata->regmap, SN_DEVICE_REV_REG, &rev);
> +	if (ret)
> +		return ret;
> +
> +	if (rev != SN_BRIDGE_REVISION_ID) {
> +		DRM_ERROR("ti_sn_bridge revision id: 0x%x mismatch\n", rev);
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static const char * const ti_sn_bridge_supply_names[] = {
> +	"vcca",
> +	"vcc",
> +	"vccio",
> +	"vpll",
> +};
> +
> +static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
> +{
> +	unsigned int i;
> +
> +	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
> +
> +	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
> +				     sizeof(*pdata->supplies), GFP_KERNEL);
> +	if (!pdata->supplies)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < pdata->num_supplies; i++)
> +		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
> +
> +	return devm_regulator_bulk_get(pdata->dev,
> +				pdata->num_supplies, pdata->supplies);
> +}
> +
> +static int ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
> +{
> +	struct device_node *panel_node, *port, *endpoint;
> +
> +	pdata->panel = NULL;
> +	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
> +	if (port) {

Flip this to avoid indenting the whole function. ie:

if (!port)
        return 0;

> +		endpoint = of_get_child_by_name(port, "endpoint");
> +		of_node_put(port);
> +		if (!endpoint) {
> +			DRM_ERROR("no output endpoint found\n");
> +			return -EINVAL;
> +		}
> +
> +		panel_node = of_graph_get_remote_port_parent(endpoint);
> +		of_node_put(endpoint);
> +		if (!panel_node) {
> +			DRM_ERROR("no output node found\n");
> +			return -EINVAL;
> +		}
> +
> +		pdata->panel = of_drm_find_panel(panel_node);
> +		of_node_put(panel_node);
> +		if (!pdata->panel) {
> +			DRM_ERROR("no panel node found\n");
> +			return -EINVAL;
> +		}
> +		drm_panel_attach(pdata->panel, &pdata->connector);
> +		DRM_DEBUG("panel attached\n");

This log should be DRM_DEBUG_KMS and you should be more descriptive in your
message (ie: "Attached sn65dsi86 panel").

> +	}
> +
> +	return 0;
> +}
> +
> +static int ti_sn_bridge_attach(struct drm_bridge *bridge)
> +{
> +	struct mipi_dsi_host *host;
> +	struct mipi_dsi_device *dsi;
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +	int ret;
> +	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
> +						   .channel = 0,
> +						   .node = NULL,
> +						 };
> +
> +	if (!bridge->encoder) {
> +		DRM_ERROR("Parent encoder object not found\n");
> +		return -ENODEV;
> +	}
> +
> +	/* HPD not supported */
> +	pdata->connector.polled = 0;
> +
> +	ret = drm_connector_init(bridge->dev, &pdata->connector,
> +				 &ti_sn_bridge_connector_funcs,
> +				 DRM_MODE_CONNECTOR_eDP);
> +	if (ret) {
> +		DRM_ERROR("Failed to initialize connector with drm\n");
> +		return ret;
> +	}
> +
> +	drm_connector_helper_add(&pdata->connector,
> +				 &ti_sn_bridge_connector_helper_funcs);
> +	drm_mode_connector_attach_encoder(&pdata->connector, bridge->encoder);
> +
> +	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
> +	if (!host) {
> +		DRM_ERROR("failed to find dsi host\n");
> +		return -ENODEV;
> +	}
> +
> +	dsi = mipi_dsi_device_register_full(host, &info);
> +	if (IS_ERR(dsi)) {
> +		DRM_ERROR("failed to create dsi device\n");
> +		ret = PTR_ERR(dsi);
> +		return ret;
> +	}
> +
> +	/* TODO: setting to 4 lanes always for now */
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
> +			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		DRM_ERROR("failed to attach dsi to host\n");
> +		mipi_dsi_device_unregister(dsi);
> +		return ret;
> +	}
> +
> +	pdata->dsi = dsi;
> +
> +	DRM_DEBUG("bridge attached\n");

Same comment here regarding being more descriptive.

> +	/* attach panel to bridge */
> +	ti_sn_bridge_attach_panel(pdata);
> +
> +	return 0;
> +}
> +
> +static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
> +				    struct drm_display_mode *mode,
> +				    struct drm_display_mode *adj_mode)
> +{
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> +	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, clock=%d\n",
> +		adj_mode->hdisplay, adj_mode->vdisplay,
> +		adj_mode->vrefresh, adj_mode->clock);
> +
> +	drm_mode_copy(&pdata->curr_mode, adj_mode);
> +}
> +
> +static void ti_sn_bridge_disable(struct drm_bridge *bridge)
> +{
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +	struct drm_panel *panel = pdata->panel;
> +
> +	if (panel) {
> +		drm_panel_disable(panel);
> +		drm_panel_unprepare(panel);
> +	}
> +
> +	/* disable video stream */
> +	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +			SN_ENABLE_VID_STREAM_BIT, 0);
> +	/* semi auto link training mode OFF */
> +	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
> +	/* disable DP PLL */
> +	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
> +}
> +
> +/* reference clk frequencies supported by the bridge in KHz */
> +static u32 ti_sn_bridge_ref_clk_table[] = {
> +	12000,
> +	19200,
> +	26000,
> +	27000,
> +	38400,
> +};
> +
> +static void ti_sn_bridge_set_refclk(struct ti_sn_bridge *pdata)
> +{
> +	int i = 0;
> +
> +	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_ref_clk_table); i++)
> +		if (ti_sn_bridge_ref_clk_table[i] == pdata->refclk_khz)
> +			break;
> +	regmap_write(pdata->regmap, SN_REFCLK_FREQ_REG,
> +		(DPPLL_CLK_SRC_REFCLK | (i << SN_DSIA_REFCLK_OFFSET)));

What if the refclk frequency isn't found?

> +}
> +
> +struct dp_data_rate {
> +	unsigned int bit_val;
> +	unsigned int dp_rate;
> +};
> +
> +/* dp data rate supported by the bridge Mbps */
> +static struct dp_data_rate ti_sn_bridge_dp_rate_table[] = {
> +	{1, 1620},
> +	{2, 2160},
> +	{3, 2430},
> +	{4, 2700},
> +	{5, 3240},
> +	{6, 4320},
> +	{7, 5400},
> +};
> +
> +static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
> +{
> +	unsigned int bit_rate_mhz, clk_freq_mhz, dp_rate_mhz;
> +	unsigned int val = 0, i = 0;
> +	struct drm_display_mode *mode = &pdata->curr_mode;
> +
> +	/* set DSIA clk frequency */
> +	bit_rate_mhz = (mode->clock / 1000) *
> +			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
> +	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
> +
> +	/* for each increment in val, frequency increases by 5MHz */
> +	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
> +		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
> +	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
> +
> +	/* set DP data rate */
> +	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * DP_CLK_FUDGE_NUM) /
> +							DP_CLK_FUDGE_DEN;
> +	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_table); i++)
> +		if (ti_sn_bridge_dp_rate_table[i].dp_rate > dp_rate_mhz)
> +			break;
> +	if (i == ARRAY_SIZE(ti_sn_bridge_dp_rate_table))
> +		i--; /* set to maximum possible */
> +
> +	val = ti_sn_bridge_dp_rate_table[i].bit_val << SN_DP_DATA_RATE_OFFSET;
> +	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
> +						SN_DP_DATA_RATE_BITS, val);

I think this can be simplified.

/* Array index corresponds to register value */
static const unsigned int ti_sn_bridge_dp_rate_lut[] = { 
        0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
};

static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
{
        ...
        for (i = ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i >= 0; i--) {
                if (ti_sn_bridge_dp_rate_lut[i] <= dp_rate_mhz)
                        break;
        }
        regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
                           SN_DP_DATA_RATE_BITS, i << SN_DP_DATA_RATE_OFFSET);
}

> +}
> +
> +static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge *pdata)
> +{
> +	struct drm_display_mode *mode = &pdata->curr_mode;
> +
> +	regmap_write(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
> +			mode->hdisplay & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
> +			(mode->hdisplay >> SN_TIMING_HIGH_OFFSET) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
> +			mode->vdisplay & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
> +			(mode->vdisplay >> SN_TIMING_HIGH_OFFSET) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
> +			(mode->hsync_end - mode->hsync_start) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
> +			((mode->hsync_end - mode->hsync_start) >>
> +			 SN_TIMING_HIGH_OFFSET) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
> +			(mode->vsync_end - mode->vsync_start) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
> +			((mode->vsync_end - mode->vsync_start) >>
> +			 SN_TIMING_HIGH_OFFSET) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
> +			(mode->htotal - mode->hsync_end) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_BACK_PORCH_REG,
> +			(mode->vtotal - mode->vsync_end) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
> +			(mode->hsync_start - mode->hdisplay) & 0xFF);
> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_FRONT_PORCH_REG,
> +			(mode->vsync_start - mode->vdisplay) & 0xFF);
> +	usleep_range(10000, 10050); /* 10ms delay recommended by spec */

You would probably be fine to set the upper bound to 10500 so we don't trigger
an unnecessary interrupt.

> +}
> +
> +static void ti_sn_bridge_enable(struct drm_bridge *bridge)
> +{
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +	struct drm_panel *panel = pdata->panel;
> +	unsigned int val = 0;
> +
> +	if (panel) {
> +		drm_panel_prepare(panel);
> +		/* in case drm_panel is connected then HPD is not supported */
> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
> +				   SN_HPD_DISABLE_BIT, SN_HPD_DISABLE_BIT);
> +	}
> +
> +	/* DSI_A lane config */
> +	val = (4 - pdata->dsi->lanes) << SN_DSIA_LANE_OFFSET;
> +	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
> +					SN_DSIA_NUM_LANES_BITS, val);
> +
> +	/* DP lane config */
> +	val = (pdata->dsi->lanes - 1) << SN_DP_LANE_OFFSET;
> +	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG,
> +					SN_DP_NUM_LANES_BITS, val);
> +
> +	/* set dsi/dp clk frequency value */
> +	ti_sn_bridge_set_dsi_dp_rate(pdata);
> +
> +	/* enable DP PLL */
> +	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
> +	usleep_range(10000, 10050); /* 10ms delay recommended by spec */
> +
> +	/**
> +	 * The SN65DSI86 only supports ASSR Display Authentication method and
> +	 * this method is enabled by default. An eDP panel must support this
> +	 * authentication method. We need to enable this method in the eDP panel
> +	 * at DisplayPort address 0x0010A prior to link training.
> +	 */
> +	regmap_write(pdata->regmap, SN_AUX_WDATA0_REG, 0x01);
> +	regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, 0x00);
> +	regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG, 0x01);
> +	regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, 0x0A);
> +	regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, 0x01);
> +	regmap_write(pdata->regmap, SN_AUX_CMD_REG, 0x81);
> +	usleep_range(10000, 10050); /* 10ms delay recommended by spec */
> +
> +	/* Semi auto link training mode */
> +	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0x0A);
> +	msleep(20); /* 20ms delay recommended by spec */
> +
> +	/* config video parameters */
> +	ti_sn_bridge_set_video_timings(pdata);
> +
> +	/* enable video stream */
> +	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
> +			   SN_ENABLE_VID_STREAM_BIT, SN_ENABLE_VID_STREAM_BIT);
> +
> +	if (panel)
> +		drm_panel_enable(panel);
> +}
> +
> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
> +{
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> +	ti_sn_bridge_power_ctrl(pdata, true);
> +
> +	/* configure bridge CLK_SRC and ref_clk */
> +	ti_sn_bridge_set_refclk(pdata);
> +}
> +
> +static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
> +{
> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
> +
> +	ti_sn_bridge_power_ctrl(pdata, false);
> +}
> +
> +static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
> +	.attach = ti_sn_bridge_attach,
> +	.pre_enable = ti_sn_bridge_pre_enable,
> +	.enable = ti_sn_bridge_enable,
> +	.disable = ti_sn_bridge_disable,
> +	.post_disable = ti_sn_bridge_post_disable,
> +	.mode_set = ti_sn_bridge_mode_set,
> +};
> +
> +static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
> +{
> +	struct device_node *np = pdata->dev->of_node;
> +	struct device_node *end_node;
> +
> +	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
> +	if (!end_node) {
> +		DRM_ERROR("remote endpoint not found\n");
> +		return -ENODEV;
> +	}
> +
> +	pdata->host_node = of_graph_get_remote_port_parent(end_node);
> +	of_node_put(end_node);
> +	if (!pdata->host_node) {
> +		DRM_ERROR("remote node not found\n");
> +		return -ENODEV;
> +	}
> +	of_node_put(pdata->host_node);
> +
> +	return 0;
> +}
> +
> +static int ti_sn_bridge_probe(struct i2c_client *client,
> +	 const struct i2c_device_id *id)
> +{
> +	struct ti_sn_bridge *pdata;
> +	struct device_node *ddc_node;
> +	int ret = 0;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
> +		DRM_ERROR("device doesn't support I2C\n");
> +		return -ENODEV;
> +	}
> +
> +	pdata = devm_kzalloc(&client->dev,
> +		sizeof(struct ti_sn_bridge), GFP_KERNEL);

        pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn_bridge),
                             GFP_KERNEL);

> +	if (!pdata)
> +		return -ENOMEM;
> +
> +	pdata->dev = &client->dev;
> +	DRM_DEBUG("I2C address is %x\n", client->addr);

unnecessary

> +
> +	pdata->regmap = devm_regmap_init_i2c(client,
> +				&ti_sn_bridge_regmap_config);

indentation is wrong here, should be:

        pdata->regmap = devm_regmap_init_i2c(client,
                                             &ti_sn_bridge_regmap_config);

> +	if (IS_ERR(pdata->regmap))

Print an error?

> +		return PTR_ERR(pdata->regmap);
> +
> +	pdata->enable_gpio = devm_gpiod_get(pdata->dev,
> +					    "enable", GPIOD_OUT_LOW);
> +	if (IS_ERR(pdata->enable_gpio)) {
> +		DRM_ERROR("failed to get enable gpio from DT\n");
> +		ret = PTR_ERR(pdata->enable_gpio);
> +		return ret;
> +	}
> +
> +	ret = ti_sn_bridge_parse_regulators(pdata);
> +	if (ret) {
> +		DRM_ERROR("failed to parse regulators\n");
> +		return ret;
> +	}
> +
> +	ret = of_property_read_u32(pdata->dev->of_node,
> +			"refclk-freq-khz", &pdata->refclk_khz);
> +	if (ret)
> +		pdata->refclk_khz = SN_DEFAULT_REF_CLK_KHZ;
> +
> +	ret = ti_sn_bridge_parse_dsi_host(pdata);
> +	if (ret)

DRM_ERROR?

> +		return ret;
> +
> +	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
> +	if (ddc_node) {
> +		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
> +		of_node_put(ddc_node);
> +		if (!pdata->ddc) {
> +			dev_dbg(pdata->dev, "failed to read ddc node\n");
> +			return -EPROBE_DEFER;
> +		}
> +	} else {
> +		dev_dbg(pdata->dev, "no ddc property found\n");

You've copied dev_dbg() logs, they should be DRM_DEBUG_KMS().

> +	}
> +
> +	ti_sn_bridge_power_ctrl(pdata, true);
> +	ret = ti_sn_bridge_read_device_rev(pdata);
> +	ti_sn_bridge_power_ctrl(pdata, false);

This could be done before handling refclk or ddc.

> +	if (ret)
> +		return ret;
> +
> +	i2c_set_clientdata(client, pdata);
> +	mutex_init(&pdata->lock);
> +
> +	pdata->bridge.funcs = &ti_sn_bridge_funcs;
> +	pdata->bridge.of_node = client->dev.of_node;
> +
> +	drm_bridge_add(&pdata->bridge);
> +
> +	DRM_DEBUG("bridge device registered successfully\n");

unnecessary

> +
> +	return 0;
> +}
> +
> +static int ti_sn_bridge_remove(struct i2c_client *client)
> +{
> +	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
> +
> +	if (!pdata)
> +		return -EINVAL;
> +
> +	mipi_dsi_detach(pdata->dsi);
> +	mipi_dsi_device_unregister(pdata->dsi);
> +
> +	drm_bridge_remove(&pdata->bridge);
> +	i2c_put_adapter(pdata->ddc);
> +
> +	return 0;
> +}
> +
> +static struct i2c_device_id ti_sn_bridge_id[] = {
> +	{ "ti,sn65dsi86", 0},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
> +
> +static const struct of_device_id ti_sn_bridge_match_table[] = {
> +	{.compatible = "ti,sn65dsi86"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
> +
> +static struct i2c_driver ti_sn_bridge_driver = {
> +	.driver = {
> +		.name = "ti_sn65dsi86",
> +		.of_match_table = ti_sn_bridge_match_table,
> +	},
> +	.probe = ti_sn_bridge_probe,
> +	.remove = ti_sn_bridge_remove,
> +	.id_table = ti_sn_bridge_id,
> +};
> +
> +module_i2c_driver(ti_sn_bridge_driver);
> +MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Stephen Boyd]

Quoting Sean Paul (2018-05-02 12:03:16)
> On Wed, May 02, 2018 at 10:01:59AM +0530, Sandeep Panda wrote:
> 
> > +     struct drm_display_mode curr_mode;
> > +     struct mutex lock;
> > +     unsigned int ctrl_ref_count;
> > +};
> > +
> > +static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
> > +     { .range_min = 0, .range_max = 0xff },
> > +};
> > +
> > +static const struct regmap_access_table ti_sn_bridge_volatile_table = {
> > +     .yes_ranges = ti_sn_bridge_volatile_ranges,
> > +     .n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges),
> > +};
> > +
> > +static const struct regmap_config ti_sn_bridge_regmap_config = {
> > +     .reg_bits = 8,
> > +     .val_bits = 8,
> > +     .volatile_table = &ti_sn_bridge_volatile_table,
> > +     .cache_type = REGCACHE_NONE,
> > +};
> > +
> > +static int ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool enable)
> > +{
> > +     int ret = 0;
> > +
> > +     mutex_lock(&pdata->lock);
> > +     if (enable)
> > +             pdata->ctrl_ref_count++;
> > +     else
> > +             pdata->ctrl_ref_count--;
> 
> I think you should use a kref instead of rolling your own ref_count. You can
> handle release by calling kref_put_mutex(), which will handle the reference and
> the lock. On the acquire side, you can use kref_get_unless_zero which will be
> fast if the reference is already active.

Why not use runtime PM?

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

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

[spanda-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-05-08 03:50, Stephen Boyd wrote:
> Quoting Sean Paul (2018-05-02 12:03:16)
>> On Wed, May 02, 2018 at 10:01:59AM +0530, Sandeep Panda wrote:
>> 
>> > +     struct drm_display_mode curr_mode;
>> > +     struct mutex lock;
>> > +     unsigned int ctrl_ref_count;
>> > +};
>> > +
>> > +static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
>> > +     { .range_min = 0, .range_max = 0xff },
>> > +};
>> > +
>> > +static const struct regmap_access_table ti_sn_bridge_volatile_table = {
>> > +     .yes_ranges = ti_sn_bridge_volatile_ranges,
>> > +     .n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges),
>> > +};
>> > +
>> > +static const struct regmap_config ti_sn_bridge_regmap_config = {
>> > +     .reg_bits = 8,
>> > +     .val_bits = 8,
>> > +     .volatile_table = &ti_sn_bridge_volatile_table,
>> > +     .cache_type = REGCACHE_NONE,
>> > +};
>> > +
>> > +static int ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool enable)
>> > +{
>> > +     int ret = 0;
>> > +
>> > +     mutex_lock(&pdata->lock);
>> > +     if (enable)
>> > +             pdata->ctrl_ref_count++;
>> > +     else
>> > +             pdata->ctrl_ref_count--;
>> 
>> I think you should use a kref instead of rolling your own ref_count. 
>> You can
>> handle release by calling kref_put_mutex(), which will handle the 
>> reference and
>> the lock. On the acquire side, you can use kref_get_unless_zero which 
>> will be
>> fast if the reference is already active.
> 
> Why not use runtime PM?

I think PM runtime will be a better approach since we are trying to 
protect bridge power source related resources here.
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[spanda-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-05-03 00:33, Sean Paul wrote:
> On Wed, May 02, 2018 at 10:01:59AM +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.
>> 
>> Changes in v1:
>>  - Split the dt-bindings and the driver support into separate patches
>>    (Andrzej Hajda).
>>  - Use of gpiod APIs to parse and configure gpios instead of obsolete 
>> ones
>>    (Andrzej Hajda).
>>  - Use macros to define the register offsets (Andrzej Hajda).
>> 
>> Changes in v2:
>>  - Separate out edp panel specific HW resource handling from bridge
>>    driver and create a separate edp panel drivers to handle panel
>>    specific mode information and HW resources (Sean Paul).
>>  - Replace pr_* APIs to DRM_* APIs to log error or debug information
>>    (Sean Paul).
>>  - Remove some of the unnecessary structure/variable from driver (Sean
>>    Paul).
>>  - Rename the function and structure prefix "sn65dsi86" to 
>> "ti_sn_bridge"
>>    (Sean Paul / Rob Herring).
>>  - Remove most of the hard-coding and modified the bridge init 
>> sequence
>>    based on current mode (Sean Paul).
>>  - Remove the existing function to retrieve the EDID data and
>>    implemented this as an i2c_adapter and use drm_get_edid() (Sean 
>> Paul).
>>  - Remove the dummy irq handler implementation, will add back the
>>    proper irq handling later (Sean Paul).
>>  - Capture the required enable gpios in a single array based on dt 
>> entry
>>    instead of having individual descriptor for each gpio (Sean Paul).
>> 
>> Changes in v3:
>>  - Remove usage of irq_gpio and replace it as "interrupts" property 
>> (Rob
>>    Herring).
>>  - Remove the unnecessary header file inclusions (Sean Paul).
>>  - Rearrange the header files in alphabetical order (Sean Paul).
>>  - Use regmap interface to perform i2c transactions.
>>  - Update Copyright/License field and address other review comments
>>    (Jordan Crouse).
>> 
>> Changes in v4:
>>  - Update License/Copyright (Sean Paul).
>>  - Add Kconfig and Makefile changes (Sean Paul).
>>  - Drop i2c gpio handling from this bridge driver, since i2c sda/scl 
>> gpios
>>    will be handled by i2c master.
>>  - Remove unnecessary goto statements (Sean Paul).
>>  - Add mutex lock to power_ctrl API to avoid race conditions (Sean
>>    Paul).
>>  - Add support to parse reference clk frequency from dt(optional).
>>  - Update the bridge chip enable/disable sequence.
> 
> It seems like you also added 2 new supply names, as well as remove the
> configurable gpios?
> 
>> 
>> Changes in v5:
>>  - Fixed Kbuild test service reported warnings.
>> 
>> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
>> ---
>>  drivers/gpu/drm/bridge/Kconfig        |   9 +
>>  drivers/gpu/drm/bridge/Makefile       |   1 +
>>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 725 
>> ++++++++++++++++++++++++++++++++++
>>  3 files changed, 735 insertions(+)
>>  create mode 100644 drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> 
>> diff --git a/drivers/gpu/drm/bridge/Kconfig 
>> b/drivers/gpu/drm/bridge/Kconfig
>> index 3b99d5a..8153150 100644
>> --- a/drivers/gpu/drm/bridge/Kconfig
>> +++ b/drivers/gpu/drm/bridge/Kconfig
>> @@ -108,6 +108,15 @@ config DRM_TI_TFP410
>>  	---help---
>>  	  Texas Instruments TFP410 DVI/HDMI Transmitter driver
>> 
>> +config DRM_TI_SN65DSI86
>> +	tristate "TI SN65DSI86 DSI to eDP bridge"
>> +	depends on OF
>> +	select DRM_KMS_HELPER
>> +	select REGMAP_I2C
>> +	select DRM_PANEL
>> +	---help---
>> +	  Texas Instruments SN65DSI86 DSI to eDP Bridge driver
>> +
>>  source "drivers/gpu/drm/bridge/analogix/Kconfig"
>> 
>>  source "drivers/gpu/drm/bridge/adv7511/Kconfig"
>> diff --git a/drivers/gpu/drm/bridge/Makefile 
>> b/drivers/gpu/drm/bridge/Makefile
>> index 373eb28..3711be8 100644
>> --- a/drivers/gpu/drm/bridge/Makefile
>> +++ b/drivers/gpu/drm/bridge/Makefile
>> @@ -12,4 +12,5 @@ obj-$(CONFIG_DRM_TOSHIBA_TC358767) += tc358767.o
>>  obj-$(CONFIG_DRM_ANALOGIX_DP) += analogix/
>>  obj-$(CONFIG_DRM_I2C_ADV7511) += adv7511/
>>  obj-$(CONFIG_DRM_TI_TFP410) += ti-tfp410.o
>> +obj-$(CONFIG_DRM_TI_SN65DSI86) += ti-sn65dsi86.o
>>  obj-y += synopsys/
>> diff --git a/drivers/gpu/drm/bridge/ti-sn65dsi86.c 
>> b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> new file mode 100644
>> index 0000000..019c7cd
>> --- /dev/null
>> +++ b/drivers/gpu/drm/bridge/ti-sn65dsi86.c
>> @@ -0,0 +1,725 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <drm/drmP.h>
>> +#include <drm/drm_atomic.h>
>> +#include <drm/drm_atomic_helper.h>
>> +#include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_panel.h>
>> +#include <linux/gpio.h>
>> +#include <linux/i2c.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_graph.h>
>> +#include <linux/regmap.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#define SN_BRIDGE_REVISION_ID 0x2
>> +
>> +/* Link Training specific registers */
>> +#define SN_DEVICE_REV_REG			0x08
>> +#define SN_HPD_DISABLE_REG			0x5C
>> +#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
>> +/* 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 MIN_DSI_CLK_FREQ_MHZ	40
>> +#define SN_DEFAULT_REF_CLK_KHZ	19200
>> +
>> +/* fudge factor required to account for 8b/10b encoding */
>> +#define DP_CLK_FUDGE_NUM	10
>> +#define DP_CLK_FUDGE_DEN	8
>> +
>> +#define DPPLL_CLK_SRC_REFCLK	0
>> +#define DPPLL_CLK_SRC_DSICLK	1
>> +
>> +#define SN_DSIA_REFCLK_OFFSET	1
>> +#define SN_DSIA_LANE_OFFSET	3
>> +#define SN_DP_LANE_OFFSET	4
>> +#define SN_DP_DATA_RATE_OFFSET	5
>> +#define SN_TIMING_HIGH_OFFSET	8
>> +
>> +#define SN_ENABLE_VID_STREAM_BIT	BIT(3)
>> +#define SN_DSIA_NUM_LANES_BITS		(BIT(4) | BIT(3))
>> +#define SN_DP_NUM_LANES_BITS		(BIT(5) | BIT(4))
>> +#define SN_DP_DATA_RATE_BITS		(BIT(7) | BIT(6) | BIT(5))
>> +#define SN_HPD_DISABLE_BIT		BIT(0)
>> +
>> +struct ti_sn_bridge {
>> +	struct device *dev;
>> +	struct regmap *regmap;
>> +	struct drm_bridge bridge;
>> +	struct drm_connector connector;
>> +	struct device_node *host_node;
>> +	struct mipi_dsi_device *dsi;
>> +	unsigned int refclk_khz;
>> +	struct drm_panel *panel;
>> +	struct gpio_desc *enable_gpio;
>> +	unsigned int num_supplies;
>> +	struct regulator_bulk_data *supplies;
>> +	struct i2c_adapter *ddc;
>> +	unsigned int num_modes;
> 
> You only use this in one function, it should be a local.
> 
Removed.
>> +	struct drm_display_mode curr_mode;
>> +	struct mutex lock;
>> +	unsigned int ctrl_ref_count;
>> +};
>> +
>> +static const struct regmap_range ti_sn_bridge_volatile_ranges[] = {
>> +	{ .range_min = 0, .range_max = 0xff },
>> +};
>> +
>> +static const struct regmap_access_table ti_sn_bridge_volatile_table = 
>> {
>> +	.yes_ranges = ti_sn_bridge_volatile_ranges,
>> +	.n_yes_ranges = ARRAY_SIZE(ti_sn_bridge_volatile_ranges),
>> +};
>> +
>> +static const struct regmap_config ti_sn_bridge_regmap_config = {
>> +	.reg_bits = 8,
>> +	.val_bits = 8,
>> +	.volatile_table = &ti_sn_bridge_volatile_table,
>> +	.cache_type = REGCACHE_NONE,
>> +};
>> +
>> +static int ti_sn_bridge_power_ctrl(struct ti_sn_bridge *pdata, bool 
>> enable)
>> +{
>> +	int ret = 0;
>> +
>> +	mutex_lock(&pdata->lock);
>> +	if (enable)
>> +		pdata->ctrl_ref_count++;
>> +	else
>> +		pdata->ctrl_ref_count--;
> 
> I think you should use a kref instead of rolling your own ref_count. 
> You can
> handle release by calling kref_put_mutex(), which will handle the 
> reference and
> the lock. On the acquire side, you can use kref_get_unless_zero which 
> will be
> fast if the reference is already active.
> 
As per comment from Stephen Boyd used PM runtime instead of our own 
ref_count.
>> +
>> +	if (enable && (pdata->ctrl_ref_count == 1)) {
>> +		ret = regulator_bulk_enable(pdata->num_supplies,
>> +						 pdata->supplies);
>> +		if (ret) {
>> +			DRM_ERROR("bridge regulator enable failed\n");
> 
> print ret on failure
> 
Done.
>> +			goto exit;
>> +		}
>> +
>> +		gpiod_set_value(pdata->enable_gpio, 1);
>> +	} else if (!enable && !pdata->ctrl_ref_count) {
>> +		gpiod_set_value(pdata->enable_gpio, 0);
>> +
>> +		regulator_bulk_disable(pdata->num_supplies, pdata->supplies);
>> +	} else {
>> +		DRM_DEBUG("ti_sn_bridge power ctrl: %d refcount: %d\n",
>> +						enable, pdata->ctrl_ref_count);
> 
> No need for this.
> 
Removed.
>> +	}
>> +
>> +exit:
>> +	mutex_unlock(&pdata->lock);
>> +	return ret;
>> +}
>> +
>> +/* Connector funcs */
>> +static struct ti_sn_bridge *
>> +connector_to_ti_sn_bridge(struct drm_connector *connector)
>> +{
>> +	return container_of(connector, struct ti_sn_bridge, connector);
>> +}
>> +
>> +static int ti_sn_bridge_connector_get_modes(struct drm_connector 
>> *connector)
>> +{
>> +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
>> +	struct drm_panel *panel = pdata->panel;
>> +	struct edid *edid;
>> +
>> +	if (panel) {
>> +		DRM_DEBUG("get mode from connected drm_panel\n");
>> +		pdata->num_modes = drm_panel_get_modes(panel);
>> +		return pdata->num_modes;
>> +	}
>> +
>> +	/* get from EDID */
> 
> This comment isn't useful.
> 
Removed.
>> +	if (!pdata->ddc)
>> +		return 0;
>> +
>> +	ti_sn_bridge_power_ctrl(pdata, true);
>> +	edid = drm_get_edid(connector, pdata->ddc);
>> +	ti_sn_bridge_power_ctrl(pdata, false);
>> +	if (!edid)
>> +		return 0;
>> +
>> +	drm_mode_connector_update_edid_property(connector, edid);
>> +	pdata->num_modes = drm_add_edid_modes(connector, edid);
>> +	kfree(edid);
>> +
>> +	return pdata->num_modes;
>> +}
>> +
>> +static enum drm_mode_status
>> +ti_sn_bridge_connector_mode_valid(struct drm_connector *connector,
>> +			     struct drm_display_mode *mode)
>> +{
>> +	/* maximum supported resolution is 4K at 60 fps */
>> +	if (mode->clock > 594000)
>> +		return MODE_CLOCK_HIGH;
>> +
>> +	return MODE_OK;
>> +}
>> +
>> +static struct drm_connector_helper_funcs 
>> ti_sn_bridge_connector_helper_funcs = {
>> +	.get_modes = ti_sn_bridge_connector_get_modes,
>> +	.mode_valid = ti_sn_bridge_connector_mode_valid,
>> +};
>> +
>> +static enum drm_connector_status
>> +ti_sn_bridge_connector_detect(struct drm_connector *connector, bool 
>> force)
>> +{
>> +	struct ti_sn_bridge *pdata = connector_to_ti_sn_bridge(connector);
>> +
>> +	/**
>> +	 * TODO: Currently if drm_panel is present, then always
>> +	 * return the status as connected. Need to add support to detect
>> +	 * device state for no panel(hot pluggable) scenarios.
>> +	 */
>> +	if (pdata->panel)
>> +		return connector_status_connected;
>> +	else
>> +		return connector_status_unknown;
>> +}
>> +
>> +static const struct drm_connector_funcs ti_sn_bridge_connector_funcs 
>> = {
>> +	.fill_modes = drm_helper_probe_single_connector_modes,
>> +	.detect = ti_sn_bridge_connector_detect,
>> +	.destroy = drm_connector_cleanup,
>> +	.reset = drm_atomic_helper_connector_reset,
>> +	.atomic_duplicate_state = 
>> drm_atomic_helper_connector_duplicate_state,
>> +	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>> +};
>> +
>> +static struct ti_sn_bridge *bridge_to_ti_sn_bridge(struct drm_bridge 
>> *bridge)
>> +{
>> +	return container_of(bridge, struct ti_sn_bridge, bridge);
>> +}
>> +
>> +static int ti_sn_bridge_read_device_rev(struct ti_sn_bridge *pdata)
>> +{
>> +	unsigned int rev = 0;
>> +	int ret = 0;
>> +
>> +	ret = regmap_read(pdata->regmap, SN_DEVICE_REV_REG, &rev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (rev != SN_BRIDGE_REVISION_ID) {
>> +		DRM_ERROR("ti_sn_bridge revision id: 0x%x mismatch\n", rev);
>> +		ret = -EINVAL;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static const char * const ti_sn_bridge_supply_names[] = {
>> +	"vcca",
>> +	"vcc",
>> +	"vccio",
>> +	"vpll",
>> +};
>> +
>> +static int ti_sn_bridge_parse_regulators(struct ti_sn_bridge *pdata)
>> +{
>> +	unsigned int i;
>> +
>> +	pdata->num_supplies = ARRAY_SIZE(ti_sn_bridge_supply_names);
>> +
>> +	pdata->supplies = devm_kcalloc(pdata->dev, pdata->num_supplies,
>> +				     sizeof(*pdata->supplies), GFP_KERNEL);
>> +	if (!pdata->supplies)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < pdata->num_supplies; i++)
>> +		pdata->supplies[i].supply = ti_sn_bridge_supply_names[i];
>> +
>> +	return devm_regulator_bulk_get(pdata->dev,
>> +				pdata->num_supplies, pdata->supplies);
>> +}
>> +
>> +static int ti_sn_bridge_attach_panel(struct ti_sn_bridge *pdata)
>> +{
>> +	struct device_node *panel_node, *port, *endpoint;
>> +
>> +	pdata->panel = NULL;
>> +	port = of_graph_get_port_by_id(pdata->dev->of_node, 1);
>> +	if (port) {
> 
> Flip this to avoid indenting the whole function. ie:
> 
> if (!port)
>         return 0;
> 
Done.
>> +		endpoint = of_get_child_by_name(port, "endpoint");
>> +		of_node_put(port);
>> +		if (!endpoint) {
>> +			DRM_ERROR("no output endpoint found\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		panel_node = of_graph_get_remote_port_parent(endpoint);
>> +		of_node_put(endpoint);
>> +		if (!panel_node) {
>> +			DRM_ERROR("no output node found\n");
>> +			return -EINVAL;
>> +		}
>> +
>> +		pdata->panel = of_drm_find_panel(panel_node);
>> +		of_node_put(panel_node);
>> +		if (!pdata->panel) {
>> +			DRM_ERROR("no panel node found\n");
>> +			return -EINVAL;
>> +		}
>> +		drm_panel_attach(pdata->panel, &pdata->connector);
>> +		DRM_DEBUG("panel attached\n");
> 
> This log should be DRM_DEBUG_KMS and you should be more descriptive in 
> your
> message (ie: "Attached sn65dsi86 panel").
> 
Updated the debug log.
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_sn_bridge_attach(struct drm_bridge *bridge)
>> +{
>> +	struct mipi_dsi_host *host;
>> +	struct mipi_dsi_device *dsi;
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +	int ret;
>> +	const struct mipi_dsi_device_info info = { .type = "ti_sn_bridge",
>> +						   .channel = 0,
>> +						   .node = NULL,
>> +						 };
>> +
>> +	if (!bridge->encoder) {
>> +		DRM_ERROR("Parent encoder object not found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	/* HPD not supported */
>> +	pdata->connector.polled = 0;
>> +
>> +	ret = drm_connector_init(bridge->dev, &pdata->connector,
>> +				 &ti_sn_bridge_connector_funcs,
>> +				 DRM_MODE_CONNECTOR_eDP);
>> +	if (ret) {
>> +		DRM_ERROR("Failed to initialize connector with drm\n");
>> +		return ret;
>> +	}
>> +
>> +	drm_connector_helper_add(&pdata->connector,
>> +				 &ti_sn_bridge_connector_helper_funcs);
>> +	drm_mode_connector_attach_encoder(&pdata->connector, 
>> bridge->encoder);
>> +
>> +	host = of_find_mipi_dsi_host_by_node(pdata->host_node);
>> +	if (!host) {
>> +		DRM_ERROR("failed to find dsi host\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	dsi = mipi_dsi_device_register_full(host, &info);
>> +	if (IS_ERR(dsi)) {
>> +		DRM_ERROR("failed to create dsi device\n");
>> +		ret = PTR_ERR(dsi);
>> +		return ret;
>> +	}
>> +
>> +	/* TODO: setting to 4 lanes always for now */
>> +	dsi->lanes = 4;
>> +	dsi->format = MIPI_DSI_FMT_RGB888;
>> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | 
>> MIPI_DSI_MODE_VIDEO_SYNC_PULSE |
>> +			  MIPI_DSI_MODE_EOT_PACKET | MIPI_DSI_MODE_VIDEO_HSE;
>> +
>> +	ret = mipi_dsi_attach(dsi);
>> +	if (ret < 0) {
>> +		DRM_ERROR("failed to attach dsi to host\n");
>> +		mipi_dsi_device_unregister(dsi);
>> +		return ret;
>> +	}
>> +
>> +	pdata->dsi = dsi;
>> +
>> +	DRM_DEBUG("bridge attached\n");
> 
> Same comment here regarding being more descriptive.
> 
>> +	/* attach panel to bridge */
>> +	ti_sn_bridge_attach_panel(pdata);
>> +
>> +	return 0;
>> +}
>> +
>> +static void ti_sn_bridge_mode_set(struct drm_bridge *bridge,
>> +				    struct drm_display_mode *mode,
>> +				    struct drm_display_mode *adj_mode)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +
>> +	DRM_DEBUG("mode_set: hdisplay=%d, vdisplay=%d, vrefresh=%d, 
>> clock=%d\n",
>> +		adj_mode->hdisplay, adj_mode->vdisplay,
>> +		adj_mode->vrefresh, adj_mode->clock);
>> +
>> +	drm_mode_copy(&pdata->curr_mode, adj_mode);
>> +}
>> +
>> +static void ti_sn_bridge_disable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +	struct drm_panel *panel = pdata->panel;
>> +
>> +	if (panel) {
>> +		drm_panel_disable(panel);
>> +		drm_panel_unprepare(panel);
>> +	}
>> +
>> +	/* disable video stream */
>> +	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
>> +			SN_ENABLE_VID_STREAM_BIT, 0);
>> +	/* semi auto link training mode OFF */
>> +	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0);
>> +	/* disable DP PLL */
>> +	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 0);
>> +}
>> +
>> +/* reference clk frequencies supported by the bridge in KHz */
>> +static u32 ti_sn_bridge_ref_clk_table[] = {
>> +	12000,
>> +	19200,
>> +	26000,
>> +	27000,
>> +	38400,
>> +};
>> +
>> +static void ti_sn_bridge_set_refclk(struct ti_sn_bridge *pdata)
>> +{
>> +	int i = 0;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_ref_clk_table); i++)
>> +		if (ti_sn_bridge_ref_clk_table[i] == pdata->refclk_khz)
>> +			break;
>> +	regmap_write(pdata->regmap, SN_REFCLK_FREQ_REG,
>> +		(DPPLL_CLK_SRC_REFCLK | (i << SN_DSIA_REFCLK_OFFSET)));
> 
> What if the refclk frequency isn't found?
> 
If refclk frequency is not found then we will program i=5, which is also 
valid as per datasheet, this means 19.2MHz.
>> +}
>> +
>> +struct dp_data_rate {
>> +	unsigned int bit_val;
>> +	unsigned int dp_rate;
>> +};
>> +
>> +/* dp data rate supported by the bridge Mbps */
>> +static struct dp_data_rate ti_sn_bridge_dp_rate_table[] = {
>> +	{1, 1620},
>> +	{2, 2160},
>> +	{3, 2430},
>> +	{4, 2700},
>> +	{5, 3240},
>> +	{6, 4320},
>> +	{7, 5400},
>> +};
>> +
>> +static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
>> +{
>> +	unsigned int bit_rate_mhz, clk_freq_mhz, dp_rate_mhz;
>> +	unsigned int val = 0, i = 0;
>> +	struct drm_display_mode *mode = &pdata->curr_mode;
>> +
>> +	/* set DSIA clk frequency */
>> +	bit_rate_mhz = (mode->clock / 1000) *
>> +			mipi_dsi_pixel_format_to_bpp(pdata->dsi->format);
>> +	clk_freq_mhz = bit_rate_mhz / (pdata->dsi->lanes * 2);
>> +
>> +	/* for each increment in val, frequency increases by 5MHz */
>> +	val = (MIN_DSI_CLK_FREQ_MHZ / 5) +
>> +		(((clk_freq_mhz - MIN_DSI_CLK_FREQ_MHZ) / 5) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_DSIA_CLK_FREQ_REG, val);
>> +
>> +	/* set DP data rate */
>> +	dp_rate_mhz = ((bit_rate_mhz / pdata->dsi->lanes) * 
>> DP_CLK_FUDGE_NUM) /
>> +							DP_CLK_FUDGE_DEN;
>> +	for (i = 0; i < ARRAY_SIZE(ti_sn_bridge_dp_rate_table); i++)
>> +		if (ti_sn_bridge_dp_rate_table[i].dp_rate > dp_rate_mhz)
>> +			break;
>> +	if (i == ARRAY_SIZE(ti_sn_bridge_dp_rate_table))
>> +		i--; /* set to maximum possible */
>> +
>> +	val = ti_sn_bridge_dp_rate_table[i].bit_val << 
>> SN_DP_DATA_RATE_OFFSET;
>> +	regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>> +						SN_DP_DATA_RATE_BITS, val);
> 
> I think this can be simplified.
> 
> /* Array index corresponds to register value */
> static const unsigned int ti_sn_bridge_dp_rate_lut[] = {
>         0, 1620, 2160, 2430, 2700, 3240, 4320, 5400
> };
> 
> static void ti_sn_bridge_set_dsi_dp_rate(struct ti_sn_bridge *pdata)
> {
>         ...
>         for (i = ARRAY_SIZE(ti_sn_bridge_dp_rate_lut) - 1; i >= 0; i--) 
> {
>                 if (ti_sn_bridge_dp_rate_lut[i] <= dp_rate_mhz)
>                         break;
>         }
>         regmap_update_bits(pdata->regmap, SN_DATARATE_CONFIG_REG,
>                            SN_DP_DATA_RATE_BITS, i << 
> SN_DP_DATA_RATE_OFFSET);
> }
> 
As per datasheet we have to program the dp data rate greater than stream 
rate.
In order to support the panel stream bit rate, the SN65DSI86 eDP 
interface must be programmed so that the total
eDP data rate is greater than the stream bit rate.
>> +}
>> +
>> +static void ti_sn_bridge_set_video_timings(struct ti_sn_bridge 
>> *pdata)
>> +{
>> +	struct drm_display_mode *mode = &pdata->curr_mode;
>> +
>> +	regmap_write(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_LOW_REG,
>> +			mode->hdisplay & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_ACTIVE_LINE_LENGTH_HIGH_REG,
>> +			(mode->hdisplay >> SN_TIMING_HIGH_OFFSET) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_LOW_REG,
>> +			mode->vdisplay & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_DISPLAY_SIZE_HIGH_REG,
>> +			(mode->vdisplay >> SN_TIMING_HIGH_OFFSET) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_LOW_REG,
>> +			(mode->hsync_end - mode->hsync_start) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_HSYNC_PULSE_WIDTH_HIGH_REG,
>> +			((mode->hsync_end - mode->hsync_start) >>
>> +			 SN_TIMING_HIGH_OFFSET) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_LOW_REG,
>> +			(mode->vsync_end - mode->vsync_start) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VSYNC_PULSE_WIDTH_HIGH_REG,
>> +			((mode->vsync_end - mode->vsync_start) >>
>> +			 SN_TIMING_HIGH_OFFSET) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_BACK_PORCH_REG,
>> +			(mode->htotal - mode->hsync_end) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_BACK_PORCH_REG,
>> +			(mode->vtotal - mode->vsync_end) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_HORIZONTAL_FRONT_PORCH_REG,
>> +			(mode->hsync_start - mode->hdisplay) & 0xFF);
>> +	regmap_write(pdata->regmap, SN_CHA_VERTICAL_FRONT_PORCH_REG,
>> +			(mode->vsync_start - mode->vdisplay) & 0xFF);
>> +	usleep_range(10000, 10050); /* 10ms delay recommended by spec */
> 
> You would probably be fine to set the upper bound to 10500 so we don't 
> trigger
> an unnecessary interrupt.
> 
Done.
>> +}
>> +
>> +static void ti_sn_bridge_enable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +	struct drm_panel *panel = pdata->panel;
>> +	unsigned int val = 0;
>> +
>> +	if (panel) {
>> +		drm_panel_prepare(panel);
>> +		/* in case drm_panel is connected then HPD is not supported */
>> +		regmap_update_bits(pdata->regmap, SN_HPD_DISABLE_REG,
>> +				   SN_HPD_DISABLE_BIT, SN_HPD_DISABLE_BIT);
>> +	}
>> +
>> +	/* DSI_A lane config */
>> +	val = (4 - pdata->dsi->lanes) << SN_DSIA_LANE_OFFSET;
>> +	regmap_update_bits(pdata->regmap, SN_DSI_LANES_REG,
>> +					SN_DSIA_NUM_LANES_BITS, val);
>> +
>> +	/* DP lane config */
>> +	val = (pdata->dsi->lanes - 1) << SN_DP_LANE_OFFSET;
>> +	regmap_update_bits(pdata->regmap, SN_SSC_CONFIG_REG,
>> +					SN_DP_NUM_LANES_BITS, val);
>> +
>> +	/* set dsi/dp clk frequency value */
>> +	ti_sn_bridge_set_dsi_dp_rate(pdata);
>> +
>> +	/* enable DP PLL */
>> +	regmap_write(pdata->regmap, SN_PLL_ENABLE_REG, 1);
>> +	usleep_range(10000, 10050); /* 10ms delay recommended by spec */
>> +
>> +	/**
>> +	 * The SN65DSI86 only supports ASSR Display Authentication method 
>> and
>> +	 * this method is enabled by default. An eDP panel must support this
>> +	 * authentication method. We need to enable this method in the eDP 
>> panel
>> +	 * at DisplayPort address 0x0010A prior to link training.
>> +	 */
>> +	regmap_write(pdata->regmap, SN_AUX_WDATA0_REG, 0x01);
>> +	regmap_write(pdata->regmap, SN_AUX_ADDR_19_16_REG, 0x00);
>> +	regmap_write(pdata->regmap, SN_AUX_ADDR_15_8_REG, 0x01);
>> +	regmap_write(pdata->regmap, SN_AUX_ADDR_7_0_REG, 0x0A);
>> +	regmap_write(pdata->regmap, SN_AUX_LENGTH_REG, 0x01);
>> +	regmap_write(pdata->regmap, SN_AUX_CMD_REG, 0x81);
>> +	usleep_range(10000, 10050); /* 10ms delay recommended by spec */
>> +
>> +	/* Semi auto link training mode */
>> +	regmap_write(pdata->regmap, SN_ML_TX_MODE_REG, 0x0A);
>> +	msleep(20); /* 20ms delay recommended by spec */
>> +
>> +	/* config video parameters */
>> +	ti_sn_bridge_set_video_timings(pdata);
>> +
>> +	/* enable video stream */
>> +	regmap_update_bits(pdata->regmap, SN_ENH_FRAME_REG,
>> +			   SN_ENABLE_VID_STREAM_BIT, SN_ENABLE_VID_STREAM_BIT);
>> +
>> +	if (panel)
>> +		drm_panel_enable(panel);
>> +}
>> +
>> +static void ti_sn_bridge_pre_enable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +
>> +	ti_sn_bridge_power_ctrl(pdata, true);
>> +
>> +	/* configure bridge CLK_SRC and ref_clk */
>> +	ti_sn_bridge_set_refclk(pdata);
>> +}
>> +
>> +static void ti_sn_bridge_post_disable(struct drm_bridge *bridge)
>> +{
>> +	struct ti_sn_bridge *pdata = bridge_to_ti_sn_bridge(bridge);
>> +
>> +	ti_sn_bridge_power_ctrl(pdata, false);
>> +}
>> +
>> +static const struct drm_bridge_funcs ti_sn_bridge_funcs = {
>> +	.attach = ti_sn_bridge_attach,
>> +	.pre_enable = ti_sn_bridge_pre_enable,
>> +	.enable = ti_sn_bridge_enable,
>> +	.disable = ti_sn_bridge_disable,
>> +	.post_disable = ti_sn_bridge_post_disable,
>> +	.mode_set = ti_sn_bridge_mode_set,
>> +};
>> +
>> +static int ti_sn_bridge_parse_dsi_host(struct ti_sn_bridge *pdata)
>> +{
>> +	struct device_node *np = pdata->dev->of_node;
>> +	struct device_node *end_node;
>> +
>> +	end_node = of_graph_get_endpoint_by_regs(np, 0, 0);
>> +	if (!end_node) {
>> +		DRM_ERROR("remote endpoint not found\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	pdata->host_node = of_graph_get_remote_port_parent(end_node);
>> +	of_node_put(end_node);
>> +	if (!pdata->host_node) {
>> +		DRM_ERROR("remote node not found\n");
>> +		return -ENODEV;
>> +	}
>> +	of_node_put(pdata->host_node);
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_sn_bridge_probe(struct i2c_client *client,
>> +	 const struct i2c_device_id *id)
>> +{
>> +	struct ti_sn_bridge *pdata;
>> +	struct device_node *ddc_node;
>> +	int ret = 0;
>> +
>> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
>> +		DRM_ERROR("device doesn't support I2C\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	pdata = devm_kzalloc(&client->dev,
>> +		sizeof(struct ti_sn_bridge), GFP_KERNEL);
> 
>         pdata = devm_kzalloc(&client->dev, sizeof(struct ti_sn_bridge),
>                              GFP_KERNEL);
> 
>> +	if (!pdata)
>> +		return -ENOMEM;
>> +
>> +	pdata->dev = &client->dev;
>> +	DRM_DEBUG("I2C address is %x\n", client->addr);
> 
> unnecessary
> 
Removed
>> +
>> +	pdata->regmap = devm_regmap_init_i2c(client,
>> +				&ti_sn_bridge_regmap_config);
> 
> indentation is wrong here, should be:
> 
>         pdata->regmap = devm_regmap_init_i2c(client,
>                                              
> &ti_sn_bridge_regmap_config);
> 
>> +	if (IS_ERR(pdata->regmap))
> 
> Print an error?
> 
>> +		return PTR_ERR(pdata->regmap);
>> +
>> +	pdata->enable_gpio = devm_gpiod_get(pdata->dev,
>> +					    "enable", GPIOD_OUT_LOW);
>> +	if (IS_ERR(pdata->enable_gpio)) {
>> +		DRM_ERROR("failed to get enable gpio from DT\n");
>> +		ret = PTR_ERR(pdata->enable_gpio);
>> +		return ret;
>> +	}
>> +
>> +	ret = ti_sn_bridge_parse_regulators(pdata);
>> +	if (ret) {
>> +		DRM_ERROR("failed to parse regulators\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = of_property_read_u32(pdata->dev->of_node,
>> +			"refclk-freq-khz", &pdata->refclk_khz);
>> +	if (ret)
>> +		pdata->refclk_khz = SN_DEFAULT_REF_CLK_KHZ;
>> +
>> +	ret = ti_sn_bridge_parse_dsi_host(pdata);
>> +	if (ret)
> 
> DRM_ERROR?
> 
I am already printing error in ti_sn_bridge_parse_dsi_host() so removed 
from here as per comment from Jordan Crouse.
>> +		return ret;
>> +
>> +	ddc_node = of_parse_phandle(pdata->dev->of_node, "ddc-i2c-bus", 0);
>> +	if (ddc_node) {
>> +		pdata->ddc = of_find_i2c_adapter_by_node(ddc_node);
>> +		of_node_put(ddc_node);
>> +		if (!pdata->ddc) {
>> +			dev_dbg(pdata->dev, "failed to read ddc node\n");
>> +			return -EPROBE_DEFER;
>> +		}
>> +	} else {
>> +		dev_dbg(pdata->dev, "no ddc property found\n");
> 
> You've copied dev_dbg() logs, they should be DRM_DEBUG_KMS().
> 
Done.
>> +	}
>> +
>> +	ti_sn_bridge_power_ctrl(pdata, true);
>> +	ret = ti_sn_bridge_read_device_rev(pdata);
>> +	ti_sn_bridge_power_ctrl(pdata, false);
> 
> This could be done before handling refclk or ddc.
> 
Moved before refclk and ddc handing.
>> +	if (ret)
>> +		return ret;
>> +
>> +	i2c_set_clientdata(client, pdata);
>> +	mutex_init(&pdata->lock);
>> +
>> +	pdata->bridge.funcs = &ti_sn_bridge_funcs;
>> +	pdata->bridge.of_node = client->dev.of_node;
>> +
>> +	drm_bridge_add(&pdata->bridge);
>> +
>> +	DRM_DEBUG("bridge device registered successfully\n");
> 
> unnecessary
> 
Removed
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_sn_bridge_remove(struct i2c_client *client)
>> +{
>> +	struct ti_sn_bridge *pdata = i2c_get_clientdata(client);
>> +
>> +	if (!pdata)
>> +		return -EINVAL;
>> +
>> +	mipi_dsi_detach(pdata->dsi);
>> +	mipi_dsi_device_unregister(pdata->dsi);
>> +
>> +	drm_bridge_remove(&pdata->bridge);
>> +	i2c_put_adapter(pdata->ddc);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct i2c_device_id ti_sn_bridge_id[] = {
>> +	{ "ti,sn65dsi86", 0},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(i2c, ti_sn_bridge_id);
>> +
>> +static const struct of_device_id ti_sn_bridge_match_table[] = {
>> +	{.compatible = "ti,sn65dsi86"},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ti_sn_bridge_match_table);
>> +
>> +static struct i2c_driver ti_sn_bridge_driver = {
>> +	.driver = {
>> +		.name = "ti_sn65dsi86",
>> +		.of_match_table = ti_sn_bridge_match_table,
>> +	},
>> +	.probe = ti_sn_bridge_probe,
>> +	.remove = ti_sn_bridge_remove,
>> +	.id_table = ti_sn_bridge_id,
>> +};
>> +
>> +module_i2c_driver(ti_sn_bridge_driver);
>> +MODULE_DESCRIPTION("sn65dsi86 DSI to eDP bridge driver");
>> +MODULE_LICENSE("GPL v2");
>> --
>> 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

[PATCH v5 2/4] dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings

[Sandeep Panda]

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

Changes in v1:
 - Rephrase the dt-binding descriptions to be more inline with existing
   bindings (Andrzej Hajda).
 - Add missing dt-binding that are parsed by corresponding driver
   (Andrzej Hajda).

Changes in v2:
 - Remove edp panel specific dt-binding entries. Only keep bridge
   specific entries (Sean Paul).
 - Remove custom-modes dt entry since its usage is removed from driver also (Sean Paul).
 - Remove is-pluggable dt entry since this will not be needed anymore (Sean Paul).

Changes in v3:
 - Remove irq-gpio dt entry and instead populate is an interrupt
   property (Rob Herring).

Changes in v4:
 - Add link to bridge chip datasheet (Stephen Boyd)
 - Add vpll and vcc regulator supply bindings (Stephen Boyd)
 - Add ref clk optional dt binding (Stephen Boyd)
 - Add gpio-controller optional dt binding (Stephen Boyd)

Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/display/bridge/ti,sn65dsi86.txt       | 76 ++++++++++++++++++++++
 1 file changed, 76 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..0d042ce
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
@@ -0,0 +1,76 @@
+SN65DSI86 DSI to eDP bridge chip
+--------------------------------
+
+This is the binding for Texas Instruments SN65DSI86 bridge.
+http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
+
+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 bridge_en pin
+
+- vccio-supply: A 1.8V supply that powers up the digital IOs.
+- vpll-supply: A 1.8V supply that powers up the displayport PLL.
+- vcca-supply: A 1.2V supply that powers up the analog circuits.
+- vcc-supply: A 1.2V supply that powers up the digital core.
+
+Optional properties:
+- interrupts: Specifier for the SN65DSI86 interrupt line.
+- hpd-gpios: OF device-tree gpio specifications for HPD pin.
+
+- refclk-freq-khz: Value specifying the frequency of reference clock in KHz unit.
+
+- gpio-controller: Marks the device has a GPIO controller.
+- #gpio-cells: Number of GPIO cells. Refer to binding document "gpio/gpio.txt"
+- #pwm-cells : Number of cells used to specify a PWM. See pwm.txt in this directory
+	       for a description of the cell formats.
+
+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>;
+	interrupt-parent = <&gpio3>;
+	interrupts = <4 IRQ_TYPE_EDGE_FALLING>;
+
+	vccio-supply = <&pm8916_l17>;
+	vcca-supply = <&pm8916_l6>;
+	vpll-supply = <&pm8916_l17>;
+	vcc-supply = <&pm8916_l6>;
+
+	refclk-freq-khz = <19200>;
+
+	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: [PATCH v5 2/4] dt-bindings: drm/bridge: Document sn65dsi86 bridge bindings

[Stephen Boyd]

Quoting Sandeep Panda (2018-05-01 21:32:00)
> 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..0d042ce
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
> @@ -0,0 +1,76 @@
> +SN65DSI86 DSI to eDP bridge chip
> +--------------------------------
> +
> +This is the binding for Texas Instruments SN65DSI86 bridge.
> +http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
> +
> +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 bridge_en pin
> +
> +- vccio-supply: A 1.8V supply that powers up the digital IOs.
> +- vpll-supply: A 1.8V supply that powers up the displayport PLL.
> +- vcca-supply: A 1.2V supply that powers up the analog circuits.
> +- vcc-supply: A 1.2V supply that powers up the digital core.
> +
> +Optional properties:
> +- interrupts: Specifier for the SN65DSI86 interrupt line.
> +- hpd-gpios: OF device-tree gpio specifications for HPD pin.
> +
> +- refclk-freq-khz: Value specifying the frequency of reference clock in KHz unit.

Please use the clocks property instead. We may need to turn the clk on
first before this can work so the driver would use the clk framework (at
least in linux). clock-names could have 'refclk' because that's the pin
name.

Is there a way in DRM to figure out the frequency of the clock frequency
for DACP/N? It looks like if refclk is grounded, then the DACP/N pins
from the DSI side should be one of a set of frequencies, so I'm just
curious how that will work and if the binding would need to be updated
to indicate what the frequency of the DSI clock lane is, or if DRM can
tell this driver through the port/graph stuff somehow.

> +
> +- gpio-controller: Marks the device has a GPIO controller.
> +- #gpio-cells: Number of GPIO cells. Refer to binding document "gpio/gpio.txt"

What's the number? 2?

> +- #pwm-cells : Number of cells used to specify a PWM. See pwm.txt in this directory
> +              for a description of the cell formats.

What's the number? 1? There's a pwm.txt in this directory?

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

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

[spanda-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-05-02 22:31, Stephen Boyd wrote:
> Quoting Sandeep Panda (2018-05-01 21:32:00)
>> 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..0d042ce
>> --- /dev/null
>> +++ 
>> b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.txt
>> @@ -0,0 +1,76 @@
>> +SN65DSI86 DSI to eDP bridge chip
>> +--------------------------------
>> +
>> +This is the binding for Texas Instruments SN65DSI86 bridge.
>> +http://www.ti.com/general/docs/lit/getliterature.tsp?genericPartNumber=sn65dsi86&fileType=pdf
>> +
>> +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 bridge_en pin
>> +
>> +- vccio-supply: A 1.8V supply that powers up the digital IOs.
>> +- vpll-supply: A 1.8V supply that powers up the displayport PLL.
>> +- vcca-supply: A 1.2V supply that powers up the analog circuits.
>> +- vcc-supply: A 1.2V supply that powers up the digital core.
>> +
>> +Optional properties:
>> +- interrupts: Specifier for the SN65DSI86 interrupt line.
>> +- hpd-gpios: OF device-tree gpio specifications for HPD pin.
>> +
>> +- refclk-freq-khz: Value specifying the frequency of reference clock 
>> in KHz unit.
> 
> Please use the clocks property instead. We may need to turn the clk on
> first before this can work so the driver would use the clk framework 
> (at
> least in linux). clock-names could have 'refclk' because that's the pin
> name.
> 
> Is there a way in DRM to figure out the frequency of the clock 
> frequency
> for DACP/N? It looks like if refclk is grounded, then the DACP/N pins
> from the DSI side should be one of a set of frequencies, so I'm just
> curious how that will work and if the binding would need to be updated
> to indicate what the frequency of the DSI clock lane is, or if DRM can
> tell this driver through the port/graph stuff somehow.
> 

Can we do something like below?
    1. Add a required dt-property to indicate what is the source of 
refclk, ti,sn-refclk-src = <0> ---> means refclk is derived from refclk 
pin.
                                                                          
   ti,sn-refclk-src = <1> ---> means refclk is derived from DACP/N pin.
    2. Add a clock property to indicate the refclk frequency for refclk 
pin.
    3. In driver, parse the refclk source dt-property. If the source is 
refclk pin then get the frquency from clock dt-property and program the 
i2c register accordingly.
       Else if the source is DACP/N pin then calculate the DSIA frequency 
based on current display mode (by the time we go for configuring refclk, 
drm_mode_set is already done and in  diver we can calculate the 
frequency) and program the i2c register accordingly.

>> +
>> +- gpio-controller: Marks the device has a GPIO controller.
>> +- #gpio-cells: Number of GPIO cells. Refer to binding document 
>> "gpio/gpio.txt"
> 
> What's the number? 2?
number is 4, i will update this in binding
> 
>> +- #pwm-cells : Number of cells used to specify a PWM. See pwm.txt in 
>> this directory
>> +              for a description of the cell formats.
> 
> What's the number? 1? There's a pwm.txt in this directory?
yes number is 1, i will update the binding. it should be pwm/pwm.txt
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

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

[Stephen Boyd]

Quoting spanda@codeaurora.org (2018-05-03 02:41:29)
> On 2018-05-02 22:31, Stephen Boyd wrote:
> > Quoting Sandeep Panda (2018-05-01 21:32:00)
> >> 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..0d042ce
> > 
> > Please use the clocks property instead. We may need to turn the clk on
> > first before this can work so the driver would use the clk framework 
> > (at
> > least in linux). clock-names could have 'refclk' because that's the pin
> > name.
> > 
> > Is there a way in DRM to figure out the frequency of the clock 
> > frequency
> > for DACP/N? It looks like if refclk is grounded, then the DACP/N pins
> > from the DSI side should be one of a set of frequencies, so I'm just
> > curious how that will work and if the binding would need to be updated
> > to indicate what the frequency of the DSI clock lane is, or if DRM can
> > tell this driver through the port/graph stuff somehow.
> > 
> 
> Can we do something like below?
>     1. Add a required dt-property to indicate what is the source of 
> refclk, ti,sn-refclk-src = <0> ---> means refclk is derived from refclk 
> pin.
>                                                                           
>    ti,sn-refclk-src = <1> ---> means refclk is derived from DACP/N pin.
>     2. Add a clock property to indicate the refclk frequency for refclk 
> pin.
>     3. In driver, parse the refclk source dt-property. If the source is 
> refclk pin then get the frquency from clock dt-property and program the 
> i2c register accordingly.
>        Else if the source is DACP/N pin then calculate the DSIA frequency 
> based on current display mode (by the time we go for configuring refclk, 
> drm_mode_set is already done and in  diver we can calculate the 
> frequency) and program the i2c register accordingly.

The presence or non-presence of the refclk should still be indicated via
the standard clock property instead of some TI specific property. The
driver can try to clk_get() the refclk and if its there it can call
clk_get_rate() to figure out the reference clk frequency. It should also
turn it on with clk_prepare_enable() to make sure the clk is clocking
and turn it off when the driver isn't using it.

If the reference clk is recovered from the DACP/N pin then there won't
be a clocks property, and the driver can do what you describe in #3.

> 
> >> +
> >> +- gpio-controller: Marks the device has a GPIO controller.
> >> +- #gpio-cells: Number of GPIO cells. Refer to binding document 
> >> "gpio/gpio.txt"
> > 
> > What's the number? 2?
> number is 4, i will update this in binding

Really? What do you need 4 cells for? The number of cells doesn't
indicate the number of GPIOs on the device.

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

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

[spanda-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-05-08 03:48, Stephen Boyd wrote:
> Quoting spanda@codeaurora.org (2018-05-03 02:41:29)
>> On 2018-05-02 22:31, Stephen Boyd wrote:
>> > Quoting Sandeep Panda (2018-05-01 21:32:00)
>> >> 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..0d042ce
>> >
>> > Please use the clocks property instead. We may need to turn the clk on
>> > first before this can work so the driver would use the clk framework
>> > (at
>> > least in linux). clock-names could have 'refclk' because that's the pin
>> > name.
>> >
>> > Is there a way in DRM to figure out the frequency of the clock
>> > frequency
>> > for DACP/N? It looks like if refclk is grounded, then the DACP/N pins
>> > from the DSI side should be one of a set of frequencies, so I'm just
>> > curious how that will work and if the binding would need to be updated
>> > to indicate what the frequency of the DSI clock lane is, or if DRM can
>> > tell this driver through the port/graph stuff somehow.
>> >
>> 
>> Can we do something like below?
>>     1. Add a required dt-property to indicate what is the source of
>> refclk, ti,sn-refclk-src = <0> ---> means refclk is derived from 
>> refclk
>> pin.
>> 
>>    ti,sn-refclk-src = <1> ---> means refclk is derived from DACP/N 
>> pin.
>>     2. Add a clock property to indicate the refclk frequency for 
>> refclk
>> pin.
>>     3. In driver, parse the refclk source dt-property. If the source 
>> is
>> refclk pin then get the frquency from clock dt-property and program 
>> the
>> i2c register accordingly.
>>        Else if the source is DACP/N pin then calculate the DSIA 
>> frequency
>> based on current display mode (by the time we go for configuring 
>> refclk,
>> drm_mode_set is already done and in  diver we can calculate the
>> frequency) and program the i2c register accordingly.
> 
> The presence or non-presence of the refclk should still be indicated 
> via
> the standard clock property instead of some TI specific property. The
> driver can try to clk_get() the refclk and if its there it can call
> clk_get_rate() to figure out the reference clk frequency. It should 
> also
> turn it on with clk_prepare_enable() to make sure the clk is clocking
> and turn it off when the driver isn't using it.
> 
> If the reference clk is recovered from the DACP/N pin then there won't
> be a clocks property, and the driver can do what you describe in #3.
> 
>> 
>> >> +
>> >> +- gpio-controller: Marks the device has a GPIO controller.
>> >> +- #gpio-cells: Number of GPIO cells. Refer to binding document
>> >> "gpio/gpio.txt"
>> >
>> > What's the number? 2?
>> number is 4, i will update this in binding
> 
> Really? What do you need 4 cells for? The number of cells doesn't
> indicate the number of GPIOs on the device.

It should be 2, got confused with number GPIOs.
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno

[PATCH v5 3/4] drm/panel: add Innolux TV123WAM panel driver support

[Sandeep Panda]

Add support for Innolux TV123WAM, which is a 12.3" eDP
display panel with 2160x1440 resolution.

Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
---
 drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 234af81..52bbcfb 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -1939,6 +1939,30 @@ static void panel_simple_shutdown(struct device *dev)
 	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
 };
 
+static const struct drm_display_mode innolux_tv123wam_mode = {
+	.clock = 206016,
+	.hdisplay = 2160,
+	.hsync_start = 2160 + 48,
+	.hsync_end = 2160 + 48 + 32,
+	.htotal = 2160 + 48 + 32 + 80,
+	.vdisplay = 1440,
+	.vsync_start = 1440 + 3,
+	.vsync_end = 1440 + 3 + 10,
+	.vtotal = 1440 + 3 + 10 + 27,
+	.vrefresh = 60,
+	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
+};
+
+static const struct panel_desc innolux_tv123wam = {
+	.modes = &innolux_tv123wam_mode,
+	.num_modes = 1,
+	.bpc = 8,
+	.size = {
+		.width = 259,
+		.height = 173,
+	},
+};
+
 static const struct of_device_id platform_of_match[] = {
 	{
 		.compatible = "ampire,am-480272h3tmqw-t01h",
@@ -2142,6 +2166,9 @@ static void panel_simple_shutdown(struct device *dev)
 		.compatible = "winstar,wf35ltiacd",
 		.data = &winstar_wf35ltiacd,
 	}, {
+		.compatible = "innolux,tv123wam",
+		.data = &innolux_tv123wam,
+	}, {
 		/* sentinel */
 	}
 };
-- 
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: [PATCH v5 3/4] drm/panel: add Innolux TV123WAM panel driver support

[Sean Paul]

On Wed, May 02, 2018 at 10:02:01AM +0530, Sandeep Panda wrote:
> Add support for Innolux TV123WAM, which is a 12.3" eDP
> display panel with 2160x1440 resolution.
> 
> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 234af81..52bbcfb 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -1939,6 +1939,30 @@ static void panel_simple_shutdown(struct device *dev)
>  	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
>  };
>  
> +static const struct drm_display_mode innolux_tv123wam_mode = {
> +	.clock = 206016,
> +	.hdisplay = 2160,
> +	.hsync_start = 2160 + 48,
> +	.hsync_end = 2160 + 48 + 32,
> +	.htotal = 2160 + 48 + 32 + 80,
> +	.vdisplay = 1440,
> +	.vsync_start = 1440 + 3,
> +	.vsync_end = 1440 + 3 + 10,
> +	.vtotal = 1440 + 3 + 10 + 27,
> +	.vrefresh = 60,
> +	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
> +};
> +
> +static const struct panel_desc innolux_tv123wam = {
> +	.modes = &innolux_tv123wam_mode,
> +	.num_modes = 1,
> +	.bpc = 8,
> +	.size = {
> +		.width = 259,
> +		.height = 173,
> +	},
> +};
> +
>  static const struct of_device_id platform_of_match[] = {
>  	{
>  		.compatible = "ampire,am-480272h3tmqw-t01h",
> @@ -2142,6 +2166,9 @@ static void panel_simple_shutdown(struct device *dev)
>  		.compatible = "winstar,wf35ltiacd",
>  		.data = &winstar_wf35ltiacd,
>  	}, {
> +		.compatible = "innolux,tv123wam",
> +		.data = &innolux_tv123wam,
> +	}, {

These are alphabetically ordered, as are the drm_display_mode/panel_desc
structs.

Sean

>  		/* sentinel */
>  	}
>  };
> -- 
> 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: [PATCH v5 3/4] drm/panel: add Innolux TV123WAM panel driver support

[spanda-sgV2jX0FEOL9JmXXK+q4OQ]

On 2018-05-03 00:35, Sean Paul wrote:
> On Wed, May 02, 2018 at 10:02:01AM +0530, Sandeep Panda wrote:
>> Add support for Innolux TV123WAM, which is a 12.3" eDP
>> display panel with 2160x1440 resolution.
>> 
>> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
>> ---
>>  drivers/gpu/drm/panel/panel-simple.c | 27 +++++++++++++++++++++++++++
>>  1 file changed, 27 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
>> b/drivers/gpu/drm/panel/panel-simple.c
>> index 234af81..52bbcfb 100644
>> --- a/drivers/gpu/drm/panel/panel-simple.c
>> +++ b/drivers/gpu/drm/panel/panel-simple.c
>> @@ -1939,6 +1939,30 @@ static void panel_simple_shutdown(struct device 
>> *dev)
>>  	.bus_format = MEDIA_BUS_FMT_RGB888_1X24,
>>  };
>> 
>> +static const struct drm_display_mode innolux_tv123wam_mode = {
>> +	.clock = 206016,
>> +	.hdisplay = 2160,
>> +	.hsync_start = 2160 + 48,
>> +	.hsync_end = 2160 + 48 + 32,
>> +	.htotal = 2160 + 48 + 32 + 80,
>> +	.vdisplay = 1440,
>> +	.vsync_start = 1440 + 3,
>> +	.vsync_end = 1440 + 3 + 10,
>> +	.vtotal = 1440 + 3 + 10 + 27,
>> +	.vrefresh = 60,
>> +	.flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC,
>> +};
>> +
>> +static const struct panel_desc innolux_tv123wam = {
>> +	.modes = &innolux_tv123wam_mode,
>> +	.num_modes = 1,
>> +	.bpc = 8,
>> +	.size = {
>> +		.width = 259,
>> +		.height = 173,
>> +	},
>> +};
>> +
>>  static const struct of_device_id platform_of_match[] = {
>>  	{
>>  		.compatible = "ampire,am-480272h3tmqw-t01h",
>> @@ -2142,6 +2166,9 @@ static void panel_simple_shutdown(struct device 
>> *dev)
>>  		.compatible = "winstar,wf35ltiacd",
>>  		.data = &winstar_wf35ltiacd,
>>  	}, {
>> +		.compatible = "innolux,tv123wam",
>> +		.data = &innolux_tv123wam,
>> +	}, {
> 
> These are alphabetically ordered, as are the 
> drm_display_mode/panel_desc
> structs.
> 
> Sean
> 
Done.
>>  		/* sentinel */
>>  	}
>>  };
>> --
>> 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

[PATCH v5 4/4] dt-bindings: drm/panel: Document Innolux TV123WAM panel bindings

[Sandeep Panda]

Innolux TV123WAM is a 12.3" eDP display panel with
2160x1440 resolution, which can be supported by simple
panel driver.

Changes in v1:
 - Make use of simple panel driver instead of creating
   a new driver for this panel (Sean Paul).
 - Combine dt-binding and driver changes into one patch
   as done by other existing panel support changes.

Changes in v2:
 - Separate driver change from dt-binding documentation (Rob Herring).
 - Add the properties from simple-panel binding that are applicable to
   this panel (Rob Herring).

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

diff --git a/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt b/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
new file mode 100644
index 0000000..a9b3526
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt
@@ -0,0 +1,20 @@
+Innolux TV123WAM 12.3 inch eDP 2K display panel
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.
+
+Required properties:
+- compatible: should be "innolux,tv123wam"
+- power-supply: regulator to provide the supply voltage
+
+Optional properties:
+- enable-gpios: GPIO pin to enable or disable the panel
+- backlight: phandle of the backlight device attached to the panel
+
+Example:
+	panel_edp: panel-edp {
+		compatible = "innolux,tv123wam";
+		enable-gpios = <&msmgpio 31 GPIO_ACTIVE_LOW>;
+		power-supply = <&pm8916_l2>;
+		backlight = <&backlight>;
+	};
-- 
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: [PATCH v5 4/4] dt-bindings: drm/panel: Document Innolux TV123WAM panel bindings

[Rob Herring]

On Wed, May 02, 2018 at 10:02:02AM +0530, Sandeep Panda wrote:
> Innolux TV123WAM is a 12.3" eDP display panel with
> 2160x1440 resolution, which can be supported by simple
> panel driver.
> 
> Changes in v1:
>  - Make use of simple panel driver instead of creating
>    a new driver for this panel (Sean Paul).
>  - Combine dt-binding and driver changes into one patch
>    as done by other existing panel support changes.
> 
> Changes in v2:
>  - Separate driver change from dt-binding documentation (Rob Herring).
>  - Add the properties from simple-panel binding that are applicable to
>    this panel (Rob Herring).
> 
> Signed-off-by: Sandeep Panda <spanda@codeaurora.org>
> ---
>  .../bindings/display/panel/innolux,tv123wam.txt      | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/innolux,tv123wam.txt

Reviewed-by: Rob Herring <robh@kernel.org>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel