[PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support
Hi Russell,
On 14.08.2015 01:56, Russell King - ARM Linux wrote:
> 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_ERRORBIT(0)
>> +#define HDMI_IH_I2CM_STAT0_DONE BIT(1)
>> +
>> +/* HDMI_I2CM_OPERATION register bits */
>> +#define HDMI_I2CM_OPERATION_READBIT(0)
>> +#define HDMI_I2CM_OPERATION_READ_EXTBIT(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_POLBIT(3)
>> +#define HDMI_I2CM_CTLINT_NAC_MASK BIT(6)
>> +#define HDMI_I2CM_CTLINT_NAC_POLBIT(7)
>
> Please put these definitions in dw_hdmi.h
>
okay.
>> +
>> +
>> #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;
>> +boolis_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(>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(>i2c->lock, flags);
>
> What exactly is this spinlock protecting against with the above code?
>
On second inspection I don't see a need of locking here.
>> +}
>> +
>> +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(>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(>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.
Correct, I had only one completion and no i2c->operation_reg in v1, will
revert the added complexity to match the previous version
http://lists.freedesktop.org/archives/dri-devel/2015-May/082887.html
>> +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.
I agree, it can be removed.
>> +
>> +hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
>> +hdmi_writeb(hdmi, i2c->operation_reg, HDMI_I2CM_OPERATION);
>> +
>> +spin_unlock_irqrestore(>lock, flags);
>> +
>> +stat = wait_for_completion_interruptible_timeout(>cmp_r,
>> + HZ / 10);
>> +if (!stat)
>> +return -ETIMEDOUT;
>> +
[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_WRITEBIT(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_MASKBIT(2)
> +#define HDMI_I2CM_CTLINT_ARB_POL BIT(3)
> +#define HDMI_I2CM_CTLINT_NAC_MASKBIT(6)
> +#define HDMI_I2CM_CTLINT_NAC_POL BIT(7)
Please put these definitions in dw_hdmi.h
> +
> +
> #define HDMI_EDID_LEN512
>
> #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;
> + boolis_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(>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(>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(>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(>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(>lock, flags);
> +
> + stat = wait_for_completion_interruptible_timeout(>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,
[PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support
Hi Vladimir, Am Montag, den 18.05.2015, 15:32 +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. > > 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. > > Signed-off-by: Vladimir Zapolskiy Tested-by: Philipp Zabel on imx6q-nitrogen6x with the DDC pads muxed to HDMI_TX. regards Philipp
[PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support
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.
Signed-off-by: Vladimir Zapolskiy
---
Changes since v1:
- fixed a devm_kfree() signature
- split completions for read and write operations
drivers/gpu/drm/bridge/dw_hdmi.c | 341 ++-
1 file changed, 335 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 49cafb6..7f64e73 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1,15 +1,17 @@
/*
+ * 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
*
* 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
*/
+
#include
#include
#include
@@ -28,6 +30,26 @@
#include "dw_hdmi.h"
+/* 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_DONEBIT(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_MASKBIT(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)
+
+
#define HDMI_EDID_LEN 512
#define RGB0
@@ -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;
+ boolis_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(>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(>i2c->lock, flags);
+}
+
+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(>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(>cmp_r);
+ i2c->stat = 0;
+
+ hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
+ hdmi_writeb(hdmi, i2c->operation_reg,
