Re: [PATCH 1/5] dt-bindings: clock: qcom: Add bindings for PDM GP_MN clock divider

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] The binding incorrectly defines and mandates generic clock configuration properties (`assigned-clocks`, `assigned-clock-rates`).
- [Medium] The compatible string `qcom,clk-gp-mnd` is too generic and lacks an SoC-specific identifier.
- [Low] Generic pinctrl properties (`pinctrl-0`, `pinctrl-names`) are marked as strictly required.
- [Low] The `pinctrl-names` property is restricted exclusively to the `"active"` state, omitting standard states or an inactive state.
--

commit 3c2905ca3a8455710ab289217cbf668b7b5c48ce
Author: Taniya Das <taniya.das@oss.qualcomm.com>

dt-bindings: clock: qcom: Add bindings for PDM GP_MN clock divider

Add device tree bindings for the Qualcomm Peripheral Web's PDM GP_MN
clock divider. The hardware generates a fractional output frequency
from a fixed input clock with a controllable duty cycle.

> diff --git a/Documentation/devicetree/bindings/clock/qcom,clk-gp-mnd.yaml b/Documentation/devicetree/bindings/clock/qcom,clk-gp-mnd.yaml
> new file mode 100644
> index 0000000000000..c1688bb3d68d8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,clk-gp-mnd.yaml

[ ... ]

> +properties:
> +  compatible:
> +    const: qcom,clk-gp-mnd

[Severity: Medium]
Should this compatible string include an SoC-specific identifier?

Devicetree guidelines usually require an SoC-specific compatible string
(e.g., "qcom,sa8775p-clk-gp-mnd") even when a generic fallback is provided.
This ensures that future hardware quirks can be handled without modifying
existing shipped device trees.

[ ... ]

> +  pinctrl-names:
> +    items:
> +      - const: active

[Severity: Low]
Does restricting this to only "active" prevent the core framework from
automatically applying the pins?

The standard pinctrl framework usually expects a "default" state to apply
the configuration upon device probe. Omitting it forces the driver to
manually apply the state. Additionally, this prevents defining a low-power
state (like "sleep" or "idle") for suspend or driver removal.

> +
> +  assigned-clocks:
> +    maxItems: 1
> +    description: Parent clock phandle used to set the input frequency.
> +
> +  assigned-clock-rates:
> +    maxItems: 1
> +    description: |
> +      Rate for the parent clock in Hz.
> +      Supported rates: 19200000, 9600000, 6400000, 4800000.

[Severity: Medium]
Should these generic clock configuration properties be explicitly defined
here?

Properties like assigned-clocks and assigned-clock-rates are common core
device tree directives. Defining them in the binding enforces software
configuration policy rather than describing the hardware.

> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +  - clock-output-names
> +  - pinctrl-0
> +  - pinctrl-names

[Severity: Low]
Is it necessary to strictly require the pinctrl properties?

Since pins can sometimes be pre-configured by earlier boot stages or
firmware, mandating them as required can reject valid hardware descriptions
unless the hardware is fundamentally non-functional without OS-level muxing.

> +  - assigned-clocks
> +  - assigned-clock-rates

[Severity: Medium]
Should generic clock configurations be mandated as strictly required?

Because these define software configuration policy, requiring them will
wrongly reject valid hardware descriptions where the bootloader has already
configured the clock or the driver relies on a default rate.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-pdm_clk_gp_mnd_v1-v1-0-1522662b6c53@oss.qualcomm.com?part=1