Re: Pinctrl DT for msm8998

[Bjorn Andersson <bjorn.andersson@xxxxxxxxxx>]

On Wed 24 Apr 08:19 PDT 2019, Marc Gonzalez wrote:

> 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>;
> };
> 

Per drivers/base/pinctrl.c as a device is about to be probed the driver
framework will look for a pinctrl state named "init", then "default" and
select this.

Beyond that point the driver itself can choose to go to additional
states. There exists standard helpers for "default", "sleep" and "idle"
(pinctrl_pm_select_*_state() in drivers/pinctrl/core.c, but the driver
could make up their own names.


As we don't yet support hitting suspend or the lower sleep states most
of our drivers doesn't deal with anything past getting the default set,
so for now you'll be good.

As Linus says, once the drivers starts to properly deal with going to
"sleep" and then back to "default", the lack of sleep would mean that
the pinctrl_pm_select_sleep_state() returns 0, without changing the
current state and then on resume pinctrl_pm_select_default_state() would
be a no-op and we would lack the proper state (unless we deal with this
in the pinctrl driver itself perhaps).

So for now you're fine with just having the default state and ignore the
"sleep", but at some point we will actually reach this and your old DT
might start malfunction. (Although having states there that was never
tested is no guarantee that we won't have that issue anyways...)

Regards,
Bjorn