Re: [PATCH 3/3] drm/bridge: Add ITE IT6251 bridge driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

On 10/17/2016 10:03 PM, Marek Vasut wrote:
Add driver for the ITE IT6251 LVDS-to-eDP bridge.

Signed-off-by: Marek Vasut <marex@xxxxxxx>
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Cc: Sean Cross <xobs@xxxxxxxxxx>
---
 drivers/gpu/drm/bridge/Kconfig      |   9 +
 drivers/gpu/drm/bridge/Makefile     |   1 +
 drivers/gpu/drm/bridge/ite-it6251.c | 606 ++++++++++++++++++++++++++++++++++++
 3 files changed, 616 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/ite-it6251.c

diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
index 10e12e7..e9c96b9 100644
--- a/drivers/gpu/drm/bridge/Kconfig
+++ b/drivers/gpu/drm/bridge/Kconfig
@@ -39,6 +39,15 @@ config DRM_DW_HDMI_AHB_AUDIO
 	  Designware HDMI block.  This is used in conjunction with
 	  the i.MX6 HDMI driver.

+config DRM_ITE_IT6251
+	tristate "ITE IT6251 LVDS/eDP bridge"
+	depends on OF
+	select DRM_KMS_HELPER
+	select DRM_PANEL
+	select REGMAP_I2C
+	---help---
+	  ITE IT6251 LVDS-eDP bridge chip driver.
+
 config DRM_NXP_PTN3460
 	tristate "NXP PTN3460 DP/LVDS bridge"
 	depends on OF
diff --git a/drivers/gpu/drm/bridge/Makefile b/drivers/gpu/drm/bridge/Makefile
index cdf3a3c..736dba7 100644
--- a/drivers/gpu/drm/bridge/Makefile
+++ b/drivers/gpu/drm/bridge/Makefile
@@ -4,6 +4,7 @@ obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o
 obj-$(CONFIG_DRM_DUMB_VGA_DAC) += dumb-vga-dac.o
 obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o
 obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o
+obj-$(CONFIG_DRM_ITE_IT6251) += ite-it6251.o
 obj-$(CONFIG_DRM_NXP_PTN3460) += nxp-ptn3460.o
 obj-$(CONFIG_DRM_PARADE_PS8622) += parade-ps8622.o
 obj-$(CONFIG_DRM_SII902X) += sii902x.o
diff --git a/drivers/gpu/drm/bridge/ite-it6251.c b/drivers/gpu/drm/bridge/ite-it6251.c
new file mode 100644
index 0000000..a19bb4d
--- /dev/null
+++ b/drivers/gpu/drm/bridge/ite-it6251.c
@@ -0,0 +1,606 @@
+/*

We're trying to mention the bridge name, and what encoding it does for all bridge
drivers. Could you describe it here too? Thanks.

+ * Copyright (C) 2014 Sean Cross <xobs@xxxxxxxxxx>
+ *
+ * Rework for mainline: Marek Vasut <marex@xxxxxxx>
+ *
+ * This software is licensed under the terms of the GNU General Public
+ * License version 2, as published by the Free Software Foundation, and
+ * may be copied, distributed, and modified under those terms.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/of_gpio.h>

This shouldn't be needed.

+#include <linux/of_graph.h>
+#include <linux/of_platform.h>

This too.

+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <drm/drm_panel.h>
+
+#include "drmP.h"
+#include "drm_crtc.h"
+#include "drm_crtc_helper.h"
+#include "drm_atomic_helper.h"
+
+struct it6251_bridge {
+	struct i2c_client	*client;
+	struct i2c_client	*lvds_client;
+	struct regmap		*regmap;
+	struct regmap		*lvds_regmap;
+	struct regulator	*regulator;
+
+	struct drm_connector	connector;
+	struct drm_bridge	bridge;
+	struct drm_panel	*panel;
+};
+
+/* Register definitions */
+#define IT6251_VENDOR_ID_LOW			0x00
+#define IT6251_VENDOR_ID_HIGH			0x01
+#define IT6251_DEVICE_ID_LOW			0x02
+#define IT6251_DEVICE_ID_HIGH			0x03
+#define IT6251_SYSTEM_STATUS			0x0d
+#define IT6251_SYSTEM_STATUS_RINTSTATUS		BIT(0)
+#define IT6251_SYSTEM_STATUS_RHPDSTATUS		BIT(1)
+#define IT6251_SYSTEM_STATUS_RVIDEOSTABLE	BIT(2)
+#define IT6251_SYSTEM_STATUS_RPLL_IOLOCK	BIT(3)
+#define IT6251_SYSTEM_STATUS_RPLL_XPLOCK	BIT(4)
+#define IT6251_SYSTEM_STATUS_RPLL_SPLOCK	BIT(5)
+#define IT6251_SYSTEM_STATUS_RAUXFREQ_LOCK	BIT(6)
+#define IT6251_REF_STATE			0x0e
+#define IT6251_REF_STATE_MAIN_LINK_DISABLED	BIT(0)
+#define IT6251_REF_STATE_AUX_CHANNEL_READ	BIT(1)
+#define IT6251_REF_STATE_CR_PATTERN		BIT(2)
+#define IT6251_REF_STATE_EQ_PATTERN		BIT(3)
+#define IT6251_REF_STATE_NORMAL_OPERATION	BIT(4)
+#define IT6251_REF_STATE_MUTED			BIT(5)
+#define IT6251_RPCLK_CNT_LOW			0x13
+#define IT6251_RPCLK_CNT_HIGH			0x14
+#define IT6251_RPC_REQ				0x2b
+#define IT6251_RPC_REQ_RPC_FIFOFULL		BIT(6)
+#define IT6251_RPC_REQ_RPC_FIFOEMPTY		BIT(7)
+#define IT6251_PCLK_CNT_LOW			0x57
+#define IT6251_PCLK_CNT_HIGH			0x58
+#define IT6251_DPHDEW_LOW			0xa5
+#define IT6251_DPHDEW_HIGH			0xa6
+#define IT6251_DPVDEW_LOW			0xaf
+#define IT6251_DPVDEW_HIGH			0xb0
+#define IT6251_LVDS_PORT_ADDR			0xfd
+#define IT6251_LVDS_PORT_CTRL			0xfe
+#define IT6251_LVDS_PORT_CTRL_EN		BIT(0)
+
+/*
+ * Register programming sequences.
+ * NOTE: There is a lot of registers here which are completely undocumented
+ *       and/or their meaning is not clear from the little documentation
+ *       that is available for this chip. These values below just seem to
+ *       work well enough.

Are these configs valid for different pixel clock rates?

+ */
+static const struct reg_sequence it6251_lvds_rx_sequence[] = {
+	{ 0x05, 0x00 },
+
+	{ 0x3b, 0x42 },	/* reset LVDSRX PLL */
+	{ 0x3b, 0x43 },
+
+	{ 0x3c, 0x08 },	/* something with SSC PLL */
+	{ 0x0b, 0x88 },	/* don't swap links, writing reserved regs */
+
+	{ 0x2c, 0x01 },	/* JEIDA, 8-bit depth 0x11, original 0x42 */
+	{ 0x32, 0x04 },	/* "reserved" */
+	{ 0x35, 0xe0 },	/* "reserved" */
+	{ 0x2b, 0x24 },	/* "reserved" + clock delay */
+
+	{ 0x05, 0x02 },	/* reset LVDSRX pix clock */
+	{ 0x05, 0x00 },
+};
+
+static const struct reg_sequence it6251_edp_tx_sequence[] = {
+	/* two lane mode, normal operation, no swapping, no downspread */
+	{ 0x16, 0x02 },
+	{ 0x23, 0x40 },	/* some AUX channel EDID magic */
+	{ 0x5c, 0xf3 },	/* power down lanes 3-0 */
+	{ 0x5f, 0x06 },	/* enable DP scrambling, change EQ CR phase */
+	{ 0x60, 0x02 },	/* color mode RGB, pclk/2 */
+	{ 0x61, 0x04 },	/* dual pixel input mode, no EO swap, no RGB swap */
+	{ 0x62, 0x01 },	/* M444B24 video format */
+
+	/* vesa range / not interlace / vsync high / hsync high */
+	{ 0xa0, 0x0F },
+
+	{ 0xc9, 0xf5 },	/* hpd event timer set to 1.6-ish ms */
+
+	{ 0xca, 0x4d },	/* more reserved magic */
+	{ 0xcb, 0x37 },
+
+	/* enhanced framing mode, auto video fifo reset, video mute disable */
+	{ 0xd3, 0x03 },
+	{ 0xd4, 0x45 },	/* "vidstmp" and some reserved stuff */
+
+	{ 0xe7, 0xa0 },	/* queue number -- reserved */
+	{ 0xe8, 0x33 },	/* info frame packets and reserved */
+	{ 0xec, 0x00 },	/* more AVI stuff */
+
+	{ 0x23, 0x42 },	/* select PC master reg for aux channel? */
+
+	{ 0x24, 0x00 },	/* send PC request commands */
+	{ 0x25, 0x00 },
+	{ 0x26, 0x00 },
+
+	{ 0x2b, 0x00 },	/* native aux read */
+	{ 0x23, 0x40 },	/* back to internal */
+
+	{ 0x19, 0xff },	/* voltage swing level 3 */
+	{ 0x1a, 0xff },	/* pre-emphasis level 3 */
+
+	{ 0x17, 0x01 },	/* start link training */

Have you tried seeing if these map to the drm_dp helpers? We should ideally
set up an aux channel to configure some of the DPCD params on the eDP panel.
You could have a look at the TC358767 driver for reference.

+};
+
+static struct it6251_bridge *bridge_to_it6251(struct drm_bridge *bridge)
+{
+	return container_of(bridge, struct it6251_bridge, bridge);
+}
+
+static struct it6251_bridge *conn_to_it6251(struct drm_connector *connector)
+{
+	return container_of(connector, struct it6251_bridge, connector);
+}
+
+static int it6251_is_stable(struct it6251_bridge *it6251)
+{
+	struct drm_display_mode *mode;
+	unsigned int status, rpclkcnt, clkcnt, refstate, rpcreq;
+	u8 regs[2];
+	u16 hactive;
+	u16 vactive;
+	int ret;
+
+	ret = regmap_read(it6251->regmap, IT6251_SYSTEM_STATUS, &status);
+	if (ret)
+		return ret;
+	dev_dbg(&it6251->client->dev, "System status: 0x%02x\n", status);
+
+	if (!(status & IT6251_SYSTEM_STATUS_RVIDEOSTABLE))
+		return -EINVAL;
+
+	ret = regmap_bulk_read(it6251->regmap, IT6251_RPCLK_CNT_LOW, regs, 2);
+	if (ret)
+		return ret;
+	rpclkcnt = (regs[0] & 0xff) | ((regs[1] & 0x0f) << 8);
+	dev_dbg(&it6251->client->dev, "RPCLKCnt: %d\n", rpclkcnt);
+
+	ret = regmap_bulk_read(it6251->lvds_regmap, IT6251_PCLK_CNT_LOW,
+			       regs, 2);
+	if (ret)
+		return ret;
+	clkcnt = (regs[0] & 0xff) | ((regs[1] & 0x0f) << 8);
+	dev_dbg(&it6251->client->dev, "Clock: 0x%02x\n", clkcnt);
+
+	ret = regmap_read(it6251->lvds_regmap, IT6251_REF_STATE, &refstate);
+	if (ret)
+		return ret;
+	dev_dbg(&it6251->client->dev, "Ref Link State: 0x%02x\n", refstate);
+
+	ret = regmap_read(it6251->lvds_regmap, IT6251_RPC_REQ, &rpcreq);
+	if (ret)
+		return ret;
+	dev_dbg(&it6251->client->dev, "RPC Req: 0x%02x\n", rpcreq);
+
+	ret = regmap_bulk_read(it6251->regmap, IT6251_DPHDEW_LOW, regs, 2);
+	if (ret)
+		return ret;
+	hactive = (regs[0] & 0xff) | ((regs[1] & 0x1f) << 8);
+	dev_dbg(&it6251->client->dev, "hactive: %d\n", hactive);
+
+	ret = regmap_bulk_read(it6251->regmap, IT6251_DPVDEW_LOW, regs, 2);
+	if (ret)
+		return ret;
+	vactive = (regs[0] & 0xff) | ((regs[1] & 0x0f) << 8);
+	dev_dbg(&it6251->client->dev, "vactive: %d\n", vactive);
+
+	if ((refstate & 0x1f) != 0)
+		return -EINVAL;
+
+	if (rpcreq & IT6251_RPC_REQ_RPC_FIFOFULL) {
+		dev_err(&it6251->client->dev,
+			"RPC fifofull is set, might be an error\n");
+		return -EINVAL;
+	}
+
+	/* If video is muted, that's a failure */
+	if (refstate & IT6251_REF_STATE_MUTED)
+		return -EINVAL;
+
+	list_for_each_entry(mode, &it6251->panel->connector->modes, head)
+		if ((mode->hdisplay == hactive) && (mode->vdisplay == vactive))
+			return 0;

Shouldn't the hactive and vactive registers be something that's configured
by the driver? It seems wrong to read off the registers and compare it
with a mode that we're trying to set in the first place.

Can you consider using the bridge mode_set and mode_fixup ops to set the
modes?

+
+	dev_info(&it6251->client->dev, "no mode match found\n");
+	return -EINVAL;
+}
+
+static int it6251_init(struct it6251_bridge *it6251)
+{
+	const struct reg_sequence it6251_reset_reg_sequence[] = {
+		{ 0x05, 0x00 },
+		{ IT6251_LVDS_PORT_ADDR, it6251->lvds_client->addr << 1 },
+		{ IT6251_LVDS_PORT_CTRL, IT6251_LVDS_PORT_CTRL_EN },
+	};
+
+	int ret, stable_delays;
+	unsigned int reg;
+
+	/*
+	 * Reset DisplayPort half. Setting bit 2 causes IT6251 to not
+	 * respond over i2c, which is considered "normal". This write
+	 * will report failure, but will actually succeed.
+	 */
+	regmap_write(it6251->regmap, 0x05, 0xff);
+
+	/* Un-reset DisplayPort half and configure LVDS receiver. */
+	ret = regmap_multi_reg_write(it6251->regmap, it6251_reset_reg_sequence,
+				     ARRAY_SIZE(it6251_reset_reg_sequence));
+	if (ret) {
+		dev_err(&it6251->client->dev, "cannot setup eDP half\n");
+		return ret;
+	}
+
+	/* LVDS RX */
+	regmap_write(it6251->lvds_regmap, 0x05, 0xff);
+	ret = regmap_multi_reg_write(it6251->lvds_regmap,
+				     it6251_lvds_rx_sequence,
+				     ARRAY_SIZE(it6251_lvds_rx_sequence));
+	if (ret) {
+		dev_err(&it6251->lvds_client->dev, "cannot setup LVDS RX\n");
+		return ret;
+	}
+
+	/* eDP TX */
+	ret = regmap_multi_reg_write(it6251->regmap,
+				     it6251_edp_tx_sequence,
+				     ARRAY_SIZE(it6251_edp_tx_sequence));
+	if (ret) {
+		dev_err(&it6251->client->dev, "cannot setup eDP TX\n");
+		return ret;
+	}
+
+	for (stable_delays = 0; stable_delays < 100; stable_delays++) {
+		ret = regmap_read(it6251->regmap, 0x0e, &reg);
+		if (ret || ((reg & 0x1f) != 0x10)) {
+			mdelay(2);

We aren't in an atomic context here, we could use usleep_range here.

+			continue;
+		}
+
+		ret = regmap_read(it6251->regmap, IT6251_SYSTEM_STATUS, &reg);
+		if (ret || !(reg & IT6251_SYSTEM_STATUS_RVIDEOSTABLE)) {
+			mdelay(2);
+			continue;
+		}
+
+		break;
+	}
+
+	/*
+	 * If we couldn't stabilize, requeue and try again, because it means
+	 * that the LVDS channel isn't stable yet.
+	 */
+	ret = it6251_is_stable(it6251);
+	if (ret)
+		dev_err(&it6251->client->dev, "bridge is not stable\n");
+
+	return ret;
+}
+
+static int it6251_power_down(struct it6251_bridge *it6251)
+{
+	struct device *dev = &it6251->client->dev;
+	int ret = 0;
+
+	if (regulator_is_enabled(it6251->regulator)) {
+		ret = regulator_disable(it6251->regulator);
+		if (ret)
+			dev_err(dev, "unable to disable regulator\n");
+	}
+
+	return ret;
+}
+
+static int it6251_power_up(struct it6251_bridge *it6251)
+{
+	struct i2c_client *client = it6251->client;
+	u8 regs[4];
+	int i, ret;
+
+	ret = regulator_enable(it6251->regulator);
+	if (ret) {
+		dev_err(&client->dev, "unable to enable regulator\n");
+		return ret;
+	}
+
+	/* Sometimes it seems like multiple tries are needed */
+	for (i = 0; i < 5; i++) {
+		ret = regmap_bulk_read(it6251->regmap, IT6251_VENDOR_ID_LOW,
+				       regs, 4);
+		if (!ret && regs[0] && regs[1] && regs[2] && regs[3]) {
+			dev_info(&client->dev, "found ITE6251 [%04x:%04x]\n",
+				 (regs[1] << 8) | regs[0],
+				 (regs[3] << 8) | regs[2]);
+			return 0;
+		}
+
+		usleep_range(100000, 200000);

Was this intended to be 100ms to 200ms? If so, msleep should be used.

+	}
+
+	dev_err(&client->dev, "unable to read product id\n");
+	it6251_power_down(it6251);
+	return -EINVAL;
+}
+
+/* I2C driver functions */
+static void it6251_pre_enable(struct drm_bridge *bridge)
+{
+	struct it6251_bridge *it6251 = bridge_to_it6251(bridge);
+
+	if (drm_panel_prepare(it6251->panel)) {
+		DRM_ERROR("failed to prepare panel\n");
+		return;
+	}
+
+	it6251_power_up(it6251);
+}
+
+static void it6251_enable(struct drm_bridge *bridge)
+{
+	struct it6251_bridge *it6251 = bridge_to_it6251(bridge);
+	int tries, ret;
+
+	if (drm_panel_enable(it6251->panel)) {
+		DRM_ERROR("failed to enable panel\n");
+		return;
+	}
+
+	for (tries = 0; tries < 5; tries++) {
+		ret = it6251_init(it6251);
+		if (!ret)
+			return;
+
+		/* If the init failed, restart the chip */
+		it6251_power_down(it6251);
+		it6251_power_up(it6251);
+	}
+}
+
+static void it6251_disable(struct drm_bridge *bridge)
+{
+	struct it6251_bridge *it6251 = bridge_to_it6251(bridge);
+
+	if (drm_panel_disable(it6251->panel))
+		DRM_ERROR("failed to disable panel\n");
+}
+
+static void it6251_post_disable(struct drm_bridge *bridge)
+{
+	struct it6251_bridge *it6251 = bridge_to_it6251(bridge);
+
+	if (drm_panel_unprepare(it6251->panel))
+		DRM_ERROR("failed to unprepare panel\n");
+
+	it6251_power_down(it6251);
+}
+
+static int it6251_get_modes(struct drm_connector *connector)
+{
+	struct it6251_bridge *it6251 = conn_to_it6251(connector);
+
+	return drm_panel_get_modes(it6251->panel);
+}
+
+static const struct drm_connector_helper_funcs it6251_connector_helper_funcs = {
+	.get_modes	= it6251_get_modes,
+};
+
+static enum drm_connector_status it6251_detect(struct drm_connector *connector,
+					       bool force)
+{
+	return connector_status_connected;
+}
+
+static const struct drm_connector_funcs it6251_connector_funcs = {
+	.dpms			= drm_atomic_helper_connector_dpms,
+	.fill_modes		= drm_helper_probe_single_connector_modes,
+	.detect			= it6251_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 int it6251_attach(struct drm_bridge *bridge)
+{
+	struct it6251_bridge *it6251 = bridge_to_it6251(bridge);
+	int ret;
+
+	if (!bridge->encoder) {
+		DRM_ERROR("Parent encoder object not found");
+		return -ENODEV;
+	}
+
+	it6251->connector.polled = DRM_CONNECTOR_POLL_HPD;
+	ret = drm_connector_init(bridge->dev, &it6251->connector,
+				 &it6251_connector_funcs,
+				 DRM_MODE_CONNECTOR_eDP);
+	if (ret) {
+		DRM_ERROR("Failed to initialize connector with drm\n");
+		return ret;
+	}
+
+	drm_atomic_helper_connector_reset(&it6251->connector);
+	drm_connector_helper_add(&it6251->connector,
+				 &it6251_connector_helper_funcs);
+	drm_mode_connector_attach_encoder(&it6251->connector, bridge->encoder);
+
+	if (it6251->panel)
+		drm_panel_attach(it6251->panel, &it6251->connector);
+
+	drm_helper_hpd_irq_event(it6251->connector.dev);

The connector isn't registered at this point. So there isn't much benefit
of calling this here.

+
+	return 0;
+}
+
+static const struct drm_bridge_funcs it6251_bridge_funcs = {
+	.pre_enable	= it6251_pre_enable,
+	.enable		= it6251_enable,
+	.disable	= it6251_disable,
+	.post_disable	= it6251_post_disable,
+	.attach		= it6251_attach,
+};
+
+static const struct regmap_config it6251_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.max_register	= 0xff,
+	.cache_type	= REGCACHE_NONE,
+};
+
+static int
+it6251_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct it6251_bridge *it6251;
+	struct device_node *endpoint, *panel_node;
+	int ret;
+
+	it6251 = devm_kzalloc(dev, sizeof(*it6251), GFP_KERNEL);
+	if (!it6251)
+		return -ENOMEM;
+
+	endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);

There is an of_node_put() missing for this endpoint.

Thanks,
Archit

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux