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

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

[Karol Lewandowski]

Changes since v3:
 - Replace DT-visible quirks with new controller type
   [Suggested by Wolfram Sang]
 - Dropped already merged of_match_ptr()-patch

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 (2):
  i2c-s3c2410: Rework device type handling
  i2c-s3c2410: Add HDMIPHY quirk for S3C2440

 .../devicetree/bindings/i2c/samsung-i2c.txt        |    8 +-
 drivers/i2c/busses/i2c-s3c2410.c                   |  105 ++++++++++++--------
 2 files changed, 70 insertions(+), 43 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] 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 |   75 +++++++++++++++++---------------------
 1 file changed, 34 insertions(+), 41 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 85e3664..23736ff 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -44,8 +44,10 @@
 #include <plat/regs-iic.h>
 #include <plat/iic.h>
 
-/* i2c controller state */
+/* 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 +56,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 +86,40 @@ struct s3c24xx_i2c {
 #endif
 };
 
-/* default platform data removed, dev should always carry data. */
+static struct platform_device_id s3c24xx_driver_ids[] = {
+	{
+		.name		= "s3c2410-i2c",
+		.driver_data	= 0,
+	}, {
+		.name		= "s3c2440-i2c",
+		.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", .data = (void *)0 },
+	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
+#endif
 
-/* 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");
-#endif
+	if (pdev->dev.of_node) {
+		const struct of_device_id *match;
+		match = of_match_node(&s3c24xx_i2c_match, pdev->dev.of_node);
+		return (unsigned int)match->data;
+	}
 
-	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 +688,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 +918,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
@@ -1110,26 +1123,6 @@ static const struct dev_pm_ops s3c24xx_i2c_dev_pm_ops = {
 
 /* device driver for platform bus bits */
 
-static struct platform_device_id s3c24xx_driver_ids[] = {
-	{
-		.name		= "s3c2410-i2c",
-		.driver_data	= TYPE_S3C2410,
-	}, {
-		.name		= "s3c2440-i2c",
-		.driver_data	= TYPE_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" },
-	{},
-};
-MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
-#endif
-
 static struct platform_driver s3c24xx_i2c_driver = {
 	.probe		= s3c24xx_i2c_probe,
 	.remove		= s3c24xx_i2c_remove,
-- 
1.7.9.5

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

[Wolfram Sang]

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

On Mon, Apr 23, 2012 at 06:24:00PM +0200, 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>
> ---
>  drivers/i2c/busses/i2c-s3c2410.c |   75 +++++++++++++++++---------------------
>  1 file changed, 34 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 85e3664..23736ff 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -44,8 +44,10 @@
>  #include <plat/regs-iic.h>
>  #include <plat/iic.h>
>  
> -/* i2c controller state */
> +/* 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 +56,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 +86,40 @@ struct s3c24xx_i2c {
>  #endif
>  };
>  
> -/* default platform data removed, dev should always carry data. */
> +static struct platform_device_id s3c24xx_driver_ids[] = {
> +	{
> +		.name		= "s3c2410-i2c",
> +		.driver_data	= 0,
> +	}, {
> +		.name		= "s3c2440-i2c",
> +		.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", .data = (void *)0 },
> +	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)QUIRK_S3C2440 },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
> +#endif
>  
> -/* 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");
> -#endif
> +	if (pdev->dev.of_node) {
> +		const struct of_device_id *match;
> +		match = of_match_node(&s3c24xx_i2c_match, pdev->dev.of_node);

I'd appreciate a comment explaining why match can't be NULL here (as to
my understanding, it can't). Or just check for it, but this way it looks
a bit fishy and people (hopefully ;)) will ask about it.

> +		return (unsigned int)match->data;
> +	}
>  
> -	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 +688,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 +918,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
> @@ -1110,26 +1123,6 @@ static const struct dev_pm_ops s3c24xx_i2c_dev_pm_ops = {
>  
>  /* device driver for platform bus bits */
>  
> -static struct platform_device_id s3c24xx_driver_ids[] = {
> -	{
> -		.name		= "s3c2410-i2c",
> -		.driver_data	= TYPE_S3C2410,
> -	}, {
> -		.name		= "s3c2440-i2c",
> -		.driver_data	= TYPE_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" },
> -	{},
> -};
> -MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
> -#endif
> -
>  static struct platform_driver s3c24xx_i2c_driver = {
>  	.probe		= s3c24xx_i2c_probe,
>  	.remove		= s3c24xx_i2c_remove,

Rest is looking good, second patch, too.

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 1/2] i2c-s3c2410: Rework device type handling

[Karol Lewandowski]

On 23.04.2012 20:20, Wolfram Sang wrote:

Hi!

>> -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");
>> -#endif
>> +	if (pdev->dev.of_node) {
>> +		const struct of_device_id *match;
>> +		match = of_match_node(&s3c24xx_i2c_match, pdev->dev.of_node);
> 
> I'd appreciate a comment explaining why match can't be NULL here (as to
> my understanding, it can't). Or just check for it, but this way it looks
> a bit fishy and people (hopefully ;)) will ask about it.


My understanding is that it can't be null for exactly same reason why
platform_get_device_id(pdev) can't be null either (see below).

I.e. prerequisite for this code to be run at all (as it's called from
driver's .probe()) is to be already matched against very same match
table.

As far I can see the only possibility for it to fail is to have
dev.of_node pointing to device tree node and NOT being instantiated
from DT description...

>> +		return (unsigned int)match->data;
>> +	}
>>  
>> -	type = platform_get_device_id(pdev)->driver_data;
>> -	return type == TYPE_S3C2440;
>> +	return platform_get_device_id(pdev)->driver_data;
>>  }


Regards,

-- 

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

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

[Wolfram Sang]

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

On Tue, Apr 24, 2012 at 10:40:49AM +0200, Karol Lewandowski wrote:
> On 23.04.2012 20:20, Wolfram Sang wrote:
> 
> Hi!
> 
> >> -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");
> >> -#endif
> >> +	if (pdev->dev.of_node) {
> >> +		const struct of_device_id *match;
> >> +		match = of_match_node(&s3c24xx_i2c_match, pdev->dev.of_node);
> > 
> > I'd appreciate a comment explaining why match can't be NULL here (as to
> > my understanding, it can't). Or just check for it, but this way it looks
> > a bit fishy and people (hopefully ;)) will ask about it.
> 
> 
> My understanding is that it can't be null for exactly same reason why
> platform_get_device_id(pdev) can't be null either (see below).
> 
> I.e. prerequisite for this code to be run at all (as it's called from
> driver's .probe()) is to be already matched against very same match
> table.

Yes, I agree. Yet, this is not obvious and people might try to fix it
(especially since programs like smatch report it as potentially
dangerous). Ah, whatever, I could simply apply the fix then :) OK.

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 1/2] i2c-s3c2410: Rework device type handling

[Karol Lewandowski]

On 24.04.2012 16:44, Wolfram Sang wrote:

> On Tue, Apr 24, 2012 at 10:40:49AM +0200, Karol Lewandowski wrote:
>> On 23.04.2012 20:20, Wolfram Sang wrote:


>>>> +	if (pdev->dev.of_node) {
>>>> +		const struct of_device_id *match;
>>>> +		match = of_match_node(&s3c24xx_i2c_match, pdev->dev.of_node);
>>>
>>> I'd appreciate a comment explaining why match can't be NULL here (as to
>>> my understanding, it can't). Or just check for it, but this way it looks
>>> a bit fishy and people (hopefully ;)) will ask about it.
>>
>>
>> My understanding is that it can't be null for exactly same reason why
>> platform_get_device_id(pdev) can't be null either (see below).
>>
>> I.e. prerequisite for this code to be run at all (as it's called from
>> driver's .probe()) is to be already matched against very same match
>> table.
> 
> Yes, I agree. Yet, this is not obvious and people might try to fix it
> (especially since programs like smatch report it as potentially
> dangerous). Ah, whatever, I could simply apply the fix then :) OK.


Great! I hope it won't cause any problems. :)

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

[PATCH 2/2] 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        |    8 ++++--
 drivers/i2c/busses/i2c-s3c2410.c                   |   30 ++++++++++++++++++++
 2 files changed, 36 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
index 38832c7..b6cb5a1 100644
--- a/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/samsung-i2c.txt
@@ -6,14 +6,18 @@ 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.
   - 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 for "samsung,s3c2440-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
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 23736ff..fa0b134 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -46,6 +46,8 @@
 
 /* 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 {
@@ -93,6 +95,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);
@@ -101,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);
@@ -483,6 +490,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);
 
@@ -492,6 +506,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;
 }
 
@@ -773,6 +796,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)) {
@@ -797,6 +823,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]);
 }
-- 
1.7.9.5

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

[Karol Lewandowski]

Hi,

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 (1):
  i2c-s3c2410: Rework device type handling

Tomasz Stanislawski (1):
  i2c-s3c2410: Add HDMIPHY quirk for S3C2440

 .../devicetree/bindings/i2c/samsung-i2c.txt        |   10 ++-
 drivers/i2c/busses/i2c-s3c2410.c                   |   90 ++++++++++++--------
 2 files changed, 62 insertions(+), 38 deletions(-)

-- 
1.7.9

[PATCH 1/2] 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 plain unsigned int that can
   hold not only device type but also hw 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 |   56 +++++++++++++------------------------
 1 files changed, 20 insertions(+), 36 deletions(-)

diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 737f721..5b400c6 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -44,8 +44,12 @@
 #include <plat/regs-iic.h>
 #include <plat/iic.h>
 
-/* i2c controller state */
+/* type */
+#define TYPE_BITS		0x000000ff
+#define TYPE_S3C2410		0x00000001
+#define TYPE_S3C2440		0x00000002
 
+/* i2c controller state */
 enum s3c24xx_i2c_state {
 	STATE_IDLE,
 	STATE_START,
@@ -54,14 +58,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            type;
 	unsigned int		suspended:1;
 
 	struct i2c_msg		*msg;
@@ -88,28 +88,6 @@ struct s3c24xx_i2c {
 #endif
 };
 
-/* default platform data removed, dev should always carry data. */
-
-/* s3c24xx_i2c_is2440()
- *
- * return true is this is an s3c2440
-*/
-
-static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
-{
-	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");
-#endif
-
-	type = platform_get_device_id(pdev)->driver_data;
-	return type == TYPE_S3C2440;
-}
-
 /* s3c24xx_i2c_master_complete
  *
  * complete the message and wake up the caller, using the given return code,
@@ -676,7 +654,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->type & TYPE_S3C2440) {
 		unsigned long sda_delay;
 
 		if (pdata->sda_delay) {
@@ -847,6 +825,8 @@ static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c)
 }
 
 #ifdef CONFIG_OF
+static const struct of_device_id s3c24xx_i2c_match[];
+
 /* s3c24xx_i2c_parse_dt
  *
  * Parse the device tree node and retreive the platform data.
@@ -856,11 +836,16 @@ static void
 s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
 {
 	struct s3c2410_platform_i2c *pdata = i2c->pdata;
+	const struct of_device_id *match;
 
 	if (!np)
 		return;
 
 	pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
+
+	match = of_match_node(&s3c24xx_i2c_match[0], np);
+	i2c->type = (unsigned int)match->data;
+
 	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",
@@ -906,9 +891,10 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
 		goto err_noclk;
 	}
 
-	if (pdata)
+	if (pdata) {
 		memcpy(i2c->pdata, pdata, sizeof(*pdata));
-	else
+		i2c->type = platform_get_device_id(pdev)->driver_data;
+	} else
 		s3c24xx_i2c_parse_dt(pdev->dev.of_node, i2c);
 
 	strlcpy(i2c->adap.name, "s3c2410-i2c", sizeof(i2c->adap.name));
@@ -1123,13 +1109,11 @@ 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 *)TYPE_S3C2410 },
+	{ .compatible = "samsung,s3c2440-i2c", .data = (void *)TYPE_S3C2440 },
 	{},
 };
 MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
-#else
-#define s3c24xx_i2c_match NULL
 #endif
 
 static struct platform_driver s3c24xx_i2c_driver = {
@@ -1140,7 +1124,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

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

[Thomas Abraham]

On 9 March 2012 22:34, Karol Lewandowski <k.lewandowsk@samsung.com> 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 plain unsigned int that can
>   hold not only device type but also hw 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 |   56 +++++++++++++------------------------
>  1 files changed, 20 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
> index 737f721..5b400c6 100644
> --- a/drivers/i2c/busses/i2c-s3c2410.c
> +++ b/drivers/i2c/busses/i2c-s3c2410.c
> @@ -44,8 +44,12 @@
>  #include <plat/regs-iic.h>
>  #include <plat/iic.h>
>
> -/* i2c controller state */
> +/* type */
> +#define TYPE_BITS              0x000000ff
> +#define TYPE_S3C2410           0x00000001
> +#define TYPE_S3C2440           0x00000002
>
> +/* i2c controller state */
>  enum s3c24xx_i2c_state {
>        STATE_IDLE,
>        STATE_START,
> @@ -54,14 +58,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            type;
>        unsigned int            suspended:1;
>
>        struct i2c_msg          *msg;
> @@ -88,28 +88,6 @@ struct s3c24xx_i2c {
>  #endif
>  };
>
> -/* default platform data removed, dev should always carry data. */
> -
> -/* s3c24xx_i2c_is2440()
> - *
> - * return true is this is an s3c2440
> -*/
> -
> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
> -{
> -       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");
> -#endif
> -
> -       type = platform_get_device_id(pdev)->driver_data;
> -       return type == TYPE_S3C2440;
> -}
> -
>  /* s3c24xx_i2c_master_complete
>  *
>  * complete the message and wake up the caller, using the given return code,
> @@ -676,7 +654,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->type & TYPE_S3C2440) {

This should be changed to
if (i2c->type == TYPE_S3C2440)

>                unsigned long sda_delay;
>
>                if (pdata->sda_delay) {
> @@ -847,6 +825,8 @@ static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c)
>  }
>
>  #ifdef CONFIG_OF
> +static const struct of_device_id s3c24xx_i2c_match[];
> +
>  /* s3c24xx_i2c_parse_dt
>  *
>  * Parse the device tree node and retreive the platform data.
> @@ -856,11 +836,16 @@ static void
>  s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
>  {
>        struct s3c2410_platform_i2c *pdata = i2c->pdata;
> +       const struct of_device_id *match;
>
>        if (!np)
>                return;
>
>        pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
> +
> +       match = of_match_node(&s3c24xx_i2c_match[0], np);

This could be
match = of_match_node(s3c24xx_i2c_match, np);

> +       i2c->type = (unsigned int)match->data;
> +

This function (s3c24xx_i2c_parse_dt) is intended to retrieve the data
from i2c device node. It would be better to not add the determination
of the i2c type inside this function.

>        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",
> @@ -906,9 +891,10 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>                goto err_noclk;
>        }
>
> -       if (pdata)
> +       if (pdata) {
>                memcpy(i2c->pdata, pdata, sizeof(*pdata));
> -       else
> +               i2c->type = platform_get_device_id(pdev)->driver_data;
> +       } else
>                s3c24xx_i2c_parse_dt(pdev->dev.of_node, i2c);

It would be better to move the task of retrieving the driver data to a
separate function. An example of this listed below (copied from the
samsung uart driver).

static inline struct s3c24xx_serial_drv_data *s3c24xx_get_driver_data(
                        struct platform_device *pdev)
{
#ifdef CONFIG_OF
        if (pdev->dev.of_node) {
                const struct of_device_id *match;
                match = of_match_node(s3c24xx_uart_dt_match, pdev->dev.of_node);
                return (struct s3c24xx_serial_drv_data *)match->data;
        }
#endif
        return (struct s3c24xx_serial_drv_data *)
                        platform_get_device_id(pdev)->driver_data;
}

>
>        strlcpy(i2c->adap.name, "s3c2410-i2c", sizeof(i2c->adap.name));
> @@ -1123,13 +1109,11 @@ 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 *)TYPE_S3C2410 },
> +       { .compatible = "samsung,s3c2440-i2c", .data = (void *)TYPE_S3C2440 },
>        {},
>  };
>  MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
> -#else
> -#define s3c24xx_i2c_match NULL
>  #endif
>
>  static struct platform_driver s3c24xx_i2c_driver = {
> @@ -1140,7 +1124,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),

Should this change be a separate patch?

Thanks,
Thomas.

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

[Karol Lewandowski]

On 12.03.2012 06:58, Thomas Abraham wrote:

Hi Thomas!

> On 9 March 2012 22:34, Karol Lewandowski <k.lewandowsk@samsung.com> 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 plain unsigned int that can
>>   hold not only device type but also hw 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 |   56 +++++++++++++------------------------
>>  1 files changed, 20 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>> index 737f721..5b400c6 100644
>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
>> @@ -44,8 +44,12 @@
>>  #include <plat/regs-iic.h>
>>  #include <plat/iic.h>
>>
>> -/* i2c controller state */
>> +/* type */
>> +#define TYPE_BITS              0x000000ff
>> +#define TYPE_S3C2410           0x00000001
>> +#define TYPE_S3C2440           0x00000002
>>
>> +/* i2c controller state */
>>  enum s3c24xx_i2c_state {
>>        STATE_IDLE,
>>        STATE_START,
>> @@ -54,14 +58,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            type;
>>        unsigned int            suspended:1;
>>
>>        struct i2c_msg          *msg;
>> @@ -88,28 +88,6 @@ struct s3c24xx_i2c {
>>  #endif
>>  };
>>
>> -/* default platform data removed, dev should always carry data. */
>> -
>> -/* s3c24xx_i2c_is2440()
>> - *
>> - * return true is this is an s3c2440
>> -*/
>> -
>> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
>> -{
>> -       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");
>> -#endif
>> -
>> -       type = platform_get_device_id(pdev)->driver_data;
>> -       return type == TYPE_S3C2440;
>> -}
>> -
>>  /* s3c24xx_i2c_master_complete
>>  *
>>  * complete the message and wake up the caller, using the given return code,
>> @@ -676,7 +654,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->type & TYPE_S3C2440) {
> 
> This should be changed to
> if (i2c->type == TYPE_S3C2440)


Equality check won't work here after second patch is applied
(i2c->type might have FLAG_*s set on upper bits).

> 
>>                unsigned long sda_delay;
>>
>>                if (pdata->sda_delay) {
>> @@ -847,6 +825,8 @@ static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c)
>>  }
>>
>>  #ifdef CONFIG_OF
>> +static const struct of_device_id s3c24xx_i2c_match[];
>> +
>>  /* s3c24xx_i2c_parse_dt
>>  *
>>  * Parse the device tree node and retreive the platform data.
>> @@ -856,11 +836,16 @@ static void
>>  s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
>>  {
>>        struct s3c2410_platform_i2c *pdata = i2c->pdata;
>> +       const struct of_device_id *match;
>>
>>        if (!np)
>>                return;
>>
>>        pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
>> +
>> +       match = of_match_node(&s3c24xx_i2c_match[0], np);
> 
> This could be
> match = of_match_node(s3c24xx_i2c_match, np);


If you (and Ben, of course) don't mind I would prefer more verbose but
also more explicit version.

>> +       i2c->type = (unsigned int)match->data;
>> +
> 
> This function (s3c24xx_i2c_parse_dt) is intended to retrieve the data
> from i2c device node. It would be better to not add the determination
> of the i2c type inside this function.
> 
>>        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",
>> @@ -906,9 +891,10 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>>                goto err_noclk;
>>        }
>>
>> -       if (pdata)
>> +       if (pdata) {
>>                memcpy(i2c->pdata, pdata, sizeof(*pdata));
>> -       else
>> +               i2c->type = platform_get_device_id(pdev)->driver_data;
>> +       } else
>>                s3c24xx_i2c_parse_dt(pdev->dev.of_node, i2c);
> 
> It would be better to move the task of retrieving the driver data to a
> separate function. An example of this listed below (copied from the
> samsung uart driver).


Well, I was under impression that #ifdefs inside functions were quite
disliked by kernel devs.  Now I see that these are quite common in kernel..
Thus, I will happily use your suggestion, thanks.

> 
> static inline struct s3c24xx_serial_drv_data *s3c24xx_get_driver_data(
>                         struct platform_device *pdev)
> {
> #ifdef CONFIG_OF
>         if (pdev->dev.of_node) {
>                 const struct of_device_id *match;
>                 match = of_match_node(s3c24xx_uart_dt_match, pdev->dev.of_node);
>                 return (struct s3c24xx_serial_drv_data *)match->data;
>         }
> #endif
>         return (struct s3c24xx_serial_drv_data *)
>                         platform_get_device_id(pdev)->driver_data;
> }
> 
>>
>>        strlcpy(i2c->adap.name, "s3c2410-i2c", sizeof(i2c->adap.name));
>> @@ -1123,13 +1109,11 @@ 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 *)TYPE_S3C2410 },
>> +       { .compatible = "samsung,s3c2440-i2c", .data = (void *)TYPE_S3C2440 },
>>        {},
>>  };
>>  MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
>> -#else
>> -#define s3c24xx_i2c_match NULL
>>  #endif
>>
>>  static struct platform_driver s3c24xx_i2c_driver = {
>> @@ -1140,7 +1124,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),
> 
> Should this change be a separate patch?


Ok, I'll move this part to another patch.

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

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

[Thomas Abraham]

On 12 March 2012 18:46, Karol Lewandowski <k.lewandowsk@samsung.com> wrote:
> On 12.03.2012 06:58, Thomas Abraham wrote:
>
> Hi Thomas!
>
>> On 9 March 2012 22:34, Karol Lewandowski <k.lewandowsk@samsung.com> 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 plain unsigned int that can
>>>   hold not only device type but also hw 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 |   56 +++++++++++++------------------------
>>>  1 files changed, 20 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
>>> index 737f721..5b400c6 100644
>>> --- a/drivers/i2c/busses/i2c-s3c2410.c
>>> +++ b/drivers/i2c/busses/i2c-s3c2410.c
>>> @@ -44,8 +44,12 @@
>>>  #include <plat/regs-iic.h>
>>>  #include <plat/iic.h>
>>>
>>> -/* i2c controller state */
>>> +/* type */
>>> +#define TYPE_BITS              0x000000ff
>>> +#define TYPE_S3C2410           0x00000001
>>> +#define TYPE_S3C2440           0x00000002
>>>
>>> +/* i2c controller state */
>>>  enum s3c24xx_i2c_state {
>>>        STATE_IDLE,
>>>        STATE_START,
>>> @@ -54,14 +58,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            type;
>>>        unsigned int            suspended:1;
>>>
>>>        struct i2c_msg          *msg;
>>> @@ -88,28 +88,6 @@ struct s3c24xx_i2c {
>>>  #endif
>>>  };
>>>
>>> -/* default platform data removed, dev should always carry data. */
>>> -
>>> -/* s3c24xx_i2c_is2440()
>>> - *
>>> - * return true is this is an s3c2440
>>> -*/
>>> -
>>> -static inline int s3c24xx_i2c_is2440(struct s3c24xx_i2c *i2c)
>>> -{
>>> -       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");
>>> -#endif
>>> -
>>> -       type = platform_get_device_id(pdev)->driver_data;
>>> -       return type == TYPE_S3C2440;
>>> -}
>>> -
>>>  /* s3c24xx_i2c_master_complete
>>>  *
>>>  * complete the message and wake up the caller, using the given return code,
>>> @@ -676,7 +654,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->type & TYPE_S3C2440) {
>>
>> This should be changed to
>> if (i2c->type == TYPE_S3C2440)
>
>
> Equality check won't work here after second patch is applied
> (i2c->type might have FLAG_*s set on upper bits).
>

Right. I assumed that if a another type is added, its value will be 3.
But looks like you intend to assign 4, 8, 16 and so on for the new
types. Probably, this approach needs to documented in this file.
Otherwise, the above check would not work if ever a new type is added
without knowing this approach.

>>
>>>                unsigned long sda_delay;
>>>
>>>                if (pdata->sda_delay) {
>>> @@ -847,6 +825,8 @@ static int s3c24xx_i2c_init(struct s3c24xx_i2c *i2c)
>>>  }
>>>
>>>  #ifdef CONFIG_OF
>>> +static const struct of_device_id s3c24xx_i2c_match[];
>>> +
>>>  /* s3c24xx_i2c_parse_dt
>>>  *
>>>  * Parse the device tree node and retreive the platform data.
>>> @@ -856,11 +836,16 @@ static void
>>>  s3c24xx_i2c_parse_dt(struct device_node *np, struct s3c24xx_i2c *i2c)
>>>  {
>>>        struct s3c2410_platform_i2c *pdata = i2c->pdata;
>>> +       const struct of_device_id *match;
>>>
>>>        if (!np)
>>>                return;
>>>
>>>        pdata->bus_num = -1; /* i2c bus number is dynamically assigned */
>>> +
>>> +       match = of_match_node(&s3c24xx_i2c_match[0], np);
>>
>> This could be
>> match = of_match_node(s3c24xx_i2c_match, np);
>
>
> If you (and Ben, of course) don't mind I would prefer more verbose but
> also more explicit version.
>

I am fine the explicit version.

>>> +       i2c->type = (unsigned int)match->data;
>>> +
>>
>> This function (s3c24xx_i2c_parse_dt) is intended to retrieve the data
>> from i2c device node. It would be better to not add the determination
>> of the i2c type inside this function.
>>
>>>        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",
>>> @@ -906,9 +891,10 @@ static int s3c24xx_i2c_probe(struct platform_device *pdev)
>>>                goto err_noclk;
>>>        }
>>>
>>> -       if (pdata)
>>> +       if (pdata) {
>>>                memcpy(i2c->pdata, pdata, sizeof(*pdata));
>>> -       else
>>> +               i2c->type = platform_get_device_id(pdev)->driver_data;
>>> +       } else
>>>                s3c24xx_i2c_parse_dt(pdev->dev.of_node, i2c);
>>
>> It would be better to move the task of retrieving the driver data to a
>> separate function. An example of this listed below (copied from the
>> samsung uart driver).
>
>
> Well, I was under impression that #ifdefs inside functions were quite
> disliked by kernel devs.  Now I see that these are quite common in kernel..
> Thus, I will happily use your suggestion, thanks.
>

It would be a bad idea to use #ifdef that would deviate from the
objective of single kernel binary image. But in the example below, the
#ifdef is used solely to exclude the OF related code if the kernel
being built do not have OF support included. The check on
pdev->dev.of_node is the one that ensures that there is runtime
compatibility with both dt and non-dt. So the #ifdef does not
necessitate separate kernel to be built for dt and non-dt case.

>>
>> static inline struct s3c24xx_serial_drv_data *s3c24xx_get_driver_data(
>>                         struct platform_device *pdev)
>> {
>> #ifdef CONFIG_OF
>>         if (pdev->dev.of_node) {
>>                 const struct of_device_id *match;
>>                 match = of_match_node(s3c24xx_uart_dt_match, pdev->dev.of_node);
>>                 return (struct s3c24xx_serial_drv_data *)match->data;
>>         }
>> #endif
>>         return (struct s3c24xx_serial_drv_data *)
>>                         platform_get_device_id(pdev)->driver_data;
>> }
>>
>>>
>>>        strlcpy(i2c->adap.name, "s3c2410-i2c", sizeof(i2c->adap.name));
>>> @@ -1123,13 +1109,11 @@ 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 *)TYPE_S3C2410 },
>>> +       { .compatible = "samsung,s3c2440-i2c", .data = (void *)TYPE_S3C2440 },
>>>        {},
>>>  };
>>>  MODULE_DEVICE_TABLE(of, s3c24xx_i2c_match);
>>> -#else
>>> -#define s3c24xx_i2c_match NULL
>>>  #endif
>>>
>>>  static struct platform_driver s3c24xx_i2c_driver = {
>>> @@ -1140,7 +1124,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),
>>
>> Should this change be a separate patch?
>
>
> Ok, I'll move this part to another patch.
>
> Thanks for review!
> --
> Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform