| Message ID | 1540990667-14109-1-git-send-email-fabrizio.castro@bp.renesas.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | [RFC] drm/bridge/sii902x: Fix EDID readback | expand |
Hi Fabrizio, thanks for your patch! On Wed, Oct 31, 2018 at 1:58 PM Fabrizio Castro <fabrizio.castro@bp.renesas.com> 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, but in order to achieve that I had to get > rid of regmap. Why did you have to remove regmap? It is perfectly possible to override any reading/writing operations locally if you don't want to use the regmap i2c variants. The code in your patch does not make it evident to me exactly where the problem is with regmap, also I bet the regmap maintainer would love to hear about it so we can attempt to fix it there instead of locally in this driver. AFAICT there is some locked vs unlocked business and I strongly hope there is some way to simply pass that down into whatever functions regmap end up calling so we don't have to change all call sites. (So please include Mark Brown on CC in future iterations.) > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > --- > Dear All, > > this is a follow up to: > https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg32072.html > > Comments very welcome! At the very least I think the patch needs to be split in two, one dealing with the locking business and one dealing with the buffer size. As it looks now it is very hard to review. > > Thanks, > Fab > > drivers/gpu/drm/bridge/sii902x.c | 404 ++++++++++++++++++++++++++------------- > 1 file changed, 274 insertions(+), 130 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c > index e59a135..137a05a 100644 > --- a/drivers/gpu/drm/bridge/sii902x.c > +++ b/drivers/gpu/drm/bridge/sii902x.c > @@ -21,9 +21,9 @@ > */ > > #include <linux/gpio/consumer.h> > +#include <linux/i2c-mux.h> > #include <linux/i2c.h> > #include <linux/module.h> > -#include <linux/regmap.h> > > #include <drm/drmP.h> > #include <drm/drm_atomic_helper.h> > @@ -82,12 +82,130 @@ > > struct sii902x { > struct i2c_client *i2c; > - struct regmap *regmap; > struct drm_bridge bridge; > struct drm_connector connector; > struct gpio_desc *reset_gpio; > + struct i2c_mux_core *i2cmux; > }; > > +static int sii902x_transfer_read(struct i2c_client *i2c, u8 reg, u8 *val, > + u16 len, bool locked) > +{ > + struct i2c_msg xfer[] = { > + { > + .addr = i2c->addr, > + .flags = 0, > + .len = 1, > + .buf = ®, > + }, { > + .addr = i2c->addr, > + .flags = I2C_M_RD, > + .len = len, > + .buf = val, > + } > + }; > + unsigned char xfers = ARRAY_SIZE(xfer); > + int ret, retries = 5; > + > + do { > + if (locked) > + ret = i2c_transfer(i2c->adapter, xfer, xfers); > + else > + ret = __i2c_transfer(i2c->adapter, xfer, xfers); > + if (ret < 0) > + return ret; > + } while (ret != xfers && --retries); > + return ret == xfers ? 0 : -1; > +} > + > +static int sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len) > +{ > + return sii902x_transfer_read(i2c, reg, val, len, true); > +} > + > +static int __sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len) > +{ > + return sii902x_transfer_read(i2c, reg, val, len, false); > +} I'm not a fan of __functions because it is syntactically ambigous what that means. Example set_bit() vs __set_bit() - is it obvious from context that the latter is non-atomic? No. Give these functions names that indicate exactly what they do and why, please. *_locked and *_unlocked variants? > +static int sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val) > +{ > + return sii902x_bulk_read(i2c, reg, val, 1); > +} > + > +static int __sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val) > +{ > + return __sii902x_bulk_read(i2c, reg, val, 1); > +} Dito > +static int sii902x_transfer_write(struct i2c_client *i2c, u8 *val, > + u16 len, bool locked) > +{ > + struct i2c_msg xfer = { > + .addr = i2c->addr, > + .flags = 0, > + .len = len, > + .buf = val, > + }; > + int ret, retries = 5; > + > + do { > + if (locked) > + ret = i2c_transfer(i2c->adapter, &xfer, 1); > + else > + ret = __i2c_transfer(i2c->adapter, &xfer, 1); This locked variable seems to be the magic dust so please document it thorughly. > + if (ret < 0) > + return ret; > + } while (!ret && --retries); > + return !ret ? -1 : 0; > +} > + > +static int sii902x_bulk_write(struct i2c_client *i2c, u8 *val, u16 len) > +{ > + return sii902x_transfer_write(i2c, val, len, true); > +} > + > +static int sii902x_write(struct i2c_client *i2c, u8 reg, u8 val) > +{ > + u8 data[2] = {reg, val}; > + > + return sii902x_transfer_write(i2c, data, 2, true); > +} > + > +static int __sii902x_write(struct i2c_client *i2c, u8 reg, u8 val) > +{ > + u8 data[2] = {reg, val}; > + > + return sii902x_transfer_write(i2c, data, 2, false); > +} > + > +static int sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, u8 val) > +{ > + int ret; > + u8 status; > + > + ret = sii902x_read(i2c, reg, &status); > + if (ret) > + return ret; > + status &= ~mask; > + status |= val & mask; > + return sii902x_write(i2c, reg, status); > +} > + > +static int __sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, > + u8 val) > +{ > + int ret; > + u8 status; > + > + ret = __sii902x_read(i2c, reg, &status); > + if (ret) > + return ret; > + status &= ~mask; > + status |= val & mask; > + return __sii902x_write(i2c, reg, status); > +} Dito. So now all read, write and rmw functions are duplicated. > static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge) > { > return container_of(bridge, struct sii902x, bridge); > @@ -115,9 +233,9 @@ static enum drm_connector_status > sii902x_connector_detect(struct drm_connector *connector, bool force) > { > struct sii902x *sii902x = connector_to_sii902x(connector); > - unsigned int status; > + u8 status; > > - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); > + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); > > return (status & SII902X_PLUGGED_STATUS) ? > connector_status_connected : connector_status_disconnected; > @@ -135,41 +253,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; > + int num = 0, 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; > - > - 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 +269,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; > } > > @@ -237,20 +289,20 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge) > { > struct sii902x *sii902x = bridge_to_sii902x(bridge); > > - regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, > - SII902X_SYS_CTRL_PWR_DWN, > - SII902X_SYS_CTRL_PWR_DWN); > + sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA, > + SII902X_SYS_CTRL_PWR_DWN, > + SII902X_SYS_CTRL_PWR_DWN); > } > > static void sii902x_bridge_enable(struct drm_bridge *bridge) > { > struct sii902x *sii902x = bridge_to_sii902x(bridge); > > - regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL, > - SII902X_AVI_POWER_STATE_MSK, > - SII902X_AVI_POWER_STATE_D(0)); > - regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, > - SII902X_SYS_CTRL_PWR_DWN, 0); > + sii902x_update_bits(sii902x->i2c, SII902X_PWR_STATE_CTRL, > + SII902X_AVI_POWER_STATE_MSK, > + SII902X_AVI_POWER_STATE_D(0)); > + sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA, > + SII902X_SYS_CTRL_PWR_DWN, 0); > } > > static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > @@ -258,25 +310,25 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > struct drm_display_mode *adj) > { > struct sii902x *sii902x = bridge_to_sii902x(bridge); > - struct regmap *regmap = sii902x->regmap; > - u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; > + u8 buf[HDMI_INFOFRAME_SIZE(AVI) + 1]; Maybe this stuff should be a separate change? > struct hdmi_avi_infoframe frame; > int ret; > > - buf[0] = adj->clock; > - buf[1] = adj->clock >> 8; > - buf[2] = adj->vrefresh; > - buf[3] = 0x00; > - buf[4] = adj->hdisplay; > - buf[5] = adj->hdisplay >> 8; > - buf[6] = adj->vdisplay; > - buf[7] = adj->vdisplay >> 8; > - buf[8] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE | > + buf[0] = SII902X_TPI_VIDEO_DATA; > + buf[1] = adj->clock; > + buf[2] = adj->clock >> 8; > + buf[3] = adj->vrefresh; > + buf[4] = 0x00; > + buf[5] = adj->hdisplay; > + buf[6] = adj->hdisplay >> 8; > + buf[7] = adj->vdisplay; > + buf[8] = adj->vdisplay >> 8; > + buf[9] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE | > SII902X_TPI_AVI_PIXEL_REP_BUS_24BIT; > - buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO | > - SII902X_TPI_AVI_INPUT_COLORSPACE_RGB; > + buf[10] = SII902X_TPI_AVI_INPUT_RANGE_AUTO | > + SII902X_TPI_AVI_INPUT_COLORSPACE_RGB; > > - ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10); > + ret = sii902x_bulk_write(sii902x->i2c, buf, 11); > if (ret) > return; > > @@ -286,16 +338,17 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > return; > } > > - ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf)); > + ret = hdmi_avi_infoframe_pack(&frame, buf + 1, sizeof(buf) - 1); > if (ret < 0) { > DRM_ERROR("failed to pack AVI infoframe: %d\n", ret); > return; > } > > + buf[0] = SII902X_TPI_AVI_INFOFRAME; > /* Do not send the infoframe header, but keep the CRC field. */ > - regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME, > - buf + HDMI_INFOFRAME_HEADER_SIZE - 1, > - HDMI_AVI_INFOFRAME_SIZE + 1); > + sii902x_bulk_write(sii902x->i2c, > + buf + HDMI_INFOFRAME_HEADER_SIZE, > + HDMI_AVI_INFOFRAME_SIZE + 1); > } > > static int sii902x_bridge_attach(struct drm_bridge *bridge) > @@ -336,29 +389,13 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = { > .enable = sii902x_bridge_enable, > }; > > -static const struct regmap_range sii902x_volatile_ranges[] = { > - { .range_min = 0, .range_max = 0xff }, > -}; > - > -static const struct regmap_access_table sii902x_volatile_table = { > - .yes_ranges = sii902x_volatile_ranges, > - .n_yes_ranges = ARRAY_SIZE(sii902x_volatile_ranges), > -}; > - > -static const struct regmap_config sii902x_regmap_config = { > - .reg_bits = 8, > - .val_bits = 8, > - .volatile_table = &sii902x_volatile_table, > - .cache_type = REGCACHE_NONE, > -}; > - > static irqreturn_t sii902x_interrupt(int irq, void *data) > { > struct sii902x *sii902x = data; > - unsigned int status = 0; > + u8 status = 0; > > - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); > - regmap_write(sii902x->regmap, SII902X_INT_STATUS, status); > + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); > + sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status); > > if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) > drm_helper_hpd_irq_event(sii902x->bridge.dev); > @@ -366,23 +403,111 @@ static irqreturn_t sii902x_interrupt(int irq, void *data) > return IRQ_HANDLED; > } > > +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(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(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(sii902x->i2c, SII902X_SYS_CTRL_DATA, status); > + if (ret) > + return ret; > + return 0; > +} > + > +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; > + > + 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(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(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(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) > { > struct device *dev = &client->dev; > - unsigned int status = 0; > struct sii902x *sii902x; > - u8 chipid[4]; > + u8 chipid[4], status = 0; > 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; > > sii902x->i2c = client; > - sii902x->regmap = devm_regmap_init_i2c(client, &sii902x_regmap_config); > - if (IS_ERR(sii902x->regmap)) > - return PTR_ERR(sii902x->regmap); > > sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset", > GPIOD_OUT_LOW); > @@ -394,14 +519,14 @@ static int sii902x_probe(struct i2c_client *client, > > sii902x_reset(sii902x); > > - ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0); > + ret = sii902x_write(sii902x->i2c, SII902X_REG_TPI_RQB, 0x0); > if (ret) > return ret; > > - ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0), > - &chipid, 4); > + ret = sii902x_bulk_read(sii902x->i2c, SII902X_REG_CHIPID(0), > + chipid, 4); > if (ret) { > - dev_err(dev, "regmap_read failed %d\n", ret); > + dev_err(dev, "Can't read chipid (error = %d)\n", ret); > return ret; > } > > @@ -412,12 +537,12 @@ static int sii902x_probe(struct i2c_client *client, > } > > /* Clear all pending interrupts */ > - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); > - regmap_write(sii902x->regmap, SII902X_INT_STATUS, status); > + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); > + sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status); > > if (client->irq > 0) { > - regmap_write(sii902x->regmap, SII902X_INT_ENABLE, > - SII902X_HOTPLUG_EVENT); > + sii902x_write(sii902x->i2c, SII902X_INT_ENABLE, > + SII902X_HOTPLUG_EVENT); > > ret = devm_request_threaded_irq(dev, client->irq, NULL, > sii902x_interrupt, > @@ -433,6 +558,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 +582,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; Yours, Linus Walleij
Hello Linus, > Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback > > Hi Fabrizio, > > thanks for your patch! Thank you for your feedback! > > On Wed, Oct 31, 2018 at 1:58 PM Fabrizio Castro > <fabrizio.castro@bp.renesas.com> 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, but in order to achieve that I had to get > > rid of regmap. > > Why did you have to remove regmap? It is perfectly possible > to override any reading/writing operations locally if you don't > want to use the regmap i2c variants. > > The code in your patch does not make it evident to me exactly > where the problem is with regmap, also I bet the regmap > maintainer would love to hear about it so we can attempt to > fix it there instead of locally in this driver. > > AFAICT there is some locked vs unlocked business and I > strongly hope there is some way to simply pass that down > into whatever functions regmap end up calling so we don't > have to change all call sites. Yeah, there is some issue with locking here, and that's coming down from the fact that when we call into drm_get_edid, we implicitly call i2c_transfer which ultimately locks the i2c adapter, and then calls into our sii902x_i2c_bypass_select, which in turn calls into the regmap functions and therefore we try and lock the same i2c adapter again, driving the system into a deadlock. Having the option of using "unlocked" flavours of reads and writes is what we need here, but looking at drivers/base/regmap/regmap-i2c.c I couldn't find anything suitable for my case, maybe Mark could advise on this one? I am sure I overlooked something here, is there a better way to address this? > > (So please include Mark Brown on CC in future iterations.) > > > Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> > > --- > > Dear All, > > > > this is a follow up to: > > https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg32072.html > > > > Comments very welcome! > > At the very least I think the patch needs to be split in two, > one dealing with the locking business and one dealing with > the buffer size. As it looks now it is very hard to review. The change with the buffer size comes down from sii902x_bulk_write implementation, which btw is the only function that doesn't resembles its regmap equivalent, in terms of parameters. > > > > > Thanks, > > Fab > > > > drivers/gpu/drm/bridge/sii902x.c | 404 ++++++++++++++++++++++++++------------- > > 1 file changed, 274 insertions(+), 130 deletions(-) > > > > diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c > > index e59a135..137a05a 100644 > > --- a/drivers/gpu/drm/bridge/sii902x.c > > +++ b/drivers/gpu/drm/bridge/sii902x.c > > @@ -21,9 +21,9 @@ > > */ > > > > #include <linux/gpio/consumer.h> > > +#include <linux/i2c-mux.h> > > #include <linux/i2c.h> > > #include <linux/module.h> > > -#include <linux/regmap.h> > > > > #include <drm/drmP.h> > > #include <drm/drm_atomic_helper.h> > > @@ -82,12 +82,130 @@ > > > > struct sii902x { > > struct i2c_client *i2c; > > - struct regmap *regmap; > > struct drm_bridge bridge; > > struct drm_connector connector; > > struct gpio_desc *reset_gpio; > > + struct i2c_mux_core *i2cmux; > > }; > > > > +static int sii902x_transfer_read(struct i2c_client *i2c, u8 reg, u8 *val, > > + u16 len, bool locked) > > +{ > > + struct i2c_msg xfer[] = { > > + { > > + .addr = i2c->addr, > > + .flags = 0, > > + .len = 1, > > + .buf = ®, > > + }, { > > + .addr = i2c->addr, > > + .flags = I2C_M_RD, > > + .len = len, > > + .buf = val, > > + } > > + }; > > + unsigned char xfers = ARRAY_SIZE(xfer); > > + int ret, retries = 5; > > + > > + do { > > + if (locked) > > + ret = i2c_transfer(i2c->adapter, xfer, xfers); > > + else > > + ret = __i2c_transfer(i2c->adapter, xfer, xfers); > > + if (ret < 0) > > + return ret; > > + } while (ret != xfers && --retries); > > + return ret == xfers ? 0 : -1; > > +} > > + > > +static int sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len) > > +{ > > + return sii902x_transfer_read(i2c, reg, val, len, true); > > +} > > + > > +static int __sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len) > > +{ > > + return sii902x_transfer_read(i2c, reg, val, len, false); > > +} > > I'm not a fan of __functions because it is syntactically > ambigous what that means. > > Example set_bit() vs __set_bit() - is it obvious from context > that the latter is non-atomic? No. > > Give these functions names that indicate exactly what they > do and why, please. > > *_locked and *_unlocked variants? I agree, will do that > > > +static int sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val) > > +{ > > + return sii902x_bulk_read(i2c, reg, val, 1); > > +} > > + > > +static int __sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val) > > +{ > > + return __sii902x_bulk_read(i2c, reg, val, 1); > > +} > > Dito > > > +static int sii902x_transfer_write(struct i2c_client *i2c, u8 *val, > > + u16 len, bool locked) > > +{ > > + struct i2c_msg xfer = { > > + .addr = i2c->addr, > > + .flags = 0, > > + .len = len, > > + .buf = val, > > + }; > > + int ret, retries = 5; > > + > > + do { > > + if (locked) > > + ret = i2c_transfer(i2c->adapter, &xfer, 1); > > + else > > + ret = __i2c_transfer(i2c->adapter, &xfer, 1); > > This locked variable seems to be the magic dust so > please document it thorughly. Will do > > > + if (ret < 0) > > + return ret; > > + } while (!ret && --retries); > > + return !ret ? -1 : 0; > > +} > > + > > +static int sii902x_bulk_write(struct i2c_client *i2c, u8 *val, u16 len) > > +{ > > + return sii902x_transfer_write(i2c, val, len, true); > > +} > > + > > +static int sii902x_write(struct i2c_client *i2c, u8 reg, u8 val) > > +{ > > + u8 data[2] = {reg, val}; > > + > > + return sii902x_transfer_write(i2c, data, 2, true); > > +} > > + > > +static int __sii902x_write(struct i2c_client *i2c, u8 reg, u8 val) > > +{ > > + u8 data[2] = {reg, val}; > > + > > + return sii902x_transfer_write(i2c, data, 2, false); > > +} > > + > > +static int sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, u8 val) > > +{ > > + int ret; > > + u8 status; > > + > > + ret = sii902x_read(i2c, reg, &status); > > + if (ret) > > + return ret; > > + status &= ~mask; > > + status |= val & mask; > > + return sii902x_write(i2c, reg, status); > > +} > > + > > +static int __sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, > > + u8 val) > > +{ > > + int ret; > > + u8 status; > > + > > + ret = __sii902x_read(i2c, reg, &status); > > + if (ret) > > + return ret; > > + status &= ~mask; > > + status |= val & mask; > > + return __sii902x_write(i2c, reg, status); > > +} > > Dito. > > So now all read, write and rmw functions are duplicated. > > > static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge) > > { > > return container_of(bridge, struct sii902x, bridge); > > @@ -115,9 +233,9 @@ static enum drm_connector_status > > sii902x_connector_detect(struct drm_connector *connector, bool force) > > { > > struct sii902x *sii902x = connector_to_sii902x(connector); > > - unsigned int status; > > + u8 status; > > > > - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); > > + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); > > > > return (status & SII902X_PLUGGED_STATUS) ? > > connector_status_connected : connector_status_disconnected; > > @@ -135,41 +253,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; > > + int num = 0, 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; > > - > > - 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 +269,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; > > } > > > > @@ -237,20 +289,20 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge) > > { > > struct sii902x *sii902x = bridge_to_sii902x(bridge); > > > > - regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, > > - SII902X_SYS_CTRL_PWR_DWN, > > - SII902X_SYS_CTRL_PWR_DWN); > > + sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA, > > + SII902X_SYS_CTRL_PWR_DWN, > > + SII902X_SYS_CTRL_PWR_DWN); > > } > > > > static void sii902x_bridge_enable(struct drm_bridge *bridge) > > { > > struct sii902x *sii902x = bridge_to_sii902x(bridge); > > > > - regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL, > > - SII902X_AVI_POWER_STATE_MSK, > > - SII902X_AVI_POWER_STATE_D(0)); > > - regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, > > - SII902X_SYS_CTRL_PWR_DWN, 0); > > + sii902x_update_bits(sii902x->i2c, SII902X_PWR_STATE_CTRL, > > + SII902X_AVI_POWER_STATE_MSK, > > + SII902X_AVI_POWER_STATE_D(0)); > > + sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA, > > + SII902X_SYS_CTRL_PWR_DWN, 0); > > } > > > > static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > > @@ -258,25 +310,25 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > > struct drm_display_mode *adj) > > { > > struct sii902x *sii902x = bridge_to_sii902x(bridge); > > - struct regmap *regmap = sii902x->regmap; > > - u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; > > + u8 buf[HDMI_INFOFRAME_SIZE(AVI) + 1]; > > Maybe this stuff should be a separate change? This change is due to the fact that sii902x_bulk_write takes an array of u8, with the first element being the register address, this makes sii902x_bulk_write not exactly the same as regmap_bulk_write. Mark may point out something better regmap related, so maybe there is no even need for this. Again, thank you very much for your comments, I am not going to send a v2 out straight away, I would like to hear at least from Wolfram and Peter about the way I have implement the i2c-gate as it is a bit unconventional, and then it would be great to have Mark's opinion on the locking bit. Fab > > > struct hdmi_avi_infoframe frame; > > int ret; > > > > - buf[0] = adj->clock; > > - buf[1] = adj->clock >> 8; > > - buf[2] = adj->vrefresh; > > - buf[3] = 0x00; > > - buf[4] = adj->hdisplay; > > - buf[5] = adj->hdisplay >> 8; > > - buf[6] = adj->vdisplay; > > - buf[7] = adj->vdisplay >> 8; > > - buf[8] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE | > > + buf[0] = SII902X_TPI_VIDEO_DATA; > > + buf[1] = adj->clock; > > + buf[2] = adj->clock >> 8; > > + buf[3] = adj->vrefresh; > > + buf[4] = 0x00; > > + buf[5] = adj->hdisplay; > > + buf[6] = adj->hdisplay >> 8; > > + buf[7] = adj->vdisplay; > > + buf[8] = adj->vdisplay >> 8; > > + buf[9] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE | > > SII902X_TPI_AVI_PIXEL_REP_BUS_24BIT; > > - buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO | > > - SII902X_TPI_AVI_INPUT_COLORSPACE_RGB; > > + buf[10] = SII902X_TPI_AVI_INPUT_RANGE_AUTO | > > + SII902X_TPI_AVI_INPUT_COLORSPACE_RGB; > > > > - ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10); > > + ret = sii902x_bulk_write(sii902x->i2c, buf, 11); > > if (ret) > > return; > > > > @@ -286,16 +338,17 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, > > return; > > } > > > > - ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf)); > > + ret = hdmi_avi_infoframe_pack(&frame, buf + 1, sizeof(buf) - 1); > > if (ret < 0) { > > DRM_ERROR("failed to pack AVI infoframe: %d\n", ret); > > return; > > } > > > > + buf[0] = SII902X_TPI_AVI_INFOFRAME; > > /* Do not send the infoframe header, but keep the CRC field. */ > > - regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME, > > - buf + HDMI_INFOFRAME_HEADER_SIZE - 1, > > - HDMI_AVI_INFOFRAME_SIZE + 1); > > + sii902x_bulk_write(sii902x->i2c, > > + buf + HDMI_INFOFRAME_HEADER_SIZE, > > + HDMI_AVI_INFOFRAME_SIZE + 1); > > } > > > > static int sii902x_bridge_attach(struct drm_bridge *bridge) > > @@ -336,29 +389,13 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = { > > .enable = sii902x_bridge_enable, > > }; > > > > -static const struct regmap_range sii902x_volatile_ranges[] = { > > - { .range_min = 0, .range_max = 0xff }, > > -}; > > - > > -static const struct regmap_access_table sii902x_volatile_table = { > > - .yes_ranges = sii902x_volatile_ranges, > > - .n_yes_ranges = ARRAY_SIZE(sii902x_volatile_ranges), > > -}; > > - > > -static const struct regmap_config sii902x_regmap_config = { > > - .reg_bits = 8, > > - .val_bits = 8, > > - .volatile_table = &sii902x_volatile_table, > > - .cache_type = REGCACHE_NONE, > > -}; > > - > > static irqreturn_t sii902x_interrupt(int irq, void *data) > > { > > struct sii902x *sii902x = data; > > - unsigned int status = 0; > > + u8 status = 0; > > > > - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); > > - regmap_write(sii902x->regmap, SII902X_INT_STATUS, status); > > + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); > > + sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status); > > > > if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) > > drm_helper_hpd_irq_event(sii902x->bridge.dev); > > @@ -366,23 +403,111 @@ static irqreturn_t sii902x_interrupt(int irq, void *data) > > return IRQ_HANDLED; > > } > > > > +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(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(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(sii902x->i2c, SII902X_SYS_CTRL_DATA, status); > > + if (ret) > > + return ret; > > + return 0; > > +} > > + > > +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; > > + > > + 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(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(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(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) > > { > > struct device *dev = &client->dev; > > - unsigned int status = 0; > > struct sii902x *sii902x; > > - u8 chipid[4]; > > + u8 chipid[4], status = 0; > > 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; > > > > sii902x->i2c = client; > > - sii902x->regmap = devm_regmap_init_i2c(client, &sii902x_regmap_config); > > - if (IS_ERR(sii902x->regmap)) > > - return PTR_ERR(sii902x->regmap); > > > > sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset", > > GPIOD_OUT_LOW); > > @@ -394,14 +519,14 @@ static int sii902x_probe(struct i2c_client *client, > > > > sii902x_reset(sii902x); > > > > - ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0); > > + ret = sii902x_write(sii902x->i2c, SII902X_REG_TPI_RQB, 0x0); > > if (ret) > > return ret; > > > > - ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0), > > - &chipid, 4); > > + ret = sii902x_bulk_read(sii902x->i2c, SII902X_REG_CHIPID(0), > > + chipid, 4); > > if (ret) { > > - dev_err(dev, "regmap_read failed %d\n", ret); > > + dev_err(dev, "Can't read chipid (error = %d)\n", ret); > > return ret; > > } > > > > @@ -412,12 +537,12 @@ static int sii902x_probe(struct i2c_client *client, > > } > > > > /* Clear all pending interrupts */ > > - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); > > - regmap_write(sii902x->regmap, SII902X_INT_STATUS, status); > > + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); > > + sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status); > > > > if (client->irq > 0) { > > - regmap_write(sii902x->regmap, SII902X_INT_ENABLE, > > - SII902X_HOTPLUG_EVENT); > > + sii902x_write(sii902x->i2c, SII902X_INT_ENABLE, > > + SII902X_HOTPLUG_EVENT); > > > > ret = devm_request_threaded_irq(dev, client->irq, NULL, > > sii902x_interrupt, > > @@ -433,6 +558,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 +582,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; > > Yours, > Linus Walleij Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
On 2018-10-31 17:55, Fabrizio Castro wrote: > Hello Linus, > >> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback >> >> Hi Fabrizio, >> >> thanks for your patch! > > Thank you for your feedback! > >> >> On Wed, Oct 31, 2018 at 1:58 PM Fabrizio Castro >> <fabrizio.castro@bp.renesas.com> 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, but in order to achieve that I had to get >>> rid of regmap. >> >> Why did you have to remove regmap? It is perfectly possible >> to override any reading/writing operations locally if you don't >> want to use the regmap i2c variants. >> >> The code in your patch does not make it evident to me exactly >> where the problem is with regmap, also I bet the regmap >> maintainer would love to hear about it so we can attempt to >> fix it there instead of locally in this driver. >> >> AFAICT there is some locked vs unlocked business and I >> strongly hope there is some way to simply pass that down >> into whatever functions regmap end up calling so we don't >> have to change all call sites. > > Yeah, there is some issue with locking here, and that's coming down > from the fact that when we call into drm_get_edid, we implicitly call > i2c_transfer which ultimately locks the i2c adapter, and then calls > into our sii902x_i2c_bypass_select, which in turn calls into the regmap > functions and therefore we try and lock the same i2c adapter again, > driving the system into a deadlock. > Having the option of using "unlocked" flavours of reads and writes > is what we need here, but looking at drivers/base/regmap/regmap-i2c.c > I couldn't find anything suitable for my case, maybe Mark could advise > on this one? I am sure I overlooked something here, is there a better > way to address this? This recursive locking problem surfaces when an i2c mux is operated by writing commands on the same i2c bus that is muxed. When this happens for a typical i2c mux chip like nxp,pca9548 this can be kept local to that driver. However, when there are interactions with other parts of the kernel (e.g. if the i2c-mux is operated by gpio pins, and those gpio pins in turn are operated by accesses to the i2c bus that is muxed) this locked vs. unlocked thing would spread like wildfire. Since I did not like that wildfire, I came up with the "mux-locked" scheme. It is not appropriate everywhere, but in this case I think it is a perfect fit. By using it, you should be able to dodge all locking issues and it should be possible to keep the regmap usage as-is and the patch would in all likelihood be much less intrusive. For some background on "mux-locked" vs. "parent-locked" (which is what you have used in this RFC patch), see Documentation/i2c/i2c-topology. There are a couple of more comments below, inline. >> >> (So please include Mark Brown on CC in future iterations.) >> >>> Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> >>> --- >>> Dear All, >>> >>> this is a follow up to: >>> https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg32072.html >>> >>> Comments very welcome! >> >> At the very least I think the patch needs to be split in two, >> one dealing with the locking business and one dealing with >> the buffer size. As it looks now it is very hard to review. > > The change with the buffer size comes down from sii902x_bulk_write implementation, > which btw is the only function that doesn't resembles its regmap equivalent, in terms > of parameters. > >> >>> >>> Thanks, >>> Fab >>> >>> drivers/gpu/drm/bridge/sii902x.c | 404 ++++++++++++++++++++++++++------------- >>> 1 file changed, 274 insertions(+), 130 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c >>> index e59a135..137a05a 100644 >>> --- a/drivers/gpu/drm/bridge/sii902x.c >>> +++ b/drivers/gpu/drm/bridge/sii902x.c >>> @@ -21,9 +21,9 @@ >>> */ >>> >>> #include <linux/gpio/consumer.h> >>> +#include <linux/i2c-mux.h> >>> #include <linux/i2c.h> >>> #include <linux/module.h> >>> -#include <linux/regmap.h> >>> >>> #include <drm/drmP.h> >>> #include <drm/drm_atomic_helper.h> >>> @@ -82,12 +82,130 @@ >>> >>> struct sii902x { >>> struct i2c_client *i2c; >>> - struct regmap *regmap; >>> struct drm_bridge bridge; >>> struct drm_connector connector; >>> struct gpio_desc *reset_gpio; >>> + struct i2c_mux_core *i2cmux; >>> }; >>> >>> +static int sii902x_transfer_read(struct i2c_client *i2c, u8 reg, u8 *val, >>> + u16 len, bool locked) >>> +{ >>> + struct i2c_msg xfer[] = { >>> + { >>> + .addr = i2c->addr, >>> + .flags = 0, >>> + .len = 1, >>> + .buf = ®, >>> + }, { >>> + .addr = i2c->addr, >>> + .flags = I2C_M_RD, >>> + .len = len, >>> + .buf = val, >>> + } >>> + }; >>> + unsigned char xfers = ARRAY_SIZE(xfer); >>> + int ret, retries = 5; >>> + >>> + do { >>> + if (locked) >>> + ret = i2c_transfer(i2c->adapter, xfer, xfers); >>> + else >>> + ret = __i2c_transfer(i2c->adapter, xfer, xfers); >>> + if (ret < 0) >>> + return ret; >>> + } while (ret != xfers && --retries); >>> + return ret == xfers ? 0 : -1; >>> +} >>> + >>> +static int sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len) >>> +{ >>> + return sii902x_transfer_read(i2c, reg, val, len, true); >>> +} >>> + >>> +static int __sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len) >>> +{ >>> + return sii902x_transfer_read(i2c, reg, val, len, false); >>> +} >> >> I'm not a fan of __functions because it is syntactically >> ambigous what that means. >> >> Example set_bit() vs __set_bit() - is it obvious from context >> that the latter is non-atomic? No. >> >> Give these functions names that indicate exactly what they >> do and why, please. >> >> *_locked and *_unlocked variants? > > I agree, will do that > >> >>> +static int sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val) >>> +{ >>> + return sii902x_bulk_read(i2c, reg, val, 1); >>> +} >>> + >>> +static int __sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val) >>> +{ >>> + return __sii902x_bulk_read(i2c, reg, val, 1); >>> +} >> >> Dito >> >>> +static int sii902x_transfer_write(struct i2c_client *i2c, u8 *val, >>> + u16 len, bool locked) >>> +{ >>> + struct i2c_msg xfer = { >>> + .addr = i2c->addr, >>> + .flags = 0, >>> + .len = len, >>> + .buf = val, >>> + }; >>> + int ret, retries = 5; >>> + >>> + do { >>> + if (locked) >>> + ret = i2c_transfer(i2c->adapter, &xfer, 1); >>> + else >>> + ret = __i2c_transfer(i2c->adapter, &xfer, 1); >> >> This locked variable seems to be the magic dust so >> please document it thorughly. > > Will do > >> >>> + if (ret < 0) >>> + return ret; >>> + } while (!ret && --retries); >>> + return !ret ? -1 : 0; >>> +} >>> + >>> +static int sii902x_bulk_write(struct i2c_client *i2c, u8 *val, u16 len) >>> +{ >>> + return sii902x_transfer_write(i2c, val, len, true); >>> +} >>> + >>> +static int sii902x_write(struct i2c_client *i2c, u8 reg, u8 val) >>> +{ >>> + u8 data[2] = {reg, val}; >>> + >>> + return sii902x_transfer_write(i2c, data, 2, true); >>> +} >>> + >>> +static int __sii902x_write(struct i2c_client *i2c, u8 reg, u8 val) >>> +{ >>> + u8 data[2] = {reg, val}; >>> + >>> + return sii902x_transfer_write(i2c, data, 2, false); >>> +} >>> + >>> +static int sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, u8 val) >>> +{ >>> + int ret; >>> + u8 status; >>> + >>> + ret = sii902x_read(i2c, reg, &status); >>> + if (ret) >>> + return ret; >>> + status &= ~mask; >>> + status |= val & mask; >>> + return sii902x_write(i2c, reg, status); >>> +} >>> + >>> +static int __sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, >>> + u8 val) >>> +{ >>> + int ret; >>> + u8 status; >>> + >>> + ret = __sii902x_read(i2c, reg, &status); >>> + if (ret) >>> + return ret; >>> + status &= ~mask; >>> + status |= val & mask; >>> + return __sii902x_write(i2c, reg, status); >>> +} >> >> Dito. >> >> So now all read, write and rmw functions are duplicated. >> >>> static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge) >>> { >>> return container_of(bridge, struct sii902x, bridge); >>> @@ -115,9 +233,9 @@ static enum drm_connector_status >>> sii902x_connector_detect(struct drm_connector *connector, bool force) >>> { >>> struct sii902x *sii902x = connector_to_sii902x(connector); >>> - unsigned int status; >>> + u8 status; >>> >>> - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); >>> + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); >>> >>> return (status & SII902X_PLUGGED_STATUS) ? >>> connector_status_connected : connector_status_disconnected; >>> @@ -135,41 +253,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; >>> + int num = 0, 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; >>> - >>> - 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 +269,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; >>> } >>> >>> @@ -237,20 +289,20 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge) >>> { >>> struct sii902x *sii902x = bridge_to_sii902x(bridge); >>> >>> - regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, >>> - SII902X_SYS_CTRL_PWR_DWN, >>> - SII902X_SYS_CTRL_PWR_DWN); >>> + sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA, >>> + SII902X_SYS_CTRL_PWR_DWN, >>> + SII902X_SYS_CTRL_PWR_DWN); >>> } >>> >>> static void sii902x_bridge_enable(struct drm_bridge *bridge) >>> { >>> struct sii902x *sii902x = bridge_to_sii902x(bridge); >>> >>> - regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL, >>> - SII902X_AVI_POWER_STATE_MSK, >>> - SII902X_AVI_POWER_STATE_D(0)); >>> - regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, >>> - SII902X_SYS_CTRL_PWR_DWN, 0); >>> + sii902x_update_bits(sii902x->i2c, SII902X_PWR_STATE_CTRL, >>> + SII902X_AVI_POWER_STATE_MSK, >>> + SII902X_AVI_POWER_STATE_D(0)); >>> + sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA, >>> + SII902X_SYS_CTRL_PWR_DWN, 0); >>> } >>> >>> static void sii902x_bridge_mode_set(struct drm_bridge *bridge, >>> @@ -258,25 +310,25 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, >>> struct drm_display_mode *adj) >>> { >>> struct sii902x *sii902x = bridge_to_sii902x(bridge); >>> - struct regmap *regmap = sii902x->regmap; >>> - u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; >>> + u8 buf[HDMI_INFOFRAME_SIZE(AVI) + 1]; >> >> Maybe this stuff should be a separate change? > > This change is due to the fact that sii902x_bulk_write takes an array of u8, with the first element > being the register address, this makes sii902x_bulk_write not exactly the same as regmap_bulk_write. > Mark may point out something better regmap related, so maybe there is no even need for this. > > Again, thank you very much for your comments, I am not going to send a v2 out straight away, I would > like to hear at least from Wolfram and Peter about the way I have implement the i2c-gate as it is a bit > unconventional, and then it would be great to have Mark's opinion on the locking bit. > > Fab > >> >>> struct hdmi_avi_infoframe frame; >>> int ret; >>> >>> - buf[0] = adj->clock; >>> - buf[1] = adj->clock >> 8; >>> - buf[2] = adj->vrefresh; >>> - buf[3] = 0x00; >>> - buf[4] = adj->hdisplay; >>> - buf[5] = adj->hdisplay >> 8; >>> - buf[6] = adj->vdisplay; >>> - buf[7] = adj->vdisplay >> 8; >>> - buf[8] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE | >>> + buf[0] = SII902X_TPI_VIDEO_DATA; >>> + buf[1] = adj->clock; >>> + buf[2] = adj->clock >> 8; >>> + buf[3] = adj->vrefresh; >>> + buf[4] = 0x00; >>> + buf[5] = adj->hdisplay; >>> + buf[6] = adj->hdisplay >> 8; >>> + buf[7] = adj->vdisplay; >>> + buf[8] = adj->vdisplay >> 8; >>> + buf[9] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE | >>> SII902X_TPI_AVI_PIXEL_REP_BUS_24BIT; >>> - buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO | >>> - SII902X_TPI_AVI_INPUT_COLORSPACE_RGB; >>> + buf[10] = SII902X_TPI_AVI_INPUT_RANGE_AUTO | >>> + SII902X_TPI_AVI_INPUT_COLORSPACE_RGB; >>> >>> - ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10); >>> + ret = sii902x_bulk_write(sii902x->i2c, buf, 11); >>> if (ret) >>> return; >>> >>> @@ -286,16 +338,17 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, >>> return; >>> } >>> >>> - ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf)); >>> + ret = hdmi_avi_infoframe_pack(&frame, buf + 1, sizeof(buf) - 1); >>> if (ret < 0) { >>> DRM_ERROR("failed to pack AVI infoframe: %d\n", ret); >>> return; >>> } >>> >>> + buf[0] = SII902X_TPI_AVI_INFOFRAME; >>> /* Do not send the infoframe header, but keep the CRC field. */ >>> - regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME, >>> - buf + HDMI_INFOFRAME_HEADER_SIZE - 1, >>> - HDMI_AVI_INFOFRAME_SIZE + 1); >>> + sii902x_bulk_write(sii902x->i2c, >>> + buf + HDMI_INFOFRAME_HEADER_SIZE, >>> + HDMI_AVI_INFOFRAME_SIZE + 1); >>> } >>> >>> static int sii902x_bridge_attach(struct drm_bridge *bridge) >>> @@ -336,29 +389,13 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = { >>> .enable = sii902x_bridge_enable, >>> }; >>> >>> -static const struct regmap_range sii902x_volatile_ranges[] = { >>> - { .range_min = 0, .range_max = 0xff }, >>> -}; >>> - >>> -static const struct regmap_access_table sii902x_volatile_table = { >>> - .yes_ranges = sii902x_volatile_ranges, >>> - .n_yes_ranges = ARRAY_SIZE(sii902x_volatile_ranges), >>> -}; >>> - >>> -static const struct regmap_config sii902x_regmap_config = { >>> - .reg_bits = 8, >>> - .val_bits = 8, >>> - .volatile_table = &sii902x_volatile_table, >>> - .cache_type = REGCACHE_NONE, >>> -}; >>> - >>> static irqreturn_t sii902x_interrupt(int irq, void *data) >>> { >>> struct sii902x *sii902x = data; >>> - unsigned int status = 0; >>> + u8 status = 0; >>> >>> - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); >>> - regmap_write(sii902x->regmap, SII902X_INT_STATUS, status); >>> + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); >>> + sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status); >>> >>> if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) >>> drm_helper_hpd_irq_event(sii902x->bridge.dev); >>> @@ -366,23 +403,111 @@ static irqreturn_t sii902x_interrupt(int irq, void *data) >>> return IRQ_HANDLED; >>> } >>> >>> +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(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(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(sii902x->i2c, SII902X_SYS_CTRL_DATA, status); >>> + if (ret) >>> + return ret; Why do I not see a delay here? I thought the whole point of adding the i2c gate was that it enabled the introduction of a delay between opening for edid reading and the actual edid reading? >>> + return 0; >>> +} >>> + >>> +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; >>> + >>> + 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(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(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(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) >>> { >>> struct device *dev = &client->dev; >>> - unsigned int status = 0; >>> struct sii902x *sii902x; >>> - u8 chipid[4]; >>> + u8 chipid[4], status = 0; >>> 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; >>> >>> sii902x->i2c = client; >>> - sii902x->regmap = devm_regmap_init_i2c(client, &sii902x_regmap_config); >>> - if (IS_ERR(sii902x->regmap)) >>> - return PTR_ERR(sii902x->regmap); >>> >>> sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset", >>> GPIOD_OUT_LOW); >>> @@ -394,14 +519,14 @@ static int sii902x_probe(struct i2c_client *client, >>> >>> sii902x_reset(sii902x); >>> >>> - ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0); >>> + ret = sii902x_write(sii902x->i2c, SII902X_REG_TPI_RQB, 0x0); >>> if (ret) >>> return ret; >>> >>> - ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0), >>> - &chipid, 4); >>> + ret = sii902x_bulk_read(sii902x->i2c, SII902X_REG_CHIPID(0), >>> + chipid, 4); >>> if (ret) { >>> - dev_err(dev, "regmap_read failed %d\n", ret); >>> + dev_err(dev, "Can't read chipid (error = %d)\n", ret); >>> return ret; >>> } >>> >>> @@ -412,12 +537,12 @@ static int sii902x_probe(struct i2c_client *client, >>> } >>> >>> /* Clear all pending interrupts */ >>> - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); >>> - regmap_write(sii902x->regmap, SII902X_INT_STATUS, status); >>> + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); >>> + sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status); >>> >>> if (client->irq > 0) { >>> - regmap_write(sii902x->regmap, SII902X_INT_ENABLE, >>> - SII902X_HOTPLUG_EVENT); >>> + sii902x_write(sii902x->i2c, SII902X_INT_ENABLE, >>> + SII902X_HOTPLUG_EVENT); >>> >>> ret = devm_request_threaded_irq(dev, client->irq, NULL, >>> sii902x_interrupt, >>> @@ -433,6 +558,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, Just use I2C_MUX_GATE | I2C_MUX_LOCKED for the flags argument, and you should be able to make normal i2c accesses. Cheers, Peter >>> + 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 +582,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; >> >> Yours, >> Linus Walleij > > > > Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709. >
Hello Peter, Thank you for your feedback! snip > > Yeah, there is some issue with locking here, and that's coming down > > from the fact that when we call into drm_get_edid, we implicitly call > > i2c_transfer which ultimately locks the i2c adapter, and then calls > > into our sii902x_i2c_bypass_select, which in turn calls into the regmap > > functions and therefore we try and lock the same i2c adapter again, > > driving the system into a deadlock. > > Having the option of using "unlocked" flavours of reads and writes > > is what we need here, but looking at drivers/base/regmap/regmap-i2c.c > > I couldn't find anything suitable for my case, maybe Mark could advise > > on this one? I am sure I overlooked something here, is there a better > > way to address this? > > This recursive locking problem surfaces when an i2c mux is operated > by writing commands on the same i2c bus that is muxed. When this > happens for a typical i2c mux chip like nxp,pca9548 this can be kept > local to that driver. However, when there are interactions with other > parts of the kernel (e.g. if the i2c-mux is operated by gpio pins, > and those gpio pins in turn are operated by accesses to the i2c bus > that is muxed) this locked vs. unlocked thing would spread like > wildfire. > > Since I did not like that wildfire, I came up with the "mux-locked" > scheme. It is not appropriate everywhere, but in this case I think it > is a perfect fit. By using it, you should be able to dodge all locking > issues and it should be possible to keep the regmap usage as-is and the > patch would in all likelihood be much less intrusive. > > For some background on "mux-locked" vs. "parent-locked" (which is what > you have used in this RFC patch), see Documentation/i2c/i2c-topology. The "mux-locked" implementation was the one I first tried and I discovered it doesn't work for me, as other traffic on the parent adapter can get in the way. What we need for this device is no other traffic on the bus during the "select transaction deselect" procedure, that's why I went with the parent locking. Also this device needs a delay between stop and start conditions while addressing the monitor. > > There are a couple of more comments below, inline. > snip > >>> > >>> +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(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(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(sii902x->i2c, SII902X_SYS_CTRL_DATA, status); > >>> + if (ret) > >>> + return ret; > > Why do I not see a delay here? I thought the whole point of adding the i2c gate > was that it enabled the introduction of a delay between opening for edid reading > and the actual edid reading? The delay is needed between STOP and START condition while in passthrough mode. __i2c_transfer gets called after the select callback (which enables the passthrough mode), when __i2c_transfer is done it generates a STOP condition and then we call into the deselect callback. We need a delay between __i2c_transfer and deselect. > > >>> + return 0; > >>> +} > >>> + > >>> +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; > >>> + > >>> + udelay(30); Here is where we need the delay snip > >>> @@ -433,6 +558,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, > > Just use I2C_MUX_GATE | I2C_MUX_LOCKED for the flags argument, and you should be > able to make normal i2c accesses. This is precisely what I tried in the first place, but by generating traffic on the parent adapter (since I don't have any other slave on the same i2c bus I have been using i2cdetect) while talking to the monitor I have been able to break communications, and sometimes stall the bus, therefore I have taken out I2C_MUX_LOCKED. Ah, do you think the following is going to cause any issue in the future? -edid = drm_get_edid(connector, sii902x->i2c->adapter); +edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]); Thanks, 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.
On Wed, Oct 31, 2018 at 04:55:53PM +0000, Fabrizio Castro wrote: > Having the option of using "unlocked" flavours of reads and writes > is what we need here, but looking at drivers/base/regmap/regmap-i2c.c > I couldn't find anything suitable for my case, maybe Mark could advise > on this one? I am sure I overlooked something here, is there a better > way to address this? As Linus said you can go as far as writing your own read and write functions if you want to. That seems to be the easiest thing, though I am suspicous that you're having to use the I2C framework in this way - it doesn't sound terribly clever. You could I guess also just have a custom function for whatever register is doing all this stuff that just ignores regmap and bodge things that way while using regmap as normal for everything else.
On 2018-11-01 12:04, Fabrizio Castro wrote: > Hello Peter, > > Thank you for your feedback! > > snip > >>> Yeah, there is some issue with locking here, and that's coming down >>> from the fact that when we call into drm_get_edid, we implicitly call >>> i2c_transfer which ultimately locks the i2c adapter, and then calls >>> into our sii902x_i2c_bypass_select, which in turn calls into the regmap >>> functions and therefore we try and lock the same i2c adapter again, >>> driving the system into a deadlock. >>> Having the option of using "unlocked" flavours of reads and writes >>> is what we need here, but looking at drivers/base/regmap/regmap-i2c.c >>> I couldn't find anything suitable for my case, maybe Mark could advise >>> on this one? I am sure I overlooked something here, is there a better >>> way to address this? >> >> This recursive locking problem surfaces when an i2c mux is operated >> by writing commands on the same i2c bus that is muxed. When this >> happens for a typical i2c mux chip like nxp,pca9548 this can be kept >> local to that driver. However, when there are interactions with other >> parts of the kernel (e.g. if the i2c-mux is operated by gpio pins, >> and those gpio pins in turn are operated by accesses to the i2c bus >> that is muxed) this locked vs. unlocked thing would spread like >> wildfire. >> >> Since I did not like that wildfire, I came up with the "mux-locked" >> scheme. It is not appropriate everywhere, but in this case I think it >> is a perfect fit. By using it, you should be able to dodge all locking >> issues and it should be possible to keep the regmap usage as-is and the >> patch would in all likelihood be much less intrusive. >> >> For some background on "mux-locked" vs. "parent-locked" (which is what >> you have used in this RFC patch), see Documentation/i2c/i2c-topology. > > The "mux-locked" implementation was the one I first tried and I discovered > it doesn't work for me, as other traffic on the parent adapter can get in the > way. What we need for this device is no other traffic on the bus during the > "select transaction deselect" procedure, that's why I went with the parent > locking. Also this device needs a delay between stop and start conditions > while addressing the monitor. Ok, I thought the problem was that a delay was needed between the STOP of the command opening the gate and the START of the edid eeprom xfer, and that everything else worked normally. Too bad this wasn't the actual problem. Hmm. If it is indeed true that other xfers must never creep into the "select xfer deselect" procedure then you are indeed stuck with a parent-locking. But is that really the case? Could it be that the extra delay between STOP-START is only needed after the absolutely last xfer before the command closing the gate? If that problem description is correct, it should be possible to go back to a mux-locked gate, but then in your deselect implementation grab the i2c adapter lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before the 30 us delay, then open code the command to close the gate with an unlocked i2c access, and finally release the i2c bus lock. That way you have ensured silence on the bus for the required time before closing the gate. You would still need to bypass regmap, but only in this one place (but maybe you should bypass regmap for opening the gate too, for symmetry). Cheers, Peter >> >> There are a couple of more comments below, inline. >> > > snip > >>>>> >>>>> +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(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(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(sii902x->i2c, SII902X_SYS_CTRL_DATA, status); >>>>> + if (ret) >>>>> + return ret; >> >> Why do I not see a delay here? I thought the whole point of adding the i2c gate >> was that it enabled the introduction of a delay between opening for edid reading >> and the actual edid reading? > > The delay is needed between STOP and START condition while in passthrough mode. > __i2c_transfer gets called after the select callback (which enables the passthrough > mode), when __i2c_transfer is done it generates a STOP condition and then we call into > the deselect callback. We need a delay between __i2c_transfer and deselect. > >> >>>>> + return 0; >>>>> +} >>>>> + >>>>> +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; >>>>> + >>>>> + udelay(30); > > Here is where we need the delay > > snip > >>>>> @@ -433,6 +558,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, >> >> Just use I2C_MUX_GATE | I2C_MUX_LOCKED for the flags argument, and you should be >> able to make normal i2c accesses. > > This is precisely what I tried in the first place, but by generating traffic on the parent adapter > (since I don't have any other slave on the same i2c bus I have been using i2cdetect) while talking > to the monitor I have been able to break communications, and sometimes stall the bus, therefore > I have taken out I2C_MUX_LOCKED. > > Ah, do you think the following is going to cause any issue in the future? > -edid = drm_get_edid(connector, sii902x->i2c->adapter); > +edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]); No, that was a critical part of the idea to use a gate to introduce the needed delay. Cheers, Peter
Hello Peter, Thank you for your feedback! > > The "mux-locked" implementation was the one I first tried and I discovered > > it doesn't work for me, as other traffic on the parent adapter can get in the > > way. What we need for this device is no other traffic on the bus during the > > "select transaction deselect" procedure, that's why I went with the parent > > locking. Also this device needs a delay between stop and start conditions > > while addressing the monitor. > > Ok, I thought the problem was that a delay was needed between the STOP > of the command opening the gate and the START of the edid eeprom xfer, and > that everything else worked normally. Too bad this wasn't the actual problem. > > Hmm. If it is indeed true that other xfers must never creep into the "select > xfer deselect" procedure then you are indeed stuck with a parent-locking. > But is that really the case? Could it be that the extra delay between > STOP-START is only needed after the absolutely last xfer before the > command closing the gate? > > If that problem description is correct, it should be possible to go back to > a mux-locked gate, but then in your deselect implementation grab the i2c adapter > lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before the > 30 us delay, then open code the command to close the gate with an unlocked i2c > access, and finally release the i2c bus lock. That way you have ensured silence > on the bus for the required time before closing the gate. You would still need > to bypass regmap, but only in this one place (but maybe you should bypass > regmap for opening the gate too, for symmetry). To further detail the problem, the system is vulnerable from before the last write performed by sii902x_i2c_bypass_select to after we confirm we need the switch to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of time we could keep the parent adapter locked for, I guess I am stuck with a parent-locking architecture, aren't I? But I guess I could release it when it's not actually needed, or is this going to be a pain to maintain? Shall I just keep going with the parent-locking but using bare i2c transactions only within select and deselect and leave regmap to deal with everything else? snip > > > > Ah, do you think the following is going to cause any issue in the future? > > -edid = drm_get_edid(connector, sii902x->i2c->adapter); > > +edid = drm_get_edid(connector, sii902x->i2cmux->adapter[0]); > > No, that was a critical part of the idea to use a gate to introduce the > needed delay. Again, thank you very much for spending your time looking into this, your feedback is very much appreciated. 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.
On 2018-11-01 17:04, Fabrizio Castro wrote: > Hello Peter, > > Thank you for your feedback! > >>> The "mux-locked" implementation was the one I first tried and I discovered >>> it doesn't work for me, as other traffic on the parent adapter can get in the >>> way. What we need for this device is no other traffic on the bus during the >>> "select transaction deselect" procedure, that's why I went with the parent >>> locking. Also this device needs a delay between stop and start conditions >>> while addressing the monitor. >> >> Ok, I thought the problem was that a delay was needed between the STOP >> of the command opening the gate and the START of the edid eeprom xfer, and >> that everything else worked normally. Too bad this wasn't the actual problem. >> >> Hmm. If it is indeed true that other xfers must never creep into the "select >> xfer deselect" procedure then you are indeed stuck with a parent-locking. >> But is that really the case? Could it be that the extra delay between >> STOP-START is only needed after the absolutely last xfer before the >> command closing the gate? >> >> If that problem description is correct, it should be possible to go back to >> a mux-locked gate, but then in your deselect implementation grab the i2c adapter >> lock manually - with i2c_lock_bus(adapter, I2C_LOCK_ROOT_ADAPTER) - before the >> 30 us delay, then open code the command to close the gate with an unlocked i2c >> access, and finally release the i2c bus lock. That way you have ensured silence >> on the bus for the required time before closing the gate. You would still need >> to bypass regmap, but only in this one place (but maybe you should bypass >> regmap for opening the gate too, for symmetry). > > To further detail the problem, the system is vulnerable from before the last write > performed by sii902x_i2c_bypass_select to after we confirm we need the switch > to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of time > we could keep the parent adapter locked for, I guess I am stuck with a parent-locking > architecture, aren't I? If that problem description is correct, then yes, I think the *only* solution is to combine the three parts "open bypass mode", "edid xfer" and "close bypass mode", and to keep the i2c adapter locked during the procedure so that other xfers do not creep in and crap thing up from the side. And one way to combine the three parts is to use a parent-locked i2c gate. And since you need to keep the i2c adapter locked over the whole procedure, you need to use unlocked xfers (as you have already discovered). But how do you know that this problem description is accurate? Why is it not ok for unrelated xfers to creep in between opening the bypass mode and the edid xfer, and how do you know that this is not ok? > But I guess I could release it when it's not actually needed, How would you figure out when it's not needed? > or is this going to be a pain to maintain? Shall I just keep going with the parent-locking > but using bare i2c transactions only within select and deselect and leave regmap > to deal with everything else? That's a possibility. Take care to not mess up any cached state in regmap though. The registers you touch from select/deselect should probably be volatile or something like that? Cheers, Peter
Hello Peter, Thank you for your feedback! > Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback snip > > > > To further detail the problem, the system is vulnerable from before the last write > > performed by sii902x_i2c_bypass_select to after we confirm we need the switch > > to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of time > > we could keep the parent adapter locked for, I guess I am stuck with a parent-locking > > architecture, aren't I? > > If that problem description is correct, then yes, I think the *only* solution > is to combine the three parts "open bypass mode", "edid xfer" and "close bypass > mode", and to keep the i2c adapter locked during the procedure so that other > xfers do not creep in and crap thing up from the side. And one way to combine > the three parts is to use a parent-locked i2c gate. And since you need to keep > the i2c adapter locked over the whole procedure, you need to use unlocked xfers > (as you have already discovered). But how do you know that this problem > description is accurate? I basically observed what was going on on the bus (with a logic analyser) while generating traffic on the parent adapter > Why is it not ok for unrelated xfers to creep in > between opening the bypass mode and the edid xfer, and how do you know that > this is not ok? Because those transfers would come with no extra delay between STOP and START conditions while the HDMI transmitter is in passthrough mode > > > But I guess I could release it when it's not actually needed, > > How would you figure out when it's not needed? The moment you tell the HDMI transmitter you are going to talk to the monitor nothing else can flow on the bus, up until you gracefully close the pass through session, which means I wouldn't really need to hold the parent lock during the entire duration of the select callback, I would need to hold the parent lock only from before the last write as that's when we tell the HDMI transmitter to activate the passthrough mode, but I guess it would make the driver hard to maintain (as in others would need to understand this properly before making any changes), wouldn't it? > > > or is this going to be a pain to maintain? Shall I just keep going with the parent-locking > > but using bare i2c transactions only within select and deselect and leave regmap > > to deal with everything else? > > That's a possibility. Take care to not mess up any cached state in regmap though. The original version of the driver wasn't using any caching, so I guess I would need to fallback exactly to the same implementation. So, what should I do? Should I keep the parent-locking, the unlocked flavours of rd/wr/rmw from within select/deselect, and put back regmap based rd/wr/rmw with no caching for everything else? Thank you! Fab > The registers you touch from select/deselect should probably be volatile or > something like that? > > Cheers, > Peter Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
On 2018-11-01 18:32, Fabrizio Castro wrote: > Hello Peter, > > Thank you for your feedback! > >> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback > > snip > >>> >>> To further detail the problem, the system is vulnerable from before the last write >>> performed by sii902x_i2c_bypass_select to after we confirm we need the switch >>> to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of time >>> we could keep the parent adapter locked for, I guess I am stuck with a parent-locking >>> architecture, aren't I? >> >> If that problem description is correct, then yes, I think the *only* solution >> is to combine the three parts "open bypass mode", "edid xfer" and "close bypass >> mode", and to keep the i2c adapter locked during the procedure so that other >> xfers do not creep in and crap thing up from the side. And one way to combine >> the three parts is to use a parent-locked i2c gate. And since you need to keep >> the i2c adapter locked over the whole procedure, you need to use unlocked xfers >> (as you have already discovered). But how do you know that this problem >> description is accurate? > > I basically observed what was going on on the bus (with a logic analyser) while generating > traffic on the parent adapter > >> Why is it not ok for unrelated xfers to creep in >> between opening the bypass mode and the edid xfer, and how do you know that >> this is not ok? > > Because those transfers would come with no extra delay between STOP and START > conditions while the HDMI transmitter is in passthrough mode Yeah yeah, now I get it. It's not the edid eeprom that's bad, it's the passthrough mode in the hdmi transmitter that's broken. Your problem description follows naturally. >> >>> But I guess I could release it when it's not actually needed, >> >> How would you figure out when it's not needed? > > The moment you tell the HDMI transmitter you are going to talk to the monitor nothing else can > flow on the bus, up until you gracefully close the pass through session, which means I wouldn't > really need to hold the parent lock during the entire duration of the select callback, I would need > to hold the parent lock only from before the last write as that's when we tell the HDMI transmitter > to activate the passthrough mode, but I guess it would make the driver hard to maintain (as in > others would need to understand this properly before making any changes), wouldn't it? Yes, that would just complicate things and would probably not have all that much benefit. I don't think I'd go there... >> >>> or is this going to be a pain to maintain? Shall I just keep going with the parent-locking >>> but using bare i2c transactions only within select and deselect and leave regmap >>> to deal with everything else? >> >> That's a possibility. Take care to not mess up any cached state in regmap though. > > The original version of the driver wasn't using any caching, so I guess I would need to fallback > exactly to the same implementation. > > So, what should I do? Should I keep the parent-locking, the unlocked flavours of rd/wr/rmw from > within select/deselect, and put back regmap based rd/wr/rmw with no caching for everything else? I think that sounds like a reasonable compromise, but be careful since you still might need the two implementations to interact, e.g. the two rmw variants might still need a common lock so that they don't trample on each others toes. At least if there are accesses to the same register (SII902X_SYS_CTRL_DATA in this case if I read it right). Cheers, Peter
Hello Mark, Thank you for your feedback! > Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback > > On Wed, Oct 31, 2018 at 04:55:53PM +0000, Fabrizio Castro wrote: > > > Having the option of using "unlocked" flavours of reads and writes > > is what we need here, but looking at drivers/base/regmap/regmap-i2c.c > > I couldn't find anything suitable for my case, maybe Mark could advise > > on this one? I am sure I overlooked something here, is there a better > > way to address this? > > As Linus said you can go as far as writing your own read and write > functions if you want to. That seems to be the easiest thing, though I > am suspicous that you're having to use the I2C framework in this way - > it doesn't sound terribly clever. You could I guess also just have a > custom function for whatever register is doing all this stuff that just > ignores regmap and bodge things that way while using regmap as normal > for everything else. I agree, I have prototyped it and it seems to be working just fine. Will send a patch addressing all of the comments, including this one. Fab Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
Hello Peter, Again, thank you very much for your precious comments. I'll send a patch out shortly addressing all of the comments I have received so far, including yours. Cheers, Fab > Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback > > On 2018-11-01 18:32, Fabrizio Castro wrote: > > Hello Peter, > > > > Thank you for your feedback! > > > >> Subject: Re: [RFC] drm/bridge/sii902x: Fix EDID readback > > > > snip > > > >>> > >>> To further detail the problem, the system is vulnerable from before the last write > >>> performed by sii902x_i2c_bypass_select to after we confirm we need the switch > >>> to be closed within sii902x_i2c_bypass_deselect, that's the minimum amount of time > >>> we could keep the parent adapter locked for, I guess I am stuck with a parent-locking > >>> architecture, aren't I? > >> > >> If that problem description is correct, then yes, I think the *only* solution > >> is to combine the three parts "open bypass mode", "edid xfer" and "close bypass > >> mode", and to keep the i2c adapter locked during the procedure so that other > >> xfers do not creep in and crap thing up from the side. And one way to combine > >> the three parts is to use a parent-locked i2c gate. And since you need to keep > >> the i2c adapter locked over the whole procedure, you need to use unlocked xfers > >> (as you have already discovered). But how do you know that this problem > >> description is accurate? > > > > I basically observed what was going on on the bus (with a logic analyser) while generating > > traffic on the parent adapter > > > >> Why is it not ok for unrelated xfers to creep in > >> between opening the bypass mode and the edid xfer, and how do you know that > >> this is not ok? > > > > Because those transfers would come with no extra delay between STOP and START > > conditions while the HDMI transmitter is in passthrough mode > > Yeah yeah, now I get it. It's not the edid eeprom that's bad, it's the > passthrough mode in the hdmi transmitter that's broken. Your problem > description follows naturally. > > >> > >>> But I guess I could release it when it's not actually needed, > >> > >> How would you figure out when it's not needed? > > > > The moment you tell the HDMI transmitter you are going to talk to the monitor nothing else can > > flow on the bus, up until you gracefully close the pass through session, which means I wouldn't > > really need to hold the parent lock during the entire duration of the select callback, I would need > > to hold the parent lock only from before the last write as that's when we tell the HDMI transmitter > > to activate the passthrough mode, but I guess it would make the driver hard to maintain (as in > > others would need to understand this properly before making any changes), wouldn't it? > > Yes, that would just complicate things and would probably not have all that > much benefit. I don't think I'd go there... > > >> > >>> or is this going to be a pain to maintain? Shall I just keep going with the parent-locking > >>> but using bare i2c transactions only within select and deselect and leave regmap > >>> to deal with everything else? > >> > >> That's a possibility. Take care to not mess up any cached state in regmap though. > > > > The original version of the driver wasn't using any caching, so I guess I would need to fallback > > exactly to the same implementation. > > > > So, what should I do? Should I keep the parent-locking, the unlocked flavours of rd/wr/rmw from > > within select/deselect, and put back regmap based rd/wr/rmw with no caching for everything else? > > I think that sounds like a reasonable compromise, but be careful since you still > might need the two implementations to interact, e.g. the two rmw variants might > still need a common lock so that they don't trample on each others toes. At > least if there are accesses to the same register (SII902X_SYS_CTRL_DATA in this > case if I read it right). > > Cheers, > Peter Renesas Electronics Europe Ltd, Dukes Meadow, Millboard Road, Bourne End, Buckinghamshire, SL8 5FH, UK. Registered in England & Wales under Registered No. 04586709.
diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c index e59a135..137a05a 100644 --- a/drivers/gpu/drm/bridge/sii902x.c +++ b/drivers/gpu/drm/bridge/sii902x.c @@ -21,9 +21,9 @@ */ #include <linux/gpio/consumer.h> +#include <linux/i2c-mux.h> #include <linux/i2c.h> #include <linux/module.h> -#include <linux/regmap.h> #include <drm/drmP.h> #include <drm/drm_atomic_helper.h> @@ -82,12 +82,130 @@ struct sii902x { struct i2c_client *i2c; - struct regmap *regmap; struct drm_bridge bridge; struct drm_connector connector; struct gpio_desc *reset_gpio; + struct i2c_mux_core *i2cmux; }; +static int sii902x_transfer_read(struct i2c_client *i2c, u8 reg, u8 *val, + u16 len, bool locked) +{ + struct i2c_msg xfer[] = { + { + .addr = i2c->addr, + .flags = 0, + .len = 1, + .buf = ®, + }, { + .addr = i2c->addr, + .flags = I2C_M_RD, + .len = len, + .buf = val, + } + }; + unsigned char xfers = ARRAY_SIZE(xfer); + int ret, retries = 5; + + do { + if (locked) + ret = i2c_transfer(i2c->adapter, xfer, xfers); + else + ret = __i2c_transfer(i2c->adapter, xfer, xfers); + if (ret < 0) + return ret; + } while (ret != xfers && --retries); + return ret == xfers ? 0 : -1; +} + +static int sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len) +{ + return sii902x_transfer_read(i2c, reg, val, len, true); +} + +static int __sii902x_bulk_read(struct i2c_client *i2c, u8 reg, u8 *val, u16 len) +{ + return sii902x_transfer_read(i2c, reg, val, len, false); +} + +static int sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val) +{ + return sii902x_bulk_read(i2c, reg, val, 1); +} + +static int __sii902x_read(struct i2c_client *i2c, u8 reg, u8 *val) +{ + return __sii902x_bulk_read(i2c, reg, val, 1); +} + +static int sii902x_transfer_write(struct i2c_client *i2c, u8 *val, + u16 len, bool locked) +{ + struct i2c_msg xfer = { + .addr = i2c->addr, + .flags = 0, + .len = len, + .buf = val, + }; + int ret, retries = 5; + + do { + if (locked) + ret = i2c_transfer(i2c->adapter, &xfer, 1); + else + ret = __i2c_transfer(i2c->adapter, &xfer, 1); + if (ret < 0) + return ret; + } while (!ret && --retries); + return !ret ? -1 : 0; +} + +static int sii902x_bulk_write(struct i2c_client *i2c, u8 *val, u16 len) +{ + return sii902x_transfer_write(i2c, val, len, true); +} + +static int sii902x_write(struct i2c_client *i2c, u8 reg, u8 val) +{ + u8 data[2] = {reg, val}; + + return sii902x_transfer_write(i2c, data, 2, true); +} + +static int __sii902x_write(struct i2c_client *i2c, u8 reg, u8 val) +{ + u8 data[2] = {reg, val}; + + return sii902x_transfer_write(i2c, data, 2, false); +} + +static int sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, u8 val) +{ + int ret; + u8 status; + + ret = sii902x_read(i2c, reg, &status); + if (ret) + return ret; + status &= ~mask; + status |= val & mask; + return sii902x_write(i2c, reg, status); +} + +static int __sii902x_update_bits(struct i2c_client *i2c, u8 reg, u8 mask, + u8 val) +{ + int ret; + u8 status; + + ret = __sii902x_read(i2c, reg, &status); + if (ret) + return ret; + status &= ~mask; + status |= val & mask; + return __sii902x_write(i2c, reg, status); +} + static inline struct sii902x *bridge_to_sii902x(struct drm_bridge *bridge) { return container_of(bridge, struct sii902x, bridge); @@ -115,9 +233,9 @@ static enum drm_connector_status sii902x_connector_detect(struct drm_connector *connector, bool force) { struct sii902x *sii902x = connector_to_sii902x(connector); - unsigned int status; + u8 status; - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); return (status & SII902X_PLUGGED_STATUS) ? connector_status_connected : connector_status_disconnected; @@ -135,41 +253,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; + int num = 0, 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; - - 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 +269,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; } @@ -237,20 +289,20 @@ static void sii902x_bridge_disable(struct drm_bridge *bridge) { struct sii902x *sii902x = bridge_to_sii902x(bridge); - regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, - SII902X_SYS_CTRL_PWR_DWN, - SII902X_SYS_CTRL_PWR_DWN); + sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA, + SII902X_SYS_CTRL_PWR_DWN, + SII902X_SYS_CTRL_PWR_DWN); } static void sii902x_bridge_enable(struct drm_bridge *bridge) { struct sii902x *sii902x = bridge_to_sii902x(bridge); - regmap_update_bits(sii902x->regmap, SII902X_PWR_STATE_CTRL, - SII902X_AVI_POWER_STATE_MSK, - SII902X_AVI_POWER_STATE_D(0)); - regmap_update_bits(sii902x->regmap, SII902X_SYS_CTRL_DATA, - SII902X_SYS_CTRL_PWR_DWN, 0); + sii902x_update_bits(sii902x->i2c, SII902X_PWR_STATE_CTRL, + SII902X_AVI_POWER_STATE_MSK, + SII902X_AVI_POWER_STATE_D(0)); + sii902x_update_bits(sii902x->i2c, SII902X_SYS_CTRL_DATA, + SII902X_SYS_CTRL_PWR_DWN, 0); } static void sii902x_bridge_mode_set(struct drm_bridge *bridge, @@ -258,25 +310,25 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, struct drm_display_mode *adj) { struct sii902x *sii902x = bridge_to_sii902x(bridge); - struct regmap *regmap = sii902x->regmap; - u8 buf[HDMI_INFOFRAME_SIZE(AVI)]; + u8 buf[HDMI_INFOFRAME_SIZE(AVI) + 1]; struct hdmi_avi_infoframe frame; int ret; - buf[0] = adj->clock; - buf[1] = adj->clock >> 8; - buf[2] = adj->vrefresh; - buf[3] = 0x00; - buf[4] = adj->hdisplay; - buf[5] = adj->hdisplay >> 8; - buf[6] = adj->vdisplay; - buf[7] = adj->vdisplay >> 8; - buf[8] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE | + buf[0] = SII902X_TPI_VIDEO_DATA; + buf[1] = adj->clock; + buf[2] = adj->clock >> 8; + buf[3] = adj->vrefresh; + buf[4] = 0x00; + buf[5] = adj->hdisplay; + buf[6] = adj->hdisplay >> 8; + buf[7] = adj->vdisplay; + buf[8] = adj->vdisplay >> 8; + buf[9] = SII902X_TPI_CLK_RATIO_1X | SII902X_TPI_AVI_PIXEL_REP_NONE | SII902X_TPI_AVI_PIXEL_REP_BUS_24BIT; - buf[9] = SII902X_TPI_AVI_INPUT_RANGE_AUTO | - SII902X_TPI_AVI_INPUT_COLORSPACE_RGB; + buf[10] = SII902X_TPI_AVI_INPUT_RANGE_AUTO | + SII902X_TPI_AVI_INPUT_COLORSPACE_RGB; - ret = regmap_bulk_write(regmap, SII902X_TPI_VIDEO_DATA, buf, 10); + ret = sii902x_bulk_write(sii902x->i2c, buf, 11); if (ret) return; @@ -286,16 +338,17 @@ static void sii902x_bridge_mode_set(struct drm_bridge *bridge, return; } - ret = hdmi_avi_infoframe_pack(&frame, buf, sizeof(buf)); + ret = hdmi_avi_infoframe_pack(&frame, buf + 1, sizeof(buf) - 1); if (ret < 0) { DRM_ERROR("failed to pack AVI infoframe: %d\n", ret); return; } + buf[0] = SII902X_TPI_AVI_INFOFRAME; /* Do not send the infoframe header, but keep the CRC field. */ - regmap_bulk_write(regmap, SII902X_TPI_AVI_INFOFRAME, - buf + HDMI_INFOFRAME_HEADER_SIZE - 1, - HDMI_AVI_INFOFRAME_SIZE + 1); + sii902x_bulk_write(sii902x->i2c, + buf + HDMI_INFOFRAME_HEADER_SIZE, + HDMI_AVI_INFOFRAME_SIZE + 1); } static int sii902x_bridge_attach(struct drm_bridge *bridge) @@ -336,29 +389,13 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = { .enable = sii902x_bridge_enable, }; -static const struct regmap_range sii902x_volatile_ranges[] = { - { .range_min = 0, .range_max = 0xff }, -}; - -static const struct regmap_access_table sii902x_volatile_table = { - .yes_ranges = sii902x_volatile_ranges, - .n_yes_ranges = ARRAY_SIZE(sii902x_volatile_ranges), -}; - -static const struct regmap_config sii902x_regmap_config = { - .reg_bits = 8, - .val_bits = 8, - .volatile_table = &sii902x_volatile_table, - .cache_type = REGCACHE_NONE, -}; - static irqreturn_t sii902x_interrupt(int irq, void *data) { struct sii902x *sii902x = data; - unsigned int status = 0; + u8 status = 0; - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); - regmap_write(sii902x->regmap, SII902X_INT_STATUS, status); + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); + sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status); if ((status & SII902X_HOTPLUG_EVENT) && sii902x->bridge.dev) drm_helper_hpd_irq_event(sii902x->bridge.dev); @@ -366,23 +403,111 @@ static irqreturn_t sii902x_interrupt(int irq, void *data) return IRQ_HANDLED; } +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(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(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(sii902x->i2c, SII902X_SYS_CTRL_DATA, status); + if (ret) + return ret; + return 0; +} + +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; + + 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(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(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(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) { struct device *dev = &client->dev; - unsigned int status = 0; struct sii902x *sii902x; - u8 chipid[4]; + u8 chipid[4], status = 0; 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; sii902x->i2c = client; - sii902x->regmap = devm_regmap_init_i2c(client, &sii902x_regmap_config); - if (IS_ERR(sii902x->regmap)) - return PTR_ERR(sii902x->regmap); sii902x->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_LOW); @@ -394,14 +519,14 @@ static int sii902x_probe(struct i2c_client *client, sii902x_reset(sii902x); - ret = regmap_write(sii902x->regmap, SII902X_REG_TPI_RQB, 0x0); + ret = sii902x_write(sii902x->i2c, SII902X_REG_TPI_RQB, 0x0); if (ret) return ret; - ret = regmap_bulk_read(sii902x->regmap, SII902X_REG_CHIPID(0), - &chipid, 4); + ret = sii902x_bulk_read(sii902x->i2c, SII902X_REG_CHIPID(0), + chipid, 4); if (ret) { - dev_err(dev, "regmap_read failed %d\n", ret); + dev_err(dev, "Can't read chipid (error = %d)\n", ret); return ret; } @@ -412,12 +537,12 @@ static int sii902x_probe(struct i2c_client *client, } /* Clear all pending interrupts */ - regmap_read(sii902x->regmap, SII902X_INT_STATUS, &status); - regmap_write(sii902x->regmap, SII902X_INT_STATUS, status); + sii902x_read(sii902x->i2c, SII902X_INT_STATUS, &status); + sii902x_write(sii902x->i2c, SII902X_INT_STATUS, status); if (client->irq > 0) { - regmap_write(sii902x->regmap, SII902X_INT_ENABLE, - SII902X_HOTPLUG_EVENT); + sii902x_write(sii902x->i2c, SII902X_INT_ENABLE, + SII902X_HOTPLUG_EVENT); ret = devm_request_threaded_irq(dev, client->irq, NULL, sii902x_interrupt, @@ -433,6 +558,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 +582,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;
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, but in order to achieve that I had to get rid of regmap. Signed-off-by: Fabrizio Castro <fabrizio.castro@bp.renesas.com> --- Dear All, this is a follow up to: https://www.mail-archive.com/linux-renesas-soc@vger.kernel.org/msg32072.html Comments very welcome! Thanks, Fab drivers/gpu/drm/bridge/sii902x.c | 404 ++++++++++++++++++++++++++------------- 1 file changed, 274 insertions(+), 130 deletions(-)