[PATCH v4 2/3] arm64: dts: sdm845: Add dsi pinctrl nodes

[Doug Anderson]

Hi (sending again since I screwed up my previous reply),

On Fri, Nov 2, 2018 at 3:26 PM Jeykumar Sankaran <jsanka at codeaurora.org> wrote:
>
> Add dsi active/suspend pinctrl nodes to sdm845 SoC dts.
>
> Changes in v4:
>         - patch introduced in the series
>
> Signed-off-by: Jeykumar Sankaran <jsanka at codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 5728b4c..35df5d2 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -822,6 +822,20 @@
>                         interrupt-controller;
>                         #interrupt-cells = <2>;
>
> +                       dpu_dsi_active: dpu-dsi-active {
> +                               pinmux {
> +                                       pins = "gpio6", "gpio52";
> +                                       function = "gpio";
> +                               };
> +                       };
> +
> +                       dpu_dsi_suspend: dpu-dsi-suspend {
> +                               pinmux {
> +                                       pins = "gpio6", "gpio52";
> +                                       function = "gpio";
> +                               };
> +                       };
> +

Ugh, I should have noticed this in my previous reply.  Sorry!

...looking closer I see that these two pins are MTP-specific.  They
belong fully in the MTP device tree file.  Other sdm845 boards won't
necessarily have the same functions on the same pins so they don't
belong here in the SoC file.

Also as a note: once you move them there they should no longer go in
the section "PINCTRL - additions to nodes defined in sdm845.dtsi".
They should go under the tlmm in the section "PINCTRL - board-specific
pinctrl".  Please make sure they are alphabetical there.

Since these are board specific, node names should be based on the
schematic name.  ...and in this case since they are two distinct named
pins I'd probably have a separate node for each one.  So I think you
want something like this in the mtp file: (untested)

disp_mode_sel_active: disp-mode-sel-active {
  pinmux {
    pins = "gpio52";
    function = "gpio";
  };
  pinconf {
    pins = "gpio52";
    drive-strength = <8>;
    bias-disable;
};

disp_mode_sel_sleep: disp-mode-sel-sleep {
  pinmux {
    pins = "gpio52";
    function = "gpio";
  };
  pinconf {
    pins = "gpio52";
    drive-strength = <2>;
    bias-pull-down;
};

...and then one for gpio6:

lcd0_reset_n_active: lcd0-reset-n-active {
  ...
};

---

I'm kinda curious if the sleep stuff is all truly necessary though.
Specifically I don't _think_ that the pulls are affected by the drive
strength.  So either we continue driving this pin while we're sleeping
in which case the drive strength matters and the pull doesn't.  ...or
we aren't driving it in sleep and the pull might matter but the drive
strength doesn't.

I definitely see a lot of this cruft coming from the Qualcomm
downstream without anyone who can explain to me that it's useful.  It
seems like everyone just blindly copies it from someone else.  As
further evidence that this isn't currently doing anything, as far as I
can tell the v10 panel driver for this panel doesn't actually even
select the suspend state.

Maybe we can just drop the whole "active" vs. "sleep" for now and we
can introduce it later when we show that it's useful for something.
Then we can confirm if it's the drive strength that's useful or the
pull down and we can also confirm that we don't end up going to sleep
with the pin being driven in the opposite direction of the pull.

Maybe +Bjorn or +Stephen has a different opinion though...

-Doug