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.
