Re: [PATCH v2 01/12] 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 3 potential issue(s) to consider:
- [Medium] dt-bindings: clock: qcom: invalid required assigned-clocks in clk-gp-mnd
- [Low] dt-bindings: clock: qcom: non-standard schema filename and clock-names suffixes
- [Low] dt-bindings: clock: qcom: unused gp_mn label in binding example
--

commit 6e14f976e140401abb92618c1275969f3fecf275
Author: Taniya Das <taniya.das@oss.qualcomm.com>

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

This commit adds the YAML device tree binding schema for the Qualcomm
Peripheral Web's PDM GP_MN fractional clock divider.

> 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..8141f392c5d70
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/qcom,clk-gp-mnd.yaml
> @@ -0,0 +1,98 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/qcom,clk-gp-mnd.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Peripheral Web's PDM GP_MN Clock Divider
> +
> +maintainers:
> +  - Taniya Das <taniya.das@oss.qualcomm.com>
> +
> +description: |
> +  The Peripheral Web's PDM GP_MN clock divider receives an input clock
> +  (TCXO4) with frequency Fin and generates an output clock with
> +  frequency Fout = Fin * (M / N) and a duty cycle controlled by D
> +  and routed over a gpio pin.

[ ... ]

> +properties:
> +  compatible:
> +    oneOf:
> +      - const: qcom,sc7280-gp-mnd
> +      - items:
> +          - enum:
> +              - qcom,sa8775p-gp-mnd
> +              - qcom,qcs8300-gp-mnd
> +              - qcom,qcs615-gp-mnd
> +          - const: qcom,sc7280-gp-mnd
> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: PDM XO4 source clock
> +      - description: PDM AHB bus clock for register access
> +
> +  clock-names:
> +    items:
> +      - const: pdm_clk
> +      - const: ahb_clk

[Severity: Low]
Could the schema filename, compatible enum ordering, and clock-names in
Documentation/devicetree/bindings/clock/qcom,clk-gp-mnd.yaml be updated to
follow Documentation/devicetree/bindings/writing-bindings.rst?

1. The schema filename and $id use qcom,clk-gp-mnd.yaml, whereas the
fallback compatible is qcom,sc7280-gp-mnd (and none of the compatibles use
a clk- prefix). Should the file and $id be named qcom,sc7280-gp-mnd.yaml?

2. In clock-names, pdm_clk and ahb_clk include redundant _clk suffixes.
Should these be named pdm and ahb instead?

3. In the compatible enum list, should the entries (qcom,qcs615-gp-mnd,
qcom,qcs8300-gp-mnd, qcom,sa8775p-gp-mnd) be sorted alphabetically?

> +
> +  '#clock-cells':
> +    const: 0
> +
> +  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.
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - '#clock-cells'
> +  - assigned-clocks
> +  - assigned-clock-rates

[Severity: Medium]
Should assigned-clocks and assigned-clock-rates be omitted from properties
and required in Documentation/devicetree/bindings/clock/qcom,clk-gp-mnd.yaml?

In dt-schema, assigned-clocks and assigned-clock-rates are standard optional
properties automatically allowed by the core clock schema whenever clocks or
#clock-cells is present, even with additionalProperties: false.

Defining them here with maxItems: 1 for the parent clock and listing them in
required causes dtbs_check to reject valid device tree nodes that either
rely on the default 19.2 MHz XO rate without explicitly assigning a parent
rate (which clk_gp_mnd_probe() in the driver also handles as a fallback via
if (!clk_get_rate(clk)) clk_set_rate(clk, 19200000);) or specify more than
one entry in assigned-clocks (for example, configuring both GCC_PDM_XO4_CLK
and the gp_mn output clock rate on the provider node).

Also, the description block and commit message describe the input clock as a
fixed TCXO4 clock, whereas assigned-clock-rates and the accompanying GCC
changes configure a parent PDM XO4 divider clock supporting 19200000,
9600000, 6400000, and 4800000 Hz. Could the description be updated to match?

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
> +    gp_mn: clock-controller@88d3000 {
> +        compatible = "qcom,sc7280-gp-mnd";

[Severity: Low]
In the examples block of
Documentation/devicetree/bindings/clock/qcom,clk-gp-mnd.yaml, the gp_mn node
label on clock-controller@88d3000 is never referenced via &gp_mn, which
triggers an unused-labels warning when running scripts/dtc/dt-check-style in
strict mode. Should the unused gp_mn: label be dropped?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261026-b4-pdm_clk_gp_mnd_v2-v2-0-ff8e4bc8d567@oss.qualcomm.com?part=1