Re: [PATCH v1] arm64: dts: qcom: msm8998: Add i2c5 pins

[Marc Gonzalez <marc.w.gonzalez@xxxxxxx>]

On 02/05/2019 17:12, Bjorn Andersson wrote:

> On Mon 29 Apr 01:38 PDT 2019, Marc Gonzalez wrote:
> 
>> On 27/04/2019 06:51, Bjorn Andersson wrote:
>>
>>> On Thu 25 Apr 09:06 PDT 2019, Marc Gonzalez wrote:
>>>
>>>> Downstream source:
>>>> https://source.codeaurora.org/quic/la/kernel/msm-4.4/tree/arch/arm/boot/dts/qcom/msm8998-pinctrl.dtsi?h=LE.UM.1.3.r3.25#n165
>>>>
>>>> Signed-off-by: Marc Gonzalez <marc.w.gonzalez@xxxxxxx>
>>>> ---
>>>>  arch/arm64/boot/dts/qcom/msm8998-pins.dtsi | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> index 6db70acd38ee..d0a95c70d1e7 100644
>>>> --- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> +++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>>>> @@ -2,6 +2,13 @@
>>>>  /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
>>>>  
>>>>  &tlmm {
>>>> +	i2c5_default: i2c5_default {
>>>> +		pins = "gpio87", "gpio88";
>>>> +		function = "blsp_i2c5";
>>>> +		drive-strength = <2>;
>>>> +		bias-disable;
>>>> +	};
>>>
>>> You need to reference this node for it to make a difference.
>>
>> Right. I do have a local board file referencing i2c5_default, which I plan
>> to submit at some point. It contains:
>>
>> &blsp1_i2c5 {
>> 	status = "ok";
>> 	clock-frequency = <100000>;
>> 	pinctrl-names = "default";
>> 	pinctrl-0 = <&i2c5_default>;
>> };
>>
>>> Also the drive-strength and bias are board specific, so please move this
>>> to your board dts (and reference the node).
>>
>> Wait... Are you saying there should be no drive-strength nor bias definitions
>> inside msm8998-pins.dtsi?
>>
>> $ grep -c 'strength\|bias' arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
>> 18
>>
>> Why are the SDHC pins different than the I2C pins?
>>
>> i2c5 is "tied" to gpio87 and gpio88. Could my board designer "reassign"
>> these pins to a different HW block? Or is that immutable?
>>
> 
> Right, so it makes a lot of sense to have a node in msm8998.dtsi that
> says that if i2c5 is probed then the associated pinmux should be set up.
> 
> But the pinconf (drive-strenght, internal vs external bias) are board
> specific, so this part better go in the board.dts.
> 
> 
> On sdm845 we put a node with pinmux in the platform.dtsi and then in the
> board we extend this node with the electrical properties of the board.
> This works out pretty well, but we haven't gone back and updated the
> older platforms/boards yet.

Wow, I had completely lost track of this thread...

OK, I think what you had in mind is the following:
(Please confirm before I spin a v2)

diff --git a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
index f09f3e03f708..9cd1f96dc3c8 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-mtp.dtsi
@@ -27,6 +27,18 @@
 	status = "okay";
 };
 
+&blsp1_i2c5 {
+	status = "ok";
+	clock-frequency = <100000>; /*** NOT SURE... This depends on which devices are on the I2C bus? ***/
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c5_default>;
+};
+
+&i2c5_default {
+	drive-strength = <2>;
+	bias-disable;
+};
+
 &qusb2phy {
 	status = "okay";
 
diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
index 6db70acd38ee..dad175a52d03 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
@@ -2,6 +2,11 @@
 /* Copyright (c) 2018, The Linux Foundation. All rights reserved. */
 
 &tlmm {
+	i2c5_default: i2c5-default {
+		pins = "gpio87", "gpio88";
+		function = "blsp_i2c5";
+	};
+
 	sdc2_clk_on: sdc2_clk_on {
 		config {
 			pins = "sdc2_clk";




Well, except that there don't seem to be any devices on the i2c5 bus
on the mediabox...

# i2cdetect -r 0
i2cdetect: WARNING! This program can confuse your I2C bus
Continue? [y/N] y
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --

But there are on several on my batfish board:

# i2cdetect -r 0
i2cdetect: WARNING! This program can confuse your I2C bus
Continue? [y/N] y
     0  1  2  3  4  5  6  7  8  9  a  b  c  d  e  f
00:          -- -- -- -- -- -- -- -- -- -- -- -- --
10: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
20: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
30: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
40: -- -- -- -- 44 -- -- 47 -- -- -- -- -- -- -- --
50: -- -- -- -- -- -- -- -- -- -- -- -- -- -- -- --
60: -- -- -- -- -- -- -- -- 68 -- -- -- -- -- -- --
70: -- -- -- -- -- -- -- --


Can I submit the arch/arm64/boot/dts/qcom/msm8998-pins.dtsi alone?

Regards.