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

[Russell King - ARM Linux <linux@arm.linux.org.uk>]

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel