[PATCH] ARM: dt: tegra: ventana: define pinmux for ddc

[PATCH] ARM: dt: tegra: ventana: define pinmux for ddc

[Mark Zhang]

Define pinmux for DDC. The DDC pinmux in Ventana is 2 pins in I2C2.

Signed-off-by: Mark Zhang <markz-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 arch/arm/boot/dts/tegra20-ventana.dts |   70 ++++++++++++++++++++++++++++++---
 1 file changed, 65 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index bec8bb2..b6b3af4 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -64,11 +64,6 @@
 				nvidia,pins = "dap4";
 				nvidia,function = "dap4";
 			};
-			ddc {
-				nvidia,pins = "ddc", "owc", "spdi", "spdo",
-					"uac";
-				nvidia,function = "rsvd2";
-			};
 			dta {
 				nvidia,pins = "dta", "dtb", "dtc", "dtd", "dte";
 				nvidia,function = "vi";
@@ -248,6 +243,39 @@
 				nvidia,slew-rate-falling = <3>;
 			};
 		};
+
+		state_i2cmux_ddc: pinmux_i2cmux_ddc {
+			ddc {
+				nvidia,pins = "ddc";
+				nvidia,function = "i2c2";
+			};
+			pta {
+				nvidia,pins = "pta";
+				nvidia,function = "rsvd4";
+			};
+		};
+
+		state_i2cmux_pta: pinmux_i2cmux_pta {
+			ddc {
+				nvidia,pins = "ddc";
+				nvidia,function = "rsvd4";
+			};
+			pta {
+				nvidia,pins = "pta";
+				nvidia,function = "i2c2";
+			};
+		};
+
+		state_i2cmux_idle: pinmux_i2cmux_idle {
+			ddc {
+				nvidia,pins = "ddc";
+				nvidia,function = "rsvd4";
+			};
+			pta {
+				nvidia,pins = "pta";
+				nvidia,function = "rsvd4";
+			};
+		};
 	};
 
 	i2s@70002800 {
@@ -291,6 +319,38 @@
 		clock-frequency = <400000>;
 	};
 
+	i2cmux {
+		compatible = "i2c-mux-pinctrl";
+		#address-cells = <1>;
+		#size-cells = <0>;
+
+		i2c-parent = <&{/i2c@7000c400}>;
+
+		pinctrl-names = "ddc", "pta", "idle";
+		pinctrl-0 = <&state_i2cmux_ddc>;
+		pinctrl-1 = <&state_i2cmux_pta>;
+		pinctrl-2 = <&state_i2cmux_idle>;
+
+		i2c@0 {
+			reg = <0>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+		};
+
+		i2c@1 {
+			reg = <1>;
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			smart-battery@b {
+				compatible = "ti,bq24617", "smart-battery-1.1";
+				reg = <0xb>;
+				ti,i2c-retry-count = <2>;
+				ti,poll-retry-count = <10>;
+			};
+		};
+	};
+
 	i2c@7000c500 {
 		status = "okay";
 		clock-frequency = <400000>;
-- 
1.7.9.5

Re: [PATCH] ARM: dt: tegra: ventana: define pinmux for ddc

[Stephen Warren]

On 10/18/2012 11:58 PM, Mark Zhang wrote:
> Define pinmux for DDC. The DDC pinmux in Ventana is 2 pins in I2C2.

> +++ b/arch/arm/boot/dts/tegra20-ventana.dts

> -			ddc {
> -				nvidia,pins = "ddc", "owc", "spdi", "spdo",
> -					"uac";
> -				nvidia,function = "rsvd2";
> -			};

So that removes the entries for 5 pin groups, yet below, entries are
only added for the ddc and pta pingroups, so the other 4 pin groups
become unconfigured.

> +
> +		state_i2cmux_ddc: pinmux_i2cmux_ddc {
> +			ddc {
> +				nvidia,pins = "ddc";
> +				nvidia,function = "i2c2";
> +			};
> +			pta {
> +				nvidia,pins = "pta";
> +				nvidia,function = "rsvd4";
> +			};

Does this actually work? The pta pingroup is configured by the "hog"
pinctrl state of the pinctrl node itself, so this state should fail to
be applied since it attempts to touch the same pingroup.

> +	i2cmux {
...
> +		i2c@1 {
> +			reg = <1>;
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +
> +			smart-battery@b {
> +				compatible = "ti,bq24617", "smart-battery-1.1";
> +				reg = <0xb>;
> +				ti,i2c-retry-count = <2>;
> +				ti,poll-retry-count = <10>;
> +			};

That wasn't there before. Does that device actually exist on Ventana?
Adding it should be a separate patch if so, since this one is just about
introducing the I2C mux, not adding an SBS device. Was this all just
cut/paste from Seaboard without validation?

Re: [PATCH] ARM: dt: tegra: ventana: define pinmux for ddc

[Mark Zhang]

On 10/19/2012 11:48 PM, Stephen Warren wrote:
> On 10/18/2012 11:58 PM, Mark Zhang wrote:
>> Define pinmux for DDC. The DDC pinmux in Ventana is 2 pins in I2C2.
>
>> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
>
>> -			ddc {
>> -				nvidia,pins = "ddc", "owc", "spdi", "spdo",
>> -					"uac";
>> -				nvidia,function = "rsvd2";
>> -			};
>
> So that removes the entries for 5 pin groups, yet below, entries are
> only added for the ddc and pta pingroups, so the other 4 pin groups
> become unconfigured.
>

Right. So I think it should be changed to:

		owc {
			nvidia,pins = "owc", "spdi", "spdo", "uac";
			nvidia,function = "rsvd2";
		};

Is this right?

>> +
>> +		state_i2cmux_ddc: pinmux_i2cmux_ddc {
>> +			ddc {
>> +				nvidia,pins = "ddc";
>> +				nvidia,function = "i2c2";
>> +			};
>> +			pta {
>> +				nvidia,pins = "pta";
>> +				nvidia,function = "rsvd4";
>> +			};
>
> Does this actually work? The pta pingroup is configured by the "hog"
> pinctrl state of the pinctrl node itself, so this state should fail to
> be applied since it attempts to touch the same pingroup.
>

I know little about kernel pinctrl subsystem. After reading some docs 
and codes, I think what you mean is, in Ventana's pinmux configuration, 
pta pingroup has been defined as "hdmi" function and this can't be 
changed(non-dynamic pinmuxing).
So I want to know why we have defined pta pingroup as hdmi function?
Can we remove this definition to make the i2cmux above working?

>> +	i2cmux {
> ...
>> +		i2c@1 {
>> +			reg = <1>;
>> +			#address-cells = <1>;
>> +			#size-cells = <0>;
>> +
>> +			smart-battery@b {
>> +				compatible = "ti,bq24617", "smart-battery-1.1";
>> +				reg = <0xb>;
>> +				ti,i2c-retry-count = <2>;
>> +				ti,poll-retry-count = <10>;
>> +			};
>
> That wasn't there before. Does that device actually exist on Ventana?
> Adding it should be a separate patch if so, since this one is just about
> introducing the I2C mux, not adding an SBS device. Was this all just
> cut/paste from Seaboard without validation?
>

Yes, I didn't validate this. Actually, I don't know how to validate. I 
have no idea about SBS device. But I have checked the schematic of 
Ventana and it has this "bq24617" battery charge controller(for 
seaboard, it's bq20z75).
So I agree with you not to include this part in the patch.

Re: [PATCH] ARM: dt: tegra: ventana: define pinmux for ddc

[Stephen Warren]

On 10/22/2012 01:29 AM, Mark Zhang wrote:
> On 10/19/2012 11:48 PM, Stephen Warren wrote:
>> On 10/18/2012 11:58 PM, Mark Zhang wrote:
>>> Define pinmux for DDC. The DDC pinmux in Ventana is 2 pins in I2C2.
>>
>>> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
>>
>>> -            ddc {
>>> -                nvidia,pins = "ddc", "owc", "spdi", "spdo",
>>> -                    "uac";
>>> -                nvidia,function = "rsvd2";
>>> -            };
>>
>> So that removes the entries for 5 pin groups, yet below, entries are
>> only added for the ddc and pta pingroups, so the other 4 pin groups
>> become unconfigured.
>>
> 
> Right. So I think it should be changed to:
> 
>         owc {
>             nvidia,pins = "owc", "spdi", "spdo", "uac";
>             nvidia,function = "rsvd2";
>         };
> 
> Is this right?

Looks correct, yes.

>>> +
>>> +        state_i2cmux_ddc: pinmux_i2cmux_ddc {
>>> +            ddc {
>>> +                nvidia,pins = "ddc";
>>> +                nvidia,function = "i2c2";
>>> +            };
>>> +            pta {
>>> +                nvidia,pins = "pta";
>>> +                nvidia,function = "rsvd4";
>>> +            };
>>
>> Does this actually work? The pta pingroup is configured by the "hog"
>> pinctrl state of the pinctrl node itself, so this state should fail to
>> be applied since it attempts to touch the same pingroup.
> 
> I know little about kernel pinctrl subsystem. After reading some docs
> and codes, I think what you mean is, in Ventana's pinmux configuration,
> pta pingroup has been defined as "hdmi" function and this can't be
> changed(non-dynamic pinmuxing).
> So I want to know why we have defined pta pingroup as hdmi function?
> Can we remove this definition to make the i2cmux above working?

I don't recall why pta was defined to be HDMI. The issue isn't that this
patch changes the pinmux selection for the pta pingroup, but simply that
both the pinctrl node's state definition, and the new I2C mux node's
state definition both attempt to configure pingroup pta. The solution is
most likely to simply remove the pta configuration from the main pinctrl
node.

Re: [PATCH] ARM: dt: tegra: ventana: define pinmux for ddc

[Mark Zhang]

On 10/23/2012 01:14 AM, Stephen Warren wrote:
> On 10/22/2012 01:29 AM, Mark Zhang wrote:
>> On 10/19/2012 11:48 PM, Stephen Warren wrote:
>>> On 10/18/2012 11:58 PM, Mark Zhang wrote:
>>>> Define pinmux for DDC. The DDC pinmux in Ventana is 2 pins in I2C2.
>>>
>>>> +++ b/arch/arm/boot/dts/tegra20-ventana.dts
>>>
>>>> -            ddc {
>>>> -                nvidia,pins = "ddc", "owc", "spdi", "spdo",
>>>> -                    "uac";
>>>> -                nvidia,function = "rsvd2";
>>>> -            };
>>>
>>> So that removes the entries for 5 pin groups, yet below, entries are
>>> only added for the ddc and pta pingroups, so the other 4 pin groups
>>> become unconfigured.
>>>
>>
>> Right. So I think it should be changed to:
>>
>>          owc {
>>              nvidia,pins = "owc", "spdi", "spdo", "uac";
>>              nvidia,function = "rsvd2";
>>          };
>>
>> Is this right?
>
> Looks correct, yes.
>
>>>> +
>>>> +        state_i2cmux_ddc: pinmux_i2cmux_ddc {
>>>> +            ddc {
>>>> +                nvidia,pins = "ddc";
>>>> +                nvidia,function = "i2c2";
>>>> +            };
>>>> +            pta {
>>>> +                nvidia,pins = "pta";
>>>> +                nvidia,function = "rsvd4";
>>>> +            };
>>>
>>> Does this actually work? The pta pingroup is configured by the "hog"
>>> pinctrl state of the pinctrl node itself, so this state should fail to
>>> be applied since it attempts to touch the same pingroup.
>>
>> I know little about kernel pinctrl subsystem. After reading some docs
>> and codes, I think what you mean is, in Ventana's pinmux configuration,
>> pta pingroup has been defined as "hdmi" function and this can't be
>> changed(non-dynamic pinmuxing).
>> So I want to know why we have defined pta pingroup as hdmi function?
>> Can we remove this definition to make the i2cmux above working?
>
> I don't recall why pta was defined to be HDMI. The issue isn't that this
> patch changes the pinmux selection for the pta pingroup, but simply that
> both the pinctrl node's state definition, and the new I2C mux node's
> state definition both attempt to configure pingroup pta. The solution is
> most likely to simply remove the pta configuration from the main pinctrl
> node.
>

Understood. I tried to remove the hdmi function definition of pta 
pingroup yesterday then found HDMI can't work anymore. The EDID of HDMI 
monitor can't be fetched. After some debugging, I have found that it's 
caused by "i2c-mux-pinctrl" driver is loaded after drm driver. That 
makes drm driver can't get EDID data via this i2c mux adapter because it 
doesn't exist at that time. So I think we need to promote the load level 
of "i2c-mux-pinctrl" driver. Any other ideas?

Mark

Re: [PATCH] ARM: dt: tegra: ventana: define pinmux for ddc

[Stephen Warren]

On 10/25/2012 12:32 AM, Mark Zhang wrote:
> On 10/23/2012 01:14 AM, Stephen Warren wrote:
...
>> I don't recall why pta was defined to be HDMI. The issue isn't that this
>> patch changes the pinmux selection for the pta pingroup, but simply that
>> both the pinctrl node's state definition, and the new I2C mux node's
>> state definition both attempt to configure pingroup pta. The solution is
>> most likely to simply remove the pta configuration from the main pinctrl
>> node.
> 
> Understood. I tried to remove the hdmi function definition of pta
> pingroup yesterday then found HDMI can't work anymore. The EDID of HDMI
> monitor can't be fetched. After some debugging, I have found that it's
> caused by "i2c-mux-pinctrl" driver is loaded after drm driver. That
> makes drm driver can't get EDID data via this i2c mux adapter because it
> doesn't exist at that time. So I think we need to promote the load level
> of "i2c-mux-pinctrl" driver. Any other ideas?

(BTW, your message that I'm replying to wasn't word-wrapped correctly)

No, i2c-mux-pinctrl shouldn't need to change. It sounds like tegra-drm
isn't supporting deferred probe correctly; when it needs access to an
I2C adapter that doesn't exist, it should defer its own probe until the
I2C adapter does exist.

Re: [PATCH] ARM: dt: tegra: ventana: define pinmux for ddc

[Mark Zhang]

On 10/25/2012 11:37 PM, Stephen Warren wrote:
> On 10/25/2012 12:32 AM, Mark Zhang wrote:
>> On 10/23/2012 01:14 AM, Stephen Warren wrote:
> ...
>>> I don't recall why pta was defined to be HDMI. The issue isn't that this
>>> patch changes the pinmux selection for the pta pingroup, but simply that
>>> both the pinctrl node's state definition, and the new I2C mux node's
>>> state definition both attempt to configure pingroup pta. The solution is
>>> most likely to simply remove the pta configuration from the main pinctrl
>>> node.
>>
>> Understood. I tried to remove the hdmi function definition of pta
>> pingroup yesterday then found HDMI can't work anymore. The EDID of HDMI
>> monitor can't be fetched. After some debugging, I have found that it's
>> caused by "i2c-mux-pinctrl" driver is loaded after drm driver. That
>> makes drm driver can't get EDID data via this i2c mux adapter because it
>> doesn't exist at that time. So I think we need to promote the load level
>> of "i2c-mux-pinctrl" driver. Any other ideas?
> 
> (BTW, your message that I'm replying to wasn't word-wrapped correctly)
> 

Yes. I did some settings in my Thunderbird, how about now?

> No, i2c-mux-pinctrl shouldn't need to change. It sounds like tegra-drm
> isn't supporting deferred probe correctly; when it needs access to an
> I2C adapter that doesn't exist, it should defer its own probe until the
> I2C adapter does exist.

Okay. Yes, tegra-drm doesn't support deferred probe now. I'll talk with
Thierry and figure out how to handle this. Thanks.

> --
> To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>