All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-02 12:13 ` Fabrizio Castro
  0 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-02 12:13 UTC (permalink / raw)
  To: Archit Taneja, Andrzej Hajda, David Airlie, Peter Rosin,
	Wolfram Sang, Mark Brown
  Cc: Fabrizio Castro, Chris Paterson, Geert Uytterhoeven,
	Boris Brezillon, dri-devel, Biju Das, linux-renesas-soc,
	Simon Horman, Laurent Pinchart, linux-i2c

While adding SiI9022A support to the iwg23s board, it came
up that when the HDMI transmitter is in pass through mode the
device is not compliant with the I2C specification anymore,
as it requires a far bigger tbuf, due to a delay the HDMI
transmitter is adding when relaying the STOP condition on the
monitor i2c side of things.

When not providing an appropriate delay after the STOP condition
the i2c bus would get stuck. Also, any other traffic on the bus
while talking to the monitor may cause the transaction to fail
or even cause issues with the i2c bus as well.

I2c-gates seemed to reach consent as a possible way to address
these issues, and as such this patch is implementing a solution
based on that. Since others are clearly relying on the current
implementation of the driver, this patch won't require any DT
changes.

Since we don't want any interference during the DDC Bus
Request/Grant procedure and while talking to the monitor, we
have to use the adapter locking primitives rather than the
i2c-mux locking primitives.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
RFC->PATCH:
* Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments

Thank you guys

 drivers/gpu/drm/bridge/sii902x.c | 264 +++++++++++++++++++++++++++++----------
 1 file changed, 196 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index e59a135..4f9d9c7 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -1,4 +1,6 @@
 /*
+ * Copyright (C) 2018 Renesas Electronics
+ *
  * Copyright (C) 2016 Atmel
  *		      Bo Shen <voice.shen@atmel.com>
  *
@@ -21,6 +23,7 @@
  */
 
 #include <linux/gpio/consumer.h>
+#include <linux/i2c-mux.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
@@ -86,8 +89,68 @@ struct sii902x {
 	struct drm_bridge bridge;
 	struct drm_connector connector;
 	struct gpio_desc *reset_gpio;
+	struct i2c_mux_core *i2cmux;
 };
 
+static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
+{
+	struct i2c_msg xfer[] = {
+		{
+			.addr = i2c->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = &reg,
+		}, {
+			.addr = i2c->addr,
+			.flags = I2C_M_RD,
+			.len = 1,
+			.buf = val,
+		}
+	};
+	unsigned char xfers = ARRAY_SIZE(xfer);
+	int ret, retries = 5;
+
+	do {
+		ret = __i2c_transfer(i2c->adapter, xfer, xfers);
+		if (ret < 0)
+			return ret;
+	} while (ret != xfers && --retries);
+	return ret == xfers ? 0 : -1;
+}
+
+static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
+{
+	u8 data[2] = {reg, val};
+	struct i2c_msg xfer = {
+		.addr = i2c->addr,
+		.flags = 0,
+		.len = sizeof(data),
+		.buf = data,
+	};
+	int ret, retries = 5;
+
+	do {
+		ret = __i2c_transfer(i2c->adapter, &xfer, 1);
+		if (ret < 0)
+			return ret;
+	} while (!ret && --retries);
+	return !ret ? -1 : 0;
+}
+
+static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8 mask,
+					u8 val)
+{
+	int ret;
+	u8 status;
+
+	ret = sii902x_read_unlocked(i2c, reg, &status);
+	if (ret)
+		return ret;
+	status &= ~mask;
+	status |= val & mask;
+	return sii902x_write_unlocked(i2c, reg, status);
+}
+
 static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct sii902x, bridge);
@@ -135,41 +198,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
 static int sii902x_get_modes(struct drm_connector *connector)
 {
 	struct sii902x *sii902x = connector_to_sii902x(connector);
-	struct regmap *regmap = sii902x->regmap;
 	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-	struct device *dev = &sii902x->i2c->dev;
-	unsigned long timeout;
-	unsigned int retries;
-	unsigned int status;
 	struct edid *edid;
-	int num = 0;
-	int ret;
-
-	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
-				 SII902X_SYS_CTRL_DDC_BUS_REQ,
-				 SII902X_SYS_CTRL_DDC_BUS_REQ);
-	if (ret)
-		return ret;
-
-	timeout = jiffies +
-		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
-	do {
-		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
-		if (ret)
-			return ret;
-	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
-		 time_before(jiffies, timeout));
-
-	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
-		dev_err(dev, "failed to acquire the i2c bus\n");
-		return -ETIMEDOUT;
-	}
-
-	ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
-	if (ret)
-		return ret;
+	int num = 0, ret;
 
-	edid = drm_get_edid(connector, sii902x->i2c->adapter);
+	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
 	drm_connector_update_edid_property(connector, edid);
 	if (edid) {
 		num = drm_add_edid_modes(connector, edid);
@@ -181,42 +214,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
 	if (ret)
 		return ret;
 
-	/*
-	 * Sometimes the I2C bus can stall after failure to use the
-	 * EDID channel. Retry a few times to see if things clear
-	 * up, else continue anyway.
-	 */
-	retries = 5;
-	do {
-		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
-				  &status);
-		retries--;
-	} while (ret && retries);
-	if (ret)
-		dev_err(dev, "failed to read status (%d)\n", ret);
-
-	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
-				 SII902X_SYS_CTRL_DDC_BUS_REQ |
-				 SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
-	if (ret)
-		return ret;
-
-	timeout = jiffies +
-		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
-	do {
-		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
-		if (ret)
-			return ret;
-	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
-			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
-		 time_before(jiffies, timeout));
-
-	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
-		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
-		dev_err(dev, "failed to release the i2c bus\n");
-		return -ETIMEDOUT;
-	}
-
 	return num;
 }
 
@@ -366,6 +363,112 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+/*
+ * The purpose of sii902x_i2c_bypass_select is to enable the pass through
+ * mode of the HDMI transmitter. Do not use regmap from within this function,
+ * only use sii902x_*_unlocked functions to read/modify/write registers.
+ * We are holding the parent adapter lock here, keep this in mind before
+ * adding more i2c transactions.
+ */
+static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
+{
+	struct sii902x *sii902x = i2c_mux_priv(mux);
+	struct device *dev = &sii902x->i2c->dev;
+	unsigned long timeout;
+	u8 status;
+	int ret;
+
+	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					   SII902X_SYS_CTRL_DDC_BUS_REQ,
+					   SII902X_SYS_CTRL_DDC_BUS_REQ);
+
+	timeout = jiffies +
+		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
+	do {
+		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					    &status);
+		if (ret)
+			return ret;
+	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
+		 time_before(jiffies, timeout));
+
+	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
+		dev_err(dev, "Failed to acquire the i2c bus\n");
+		return -ETIMEDOUT;
+	}
+
+	ret = sii902x_write_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+				     status);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+/*
+ * The purpose of sii902x_i2c_bypass_deselect is to disable the pass through
+ * mode of the HDMI transmitter. Do not use regmap from within this function,
+ * only use sii902x_*_unlocked functions to read/modify/write registers.
+ * We are holding the parent adapter lock here, keep this in mind before
+ * adding more i2c transactions.
+ */
+static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
+{
+	struct sii902x *sii902x = i2c_mux_priv(mux);
+	struct device *dev = &sii902x->i2c->dev;
+	unsigned long timeout;
+	unsigned int retries;
+	u8 status;
+	int ret;
+
+	/*
+	 * When the HDMI transmitter is in pass through mode, we need an
+	 * (undocumented) additional delay between STOP and START conditions
+	 * to guarantee the bus won't get stuck.
+	 */
+	udelay(30);
+
+	/*
+	 * Sometimes the I2C bus can stall after failure to use the
+	 * EDID channel. Retry a few times to see if things clear
+	 * up, else continue anyway.
+	 */
+	retries = 5;
+	do {
+		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					    &status);
+		retries--;
+	} while (ret && retries);
+	if (ret) {
+		dev_err(dev, "failed to read status (%d)\n", ret);
+		return ret;
+	}
+
+	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					   SII902X_SYS_CTRL_DDC_BUS_REQ |
+					   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
+	if (ret)
+		return ret;
+
+	timeout = jiffies +
+		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
+	do {
+		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					    &status);
+		if (ret)
+			return ret;
+	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
+			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
+		 time_before(jiffies, timeout));
+
+	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
+		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
+		dev_err(dev, "failed to release the i2c bus\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
 static int sii902x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -375,6 +478,12 @@ static int sii902x_probe(struct i2c_client *client,
 	u8 chipid[4];
 	int ret;
 
+	ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
+	if (!ret) {
+		dev_err(dev, "I2C adapter not suitable\n");
+		return -EIO;
+	}
+
 	sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
 	if (!sii902x)
 		return -ENOMEM;
@@ -433,6 +542,22 @@ static int sii902x_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, sii902x);
 
+	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
+					1, 0, I2C_MUX_GATE,
+					sii902x_i2c_bypass_select,
+					sii902x_i2c_bypass_deselect);
+	if (!sii902x->i2cmux) {
+		dev_err(dev, "failed to allocate I2C mux\n");
+		return -ENOMEM;
+	}
+
+	sii902x->i2cmux->priv = sii902x;
+	ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
+	if (ret) {
+		dev_err(dev, "Couldn't add i2c mux adapter\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -441,6 +566,9 @@ static int sii902x_remove(struct i2c_client *client)
 {
 	struct sii902x *sii902x = i2c_get_clientdata(client);
 
+	if (sii902x->i2cmux)
+		i2c_mux_del_adapters(sii902x->i2cmux);
+
 	drm_bridge_remove(&sii902x->bridge);
 
 	return 0;
-- 
2.7.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-02 12:13 ` Fabrizio Castro
  0 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-02 12:13 UTC (permalink / raw)
  To: Archit Taneja, Andrzej Hajda, David Airlie, Peter Rosin,
	Wolfram Sang, Mark Brown
  Cc: Fabrizio Castro, Laurent Pinchart, dri-devel, Simon Horman,
	Geert Uytterhoeven, Chris Paterson, Biju Das, linux-renesas-soc,
	linux-i2c, Inki Dae, Boris Brezillon, Linus Walleij

While adding SiI9022A support to the iwg23s board, it came
up that when the HDMI transmitter is in pass through mode the
device is not compliant with the I2C specification anymore,
as it requires a far bigger tbuf, due to a delay the HDMI
transmitter is adding when relaying the STOP condition on the
monitor i2c side of things.

When not providing an appropriate delay after the STOP condition
the i2c bus would get stuck. Also, any other traffic on the bus
while talking to the monitor may cause the transaction to fail
or even cause issues with the i2c bus as well.

I2c-gates seemed to reach consent as a possible way to address
these issues, and as such this patch is implementing a solution
based on that. Since others are clearly relying on the current
implementation of the driver, this patch won't require any DT
changes.

Since we don't want any interference during the DDC Bus
Request/Grant procedure and while talking to the monitor, we
have to use the adapter locking primitives rather than the
i2c-mux locking primitives.

Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
---
RFC->PATCH:
* Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments

Thank you guys

 drivers/gpu/drm/bridge/sii902x.c | 264 +++++++++++++++++++++++++++++----------
 1 file changed, 196 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index e59a135..4f9d9c7 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -1,4 +1,6 @@
 /*
+ * Copyright (C) 2018 Renesas Electronics
+ *
  * Copyright (C) 2016 Atmel
  *		      Bo Shen <voice.shen@atmel.com>
  *
@@ -21,6 +23,7 @@
  */
 
 #include <linux/gpio/consumer.h>
+#include <linux/i2c-mux.h>
 #include <linux/i2c.h>
 #include <linux/module.h>
 #include <linux/regmap.h>
@@ -86,8 +89,68 @@ struct sii902x {
 	struct drm_bridge bridge;
 	struct drm_connector connector;
 	struct gpio_desc *reset_gpio;
+	struct i2c_mux_core *i2cmux;
 };
 
+static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
+{
+	struct i2c_msg xfer[] = {
+		{
+			.addr = i2c->addr,
+			.flags = 0,
+			.len = 1,
+			.buf = &reg,
+		}, {
+			.addr = i2c->addr,
+			.flags = I2C_M_RD,
+			.len = 1,
+			.buf = val,
+		}
+	};
+	unsigned char xfers = ARRAY_SIZE(xfer);
+	int ret, retries = 5;
+
+	do {
+		ret = __i2c_transfer(i2c->adapter, xfer, xfers);
+		if (ret < 0)
+			return ret;
+	} while (ret != xfers && --retries);
+	return ret == xfers ? 0 : -1;
+}
+
+static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
+{
+	u8 data[2] = {reg, val};
+	struct i2c_msg xfer = {
+		.addr = i2c->addr,
+		.flags = 0,
+		.len = sizeof(data),
+		.buf = data,
+	};
+	int ret, retries = 5;
+
+	do {
+		ret = __i2c_transfer(i2c->adapter, &xfer, 1);
+		if (ret < 0)
+			return ret;
+	} while (!ret && --retries);
+	return !ret ? -1 : 0;
+}
+
+static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8 mask,
+					u8 val)
+{
+	int ret;
+	u8 status;
+
+	ret = sii902x_read_unlocked(i2c, reg, &status);
+	if (ret)
+		return ret;
+	status &= ~mask;
+	status |= val & mask;
+	return sii902x_write_unlocked(i2c, reg, status);
+}
+
 static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
 {
 	return container_of(bridge, struct sii902x, bridge);
@@ -135,41 +198,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
 static int sii902x_get_modes(struct drm_connector *connector)
 {
 	struct sii902x *sii902x = connector_to_sii902x(connector);
-	struct regmap *regmap = sii902x->regmap;
 	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
-	struct device *dev = &sii902x->i2c->dev;
-	unsigned long timeout;
-	unsigned int retries;
-	unsigned int status;
 	struct edid *edid;
-	int num = 0;
-	int ret;
-
-	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
-				 SII902X_SYS_CTRL_DDC_BUS_REQ,
-				 SII902X_SYS_CTRL_DDC_BUS_REQ);
-	if (ret)
-		return ret;
-
-	timeout = jiffies +
-		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
-	do {
-		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
-		if (ret)
-			return ret;
-	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
-		 time_before(jiffies, timeout));
-
-	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
-		dev_err(dev, "failed to acquire the i2c bus\n");
-		return -ETIMEDOUT;
-	}
-
-	ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
-	if (ret)
-		return ret;
+	int num = 0, ret;
 
-	edid = drm_get_edid(connector, sii902x->i2c->adapter);
+	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
 	drm_connector_update_edid_property(connector, edid);
 	if (edid) {
 		num = drm_add_edid_modes(connector, edid);
@@ -181,42 +214,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
 	if (ret)
 		return ret;
 
-	/*
-	 * Sometimes the I2C bus can stall after failure to use the
-	 * EDID channel. Retry a few times to see if things clear
-	 * up, else continue anyway.
-	 */
-	retries = 5;
-	do {
-		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
-				  &status);
-		retries--;
-	} while (ret && retries);
-	if (ret)
-		dev_err(dev, "failed to read status (%d)\n", ret);
-
-	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
-				 SII902X_SYS_CTRL_DDC_BUS_REQ |
-				 SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
-	if (ret)
-		return ret;
-
-	timeout = jiffies +
-		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
-	do {
-		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
-		if (ret)
-			return ret;
-	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
-			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
-		 time_before(jiffies, timeout));
-
-	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
-		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
-		dev_err(dev, "failed to release the i2c bus\n");
-		return -ETIMEDOUT;
-	}
-
 	return num;
 }
 
@@ -366,6 +363,112 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+/*
+ * The purpose of sii902x_i2c_bypass_select is to enable the pass through
+ * mode of the HDMI transmitter. Do not use regmap from within this function,
+ * only use sii902x_*_unlocked functions to read/modify/write registers.
+ * We are holding the parent adapter lock here, keep this in mind before
+ * adding more i2c transactions.
+ */
+static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
+{
+	struct sii902x *sii902x = i2c_mux_priv(mux);
+	struct device *dev = &sii902x->i2c->dev;
+	unsigned long timeout;
+	u8 status;
+	int ret;
+
+	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					   SII902X_SYS_CTRL_DDC_BUS_REQ,
+					   SII902X_SYS_CTRL_DDC_BUS_REQ);
+
+	timeout = jiffies +
+		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
+	do {
+		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					    &status);
+		if (ret)
+			return ret;
+	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
+		 time_before(jiffies, timeout));
+
+	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
+		dev_err(dev, "Failed to acquire the i2c bus\n");
+		return -ETIMEDOUT;
+	}
+
+	ret = sii902x_write_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+				     status);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+/*
+ * The purpose of sii902x_i2c_bypass_deselect is to disable the pass through
+ * mode of the HDMI transmitter. Do not use regmap from within this function,
+ * only use sii902x_*_unlocked functions to read/modify/write registers.
+ * We are holding the parent adapter lock here, keep this in mind before
+ * adding more i2c transactions.
+ */
+static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
+{
+	struct sii902x *sii902x = i2c_mux_priv(mux);
+	struct device *dev = &sii902x->i2c->dev;
+	unsigned long timeout;
+	unsigned int retries;
+	u8 status;
+	int ret;
+
+	/*
+	 * When the HDMI transmitter is in pass through mode, we need an
+	 * (undocumented) additional delay between STOP and START conditions
+	 * to guarantee the bus won't get stuck.
+	 */
+	udelay(30);
+
+	/*
+	 * Sometimes the I2C bus can stall after failure to use the
+	 * EDID channel. Retry a few times to see if things clear
+	 * up, else continue anyway.
+	 */
+	retries = 5;
+	do {
+		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					    &status);
+		retries--;
+	} while (ret && retries);
+	if (ret) {
+		dev_err(dev, "failed to read status (%d)\n", ret);
+		return ret;
+	}
+
+	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					   SII902X_SYS_CTRL_DDC_BUS_REQ |
+					   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
+	if (ret)
+		return ret;
+
+	timeout = jiffies +
+		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
+	do {
+		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
+					    &status);
+		if (ret)
+			return ret;
+	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
+			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
+		 time_before(jiffies, timeout));
+
+	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
+		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
+		dev_err(dev, "failed to release the i2c bus\n");
+		return -ETIMEDOUT;
+	}
+
+	return 0;
+}
+
 static int sii902x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -375,6 +478,12 @@ static int sii902x_probe(struct i2c_client *client,
 	u8 chipid[4];
 	int ret;
 
+	ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
+	if (!ret) {
+		dev_err(dev, "I2C adapter not suitable\n");
+		return -EIO;
+	}
+
 	sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
 	if (!sii902x)
 		return -ENOMEM;
@@ -433,6 +542,22 @@ static int sii902x_probe(struct i2c_client *client,
 
 	i2c_set_clientdata(client, sii902x);
 
+	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
+					1, 0, I2C_MUX_GATE,
+					sii902x_i2c_bypass_select,
+					sii902x_i2c_bypass_deselect);
+	if (!sii902x->i2cmux) {
+		dev_err(dev, "failed to allocate I2C mux\n");
+		return -ENOMEM;
+	}
+
+	sii902x->i2cmux->priv = sii902x;
+	ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
+	if (ret) {
+		dev_err(dev, "Couldn't add i2c mux adapter\n");
+		return ret;
+	}
+
 	return 0;
 }
 
@@ -441,6 +566,9 @@ static int sii902x_remove(struct i2c_client *client)
 {
 	struct sii902x *sii902x = i2c_get_clientdata(client);
 
+	if (sii902x->i2cmux)
+		i2c_mux_del_adapters(sii902x->i2cmux);
+
 	drm_bridge_remove(&sii902x->bridge);
 
 	return 0;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
  2018-11-02 12:13 ` Fabrizio Castro
@ 2018-11-02 12:26   ` Peter Rosin
  -1 siblings, 0 replies; 16+ messages in thread
From: Peter Rosin @ 2018-11-02 12:26 UTC (permalink / raw)
  To: Fabrizio Castro, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Chris Paterson, Geert Uytterhoeven, Boris Brezillon, dri-devel,
	Biju Das, linux-renesas-soc, Simon Horman, Laurent Pinchart,
	linux-i2c

On 2018-11-02 13:13, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
> 
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
> 
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
> 
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
> RFC->PATCH:
> * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
> 
> Thank you guys
> 
>  drivers/gpu/drm/bridge/sii902x.c | 264 +++++++++++++++++++++++++++++----------
>  1 file changed, 196 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..4f9d9c7 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
>  /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
>   * Copyright (C) 2016 Atmel
>   *		      Bo Shen <voice.shen@atmel.com>
>   *
> @@ -21,6 +23,7 @@
>   */
>  
>  #include <linux/gpio/consumer.h>
> +#include <linux/i2c-mux.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> @@ -86,8 +89,68 @@ struct sii902x {
>  	struct drm_bridge bridge;
>  	struct drm_connector connector;
>  	struct gpio_desc *reset_gpio;
> +	struct i2c_mux_core *i2cmux;
>  };
>  
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> +	struct i2c_msg xfer[] = {
> +		{
> +			.addr = i2c->addr,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &reg,
> +		}, {
> +			.addr = i2c->addr,
> +			.flags = I2C_M_RD,
> +			.len = 1,
> +			.buf = val,
> +		}
> +	};
> +	unsigned char xfers = ARRAY_SIZE(xfer);
> +	int ret, retries = 5;
> +
> +	do {
> +		ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> +		if (ret < 0)
> +			return ret;
> +	} while (ret != xfers && --retries);
> +	return ret == xfers ? 0 : -1;
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> +	u8 data[2] = {reg, val};
> +	struct i2c_msg xfer = {
> +		.addr = i2c->addr,
> +		.flags = 0,
> +		.len = sizeof(data),
> +		.buf = data,
> +	};
> +	int ret, retries = 5;
> +
> +	do {
> +		ret = __i2c_transfer(i2c->adapter, &xfer, 1);
> +		if (ret < 0)
> +			return ret;
> +	} while (!ret && --retries);
> +	return !ret ? -1 : 0;
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8 mask,
> +					u8 val)
> +{
> +	int ret;
> +	u8 status;
> +
> +	ret = sii902x_read_unlocked(i2c, reg, &status);
> +	if (ret)
> +		return ret;
> +	status &= ~mask;
> +	status |= val & mask;
> +	return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
>  static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sii902x, bridge);
> @@ -135,41 +198,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
>  static int sii902x_get_modes(struct drm_connector *connector)
>  {
>  	struct sii902x *sii902x = connector_to_sii902x(connector);
> -	struct regmap *regmap = sii902x->regmap;
>  	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> -	struct device *dev = &sii902x->i2c->dev;
> -	unsigned long timeout;
> -	unsigned int retries;
> -	unsigned int status;
>  	struct edid *edid;
> -	int num = 0;
> -	int ret;
> -
> -	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ);
> -	if (ret)
> -		return ret;
> -
> -	timeout = jiffies +
> -		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> -		if (ret)
> -			return ret;
> -	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> -		 time_before(jiffies, timeout));
> -
> -	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> -		dev_err(dev, "failed to acquire the i2c bus\n");
> -		return -ETIMEDOUT;
> -	}
> -
> -	ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
> -	if (ret)
> -		return ret;
> +	int num = 0, ret;
>  
> -	edid = drm_get_edid(connector, sii902x->i2c->adapter);
> +	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
>  	drm_connector_update_edid_property(connector, edid);
>  	if (edid) {
>  		num = drm_add_edid_modes(connector, edid);
> @@ -181,42 +214,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * Sometimes the I2C bus can stall after failure to use the
> -	 * EDID channel. Retry a few times to see if things clear
> -	 * up, else continue anyway.
> -	 */
> -	retries = 5;
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
> -				  &status);
> -		retries--;
> -	} while (ret && retries);
> -	if (ret)
> -		dev_err(dev, "failed to read status (%d)\n", ret);
> -
> -	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ |
> -				 SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> -	if (ret)
> -		return ret;
> -
> -	timeout = jiffies +
> -		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> -		if (ret)
> -			return ret;
> -	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> -			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> -		 time_before(jiffies, timeout));
> -
> -	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> -		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> -		dev_err(dev, "failed to release the i2c bus\n");
> -		return -ETIMEDOUT;
> -	}
> -
>  	return num;
>  }
>  
> @@ -366,6 +363,112 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * The purpose of sii902x_i2c_bypass_select is to enable the pass through
> + * mode of the HDMI transmitter. Do not use regmap from within this function,
> + * only use sii902x_*_unlocked functions to read/modify/write registers.
> + * We are holding the parent adapter lock here, keep this in mind before
> + * adding more i2c transactions.
> + */
> +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> +{
> +	struct sii902x *sii902x = i2c_mux_priv(mux);
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned long timeout;
> +	u8 status;
> +	int ret;
> +
> +	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ);

Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the
SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is
also doing rmw accesses to that register in other parts of the driver. I
think you need to either add comment as to why that is safe (maybe other
things make it impossible for the two rmw accesses to cross?), or add the
missing coordination.

> +
> +	timeout = jiffies +
> +		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		if (ret)
> +			return ret;
> +	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +		 time_before(jiffies, timeout));
> +
> +	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> +		dev_err(dev, "Failed to acquire the i2c bus\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = sii902x_write_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +				     status);
> +	if (ret)
> +		return ret;
> +	return 0;
> +}
> +
> +/*
> + * The purpose of sii902x_i2c_bypass_deselect is to disable the pass through
> + * mode of the HDMI transmitter. Do not use regmap from within this function,
> + * only use sii902x_*_unlocked functions to read/modify/write registers.
> + * We are holding the parent adapter lock here, keep this in mind before
> + * adding more i2c transactions.
> + */
> +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> +{
> +	struct sii902x *sii902x = i2c_mux_priv(mux);
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned long timeout;
> +	unsigned int retries;
> +	u8 status;
> +	int ret;
> +
> +	/*
> +	 * When the HDMI transmitter is in pass through mode, we need an
> +	 * (undocumented) additional delay between STOP and START conditions
> +	 * to guarantee the bus won't get stuck.
> +	 */
> +	udelay(30);
> +
> +	/*
> +	 * Sometimes the I2C bus can stall after failure to use the
> +	 * EDID channel. Retry a few times to see if things clear
> +	 * up, else continue anyway.
> +	 */
> +	retries = 5;
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		retries--;
> +	} while (ret && retries);
> +	if (ret) {
> +		dev_err(dev, "failed to read status (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ |
> +					   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> +	if (ret)
> +		return ret;
> +
> +	timeout = jiffies +
> +		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		if (ret)
> +			return ret;
> +	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> +			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +		 time_before(jiffies, timeout));
> +
> +	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> +		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> +		dev_err(dev, "failed to release the i2c bus\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
>  static int sii902x_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -375,6 +478,12 @@ static int sii902x_probe(struct i2c_client *client,
>  	u8 chipid[4];
>  	int ret;
>  
> +	ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
> +	if (!ret) {
> +		dev_err(dev, "I2C adapter not suitable\n");
> +		return -EIO;
> +	}
> +
>  	sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
>  	if (!sii902x)
>  		return -ENOMEM;
> @@ -433,6 +542,22 @@ static int sii902x_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, sii902x);
>  
> +	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> +					1, 0, I2C_MUX_GATE,
> +					sii902x_i2c_bypass_select,
> +					sii902x_i2c_bypass_deselect);
> +	if (!sii902x->i2cmux) {
> +		dev_err(dev, "failed to allocate I2C mux\n");
> +		return -ENOMEM;
> +	}
> +
> +	sii902x->i2cmux->priv = sii902x;
> +	ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> +	if (ret) {
> +		dev_err(dev, "Couldn't add i2c mux adapter\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -441,6 +566,9 @@ static int sii902x_remove(struct i2c_client *client)
>  {
>  	struct sii902x *sii902x = i2c_get_clientdata(client);
>  
> +	if (sii902x->i2cmux)
> +		i2c_mux_del_adapters(sii902x->i2cmux);
> +
>  	drm_bridge_remove(&sii902x->bridge);
>  
>  	return 0;
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-02 12:26   ` Peter Rosin
  0 siblings, 0 replies; 16+ messages in thread
From: Peter Rosin @ 2018-11-02 12:26 UTC (permalink / raw)
  To: Fabrizio Castro, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Laurent Pinchart, dri-devel, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Biju Das, linux-renesas-soc, linux-i2c, Inki Dae,
	Boris Brezillon, Linus Walleij

On 2018-11-02 13:13, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
> 
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
> 
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
> 
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
> RFC->PATCH:
> * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
> 
> Thank you guys
> 
>  drivers/gpu/drm/bridge/sii902x.c | 264 +++++++++++++++++++++++++++++----------
>  1 file changed, 196 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..4f9d9c7 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
>  /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
>   * Copyright (C) 2016 Atmel
>   *		      Bo Shen <voice.shen@atmel.com>
>   *
> @@ -21,6 +23,7 @@
>   */
>  
>  #include <linux/gpio/consumer.h>
> +#include <linux/i2c-mux.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> @@ -86,8 +89,68 @@ struct sii902x {
>  	struct drm_bridge bridge;
>  	struct drm_connector connector;
>  	struct gpio_desc *reset_gpio;
> +	struct i2c_mux_core *i2cmux;
>  };
>  
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> +	struct i2c_msg xfer[] = {
> +		{
> +			.addr = i2c->addr,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &reg,
> +		}, {
> +			.addr = i2c->addr,
> +			.flags = I2C_M_RD,
> +			.len = 1,
> +			.buf = val,
> +		}
> +	};
> +	unsigned char xfers = ARRAY_SIZE(xfer);
> +	int ret, retries = 5;
> +
> +	do {
> +		ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> +		if (ret < 0)
> +			return ret;
> +	} while (ret != xfers && --retries);
> +	return ret == xfers ? 0 : -1;
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> +	u8 data[2] = {reg, val};
> +	struct i2c_msg xfer = {
> +		.addr = i2c->addr,
> +		.flags = 0,
> +		.len = sizeof(data),
> +		.buf = data,
> +	};
> +	int ret, retries = 5;
> +
> +	do {
> +		ret = __i2c_transfer(i2c->adapter, &xfer, 1);
> +		if (ret < 0)
> +			return ret;
> +	} while (!ret && --retries);
> +	return !ret ? -1 : 0;
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8 mask,
> +					u8 val)
> +{
> +	int ret;
> +	u8 status;
> +
> +	ret = sii902x_read_unlocked(i2c, reg, &status);
> +	if (ret)
> +		return ret;
> +	status &= ~mask;
> +	status |= val & mask;
> +	return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
>  static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sii902x, bridge);
> @@ -135,41 +198,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
>  static int sii902x_get_modes(struct drm_connector *connector)
>  {
>  	struct sii902x *sii902x = connector_to_sii902x(connector);
> -	struct regmap *regmap = sii902x->regmap;
>  	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> -	struct device *dev = &sii902x->i2c->dev;
> -	unsigned long timeout;
> -	unsigned int retries;
> -	unsigned int status;
>  	struct edid *edid;
> -	int num = 0;
> -	int ret;
> -
> -	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ);
> -	if (ret)
> -		return ret;
> -
> -	timeout = jiffies +
> -		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> -		if (ret)
> -			return ret;
> -	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> -		 time_before(jiffies, timeout));
> -
> -	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> -		dev_err(dev, "failed to acquire the i2c bus\n");
> -		return -ETIMEDOUT;
> -	}
> -
> -	ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
> -	if (ret)
> -		return ret;
> +	int num = 0, ret;
>  
> -	edid = drm_get_edid(connector, sii902x->i2c->adapter);
> +	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
>  	drm_connector_update_edid_property(connector, edid);
>  	if (edid) {
>  		num = drm_add_edid_modes(connector, edid);
> @@ -181,42 +214,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * Sometimes the I2C bus can stall after failure to use the
> -	 * EDID channel. Retry a few times to see if things clear
> -	 * up, else continue anyway.
> -	 */
> -	retries = 5;
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
> -				  &status);
> -		retries--;
> -	} while (ret && retries);
> -	if (ret)
> -		dev_err(dev, "failed to read status (%d)\n", ret);
> -
> -	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ |
> -				 SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> -	if (ret)
> -		return ret;
> -
> -	timeout = jiffies +
> -		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> -		if (ret)
> -			return ret;
> -	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> -			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> -		 time_before(jiffies, timeout));
> -
> -	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> -		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> -		dev_err(dev, "failed to release the i2c bus\n");
> -		return -ETIMEDOUT;
> -	}
> -
>  	return num;
>  }
>  
> @@ -366,6 +363,112 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * The purpose of sii902x_i2c_bypass_select is to enable the pass through
> + * mode of the HDMI transmitter. Do not use regmap from within this function,
> + * only use sii902x_*_unlocked functions to read/modify/write registers.
> + * We are holding the parent adapter lock here, keep this in mind before
> + * adding more i2c transactions.
> + */
> +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> +{
> +	struct sii902x *sii902x = i2c_mux_priv(mux);
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned long timeout;
> +	u8 status;
> +	int ret;
> +
> +	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ);

Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the
SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is
also doing rmw accesses to that register in other parts of the driver. I
think you need to either add comment as to why that is safe (maybe other
things make it impossible for the two rmw accesses to cross?), or add the
missing coordination.

> +
> +	timeout = jiffies +
> +		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		if (ret)
> +			return ret;
> +	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +		 time_before(jiffies, timeout));
> +
> +	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> +		dev_err(dev, "Failed to acquire the i2c bus\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = sii902x_write_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +				     status);
> +	if (ret)
> +		return ret;
> +	return 0;
> +}
> +
> +/*
> + * The purpose of sii902x_i2c_bypass_deselect is to disable the pass through
> + * mode of the HDMI transmitter. Do not use regmap from within this function,
> + * only use sii902x_*_unlocked functions to read/modify/write registers.
> + * We are holding the parent adapter lock here, keep this in mind before
> + * adding more i2c transactions.
> + */
> +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> +{
> +	struct sii902x *sii902x = i2c_mux_priv(mux);
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned long timeout;
> +	unsigned int retries;
> +	u8 status;
> +	int ret;
> +
> +	/*
> +	 * When the HDMI transmitter is in pass through mode, we need an
> +	 * (undocumented) additional delay between STOP and START conditions
> +	 * to guarantee the bus won't get stuck.
> +	 */
> +	udelay(30);
> +
> +	/*
> +	 * Sometimes the I2C bus can stall after failure to use the
> +	 * EDID channel. Retry a few times to see if things clear
> +	 * up, else continue anyway.
> +	 */
> +	retries = 5;
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		retries--;
> +	} while (ret && retries);
> +	if (ret) {
> +		dev_err(dev, "failed to read status (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ |
> +					   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> +	if (ret)
> +		return ret;
> +
> +	timeout = jiffies +
> +		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		if (ret)
> +			return ret;
> +	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> +			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +		 time_before(jiffies, timeout));
> +
> +	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> +		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> +		dev_err(dev, "failed to release the i2c bus\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
>  static int sii902x_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -375,6 +478,12 @@ static int sii902x_probe(struct i2c_client *client,
>  	u8 chipid[4];
>  	int ret;
>  
> +	ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
> +	if (!ret) {
> +		dev_err(dev, "I2C adapter not suitable\n");
> +		return -EIO;
> +	}
> +
>  	sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
>  	if (!sii902x)
>  		return -ENOMEM;
> @@ -433,6 +542,22 @@ static int sii902x_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, sii902x);
>  
> +	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> +					1, 0, I2C_MUX_GATE,
> +					sii902x_i2c_bypass_select,
> +					sii902x_i2c_bypass_deselect);
> +	if (!sii902x->i2cmux) {
> +		dev_err(dev, "failed to allocate I2C mux\n");
> +		return -ENOMEM;
> +	}
> +
> +	sii902x->i2cmux->priv = sii902x;
> +	ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> +	if (ret) {
> +		dev_err(dev, "Couldn't add i2c mux adapter\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -441,6 +566,9 @@ static int sii902x_remove(struct i2c_client *client)
>  {
>  	struct sii902x *sii902x = i2c_get_clientdata(client);
>  
> +	if (sii902x->i2cmux)
> +		i2c_mux_del_adapters(sii902x->i2cmux);
> +
>  	drm_bridge_remove(&sii902x->bridge);
>  
>  	return 0;
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] drm/bridge/sii902x: Fix EDID readback
  2018-11-02 12:26   ` Peter Rosin
@ 2018-11-02 15:18     ` Fabrizio Castro
  -1 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-02 15:18 UTC (permalink / raw)
  To: Peter Rosin, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Chris Paterson, Geert Uytterhoeven, Boris Brezillon, dri-devel,
	Biju Das, linux-renesas-soc, Simon Horman, Laurent Pinchart,
	linux-i2c

Hello Peter,

Thank you for your feedback!

> > +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> > +{
> > +struct sii902x *sii902x = i2c_mux_priv(mux);
> > +struct device *dev = &sii902x->i2c->dev;
> > +unsigned long timeout;
> > +u8 status;
> > +int ret;
> > +
> > +ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ);
>
> Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the
> SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is
> also doing rmw accesses to that register in other parts of the driver. I
> think you need to either add comment as to why that is safe (maybe other
> things make it impossible for the two rmw accesses to cross?), or add the
> missing coordination.
>

The other two places where SII902X_SYS_CTRL_DATA is being handled are
sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is
any chance of the modes being probed while the bridge gets enabled/disabled,
but now that you make me think about it user space may trigger the readback
of the EDID at the most inconvenient times. Anyway, this is a good point, and
since I don't think I am supposed to access regmap's lock from outside the APIs,
I could switch to the unlocked version of update_bits from within sii902x_bridge_disable
and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do you think?

Fab



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-02 15:18     ` Fabrizio Castro
  0 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-02 15:18 UTC (permalink / raw)
  To: Peter Rosin, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Laurent Pinchart, dri-devel, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Biju Das, linux-renesas-soc, linux-i2c, Inki Dae,
	Boris Brezillon, Linus Walleij

Hello Peter,

Thank you for your feedback!

> > +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> > +{
> > +struct sii902x *sii902x = i2c_mux_priv(mux);
> > +struct device *dev = &sii902x->i2c->dev;
> > +unsigned long timeout;
> > +u8 status;
> > +int ret;
> > +
> > +ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ);
>
> Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the
> SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is
> also doing rmw accesses to that register in other parts of the driver. I
> think you need to either add comment as to why that is safe (maybe other
> things make it impossible for the two rmw accesses to cross?), or add the
> missing coordination.
>

The other two places where SII902X_SYS_CTRL_DATA is being handled are
sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is
any chance of the modes being probed while the bridge gets enabled/disabled,
but now that you make me think about it user space may trigger the readback
of the EDID at the most inconvenient times. Anyway, this is a good point, and
since I don't think I am supposed to access regmap's lock from outside the APIs,
I could switch to the unlocked version of update_bits from within sii902x_bridge_disable
and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do you think?

Fab



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] drm/bridge/sii902x: Fix EDID readback
  2018-11-02 15:18     ` Fabrizio Castro
@ 2018-11-02 16:16       ` Fabrizio Castro
  -1 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-02 16:16 UTC (permalink / raw)
  To: Fabrizio Castro, Peter Rosin, Archit Taneja, Andrzej Hajda,
	David Airlie, Wolfram Sang, Mark Brown
  Cc: Chris Paterson, Geert Uytterhoeven, Boris Brezillon, dri-devel,
	Biju Das, linux-renesas-soc, Simon Horman, Laurent Pinchart,
	linux-i2c

> > Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the
> > SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is
> > also doing rmw accesses to that register in other parts of the driver. I
> > think you need to either add comment as to why that is safe (maybe other
> > things make it impossible for the two rmw accesses to cross?), or add the
> > missing coordination.
> >
>
> The other two places where SII902X_SYS_CTRL_DATA is being handled are
> sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is
> any chance of the modes being probed while the bridge gets enabled/disabled,
> but now that you make me think about it user space may trigger the readback
> of the EDID at the most inconvenient times. Anyway, this is a good point, and
> since I don't think I am supposed to access regmap's lock from outside the APIs,
> I could switch to the unlocked version of update_bits from within sii902x_bridge_disable
> and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do you think?
>

The bridge enable/disable callbacks deal with different bits of register
SII902X_SYS_CTRL_DATA, and the value of register SII902X_SYS_CTRL_DATA  after
sii902x_i2c_bypass_deselect is the same as when sii902x_i2c_bypass_select gets triggered
so basically even if we managed to get in the way of the regmap rmw (in the sense that
for example sii902x_bridge_enable reads SII902X_SYS_CTRL_DATA and then we call
into sii902x_i2c_bypass_select) by the time regmap_update_bits can perform the
write operation the value of register SII902X_SYS_CTRL_DATA is the same as the one
read by regmap, as "select xfer deselect" won't alter the final value of SII902X_SYS_CTRL_DATA
and nobody can get in between.
What do you think I should do? Do you think I can leave this alone and maybe add some
comments or do you think I should explicitly protect access to SII902X_SYS_CTRL_DATA?

Thanks,
Fab





Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-02 16:16       ` Fabrizio Castro
  0 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-02 16:16 UTC (permalink / raw)
  To: Fabrizio Castro, Peter Rosin, Archit Taneja, Andrzej Hajda,
	David Airlie, Wolfram Sang, Mark Brown
  Cc: Laurent Pinchart, dri-devel, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Biju Das, linux-renesas-soc, linux-i2c, Inki Dae,
	Boris Brezillon, Linus Walleij

> > Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the
> > SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is
> > also doing rmw accesses to that register in other parts of the driver. I
> > think you need to either add comment as to why that is safe (maybe other
> > things make it impossible for the two rmw accesses to cross?), or add the
> > missing coordination.
> >
>
> The other two places where SII902X_SYS_CTRL_DATA is being handled are
> sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is
> any chance of the modes being probed while the bridge gets enabled/disabled,
> but now that you make me think about it user space may trigger the readback
> of the EDID at the most inconvenient times. Anyway, this is a good point, and
> since I don't think I am supposed to access regmap's lock from outside the APIs,
> I could switch to the unlocked version of update_bits from within sii902x_bridge_disable
> and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do you think?
>

The bridge enable/disable callbacks deal with different bits of register
SII902X_SYS_CTRL_DATA, and the value of register SII902X_SYS_CTRL_DATA  after
sii902x_i2c_bypass_deselect is the same as when sii902x_i2c_bypass_select gets triggered
so basically even if we managed to get in the way of the regmap rmw (in the sense that
for example sii902x_bridge_enable reads SII902X_SYS_CTRL_DATA and then we call
into sii902x_i2c_bypass_select) by the time regmap_update_bits can perform the
write operation the value of register SII902X_SYS_CTRL_DATA is the same as the one
read by regmap, as "select xfer deselect" won't alter the final value of SII902X_SYS_CTRL_DATA
and nobody can get in between.
What do you think I should do? Do you think I can leave this alone and maybe add some
comments or do you think I should explicitly protect access to SII902X_SYS_CTRL_DATA?

Thanks,
Fab





Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
  2018-11-02 16:16       ` Fabrizio Castro
@ 2018-11-02 21:16         ` Peter Rosin
  -1 siblings, 0 replies; 16+ messages in thread
From: Peter Rosin @ 2018-11-02 21:16 UTC (permalink / raw)
  To: Fabrizio Castro, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Chris Paterson, Geert Uytterhoeven, Boris Brezillon, dri-devel,
	Biju Das, linux-renesas-soc, Simon Horman, Laurent Pinchart,
	linux-i2c

On 2018-11-02 17:16, Fabrizio Castro wrote:
>>> Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the
>>> SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is
>>> also doing rmw accesses to that register in other parts of the driver. I
>>> think you need to either add comment as to why that is safe (maybe other
>>> things make it impossible for the two rmw accesses to cross?), or add the
>>> missing coordination.
>>>
>>
>> The other two places where SII902X_SYS_CTRL_DATA is being handled are
>> sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is
>> any chance of the modes being probed while the bridge gets enabled/disabled,
>> but now that you make me think about it user space may trigger the readback
>> of the EDID at the most inconvenient times. Anyway, this is a good point, and
>> since I don't think I am supposed to access regmap's lock from outside the APIs,
>> I could switch to the unlocked version of update_bits from within sii902x_bridge_disable
>> and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do you think?
>>
> 
> The bridge enable/disable callbacks deal with different bits of register
> SII902X_SYS_CTRL_DATA, and the value of register SII902X_SYS_CTRL_DATA  after
> sii902x_i2c_bypass_deselect is the same as when sii902x_i2c_bypass_select gets triggered
> so basically even if we managed to get in the way of the regmap rmw (in the sense that
> for example sii902x_bridge_enable reads SII902X_SYS_CTRL_DATA and then we call
> into sii902x_i2c_bypass_select) by the time regmap_update_bits can perform the
> write operation the value of register SII902X_SYS_CTRL_DATA is the same as the one
> read by regmap, as "select xfer deselect" won't alter the final value of SII902X_SYS_CTRL_DATA
> and nobody can get in between.
> What do you think I should do? Do you think I can leave this alone and maybe add some
> comments or do you think I should explicitly protect access to SII902X_SYS_CTRL_DATA?

If the things you write about the bits are true (haven't looked closely), I wouldn't
add any regmap coordination. But if I was the maintainer, I'd like a comment about
this so that the next contributor has a better chance to understand the subtlety.

But I'm not the maintainer, so this is not my call, but by adding the comment you
also clarify the situation for the maintainer, which is something that is likely
to be appreciated.

Cheers,
Peter
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-02 21:16         ` Peter Rosin
  0 siblings, 0 replies; 16+ messages in thread
From: Peter Rosin @ 2018-11-02 21:16 UTC (permalink / raw)
  To: Fabrizio Castro, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Laurent Pinchart, dri-devel, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Biju Das, linux-renesas-soc, linux-i2c, Inki Dae,
	Boris Brezillon, Linus Walleij

On 2018-11-02 17:16, Fabrizio Castro wrote:
>>> Here (and also in sii902x_i2c_bypass_deselect) you do a rmw access to the
>>> SII902X_SYS_CTRL_DATA register without coordinating with regmap. Regmap is
>>> also doing rmw accesses to that register in other parts of the driver. I
>>> think you need to either add comment as to why that is safe (maybe other
>>> things make it impossible for the two rmw accesses to cross?), or add the
>>> missing coordination.
>>>
>>
>> The other two places where SII902X_SYS_CTRL_DATA is being handled are
>> sii902x_bridge_disable and sii902x_bridge_enable, I didn’t think there is
>> any chance of the modes being probed while the bridge gets enabled/disabled,
>> but now that you make me think about it user space may trigger the readback
>> of the EDID at the most inconvenient times. Anyway, this is a good point, and
>> since I don't think I am supposed to access regmap's lock from outside the APIs,
>> I could switch to the unlocked version of update_bits from within sii902x_bridge_disable
>> and sii902x_bridge_enable, and manually grab the i2c adapter lock, what do you think?
>>
> 
> The bridge enable/disable callbacks deal with different bits of register
> SII902X_SYS_CTRL_DATA, and the value of register SII902X_SYS_CTRL_DATA  after
> sii902x_i2c_bypass_deselect is the same as when sii902x_i2c_bypass_select gets triggered
> so basically even if we managed to get in the way of the regmap rmw (in the sense that
> for example sii902x_bridge_enable reads SII902X_SYS_CTRL_DATA and then we call
> into sii902x_i2c_bypass_select) by the time regmap_update_bits can perform the
> write operation the value of register SII902X_SYS_CTRL_DATA is the same as the one
> read by regmap, as "select xfer deselect" won't alter the final value of SII902X_SYS_CTRL_DATA
> and nobody can get in between.
> What do you think I should do? Do you think I can leave this alone and maybe add some
> comments or do you think I should explicitly protect access to SII902X_SYS_CTRL_DATA?

If the things you write about the bits are true (haven't looked closely), I wouldn't
add any regmap coordination. But if I was the maintainer, I'd like a comment about
this so that the next contributor has a better chance to understand the subtlety.

But I'm not the maintainer, so this is not my call, but by adding the comment you
also clarify the situation for the maintainer, which is something that is likely
to be appreciated.

Cheers,
Peter

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
  2018-11-02 12:13 ` Fabrizio Castro
@ 2018-11-02 21:33   ` Peter Rosin
  -1 siblings, 0 replies; 16+ messages in thread
From: Peter Rosin @ 2018-11-02 21:33 UTC (permalink / raw)
  To: Fabrizio Castro, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Chris Paterson, Geert Uytterhoeven, Boris Brezillon, dri-devel,
	Biju Das, linux-renesas-soc, Simon Horman, Laurent Pinchart,
	linux-i2c

On 2018-11-02 13:13, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
> 
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
> 
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
> 
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
> RFC->PATCH:
> * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
> 
> Thank you guys
> 
>  drivers/gpu/drm/bridge/sii902x.c | 264 +++++++++++++++++++++++++++++----------
>  1 file changed, 196 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..4f9d9c7 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
>  /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
>   * Copyright (C) 2016 Atmel
>   *		      Bo Shen <voice.shen@atmel.com>
>   *
> @@ -21,6 +23,7 @@
>   */
>  
>  #include <linux/gpio/consumer.h>
> +#include <linux/i2c-mux.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> @@ -86,8 +89,68 @@ struct sii902x {
>  	struct drm_bridge bridge;
>  	struct drm_connector connector;
>  	struct gpio_desc *reset_gpio;
> +	struct i2c_mux_core *i2cmux;
>  };
>  
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> +	struct i2c_msg xfer[] = {
> +		{
> +			.addr = i2c->addr,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &reg,
> +		}, {
> +			.addr = i2c->addr,
> +			.flags = I2C_M_RD,
> +			.len = 1,
> +			.buf = val,
> +		}
> +	};
> +	unsigned char xfers = ARRAY_SIZE(xfer);
> +	int ret, retries = 5;
> +
> +	do {
> +		ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> +		if (ret < 0)
> +			return ret;
> +	} while (ret != xfers && --retries);
> +	return ret == xfers ? 0 : -1;

New in 4.19 __i2c_smbus_xfer, and I think it helps here? (untested code)

	union i2c_smbus_data data;
	int ret;

	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
			       I2C_SMBUS_READ, reg,
			       I2C_SMBUS_BYTE_DATA, &data);

	if (ret < 0)
		return ret;

	*val = data.byte;
	return 0;
	
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> +	u8 data[2] = {reg, val};
> +	struct i2c_msg xfer = {
> +		.addr = i2c->addr,
> +		.flags = 0,
> +		.len = sizeof(data),
> +		.buf = data,
> +	};
> +	int ret, retries = 5;
> +
> +	do {
> +		ret = __i2c_transfer(i2c->adapter, &xfer, 1);
> +		if (ret < 0)
> +			return ret;
> +	} while (!ret && --retries);
> +	return !ret ? -1 : 0;

	union i2c_smbus_data data;

	data.byte = val;

	return __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
				I2C_SMBUS_WRITE, reg,
				I2C_SMBUS_BYTE_DATA, &data);
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8 mask,
> +					u8 val)
> +{
> +	int ret;
> +	u8 status;
> +
> +	ret = sii902x_read_unlocked(i2c, reg, &status);
> +	if (ret)
> +		return ret;
> +	status &= ~mask;
> +	status |= val & mask;
> +	return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
>  static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sii902x, bridge);
> @@ -135,41 +198,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
>  static int sii902x_get_modes(struct drm_connector *connector)
>  {
>  	struct sii902x *sii902x = connector_to_sii902x(connector);
> -	struct regmap *regmap = sii902x->regmap;
>  	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> -	struct device *dev = &sii902x->i2c->dev;
> -	unsigned long timeout;
> -	unsigned int retries;
> -	unsigned int status;
>  	struct edid *edid;
> -	int num = 0;
> -	int ret;
> -
> -	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ);
> -	if (ret)
> -		return ret;
> -
> -	timeout = jiffies +
> -		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> -		if (ret)
> -			return ret;
> -	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> -		 time_before(jiffies, timeout));
> -
> -	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> -		dev_err(dev, "failed to acquire the i2c bus\n");
> -		return -ETIMEDOUT;
> -	}
> -
> -	ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
> -	if (ret)
> -		return ret;
> +	int num = 0, ret;
>  
> -	edid = drm_get_edid(connector, sii902x->i2c->adapter);
> +	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
>  	drm_connector_update_edid_property(connector, edid);
>  	if (edid) {
>  		num = drm_add_edid_modes(connector, edid);
> @@ -181,42 +214,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * Sometimes the I2C bus can stall after failure to use the
> -	 * EDID channel. Retry a few times to see if things clear
> -	 * up, else continue anyway.
> -	 */
> -	retries = 5;
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
> -				  &status);
> -		retries--;
> -	} while (ret && retries);
> -	if (ret)
> -		dev_err(dev, "failed to read status (%d)\n", ret);
> -
> -	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ |
> -				 SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> -	if (ret)
> -		return ret;
> -
> -	timeout = jiffies +
> -		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> -		if (ret)
> -			return ret;
> -	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> -			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> -		 time_before(jiffies, timeout));
> -
> -	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> -		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> -		dev_err(dev, "failed to release the i2c bus\n");
> -		return -ETIMEDOUT;
> -	}
> -
>  	return num;
>  }
>  
> @@ -366,6 +363,112 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * The purpose of sii902x_i2c_bypass_select is to enable the pass through
> + * mode of the HDMI transmitter. Do not use regmap from within this function,
> + * only use sii902x_*_unlocked functions to read/modify/write registers.
> + * We are holding the parent adapter lock here, keep this in mind before
> + * adding more i2c transactions.
> + */
> +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> +{
> +	struct sii902x *sii902x = i2c_mux_priv(mux);
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned long timeout;
> +	u8 status;
> +	int ret;
> +
> +	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ);
> +
> +	timeout = jiffies +
> +		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		if (ret)
> +			return ret;
> +	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +		 time_before(jiffies, timeout));
> +
> +	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> +		dev_err(dev, "Failed to acquire the i2c bus\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = sii902x_write_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +				     status);
> +	if (ret)
> +		return ret;
> +	return 0;
> +}
> +
> +/*
> + * The purpose of sii902x_i2c_bypass_deselect is to disable the pass through
> + * mode of the HDMI transmitter. Do not use regmap from within this function,
> + * only use sii902x_*_unlocked functions to read/modify/write registers.
> + * We are holding the parent adapter lock here, keep this in mind before
> + * adding more i2c transactions.
> + */
> +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> +{
> +	struct sii902x *sii902x = i2c_mux_priv(mux);
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned long timeout;
> +	unsigned int retries;
> +	u8 status;
> +	int ret;
> +
> +	/*
> +	 * When the HDMI transmitter is in pass through mode, we need an
> +	 * (undocumented) additional delay between STOP and START conditions
> +	 * to guarantee the bus won't get stuck.
> +	 */
> +	udelay(30);
> +
> +	/*
> +	 * Sometimes the I2C bus can stall after failure to use the
> +	 * EDID channel. Retry a few times to see if things clear
> +	 * up, else continue anyway.
> +	 */
> +	retries = 5;
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		retries--;
> +	} while (ret && retries);
> +	if (ret) {
> +		dev_err(dev, "failed to read status (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ |
> +					   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> +	if (ret)
> +		return ret;
> +
> +	timeout = jiffies +
> +		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		if (ret)
> +			return ret;
> +	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> +			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +		 time_before(jiffies, timeout));
> +
> +	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> +		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> +		dev_err(dev, "failed to release the i2c bus\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
>  static int sii902x_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -375,6 +478,12 @@ static int sii902x_probe(struct i2c_client *client,
>  	u8 chipid[4];
>  	int ret;
>  
> +	ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);

And then I2C_FUNC_SMBUS_BYTE_DATA here, instead of I2C_FUNC_I2C, of course.

Cheers,
Peter

> +	if (!ret) {
> +		dev_err(dev, "I2C adapter not suitable\n");
> +		return -EIO;
> +	}
> +
>  	sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
>  	if (!sii902x)
>  		return -ENOMEM;
> @@ -433,6 +542,22 @@ static int sii902x_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, sii902x);
>  
> +	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> +					1, 0, I2C_MUX_GATE,
> +					sii902x_i2c_bypass_select,
> +					sii902x_i2c_bypass_deselect);
> +	if (!sii902x->i2cmux) {
> +		dev_err(dev, "failed to allocate I2C mux\n");
> +		return -ENOMEM;
> +	}
> +
> +	sii902x->i2cmux->priv = sii902x;
> +	ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> +	if (ret) {
> +		dev_err(dev, "Couldn't add i2c mux adapter\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -441,6 +566,9 @@ static int sii902x_remove(struct i2c_client *client)
>  {
>  	struct sii902x *sii902x = i2c_get_clientdata(client);
>  
> +	if (sii902x->i2cmux)
> +		i2c_mux_del_adapters(sii902x->i2cmux);
> +
>  	drm_bridge_remove(&sii902x->bridge);
>  
>  	return 0;
> 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-02 21:33   ` Peter Rosin
  0 siblings, 0 replies; 16+ messages in thread
From: Peter Rosin @ 2018-11-02 21:33 UTC (permalink / raw)
  To: Fabrizio Castro, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Laurent Pinchart, dri-devel, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Biju Das, linux-renesas-soc, linux-i2c, Inki Dae,
	Boris Brezillon, Linus Walleij

On 2018-11-02 13:13, Fabrizio Castro wrote:
> While adding SiI9022A support to the iwg23s board, it came
> up that when the HDMI transmitter is in pass through mode the
> device is not compliant with the I2C specification anymore,
> as it requires a far bigger tbuf, due to a delay the HDMI
> transmitter is adding when relaying the STOP condition on the
> monitor i2c side of things.
> 
> When not providing an appropriate delay after the STOP condition
> the i2c bus would get stuck. Also, any other traffic on the bus
> while talking to the monitor may cause the transaction to fail
> or even cause issues with the i2c bus as well.
> 
> I2c-gates seemed to reach consent as a possible way to address
> these issues, and as such this patch is implementing a solution
> based on that. Since others are clearly relying on the current
> implementation of the driver, this patch won't require any DT
> changes.
> 
> Since we don't want any interference during the DDC Bus
> Request/Grant procedure and while talking to the monitor, we
> have to use the adapter locking primitives rather than the
> i2c-mux locking primitives.
> 
> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> ---
> RFC->PATCH:
> * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
> 
> Thank you guys
> 
>  drivers/gpu/drm/bridge/sii902x.c | 264 +++++++++++++++++++++++++++++----------
>  1 file changed, 196 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index e59a135..4f9d9c7 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -1,4 +1,6 @@
>  /*
> + * Copyright (C) 2018 Renesas Electronics
> + *
>   * Copyright (C) 2016 Atmel
>   *		      Bo Shen <voice.shen@atmel.com>
>   *
> @@ -21,6 +23,7 @@
>   */
>  
>  #include <linux/gpio/consumer.h>
> +#include <linux/i2c-mux.h>
>  #include <linux/i2c.h>
>  #include <linux/module.h>
>  #include <linux/regmap.h>
> @@ -86,8 +89,68 @@ struct sii902x {
>  	struct drm_bridge bridge;
>  	struct drm_connector connector;
>  	struct gpio_desc *reset_gpio;
> +	struct i2c_mux_core *i2cmux;
>  };
>  
> +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> +{
> +	struct i2c_msg xfer[] = {
> +		{
> +			.addr = i2c->addr,
> +			.flags = 0,
> +			.len = 1,
> +			.buf = &reg,
> +		}, {
> +			.addr = i2c->addr,
> +			.flags = I2C_M_RD,
> +			.len = 1,
> +			.buf = val,
> +		}
> +	};
> +	unsigned char xfers = ARRAY_SIZE(xfer);
> +	int ret, retries = 5;
> +
> +	do {
> +		ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> +		if (ret < 0)
> +			return ret;
> +	} while (ret != xfers && --retries);
> +	return ret == xfers ? 0 : -1;

New in 4.19 __i2c_smbus_xfer, and I think it helps here? (untested code)

	union i2c_smbus_data data;
	int ret;

	ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
			       I2C_SMBUS_READ, reg,
			       I2C_SMBUS_BYTE_DATA, &data);

	if (ret < 0)
		return ret;

	*val = data.byte;
	return 0;
	
> +}
> +
> +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> +{
> +	u8 data[2] = {reg, val};
> +	struct i2c_msg xfer = {
> +		.addr = i2c->addr,
> +		.flags = 0,
> +		.len = sizeof(data),
> +		.buf = data,
> +	};
> +	int ret, retries = 5;
> +
> +	do {
> +		ret = __i2c_transfer(i2c->adapter, &xfer, 1);
> +		if (ret < 0)
> +			return ret;
> +	} while (!ret && --retries);
> +	return !ret ? -1 : 0;

	union i2c_smbus_data data;

	data.byte = val;

	return __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
				I2C_SMBUS_WRITE, reg,
				I2C_SMBUS_BYTE_DATA, &data);
> +}
> +
> +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8 mask,
> +					u8 val)
> +{
> +	int ret;
> +	u8 status;
> +
> +	ret = sii902x_read_unlocked(i2c, reg, &status);
> +	if (ret)
> +		return ret;
> +	status &= ~mask;
> +	status |= val & mask;
> +	return sii902x_write_unlocked(i2c, reg, status);
> +}
> +
>  static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
>  {
>  	return container_of(bridge, struct sii902x, bridge);
> @@ -135,41 +198,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
>  static int sii902x_get_modes(struct drm_connector *connector)
>  {
>  	struct sii902x *sii902x = connector_to_sii902x(connector);
> -	struct regmap *regmap = sii902x->regmap;
>  	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> -	struct device *dev = &sii902x->i2c->dev;
> -	unsigned long timeout;
> -	unsigned int retries;
> -	unsigned int status;
>  	struct edid *edid;
> -	int num = 0;
> -	int ret;
> -
> -	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ);
> -	if (ret)
> -		return ret;
> -
> -	timeout = jiffies +
> -		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> -		if (ret)
> -			return ret;
> -	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> -		 time_before(jiffies, timeout));
> -
> -	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> -		dev_err(dev, "failed to acquire the i2c bus\n");
> -		return -ETIMEDOUT;
> -	}
> -
> -	ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
> -	if (ret)
> -		return ret;
> +	int num = 0, ret;
>  
> -	edid = drm_get_edid(connector, sii902x->i2c->adapter);
> +	edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
>  	drm_connector_update_edid_property(connector, edid);
>  	if (edid) {
>  		num = drm_add_edid_modes(connector, edid);
> @@ -181,42 +214,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
>  	if (ret)
>  		return ret;
>  
> -	/*
> -	 * Sometimes the I2C bus can stall after failure to use the
> -	 * EDID channel. Retry a few times to see if things clear
> -	 * up, else continue anyway.
> -	 */
> -	retries = 5;
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
> -				  &status);
> -		retries--;
> -	} while (ret && retries);
> -	if (ret)
> -		dev_err(dev, "failed to read status (%d)\n", ret);
> -
> -	ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> -				 SII902X_SYS_CTRL_DDC_BUS_REQ |
> -				 SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> -	if (ret)
> -		return ret;
> -
> -	timeout = jiffies +
> -		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> -	do {
> -		ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> -		if (ret)
> -			return ret;
> -	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> -			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> -		 time_before(jiffies, timeout));
> -
> -	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> -		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> -		dev_err(dev, "failed to release the i2c bus\n");
> -		return -ETIMEDOUT;
> -	}
> -
>  	return num;
>  }
>  
> @@ -366,6 +363,112 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +/*
> + * The purpose of sii902x_i2c_bypass_select is to enable the pass through
> + * mode of the HDMI transmitter. Do not use regmap from within this function,
> + * only use sii902x_*_unlocked functions to read/modify/write registers.
> + * We are holding the parent adapter lock here, keep this in mind before
> + * adding more i2c transactions.
> + */
> +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> +{
> +	struct sii902x *sii902x = i2c_mux_priv(mux);
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned long timeout;
> +	u8 status;
> +	int ret;
> +
> +	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ);
> +
> +	timeout = jiffies +
> +		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		if (ret)
> +			return ret;
> +	} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +		 time_before(jiffies, timeout));
> +
> +	if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> +		dev_err(dev, "Failed to acquire the i2c bus\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	ret = sii902x_write_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +				     status);
> +	if (ret)
> +		return ret;
> +	return 0;
> +}
> +
> +/*
> + * The purpose of sii902x_i2c_bypass_deselect is to disable the pass through
> + * mode of the HDMI transmitter. Do not use regmap from within this function,
> + * only use sii902x_*_unlocked functions to read/modify/write registers.
> + * We are holding the parent adapter lock here, keep this in mind before
> + * adding more i2c transactions.
> + */
> +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> +{
> +	struct sii902x *sii902x = i2c_mux_priv(mux);
> +	struct device *dev = &sii902x->i2c->dev;
> +	unsigned long timeout;
> +	unsigned int retries;
> +	u8 status;
> +	int ret;
> +
> +	/*
> +	 * When the HDMI transmitter is in pass through mode, we need an
> +	 * (undocumented) additional delay between STOP and START conditions
> +	 * to guarantee the bus won't get stuck.
> +	 */
> +	udelay(30);
> +
> +	/*
> +	 * Sometimes the I2C bus can stall after failure to use the
> +	 * EDID channel. Retry a few times to see if things clear
> +	 * up, else continue anyway.
> +	 */
> +	retries = 5;
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		retries--;
> +	} while (ret && retries);
> +	if (ret) {
> +		dev_err(dev, "failed to read status (%d)\n", ret);
> +		return ret;
> +	}
> +
> +	ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					   SII902X_SYS_CTRL_DDC_BUS_REQ |
> +					   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> +	if (ret)
> +		return ret;
> +
> +	timeout = jiffies +
> +		  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> +	do {
> +		ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> +					    &status);
> +		if (ret)
> +			return ret;
> +	} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> +			   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> +		 time_before(jiffies, timeout));
> +
> +	if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> +		      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> +		dev_err(dev, "failed to release the i2c bus\n");
> +		return -ETIMEDOUT;
> +	}
> +
> +	return 0;
> +}
> +
>  static int sii902x_probe(struct i2c_client *client,
>  			 const struct i2c_device_id *id)
>  {
> @@ -375,6 +478,12 @@ static int sii902x_probe(struct i2c_client *client,
>  	u8 chipid[4];
>  	int ret;
>  
> +	ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);

And then I2C_FUNC_SMBUS_BYTE_DATA here, instead of I2C_FUNC_I2C, of course.

Cheers,
Peter

> +	if (!ret) {
> +		dev_err(dev, "I2C adapter not suitable\n");
> +		return -EIO;
> +	}
> +
>  	sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
>  	if (!sii902x)
>  		return -ENOMEM;
> @@ -433,6 +542,22 @@ static int sii902x_probe(struct i2c_client *client,
>  
>  	i2c_set_clientdata(client, sii902x);
>  
> +	sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> +					1, 0, I2C_MUX_GATE,
> +					sii902x_i2c_bypass_select,
> +					sii902x_i2c_bypass_deselect);
> +	if (!sii902x->i2cmux) {
> +		dev_err(dev, "failed to allocate I2C mux\n");
> +		return -ENOMEM;
> +	}
> +
> +	sii902x->i2cmux->priv = sii902x;
> +	ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> +	if (ret) {
> +		dev_err(dev, "Couldn't add i2c mux adapter\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
> @@ -441,6 +566,9 @@ static int sii902x_remove(struct i2c_client *client)
>  {
>  	struct sii902x *sii902x = i2c_get_clientdata(client);
>  
> +	if (sii902x->i2cmux)
> +		i2c_mux_del_adapters(sii902x->i2cmux);
> +
>  	drm_bridge_remove(&sii902x->bridge);
>  
>  	return 0;
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] drm/bridge/sii902x: Fix EDID readback
  2018-11-02 21:33   ` Peter Rosin
@ 2018-11-05 13:52     ` Fabrizio Castro
  -1 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-05 13:52 UTC (permalink / raw)
  To: Peter Rosin, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Chris Paterson, Geert Uytterhoeven, Boris Brezillon, dri-devel,
	Biju Das, linux-renesas-soc, Simon Horman, Laurent Pinchart,
	linux-i2c

Hello Peter,

Thank you for your feedback!

The changes you have suggested seem to be working just fine,
I have tested them this morning.

Thanks,
Fab

> Subject: Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
>
> On 2018-11-02 13:13, Fabrizio Castro wrote:
> > While adding SiI9022A support to the iwg23s board, it came
> > up that when the HDMI transmitter is in pass through mode the
> > device is not compliant with the I2C specification anymore,
> > as it requires a far bigger tbuf, due to a delay the HDMI
> > transmitter is adding when relaying the STOP condition on the
> > monitor i2c side of things.
> >
> > When not providing an appropriate delay after the STOP condition
> > the i2c bus would get stuck. Also, any other traffic on the bus
> > while talking to the monitor may cause the transaction to fail
> > or even cause issues with the i2c bus as well.
> >
> > I2c-gates seemed to reach consent as a possible way to address
> > these issues, and as such this patch is implementing a solution
> > based on that. Since others are clearly relying on the current
> > implementation of the driver, this patch won't require any DT
> > changes.
> >
> > Since we don't want any interference during the DDC Bus
> > Request/Grant procedure and while talking to the monitor, we
> > have to use the adapter locking primitives rather than the
> > i2c-mux locking primitives.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > ---
> > RFC->PATCH:
> > * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
> >
> > Thank you guys
> >
> >  drivers/gpu/drm/bridge/sii902x.c | 264 +++++++++++++++++++++++++++++----------
> >  1 file changed, 196 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> > index e59a135..4f9d9c7 100644
> > --- a/drivers/gpu/drm/bridge/sii902x.c
> > +++ b/drivers/gpu/drm/bridge/sii902x.c
> > @@ -1,4 +1,6 @@
> >  /*
> > + * Copyright (C) 2018 Renesas Electronics
> > + *
> >   * Copyright (C) 2016 Atmel
> >   *      Bo Shen <voice.shen@atmel.com>
> >   *
> > @@ -21,6 +23,7 @@
> >   */
> >
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/i2c-mux.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> >  #include <linux/regmap.h>
> > @@ -86,8 +89,68 @@ struct sii902x {
> >  struct drm_bridge bridge;
> >  struct drm_connector connector;
> >  struct gpio_desc *reset_gpio;
> > +struct i2c_mux_core *i2cmux;
> >  };
> >
> > +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> > +{
> > +struct i2c_msg xfer[] = {
> > +{
> > +.addr = i2c->addr,
> > +.flags = 0,
> > +.len = 1,
> > +.buf = &reg,
> > +}, {
> > +.addr = i2c->addr,
> > +.flags = I2C_M_RD,
> > +.len = 1,
> > +.buf = val,
> > +}
> > +};
> > +unsigned char xfers = ARRAY_SIZE(xfer);
> > +int ret, retries = 5;
> > +
> > +do {
> > +ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> > +if (ret < 0)
> > +return ret;
> > +} while (ret != xfers && --retries);
> > +return ret == xfers ? 0 : -1;
>
> New in 4.19 __i2c_smbus_xfer, and I think it helps here? (untested code)
>
> union i2c_smbus_data data;
> int ret;
>
> ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
>        I2C_SMBUS_READ, reg,
>        I2C_SMBUS_BYTE_DATA, &data);
>
> if (ret < 0)
> return ret;
>
> *val = data.byte;
> return 0;
>
> > +}
> > +
> > +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> > +{
> > +u8 data[2] = {reg, val};
> > +struct i2c_msg xfer = {
> > +.addr = i2c->addr,
> > +.flags = 0,
> > +.len = sizeof(data),
> > +.buf = data,
> > +};
> > +int ret, retries = 5;
> > +
> > +do {
> > +ret = __i2c_transfer(i2c->adapter, &xfer, 1);
> > +if (ret < 0)
> > +return ret;
> > +} while (!ret && --retries);
> > +return !ret ? -1 : 0;
>
> union i2c_smbus_data data;
>
> data.byte = val;
>
> return __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> I2C_SMBUS_WRITE, reg,
> I2C_SMBUS_BYTE_DATA, &data);
> > +}
> > +
> > +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8 mask,
> > +u8 val)
> > +{
> > +int ret;
> > +u8 status;
> > +
> > +ret = sii902x_read_unlocked(i2c, reg, &status);
> > +if (ret)
> > +return ret;
> > +status &= ~mask;
> > +status |= val & mask;
> > +return sii902x_write_unlocked(i2c, reg, status);
> > +}
> > +
> >  static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> >  {
> >  return container_of(bridge, struct sii902x, bridge);
> > @@ -135,41 +198,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
> >  static int sii902x_get_modes(struct drm_connector *connector)
> >  {
> >  struct sii902x *sii902x = connector_to_sii902x(connector);
> > -struct regmap *regmap = sii902x->regmap;
> >  u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > -struct device *dev = &sii902x->i2c->dev;
> > -unsigned long timeout;
> > -unsigned int retries;
> > -unsigned int status;
> >  struct edid *edid;
> > -int num = 0;
> > -int ret;
> > -
> > -ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> > - SII902X_SYS_CTRL_DDC_BUS_REQ,
> > - SII902X_SYS_CTRL_DDC_BUS_REQ);
> > -if (ret)
> > -return ret;
> > -
> > -timeout = jiffies +
> > -  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > -do {
> > -ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> > -if (ret)
> > -return ret;
> > -} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > - time_before(jiffies, timeout));
> > -
> > -if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > -dev_err(dev, "failed to acquire the i2c bus\n");
> > -return -ETIMEDOUT;
> > -}
> > -
> > -ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
> > -if (ret)
> > -return ret;
> > +int num = 0, ret;
> >
> > -edid = drm_get_edid(connector, sii902x->i2c->adapter);
> > +edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
> >  drm_connector_update_edid_property(connector, edid);
> >  if (edid) {
> >  num = drm_add_edid_modes(connector, edid);
> > @@ -181,42 +214,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
> >  if (ret)
> >  return ret;
> >
> > -/*
> > - * Sometimes the I2C bus can stall after failure to use the
> > - * EDID channel. Retry a few times to see if things clear
> > - * up, else continue anyway.
> > - */
> > -retries = 5;
> > -do {
> > -ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
> > -  &status);
> > -retries--;
> > -} while (ret && retries);
> > -if (ret)
> > -dev_err(dev, "failed to read status (%d)\n", ret);
> > -
> > -ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> > - SII902X_SYS_CTRL_DDC_BUS_REQ |
> > - SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > -if (ret)
> > -return ret;
> > -
> > -timeout = jiffies +
> > -  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > -do {
> > -ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> > -if (ret)
> > -return ret;
> > -} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > -   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > - time_before(jiffies, timeout));
> > -
> > -if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > -      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > -dev_err(dev, "failed to release the i2c bus\n");
> > -return -ETIMEDOUT;
> > -}
> > -
> >  return num;
> >  }
> >
> > @@ -366,6 +363,112 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
> >  return IRQ_HANDLED;
> >  }
> >
> > +/*
> > + * The purpose of sii902x_i2c_bypass_select is to enable the pass through
> > + * mode of the HDMI transmitter. Do not use regmap from within this function,
> > + * only use sii902x_*_unlocked functions to read/modify/write registers.
> > + * We are holding the parent adapter lock here, keep this in mind before
> > + * adding more i2c transactions.
> > + */
> > +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> > +{
> > +struct sii902x *sii902x = i2c_mux_priv(mux);
> > +struct device *dev = &sii902x->i2c->dev;
> > +unsigned long timeout;
> > +u8 status;
> > +int ret;
> > +
> > +ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ);
> > +
> > +timeout = jiffies +
> > +  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > +do {
> > +ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +    &status);
> > +if (ret)
> > +return ret;
> > +} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > + time_before(jiffies, timeout));
> > +
> > +if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > +dev_err(dev, "Failed to acquire the i2c bus\n");
> > +return -ETIMEDOUT;
> > +}
> > +
> > +ret = sii902x_write_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +     status);
> > +if (ret)
> > +return ret;
> > +return 0;
> > +}
> > +
> > +/*
> > + * The purpose of sii902x_i2c_bypass_deselect is to disable the pass through
> > + * mode of the HDMI transmitter. Do not use regmap from within this function,
> > + * only use sii902x_*_unlocked functions to read/modify/write registers.
> > + * We are holding the parent adapter lock here, keep this in mind before
> > + * adding more i2c transactions.
> > + */
> > +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> > +{
> > +struct sii902x *sii902x = i2c_mux_priv(mux);
> > +struct device *dev = &sii902x->i2c->dev;
> > +unsigned long timeout;
> > +unsigned int retries;
> > +u8 status;
> > +int ret;
> > +
> > +/*
> > + * When the HDMI transmitter is in pass through mode, we need an
> > + * (undocumented) additional delay between STOP and START conditions
> > + * to guarantee the bus won't get stuck.
> > + */
> > +udelay(30);
> > +
> > +/*
> > + * Sometimes the I2C bus can stall after failure to use the
> > + * EDID channel. Retry a few times to see if things clear
> > + * up, else continue anyway.
> > + */
> > +retries = 5;
> > +do {
> > +ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +    &status);
> > +retries--;
> > +} while (ret && retries);
> > +if (ret) {
> > +dev_err(dev, "failed to read status (%d)\n", ret);
> > +return ret;
> > +}
> > +
> > +ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > +if (ret)
> > +return ret;
> > +
> > +timeout = jiffies +
> > +  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > +do {
> > +ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +    &status);
> > +if (ret)
> > +return ret;
> > +} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > + time_before(jiffies, timeout));
> > +
> > +if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > +dev_err(dev, "failed to release the i2c bus\n");
> > +return -ETIMEDOUT;
> > +}
> > +
> > +return 0;
> > +}
> > +
> >  static int sii902x_probe(struct i2c_client *client,
> >   const struct i2c_device_id *id)
> >  {
> > @@ -375,6 +478,12 @@ static int sii902x_probe(struct i2c_client *client,
> >  u8 chipid[4];
> >  int ret;
> >
> > +ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
>
> And then I2C_FUNC_SMBUS_BYTE_DATA here, instead of I2C_FUNC_I2C, of course.
>
> Cheers,
> Peter
>
> > +if (!ret) {
> > +dev_err(dev, "I2C adapter not suitable\n");
> > +return -EIO;
> > +}
> > +
> >  sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
> >  if (!sii902x)
> >  return -ENOMEM;
> > @@ -433,6 +542,22 @@ static int sii902x_probe(struct i2c_client *client,
> >
> >  i2c_set_clientdata(client, sii902x);
> >
> > +sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> > +1, 0, I2C_MUX_GATE,
> > +sii902x_i2c_bypass_select,
> > +sii902x_i2c_bypass_deselect);
> > +if (!sii902x->i2cmux) {
> > +dev_err(dev, "failed to allocate I2C mux\n");
> > +return -ENOMEM;
> > +}
> > +
> > +sii902x->i2cmux->priv = sii902x;
> > +ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> > +if (ret) {
> > +dev_err(dev, "Couldn't add i2c mux adapter\n");
> > +return ret;
> > +}
> > +
> >  return 0;
> >  }
> >
> > @@ -441,6 +566,9 @@ static int sii902x_remove(struct i2c_client *client)
> >  {
> >  struct sii902x *sii902x = i2c_get_clientdata(client);
> >
> > +if (sii902x->i2cmux)
> > +i2c_mux_del_adapters(sii902x->i2cmux);
> > +
> >  drm_bridge_remove(&sii902x->bridge);
> >
> >  return 0;
> >




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-05 13:52     ` Fabrizio Castro
  0 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-05 13:52 UTC (permalink / raw)
  To: Peter Rosin, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Laurent Pinchart, dri-devel, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Biju Das, linux-renesas-soc, linux-i2c, Inki Dae,
	Boris Brezillon, Linus Walleij

Hello Peter,

Thank you for your feedback!

The changes you have suggested seem to be working just fine,
I have tested them this morning.

Thanks,
Fab

> Subject: Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
>
> On 2018-11-02 13:13, Fabrizio Castro wrote:
> > While adding SiI9022A support to the iwg23s board, it came
> > up that when the HDMI transmitter is in pass through mode the
> > device is not compliant with the I2C specification anymore,
> > as it requires a far bigger tbuf, due to a delay the HDMI
> > transmitter is adding when relaying the STOP condition on the
> > monitor i2c side of things.
> >
> > When not providing an appropriate delay after the STOP condition
> > the i2c bus would get stuck. Also, any other traffic on the bus
> > while talking to the monitor may cause the transaction to fail
> > or even cause issues with the i2c bus as well.
> >
> > I2c-gates seemed to reach consent as a possible way to address
> > these issues, and as such this patch is implementing a solution
> > based on that. Since others are clearly relying on the current
> > implementation of the driver, this patch won't require any DT
> > changes.
> >
> > Since we don't want any interference during the DDC Bus
> > Request/Grant procedure and while talking to the monitor, we
> > have to use the adapter locking primitives rather than the
> > i2c-mux locking primitives.
> >
> > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > ---
> > RFC->PATCH:
> > * Incorporated Linus Walleij, Peter Rosin, and Mark Brown comments
> >
> > Thank you guys
> >
> >  drivers/gpu/drm/bridge/sii902x.c | 264 +++++++++++++++++++++++++++++----------
> >  1 file changed, 196 insertions(+), 68 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> > index e59a135..4f9d9c7 100644
> > --- a/drivers/gpu/drm/bridge/sii902x.c
> > +++ b/drivers/gpu/drm/bridge/sii902x.c
> > @@ -1,4 +1,6 @@
> >  /*
> > + * Copyright (C) 2018 Renesas Electronics
> > + *
> >   * Copyright (C) 2016 Atmel
> >   *      Bo Shen <voice.shen@atmel.com>
> >   *
> > @@ -21,6 +23,7 @@
> >   */
> >
> >  #include <linux/gpio/consumer.h>
> > +#include <linux/i2c-mux.h>
> >  #include <linux/i2c.h>
> >  #include <linux/module.h>
> >  #include <linux/regmap.h>
> > @@ -86,8 +89,68 @@ struct sii902x {
> >  struct drm_bridge bridge;
> >  struct drm_connector connector;
> >  struct gpio_desc *reset_gpio;
> > +struct i2c_mux_core *i2cmux;
> >  };
> >
> > +static int sii902x_read_unlocked(struct i2c_client *i2c, u8 reg, u8 *val)
> > +{
> > +struct i2c_msg xfer[] = {
> > +{
> > +.addr = i2c->addr,
> > +.flags = 0,
> > +.len = 1,
> > +.buf = &reg,
> > +}, {
> > +.addr = i2c->addr,
> > +.flags = I2C_M_RD,
> > +.len = 1,
> > +.buf = val,
> > +}
> > +};
> > +unsigned char xfers = ARRAY_SIZE(xfer);
> > +int ret, retries = 5;
> > +
> > +do {
> > +ret = __i2c_transfer(i2c->adapter, xfer, xfers);
> > +if (ret < 0)
> > +return ret;
> > +} while (ret != xfers && --retries);
> > +return ret == xfers ? 0 : -1;
>
> New in 4.19 __i2c_smbus_xfer, and I think it helps here? (untested code)
>
> union i2c_smbus_data data;
> int ret;
>
> ret = __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
>        I2C_SMBUS_READ, reg,
>        I2C_SMBUS_BYTE_DATA, &data);
>
> if (ret < 0)
> return ret;
>
> *val = data.byte;
> return 0;
>
> > +}
> > +
> > +static int sii902x_write_unlocked(struct i2c_client *i2c, u8 reg, u8 val)
> > +{
> > +u8 data[2] = {reg, val};
> > +struct i2c_msg xfer = {
> > +.addr = i2c->addr,
> > +.flags = 0,
> > +.len = sizeof(data),
> > +.buf = data,
> > +};
> > +int ret, retries = 5;
> > +
> > +do {
> > +ret = __i2c_transfer(i2c->adapter, &xfer, 1);
> > +if (ret < 0)
> > +return ret;
> > +} while (!ret && --retries);
> > +return !ret ? -1 : 0;
>
> union i2c_smbus_data data;
>
> data.byte = val;
>
> return __i2c_smbus_xfer(i2c->adapter, i2c->addr, i2c->flags,
> I2C_SMBUS_WRITE, reg,
> I2C_SMBUS_BYTE_DATA, &data);
> > +}
> > +
> > +static int sii902x_update_bits_unlocked(struct i2c_client *i2c, u8 reg, u8 mask,
> > +u8 val)
> > +{
> > +int ret;
> > +u8 status;
> > +
> > +ret = sii902x_read_unlocked(i2c, reg, &status);
> > +if (ret)
> > +return ret;
> > +status &= ~mask;
> > +status |= val & mask;
> > +return sii902x_write_unlocked(i2c, reg, status);
> > +}
> > +
> >  static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge)
> >  {
> >  return container_of(bridge, struct sii902x, bridge);
> > @@ -135,41 +198,11 @@ static const struct drm_connector_funcs sii902x_connector_funcs = {
> >  static int sii902x_get_modes(struct drm_connector *connector)
> >  {
> >  struct sii902x *sii902x = connector_to_sii902x(connector);
> > -struct regmap *regmap = sii902x->regmap;
> >  u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> > -struct device *dev = &sii902x->i2c->dev;
> > -unsigned long timeout;
> > -unsigned int retries;
> > -unsigned int status;
> >  struct edid *edid;
> > -int num = 0;
> > -int ret;
> > -
> > -ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> > - SII902X_SYS_CTRL_DDC_BUS_REQ,
> > - SII902X_SYS_CTRL_DDC_BUS_REQ);
> > -if (ret)
> > -return ret;
> > -
> > -timeout = jiffies +
> > -  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > -do {
> > -ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> > -if (ret)
> > -return ret;
> > -} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > - time_before(jiffies, timeout));
> > -
> > -if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > -dev_err(dev, "failed to acquire the i2c bus\n");
> > -return -ETIMEDOUT;
> > -}
> > -
> > -ret = regmap_write(regmap, SII902X_SYS_CTRL_DATA, status);
> > -if (ret)
> > -return ret;
> > +int num = 0, ret;
> >
> > -edid = drm_get_edid(connector, sii902x->i2c->adapter);
> > +edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]);
> >  drm_connector_update_edid_property(connector, edid);
> >  if (edid) {
> >  num = drm_add_edid_modes(connector, edid);
> > @@ -181,42 +214,6 @@ static int sii902x_get_modes(struct drm_connector *connector)
> >  if (ret)
> >  return ret;
> >
> > -/*
> > - * Sometimes the I2C bus can stall after failure to use the
> > - * EDID channel. Retry a few times to see if things clear
> > - * up, else continue anyway.
> > - */
> > -retries = 5;
> > -do {
> > -ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA,
> > -  &status);
> > -retries--;
> > -} while (ret && retries);
> > -if (ret)
> > -dev_err(dev, "failed to read status (%d)\n", ret);
> > -
> > -ret = regmap_update_bits(regmap, SII902X_SYS_CTRL_DATA,
> > - SII902X_SYS_CTRL_DDC_BUS_REQ |
> > - SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > -if (ret)
> > -return ret;
> > -
> > -timeout = jiffies +
> > -  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > -do {
> > -ret = regmap_read(regmap, SII902X_SYS_CTRL_DATA, &status);
> > -if (ret)
> > -return ret;
> > -} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > -   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > - time_before(jiffies, timeout));
> > -
> > -if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > -      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > -dev_err(dev, "failed to release the i2c bus\n");
> > -return -ETIMEDOUT;
> > -}
> > -
> >  return num;
> >  }
> >
> > @@ -366,6 +363,112 @@ static irqreturn_t sii902x_interrupt(int irq, void *data)
> >  return IRQ_HANDLED;
> >  }
> >
> > +/*
> > + * The purpose of sii902x_i2c_bypass_select is to enable the pass through
> > + * mode of the HDMI transmitter. Do not use regmap from within this function,
> > + * only use sii902x_*_unlocked functions to read/modify/write registers.
> > + * We are holding the parent adapter lock here, keep this in mind before
> > + * adding more i2c transactions.
> > + */
> > +static int sii902x_i2c_bypass_select(struct i2c_mux_core *mux, u32 chan_id)
> > +{
> > +struct sii902x *sii902x = i2c_mux_priv(mux);
> > +struct device *dev = &sii902x->i2c->dev;
> > +unsigned long timeout;
> > +u8 status;
> > +int ret;
> > +
> > +ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ);
> > +
> > +timeout = jiffies +
> > +  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > +do {
> > +ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +    &status);
> > +if (ret)
> > +return ret;
> > +} while (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > + time_before(jiffies, timeout));
> > +
> > +if (!(status & SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > +dev_err(dev, "Failed to acquire the i2c bus\n");
> > +return -ETIMEDOUT;
> > +}
> > +
> > +ret = sii902x_write_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +     status);
> > +if (ret)
> > +return ret;
> > +return 0;
> > +}
> > +
> > +/*
> > + * The purpose of sii902x_i2c_bypass_deselect is to disable the pass through
> > + * mode of the HDMI transmitter. Do not use regmap from within this function,
> > + * only use sii902x_*_unlocked functions to read/modify/write registers.
> > + * We are holding the parent adapter lock here, keep this in mind before
> > + * adding more i2c transactions.
> > + */
> > +static int sii902x_i2c_bypass_deselect(struct i2c_mux_core *mux, u32 chan_id)
> > +{
> > +struct sii902x *sii902x = i2c_mux_priv(mux);
> > +struct device *dev = &sii902x->i2c->dev;
> > +unsigned long timeout;
> > +unsigned int retries;
> > +u8 status;
> > +int ret;
> > +
> > +/*
> > + * When the HDMI transmitter is in pass through mode, we need an
> > + * (undocumented) additional delay between STOP and START conditions
> > + * to guarantee the bus won't get stuck.
> > + */
> > +udelay(30);
> > +
> > +/*
> > + * Sometimes the I2C bus can stall after failure to use the
> > + * EDID channel. Retry a few times to see if things clear
> > + * up, else continue anyway.
> > + */
> > +retries = 5;
> > +do {
> > +ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +    &status);
> > +retries--;
> > +} while (ret && retries);
> > +if (ret) {
> > +dev_err(dev, "failed to read status (%d)\n", ret);
> > +return ret;
> > +}
> > +
> > +ret = sii902x_update_bits_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +   SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +   SII902X_SYS_CTRL_DDC_BUS_GRTD, 0);
> > +if (ret)
> > +return ret;
> > +
> > +timeout = jiffies +
> > +  msecs_to_jiffies(SII902X_I2C_BUS_ACQUISITION_TIMEOUT_MS);
> > +do {
> > +ret = sii902x_read_unlocked(sii902x->i2c, SII902X_SYS_CTRL_DATA,
> > +    &status);
> > +if (ret)
> > +return ret;
> > +} while (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +   SII902X_SYS_CTRL_DDC_BUS_GRTD) &&
> > + time_before(jiffies, timeout));
> > +
> > +if (status & (SII902X_SYS_CTRL_DDC_BUS_REQ |
> > +      SII902X_SYS_CTRL_DDC_BUS_GRTD)) {
> > +dev_err(dev, "failed to release the i2c bus\n");
> > +return -ETIMEDOUT;
> > +}
> > +
> > +return 0;
> > +}
> > +
> >  static int sii902x_probe(struct i2c_client *client,
> >   const struct i2c_device_id *id)
> >  {
> > @@ -375,6 +478,12 @@ static int sii902x_probe(struct i2c_client *client,
> >  u8 chipid[4];
> >  int ret;
> >
> > +ret = i2c_check_functionality(client->adapter, I2C_FUNC_I2C);
>
> And then I2C_FUNC_SMBUS_BYTE_DATA here, instead of I2C_FUNC_I2C, of course.
>
> Cheers,
> Peter
>
> > +if (!ret) {
> > +dev_err(dev, "I2C adapter not suitable\n");
> > +return -EIO;
> > +}
> > +
> >  sii902x = devm_kzalloc(dev, sizeof(*sii902x), GFP_KERNEL);
> >  if (!sii902x)
> >  return -ENOMEM;
> > @@ -433,6 +542,22 @@ static int sii902x_probe(struct i2c_client *client,
> >
> >  i2c_set_clientdata(client, sii902x);
> >
> > +sii902x->i2cmux = i2c_mux_alloc(client->adapter, dev,
> > +1, 0, I2C_MUX_GATE,
> > +sii902x_i2c_bypass_select,
> > +sii902x_i2c_bypass_deselect);
> > +if (!sii902x->i2cmux) {
> > +dev_err(dev, "failed to allocate I2C mux\n");
> > +return -ENOMEM;
> > +}
> > +
> > +sii902x->i2cmux->priv = sii902x;
> > +ret = i2c_mux_add_adapter(sii902x->i2cmux, 0, 0, 0);
> > +if (ret) {
> > +dev_err(dev, "Couldn't add i2c mux adapter\n");
> > +return ret;
> > +}
> > +
> >  return 0;
> >  }
> >
> > @@ -441,6 +566,9 @@ static int sii902x_remove(struct i2c_client *client)
> >  {
> >  struct sii902x *sii902x = i2c_get_clientdata(client);
> >
> > +if (sii902x->i2cmux)
> > +i2c_mux_del_adapters(sii902x->i2cmux);
> > +
> >  drm_bridge_remove(&sii902x->bridge);
> >
> >  return 0;
> >




Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] drm/bridge/sii902x: Fix EDID readback
  2018-11-02 21:16         ` Peter Rosin
@ 2018-11-05 13:53           ` Fabrizio Castro
  -1 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-05 13:53 UTC (permalink / raw)
  To: Peter Rosin, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Chris Paterson, Geert Uytterhoeven, Boris Brezillon, dri-devel,
	Biju Das, linux-renesas-soc, Simon Horman, Laurent Pinchart,
	linux-i2c

Hello Peter,

Thank you for your feedback!

> Subject: Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
>
snip
>
> If the things you write about the bits are true (haven't looked closely), I wouldn't
> add any regmap coordination. But if I was the maintainer, I'd like a comment about
> this so that the next contributor has a better chance to understand the subtlety.
>
> But I'm not the maintainer, so this is not my call, but by adding the comment you
> also clarify the situation for the maintainer, which is something that is likely
> to be appreciated.

I'll add comments then.

Cheers,
Fab

>
> Cheers,
> Peter



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: [PATCH] drm/bridge/sii902x: Fix EDID readback
@ 2018-11-05 13:53           ` Fabrizio Castro
  0 siblings, 0 replies; 16+ messages in thread
From: Fabrizio Castro @ 2018-11-05 13:53 UTC (permalink / raw)
  To: Peter Rosin, Archit Taneja, Andrzej Hajda, David Airlie,
	Wolfram Sang, Mark Brown
  Cc: Laurent Pinchart, dri-devel, Simon Horman, Geert Uytterhoeven,
	Chris Paterson, Biju Das, linux-renesas-soc, linux-i2c, Inki Dae,
	Boris Brezillon, Linus Walleij

Hello Peter,

Thank you for your feedback!

> Subject: Re: [PATCH] drm/bridge/sii902x: Fix EDID readback
>
snip
>
> If the things you write about the bits are true (haven't looked closely), I wouldn't
> add any regmap coordination. But if I was the maintainer, I'd like a comment about
> this so that the next contributor has a better chance to understand the subtlety.
>
> But I'm not the maintainer, so this is not my call, but by adding the comment you
> also clarify the situation for the maintainer, which is something that is likely
> to be appreciated.

I'll add comments then.

Cheers,
Fab

>
> Cheers,
> Peter



Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2018-11-05 23:12 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-02 12:13 [PATCH] drm/bridge/sii902x: Fix EDID readback Fabrizio Castro
2018-11-02 12:13 ` Fabrizio Castro
2018-11-02 12:26 ` Peter Rosin
2018-11-02 12:26   ` Peter Rosin
2018-11-02 15:18   ` Fabrizio Castro
2018-11-02 15:18     ` Fabrizio Castro
2018-11-02 16:16     ` Fabrizio Castro
2018-11-02 16:16       ` Fabrizio Castro
2018-11-02 21:16       ` Peter Rosin
2018-11-02 21:16         ` Peter Rosin
2018-11-05 13:53         ` Fabrizio Castro
2018-11-05 13:53           ` Fabrizio Castro
2018-11-02 21:33 ` Peter Rosin
2018-11-02 21:33   ` Peter Rosin
2018-11-05 13:52   ` Fabrizio Castro
2018-11-05 13:52     ` Fabrizio Castro

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.