[PATCH v3 0/3] i2c-s3c2410: Updates for exynos4210 and DT-based systems

[PATCH v3 0/3] i2c-s3c2410: Updates for exynos4210 and DT-based systems

[Karol Lewandowski]

Changes since v2:
 - Merge device type and flags into flat bitmask named quirks -
   Consequently, treat s3c24xx as baseline hardware platform and
   support all hw variations via quirks [Suggested by Mark Brown]

Changes since v1:
 - Move unrelated code fragment to separate patch (of_match_ptr())
   [Suggested by Thomas Abracham]
 - Move device-type handling to separate function and rework its
   internals a bit [likewise]

This patchset reworks i2c-s3c2410 driver a bit to better handle
device tree-enabled platforms and adds two quirks required by
exynos4210-specific I2C controller used by s5p-hdmi driver.

This patchset is based on "i2c-bjdooks/for-34/i2c/i2c-samsung" branch
taken from:

  git://git.fluff.org/bjdooks/linux.git

Karol Lewandowski (3):
  i2c-s3c2410: Drop unused define
  i2c-s3c2410: Rework device type handling
  i2c-s3c2410: Add HDMIPHY quirk for S3C2440

 .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 ++-
 drivers/i2c/busses/i2c-s3c2410.c                   |   86 +++++++++++++------
 2 files changed, 68 insertions(+), 29 deletions(-)

-- 
1.7.9

[PATCH v3 0/3] i2c-s3c2410: Updates for exynos4210 and DT-based systems

[Karol Lewandowski]

Changes since v2:
 - Merge device type and flags into flat bitmask named quirks -
   Consequently, treat s3c24xx as baseline hardware platform and
   support all hw variations via quirks [Suggested by Mark Brown]

Changes since v1:
 - Move unrelated code fragment to separate patch (of_match_ptr())
   [Suggested by Thomas Abracham]
 - Move device-type handling to separate function and rework its
   internals a bit [likewise]

This patchset reworks i2c-s3c2410 driver a bit to better handle
device tree-enabled platforms and adds two quirks required by
exynos4210-specific I2C controller used by s5p-hdmi driver.

This patchset is based on "i2c-bjdooks/for-34/i2c/i2c-samsung" branch
taken from:

  git://git.fluff.org/bjdooks/linux.git

Karol Lewandowski (3):
  i2c-s3c2410: Drop unused define
  i2c-s3c2410: Rework device type handling
  i2c-s3c2410: Add HDMIPHY quirk for S3C2440

 .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 ++-
 drivers/i2c/busses/i2c-s3c2410.c                   |   86 +++++++++++++------
 2 files changed, 68 insertions(+), 29 deletions(-)

-- 
1.7.9

[PATCH 1/3] i2c-s3c2410: Drop unused define

[Karol Lewandowski]

Use standard of_match_ptr() to avoid defining variable unused
in non device tree builds.

Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
---
 drivers/i2c/busses/i2c-s3c2410.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 737f721..85e3664 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -1128,8 +1128,6 @@ static const struct of_device_id s3c24xx_i2c_match[] = {
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
-#else
-#define s3c24xx_i2c_match NULL
 #endif
 
 static struct platform_driver s3c24xx_i2c_driver = {
@@ -1140,7 +1138,7 @@ static struct platform_driver s3c24xx_i2c_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "s3c-i2c",
 		.pm	= S3C24XX_DEV_PM_OPS,
-		.of_match_table = s3c24xx_i2c_match,
+		.of_match_table = of_match_ptr(s3c24xx_i2c_match),
 	},
 };
 
-- 
1.7.9

[PATCH 1/3] i2c-s3c2410: Drop unused define

[Karol Lewandowski]

Use standard of_match_ptr() to avoid defining variable unused
in non device tree builds.

Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
---
 drivers/i2c/busses/i2c-s3c2410.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 737f721..85e3664 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -1128,8 +1128,6 @@ static const struct of_device_id s3c24xx_i2c_match[] = {
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
-#else
-#define s3c24xx_i2c_match NULL
 #endif
 
 static struct platform_driver s3c24xx_i2c_driver = {
@@ -1140,7 +1138,7 @@ static struct platform_driver s3c24xx_i2c_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "s3c-i2c",
 		.pm	= S3C24XX_DEV_PM_OPS,
-		.of_match_table = s3c24xx_i2c_match,
+		.of_match_table = of_match_ptr(s3c24xx_i2c_match),
 	},
 };
 
-- 
1.7.9

[PATCH 2/3] i2c-s3c2410: Rework device type handling

[Karol Lewandowski]

Reorganize driver a bit to better handle device tree-based systems:

 - move machine type to driver's private structure instead of
   quering platform device variants in runtime

 - replace s3c24xx_i2c_type enum with unsigned int that holds
   bitmask with revision-specific quirks

Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/i2c/busses/i2c-s3c2410.c |   47 ++++++++++++++++++-------------------
 1 files changed, 23 insertions(+), 24 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 85e3664..f7b6a14 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -44,8 +44,14 @@
 #include <plat/regs-iic.h>
 #include <plat/iic.h>
 
-/* i2c controller state */
+#ifdef CONFIG_OF
+static const struct of_device_id s3c24xx_i2c_match[];
+#endif
 
+/* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
+#define QUIRK_S3C2440		(1 << 0)
+
+/* i2c controller state */
 enum s3c24xx_i2c_state {
 	STATE_IDLE,
 	STATE_START,
@@ -54,14 +60,10 @@ enum s3c24xx_i2c_state {
 	STATE_STOP
 };
 
-enum s3c24xx_i2c_type {
-	TYPE_S3C2410,
-	TYPE_S3C2440,
-};
-
 struct s3c24xx_i2c {
 	spinlock_t		lock;
 	wait_queue_head_t	wait;
+	unsigned int            quirks;
 	unsigned int		suspended:1;
 
 	struct i2c_msg		*msg;
@@ -88,26 +90,22 @@ struct s3c24xx_i2c {
 #endif
 };
 
-/* default platform data removed, dev should always carry data. */
-
-/* s3c24xx_i2c_is2440()
+/* s3c24xx_get_device_quirks
  *
- * return true is this is an s3c2440
+ * Get controller type either from device tree or platform device variant.
 */
 
-static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
+static inline unsigned int s3c24xx_get_device_quirks(struct platform_device *pdev)
 {
-	struct platform_device *pdev = to_platform_device(i2c->dev);
-	enum s3c24xx_i2c_type type;
-
 #ifdef CONFIG_OF
-	if (i2c->dev->of_node)
-		return of_device_is_compatible(i2c->dev->of_node,
-				"samsung,s3c2440-i2c");
+        if (pdev->dev.of_node) {
+		const struct of_device_id *match;
+		match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);
+		return (unsigned int)match->data;
+        }
 #endif
 
-	type = platform_get_device_id(pdev)->driver_data;
-	return type == TYPE_S3C2440;
+	return platform_get_device_id(pdev)->driver_data;
 }
 
 /* s3c24xx_i2c_master_complete
@@ -676,7 +674,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c, unsigned int *got)
 
 	writel(iiccon, i2c->regs + S3C2410_IICCON);
 
-	if (s3c24xx_i2c_is2440(i2c)) {
+	if (i2c->quirks & QUIRK_S3C2440) {
 		unsigned long sda_delay;
 
 		if (pdata->sda_delay) {
@@ -906,6 +904,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 		goto err_noclk;
 	}
 
+	i2c->quirks = s3c24xx_get_device_quirks(pdev);
 	if (pdata)
 		memcpy(i2c->pdata, pdata, sizeof(*pdata));
 	else
@@ -1113,18 +1112,18 @@ static const struct dev_pm_ops s3c24xx_i2c_dev_pm_ops = {
 static struct platform_device_id s3c24xx_driver_ids[] = {
 	{
 		.name		= "s3c2410-i2c",
-		.driver_data	= TYPE_S3C2410,
+		.driver_data	= 0,
 	}, {
 		.name		= "s3c2440-i2c",
-		.driver_data	= TYPE_S3C2440,
+		.driver_data	= QUIRK_S3C2440,
 	}, { },
 };
 MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
 
 #ifdef CONFIG_OF
 static const struct of_device_id s3c24xx_i2c_match[] = {
-	{ .compatible = "samsung,s3c2410-i2c" },
-	{ .compatible = "samsung,s3c2440-i2c" },
+	{ .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
+	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
-- 
1.7.9

[PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Karol Lewandowski]

This patch adds support for s3c2440 I2C bus controller dedicated HDMIPHY device on
Exynos4 platform. Some quirks are introduced due to differences between HDMIPHY
and other I2C controllers on Exynos4.  These differences are:
- no GPIOs, HDMIPHY is inside the SoC and the controller is connected
  internally
- due to unknown reason (probably HW bug in HDMIPHY and/or the controller) a
  transfer fails to finish. The controller hangs after sending the last byte,
  the workaround for this bug is resetting the controller after each transfer

Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 +++++-
 drivers/i2c/busses/i2c-s3c2410.c                   |   35 ++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
index 38832c7..c6670f9 100644
--- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
@@ -10,14 +10,21 @@ Required properties:
     region.
   - interrupts: interrupt number to the cpu.
   - samsung,i2c-sda-delay: Delay (in ns) applied to data line (SDA) edges.
-  - gpios: The order of the gpios should be the following: <SDA, SCL>.
-    The gpio specifier depends on the gpio controller.
 
 Optional properties:
+  - gpios: The order of the gpios should be the following: <SDA, SCL>.
+    The gpio specifier depends on the gpio controller. Required in all cases
+    except when "samsung,i2c-no-gpio" is also specified.
+  - samsung,i2c-no-gpio: input/output lines of the controller are
+    permanently wired to the respective client, there are no gpio
+    lines that need to be configured to enable this controller
   - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
     specified, default value is 0.
   - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
     specified, the default value in Hz is 100000.
+  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
+    Exynos4 platform - reduce timeout and reset controller after each
+    transfer
 
 Example:
 
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index f7b6a14..e50f523 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -50,6 +50,8 @@ static const struct of_device_id s3c24xx_i2c_match[];
 
 /* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
 #define QUIRK_S3C2440		(1 << 0)
+#define QUIRK_HDMIPHY		(1 << 1)
+#define QUIRK_NO_GPIO		(1 << 2)
 
 /* i2c controller state */
 enum s3c24xx_i2c_state {
@@ -469,6 +471,13 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
 	unsigned long iicstat;
 	int timeout = 400;
 
+	/* the timeout for HDMIPHY is reduced to 10 ms because
+	 * the hangup is expected to happen, so waiting 400 ms
+	 * causes only unnecessary system hangup
+	 */
+	if (i2c->quirks & QUIRK_HDMIPHY)
+		timeout = 10;
+
 	while (timeout-- > 0) {
 		iicstat = readl(i2c->regs + S3C2410_IICSTAT);
 
@@ -478,6 +487,15 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
 		msleep(1);
 	}
 
+	/* hang-up of bus dedicated for HDMIPHY occurred, resetting */
+	if (i2c->quirks & QUIRK_HDMIPHY) {
+		writel(0, i2c->regs + S3C2410_IICCON);
+		writel(0, i2c->regs + S3C2410_IICSTAT);
+		writel(0, i2c->regs + S3C2410_IICDS);
+
+		return 0;
+	}
+
 	return -ETIMEDOUT;
 }
 
@@ -759,6 +777,9 @@ static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
 {
 	int idx, gpio, ret;
 
+	if (i2c->quirks & QUIRK_NO_GPIO)
+		return 0;
+
 	for (idx = 0; idx < 2; idx++) {
 		gpio = of_get_gpio(i2c->dev->of_node, idx);
 		if (!gpio_is_valid(gpio)) {
@@ -783,6 +804,10 @@ free_gpio:
 static void s3c24xx_i2c_dt_gpio_free(struct s3c24xx_i2c *i2c)
 {
 	unsigned int idx;
+
+	if (i2c->quirks & QUIRK_NO_GPIO)
+		return;
+
 	for (idx = 0; idx < 2; idx++)
 		gpio_free(i2c->gpios[idx]);
 }
@@ -859,6 +884,13 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
 		return;
 
 	pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
+
+	if (of_get_property(np, "samsung,i2c-quirk-hdmiphy", NULL))
+		i2c->quirks |= QUIRK_HDMIPHY;
+
+	if (of_get_property(np, "samsung,i2c-no-gpio", NULL))
+		i2c->quirks |= QUIRK_NO_GPIO;
+
 	of_property_read_u32(np, "samsung,i2c-sda-delay", &pdata->sda_delay);
 	of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);
 	of_property_read_u32(np, "samsung,i2c-max-bus-freq",
@@ -1116,6 +1148,9 @@ static struct platform_device_id s3c24xx_driver_ids[] = {
 	}, {
 		.name		= "s3c2440-i2c",
 		.driver_data	= QUIRK_S3C2440,
+	}, {
+		.name		= "s3c2440-hdmiphy-i2c",
+		.driver_data	= QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO,
 	}, { },
 };
 MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
-- 
1.7.9

[PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Karol Lewandowski]

This patch adds support for s3c2440 I2C bus controller dedicated HDMIPHY device on
Exynos4 platform. Some quirks are introduced due to differences between HDMIPHY
and other I2C controllers on Exynos4.  These differences are:
- no GPIOs, HDMIPHY is inside the SoC and the controller is connected
  internally
- due to unknown reason (probably HW bug in HDMIPHY and/or the controller) a
  transfer fails to finish. The controller hangs after sending the last byte,
  the workaround for this bug is resetting the controller after each transfer

Signed-off-by: Tomasz Stanislawski <t.stanislaws-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Tested-by: Tomasz Stanislawski <t.stanislaws-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 +++++-
 drivers/i2c/busses/i2c-s3c2410.c                   |   35 ++++++++++++++++++++
 2 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
index 38832c7..c6670f9 100644
--- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
@@ -10,14 +10,21 @@ Required properties:
     region.
   - interrupts: interrupt number to the cpu.
   - samsung,i2c-sda-delay: Delay (in ns) applied to data line (SDA) edges.
-  - gpios: The order of the gpios should be the following: <SDA, SCL>.
-    The gpio specifier depends on the gpio controller.
 
 Optional properties:
+  - gpios: The order of the gpios should be the following: <SDA, SCL>.
+    The gpio specifier depends on the gpio controller. Required in all cases
+    except when "samsung,i2c-no-gpio" is also specified.
+  - samsung,i2c-no-gpio: input/output lines of the controller are
+    permanently wired to the respective client, there are no gpio
+    lines that need to be configured to enable this controller
   - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
     specified, default value is 0.
   - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
     specified, the default value in Hz is 100000.
+  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
+    Exynos4 platform - reduce timeout and reset controller after each
+    transfer
 
 Example:
 
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index f7b6a14..e50f523 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -50,6 +50,8 @@ static const struct of_device_id s3c24xx_i2c_match[];
 
 /* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
 #define QUIRK_S3C2440		(1 << 0)
+#define QUIRK_HDMIPHY		(1 << 1)
+#define QUIRK_NO_GPIO		(1 << 2)
 
 /* i2c controller state */
 enum s3c24xx_i2c_state {
@@ -469,6 +471,13 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
 	unsigned long iicstat;
 	int timeout = 400;
 
+	/* the timeout for HDMIPHY is reduced to 10 ms because
+	 * the hangup is expected to happen, so waiting 400 ms
+	 * causes only unnecessary system hangup
+	 */
+	if (i2c->quirks & QUIRK_HDMIPHY)
+		timeout = 10;
+
 	while (timeout-- > 0) {
 		iicstat = readl(i2c->regs + S3C2410_IICSTAT);
 
@@ -478,6 +487,15 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
 		msleep(1);
 	}
 
+	/* hang-up of bus dedicated for HDMIPHY occurred, resetting */
+	if (i2c->quirks & QUIRK_HDMIPHY) {
+		writel(0, i2c->regs + S3C2410_IICCON);
+		writel(0, i2c->regs + S3C2410_IICSTAT);
+		writel(0, i2c->regs + S3C2410_IICDS);
+
+		return 0;
+	}
+
 	return -ETIMEDOUT;
 }
 
@@ -759,6 +777,9 @@ static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
 {
 	int idx, gpio, ret;
 
+	if (i2c->quirks & QUIRK_NO_GPIO)
+		return 0;
+
 	for (idx = 0; idx < 2; idx++) {
 		gpio = of_get_gpio(i2c->dev->of_node, idx);
 		if (!gpio_is_valid(gpio)) {
@@ -783,6 +804,10 @@ free_gpio:
 static void s3c24xx_i2c_dt_gpio_free(struct s3c24xx_i2c *i2c)
 {
 	unsigned int idx;
+
+	if (i2c->quirks & QUIRK_NO_GPIO)
+		return;
+
 	for (idx = 0; idx < 2; idx++)
 		gpio_free(i2c->gpios[idx]);
 }
@@ -859,6 +884,13 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
 		return;
 
 	pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
+
+	if (of_get_property(np, "samsung,i2c-quirk-hdmiphy", NULL))
+		i2c->quirks |= QUIRK_HDMIPHY;
+
+	if (of_get_property(np, "samsung,i2c-no-gpio", NULL))
+		i2c->quirks |= QUIRK_NO_GPIO;
+
 	of_property_read_u32(np, "samsung,i2c-sda-delay", &pdata->sda_delay);
 	of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);
 	of_property_read_u32(np, "samsung,i2c-max-bus-freq",
@@ -1116,6 +1148,9 @@ static struct platform_device_id s3c24xx_driver_ids[] = {
 	}, {
 		.name		= "s3c2440-i2c",
 		.driver_data	= QUIRK_S3C2440,
+	}, {
+		.name		= "s3c2440-hdmiphy-i2c",
+		.driver_data	= QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO,
 	}, { },
 };
 MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
-- 
1.7.9

Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 436 bytes --]

On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote:
> Reorganize driver a bit to better handle device tree-based systems:
> 
>  - move machine type to driver's private structure instead of
>    quering platform device variants in runtime
> 
>  - replace s3c24xx_i2c_type enum with unsigned int that holds
>    bitmask with revision-specific quirks

Reviewed-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 469 bytes --]

On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote:
> Reorganize driver a bit to better handle device tree-based systems:
> 
>  - move machine type to driver's private structure instead of
>    quering platform device variants in runtime
> 
>  - replace s3c24xx_i2c_type enum with unsigned int that holds
>    bitmask with revision-specific quirks

Reviewed-by: Mark Brown <broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 0/3] i2c-s3c2410: Updates for exynos4210 and DT-based systems

[Karol Lewandowski]

On 21.03.2012 20:11, Karol Lewandowski wrote:

> Changes since v2:
>  - Merge device type and flags into flat bitmask named quirks -
>    Consequently, treat s3c24xx as baseline hardware platform and
>    support all hw variations via quirks [Suggested by Mark Brown]
> 
> Changes since v1:
>  - Move unrelated code fragment to separate patch (of_match_ptr())
>    [Suggested by Thomas Abraham]
>  - Move device-type handling to separate function and rework its
>    internals a bit [likewise]
> 
> This patchset reworks i2c-s3c2410 driver a bit to better handle
> device tree-enabled platforms and adds two quirks required by
> exynos4210-specific I2C controller used by s5p-hdmi driver.
> 
> This patchset is based on "i2c-bjdooks/for-34/i2c/i2c-samsung" branch
> taken from:
> 
>   git://git.fluff.org/bjdooks/linux.git
> 
> Karol Lewandowski (3):
>   i2c-s3c2410: Drop unused define
>   i2c-s3c2410: Rework device type handling
>   i2c-s3c2410: Add HDMIPHY quirk for S3C2440
> 
>  .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 ++-
>  drivers/i2c/busses/i2c-s3c2410.c                   |   86 +++++++++++++------
>  2 files changed, 68 insertions(+), 29 deletions(-)


Wolfram,

Is there anything I can do to have these patches in -next
(and, consequently, in 3.5-rc1)?

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH v3 0/3] i2c-s3c2410: Updates for exynos4210 and DT-based systems

[Karol Lewandowski]

On 21.03.2012 20:11, Karol Lewandowski wrote:

> Changes since v2:
>  - Merge device type and flags into flat bitmask named quirks -
>    Consequently, treat s3c24xx as baseline hardware platform and
>    support all hw variations via quirks [Suggested by Mark Brown]
> 
> Changes since v1:
>  - Move unrelated code fragment to separate patch (of_match_ptr())
>    [Suggested by Thomas Abraham]
>  - Move device-type handling to separate function and rework its
>    internals a bit [likewise]
> 
> This patchset reworks i2c-s3c2410 driver a bit to better handle
> device tree-enabled platforms and adds two quirks required by
> exynos4210-specific I2C controller used by s5p-hdmi driver.
> 
> This patchset is based on "i2c-bjdooks/for-34/i2c/i2c-samsung" branch
> taken from:
> 
>   git://git.fluff.org/bjdooks/linux.git
> 
> Karol Lewandowski (3):
>   i2c-s3c2410: Drop unused define
>   i2c-s3c2410: Rework device type handling
>   i2c-s3c2410: Add HDMIPHY quirk for S3C2440
> 
>  .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 ++-
>  drivers/i2c/busses/i2c-s3c2410.c                   |   86 +++++++++++++------
>  2 files changed, 68 insertions(+), 29 deletions(-)


Wolfram,

Is there anything I can do to have these patches in -next
(and, consequently, in 3.5-rc1)?

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 5013 bytes --]

Hi,

On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote:
> Reorganize driver a bit to better handle device tree-based systems:
> 
>  - move machine type to driver's private structure instead of
>    quering platform device variants in runtime
> 
>  - replace s3c24xx_i2c_type enum with unsigned int that holds
>    bitmask with revision-specific quirks
> 
> Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Okay, so this driver needs to the 'data' field from either
platform_device_id or of_device_id and implements a function for that,
namely s3c24xx_get_device_quirks(). Grant, Rob: I'd think there might be
more drivers in need of that, so maybe it makes sense to have a generic
of-helper function?

> ---
>  drivers/i2c/busses/i2c-s3c2410.c |   47 ++++++++++++++++++-------------------
>  1 files changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 85e3664..f7b6a14 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -44,8 +44,14 @@
>  #include <plat/regs-iic.h>
>  #include <plat/iic.h>
>  
> -/* i2c controller state */
> +#ifdef CONFIG_OF
> +static const struct of_device_id s3c24xx_i2c_match[];
> +#endif

Uh, forward declaration with #ifdef. I'd think we should get this simply
to the front.

>  dd
> +/* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
> +#define QUIRK_S3C2440		(1 << 0)

Minor: Is it really a quirk being s3c2440? Maybe FLAG_*, dunno.

> +
> +/* i2c controller state */
>  enum s3c24xx_i2c_state {
>  	STATE_IDLE,
>  	STATE_START,
> @@ -54,14 +60,10 @@ enum s3c24xx_i2c_state {
>  	STATE_STOP
>  };
>  
> -enum s3c24xx_i2c_type {
> -	TYPE_S3C2410,
> -	TYPE_S3C2440,
> -};
> -
>  struct s3c24xx_i2c {
>  	spinlock_t		lock;
>  	wait_queue_head_t	wait;
> +	unsigned int            quirks;
>  	unsigned int		suspended:1;
>  
>  	struct i2c_msg		*msg;
> @@ -88,26 +90,22 @@ struct s3c24xx_i2c {
>  #endif
>  };
>  
> -/* default platform data removed, dev should always carry data. */
> -
> -/* s3c24xx_i2c_is2440()
> +/* s3c24xx_get_device_quirks
>   *
> - * return true is this is an s3c2440
> + * Get controller type either from device tree or platform device variant.
>  */
>  
> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
> +static inline unsigned int s3c24xx_get_device_quirks(struct platform_device *pdev)
>  {
> -	struct platform_device *pdev = to_platform_device(i2c->dev);
> -	enum s3c24xx_i2c_type type;
> -
>  #ifdef CONFIG_OF
> -	if (i2c->dev->of_node)
> -		return of_device_is_compatible(i2c->dev->of_node,
> -				"samsung,s3c2440-i2c");
> +        if (pdev->dev.of_node) {
> +		const struct of_device_id *match;
> +		match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);

Minor: I think it is more readable to drop the [0]

> +		return (unsigned int)match->data;
> +        }
>  #endif
>  
> -	type = platform_get_device_id(pdev)->driver_data;
> -	return type == TYPE_S3C2440;
> +	return platform_get_device_id(pdev)->driver_data;
>  }
>  
>  /* s3c24xx_i2c_master_complete
> @@ -676,7 +674,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c, unsigned int *got)
>  
>  	writel(iiccon, i2c->regs + S3C2410_IICCON);
>  
> -	if (s3c24xx_i2c_is2440(i2c)) {
> +	if (i2c->quirks & QUIRK_S3C2440) {
>  		unsigned long sda_delay;
>  
>  		if (pdata->sda_delay) {
> @@ -906,6 +904,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>  		goto err_noclk;
>  	}
>  
> +	i2c->quirks = s3c24xx_get_device_quirks(pdev);
>  	if (pdata)
>  		memcpy(i2c->pdata, pdata, sizeof(*pdata));
>  	else
> @@ -1113,18 +1112,18 @@ static const struct dev_pm_ops s3c24xx_i2c_dev_pm_ops = {
>  static struct platform_device_id s3c24xx_driver_ids[] = {
>  	{
>  		.name		= "s3c2410-i2c",
> -		.driver_data	= TYPE_S3C2410,
> +		.driver_data	= 0,
>  	}, {
>  		.name		= "s3c2440-i2c",
> -		.driver_data	= TYPE_S3C2440,
> +		.driver_data	= QUIRK_S3C2440,
>  	}, { },
>  };
>  MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id s3c24xx_i2c_match[] = {
> -	{ .compatible = "samsung,s3c2410-i2c" },
> -	{ .compatible = "samsung,s3c2440-i2c" },
> +	{ .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
> +	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },

Hmm, the need to sepcify the quirks twice may lead to only one instance
being updated in future patches, but I don't see a way around that,
currently :(

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
> -- 
> 1.7.9
> 

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 5069 bytes --]

Hi,

On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote:
> Reorganize driver a bit to better handle device tree-based systems:
> 
>  - move machine type to driver's private structure instead of
>    quering platform device variants in runtime
> 
>  - replace s3c24xx_i2c_type enum with unsigned int that holds
>    bitmask with revision-specific quirks
> 
> Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>

Okay, so this driver needs to the 'data' field from either
platform_device_id or of_device_id and implements a function for that,
namely s3c24xx_get_device_quirks(). Grant, Rob: I'd think there might be
more drivers in need of that, so maybe it makes sense to have a generic
of-helper function?

> ---
>  drivers/i2c/busses/i2c-s3c2410.c |   47 ++++++++++++++++++-------------------
>  1 files changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 85e3664..f7b6a14 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -44,8 +44,14 @@
>  #include <plat/regs-iic.h>
>  #include <plat/iic.h>
>  
> -/* i2c controller state */
> +#ifdef CONFIG_OF
> +static const struct of_device_id s3c24xx_i2c_match[];
> +#endif

Uh, forward declaration with #ifdef. I'd think we should get this simply
to the front.

>  dd
> +/* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
> +#define QUIRK_S3C2440		(1 << 0)

Minor: Is it really a quirk being s3c2440? Maybe FLAG_*, dunno.

> +
> +/* i2c controller state */
>  enum s3c24xx_i2c_state {
>  	STATE_IDLE,
>  	STATE_START,
> @@ -54,14 +60,10 @@ enum s3c24xx_i2c_state {
>  	STATE_STOP
>  };
>  
> -enum s3c24xx_i2c_type {
> -	TYPE_S3C2410,
> -	TYPE_S3C2440,
> -};
> -
>  struct s3c24xx_i2c {
>  	spinlock_t		lock;
>  	wait_queue_head_t	wait;
> +	unsigned int            quirks;
>  	unsigned int		suspended:1;
>  
>  	struct i2c_msg		*msg;
> @@ -88,26 +90,22 @@ struct s3c24xx_i2c {
>  #endif
>  };
>  
> -/* default platform data removed, dev should always carry data. */
> -
> -/* s3c24xx_i2c_is2440()
> +/* s3c24xx_get_device_quirks
>   *
> - * return true is this is an s3c2440
> + * Get controller type either from device tree or platform device variant.
>  */
>  
> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
> +static inline unsigned int s3c24xx_get_device_quirks(struct platform_device *pdev)
>  {
> -	struct platform_device *pdev = to_platform_device(i2c->dev);
> -	enum s3c24xx_i2c_type type;
> -
>  #ifdef CONFIG_OF
> -	if (i2c->dev->of_node)
> -		return of_device_is_compatible(i2c->dev->of_node,
> -				"samsung,s3c2440-i2c");
> +        if (pdev->dev.of_node) {
> +		const struct of_device_id *match;
> +		match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);

Minor: I think it is more readable to drop the [0]

> +		return (unsigned int)match->data;
> +        }
>  #endif
>  
> -	type = platform_get_device_id(pdev)->driver_data;
> -	return type == TYPE_S3C2440;
> +	return platform_get_device_id(pdev)->driver_data;
>  }
>  
>  /* s3c24xx_i2c_master_complete
> @@ -676,7 +674,7 @@ static int s3c24xx_i2c_clockrate(struct s3c24xx_i2c *i2c, unsigned int *got)
>  
>  	writel(iiccon, i2c->regs + S3C2410_IICCON);
>  
> -	if (s3c24xx_i2c_is2440(i2c)) {
> +	if (i2c->quirks & QUIRK_S3C2440) {
>  		unsigned long sda_delay;
>  
>  		if (pdata->sda_delay) {
> @@ -906,6 +904,7 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>  		goto err_noclk;
>  	}
>  
> +	i2c->quirks = s3c24xx_get_device_quirks(pdev);
>  	if (pdata)
>  		memcpy(i2c->pdata, pdata, sizeof(*pdata));
>  	else
> @@ -1113,18 +1112,18 @@ static const struct dev_pm_ops s3c24xx_i2c_dev_pm_ops = {
>  static struct platform_device_id s3c24xx_driver_ids[] = {
>  	{
>  		.name		= "s3c2410-i2c",
> -		.driver_data	= TYPE_S3C2410,
> +		.driver_data	= 0,
>  	}, {
>  		.name		= "s3c2440-i2c",
> -		.driver_data	= TYPE_S3C2440,
> +		.driver_data	= QUIRK_S3C2440,
>  	}, { },
>  };
>  MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
>  
>  #ifdef CONFIG_OF
>  static const struct of_device_id s3c24xx_i2c_match[] = {
> -	{ .compatible = "samsung,s3c2410-i2c" },
> -	{ .compatible = "samsung,s3c2440-i2c" },
> +	{ .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
> +	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },

Hmm, the need to sepcify the quirks twice may lead to only one instance
being updated in future patches, but I don't see a way around that,
currently :(

>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
> -- 
> 1.7.9
> 

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[-- Attachment #2: Type: text/plain, Size: 192 bytes --]

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss

Re: [PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 5848 bytes --]

Hi,

On Wed, Mar 21, 2012 at 08:11:53PM +0100, Karol Lewandowski wrote:
> This patch adds support for s3c2440 I2C bus controller dedicated HDMIPHY device on
> Exynos4 platform. Some quirks are introduced due to differences between HDMIPHY
> and other I2C controllers on Exynos4.  These differences are:
> - no GPIOs, HDMIPHY is inside the SoC and the controller is connected
>   internally
> - due to unknown reason (probably HW bug in HDMIPHY and/or the controller) a
>   transfer fails to finish. The controller hangs after sending the last byte,
>   the workaround for this bug is resetting the controller after each transfer
> 
> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
> Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 +++++-
>  drivers/i2c/busses/i2c-s3c2410.c                   |   35 ++++++++++++++++++++
>  2 files changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> index 38832c7..c6670f9 100644
> --- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> +++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> @@ -10,14 +10,21 @@ Required properties:
>      region.
>    - interrupts: interrupt number to the cpu.
>    - samsung,i2c-sda-delay: Delay (in ns) applied to data line (SDA) edges.
> -  - gpios: The order of the gpios should be the following: <SDA, SCL>.
> -    The gpio specifier depends on the gpio controller.
>  
>  Optional properties:
> +  - gpios: The order of the gpios should be the following: <SDA, SCL>.
> +    The gpio specifier depends on the gpio controller. Required in all cases
> +    except when "samsung,i2c-no-gpio" is also specified.
> +  - samsung,i2c-no-gpio: input/output lines of the controller are
> +    permanently wired to the respective client, there are no gpio
> +    lines that need to be configured to enable this controller

Can't we just skip this property...


>    - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
>      specified, default value is 0.
>    - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
>      specified, the default value in Hz is 100000.
> +  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
> +    Exynos4 platform - reduce timeout and reset controller after each
> +    transfer

... and this one, if we declare a new compatible-entry for exynos4?

>  
>  Example:
>  
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index f7b6a14..e50f523 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -50,6 +50,8 @@ static const struct of_device_id s3c24xx_i2c_match[];
>  
>  /* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
>  #define QUIRK_S3C2440		(1 << 0)
> +#define QUIRK_HDMIPHY		(1 << 1)
> +#define QUIRK_NO_GPIO		(1 << 2)
>  
>  /* i2c controller state */
>  enum s3c24xx_i2c_state {
> @@ -469,6 +471,13 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
>  	unsigned long iicstat;
>  	int timeout = 400;
>  
> +	/* the timeout for HDMIPHY is reduced to 10 ms because
> +	 * the hangup is expected to happen, so waiting 400 ms
> +	 * causes only unnecessary system hangup
> +	 */
> +	if (i2c->quirks & QUIRK_HDMIPHY)
> +		timeout = 10;
> +
>  	while (timeout-- > 0) {
>  		iicstat = readl(i2c->regs + S3C2410_IICSTAT);
>  
> @@ -478,6 +487,15 @@ static int s3c24xx_i2c_set_master(struct s3c24xx_i2c *i2c)
>  		msleep(1);
>  	}
>  
> +	/* hang-up of bus dedicated for HDMIPHY occurred, resetting */
> +	if (i2c->quirks & QUIRK_HDMIPHY) {
> +		writel(0, i2c->regs + S3C2410_IICCON);
> +		writel(0, i2c->regs + S3C2410_IICSTAT);
> +		writel(0, i2c->regs + S3C2410_IICDS);
> +
> +		return 0;
> +	}
> +
>  	return -ETIMEDOUT;
>  }
>  
> @@ -759,6 +777,9 @@ static int s3c24xx_i2c_parse_dt_gpio(struct s3c24xx_i2c *i2c)
>  {
>  	int idx, gpio, ret;
>  
> +	if (i2c->quirks & QUIRK_NO_GPIO)
> +		return 0;
> +
>  	for (idx = 0; idx < 2; idx++) {
>  		gpio = of_get_gpio(i2c->dev->of_node, idx);
>  		if (!gpio_is_valid(gpio)) {
> @@ -783,6 +804,10 @@ free_gpio:
>  static void s3c24xx_i2c_dt_gpio_free(struct s3c24xx_i2c *i2c)
>  {
>  	unsigned int idx;
> +
> +	if (i2c->quirks & QUIRK_NO_GPIO)
> +		return;
> +
>  	for (idx = 0; idx < 2; idx++)
>  		gpio_free(i2c->gpios[idx]);
>  }
> @@ -859,6 +884,13 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
>  		return;
>  
>  	pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
> +
> +	if (of_get_property(np, "samsung,i2c-quirk-hdmiphy", NULL))
> +		i2c->quirks |= QUIRK_HDMIPHY;
> +
> +	if (of_get_property(np, "samsung,i2c-no-gpio", NULL))
> +		i2c->quirks |= QUIRK_NO_GPIO;
> +
>  	of_property_read_u32(np, "samsung,i2c-sda-delay", &pdata->sda_delay);
>  	of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);
>  	of_property_read_u32(np, "samsung,i2c-max-bus-freq",
> @@ -1116,6 +1148,9 @@ static struct platform_device_id s3c24xx_driver_ids[] = {
>  	}, {
>  		.name		= "s3c2440-i2c",
>  		.driver_data	= QUIRK_S3C2440,
> +	}, {
> +		.name		= "s3c2440-hdmiphy-i2c",
> +		.driver_data	= QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO,
>  	}, { },
>  };
>  MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
> -- 
> 1.7.9
> 

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] i2c-s3c2410: Drop unused define

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 534 bytes --]

On Wed, Mar 21, 2012 at 08:11:51PM +0100, Karol Lewandowski wrote:
> Use standard of_match_ptr() to avoid defining variable unused
> in non device tree builds.
> 
> Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

I applied this one already.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 1/3] i2c-s3c2410: Drop unused define

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Wed, Mar 21, 2012 at 08:11:51PM +0100, Karol Lewandowski wrote:
> Use standard of_match_ptr() to avoid defining variable unused
> in non device tree builds.
> 
> Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> Acked-by: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>

I applied this one already.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling

[Karol Lewandowski]

On 17.04.2012 19:31, Wolfram Sang wrote:

> Hi,


Hi Wolfram!

> 
> On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote:
>> Reorganize driver a bit to better handle device tree-based systems:
>>
>>  - move machine type to driver's private structure instead of
>>    quering platform device variants in runtime
>>
>>  - replace s3c24xx_i2c_type enum with unsigned int that holds
>>    bitmask with revision-specific quirks
>>
>> Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> Okay, so this driver needs to the 'data' field from either
> platform_device_id or of_device_id and implements a function for that,
> namely s3c24xx_get_device_quirks(). Grant, Rob: I'd think there might be
> more drivers in need of that, so maybe it makes sense to have a generic
> of-helper function?
> 
>> ---
>>  drivers/i2c/busses/i2c-s3c2410.c |   47 ++++++++++++++++++-------------------
>>  1 files changed, 23 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>> index 85e3664..f7b6a14 100644
>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
>> @@ -44,8 +44,14 @@
>>  #include <plat/regs-iic.h>
>>  #include <plat/iic.h>
>>  
>> -/* i2c controller state */
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id s3c24xx_i2c_match[];
>> +#endif
> 
> Uh, forward declaration with #ifdef. I'd think we should get this simply
> to the front.


Ok, as I think it's better to have dt and non-dt definitions together
I'll move both of_device_id and platform_device_id to the top.

>> +/* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
>> +#define QUIRK_S3C2440		(1 << 0)
> 
> Minor: Is it really a quirk being s3c2440? Maybe FLAG_*, dunno.


In first version[1] of this patch I've used TYPEs for device types
and FLAGS for quirks. However, I've squashed these into "quirks" after
discussion with Mark[2].

[1] http://permalink.gmane.org/gmane.linux.kernel/1266759
[2] http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/10255


>> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
>> +static inline unsigned int s3c24xx_get_device_quirks(struct platform_device *pdev)
>>  {
>> -	struct platform_device *pdev = to_platform_device(i2c->dev);
>> -	enum s3c24xx_i2c_type type;
>> -
>>  #ifdef CONFIG_OF
>> -	if (i2c->dev->of_node)
>> -		return of_device_is_compatible(i2c->dev->of_node,
>> -				"samsung,s3c2440-i2c");
>> +        if (pdev->dev.of_node) {
>> +		const struct of_device_id *match;
>> +		match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);
> 
> Minor: I think it is more readable to drop the [0]


I prefer explicit version, but I'll drop [] as both you and Thomas
found implicit version more readable.

[ I'll also drop above CONFIG_OF ifdef, as of_match_node() seem
  to be always defined since v3.2-rc1. ]

Thanks for review!  I'll resubmit updated version shortly.

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling

[Karol Lewandowski]

On 17.04.2012 19:31, Wolfram Sang wrote:

> Hi,


Hi Wolfram!

> 
> On Wed, Mar 21, 2012 at 08:11:52PM +0100, Karol Lewandowski wrote:
>> Reorganize driver a bit to better handle device tree-based systems:
>>
>>  - move machine type to driver's private structure instead of
>>    quering platform device variants in runtime
>>
>>  - replace s3c24xx_i2c_type enum with unsigned int that holds
>>    bitmask with revision-specific quirks
>>
>> Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
>> Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> 
> Okay, so this driver needs to the 'data' field from either
> platform_device_id or of_device_id and implements a function for that,
> namely s3c24xx_get_device_quirks(). Grant, Rob: I'd think there might be
> more drivers in need of that, so maybe it makes sense to have a generic
> of-helper function?
> 
>> ---
>>  drivers/i2c/busses/i2c-s3c2410.c |   47 ++++++++++++++++++-------------------
>>  1 files changed, 23 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>> index 85e3664..f7b6a14 100644
>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
>> @@ -44,8 +44,14 @@
>>  #include <plat/regs-iic.h>
>>  #include <plat/iic.h>
>>  
>> -/* i2c controller state */
>> +#ifdef CONFIG_OF
>> +static const struct of_device_id s3c24xx_i2c_match[];
>> +#endif
> 
> Uh, forward declaration with #ifdef. I'd think we should get this simply
> to the front.


Ok, as I think it's better to have dt and non-dt definitions together
I'll move both of_device_id and platform_device_id to the top.

>> +/* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
>> +#define QUIRK_S3C2440		(1 << 0)
> 
> Minor: Is it really a quirk being s3c2440? Maybe FLAG_*, dunno.


In first version[1] of this patch I've used TYPEs for device types
and FLAGS for quirks. However, I've squashed these into "quirks" after
discussion with Mark[2].

[1] http://permalink.gmane.org/gmane.linux.kernel/1266759
[2] http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/10255


>> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
>> +static inline unsigned int s3c24xx_get_device_quirks(struct platform_device *pdev)
>>  {
>> -	struct platform_device *pdev = to_platform_device(i2c->dev);
>> -	enum s3c24xx_i2c_type type;
>> -
>>  #ifdef CONFIG_OF
>> -	if (i2c->dev->of_node)
>> -		return of_device_is_compatible(i2c->dev->of_node,
>> -				"samsung,s3c2440-i2c");
>> +        if (pdev->dev.of_node) {
>> +		const struct of_device_id *match;
>> +		match = of_match_node(&s3c24xx_i2c_match[0], pdev->dev.of_node);
> 
> Minor: I think it is more readable to drop the [0]


I prefer explicit version, but I'll drop [] as both you and Thomas
found implicit version more readable.

[ I'll also drop above CONFIG_OF ifdef, as of_match_node() seem
  to be always defined since v3.2-rc1. ]

Thanks for review!  I'll resubmit updated version shortly.

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

RE: [PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Marek Szyprowski]

Hi Wolfram,

On Tuesday, April 17, 2012 7:40 PM Wolfram Sang wrote:

> On Wed, Mar 21, 2012 at 08:11:53PM +0100, Karol Lewandowski wrote:
> > This patch adds support for s3c2440 I2C bus controller dedicated HDMIPHY device on
> > Exynos4 platform. Some quirks are introduced due to differences between HDMIPHY
> > and other I2C controllers on Exynos4.  These differences are:
> > - no GPIOs, HDMIPHY is inside the SoC and the controller is connected
> >   internally
> > - due to unknown reason (probably HW bug in HDMIPHY and/or the controller) a
> >   transfer fails to finish. The controller hangs after sending the last byte,
> >   the workaround for this bug is resetting the controller after each transfer
> >
> > Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> > Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
> > Tested-by: Tomasz Stanislawski <t.stanislaws@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > ---
> >  .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 +++++-
> >  drivers/i2c/busses/i2c-s3c2410.c                   |   35 ++++++++++++++++++++
> >  2 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> > index 38832c7..c6670f9 100644
> > --- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> > +++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> > @@ -10,14 +10,21 @@ Required properties:
> >      region.
> >    - interrupts: interrupt number to the cpu.
> >    - samsung,i2c-sda-delay: Delay (in ns) applied to data line (SDA) edges.
> > -  - gpios: The order of the gpios should be the following: <SDA, SCL>.
> > -    The gpio specifier depends on the gpio controller.
> >
> >  Optional properties:
> > +  - gpios: The order of the gpios should be the following: <SDA, SCL>.
> > +    The gpio specifier depends on the gpio controller. Required in all cases
> > +    except when "samsung,i2c-no-gpio" is also specified.
> > +  - samsung,i2c-no-gpio: input/output lines of the controller are
> > +    permanently wired to the respective client, there are no gpio
> > +    lines that need to be configured to enable this controller
> 
> Can't we just skip this property...

All standard s3c-24x0 i2c controllers require gpio lines for proper operation,
so lack of the gpios property should be considered as an error. However there
is a special case of internal, embedded i2c controller which has no such gpio 
lines at all.

> >    - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
> >      specified, default value is 0.
> >    - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
> >      specified, the default value in Hz is 100000.
> > +  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
> > +    Exynos4 platform - reduce timeout and reset controller after each
> > +    transfer
> 
> ... and this one, if we declare a new compatible-entry for exynos4?

It is not strictly related to Exynos4 SoCs. Exynos4 SoC has 8 standard s3c2440 style
i2c controllers and one additional, internal controller for HDMIPHY, which needs 
some workarounds in the driver. Maybe the quirk should be named 'broken timeout 
detection'

(snipped)

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

RE: [PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Marek Szyprowski]

Hi Wolfram,

On Tuesday, April 17, 2012 7:40 PM Wolfram Sang wrote:

> On Wed, Mar 21, 2012 at 08:11:53PM +0100, Karol Lewandowski wrote:
> > This patch adds support for s3c2440 I2C bus controller dedicated HDMIPHY device on
> > Exynos4 platform. Some quirks are introduced due to differences between HDMIPHY
> > and other I2C controllers on Exynos4.  These differences are:
> > - no GPIOs, HDMIPHY is inside the SoC and the controller is connected
> >   internally
> > - due to unknown reason (probably HW bug in HDMIPHY and/or the controller) a
> >   transfer fails to finish. The controller hangs after sending the last byte,
> >   the workaround for this bug is resetting the controller after each transfer
> >
> > Signed-off-by: Tomasz Stanislawski <t.stanislaws-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > Signed-off-by: Karol Lewandowski <k.lewandowsk-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > Tested-by: Tomasz Stanislawski <t.stanislaws-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > Signed-off-by: Kyungmin Park <kyungmin.park-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  .../devicetree/bindings/i2c/samsung-i2c.txt        |   11 +++++-
> >  drivers/i2c/busses/i2c-s3c2410.c                   |   35 ++++++++++++++++++++
> >  2 files changed, 44 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> > index 38832c7..c6670f9 100644
> > --- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> > +++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
> > @@ -10,14 +10,21 @@ Required properties:
> >      region.
> >    - interrupts: interrupt number to the cpu.
> >    - samsung,i2c-sda-delay: Delay (in ns) applied to data line (SDA) edges.
> > -  - gpios: The order of the gpios should be the following: <SDA, SCL>.
> > -    The gpio specifier depends on the gpio controller.
> >
> >  Optional properties:
> > +  - gpios: The order of the gpios should be the following: <SDA, SCL>.
> > +    The gpio specifier depends on the gpio controller. Required in all cases
> > +    except when "samsung,i2c-no-gpio" is also specified.
> > +  - samsung,i2c-no-gpio: input/output lines of the controller are
> > +    permanently wired to the respective client, there are no gpio
> > +    lines that need to be configured to enable this controller
> 
> Can't we just skip this property...

All standard s3c-24x0 i2c controllers require gpio lines for proper operation,
so lack of the gpios property should be considered as an error. However there
is a special case of internal, embedded i2c controller which has no such gpio 
lines at all.

> >    - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
> >      specified, default value is 0.
> >    - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
> >      specified, the default value in Hz is 100000.
> > +  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
> > +    Exynos4 platform - reduce timeout and reset controller after each
> > +    transfer
> 
> ... and this one, if we declare a new compatible-entry for exynos4?

It is not strictly related to Exynos4 SoCs. Exynos4 SoC has 8 standard s3c2440 style
i2c controllers and one additional, internal controller for HDMIPHY, which needs 
some workarounds in the driver. Maybe the quirk should be named 'broken timeout 
detection'

(snipped)

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center

Re: [PATCH 2/3] i2c-s3c2410: Rework device type handling

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2237 bytes --]

Hi,

> >> Reorganize driver a bit to better handle device tree-based systems:
> >>
> >>  - move machine type to driver's private structure instead of
> >>    quering platform device variants in runtime
> >>
> >>  - replace s3c24xx_i2c_type enum with unsigned int that holds
> >>    bitmask with revision-specific quirks
> >>
> >> Signed-off-by: Karol Lewandowski <k.lewandowsk@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > 
> > Okay, so this driver needs to the 'data' field from either
> > platform_device_id or of_device_id and implements a function for that,
> > namely s3c24xx_get_device_quirks(). Grant, Rob: I'd think there might be
> > more drivers in need of that, so maybe it makes sense to have a generic
> > of-helper function?

Please wait one or two days before resending. Maybe Grant or Rob find
some time to answer this question. (Yeah, we can fix it later if such a
generic function is introduced somewhen).

> >> -/* i2c controller state */
> >> +#ifdef CONFIG_OF
> >> +static const struct of_device_id s3c24xx_i2c_match[];
> >> +#endif
> > 
> > Uh, forward declaration with #ifdef. I'd think we should get this simply
> > to the front.
> 
> 
> Ok, as I think it's better to have dt and non-dt definitions together
> I'll move both of_device_id and platform_device_id to the top.

Agreed.

> 
> >> +/* Treat S3C2410 as baseline hardware, anything else is supported via quirks */
> >> +#define QUIRK_S3C2440		(1 << 0)
> > 
> > Minor: Is it really a quirk being s3c2440? Maybe FLAG_*, dunno.
> 
> 
> In first version[1] of this patch I've used TYPEs for device types
> and FLAGS for quirks. However, I've squashed these into "quirks" after
> discussion with Mark[2].
> 
> [1] http://permalink.gmane.org/gmane.linux.kernel/1266759
> [2] http://permalink.gmane.org/gmane.linux.kernel.samsung-soc/10255

It's minor, keep the QUIRKs.

> [ I'll also drop above CONFIG_OF ifdef, as of_match_node() seem
>   to be always defined since v3.2-rc1. ]

Great.

Thanks,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1995 bytes --]


> > >  Optional properties:
> > > +  - gpios: The order of the gpios should be the following: <SDA, SCL>.
> > > +    The gpio specifier depends on the gpio controller. Required in all cases
> > > +    except when "samsung,i2c-no-gpio" is also specified.
> > > +  - samsung,i2c-no-gpio: input/output lines of the controller are
> > > +    permanently wired to the respective client, there are no gpio
> > > +    lines that need to be configured to enable this controller
> > 
> > Can't we just skip this property...
> 
> All standard s3c-24x0 i2c controllers require gpio lines for proper operation,
> so lack of the gpios property should be considered as an error. However there
> is a special case of internal, embedded i2c controller which has no such gpio 
> lines at all.
> 
> > >    - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
> > >      specified, default value is 0.
> > >    - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
> > >      specified, the default value in Hz is 100000.
> > > +  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
> > > +    Exynos4 platform - reduce timeout and reset controller after each
> > > +    transfer
> > 
> > ... and this one, if we declare a new compatible-entry for exynos4?
> 
> It is not strictly related to Exynos4 SoCs. Exynos4 SoC has 8 standard s3c2440 style
> i2c controllers and one additional, internal controller for HDMIPHY, which needs 
> some workarounds in the driver. Maybe the quirk should be named 'broken timeout 
> detection'

I was wondering since you do what I suggested for platform devices already:

+               .name           = "s3c2440-hdmiphy-i2c",
+               .driver_data    = QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO,

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Karol Lewandowski]

On 18.04.2012 15:46, Wolfram Sang wrote:

> 
>>>>  Optional properties:
>>>> +  - gpios: The order of the gpios should be the following: <SDA, SCL>.
>>>> +    The gpio specifier depends on the gpio controller. Required in all cases
>>>> +    except when "samsung,i2c-no-gpio" is also specified.
>>>> +  - samsung,i2c-no-gpio: input/output lines of the controller are
>>>> +    permanently wired to the respective client, there are no gpio
>>>> +    lines that need to be configured to enable this controller
>>>
>>> Can't we just skip this property...
>>
>> All standard s3c-24x0 i2c controllers require gpio lines for proper operation,
>> so lack of the gpios property should be considered as an error. However there
>> is a special case of internal, embedded i2c controller which has no such gpio 
>> lines at all.
>>
>>>>    - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
>>>>      specified, default value is 0.
>>>>    - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
>>>>      specified, the default value in Hz is 100000.
>>>> +  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
>>>> +    Exynos4 platform - reduce timeout and reset controller after each
>>>> +    transfer
>>>
>>> ... and this one, if we declare a new compatible-entry for exynos4?
>>
>> It is not strictly related to Exynos4 SoCs. Exynos4 SoC has 8 standard s3c2440 style
>> i2c controllers and one additional, internal controller for HDMIPHY, which needs 
>> some workarounds in the driver. Maybe the quirk should be named 'broken timeout 
>> detection'
> 
> I was wondering since you do what I suggested for platform devices already:
> 
> +               .name           = "s3c2440-hdmiphy-i2c",
> +               .driver_data    = QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO,


Here I've done it this way for compatibility with legacy driver and
board(s).

Device tree is new interface, and I thought that it would be better
to describe things explicitly and separately from device type.

Right now these properties are used only on S3C2440, but I don't
consider it highly unlikely to see these on S3C**** in future.

Tomasz had similar doubts when I've posted patch that checked these
quirks only for S3C2440:

  http://permalink.gmane.org/gmane.linux.drivers.i2c/10305

Thus, I've chosen properties and not separate type.

It's easy to introduce compat string (see below), but given above
I'm afraid that we might end up adding -hdmiphy- variant for every
new version of i2c controller.


  E.g.

diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
index c6670f9..b1d106e 100644
--- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
@@ -6,6 +6,8 @@ Required properties:
   - compatible: value should be either of the following.
       (a) "samsung, s3c2410-i2c", for i2c compatible with s3c2410 i2c.
       (b) "samsung, s3c2440-i2c", for i2c compatible with s3c2440 i2c.
+      (c) "samsung, s3c2440-hdmiphy-i2c", for s3c2440-like i2c used
+          inside HDMIPHY block found on several samsung SoCs
   - reg: physical base address of the controller and length of memory mapped
     region.
   - interrupts: interrupt number to the cpu.
@@ -13,18 +15,13 @@ Required properties:
 
 Optional properties:
   - gpios: The order of the gpios should be the following: <SDA, SCL>.
-    The gpio specifier depends on the gpio controller. Required in all cases
-    except when "samsung,i2c-no-gpio" is also specified.
-  - samsung,i2c-no-gpio: input/output lines of the controller are
-    permanently wired to the respective client, there are no gpio
-    lines that need to be configured to enable this controller
+    The gpio specifier depends on the gpio controller. Required in all
+    cases except for "samsung,i2c-hdmiphy-i2c" whose input/output
+    lines are permanently wired to the respective client
   - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
     specified, default value is 0.
   - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
     specified, the default value in Hz is 100000.
-  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
-    Exynos4 platform - reduce timeout and reset controller after each
-    transfer
 
 Example:
 
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 71438eb..bc82045 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -106,6 +106,8 @@ MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
 static const struct of_device_id s3c24xx_i2c_match[] = {
        { .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
        { .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },
+       { .compatible = "samsung,s3c2440-hdmiphy-i2c",
+         .data = (void *)(QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO) },
        {},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
@@ -902,12 +904,6 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
 
        pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
 
-       if (of_get_property(np, "samsung,i2c-quirk-hdmiphy", NULL))
-               i2c->quirks |= QUIRK_HDMIPHY;
-
-       if (of_get_property(np, "samsung,i2c-no-gpio", NULL))
-               i2c->quirks |= QUIRK_NO_GPIO;
-
        of_property_read_u32(np, "samsung,i2c-sda-delay", &pdata->sda_delay);
        of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);
        of_property_read_u32(np, "samsung,i2c-max-bus-freq",


Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Karol Lewandowski]

On 18.04.2012 15:46, Wolfram Sang wrote:

> 
>>>>  Optional properties:
>>>> +  - gpios: The order of the gpios should be the following: <SDA, SCL>.
>>>> +    The gpio specifier depends on the gpio controller. Required in all cases
>>>> +    except when "samsung,i2c-no-gpio" is also specified.
>>>> +  - samsung,i2c-no-gpio: input/output lines of the controller are
>>>> +    permanently wired to the respective client, there are no gpio
>>>> +    lines that need to be configured to enable this controller
>>>
>>> Can't we just skip this property...
>>
>> All standard s3c-24x0 i2c controllers require gpio lines for proper operation,
>> so lack of the gpios property should be considered as an error. However there
>> is a special case of internal, embedded i2c controller which has no such gpio 
>> lines at all.
>>
>>>>    - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
>>>>      specified, default value is 0.
>>>>    - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
>>>>      specified, the default value in Hz is 100000.
>>>> +  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
>>>> +    Exynos4 platform - reduce timeout and reset controller after each
>>>> +    transfer
>>>
>>> ... and this one, if we declare a new compatible-entry for exynos4?
>>
>> It is not strictly related to Exynos4 SoCs. Exynos4 SoC has 8 standard s3c2440 style
>> i2c controllers and one additional, internal controller for HDMIPHY, which needs 
>> some workarounds in the driver. Maybe the quirk should be named 'broken timeout 
>> detection'
> 
> I was wondering since you do what I suggested for platform devices already:
> 
> +               .name           = "s3c2440-hdmiphy-i2c",
> +               .driver_data    = QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO,


Here I've done it this way for compatibility with legacy driver and
board(s).

Device tree is new interface, and I thought that it would be better
to describe things explicitly and separately from device type.

Right now these properties are used only on S3C2440, but I don't
consider it highly unlikely to see these on S3C**** in future.

Tomasz had similar doubts when I've posted patch that checked these
quirks only for S3C2440:

  http://permalink.gmane.org/gmane.linux.drivers.i2c/10305

Thus, I've chosen properties and not separate type.

It's easy to introduce compat string (see below), but given above
I'm afraid that we might end up adding -hdmiphy- variant for every
new version of i2c controller.


  E.g.

diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
index c6670f9..b1d106e 100644
--- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
@@ -6,6 +6,8 @@ Required properties:
   - compatible: value should be either of the following.
       (a) "samsung, s3c2410-i2c", for i2c compatible with s3c2410 i2c.
       (b) "samsung, s3c2440-i2c", for i2c compatible with s3c2440 i2c.
+      (c) "samsung, s3c2440-hdmiphy-i2c", for s3c2440-like i2c used
+          inside HDMIPHY block found on several samsung SoCs
   - reg: physical base address of the controller and length of memory mapped
     region.
   - interrupts: interrupt number to the cpu.
@@ -13,18 +15,13 @@ Required properties:
 
 Optional properties:
   - gpios: The order of the gpios should be the following: <SDA, SCL>.
-    The gpio specifier depends on the gpio controller. Required in all cases
-    except when "samsung,i2c-no-gpio" is also specified.
-  - samsung,i2c-no-gpio: input/output lines of the controller are
-    permanently wired to the respective client, there are no gpio
-    lines that need to be configured to enable this controller
+    The gpio specifier depends on the gpio controller. Required in all
+    cases except for "samsung,i2c-hdmiphy-i2c" whose input/output
+    lines are permanently wired to the respective client
   - samsung,i2c-slave-addr: Slave address in multi-master enviroment. If not
     specified, default value is 0.
   - samsung,i2c-max-bus-freq: Desired frequency in Hz of the bus. If not
     specified, the default value in Hz is 100000.
-  - samsung,i2c-quirk-hdmiphy: Quirk for HDMI PHY block found on
-    Exynos4 platform - reduce timeout and reset controller after each
-    transfer
 
 Example:
 
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 71438eb..bc82045 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -106,6 +106,8 @@ MODULE_DEVICE_TABLE(platform, s3c24xx_driver_ids);
 static const struct of_device_id s3c24xx_i2c_match[] = {
        { .compatible = "samsung,s3c2410-i2c", .data = (void *)0 },
        { .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },
+       { .compatible = "samsung,s3c2440-hdmiphy-i2c",
+         .data = (void *)(QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO) },
        {},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
@@ -902,12 +904,6 @@ s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
 
        pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
 
-       if (of_get_property(np, "samsung,i2c-quirk-hdmiphy", NULL))
-               i2c->quirks |= QUIRK_HDMIPHY;
-
-       if (of_get_property(np, "samsung,i2c-no-gpio", NULL))
-               i2c->quirks |= QUIRK_NO_GPIO;
-
        of_property_read_u32(np, "samsung,i2c-sda-delay", &pdata->sda_delay);
        of_property_read_u32(np, "samsung,i2c-slave-addr", &pdata->slave_addr);
        of_property_read_u32(np, "samsung,i2c-max-bus-freq",


Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2708 bytes --]

Hi Karol,

> >>> ... and this one, if we declare a new compatible-entry for exynos4?
> >>
> >> It is not strictly related to Exynos4 SoCs. Exynos4 SoC has 8 standard s3c2440 style
> >> i2c controllers and one additional, internal controller for HDMIPHY, which needs 
> >> some workarounds in the driver. Maybe the quirk should be named 'broken timeout 
> >> detection'
> > 
> > I was wondering since you do what I suggested for platform devices already:
> > 
> > +               .name           = "s3c2440-hdmiphy-i2c",
> > +               .driver_data    = QUIRK_S3C2440 | QUIRK_HDMIPHY | QUIRK_NO_GPIO,
> 
> 
> Here I've done it this way for compatibility with legacy driver and
> board(s).

Understood.

> Device tree is new interface, and I thought that it would be better
> to describe things explicitly and separately from device type.
> 
> Right now these properties are used only on S3C2440, but I don't
> consider it highly unlikely to see these on S3C**** in future.

My experience also says that it easily can get even worse :(

> Tomasz had similar doubts when I've posted patch that checked these
> quirks only for S3C2440:
> 
>   http://permalink.gmane.org/gmane.linux.drivers.i2c/10305
> 
> Thus, I've chosen properties and not separate type.

I understand this reasoning. I still differ, though. Think about my
above example about things getting worse. Then, you'd need another
quirk-property for $FLAW. Later, $FLAW is still there, but the timeout
issue was fixed. That would mean, the poor device-tree making person has
to know which quirks to select for this version of the controller. Just
specifying that it is the HDMI-phy and not a regular I2C controller is
much more convenient, and the driver will figure the rest.

> It's easy to introduce compat string (see below), but given above
> I'm afraid that we might end up adding -hdmiphy- variant for every
> new version of i2c controller.

I'd be fine with that, given that the upcoming hdmiphy versions will not
need all the same set of quirk-flags. I think we want that "quirk lookup
table" fixed in the driver and not encoded in the device tree where
people could get it wrong. Also, the quirks are nothing a board maker
can select from; it is implicit as soons as you want the HDMIPHY on that
SoC, thus the compatible-entry should be enough of a description.

I am not the ultimate expert about bindings, though, and am open for
corrections (I feel kinda confident on this issue, though ;))

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/3] i2c-s3c2410: Add HDMIPHY quirk for S3C2440

[Karol Lewandowski]

On 23.04.2012 12:01, Wolfram Sang wrote:

> Hi Karol,
>> Tomasz had similar doubts when I've posted patch that checked these
>> quirks only for S3C2440:
>>
>>   http://permalink.gmane.org/gmane.linux.drivers.i2c/10305
>>
>> Thus, I've chosen properties and not separate type.
> 
> I understand this reasoning. I still differ, though. Think about my
> above example about things getting worse. Then, you'd need another
> quirk-property for $FLAW. Later, $FLAW is still there, but the timeout
> issue was fixed. That would mean, the poor device-tree making person has
> to know which quirks to select for this version of the controller. Just
> specifying that it is the HDMI-phy and not a regular I2C controller is
> much more convenient, and the driver will figure the rest.

>

>> It's easy to introduce compat string (see below), but given above
>> I'm afraid that we might end up adding -hdmiphy- variant for every
>> new version of i2c controller.
> 
> I'd be fine with that, given that the upcoming hdmiphy versions will not
> need all the same set of quirk-flags. I think we want that "quirk lookup
> table" fixed in the driver and not encoded in the device tree where
> people could get it wrong. Also, the quirks are nothing a board maker
> can select from; it is implicit as soons as you want the HDMIPHY on that
> SoC, thus the compatible-entry should be enough of a description.


Fair point, from integrator/board maker POV this makes much more sense.

> I am not the ultimate expert about bindings, though, and am open for
> corrections (I feel kinda confident on this issue, though ;))


I'm quite happy with doing it the way you just described.

I'll resend whole patchset in a minute.

Thanks!
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform