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

Re: [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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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@fcb08000 {
> +		compatible = "hs,designware-i2c";

A few comments on this one:

1. You should Cc devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.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@fcb08000 {
> +		status = "ok";

The convention is to use "okay".

> +		pinctrl-names = "default";
> +		pinctrl-0 = <&i2c0_pmx_func &i2c0_cfg_func>;
> +		i2c_client1: i2c_client@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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [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@fcb08000 {
>> +             compatible = "hs,designware-i2c";
>
> A few comments on this one:
>
> 1. You should Cc devicetree-discuss@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@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@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},

Re: [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@fcb08000 {
> >> +             compatible = "hs,designware-i2c";
> >
> > A few comments on this one:
> >
> > 1. You should Cc devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.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@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@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-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [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