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

[Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>]

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.

Regards,
Bjorn