[PATCH 0/2] support i2c-designware-hs

[PATCH 0/2] support i2c-designware-hs

[Zhangfei Gao]

Add support i2c-designware-hs.c, based on designware i2c ip 

Zhangfei Gao (2):
  i2c-designware: support no DW_IC_COMP_TYPE platform
  i2c: designware: Add i2c-designware-hs

 .../devicetree/bindings/i2c/i2c-designware-hs.txt  |   30 +++
 drivers/i2c/busses/Kconfig                         |   10 +
 drivers/i2c/busses/Makefile                        |    1 +
 drivers/i2c/busses/i2c-designware-core.c           |   24 +--
 drivers/i2c/busses/i2c-designware-core.h           |    1 +
 drivers/i2c/busses/i2c-designware-hs.c             |  194 ++++++++++++++++++++
 6 files changed, 249 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
 create mode 100644 drivers/i2c/busses/i2c-designware-hs.c

-- 
1.7.9.5

[PATCH 1/2] i2c-designware: support no DW_IC_COMP_TYPE platform

[Zhangfei Gao]

Check accessor_flags before reading DW_IC_COMP_TYPE
Give chance to platform no such register

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/i2c/busses/i2c-designware-core.c |   24 +++++++++++++-----------
 drivers/i2c/busses/i2c-designware-core.h |    1 +
 2 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index c41ca63..80cabbd 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -286,17 +286,19 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 
 	input_clock_khz = dev->get_clk_rate_khz(dev);
 
-	reg = dw_readl(dev, DW_IC_COMP_TYPE);
-	if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
-		/* Configure register endianess access */
-		dev->accessor_flags |= ACCESS_SWAP;
-	} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
-		/* Configure register access mode 16bit */
-		dev->accessor_flags |= ACCESS_16BIT;
-	} else if (reg != DW_IC_COMP_TYPE_VALUE) {
-		dev_err(dev->dev, "Unknown Synopsys component type: "
-			"0x%08x\n", reg);
-		return -ENODEV;
+	if (!dev->accessor_flags) {
+		reg = dw_readl(dev, DW_IC_COMP_TYPE);
+		if (reg == ___constant_swab32(DW_IC_COMP_TYPE_VALUE)) {
+			/* Configure register endianess access */
+			dev->accessor_flags |= ACCESS_SWAP;
+		} else if (reg == (DW_IC_COMP_TYPE_VALUE & 0x0000ffff)) {
+			/* Configure register access mode 16bit */
+			dev->accessor_flags |= ACCESS_16BIT;
+		} else if (reg != DW_IC_COMP_TYPE_VALUE) {
+			dev_err(dev->dev, "Unknown Synopsys component type: "
+					"0x%08x\n", reg);
+			return -ENODEV;
+		}
 	}
 
 	/* Disable the adapter */
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index e761ad1..b38b54e 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -94,6 +94,7 @@ struct dw_i2c_dev {
 
 #define ACCESS_SWAP		0x00000001
 #define ACCESS_16BIT		0x00000002
+#define ACCESS_32BIT		0x00000004
 
 extern u32 dw_readl(struct dw_i2c_dev *dev, int offset);
 extern void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset);
-- 
1.7.9.5

[PATCH 2/2] i2c: designware: Add i2c-designware-hs

[Zhangfei Gao]

Add support hisilicon i2c driver, which reuse designware i2c ip

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 .../devicetree/bindings/i2c/i2c-designware-hs.txt  |   30 +++
 drivers/i2c/busses/Kconfig                         |   10 +
 drivers/i2c/busses/Makefile                        |    1 +
 drivers/i2c/busses/i2c-designware-hs.c             |  194 ++++++++++++++++++++
 4 files changed, 235 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
 create mode 100644 drivers/i2c/busses/i2c-designware-hs.c

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt b/Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
new file mode 100644
index 0000000..08908fa
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
@@ -0,0 +1,30 @@
+* Hisilicon I2C Controller
+
+Required properties :
+
+ - compatible : should be "hisilicon,designware-i2c"
+ - reg : Offset and length of the register set for the device
+ - interrupts : <IRQ> where IRQ is the interrupt number.
+
+Example :
+
+	i2c0: i2c at fcb08000 {
+		compatible = "hs,designware-i2c";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0xfcb08000 0x1000>;
+		interrupts = <0 28 4>;
+		clocks = <&pclk>;
+		status = "disabled";
+	};
+
+	Client in i2c0 bus with add 0x58 could be added as example
+	i2c0: i2c at fcb08000 {
+		status = "ok";
+		pinctrl-names = "default";
+		pinctrl-0 = <&i2c0_pmx_func &i2c0_cfg_func>;
+		i2c_client1: i2c_client at 58 {
+			compatible = "hisilicon,i2c_client_tpa2028";
+			reg = <0x58>;
+		};
+	};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 631736e..4405476 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -419,6 +419,16 @@ config I2C_DESIGNWARE_PCI
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-designware-pci.
 
+config I2C_DESIGNWARE_HS
+	tristate "Synopsys DesignWare Hisilicon"
+	depends on ARCH_HI3xxx
+	depends on HAVE_CLK
+	select I2C_DESIGNWARE_CORE
+	help
+	  If you say yes to this option, support will be included for the
+	  Synopsys DesignWare I2C adapter on Hisilicon.
+	  Only master mode is supported.
+
 config I2C_EG20T
 	tristate "Intel EG20T PCH/LAPIS Semicon IOH(ML7213/ML7223/ML7831) I2C"
 	depends on PCI
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 8f4fc23..2408932 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -41,6 +41,7 @@ obj-$(CONFIG_I2C_DESIGNWARE_PLATFORM)	+= i2c-designware-platform.o
 i2c-designware-platform-objs := i2c-designware-platdrv.o
 obj-$(CONFIG_I2C_DESIGNWARE_PCI)	+= i2c-designware-pci.o
 i2c-designware-pci-objs := i2c-designware-pcidrv.o
+obj-$(CONFIG_I2C_DESIGNWARE_HS)	+= i2c-designware-hs.o
 obj-$(CONFIG_I2C_EG20T)		+= i2c-eg20t.o
 obj-$(CONFIG_I2C_GPIO)		+= i2c-gpio.o
 obj-$(CONFIG_I2C_HIGHLANDER)	+= i2c-highlander.o
diff --git a/drivers/i2c/busses/i2c-designware-hs.c b/drivers/i2c/busses/i2c-designware-hs.c
new file mode 100644
index 0000000..94d1496
--- /dev/null
+++ b/drivers/i2c/busses/i2c-designware-hs.c
@@ -0,0 +1,194 @@
+/*
+ * Hisilicon Synopsys DesignWare I2C adapter driver (master only).
+ *
+ * Copyright (c) 2013 Linaro Ltd.
+ * Copyright (c) 2013 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/clk.h>
+#include <linux/errno.h>
+#include <linux/sched.h>
+#include <linux/err.h>
+#include <linux/interrupt.h>
+#include <linux/of_i2c.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/pinctrl/consumer.h>
+#include "i2c-designware-core.h"
+
+static struct i2c_algorithm hs_i2c_dw_algo = {
+	.master_xfer	= i2c_dw_xfer,
+	.functionality	= i2c_dw_func,
+};
+static u32 hs_i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
+{
+	return clk_get_rate(dev->clk)/1000;
+}
+
+static int hs_dw_i2c_probe(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *d;
+	struct i2c_adapter *adap;
+	struct resource *iores;
+	struct pinctrl *pinctrl;
+	int r;
+
+	d = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
+	if (!d)
+		return -ENOMEM;
+
+	/* NOTE: driver uses the static register mapping */
+	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!iores)
+		return -EINVAL;
+
+	d->base = devm_request_and_ioremap(&pdev->dev, iores);
+	if (!d->base)
+		return -EADDRNOTAVAIL;
+
+	d->irq = platform_get_irq(pdev, 0);
+	if (d->irq < 0) {
+		dev_err(&pdev->dev, "no irq resource?\n");
+		return d->irq; /* -ENXIO */
+	}
+
+	r = devm_request_irq(&pdev->dev, d->irq,
+		i2c_dw_isr, IRQF_DISABLED, pdev->name, d);
+	if (r) {
+		dev_err(&pdev->dev, "failure requesting irq %i\n", d->irq);
+		return -EINVAL;
+	}
+
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl))
+		dev_warn(&pdev->dev,
+			"pins are not configured from the driver\n");
+
+	d->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(d->clk))
+		return -ENODEV;
+
+	d->get_clk_rate_khz = hs_i2c_dw_get_clk_rate_khz;
+	clk_prepare_enable(d->clk);
+	init_completion(&d->cmd_complete);
+	mutex_init(&d->lock);
+	d->dev = get_device(&pdev->dev);
+
+	d->functionality =
+		I2C_FUNC_I2C |
+		I2C_FUNC_10BIT_ADDR |
+		I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_I2C_BLOCK;
+	d->master_cfg =  DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
+		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
+	d->accessor_flags = ACCESS_32BIT;
+	d->tx_fifo_depth = 16;
+	d->rx_fifo_depth = 16;
+
+	r = i2c_dw_init(d);
+	if (r)
+		goto err;
+
+	i2c_dw_disable_int(d);
+
+	adap = &d->adapter;
+	i2c_set_adapdata(adap, d);
+	adap->owner = THIS_MODULE;
+	adap->class = I2C_CLASS_HWMON;
+	strlcpy(adap->name, "Synopsys DesignWare I2C adapter",
+			sizeof(adap->name));
+	adap->algo = &hs_i2c_dw_algo;
+	adap->dev.parent = &pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
+
+	adap->nr = pdev->id;
+	r = i2c_add_numbered_adapter(adap);
+	if (r) {
+		dev_err(&pdev->dev, "failure adding adapter\n");
+		goto err;
+	}
+	of_i2c_register_devices(adap);
+	platform_set_drvdata(pdev, d);
+
+	return 0;
+err:
+	clk_disable_unprepare(d->clk);
+	put_device(&pdev->dev);
+	return r;
+}
+
+static int hs_dw_i2c_remove(struct platform_device *pdev)
+{
+	struct dw_i2c_dev *d = platform_get_drvdata(pdev);
+
+	platform_set_drvdata(pdev, NULL);
+	i2c_del_adapter(&d->adapter);
+	put_device(&pdev->dev);
+	clk_disable_unprepare(d->clk);
+	i2c_dw_disable(d);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id hs_dw_i2c_of_match[] = {
+	{ .compatible = "hisilicon,designware-i2c", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, hs_dw_i2c_of_match);
+#endif
+
+#ifdef CONFIG_PM
+static int hs_dw_i2c_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+
+	clk_disable_unprepare(i_dev->clk);
+
+	return 0;
+}
+
+static int hs_dw_i2c_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct dw_i2c_dev *i_dev = platform_get_drvdata(pdev);
+
+	clk_prepare_enable(i_dev->clk);
+	i2c_dw_init(i_dev);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(hs_dw_i2c_dev_pm_ops, hs_dw_i2c_suspend,
+			hs_dw_i2c_resume);
+
+static struct platform_driver hs_dw_i2c_driver = {
+	.probe		= hs_dw_i2c_probe,
+	.remove		= hs_dw_i2c_remove,
+	.driver		= {
+		.name	= "i2c_designware-hs",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(hs_dw_i2c_of_match),
+		.pm	= &hs_dw_i2c_dev_pm_ops,
+	},
+};
+module_platform_driver(hs_dw_i2c_driver);
+
+MODULE_DESCRIPTION("HS Synopsys DesignWare I2C bus adapter");
+MODULE_ALIAS("platform:i2c_designware-hs");
+MODULE_LICENSE("GPL");
-- 
1.7.9.5

[PATCH 1/2] i2c-designware: support no DW_IC_COMP_TYPE platform

[Baruch Siach]

Hi Zhangfei Gao,

On Sun, Jun 09, 2013 at 10:36:41AM +0800, Zhangfei Gao wrote:
> Check accessor_flags before reading DW_IC_COMP_TYPE
> Give chance to platform no such register
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  drivers/i2c/busses/i2c-designware-core.c |   24 +++++++++++++-----------
>  drivers/i2c/busses/i2c-designware-core.h |    1 +
>  2 files changed, 14 insertions(+), 11 deletions(-)

[...]

> diff --git a/drivers/i2c/busses/i2c-designware-core.h 
> b/drivers/i2c/busses/i2c-designware-core.h
> index e761ad1..b38b54e 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -94,6 +94,7 @@ struct dw_i2c_dev {
>  
>  #define ACCESS_SWAP		0x00000001
>  #define ACCESS_16BIT		0x00000002
> +#define ACCESS_32BIT		0x00000004

This belongs to the next patch, not to this one.

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/2] i2c: designware: Add i2c-designware-hs

[Baruch Siach]

Hi Zhangfei Gao,

On Sun, Jun 09, 2013 at 10:36:42AM +0800, Zhangfei Gao wrote:
> Add support hisilicon i2c driver, which reuse designware i2c ip
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  .../devicetree/bindings/i2c/i2c-designware-hs.txt  |   30 +++
>  drivers/i2c/busses/Kconfig                         |   10 +
>  drivers/i2c/busses/Makefile                        |    1 +
>  drivers/i2c/busses/i2c-designware-hs.c             |  194 ++++++++++++++++++++
>  4 files changed, 235 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
>  create mode 100644 drivers/i2c/busses/i2c-designware-hs.c
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt b/Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
> new file mode 100644
> index 0000000..08908fa
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
> @@ -0,0 +1,30 @@
> +* Hisilicon I2C Controller
> +
> +Required properties :
> +
> + - compatible : should be "hisilicon,designware-i2c"
> + - reg : Offset and length of the register set for the device
> + - interrupts : <IRQ> where IRQ is the interrupt number.
> +
> +Example :
> +
> +	i2c0: i2c at fcb08000 {
> +		compatible = "hs,designware-i2c";

A few comments on this one:

1. You should Cc devicetree-discuss at lists.ozlabs.org on patches touching ftd 
   bindings (added to Cc)

2. The convention is to use the IC block designer in the "compatible" property 
   prefix, in this case Symopsys (snps)

3. This does not match the compatible property in hs_dw_i2c_of_match[] below 
   where you use "hisilicon,designware-i2c"

4. Please update Documentation/devicetree/bindings/vendor-prefixes.txt when 
   adding new vendor prefixes

> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0xfcb08000 0x1000>;
> +		interrupts = <0 28 4>;
> +		clocks = <&pclk>;
> +		status = "disabled";
> +	};
> +
> +	Client in i2c0 bus with add 0x58 could be added as example
> +	i2c0: i2c at fcb08000 {
> +		status = "ok";

The convention is to use "okay".

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&i2c0_pmx_func &i2c0_cfg_func>;
> +		i2c_client1: i2c_client at 58 {
> +			compatible = "hisilicon,i2c_client_tpa2028";
> +			reg = <0x58>;
> +		};
> +	};

[...]

The code below looks like a direct copy of i2c-designware-platdrv.c. Is there 
any reason you can't use that code instead?

> +static struct i2c_algorithm hs_i2c_dw_algo = {
> +	.master_xfer	= i2c_dw_xfer,
> +	.functionality	= i2c_dw_func,
> +};
> +static u32 hs_i2c_dw_get_clk_rate_khz(struct dw_i2c_dev *dev)
> +{
> +	return clk_get_rate(dev->clk)/1000;
> +}
> +
> +static int hs_dw_i2c_probe(struct platform_device *pdev)
> +{
> +	struct dw_i2c_dev *d;
> +	struct i2c_adapter *adap;
> +	struct resource *iores;
> +	struct pinctrl *pinctrl;
> +	int r;
> +
> +	d = devm_kzalloc(&pdev->dev, sizeof(struct dw_i2c_dev), GFP_KERNEL);
> +	if (!d)
> +		return -ENOMEM;
> +
> +	/* NOTE: driver uses the static register mapping */
> +	iores = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!iores)
> +		return -EINVAL;
> +
> +	d->base = devm_request_and_ioremap(&pdev->dev, iores);
> +	if (!d->base)
> +		return -EADDRNOTAVAIL;
> +
> +	d->irq = platform_get_irq(pdev, 0);
> +	if (d->irq < 0) {
> +		dev_err(&pdev->dev, "no irq resource?\n");
> +		return d->irq; /* -ENXIO */
> +	}

[...]

baruch

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 1/2] i2c-designware: support no DW_IC_COMP_TYPE platform

[zhangfei]

On 13-06-09 11:06 AM, Baruch Siach wrote:
> Hi Zhangfei Gao,
>
> On Sun, Jun 09, 2013 at 10:36:41AM +0800, Zhangfei Gao wrote:
>> Check accessor_flags before reading DW_IC_COMP_TYPE
>> Give chance to platform no such register
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-designware-core.c |   24 +++++++++++++-----------
>>   drivers/i2c/busses/i2c-designware-core.h |    1 +
>>   2 files changed, 14 insertions(+), 11 deletions(-)
>
> [...]
>
>> diff --git a/drivers/i2c/busses/i2c-designware-core.h
>> b/drivers/i2c/busses/i2c-designware-core.h
>> index e761ad1..b38b54e 100644
>> --- a/drivers/i2c/busses/i2c-designware-core.h
>> +++ b/drivers/i2c/busses/i2c-designware-core.h
>> @@ -94,6 +94,7 @@ struct dw_i2c_dev {
>>
>>   #define ACCESS_SWAP		0x00000001
>>   #define ACCESS_16BIT		0x00000002
>> +#define ACCESS_32BIT		0x00000004
>
> This belongs to the next patch, not to this one.
>
> baruch
>
Thanks baruch, make sense.

[PATCH 2/2] i2c: designware: Add i2c-designware-hs

[zhangfei gao]

>> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
>> @@ -0,0 +1,30 @@
>> +* Hisilicon I2C Controller
>> +
>> +Required properties :
>> +
>> + - compatible : should be "hisilicon,designware-i2c"
>> + - reg : Offset and length of the register set for the device
>> + - interrupts : <IRQ> where IRQ is the interrupt number.
>> +
>> +Example :
>> +
>> +     i2c0: i2c at fcb08000 {
>> +             compatible = "hs,designware-i2c";
>
> A few comments on this one:
>
> 1. You should Cc devicetree-discuss at lists.ozlabs.org on patches touching ftd
>    bindings (added to Cc)
>
> 2. The convention is to use the IC block designer in the "compatible" property
>    prefix, in this case Symopsys (snps)
>
> 3. This does not match the compatible property in hs_dw_i2c_of_match[] below
>    where you use "hisilicon,designware-i2c"
>
> 4. Please update Documentation/devicetree/bindings/vendor-prefixes.txt when
>    adding new vendor prefixes

Thanks Baruch for the kind education, really useful.
How about using .compatible = "snps,hisilicon-i2c"

>> +     Client in i2c0 bus with add 0x58 could be added as example
>> +     i2c0: i2c at fcb08000 {
>> +             status = "ok";
>
> The convention is to use "okay".
got it.

>
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&i2c0_pmx_func &i2c0_cfg_func>;
>> +             i2c_client1: i2c_client at 58 {
>> +                     compatible = "hisilicon,i2c_client_tpa2028";
>> +                     reg = <0x58>;
>> +             };
>> +     };
>
> [...]
>
> The code below looks like a direct copy of i2c-designware-platdrv.c. Is there
> any reason you can't use that code instead?

Not understood i2c-designware-platdrv.c can be directly touched.
Usually, there is register function, or external function call.

It would be great if we could directly add hisilicon support in
i2c-designware-platdrv.c.
How about adding these code to distinguish.

The concern is will platdrv.c become bigger and bigger?
What in case private register have to be accessed?

struct dw_i2c_data {
        u32 accessor_flags;
        unsigned int tx_fifo_depth;
        unsigned int rx_fifo_depth;
};

static struct dw_i2c_data hisilicon_data = {
        .accessor_flags = ACCESS_32BIT,
        .tx_fifo_depth = 16,
        .rx_fifo_depth = 16,
};
{ .compatible = "snps,hisilicon-i2c", .data = &hisilicon_data},

[PATCH 2/2] i2c: designware: Add i2c-designware-hs

[Baruch Siach]

Hi zhangfei gao,

On Sun, Jun 09, 2013 at 04:59:48PM +0800, zhangfei gao wrote:
> >> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware-hs.txt
> >> @@ -0,0 +1,30 @@
> >> +* Hisilicon I2C Controller
> >> +
> >> +Required properties :
> >> +
> >> + - compatible : should be "hisilicon,designware-i2c"
> >> + - reg : Offset and length of the register set for the device
> >> + - interrupts : <IRQ> where IRQ is the interrupt number.
> >> +
> >> +Example :
> >> +
> >> +     i2c0: i2c at fcb08000 {
> >> +             compatible = "hs,designware-i2c";
> >
> > A few comments on this one:
> >
> > 1. You should Cc devicetree-discuss at lists.ozlabs.org on patches touching ftd
> >    bindings (added to Cc)
> >
> > 2. The convention is to use the IC block designer in the "compatible" property
> >    prefix, in this case Symopsys (snps)
> >
> > 3. This does not match the compatible property in hs_dw_i2c_of_match[] below
> >    where you use "hisilicon,designware-i2c"
> >
> > 4. Please update Documentation/devicetree/bindings/vendor-prefixes.txt when
> >    adding new vendor prefixes
> 
> Thanks Baruch for the kind education, really useful.
> How about using .compatible = "snps,hisilicon-i2c"

I don't think this is needed. See below.

> >> +     Client in i2c0 bus with add 0x58 could be added as example
> >> +     i2c0: i2c at fcb08000 {
> >> +             status = "ok";
> >
> > The convention is to use "okay".
> got it.
> 
> >
> >> +             pinctrl-names = "default";
> >> +             pinctrl-0 = <&i2c0_pmx_func &i2c0_cfg_func>;
> >> +             i2c_client1: i2c_client at 58 {
> >> +                     compatible = "hisilicon,i2c_client_tpa2028";
> >> +                     reg = <0x58>;
> >> +             };
> >> +     };
> >
> > [...]
> >
> > The code below looks like a direct copy of i2c-designware-platdrv.c. Is there
> > any reason you can't use that code instead?
> 
> Not understood i2c-designware-platdrv.c can be directly touched.
> Usually, there is register function, or external function call.
> 
> It would be great if we could directly add hisilicon support in
> i2c-designware-platdrv.c.
> How about adding these code to distinguish.
> 
> The concern is will platdrv.c become bigger and bigger?

The overall code size becomes much bigger when duplicating the code. It makes 
code maintenance harder.

> What in case private register have to be accessed?

Good question. I don't know what is the common convention in this case. Do you 
have such a need here?

> struct dw_i2c_data {
>         u32 accessor_flags;
>         unsigned int tx_fifo_depth;
>         unsigned int rx_fifo_depth;
> };
> 
> static struct dw_i2c_data hisilicon_data = {
>         .accessor_flags = ACCESS_32BIT,

This should be detected automatically in i2c_dw_init(). When ACCESS_16BIT is 
not set, access is 32bit wide. Doesn't it work for you?

>         .tx_fifo_depth = 16,
>         .rx_fifo_depth = 16,

These should be encoded in new device-tree properties named "tx-fifo-size", 
and "rx-fifo-size". For example, see 
Documentation/devicetree/bindings/powerpc/4xx/emac.txt.

baruch

> };
> { .compatible = "snps,hisilicon-i2c", .data = &hisilicon_data},

-- 
     http://baruch.siach.name/blog/                  ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -

[PATCH 2/2] i2c: designware: Add i2c-designware-hs

[zhangfei]

>> What in case private register have to be accessed?
>
> Good question. I don't know what is the common convention in this case. Do you
> have such a need here?

Originally, there is private register for tuning timing of clk and data.
Suppose there may some issue of catching data in clk trigger stage.
However, in the test, still not need to set timing yet.

>
>> struct dw_i2c_data {
>>          u32 accessor_flags;
>>          unsigned int tx_fifo_depth;
>>          unsigned int rx_fifo_depth;
>> };
>>
>> static struct dw_i2c_data hisilicon_data = {
>>          .accessor_flags = ACCESS_32BIT,
>
> This should be detected automatically in i2c_dw_init(). When ACCESS_16BIT is
> not set, access is 32bit wide. Doesn't it work for you?

Cool, this works, then the first patch can be ignored at all.
My bad, not check when no value of DW_IC_COMP_TYPE.

>
>>          .tx_fifo_depth = 16,
>>          .rx_fifo_depth = 16,
>
> These should be encoded in new device-tree properties named "tx-fifo-size",
> and "rx-fifo-size". For example, see
> Documentation/devicetree/bindings/powerpc/4xx/emac.txt.

OK, will add these two property to
Documentation/devicetree/bindings/i2c/i2c-designware.txt

Besides, would you mind change
subsys_initcall(dw_i2c_init_driver);
to
module_platform_driver(dw_i2c_driver);
Otherwise default pinctrl can not be auto-set, since subsys_initcall is 
too early.

Thanks