| Message ID | 1440970470-7155-1-git-send-email-vladimir_zapolskiy@mentor.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vladimir, Am Montag, den 31.08.2015, 00:34 +0300 schrieb Vladimir Zapolskiy: > The change adds support of internal HDMI I2C master controller, this > subdevice is used by default, if "ddc-i2c-bus" DT property is omitted. I think this should be mentioned in Documentation/devicetree/bindings/drm/bridge/dw_hdmi.txt > The main purpose of this functionality is to support reading EDID from > an HDMI monitor on boards, which don't have an I2C bus connected to > DDC pins. > > The current implementation does not support "I2C Master Interface > Extended Read Mode" to read data addressed by non-zero segment > pointer, this means that if EDID has more than 1 extension blocks, > EDID reading operation won't succeed, in my practice all tested HDMI > monitors have at maximum one extension block. > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > --- > The change is based on today's v4.2, please let me know, if it should be rebased. > > The change has compilation dependency on I2CM_ADDRESS register name fix, > see http://lists.freedesktop.org/archives/dri-devel/2015-May/082980.html > > From http://lists.freedesktop.org/archives/dri-devel/2015-May/082981.html > v1 of the change was > > Tested-by: Philipp Zabel <p.zabel@pengutronix.de> > > but I hesitate to add the tag here due to multiple updates in v2. Tested again, v2 still works fine on Nitrogen6X with HDMI monitor. regards Philipp
Hi, On Sun, Aug 30, 2015 at 2:34 PM, Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: > +static struct i2c_adapter *dw_hdmi_i2c_adapter(struct dw_hdmi *hdmi) > +{ > + struct i2c_adapter *adap; > + struct dw_hdmi_i2c *i2c; > + int ret; > + > + i2c = devm_kzalloc(hdmi->dev, sizeof(*i2c), GFP_KERNEL); > + if (!i2c) > + return ERR_PTR(-ENOMEM); > + > + mutex_init(&i2c->lock); > + init_completion(&i2c->cmp); > + > + adap = &i2c->adap; > + adap->class = I2C_CLASS_DDC; > + adap->owner = THIS_MODULE; > + adap->dev.parent = hdmi->dev; I think you may want to add "adap->dev.of_node = hdmi->dev->of_node;" here. That will allow device trees to specify the i2c bus by using an alias. See <https://chromium-review.googlesource.com/#/c/297040/> where I've added this line and <https://chromium-review.googlesource.com/#/c/297041/> where I've used it.
On Wed, Sep 02, 2015 at 03:07:49PM -0700, Doug Anderson wrote: > Hi, > > On Sun, Aug 30, 2015 at 2:34 PM, Vladimir Zapolskiy > <vladimir_zapolskiy@mentor.com> wrote: > > +static struct i2c_adapter *dw_hdmi_i2c_adapter(struct dw_hdmi *hdmi) > > +{ > > + struct i2c_adapter *adap; > > + struct dw_hdmi_i2c *i2c; > > + int ret; > > + > > + i2c = devm_kzalloc(hdmi->dev, sizeof(*i2c), GFP_KERNEL); > > + if (!i2c) > > + return ERR_PTR(-ENOMEM); > > + > > + mutex_init(&i2c->lock); > > + init_completion(&i2c->cmp); > > + > > + adap = &i2c->adap; > > + adap->class = I2C_CLASS_DDC; > > + adap->owner = THIS_MODULE; > > + adap->dev.parent = hdmi->dev; > > I think you may want to add "adap->dev.of_node = hdmi->dev->of_node;" > here. That will allow device trees to specify the i2c bus by using an > alias. Never copy the of_node from one device to another. That allows the bus matching to unintentionally match the of_node against the wrong driver. Also, is it appropriate to hook non-DDC devices to a DDC bus? I suspect that's asking for trouble.
Russell, On Wed, Sep 2, 2015 at 3:50 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> I think you may want to add "adap->dev.of_node = hdmi->dev->of_node;" >> here. That will allow device trees to specify the i2c bus by using an >> alias. > > Never copy the of_node from one device to another. That allows the > bus matching to unintentionally match the of_node against the wrong > driver. So does that mean we need to create a sub-node for the i2c bus if we want something like this? > Also, is it appropriate to hook non-DDC devices to a DDC bus? I suspect > that's asking for trouble. I doubt it's appropriate. Why do you ask? -Doug
On Wed, Sep 02, 2015 at 04:00:07PM -0700, Doug Anderson wrote: > Russell, > > On Wed, Sep 2, 2015 at 3:50 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > >> I think you may want to add "adap->dev.of_node = hdmi->dev->of_node;" > >> here. That will allow device trees to specify the i2c bus by using an > >> alias. > > > > Never copy the of_node from one device to another. That allows the > > bus matching to unintentionally match the of_node against the wrong > > driver. > > So does that mean we need to create a sub-node for the i2c bus if we > want something like this? > > > > Also, is it appropriate to hook non-DDC devices to a DDC bus? I suspect > > that's asking for trouble. > > I doubt it's appropriate. Why do you ask? To find out why you want to "specify the I2C bus". Surely if we're talking about the DDC bus, we either want to use a separate I2C bus (in which case the HDMI DT description needs to specify which I2C bus to use) or we want to use the HDMI-internal I2C bus, which being part of the HDMI driver, the HDMI driver will know how to find it itself - there should be no need to put an explicit ddc-i2c-bus self-reference there in that case.
Russell, On Wed, Sep 2, 2015 at 4:04 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: >> > Also, is it appropriate to hook non-DDC devices to a DDC bus? I suspect >> > that's asking for trouble. >> >> I doubt it's appropriate. Why do you ask? > > To find out why you want to "specify the I2C bus". > > Surely if we're talking about the DDC bus, we either want to use a > separate I2C bus (in which case the HDMI DT description needs to > specify which I2C bus to use) or we want to use the HDMI-internal > I2C bus, which being part of the HDMI driver, the HDMI driver will > know how to find it itself - there should be no need to put an > explicit ddc-i2c-bus self-reference there in that case. Overall it comes down to bus numbering. Possibly that's a stupid reason, but it is my reason nevertheless. Specifically it significantly helps my brain process kernel log messages if the i2c bus that's referred to "bus 5" in my SoC's user manual shows up consistently as "i2c5" in kernel log messages. It's helpful it it shows up as "i2c5" even if "i2c0" - "i2c4" aren't enabled. That's all totally possible by using this type of syntax, like in rk3288.dtsi: aliases { i2c0 = &i2c0; i2c1 = &i2c1; i2c2 = &i2c2; i2c3 = &i2c3; i2c4 = &i2c4; i2c5 = &i2c5; Similarly, I'd like "bus 0" to show up as i2c0, which will happen as you can see in the above. The problem is that if another bus registers itself before the SoC's i2c0 registers itself and that bus doesn't give a number to itself then the i2c subsystem will chose "I2C 0". ...and then when the main SoC i2c bus registers itself it will fail because i2c0 is already taken. By having a of_node for the hdmi i2c bus, we can assign a number to it like: i2c15 = &hdmi; This is all described in the two links I referenced in my original reply. A possible other option is to have the i2c subsystem try to start numbering at a larger base for all automatically numbered busses (those that didn't specify a number). Then it's more likely (though still not guaranteed) to conflict with another bus... -Doug
Russell, On Wed, Sep 2, 2015 at 3:50 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > Never copy the of_node from one device to another. That allows the > bus matching to unintentionally match the of_node against the wrong > driver. Can you be more specific about what problems you'd expect. It seems like a terribly common practice to do this, but maybe I'm misunderstanding: On today's next, I do a simple: git grep 'dev.of_node = ' ...and I find lots of hits: drivers/i2c/busses/i2c-at91.c: dev->adapter.dev.of_node = pdev->dev.of_node; drivers/i2c/busses/i2c-axxia.c: idev->adapter.dev.of_node = pdev->dev.of_node; drivers/i2c/busses/i2c-bcm-iproc.c: adap->dev.of_node = pdev->dev.of_node; drivers/i2c/busses/i2c-bcm-kona.c: adap->dev.of_node = pdev->dev.of_node; drivers/i2c/busses/i2c-bcm2835.c: adap->dev.of_node = pdev->dev.of_node; drivers/i2c/busses/i2c-brcmstb.c: adap->dev.of_node = pdev->dev.of_node; ... So unless I'm mistaken, the code I'm suggesting is a common practice. Perhaps there is a latent bug that's waiting to bite us. If so then that bug should be reported and fixed. ...but without seeing some concrete problem (or some reason that the code I'm suggesting is different than everyone else's code) it seems best to take it and to later fix it (along with all the other code) if/when we find some problem. Objections? -Doug
On Wed, Sep 02, 2015 at 04:43:38PM -0700, Doug Anderson wrote: > Russell, > > On Wed, Sep 2, 2015 at 3:50 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > Never copy the of_node from one device to another. That allows the > > bus matching to unintentionally match the of_node against the wrong > > driver. > > Can you be more specific about what problems you'd expect. It seems > like a terribly common practice to do this, but maybe I'm > misunderstanding: The problem with copying an of_node from one struct device to another struct device is of_match_device(). Devices in a DT setup are bound to drivers using dev->of_node - when a new device or driver is registered onto a bus, the new object is matched against all un-bound objects (devices or drivers as appropriate) comparing the compatible string gained from dev->of_node against the list of of_device_id strings provided by the driver. When a match is found, the two are attempted to be bound together. Consider what happens when you have a platform device with an of_node, and you bind that to a driver matched via the compatible string. The driver creates a new platform device, and copies the of_node to this new device so the new device can have access to the properties of the of_node. What happens next? Does the new device bind to the same driver (and thus we enter an infinite loop) or does it bind to some other driver as the author originally hoped? Depends whether the other driver is earlier in the list of drivers or later, whether it's in the kernel or not. It's less of a problem when you copy the of_node between two different buses, because they won't match the same driver, but this is a dangerous act - if stuff copies it in one direction, what's to stop someone copying it back to a new device on the original bus. As soon as that happens, things can go awol. It would be a good idea if either: 1) this never happened or 2) we had a flag which said "ignore the of_node for driver matching" And yes, we do have drivers which do exactly what I said above. They work through luck rather than design.
On Thu, Sep 03, 2015 at 10:19:00AM +0100, Russell King - ARM Linux wrote: > On Wed, Sep 02, 2015 at 04:43:38PM -0700, Doug Anderson wrote: > > Russell, > > > > On Wed, Sep 2, 2015 at 3:50 PM, Russell King - ARM Linux > > <linux@arm.linux.org.uk> wrote: > > > Never copy the of_node from one device to another. That allows the > > > bus matching to unintentionally match the of_node against the wrong > > > driver. > > > > Can you be more specific about what problems you'd expect. It seems > > like a terribly common practice to do this, but maybe I'm > > misunderstanding: > > The problem with copying an of_node from one struct device to another > struct device is of_match_device(). > > Devices in a DT setup are bound to drivers using dev->of_node - when a > new device or driver is registered onto a bus, the new object is matched > against all un-bound objects (devices or drivers as appropriate) > comparing the compatible string gained from dev->of_node against the > list of of_device_id strings provided by the driver. > > When a match is found, the two are attempted to be bound together. > > Consider what happens when you have a platform device with an of_node, > and you bind that to a driver matched via the compatible string. The > driver creates a new platform device, and copies the of_node to this > new device so the new device can have access to the properties of the > of_node. > > What happens next? Does the new device bind to the same driver (and > thus we enter an infinite loop) or does it bind to some other driver > as the author originally hoped? Depends whether the other driver is > earlier in the list of drivers or later, whether it's in the kernel > or not. > > It's less of a problem when you copy the of_node between two different > buses, because they won't match the same driver, but this is a dangerous > act - if stuff copies it in one direction, what's to stop someone copying > it back to a new device on the original bus. As soon as that happens, > things can go awol. > > It would be a good idea if either: > 1) this never happened > or > 2) we had a flag which said "ignore the of_node for driver matching" > > And yes, we do have drivers which do exactly what I said above. They > work through luck rather than design. Perhaps the I2C core needs to be taught to look at the adapter's ->dev.parent->of_node in of_find_i2c_adapter_by_node(). As I understand it the purpose for registering a separate struct device for each I2C controller is so that there's an I2C parent for all slaves. At the same time every controller sets up ->dev.parent, and that parent's OF node is what we really want when we look up an I2C adapter by OF node. In other words, the OF node by which I2C adapters are referenced is always that of its parent device, as far as I can tell. Wolfram, do you know of any cases where adapter->dev.of_node would not be the same as adapter->dev.parent->of_node? Thierry
Thierry, On Thu, Sep 3, 2015 at 2:39 AM, Thierry Reding <treding@nvidia.com> wrote: > Perhaps the I2C core needs to be taught to look at the adapter's > ->dev.parent->of_node in of_find_i2c_adapter_by_node(). As I understand > it the purpose for registering a separate struct device for each I2C > controller is so that there's an I2C parent for all slaves. At the same > time every controller sets up ->dev.parent, and that parent's OF node is > what we really want when we look up an I2C adapter by OF node. In other > words, the OF node by which I2C adapters are referenced is always that > of its parent device, as far as I can tell. > > Wolfram, do you know of any cases where adapter->dev.of_node would not > be the same as adapter->dev.parent->of_node? This sounds pretty reasonable to me. Certainly it would be a pretty reasonable fallback if adapter->dev.of_node was NULL. -Doug
> Wolfram, do you know of any cases where adapter->dev.of_node would not > be the same as adapter->dev.parent->of_node? i2c-muxes, I'd say. dev.parent is the device of the parent I2C adapter, while the devices get populated from the of_node of the muxing device.
Hi, On Sun, Aug 30, 2015 at 2:34 PM, Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: > The change adds support of internal HDMI I2C master controller, this > subdevice is used by default, if "ddc-i2c-bus" DT property is omitted. > > The main purpose of this functionality is to support reading EDID from > an HDMI monitor on boards, which don't have an I2C bus connected to > DDC pins. > > The current implementation does not support "I2C Master Interface > Extended Read Mode" to read data addressed by non-zero segment > pointer, this means that if EDID has more than 1 extension blocks, > EDID reading operation won't succeed, in my practice all tested HDMI > monitors have at maximum one extension block. > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > --- > The change is based on today's v4.2, please let me know, if it should be rebased. > > The change has compilation dependency on I2CM_ADDRESS register name fix, > see http://lists.freedesktop.org/archives/dri-devel/2015-May/082980.html > > From http://lists.freedesktop.org/archives/dri-devel/2015-May/082981.html > v1 of the change was > > Tested-by: Philipp Zabel <p.zabel@pengutronix.de> > > but I hesitate to add the tag here due to multiple updates in v2. > > Changes from v2 to v3, thanks to Russell: > - moved register field value definitions to dw_hdmi.h > - made completions uninterruptible to avoid transfer retries if interrupted > - use one completion for both read and write transfers as in v1, operation_reg is removed > - redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write() > - struct i2c_algorithm dw_hdmi_algorithm is qualified as const > - dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path > - dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is not for I2CM > - spin lock is removed from dw_hdmi_i2c_irq() > - removed spin lock from dw_hdmi_i2c_xfer() around write to HDMI_IH_MUTE_I2CM_STAT0 register > - split loop over message array in dw_hdmi_i2c_xfer() to validation and transfer parts > - added a mutex to serialize I2C transfer requests, i2c->lock is completely removed > - removed if(len) check from dw_hdmi_i2c_write(), hardware supports only len>0 transfers > - described extension blocks <= 1 limitation in the commit message > - a number of minor clean ups > > Changes from v1 to v2: > - fixed a devm_kfree() signature > - split completions for read and write operations > > drivers/gpu/drm/bridge/dw_hdmi.c | 262 ++++++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/bridge/dw_hdmi.h | 19 +++ > 2 files changed, 275 insertions(+), 6 deletions(-) Not sure what the status of this patch is, but I'll make a few more suggestions in case they're helpful. > +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi) > +{ > + /* Set Fast Mode speed */ > + hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV); If you're going to hardcode to something, it seems like hardcoding to standard mode might be better? It's likely that users will plug into all kinds of broken / crappy HDMI devices out there. I think you'll get better compatibility by using the slower mode, and EDID transfers really aren't too performance critical are they? > + > + /* Software reset */ > + hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ); Unless there's a reason not to, it seems like the reset ought to be the first thing in this function (before setting the I2CM_DIV). Even if it doesn't actually reset the speed on current hardware, it just seems cleaner. > +static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, > + struct i2c_msg *msgs, int num) > +{ > + struct dw_hdmi *hdmi = i2c_get_adapdata(adap); > + struct dw_hdmi_i2c *i2c = hdmi->i2c; > + u8 addr = msgs[0].addr; > + int i, ret = 0; > + > + dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr); > + > + for (i = 0; i < num; i++) { > + if (msgs[i].addr != addr) { > + dev_warn(hdmi->dev, > + "unsupported transfer, changed slave address\n"); > + return -EOPNOTSUPP; > + } > + > + if (msgs[i].len == 0) { > + dev_dbg(hdmi->dev, > + "unsupported transfer %d/%d, no data\n", > + i + 1, num); > + return -EOPNOTSUPP; > + } > + } > + > + mutex_lock(&i2c->lock); > + If I may make a suggestion, it's worth considering putting dw_hdmi_i2c_init() here and removing it from the dw_hdmi_bind() function. There isn't any state kept between i2c transfers (each one is independent), so re-initting each time won't hurt. ...and doing so has the potential to fix some things. On rk3288 there's at least one case where the i2c controller can get wedged and just totally stops working. Although this particular wedge isn't fixed by calling dw_hdmi_i2c_init() (it needs a full controller reset to fix), it's possible that there may be other cases where the HDMI_I2CM_SOFTRSTZ fixes some bad state. Note: if you do that, you can just init HDMI_IH_MUTE_I2CM_STAT0 to 0x00 at the end of dw_hdmi_i2c_init() and skip the next statement... > + hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0); -Doug On Sun, Aug 30, 2015 at 2:34 PM, Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: > The change adds support of internal HDMI I2C master controller, this > subdevice is used by default, if "ddc-i2c-bus" DT property is omitted. > > The main purpose of this functionality is to support reading EDID from > an HDMI monitor on boards, which don't have an I2C bus connected to > DDC pins. > > The current implementation does not support "I2C Master Interface > Extended Read Mode" to read data addressed by non-zero segment > pointer, this means that if EDID has more than 1 extension blocks, > EDID reading operation won't succeed, in my practice all tested HDMI > monitors have at maximum one extension block. > > Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> > --- > The change is based on today's v4.2, please let me know, if it should be rebased. > > The change has compilation dependency on I2CM_ADDRESS register name fix, > see http://lists.freedesktop.org/archives/dri-devel/2015-May/082980.html > > From http://lists.freedesktop.org/archives/dri-devel/2015-May/082981.html > v1 of the change was > > Tested-by: Philipp Zabel <p.zabel@pengutronix.de> > > but I hesitate to add the tag here due to multiple updates in v2. > > Changes from v2 to v3, thanks to Russell: > - moved register field value definitions to dw_hdmi.h > - made completions uninterruptible to avoid transfer retries if interrupted > - use one completion for both read and write transfers as in v1, operation_reg is removed > - redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write() > - struct i2c_algorithm dw_hdmi_algorithm is qualified as const > - dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path > - dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is not for I2CM > - spin lock is removed from dw_hdmi_i2c_irq() > - removed spin lock from dw_hdmi_i2c_xfer() around write to HDMI_IH_MUTE_I2CM_STAT0 register > - split loop over message array in dw_hdmi_i2c_xfer() to validation and transfer parts > - added a mutex to serialize I2C transfer requests, i2c->lock is completely removed > - removed if(len) check from dw_hdmi_i2c_write(), hardware supports only len>0 transfers > - described extension blocks <= 1 limitation in the commit message > - a number of minor clean ups > > Changes from v1 to v2: > - fixed a devm_kfree() signature > - split completions for read and write operations > > drivers/gpu/drm/bridge/dw_hdmi.c | 262 ++++++++++++++++++++++++++++++++++++++- > drivers/gpu/drm/bridge/dw_hdmi.h | 19 +++ > 2 files changed, 275 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c > index 816d104..d329e04 100644 > --- a/drivers/gpu/drm/bridge/dw_hdmi.c > +++ b/drivers/gpu/drm/bridge/dw_hdmi.c > @@ -1,14 +1,15 @@ > /* > + * DesignWare High-Definition Multimedia Interface (HDMI) driver > + * > + * Copyright (C) 2013-2015 Mentor Graphics Inc. > * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. > + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > * > * This program is free software; you can redistribute it and/or modify > * it under the terms of the GNU General Public License as published by > * the Free Software Foundation; either version 2 of the License, or > * (at your option) any later version. > * > - * Designware High-Definition Multimedia Interface (HDMI) driver > - * > - * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de> > */ > #include <linux/module.h> > #include <linux/irq.h> > @@ -102,6 +103,17 @@ struct hdmi_data_info { > struct hdmi_vmode video_mode; > }; > > +struct dw_hdmi_i2c { > + struct i2c_adapter adap; > + > + struct mutex lock; > + struct completion cmp; > + u8 stat; > + > + u8 slave_reg; > + bool is_regaddr; > +}; > + > struct dw_hdmi { > struct drm_connector connector; > struct drm_encoder *encoder; > @@ -111,6 +123,7 @@ struct dw_hdmi { > struct device *dev; > struct clk *isfr_clk; > struct clk *iahb_clk; > + struct dw_hdmi_i2c *i2c; > > struct hdmi_data_info hdmi_data; > const struct dw_hdmi_plat_data *plat_data; > @@ -179,6 +192,200 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg, > hdmi_modb(hdmi, data << shift, mask, reg); > } > > +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi) > +{ > + /* Set Fast Mode speed */ > + hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV); > + > + /* Software reset */ > + hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ); > + > + /* Set done, not acknowledged and arbitration interrupt polarities */ > + hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT); > + hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL, > + HDMI_I2CM_CTLINT); > + > + /* Clear DONE and ERROR interrupts */ > + hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE, > + HDMI_IH_I2CM_STAT0); > + > + /* Mute DONE and ERROR interrupts */ > + hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE, > + HDMI_IH_MUTE_I2CM_STAT0); > +} > + > +static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi, > + unsigned char *buf, unsigned int length) > +{ > + struct dw_hdmi_i2c *i2c = hdmi->i2c; > + int stat; > + > + if (!i2c->is_regaddr) { > + dev_dbg(hdmi->dev, "set read register address to 0\n"); > + i2c->slave_reg = 0x00; > + i2c->is_regaddr = true; > + } > + > + while (length--) { > + reinit_completion(&i2c->cmp); > + > + hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS); > + hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ, > + HDMI_I2CM_OPERATION); > + > + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); > + if (!stat) > + return -EAGAIN; > + > + /* Check for error condition on the bus */ > + if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) > + return -EIO; > + > + *buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI); > + } > + > + return 0; > +} > + > +static int dw_hdmi_i2c_write(struct dw_hdmi *hdmi, > + unsigned char *buf, unsigned int length) > +{ > + struct dw_hdmi_i2c *i2c = hdmi->i2c; > + int stat; > + > + if (!i2c->is_regaddr) { > + /* Use the first write byte as register address */ > + i2c->slave_reg = buf[0]; > + length--; > + buf++; > + i2c->is_regaddr = true; > + } > + > + while (length--) { > + reinit_completion(&i2c->cmp); > + > + hdmi_writeb(hdmi, *buf++, HDMI_I2CM_DATAO); > + hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS); > + hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_WRITE, > + HDMI_I2CM_OPERATION); > + > + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); > + if (!stat) > + return -EAGAIN; > + > + /* Check for error condition on the bus */ > + if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) > + return -EIO; > + } > + > + return 0; > +} > + > +static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, > + struct i2c_msg *msgs, int num) > +{ > + struct dw_hdmi *hdmi = i2c_get_adapdata(adap); > + struct dw_hdmi_i2c *i2c = hdmi->i2c; > + u8 addr = msgs[0].addr; > + int i, ret = 0; > + > + dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr); > + > + for (i = 0; i < num; i++) { > + if (msgs[i].addr != addr) { > + dev_warn(hdmi->dev, > + "unsupported transfer, changed slave address\n"); > + return -EOPNOTSUPP; > + } > + > + if (msgs[i].len == 0) { > + dev_dbg(hdmi->dev, > + "unsupported transfer %d/%d, no data\n", > + i + 1, num); > + return -EOPNOTSUPP; > + } > + } > + > + mutex_lock(&i2c->lock); > + > + hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0); > + > + /* Set slave device address taken from the first I2C message */ > + hdmi_writeb(hdmi, addr, HDMI_I2CM_SLAVE); > + > + /* Set slave device register address on transfer */ > + i2c->is_regaddr = false; > + > + for (i = 0; i < num; i++) { > + dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: %#x\n", > + i + 1, num, msgs[i].len, msgs[i].flags); > + > + if (msgs[i].flags & I2C_M_RD) > + ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len); > + else > + ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len); > + > + if (ret < 0) > + break; > + } > + > + if (!ret) > + ret = num; > + > + /* Mute DONE and ERROR interrupts */ > + hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE, > + HDMI_IH_MUTE_I2CM_STAT0); > + > + mutex_unlock(&i2c->lock); > + > + return ret; > +} > + > +static u32 dw_hdmi_i2c_func(struct i2c_adapter *adapter) > +{ > + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; > +} > + > +static const struct i2c_algorithm dw_hdmi_algorithm = { > + .master_xfer = dw_hdmi_i2c_xfer, > + .functionality = dw_hdmi_i2c_func, > +}; > + > +static struct i2c_adapter *dw_hdmi_i2c_adapter(struct dw_hdmi *hdmi) > +{ > + struct i2c_adapter *adap; > + struct dw_hdmi_i2c *i2c; > + int ret; > + > + i2c = devm_kzalloc(hdmi->dev, sizeof(*i2c), GFP_KERNEL); > + if (!i2c) > + return ERR_PTR(-ENOMEM); > + > + mutex_init(&i2c->lock); > + init_completion(&i2c->cmp); > + > + adap = &i2c->adap; > + adap->class = I2C_CLASS_DDC; > + adap->owner = THIS_MODULE; > + adap->dev.parent = hdmi->dev; > + adap->algo = &dw_hdmi_algorithm; > + strlcpy(adap->name, "DesignWare HDMI", sizeof(adap->name)); > + i2c_set_adapdata(adap, hdmi); > + > + ret = i2c_add_adapter(adap); > + if (ret) { > + dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name); > + devm_kfree(hdmi->dev, i2c); > + return ERR_PTR(ret); > + } > + > + hdmi->i2c = i2c; > + > + dev_info(hdmi->dev, "registered %s I2C bus driver\n", adap->name); > + > + return adap; > +} > + > static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts, > unsigned int n) > { > @@ -1466,16 +1673,40 @@ static struct drm_bridge_funcs dw_hdmi_bridge_funcs = { > .mode_fixup = dw_hdmi_bridge_mode_fixup, > }; > > +static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi) > +{ > + struct dw_hdmi_i2c *i2c = hdmi->i2c; > + unsigned int stat; > + > + stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0); > + if (!stat) > + return IRQ_NONE; > + > + hdmi_writeb(hdmi, stat, HDMI_IH_I2CM_STAT0); > + > + i2c->stat = stat; > + > + complete(&i2c->cmp); > + > + return IRQ_HANDLED; > +} > + > static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id) > { > struct dw_hdmi *hdmi = dev_id; > u8 intr_stat; > + irqreturn_t ret = IRQ_NONE; > + > + if (hdmi->i2c) > + ret = dw_hdmi_i2c_irq(hdmi); > > intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0); > - if (intr_stat) > + if (intr_stat) { > hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); > + return IRQ_WAKE_THREAD; > + } > > - return intr_stat ? IRQ_WAKE_THREAD : IRQ_NONE; > + return ret; > } > > static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) > @@ -1654,6 +1885,13 @@ int dw_hdmi_bind(struct device *dev, struct device *master, > */ > hdmi_init_clk_regenerator(hdmi); > > + /* If DDC bus is not specified, try to register HDMI I2C bus */ > + if (!hdmi->ddc) { > + hdmi->ddc = dw_hdmi_i2c_adapter(hdmi); > + if (IS_ERR(hdmi->ddc)) > + hdmi->ddc = NULL; > + } > + > /* > * Configure registers related to HDMI interrupt > * generation before registering IRQ. > @@ -1674,11 +1912,18 @@ int dw_hdmi_bind(struct device *dev, struct device *master, > /* Unmute interrupts */ > hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0); > > + /* Unmute I2CM interrupts and reset HDMI DDC I2C master controller */ > + if (hdmi->i2c) > + dw_hdmi_i2c_init(hdmi); > + > dev_set_drvdata(dev, hdmi); > > return 0; > > err_iahb: > + if (hdmi->i2c) > + i2c_del_adapter(&hdmi->i2c->adap); > + > clk_disable_unprepare(hdmi->iahb_clk); > err_isfr: > clk_disable_unprepare(hdmi->isfr_clk); > @@ -1699,13 +1944,18 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data) > > clk_disable_unprepare(hdmi->iahb_clk); > clk_disable_unprepare(hdmi->isfr_clk); > - i2c_put_adapter(hdmi->ddc); > + > + if (hdmi->i2c) > + i2c_del_adapter(&hdmi->i2c->adap); > + else > + i2c_put_adapter(hdmi->ddc); > } > EXPORT_SYMBOL_GPL(dw_hdmi_unbind); > > MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>"); > MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>"); > MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>"); > +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>"); > MODULE_DESCRIPTION("DW HDMI transmitter driver"); > MODULE_LICENSE("GPL"); > MODULE_ALIAS("platform:dw-hdmi"); > diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h > index ee7f7ed..9ed85eb 100644 > --- a/drivers/gpu/drm/bridge/dw_hdmi.h > +++ b/drivers/gpu/drm/bridge/dw_hdmi.h > @@ -563,6 +563,10 @@ enum { > HDMI_IH_PHY_STAT0_TX_PHY_LOCK = 0x2, > HDMI_IH_PHY_STAT0_HPD = 0x1, > > +/* IH_I2CM_STAT0 and IH_MUTE_I2CM_STAT0 field values */ > + HDMI_IH_I2CM_STAT0_DONE = 0x2, > + HDMI_IH_I2CM_STAT0_ERROR = 0x1, > + > /* IH_MUTE_I2CMPHY_STAT0 field values */ > HDMI_IH_MUTE_I2CMPHY_STAT0_I2CMPHYDONE = 0x2, > HDMI_IH_MUTE_I2CMPHY_STAT0_I2CMPHYERROR = 0x1, > @@ -1029,6 +1033,21 @@ enum { > HDMI_A_VIDPOLCFG_HSYNCPOL_MASK = 0x2, > HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_HIGH = 0x2, > HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0, > + > +/* I2CM_OPERATION field values */ > + HDMI_I2CM_OPERATION_WRITE = 0x10, > + HDMI_I2CM_OPERATION_READ_EXT = 0x2, > + HDMI_I2CM_OPERATION_READ = 0x1, > + > +/* I2CM_INT field values */ > + HDMI_I2CM_INT_DONE_POL = 0x8, > + HDMI_I2CM_INT_DONE_MASK = 0x4, > + > +/* I2CM_CTLINT field values */ > + HDMI_I2CM_CTLINT_NAC_POL = 0x80, > + HDMI_I2CM_CTLINT_NAC_MASK = 0x40, > + HDMI_I2CM_CTLINT_ARB_POL = 0x8, > + HDMI_I2CM_CTLINT_ARB_MASK = 0x4, > }; > > #endif /* __DW_HDMI_H__ */ > -- > 2.5.0 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Doug, On 16.09.2015 23:04, Doug Anderson wrote: > Hi, > > On Sun, Aug 30, 2015 at 2:34 PM, Vladimir Zapolskiy > <vladimir_zapolskiy@mentor.com> wrote: >> The change adds support of internal HDMI I2C master controller, this >> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted. >> >> The main purpose of this functionality is to support reading EDID from >> an HDMI monitor on boards, which don't have an I2C bus connected to >> DDC pins. >> >> The current implementation does not support "I2C Master Interface >> Extended Read Mode" to read data addressed by non-zero segment >> pointer, this means that if EDID has more than 1 extension blocks, >> EDID reading operation won't succeed, in my practice all tested HDMI >> monitors have at maximum one extension block. >> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> >> --- >> The change is based on today's v4.2, please let me know, if it should be rebased. >> >> The change has compilation dependency on I2CM_ADDRESS register name fix, >> see http://lists.freedesktop.org/archives/dri-devel/2015-May/082980.html >> >> From http://lists.freedesktop.org/archives/dri-devel/2015-May/082981.html >> v1 of the change was >> >> Tested-by: Philipp Zabel <p.zabel@pengutronix.de> >> >> but I hesitate to add the tag here due to multiple updates in v2. >> >> Changes from v2 to v3, thanks to Russell: >> - moved register field value definitions to dw_hdmi.h >> - made completions uninterruptible to avoid transfer retries if interrupted >> - use one completion for both read and write transfers as in v1, operation_reg is removed >> - redundant i2c->stat = 0 is removed from dw_hdmi_i2c_read/write() >> - struct i2c_algorithm dw_hdmi_algorithm is qualified as const >> - dw_hdmi_i2c_adapter() does not modify struct dw_hdmi on error path >> - dw_hdmi_i2c_irq() does not modify hdmi->i2c->stat, if interrupt is not for I2CM >> - spin lock is removed from dw_hdmi_i2c_irq() >> - removed spin lock from dw_hdmi_i2c_xfer() around write to HDMI_IH_MUTE_I2CM_STAT0 register >> - split loop over message array in dw_hdmi_i2c_xfer() to validation and transfer parts >> - added a mutex to serialize I2C transfer requests, i2c->lock is completely removed >> - removed if(len) check from dw_hdmi_i2c_write(), hardware supports only len>0 transfers >> - described extension blocks <= 1 limitation in the commit message >> - a number of minor clean ups >> >> Changes from v1 to v2: >> - fixed a devm_kfree() signature >> - split completions for read and write operations >> >> drivers/gpu/drm/bridge/dw_hdmi.c | 262 ++++++++++++++++++++++++++++++++++++++- >> drivers/gpu/drm/bridge/dw_hdmi.h | 19 +++ >> 2 files changed, 275 insertions(+), 6 deletions(-) > > Not sure what the status of this patch is, but I'll make a few more > suggestions in case they're helpful. sure, all review comments are welcome. I have in mind that Philipp asked to add an update to documentation, the next version will contain the fixes. > >> +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi) >> +{ >> + /* Set Fast Mode speed */ >> + hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV); > > If you're going to hardcode to something, it seems like hardcoding to > standard mode might be better? It's likely that users will plug into > all kinds of broken / crappy HDMI devices out there. I think you'll > get better compatibility by using the slower mode, and EDID transfers > really aren't too performance critical are they? > Accepted. I don't know what are the exact divisors of master clock for fast and standard modes, for reference I'll make a simple performance test and publish its results. I expect that in standard mode SCL is about 100KHz and in fast mode SCL is about 400KHz, and, if in standard mode SCL is less than 100KHz, it will take more than 50ms to read out 512 bytes of data, which might be not acceptable from performance perspective. >> + >> + /* Software reset */ >> + hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ); > > Unless there's a reason not to, it seems like the reset ought to be > the first thing in this function (before setting the I2CM_DIV). Even > if it doesn't actually reset the speed on current hardware, it just > seems cleaner. It makes sense for me. > >> +static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, >> + struct i2c_msg *msgs, int num) >> +{ >> + struct dw_hdmi *hdmi = i2c_get_adapdata(adap); >> + struct dw_hdmi_i2c *i2c = hdmi->i2c; >> + u8 addr = msgs[0].addr; >> + int i, ret = 0; >> + >> + dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr); >> + >> + for (i = 0; i < num; i++) { >> + if (msgs[i].addr != addr) { >> + dev_warn(hdmi->dev, >> + "unsupported transfer, changed slave address\n"); >> + return -EOPNOTSUPP; >> + } >> + >> + if (msgs[i].len == 0) { >> + dev_dbg(hdmi->dev, >> + "unsupported transfer %d/%d, no data\n", >> + i + 1, num); >> + return -EOPNOTSUPP; >> + } >> + } >> + >> + mutex_lock(&i2c->lock); >> + > > If I may make a suggestion, it's worth considering putting > dw_hdmi_i2c_init() here and removing it from the dw_hdmi_bind() > function. There isn't any state kept between i2c transfers (each one > is independent), so re-initting each time won't hurt. ...and doing so > has the potential to fix some things. I think it is resourse wasteful, especially since it does not solve any actual problem, if I understand your arguments correctly. I'd like to see the I2C bus registered even if there is no ongoing transfer, for instance running "modprobe i2c-dev; i2cdetect -l". For information I notice that there is a movement of I2C bus configuration/capture from bind() to probe() in DRM drivers, e.g. see commit 53bdcf5f026c . > On rk3288 there's at least one case where the i2c controller can get > wedged and just totally stops working. Although this particular wedge > isn't fixed by calling dw_hdmi_i2c_init() (it needs a full controller > reset to fix), it's possible that there may be other cases where the > HDMI_I2CM_SOFTRSTZ fixes some bad state. I don't know, may be the shared part of the driver should export a reset() function for RK3288, if it (or its next version) helps? Unlickily I don't possess an RK3288 power board, but in any case I would appreciate, if you share your test, I'll try to reproduce the same problem on iMX6. At the moment I am inclided to preserve I2C bus registration in bind() > Note: if you do that, you can just init HDMI_IH_MUTE_I2CM_STAT0 to > 0x00 at the end of dw_hdmi_i2c_init() and skip the next statement... > >> + hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0); > > With best wishes, Vladimir
Hi Doug, On 03.09.2015 02:19, Doug Anderson wrote: > Russell, > > On Wed, Sep 2, 2015 at 4:04 PM, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: >>>> Also, is it appropriate to hook non-DDC devices to a DDC bus? I suspect >>>> that's asking for trouble. >>> >>> I doubt it's appropriate. Why do you ask? >> >> To find out why you want to "specify the I2C bus". >> >> Surely if we're talking about the DDC bus, we either want to use a >> separate I2C bus (in which case the HDMI DT description needs to >> specify which I2C bus to use) or we want to use the HDMI-internal >> I2C bus, which being part of the HDMI driver, the HDMI driver will >> know how to find it itself - there should be no need to put an >> explicit ddc-i2c-bus self-reference there in that case. > > Overall it comes down to bus numbering. Possibly that's a stupid > reason, but it is my reason nevertheless. this is a known issue regarding I2C bus numbering. > Specifically it significantly helps my brain process kernel log > messages if the i2c bus that's referred to "bus 5" in my SoC's user > manual shows up consistently as "i2c5" in kernel log messages. It's > helpful it it shows up as "i2c5" even if "i2c0" - "i2c4" aren't > enabled. > > That's all totally possible by using this type of syntax, like in rk3288.dtsi: > > aliases { > i2c0 = &i2c0; > i2c1 = &i2c1; > i2c2 = &i2c2; > i2c3 = &i2c3; > i2c4 = &i2c4; > i2c5 = &i2c5; > > Similarly, I'd like "bus 0" to show up as i2c0, which will happen as > you can see in the above. > > The problem is that if another bus registers itself before the SoC's > i2c0 registers itself and that bus doesn't give a number to itself > then the i2c subsystem will chose "I2C 0". ...and then when the main > SoC i2c bus registers itself it will fail because i2c0 is already > taken. > > By having a of_node for the hdmi i2c bus, we can assign a number to it like: > i2c15 = &hdmi; > > This is all described in the two links I referenced in my original reply. > > > A possible other option is to have the i2c subsystem try to start > numbering at a larger base for all automatically numbered busses > (those that didn't specify a number). Then it's more likely (though > still not guaranteed) to conflict with another bus... Could you please check if commit 03bde7c31a3 serves you? -- With best wishes, Vladimir
Vladimir, On Wed, Sep 16, 2015 at 1:58 PM, Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: >>> +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi) >>> +{ >>> + /* Set Fast Mode speed */ >>> + hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV); >> >> If you're going to hardcode to something, it seems like hardcoding to >> standard mode might be better? It's likely that users will plug into >> all kinds of broken / crappy HDMI devices out there. I think you'll >> get better compatibility by using the slower mode, and EDID transfers >> really aren't too performance critical are they? >> > > Accepted. I don't know what are the exact divisors of master clock for > fast and standard modes, for reference I'll make a simple performance > test and publish its results. I expect that in standard mode SCL is > about 100KHz and in fast mode SCL is about 400KHz, and, if in standard > mode SCL is less than 100KHz, it will take more than 50ms to read out > 512 bytes of data, which might be not acceptable from performance > perspective. Yes, I'd expect 100kHz and 400kHz. I agree that 50ms is non-trivial, but it's also not something you're doing lots of. I'd expect that the EDID is read over this channel at cable plugin time and then not used much after that. Adding an extra 40ms (10ms vs 50ms) before we can access the TV doesn't seem terrible for compatibility. Doing a quick scan for what others in mainline do: A few can be found with: $ git grep -A3 hdmiddc | grep clock-freq arch/arm/boot/dts/stihxxx-b2120.dtsi- clock-frequency = <100000>; arch/arm/boot/dts/tegra30-apalis.dtsi- clock-frequency = <100000>; arch/arm/boot/dts/tegra30-beaver.dts- clock-frequency = <100000>; arch/arm/boot/dts/tegra30-colibri.dtsi- clock-frequency = <100000>; A few more done by hand (picked arbitrarily; note that unspecified clock freq means 100000 on rk3288): arch/arm/boot/dts/omap3-beagle.dts: ddc-i2c-bus = <&i2c3>; arch/arm/boot/dts/omap3-beagle.dts:&i2c3 { arch/arm/boot/dts/omap3-beagle.dts- clock-frequency = <100000>; arch/arm/boot/dts/omap3-beagle.dts-}; arch/arm/boot/dts/omap3-beagle-xm.dts: ddc-i2c-bus = <&i2c3>; arch/arm/boot/dts/omap3-beagle-xm.dts:&i2c3 { arch/arm/boot/dts/omap3-beagle-xm.dts- clock-frequency = <100000>; arch/arm/boot/dts/omap3-beagle-xm.dts-}; arch/arm/boot/dts/rk3288-evb.dtsi: ddc-i2c-bus = <&i2c5>; arch/arm/boot/dts/rk3288-evb.dtsi:&i2c5 { arch/arm/boot/dts/rk3288-evb.dtsi- status = "okay"; arch/arm/boot/dts/rk3288-evb.dtsi-}; arch/arm/boot/dts/rk3288-firefly.dtsi: ddc-i2c-bus = <&i2c5>; arch/arm/boot/dts/rk3288-firefly.dtsi:&i2c5 { arch/arm/boot/dts/rk3288-firefly.dtsi- status = "okay"; arch/arm/boot/dts/rk3288-firefly.dtsi-}; arch/arm/boot/dts/rk3288-popmetal.dts: ddc-i2c-bus = <&i2c5>; arch/arm/boot/dts/rk3288-popmetal.dts:&i2c5 { arch/arm/boot/dts/rk3288-popmetal.dts- status = "okay"; arch/arm/boot/dts/rk3288-popmetal.dts-}; arch/arm/boot/dts/imx6q-gk802.dts: ddc-i2c-bus = <&i2c3>; arch/arm/boot/dts/imx6q-gk802.dts:&i2c3 { arch/arm/boot/dts/imx6q-gk802.dts- pinctrl-names = "default"; arch/arm/boot/dts/imx6q-gk802.dts: pinctrl-0 = <&pinctrl_i2c3>; arch/arm/boot/dts/imx6q-gk802.dts- clock-frequency = <100000>; arch/arm/boot/dts/imx6q-gw5400-a.dts: ddc-i2c-bus = <&i2c3>; arch/arm/boot/dts/imx6q-gw5400-a.dts:&i2c3 { arch/arm/boot/dts/imx6q-gw5400-a.dts- clock-frequency = <100000>; arch/arm/boot/dts/imx6qdl-apf6dev.dtsi: ddc-i2c-bus = <&i2c3>; arch/arm/boot/dts/imx6qdl-apf6dev.dtsi:&i2c3 { arch/arm/boot/dts/imx6qdl-apf6dev.dtsi- clock-frequency = <400000>; ...ah ha! Found one at 400kHz. ...but it sure looks like most people have decided that 100kHz is wiser... >>> +static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, >>> + struct i2c_msg *msgs, int num) >>> +{ >>> + struct dw_hdmi *hdmi = i2c_get_adapdata(adap); >>> + struct dw_hdmi_i2c *i2c = hdmi->i2c; >>> + u8 addr = msgs[0].addr; >>> + int i, ret = 0; >>> + >>> + dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr); >>> + >>> + for (i = 0; i < num; i++) { >>> + if (msgs[i].addr != addr) { >>> + dev_warn(hdmi->dev, >>> + "unsupported transfer, changed slave address\n"); >>> + return -EOPNOTSUPP; >>> + } >>> + >>> + if (msgs[i].len == 0) { >>> + dev_dbg(hdmi->dev, >>> + "unsupported transfer %d/%d, no data\n", >>> + i + 1, num); >>> + return -EOPNOTSUPP; >>> + } >>> + } >>> + >>> + mutex_lock(&i2c->lock); >>> + >> >> If I may make a suggestion, it's worth considering putting >> dw_hdmi_i2c_init() here and removing it from the dw_hdmi_bind() >> function. There isn't any state kept between i2c transfers (each one >> is independent), so re-initting each time won't hurt. ...and doing so >> has the potential to fix some things. > > I think it is resourse wasteful, especially since it does not solve any > actual problem, if I understand your arguments correctly. > > I'd like to see the I2C bus registered even if there is no ongoing > transfer, for instance running "modprobe i2c-dev; i2cdetect -l". > > For information I notice that there is a movement of I2C bus > configuration/capture from bind() to probe() in DRM drivers, e.g. see > commit 53bdcf5f026c . Skipping dw_hdmi_i2c_init() won't prevent the bus from showing up. We should still call i2c_add_adapter() at init time. I'm only suggesting calling dw_hdmi_i2c_init() at each transfer, and that does a tiny amount of work (set soft reset, init 4 extra registers). Note that in my tree moving it here actually does solve a real problem, but that's not a great argument for upstream. In my tree we have suspend/resume support, and the resume code wasn't calling dw_hdmi_i2c_init() (though it was needed). Thus putting the init() at the start of the transfer function does solve a real problem for me. In my case I could also have solved the problem by calling dw_hdmi_i2c_init() in the resume code, but (to me) adding 4 extra register writes for each i2c transfer seemed like a cleaner fix since I like the idea defensively programming and making sure we can't get stuck in a bad state. >> On rk3288 there's at least one case where the i2c controller can get >> wedged and just totally stops working. Although this particular wedge >> isn't fixed by calling dw_hdmi_i2c_init() (it needs a full controller >> reset to fix), it's possible that there may be other cases where the >> HDMI_I2CM_SOFTRSTZ fixes some bad state. > > I don't know, may be the shared part of the driver should export a > reset() function for RK3288, if it (or its next version) helps? Locally we've got the reset master wired up now in <https://chromium-review.googlesource.com/#/c/299601/>. ...but it appears that our tree is pretty far away from upstream in many ways at the moment, so this isn't trivial to just post upstream. :( Where I can I've still been trying to post upstream / give feedback upstream, but where there are too many differences it's a bit hard. Hopefully Rockchip will be able to get everything resolved, since I know several of their engineers participate upstream. > Unlickily I don't possess an RK3288 power board, but in any case I would > appreciate, if you share your test, I'll try to reproduce the same > problem on iMX6. It's possible it's a shared problem with all dw_hdmi users, but hard to know. I've got a pretty good description of it at <https://chromium-review.googlesource.com/#/c/299493/1/drivers/gpu/drm/bridge/dw_hdmi.c>. I probably won't land that change locally (nor submit it upstream) since IMHO doing the full reset of the HDMI part at HPD time is a better solution. -Doug
Hi, On Wed, Sep 16, 2015 at 2:16 PM, Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> wrote: > Hi Doug, > > On 03.09.2015 02:19, Doug Anderson wrote: >> Russell, >> >> On Wed, Sep 2, 2015 at 4:04 PM, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >>>>> Also, is it appropriate to hook non-DDC devices to a DDC bus? I suspect >>>>> that's asking for trouble. >>>> >>>> I doubt it's appropriate. Why do you ask? >>> >>> To find out why you want to "specify the I2C bus". >>> >>> Surely if we're talking about the DDC bus, we either want to use a >>> separate I2C bus (in which case the HDMI DT description needs to >>> specify which I2C bus to use) or we want to use the HDMI-internal >>> I2C bus, which being part of the HDMI driver, the HDMI driver will >>> know how to find it itself - there should be no need to put an >>> explicit ddc-i2c-bus self-reference there in that case. >> >> Overall it comes down to bus numbering. Possibly that's a stupid >> reason, but it is my reason nevertheless. > > this is a known issue regarding I2C bus numbering. > >> Specifically it significantly helps my brain process kernel log >> messages if the i2c bus that's referred to "bus 5" in my SoC's user >> manual shows up consistently as "i2c5" in kernel log messages. It's >> helpful it it shows up as "i2c5" even if "i2c0" - "i2c4" aren't >> enabled. >> >> That's all totally possible by using this type of syntax, like in rk3288.dtsi: >> >> aliases { >> i2c0 = &i2c0; >> i2c1 = &i2c1; >> i2c2 = &i2c2; >> i2c3 = &i2c3; >> i2c4 = &i2c4; >> i2c5 = &i2c5; >> >> Similarly, I'd like "bus 0" to show up as i2c0, which will happen as >> you can see in the above. >> >> The problem is that if another bus registers itself before the SoC's >> i2c0 registers itself and that bus doesn't give a number to itself >> then the i2c subsystem will chose "I2C 0". ...and then when the main >> SoC i2c bus registers itself it will fail because i2c0 is already >> taken. >> >> By having a of_node for the hdmi i2c bus, we can assign a number to it like: >> i2c15 = &hdmi; >> >> This is all described in the two links I referenced in my original reply. >> >> >> A possible other option is to have the i2c subsystem try to start >> numbering at a larger base for all automatically numbered busses >> (those that didn't specify a number). Then it's more likely (though >> still not guaranteed) to conflict with another bus... > > Could you please check if commit 03bde7c31a3 serves you? I'm 99.9% certain that it's the right fix. Sorry I wasn't aware of it. :( Personally I still think that it is correct to add "adap->dev.of_node = hdmi->dev->of_node;", but I won't yell if you don't. ;) While Russell's concerns are valid, my reading of his email was "there's no actual bug caused by doing this, but it's a bad idea in general to copy of_node". From my research into all other i2c busses, it appears that it is the convention to copy the of_node in the case of creating an i2c device. ...so since there is no actual bug, following convention seems like the right move. When someone fixes all the i2c busses because they have a patch making this work better then we should fix your i2c bus too. ...anyway, maybe that's too pragmatic of an opinion, but it is my opinion nonetheless. ;) -Doug
On Wed, Sep 16, 2015 at 02:56:57PM -0700, Doug Anderson wrote: > Yes, I'd expect 100kHz and 400kHz. > > I agree that 50ms is non-trivial, but it's also not something you're > doing lots of. I'd expect that the EDID is read over this channel at > cable plugin time and then not used much after that. Adding an extra > 40ms (10ms vs 50ms) before we can access the TV doesn't seem terrible > for compatibility. > > Doing a quick scan for what others in mainline do: > > A few can be found with: > > $ git grep -A3 hdmiddc | grep clock-freq > arch/arm/boot/dts/stihxxx-b2120.dtsi- > clock-frequency = <100000>; > arch/arm/boot/dts/tegra30-apalis.dtsi- clock-frequency = <100000>; > arch/arm/boot/dts/tegra30-beaver.dts- clock-frequency = <100000>; > arch/arm/boot/dts/tegra30-colibri.dtsi- clock-frequency = <100000>; This is a sure way to propagate a bug. I said in a previous email that you need to check the HDMI and CEA specs. I've done this, and HDMI 1.3a specifies a maximum SCL clock rate of 100kHz. So that's settled then. 100kHz is must be. Using 400kHz is out of specification.
On 17.09.2015 01:18, Russell King - ARM Linux wrote: > On Wed, Sep 16, 2015 at 02:56:57PM -0700, Doug Anderson wrote: >> Yes, I'd expect 100kHz and 400kHz. >> >> I agree that 50ms is non-trivial, but it's also not something you're >> doing lots of. I'd expect that the EDID is read over this channel at >> cable plugin time and then not used much after that. Adding an extra >> 40ms (10ms vs 50ms) before we can access the TV doesn't seem terrible >> for compatibility. >> >> Doing a quick scan for what others in mainline do: >> >> A few can be found with: >> >> $ git grep -A3 hdmiddc | grep clock-freq >> arch/arm/boot/dts/stihxxx-b2120.dtsi- >> clock-frequency = <100000>; >> arch/arm/boot/dts/tegra30-apalis.dtsi- clock-frequency = <100000>; >> arch/arm/boot/dts/tegra30-beaver.dts- clock-frequency = <100000>; >> arch/arm/boot/dts/tegra30-colibri.dtsi- clock-frequency = <100000>; > > This is a sure way to propagate a bug. > > I said in a previous email that you need to check the HDMI and CEA > specs. I've done this, and HDMI 1.3a specifies a maximum SCL clock > rate of 100kHz. > > So that's settled then. 100kHz is must be. Using 400kHz is out of > specification. > FYI I've managed to measure SCL on iMX6, fast mode stands for 333KHz and standard mode stands for 100KHz, therefore I'll set the latter mode. -- With best wishes, Vladimir
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c index 816d104..d329e04 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.c +++ b/drivers/gpu/drm/bridge/dw_hdmi.c @@ -1,14 +1,15 @@ /* + * DesignWare High-Definition Multimedia Interface (HDMI) driver + * + * Copyright (C) 2013-2015 Mentor Graphics Inc. * Copyright (C) 2011-2013 Freescale Semiconductor, Inc. + * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de> * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by * the Free Software Foundation; either version 2 of the License, or * (at your option) any later version. * - * Designware High-Definition Multimedia Interface (HDMI) driver - * - * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de> */ #include <linux/module.h> #include <linux/irq.h> @@ -102,6 +103,17 @@ struct hdmi_data_info { struct hdmi_vmode video_mode; }; +struct dw_hdmi_i2c { + struct i2c_adapter adap; + + struct mutex lock; + struct completion cmp; + u8 stat; + + u8 slave_reg; + bool is_regaddr; +}; + struct dw_hdmi { struct drm_connector connector; struct drm_encoder *encoder; @@ -111,6 +123,7 @@ struct dw_hdmi { struct device *dev; struct clk *isfr_clk; struct clk *iahb_clk; + struct dw_hdmi_i2c *i2c; struct hdmi_data_info hdmi_data; const struct dw_hdmi_plat_data *plat_data; @@ -179,6 +192,200 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg, hdmi_modb(hdmi, data << shift, mask, reg); } +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi) +{ + /* Set Fast Mode speed */ + hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV); + + /* Software reset */ + hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ); + + /* Set done, not acknowledged and arbitration interrupt polarities */ + hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT); + hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL, + HDMI_I2CM_CTLINT); + + /* Clear DONE and ERROR interrupts */ + hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE, + HDMI_IH_I2CM_STAT0); + + /* Mute DONE and ERROR interrupts */ + hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE, + HDMI_IH_MUTE_I2CM_STAT0); +} + +static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi, + unsigned char *buf, unsigned int length) +{ + struct dw_hdmi_i2c *i2c = hdmi->i2c; + int stat; + + if (!i2c->is_regaddr) { + dev_dbg(hdmi->dev, "set read register address to 0\n"); + i2c->slave_reg = 0x00; + i2c->is_regaddr = true; + } + + while (length--) { + reinit_completion(&i2c->cmp); + + hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS); + hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ, + HDMI_I2CM_OPERATION); + + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); + if (!stat) + return -EAGAIN; + + /* Check for error condition on the bus */ + if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) + return -EIO; + + *buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI); + } + + return 0; +} + +static int dw_hdmi_i2c_write(struct dw_hdmi *hdmi, + unsigned char *buf, unsigned int length) +{ + struct dw_hdmi_i2c *i2c = hdmi->i2c; + int stat; + + if (!i2c->is_regaddr) { + /* Use the first write byte as register address */ + i2c->slave_reg = buf[0]; + length--; + buf++; + i2c->is_regaddr = true; + } + + while (length--) { + reinit_completion(&i2c->cmp); + + hdmi_writeb(hdmi, *buf++, HDMI_I2CM_DATAO); + hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS); + hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_WRITE, + HDMI_I2CM_OPERATION); + + stat = wait_for_completion_timeout(&i2c->cmp, HZ / 10); + if (!stat) + return -EAGAIN; + + /* Check for error condition on the bus */ + if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) + return -EIO; + } + + return 0; +} + +static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, + struct i2c_msg *msgs, int num) +{ + struct dw_hdmi *hdmi = i2c_get_adapdata(adap); + struct dw_hdmi_i2c *i2c = hdmi->i2c; + u8 addr = msgs[0].addr; + int i, ret = 0; + + dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr); + + for (i = 0; i < num; i++) { + if (msgs[i].addr != addr) { + dev_warn(hdmi->dev, + "unsupported transfer, changed slave address\n"); + return -EOPNOTSUPP; + } + + if (msgs[i].len == 0) { + dev_dbg(hdmi->dev, + "unsupported transfer %d/%d, no data\n", + i + 1, num); + return -EOPNOTSUPP; + } + } + + mutex_lock(&i2c->lock); + + hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0); + + /* Set slave device address taken from the first I2C message */ + hdmi_writeb(hdmi, addr, HDMI_I2CM_SLAVE); + + /* Set slave device register address on transfer */ + i2c->is_regaddr = false; + + for (i = 0; i < num; i++) { + dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: %#x\n", + i + 1, num, msgs[i].len, msgs[i].flags); + + if (msgs[i].flags & I2C_M_RD) + ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len); + else + ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len); + + if (ret < 0) + break; + } + + if (!ret) + ret = num; + + /* Mute DONE and ERROR interrupts */ + hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE, + HDMI_IH_MUTE_I2CM_STAT0); + + mutex_unlock(&i2c->lock); + + return ret; +} + +static u32 dw_hdmi_i2c_func(struct i2c_adapter *adapter) +{ + return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL; +} + +static const struct i2c_algorithm dw_hdmi_algorithm = { + .master_xfer = dw_hdmi_i2c_xfer, + .functionality = dw_hdmi_i2c_func, +}; + +static struct i2c_adapter *dw_hdmi_i2c_adapter(struct dw_hdmi *hdmi) +{ + struct i2c_adapter *adap; + struct dw_hdmi_i2c *i2c; + int ret; + + i2c = devm_kzalloc(hdmi->dev, sizeof(*i2c), GFP_KERNEL); + if (!i2c) + return ERR_PTR(-ENOMEM); + + mutex_init(&i2c->lock); + init_completion(&i2c->cmp); + + adap = &i2c->adap; + adap->class = I2C_CLASS_DDC; + adap->owner = THIS_MODULE; + adap->dev.parent = hdmi->dev; + adap->algo = &dw_hdmi_algorithm; + strlcpy(adap->name, "DesignWare HDMI", sizeof(adap->name)); + i2c_set_adapdata(adap, hdmi); + + ret = i2c_add_adapter(adap); + if (ret) { + dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name); + devm_kfree(hdmi->dev, i2c); + return ERR_PTR(ret); + } + + hdmi->i2c = i2c; + + dev_info(hdmi->dev, "registered %s I2C bus driver\n", adap->name); + + return adap; +} + static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts, unsigned int n) { @@ -1466,16 +1673,40 @@ static struct drm_bridge_funcs dw_hdmi_bridge_funcs = { .mode_fixup = dw_hdmi_bridge_mode_fixup, }; +static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi) +{ + struct dw_hdmi_i2c *i2c = hdmi->i2c; + unsigned int stat; + + stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0); + if (!stat) + return IRQ_NONE; + + hdmi_writeb(hdmi, stat, HDMI_IH_I2CM_STAT0); + + i2c->stat = stat; + + complete(&i2c->cmp); + + return IRQ_HANDLED; +} + static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id) { struct dw_hdmi *hdmi = dev_id; u8 intr_stat; + irqreturn_t ret = IRQ_NONE; + + if (hdmi->i2c) + ret = dw_hdmi_i2c_irq(hdmi); intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0); - if (intr_stat) + if (intr_stat) { hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0); + return IRQ_WAKE_THREAD; + } - return intr_stat ? IRQ_WAKE_THREAD : IRQ_NONE; + return ret; } static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) @@ -1654,6 +1885,13 @@ int dw_hdmi_bind(struct device *dev, struct device *master, */ hdmi_init_clk_regenerator(hdmi); + /* If DDC bus is not specified, try to register HDMI I2C bus */ + if (!hdmi->ddc) { + hdmi->ddc = dw_hdmi_i2c_adapter(hdmi); + if (IS_ERR(hdmi->ddc)) + hdmi->ddc = NULL; + } + /* * Configure registers related to HDMI interrupt * generation before registering IRQ. @@ -1674,11 +1912,18 @@ int dw_hdmi_bind(struct device *dev, struct device *master, /* Unmute interrupts */ hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0); + /* Unmute I2CM interrupts and reset HDMI DDC I2C master controller */ + if (hdmi->i2c) + dw_hdmi_i2c_init(hdmi); + dev_set_drvdata(dev, hdmi); return 0; err_iahb: + if (hdmi->i2c) + i2c_del_adapter(&hdmi->i2c->adap); + clk_disable_unprepare(hdmi->iahb_clk); err_isfr: clk_disable_unprepare(hdmi->isfr_clk); @@ -1699,13 +1944,18 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data) clk_disable_unprepare(hdmi->iahb_clk); clk_disable_unprepare(hdmi->isfr_clk); - i2c_put_adapter(hdmi->ddc); + + if (hdmi->i2c) + i2c_del_adapter(&hdmi->i2c->adap); + else + i2c_put_adapter(hdmi->ddc); } EXPORT_SYMBOL_GPL(dw_hdmi_unbind); MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>"); MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>"); MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>"); +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>"); MODULE_DESCRIPTION("DW HDMI transmitter driver"); MODULE_LICENSE("GPL"); MODULE_ALIAS("platform:dw-hdmi"); diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h index ee7f7ed..9ed85eb 100644 --- a/drivers/gpu/drm/bridge/dw_hdmi.h +++ b/drivers/gpu/drm/bridge/dw_hdmi.h @@ -563,6 +563,10 @@ enum { HDMI_IH_PHY_STAT0_TX_PHY_LOCK = 0x2, HDMI_IH_PHY_STAT0_HPD = 0x1, +/* IH_I2CM_STAT0 and IH_MUTE_I2CM_STAT0 field values */ + HDMI_IH_I2CM_STAT0_DONE = 0x2, + HDMI_IH_I2CM_STAT0_ERROR = 0x1, + /* IH_MUTE_I2CMPHY_STAT0 field values */ HDMI_IH_MUTE_I2CMPHY_STAT0_I2CMPHYDONE = 0x2, HDMI_IH_MUTE_I2CMPHY_STAT0_I2CMPHYERROR = 0x1, @@ -1029,6 +1033,21 @@ enum { HDMI_A_VIDPOLCFG_HSYNCPOL_MASK = 0x2, HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_HIGH = 0x2, HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0, + +/* I2CM_OPERATION field values */ + HDMI_I2CM_OPERATION_WRITE = 0x10, + HDMI_I2CM_OPERATION_READ_EXT = 0x2, + HDMI_I2CM_OPERATION_READ = 0x1, + +/* I2CM_INT field values */ + HDMI_I2CM_INT_DONE_POL = 0x8, + HDMI_I2CM_INT_DONE_MASK = 0x4, + +/* I2CM_CTLINT field values */ + HDMI_I2CM_CTLINT_NAC_POL = 0x80, + HDMI_I2CM_CTLINT_NAC_MASK = 0x40, + HDMI_I2CM_CTLINT_ARB_POL = 0x8, + HDMI_I2CM_CTLINT_ARB_MASK = 0x4, }; #endif /* __DW_HDMI_H__ */