Re: [PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

On Mon, May 18, 2015 at 03:32:21PM +0300, Vladimir Zapolskiy wrote:

+/* HDMI_IH_I2CM_STAT0 and HDMI_IH_MUTE_I2CM_STAT0 register bits */ +#define HDMI_IH_I2CM_STAT0_ERROR BIT(0) +#define HDMI_IH_I2CM_STAT0_DONE BIT(1)

+/* HDMI_I2CM_OPERATION register bits */ +#define HDMI_I2CM_OPERATION_READ BIT(0) +#define HDMI_I2CM_OPERATION_READ_EXT BIT(1) +#define HDMI_I2CM_OPERATION_WRITE BIT(4)

+/* HDMI_I2CM_INT register bits */ +#define HDMI_I2CM_INT_DONE_MASK BIT(2) +#define HDMI_I2CM_INT_DONE_POL BIT(3)

+/* HDMI_I2CM_CTLINT register bits */ +#define HDMI_I2CM_CTLINT_ARB_MASK BIT(2) +#define HDMI_I2CM_CTLINT_ARB_POL BIT(3) +#define HDMI_I2CM_CTLINT_NAC_MASK BIT(6) +#define HDMI_I2CM_CTLINT_NAC_POL BIT(7)

Please put these definitions in dw_hdmi.h

#define HDMI_EDID_LEN 512

#define RGB 0 @@ -102,6 +124,19 @@ struct hdmi_data_info { struct hdmi_vmode video_mode; };

+struct dw_hdmi_i2c {

• struct i2c_adapter adap;
• spinlock_t lock;
• struct completion cmp_r;
• struct completion cmp_w;
• u8 stat;
• u8 operation_reg;
• u8 slave_reg;
• bool is_regaddr;

+};

struct dw_hdmi { struct drm_connector connector; struct drm_encoder *encoder; @@ -111,6 +146,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 +215,249 @@ 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) +{

• unsigned long flags;
• spin_lock_irqsave(&hdmi->i2c->lock, flags);
• /* 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);
  

• spin_unlock_irqrestore(&hdmi->i2c->lock, flags);

What exactly is this spinlock protecting against with the above code?

+}

+static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,

• 	    unsigned char *buf, int length)
  

+{

• int stat;
• unsigned long flags;
• struct dw_hdmi_i2c *i2c = hdmi->i2c;
• spin_lock_irqsave(&i2c->lock, flags);
• i2c->operation_reg = HDMI_I2CM_OPERATION_READ;
• 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_r);
  

If you're re-initialising the completion on every byte, why do you need separate completions for the read path and the write path?

If a single completion can be used, you then don't have to store the operation register value in struct dw_hdmi_i2c either.

• i2c->stat = 0;
  

What use does zeroing this have? You don't read it, except after you've had a successful completion, which implies that the interrupt handler must have written to it.

• hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
  

• hdmi_writeb(hdmi, i2c->operation_reg, HDMI_I2CM_OPERATION);
  

• spin_unlock_irqrestore(&i2c->lock, flags);
  

• stat = wait_for_completion_interruptible_timeout(&i2c->cmp_r,
  

• 						 HZ / 10);
  

• if (!stat)
  

• 	return -ETIMEDOUT;
  

• if (stat < 0)
  

• 	return stat;
  

Can a DDC read/write really be interrupted and restarted correctly? Won't this restart the transaction from the very beginning? Have you validated that all code paths calling into here can cope with -ERESTARTSYS?

If you look at drm_do_probe_ddc_edid() for example, it will retry the transfer if you return -ERESTARTSYS here, but as the signal has not been handled, wait_for_completion_interruptible_timeout() will immediately return -ERESTARTSYS again... and again... and again. Each time will queue another operation _without_ waiting for the last one to complete.

• spin_lock_irqsave(&i2c->lock, flags);
  

• /* Check for error condition on the bus */
  

• if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) {
  

• 	spin_unlock_irqrestore(&i2c->lock, flags);
  

• 	return -EIO;
  

• }
  

• *buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
  

• }
• spin_unlock_irqrestore(&i2c->lock, flags);

I'd be very surprised if you need the spinlocks in the above code. You'll see the update to i2c->stat after the completion, becuase wait_for_completion*() is in the same class as the other event-waiting functions, and contains barriers which will ensure that you will not read i2c->stat before you see the completion even on SMP platforms.

• 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;
• int i, ret;
• u8 addr;
• unsigned long flags;
• dev_dbg(hdmi->dev, "xfer: num: %d, addr: 0x%x\n", num, msgs[0].addr);
• spin_lock_irqsave(&i2c->lock, flags);
• hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0);
• /* Set slave device address from the first transaction */
• addr = msgs[0].addr;
• hdmi_writeb(hdmi, addr, HDMI_I2CM_SLAVE);
• /* Set slave device register address on transfer */
• i2c->is_regaddr = false;
• spin_unlock_irqrestore(&i2c->lock, flags);
• for (i = 0; i < num; i++) {

• dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: 0x%x\n",
  

• 	i + 1, num, msgs[i].len, msgs[i].flags);
  

• if (msgs[i].addr != addr) {
  

• 	dev_warn(hdmi->dev,
  

• 		 "unsupported transaction, changed slave address\n");
  

• 	ret = -EOPNOTSUPP;
  

• 	break;
  

• }
  

Probably ought to validate that before starting any transaction?

• if (msgs[i].len == 0) {
  

• 	dev_dbg(hdmi->dev,
  

• 		 "unsupported transaction %d/%d, no data\n",
  

• 		 i + 1, num);
  

• 	ret = -EOPNOTSUPP;
  

• 	break;
  

• }
  

Ditto.

• 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;
  

• spin_lock_irqsave(&i2c->lock, flags);
• /* Mute DONE and ERROR interrupts */
• hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,

•     HDMI_IH_MUTE_I2CM_STAT0);
  

• spin_unlock_irqrestore(&i2c->lock, flags);

What exactly is this spinlock protecting us from? A single write to a register is always atomic.

• return ret;

+}

+static u32 dw_hdmi_i2c_func(struct i2c_adapter *adapter) +{

• return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;

+}

+static struct i2c_algorithm dw_hdmi_algorithm = {

const?

• .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;
• int ret;
• hdmi->i2c = devm_kzalloc(hdmi->dev, sizeof(*hdmi->i2c), GFP_KERNEL);
• if (!hdmi->i2c)

• return ERR_PTR(-ENOMEM);
  

I'd much prefer: struct dw_hdmi_i2c *i2c;

i2c = devm_kzalloc(...);

• spin_lock_init(&hdmi->i2c->lock);
• init_completion(&hdmi->i2c->cmp_r);
• init_completion(&hdmi->i2c->cmp_w);
• adap = &hdmi->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, hdmi->i2c);
  

• hdmi->i2c = NULL;
  

• return ERR_PTR(ret);
  

• }

hdmi->i2c = i2c;

rather than having to remember to clear out hdmi->i2c in error paths.

• 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 +1745,47 @@ 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 long flags;
• spin_lock_irqsave(&i2c->lock, flags);
• i2c->stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0);
• if (!i2c->stat) {

• spin_unlock_irqrestore(&i2c->lock, flags);
  

• return IRQ_NONE;
  

• }
• hdmi_writeb(hdmi, i2c->stat, HDMI_IH_I2CM_STAT0);
• if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ)

• complete(&i2c->cmp_r);
  

• else

• complete(&i2c->cmp_w);
  

• spin_unlock_irqrestore(&i2c->lock, flags);

Again, what function does this spinlock perform? Wouldn't:

unsigned int stat;

stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0); if (stat == 0) return IRQ_NONE;

/* Clear interrupts */ hdmi_writeb(hdmi, stat, HDMI_IH_I2CM_STAT0);

i2c->stat = stat;

if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ) complete(&i2c->cmp_r); else complete(&i2c->cmp_w);

be fine, or maybe with the spinlock around the assignment to i2c->stat and this point if you need the assignment and completion to be atomic?

Note that the write to 'i2c->stat' would be atomic in itself anyway.

In any case, complete() implies a write memory barrier, so even on SMP systems, the write to i2c->stat will be visible before the completion "completes". So, I don't think you need any locking here.

Thanks.