Pinctrl DT for msm8998

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

Hello,

linusw wrote:

> Some drivers prefer to handle individual pins by name, such as msm.
> Some drivers, especially those with hardware tailored to handle pin
> muxing groupwise, prefer to handle them by function group name. In
> the case of msm, every group consists of one pin and these groups are
> all named "gpioNN". What the pin control subsystem does is assign
> functions to 1 or several groups of pins. In the msm case as there is
> just one pin in every group, this becomes a bit confusing, it is easy
> to mix up what is a pin and what is a group. The pin multiplexing is
> done on groups while the pin configuration is done on individual
> pins. As to why there is one-to-one pin to group name mapping in the
> msm driver you need to ask bamse, for the pin control subsystem this
> is just some string. I suppose there was a good reason to set it up
> this way on msm. I think the msm may be set up this way because the
> pins can be configured in so many ways that it is hard to come up
> with natural groups that map to physical use cases (these are often
> enumerable on other platforms). In many systems the driver authors
> are restricted to how groups can be activated with functions, see eg
> pinctrl-gemini.c, in many other cases the driver author tries to
> half-guess the groups based on use cases that makes sense.

bamse wrote:

> You don't have to specify pinconf/pinmux in different subnodes
> anymore, that changed a few years back. For the blsp you typically
> want subnodes for each pin though, because they have different
> configuration...but you don't need to split e.g gpio88 in a mux and
> conf. Or per your production example, you don't need any subnodes at
> all...just put the properties in i2c_5_{active,sleep} directly. And
> as each gpioXX is configurable we need to define how we want each one
> to be configured...and we need to list all the configureables in the
> driver. In the qcom platform you can mux i2c on gpio87 at the same
> time as running gpio88 as gpio. Not that it would make sense in this
> case, but there are other such cases where we need the control of
> each configurable item.


diff --git a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
index 6db70acd38ee..bc1a1a4081da 100644
--- a/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8998-pins.dtsi
@@ -75,4 +75,18 @@
                        drive-strength = <2>;   /* 2 mA */
                };
        };
+
+       i2c5_default: i2c5_default {
+               pins = "gpio87", "gpio88";
+               function = "blsp_i2c5";
+               drive-strength = <2>;
+               bias-disable;
+       };
+
+       i2c5_sleep: i2c5_sleep {
+               pins = "gpio87", "gpio88";
+               function = "blsp_i2c5";
+               drive-strength = <2>;
+               bias-disable;
+       };
 };


Note that the two nodes are identical. Do I still need both then?


&blsp1_i2c5 {
	status = "ok";
	clock-frequency = <100000>;
	pinctrl-names = "default", "sleep";
	pinctrl-0 = <&i2c5_default>;
	pinctrl-1 = <&i2c5_sleep>;
};

Couldn't I just change the above to:

&blsp1_i2c5 {
	status = "ok";
	clock-frequency = <100000>;
	pinctrl-names = "default";
	pinctrl-0 = <&i2c5_default>;
};


Regards.