Re: [PATCH 1/2] dt-bindings: clock: qcom: Document the SM7250 display clock controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: clock: qcom: duplicate SM7150 DISPCC schema and header for SM7250
- [Medium] dt-bindings: clock: qcom: missing #power-domain-cells in sm7250-dispcc required
--

commit 38f912b989d9f04661892eb5bd0bbd2206b4f767
Author: Ishu Brar <patches@brar.si>

dt-bindings: clock: qcom: Document the SM7250 display clock controller

This commit adds the Device Tree binding schema and dt-bindings header for
the Qualcomm SM7250 display clock controller (DISPCC).

> diff --git a/Documentation/devicetree/bindings/clock/qcom,sm7250-dispcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sm7250-dispcc.yaml
> new file mode 100644
> index 0000000000000..684aaf7bb6b48
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,sm7250-dispcc.yaml
> @@ -0,0 +1,73 @@
[ ... ]
> +properties:
> +  compatible:
> +    const: qcom,sm7250-dispcc
> +
> +  clocks:
> +    items:
> +      - description: Board XO source
> +      - description: Board Always On XO source
> +      - description: GPLL0 source from GCC
> +      - description: Sleep clock source
> +      - description: Byte clock from MDSS DSI PHY0
> +      - description: Pixel clock from MDSS DSI PHY0
> +      - description: Byte clock from MDSS DSI PHY1
> +      - description: Pixel clock from MDSS DSI PHY1
> +      - description: Link clock from DP PHY
> +      - description: VCO DIV clock from DP PHY
> +
> +  power-domains:
> +    maxItems: 1
> +    description:
> +      CX power domain.
> +
> +required:
> +  - compatible
> +  - clocks
> +  - power-domains
> +
> +allOf:
> +  - $ref: qcom,gcc.yaml#

[Severity: Medium]
Should #power-domain-cells also be listed under required in
Documentation/devicetree/bindings/clock/qcom,sm7250-dispcc.yaml?

The SM7250 DISPCC provides the MDSS_GDSC power domain, and the example node
includes #power-domain-cells = <1>.

However, since commit b0ef3434da07 ("dt-bindings: clock: qcom,gcc: sort out
power-domains support"), Documentation/devicetree/bindings/clock/qcom,gcc.yaml
only lists reg, #clock-cells, and #reset-cells under required:

  required:
    - reg
    - '#clock-cells'
    - '#reset-cells'

Without '#power-domain-cells' in required here, a qcom,sm7250-dispcc device
tree node that omits #power-domain-cells will pass dt_binding_check
validation and fail at runtime when consumers reference MDSS_GDSC.

[ ... ]
> diff --git a/include/dt-bindings/clock/qcom,sm7250-dispcc.h b/include/dt-bindings/clock/qcom,sm7250-dispcc.h
> new file mode 100644
> index 0000000000000..907f4122cb19b
> --- /dev/null
> +++ b/include/dt-bindings/clock/qcom,sm7250-dispcc.h
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (c) 2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2024, Danila Tikhonov <danila@jiaxyga.com>
> + * Copyright (c) 2024, David Wronek <david@mainlining.org>
> + */
> +
> +#ifndef _DT_BINDINGS_CLK_QCOM_DISPCC_SM7250_H
> +#define _DT_BINDINGS_CLK_QCOM_DISPCC_SM7250_H
> +
> +/* DISPCC clock registers */
> +#define DISPCC_PLL0				0
> +#define DISPCC_MDSS_AHB_CLK			1

[Severity: Medium]
Could qcom,sm7250-dispcc be added to the existing
Documentation/devicetree/bindings/clock/qcom,sm7150-dispcc.yaml schema and
share include/dt-bindings/clock/qcom,sm7150-dispcc.h instead of duplicating
both files?

The commit message notes:

  The header and the binding are derived from the SM7150 ones: the register
  layout is the same, but the controller takes GPLL0 from the GCC as an
  extra parent and has its own PLL.

However, Documentation/devicetree/bindings/clock/qcom,sm7150-dispcc.yaml
already defines the exact same properties, including "GPLL0 source from GCC"
(<&gcc GCC_DISP_GPLL0_CLK_SRC>) as the third clock item, the same 10 input
clocks in the same order, and the same CX power domain:

  properties:
    compatible:
      const: qcom,sm7150-dispcc

    clocks:
      items:
        - description: Board XO source
        - description: Board Always On XO source
        - description: GPLL0 source from GCC
        - description: Sleep clock source
        - description: Byte clock from MDSS DSI PHY0
        - description: Pixel clock from MDSS DSI PHY0
        - description: Byte clock from MDSS DSI PHY1
        - description: Pixel clock from MDSS DSI PHY1
        - description: Link clock from DP PHY
        - description: VCO DIV clock from DP PHY

Likewise, include/dt-bindings/clock/qcom,sm7150-dispcc.h already defines
DISPCC_PLL0 as 0 along with the exact same 43 clock indices,
DISPCC_MDSS_CORE_BCR, and MDSS_GDSC, making
include/dt-bindings/clock/qcom,sm7250-dispcc.h a verbatim copy apart from
the header guard.

Similar Qualcomm clock bindings have been consolidated into existing schemas
(for example, commits 0e094dd19e04 and 67044b2df840 merging SM7150 and
SM7250 GCC into qcom,sm6375-gcc.yaml, and commit c984dde8f3f8 dropping the
duplicate SM8650 DISPCC schema in favor of qcom,sm8550-dispcc.yaml).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261023-dispcc-sm7250-v1-0-07bdefb99801@brar.si?part=1