Re: [v1] dt-bindings: msm:disp: update dsi and dpu bindings

Hi,

On Tue, Feb 4, 2020 at 6:15 AM Harigovindan P harigovi@codeaurora.org wrote:

Updating bindings of dsi and dpu by adding and removing certain properties.

Signed-off-by: Harigovindan P harigovi@codeaurora.org

Changes in v1: - Adding "ahb" clock as a required property. - Adding "bus", "rot", "lut" as optional properties for sc7180 device. - Removing properties from dsi bindings that are unused. - Removing power-domain property since DSI is the child node of MDSS and it will inherit supply from its parent.

Documentation/devicetree/bindings/display/msm/dpu.txt | 7 +++++++ Documentation/devicetree/bindings/display/msm/dsi.txt | 5 ----- 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt index 551ae26..dd58472a 100644 --- a/Documentation/devicetree/bindings/display/msm/dpu.txt +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt @@ -19,6 +19,7 @@ Required properties: The following clocks are required:

• "iface"
• "bus"

• • "ahb"

This is only required for sc7180? ...or old SoCs should have had it all along too?

• "core"

• interrupts: interrupt signal from MDSS.
• interrupt-controller: identifies the node as an interrupt controller.

@@ -50,6 +51,8 @@ Required properties:

• clock-names: device clock names, must be in same order as clocks property. The following clocks are required.
  • "bus"

• For the device "qcom,sc7180-dpu":
• • "bus" - is an optional property due to architecture change.

This is a really odd way to write it for two reasons: * You're breaking up the flow of the list. * This shouldn't be listed as "optional" in sc7180 but unless there is some reason to ever provide it on sc7180. It should simply be disallowed.

Maybe instead just:

The following clocks are required. - * "bus" + * "bus" (anything other than qcom,sc7180-dpu)

We really need to get this into yaml ASAP but that'd probably be OK to tide us over.

NOTE: when converting to yaml, ideally we'll have a separate file per SoC to avoid crazy spaghetti, see commit 2a8aa18c1131 ("dt-bindings: clk: qcom: Fix self-validation, split, and clean cruft") in clk-next for an example of starting the transition to one yaml per SoC (at least for anything majorly different).

• "iface"
• "core"
• "vsync"

@@ -70,6 +73,10 @@ Optional properties:

• assigned-clocks: list of clock specifiers for clocks needing rate assignment
• assigned-clock-rates: list of clock frequencies sorted in the same order as the assigned-clocks property.

+- For the device "qcom,sc7180-dpu":

• clock-names: optional device clocks, needed for accessing LUT blocks.
• • "rot"
• • "lut"

Example:

diff --git a/Documentation/devicetree/bindings/display/msm/dsi.txt b/Documentation/devicetree/bindings/display/msm/dsi.txt index af95586..61d659a 100644 --- a/Documentation/devicetree/bindings/display/msm/dsi.txt +++ b/Documentation/devicetree/bindings/display/msm/dsi.txt @@ -8,13 +8,10 @@ Required properties:

• reg-names: The names of register regions. The following regions are required:
  • "dsi_ctrl"
• interrupts: The interrupt signal from the DSI block.

-- power-domains: Should be <&mmcc MDSS_GDSC>.

Is this supposed to be removed from all SoCs using this bindings, or just yours?

I'll also note that you left it in the "Example:" below.

• clocks: Phandles to device clocks.
• clock-names: the following clocks are required:
• • "mdp_core"
  • "iface"
  • "bus"
• • "core_mmss"

As Jeffrey pointed out, you shouldn't be removing these from old SoCs. In "drivers/gpu/drm/msm/dsi/dsi_cfg.c" you can clearly see them used. Maybe it's time for you to do the yaml conversion and handle this correctly per-SoC.

-Doug