Patchwork [2/4] ARM: STi: Supply I2C configuration to STiH416 SoC

login
register
mail settings
Submitter Maxime COQUELIN
Date Sept. 18, 2013, 10:01 a.m.
Message ID <1379498483-4236-3-git-send-email-maxime.coquelin@st.com>
Download mbox | patch
Permalink /patch/275636/
State Superseded
Headers show

Comments

Maxime COQUELIN - Sept. 18, 2013, 10:01 a.m.
This patch supplies I2C configuration to STiH416 SoC.

Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
---
 arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
 arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+)
Lee Jones - Sept. 18, 2013, 12:03 p.m.
> This patch supplies I2C configuration to STiH416 SoC.
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
> ---
>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>  2 files changed, 92 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi

I genuinely don't know the answer to this question, but are these
nodes identical to the ones you've just put in the stih415 DTSI file?
If so, I think it will be worth creating a stih41x DTSI rather than
duplicating lots of stuff unnecessarily.

> index 0f246c9..b29ff4b 100644
> --- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
> @@ -97,6 +97,24 @@
>  					};
>  				};
>  			};
> +
> +			sbc_i2c0 {
> +				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
> +					st,pins {
> +						sda = <&PIO4 6 ALT1 BIDIR>;
> +						scl = <&PIO4 5 ALT1 BIDIR>;
> +					};
> +				};
> +			};
> +
> +			sbc_i2c1 {
> +				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
> +					st,pins {
> +						sda = <&PIO3 2 ALT2 BIDIR>;
> +						scl = <&PIO3 1 ALT2 BIDIR>;
> +					};
> +				};
> +			};
>  		};
>  
>  		pin-controller-front {
> @@ -175,6 +193,23 @@
>  				};
>  			};
>  
> +			i2c0 {
> +				pinctrl_i2c0_default: i2c0-default {
> +					st,pins {
> +						sda = <&PIO9 3 ALT1 BIDIR>;
> +						scl = <&PIO9 2 ALT1 BIDIR>;
> +					};
> +				};
> +			};
> +
> +			i2c1 {
> +				pinctrl_i2c1_default: i2c1-default {
> +					st,pins {
> +						sda = <&PIO12 1 ALT1 BIDIR>;
> +						scl = <&PIO12 0 ALT1 BIDIR>;
> +					};
> +				};
> +			};
>  		};
>  
>  		pin-controller-rear {
> diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
> index 1a0326e..8856470 100644
> --- a/arch/arm/boot/dts/stih416.dtsi
> +++ b/arch/arm/boot/dts/stih416.dtsi
> @@ -9,6 +9,7 @@
>  #include "stih41x.dtsi"
>  #include "stih416-clock.dtsi"
>  #include "stih416-pinctrl.dtsi"
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>  / {
>  	L2: cache-controller {
>  		compatible = "arm,pl310-cache";
> @@ -92,5 +93,61 @@
>  			pinctrl-0 	= <&pinctrl_sbc_serial1>;
>  			clocks          = <&CLK_SYSIN>;
>  		};
> +
> +		i2c0: i2c@fed40000{

Same issues here. I assume most of this is copy paste.

> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfed40000 0x110>;
> +			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_S_ICN_REG_0>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_i2c0_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		i2c1: i2c@fed41000{
> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfed41000 0x110>;
> +			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_S_ICN_REG_0>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_i2c1_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		sbc_i2c0: i2c@fe540000{
> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfe540000 0x110>;
> +			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_SYSIN>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
> +
> +		sbc_i2c1: i2c@fe541000{
> +			compatible	= "st,comms-i2c";
> +			status		= "disabled";
> +			reg		= <0xfe541000 0x110>;
> +			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
> +			clocks		= <&CLK_SYSIN>;
> +			clock-frequency = <400000>;
> +			pinctrl-names	= "default";
> +			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
> +			st,glitches;
> +			st,glitch-clk	= <500>;
> +			st,glitch-dat	= <500>;
> +		};
>  	};
>  };
Maxime COQUELIN - Sept. 18, 2013, 12:46 p.m.
On 09/18/2013 02:03 PM, Lee Jones wrote:
>> This patch supplies I2C configuration to STiH416 SoC.
>>
>> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>> ---
>>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>  2 files changed, 92 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
> I genuinely don't know the answer to this question, but are these
> nodes identical to the ones you've just put in the stih415 DTSI file?
> If so, I think it will be worth creating a stih41x DTSI rather than
> duplicating lots of stuff unnecessarily.
There are close to be identical indeed.
For the clocks and pinctrl, the references names are the same, but they are
pointing on different nodes, as STiH415 and STiH416 have their own
clocks and pinctrl dtsi files.

Srini, what is opinion about this?
>
>> index 0f246c9..b29ff4b 100644
>> --- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
>> +++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>> @@ -97,6 +97,24 @@
>>  					};
>>  				};
>>  			};
>> +
>> +			sbc_i2c0 {
>> +				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
>> +					st,pins {
>> +						sda = <&PIO4 6 ALT1 BIDIR>;
>> +						scl = <&PIO4 5 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>> +
>> +			sbc_i2c1 {
>> +				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
>> +					st,pins {
>> +						sda = <&PIO3 2 ALT2 BIDIR>;
>> +						scl = <&PIO3 1 ALT2 BIDIR>;
>> +					};
>> +				};
>> +			};
>>  		};
>>  
>>  		pin-controller-front {
>> @@ -175,6 +193,23 @@
>>  				};
>>  			};
>>  
>> +			i2c0 {
>> +				pinctrl_i2c0_default: i2c0-default {
>> +					st,pins {
>> +						sda = <&PIO9 3 ALT1 BIDIR>;
>> +						scl = <&PIO9 2 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>> +
>> +			i2c1 {
>> +				pinctrl_i2c1_default: i2c1-default {
>> +					st,pins {
>> +						sda = <&PIO12 1 ALT1 BIDIR>;
>> +						scl = <&PIO12 0 ALT1 BIDIR>;
>> +					};
>> +				};
>> +			};
>>  		};
>>  
>>  		pin-controller-rear {
>> diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
>> index 1a0326e..8856470 100644
>> --- a/arch/arm/boot/dts/stih416.dtsi
>> +++ b/arch/arm/boot/dts/stih416.dtsi
>> @@ -9,6 +9,7 @@
>>  #include "stih41x.dtsi"
>>  #include "stih416-clock.dtsi"
>>  #include "stih416-pinctrl.dtsi"
>> +#include <dt-bindings/interrupt-controller/arm-gic.h>
>>  / {
>>  	L2: cache-controller {
>>  		compatible = "arm,pl310-cache";
>> @@ -92,5 +93,61 @@
>>  			pinctrl-0 	= <&pinctrl_sbc_serial1>;
>>  			clocks          = <&CLK_SYSIN>;
>>  		};
>> +
>> +		i2c0: i2c@fed40000{
> Same issues here. I assume most of this is copy paste.
Yes indeed. This will be fixed in next version.
>
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfed40000 0x110>;
>> +			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_S_ICN_REG_0>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_i2c0_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		i2c1: i2c@fed41000{
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfed41000 0x110>;
>> +			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_S_ICN_REG_0>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_i2c1_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		sbc_i2c0: i2c@fe540000{
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfe540000 0x110>;
>> +			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_SYSIN>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>> +
>> +		sbc_i2c1: i2c@fe541000{
>> +			compatible	= "st,comms-i2c";
>> +			status		= "disabled";
>> +			reg		= <0xfe541000 0x110>;
>> +			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
>> +			clocks		= <&CLK_SYSIN>;
>> +			clock-frequency = <400000>;
>> +			pinctrl-names	= "default";
>> +			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
>> +			st,glitches;
>> +			st,glitch-clk	= <500>;
>> +			st,glitch-dat	= <500>;
>> +		};
>>  	};
>>  };
Thanks,
Maxime

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas KANDAGATLA - Sept. 18, 2013, 12:57 p.m.
On 18/09/13 13:46, Maxime COQUELIN wrote:
> On 09/18/2013 02:03 PM, Lee Jones wrote:
>>> >> This patch supplies I2C configuration to STiH416 SoC.
>>> >>
>>> >> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>>> >> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>>> >> ---
>>> >>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>> >>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>> >>  2 files changed, 92 insertions(+)
>>> >>
>>> >> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>> > I genuinely don't know the answer to this question, but are these
>> > nodes identical to the ones you've just put in the stih415 DTSI file?
>> > If so, I think it will be worth creating a stih41x DTSI rather than
>> > duplicating lots of stuff unnecessarily.
> There are close to be identical indeed.
> For the clocks and pinctrl, the references names are the same, but they are
> pointing on different nodes, as STiH415 and STiH416 have their own
> clocks and pinctrl dtsi files.
> 
> Srini, what is opinion about this?

There is already a stih41x.dtsi file, but I don't think it is the right
place for the pinctrl nodes there.

Am not OK with the idea of common pinctrl nodes for STiH415 and STiH416
for two reasons.

1> If we common up the pinctrl nodes, it will be very difficult to
accommodate new pinctrls layout which is not guaranteed to be in same
layout in future SOCs.

2> The retiming values in the pinctrl nodes tend to change as per SOC,
so it will be difficult to manage it if we common it up.

Am sure we can come up with a dt layout which can reduce duplication,
but we have to be careful here not to lose the flexiblity to accommodate
new picntrl layouts, new retimings values based on SOC.


thanks,
srini

>> >

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime COQUELIN - Sept. 19, 2013, 7:16 a.m.
Hi Srini,

On 09/18/2013 03:17 PM, Srinivas KANDAGATLA wrote:
> On 18/09/13 13:46, Maxime COQUELIN wrote:
>> On 09/18/2013 02:03 PM, Lee Jones wrote:
>>>>>> This patch supplies I2C configuration to STiH416 SoC.
>>>>>>
>>>>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>>>>>> ---
>>>>>>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>>>>>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>>>>>  2 files changed, 92 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>>>> I genuinely don't know the answer to this question, but are these
>>>> nodes identical to the ones you've just put in the stih415 DTSI file?
>>>> If so, I think it will be worth creating a stih41x DTSI rather than
>>>> duplicating lots of stuff unnecessarily.
>> There are close to be identical indeed.
>> For the clocks and pinctrl, the references names are the same, but they are
>> pointing on different nodes, as STiH415 and STiH416 have their own
>> clocks and pinctrl dtsi files.
>>
>> Srini, what is opinion about this?
> There is already a stih41x.dtsi file, but I don't think it is the right
> place for the pinctrl nodes there.
>
> Am not OK with the idea of common pinctrl nodes for STiH415 and STiH416
> for two reasons.
>
> 1> If we common up the pinctrl nodes, it will be very difficult to
> accommodate new pinctrls layout which is not guaranteed to be in same
> layout in future SOCs.
>
> 2> The retiming values in the pinctrl nodes tend to change as per SOC,
> so it will be difficult to manage it if we common it up.
>
> Am sure we can come up with a dt layout which can reduce duplication,
> but we have to be careful here not to lose the flexiblity to accommodate
> new picntrl layouts, new retimings values based on SOC.
Ok. What do you think of declaring the i2c nodes inside the stih41x.dtsi
file,
and overload them with the pinctrl and clock properties in the stih416
and stih415 dtsi files?

Regards,
Maxime
>
>
> thanks,
> srini
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Srinivas KANDAGATLA - Sept. 19, 2013, 12:59 p.m.
On 19/09/13 08:16, Maxime COQUELIN wrote:
> Hi Srini,
> 
> On 09/18/2013 03:17 PM, Srinivas KANDAGATLA wrote:
>> On 18/09/13 13:46, Maxime COQUELIN wrote:
>>> On 09/18/2013 02:03 PM, Lee Jones wrote:
>>>>>>> This patch supplies I2C configuration to STiH416 SoC.
>>>>>>>
>>>>>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>>>>>>> ---
>>>>>>>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>>>>>>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>>>>>>  2 files changed, 92 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>>>>> I genuinely don't know the answer to this question, but are these
>>>>> nodes identical to the ones you've just put in the stih415 DTSI file?
>>>>> If so, I think it will be worth creating a stih41x DTSI rather than
>>>>> duplicating lots of stuff unnecessarily.
>>> There are close to be identical indeed.
>>> For the clocks and pinctrl, the references names are the same, but they are
>>> pointing on different nodes, as STiH415 and STiH416 have their own
>>> clocks and pinctrl dtsi files.
>>>
>>> Srini, what is opinion about this?
>> There is already a stih41x.dtsi file, but I don't think it is the right
>> place for the pinctrl nodes there.
>>
>> Am not OK with the idea of common pinctrl nodes for STiH415 and STiH416
>> for two reasons.
>>
>> 1> If we common up the pinctrl nodes, it will be very difficult to
>> accommodate new pinctrls layout which is not guaranteed to be in same
>> layout in future SOCs.
>>
>> 2> The retiming values in the pinctrl nodes tend to change as per SOC,
>> so it will be difficult to manage it if we common it up.
>>
>> Am sure we can come up with a dt layout which can reduce duplication,
>> but we have to be careful here not to lose the flexiblity to accommodate
>> new picntrl layouts, new retimings values based on SOC.
> Ok. What do you think of declaring the i2c nodes inside the stih41x.dtsi
> file,
> and overload them with the pinctrl and clock properties in the stih416
> and stih415 dtsi files?
Am not very comfortable with this idea.

As there is no guarantee that the interrupt number/memory map and the
i2c numbering will be same in future SOCs or other IPs.

You might be already aware that the number of i2cs on each SOC are
different as example on STiH415 we have 10 SSCs and on STiH416 we have
11 SSCs. So, At what point you decide that which devices/IPs should be
in stih41x and which should in stih415/Stih416?

Each i2c node will save around 5 lines if we common it up, but if the
interrupt number or map changes this difference will be negligible.

Common up at this level and mixing un-common ones in stih415.dtsi or
stih416.dtsi will add confusion to readers as the information is split
at multiple places.

IMO the common up idea sounds good but reduces the readability and has
no effect on final dtb size.

Thanks,
srini


> 
> Regards,
> Maxime
>>
>>
>> thanks,
>> srini
>>
>>
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Maxime COQUELIN - Sept. 19, 2013, 3:22 p.m.
On 09/19/2013 03:01 PM, Srinivas KANDAGATLA wrote:
> On 19/09/13 08:16, Maxime COQUELIN wrote:
>> Hi Srini,
>>
>> On 09/18/2013 03:17 PM, Srinivas KANDAGATLA wrote:
>>> On 18/09/13 13:46, Maxime COQUELIN wrote:
>>>> On 09/18/2013 02:03 PM, Lee Jones wrote:
>>>>>>>> This patch supplies I2C configuration to STiH416 SoC.
>>>>>>>>
>>>>>>>> Cc: Srinivas Kandagatla <srinivas.kandagatla@st.com>
>>>>>>>> Signed-off-by: Maxime Coquelin <maxime.coquelin@st.com>
>>>>>>>> ---
>>>>>>>>  arch/arm/boot/dts/stih416-pinctrl.dtsi |   35 ++++++++++++++++++++
>>>>>>>>  arch/arm/boot/dts/stih416.dtsi         |   57 ++++++++++++++++++++++++++++++++
>>>>>>>>  2 files changed, 92 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
>>>>>> I genuinely don't know the answer to this question, but are these
>>>>>> nodes identical to the ones you've just put in the stih415 DTSI file?
>>>>>> If so, I think it will be worth creating a stih41x DTSI rather than
>>>>>> duplicating lots of stuff unnecessarily.
>>>> There are close to be identical indeed.
>>>> For the clocks and pinctrl, the references names are the same, but they are
>>>> pointing on different nodes, as STiH415 and STiH416 have their own
>>>> clocks and pinctrl dtsi files.
>>>>
>>>> Srini, what is opinion about this?
>>> There is already a stih41x.dtsi file, but I don't think it is the right
>>> place for the pinctrl nodes there.
>>>
>>> Am not OK with the idea of common pinctrl nodes for STiH415 and STiH416
>>> for two reasons.
>>>
>>> 1> If we common up the pinctrl nodes, it will be very difficult to
>>> accommodate new pinctrls layout which is not guaranteed to be in same
>>> layout in future SOCs.
>>>
>>> 2> The retiming values in the pinctrl nodes tend to change as per SOC,
>>> so it will be difficult to manage it if we common it up.
>>>
>>> Am sure we can come up with a dt layout which can reduce duplication,
>>> but we have to be careful here not to lose the flexiblity to accommodate
>>> new picntrl layouts, new retimings values based on SOC.
>> Ok. What do you think of declaring the i2c nodes inside the stih41x.dtsi
>> file,
>> and overload them with the pinctrl and clock properties in the stih416
>> and stih415 dtsi files?
> Am not very comfortable with this idea.
>
> As there is no guarantee that the interrupt number/memory map and the
> i2c numbering will be same in future SOCs or other IPs.
>
> You might be already aware that the number of i2cs on each SOC are
> different as example on STiH415 we have 10 SSCs and on STiH416 we have
> 11 SSCs. So, At what point you decide that which devices/IPs should be
> in stih41x and which should in stih415/Stih416?
Yes, I know there is one more SSC on STiH416.

On one hand, this could add some confusion. But on the other hand,
someone who will need to activate a SSP will know which one he has
to activate.

>
> Each i2c node will save around 5 lines if we common it up, but if the
> interrupt number or map changes this difference will be negligible.
>
> Common up at this level and mixing un-common ones in stih415.dtsi or
> stih416.dtsi will add confusion to readers as the information is split
> at multiple places.
I agree it will be messy if one part of the node declared at one place,
and the rest at another place.
>
> IMO the common up idea sounds good but reduces the readability and has
> no effect on final dtb size.

Fair enough. Lee, are you ok with keeping it as is?

Thanks,
Maxime
>
> Thanks,
> srini
>
>
>> Regards,
>> Maxime
>>>
>>> thanks,
>>> srini
>>>
>>>
>>
>>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones - Sept. 19, 2013, 3:32 p.m.
> > Am not very comfortable with this idea.
> >
> > As there is no guarantee that the interrupt number/memory map and the
> > i2c numbering will be same in future SOCs or other IPs.
> >
> > You might be already aware that the number of i2cs on each SOC are
> > different as example on STiH415 we have 10 SSCs and on STiH416 we have
> > 11 SSCs. So, At what point you decide that which devices/IPs should be
> > in stih41x and which should in stih415/Stih416?
> Yes, I know there is one more SSC on STiH416.
> 
> On one hand, this could add some confusion. But on the other hand,
> someone who will need to activate a SSP will know which one he has
> to activate.
> 
> > Each i2c node will save around 5 lines if we common it up, but if the
> > interrupt number or map changes this difference will be negligible.
> >
> > Common up at this level and mixing un-common ones in stih415.dtsi or
> > stih416.dtsi will add confusion to readers as the information is split
> > at multiple places.
> I agree it will be messy if one part of the node declared at one place,
> and the rest at another place.
> >
> > IMO the common up idea sounds good but reduces the readability and has
> > no effect on final dtb size.
> 
> Fair enough. Lee, are you ok with keeping it as is?

To be honest I haven't taken a look at the layout of the dts[i] files
yet, so I can't really comment. Srini knows then better than anyone,
so if he says it doesn't make sense, then I'm happy to take his word
for it.

Patch

diff --git a/arch/arm/boot/dts/stih416-pinctrl.dtsi b/arch/arm/boot/dts/stih416-pinctrl.dtsi
index 0f246c9..b29ff4b 100644
--- a/arch/arm/boot/dts/stih416-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stih416-pinctrl.dtsi
@@ -97,6 +97,24 @@ 
 					};
 				};
 			};
+
+			sbc_i2c0 {
+				pinctrl_sbc_i2c0_default: sbc_i2c0-default {
+					st,pins {
+						sda = <&PIO4 6 ALT1 BIDIR>;
+						scl = <&PIO4 5 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			sbc_i2c1 {
+				pinctrl_sbc_i2c1_default: sbc_i2c1-default {
+					st,pins {
+						sda = <&PIO3 2 ALT2 BIDIR>;
+						scl = <&PIO3 1 ALT2 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-front {
@@ -175,6 +193,23 @@ 
 				};
 			};
 
+			i2c0 {
+				pinctrl_i2c0_default: i2c0-default {
+					st,pins {
+						sda = <&PIO9 3 ALT1 BIDIR>;
+						scl = <&PIO9 2 ALT1 BIDIR>;
+					};
+				};
+			};
+
+			i2c1 {
+				pinctrl_i2c1_default: i2c1-default {
+					st,pins {
+						sda = <&PIO12 1 ALT1 BIDIR>;
+						scl = <&PIO12 0 ALT1 BIDIR>;
+					};
+				};
+			};
 		};
 
 		pin-controller-rear {
diff --git a/arch/arm/boot/dts/stih416.dtsi b/arch/arm/boot/dts/stih416.dtsi
index 1a0326e..8856470 100644
--- a/arch/arm/boot/dts/stih416.dtsi
+++ b/arch/arm/boot/dts/stih416.dtsi
@@ -9,6 +9,7 @@ 
 #include "stih41x.dtsi"
 #include "stih416-clock.dtsi"
 #include "stih416-pinctrl.dtsi"
+#include <dt-bindings/interrupt-controller/arm-gic.h>
 / {
 	L2: cache-controller {
 		compatible = "arm,pl310-cache";
@@ -92,5 +93,61 @@ 
 			pinctrl-0 	= <&pinctrl_sbc_serial1>;
 			clocks          = <&CLK_SYSIN>;
 		};
+
+		i2c0: i2c@fed40000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfed40000 0x110>;
+			interrupts	=  <GIC_SPI 187 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_S_ICN_REG_0>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c0_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		i2c1: i2c@fed41000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfed41000 0x110>;
+			interrupts	=  <GIC_SPI 188 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_S_ICN_REG_0>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_i2c1_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		sbc_i2c0: i2c@fe540000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfe540000 0x110>;
+			interrupts	=  <GIC_SPI 206 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_SYSIN>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c0_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
+
+		sbc_i2c1: i2c@fe541000{
+			compatible	= "st,comms-i2c";
+			status		= "disabled";
+			reg		= <0xfe541000 0x110>;
+			interrupts	=  <GIC_SPI 207 IRQ_TYPE_EDGE_RISING>;
+			clocks		= <&CLK_SYSIN>;
+			clock-frequency = <400000>;
+			pinctrl-names	= "default";
+			pinctrl-0	= <&pinctrl_sbc_i2c1_default>;
+			st,glitches;
+			st,glitch-clk	= <500>;
+			st,glitch-dat	= <500>;
+		};
 	};
 };