Re: [PATCH 4/9] dt-bindings: pinctrl: Add UltraRISC DP1000 pinctrl bindings

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] The schema lacks a top-level `required` block for mandatory controller properties.
- [Medium] The binding introduces custom legacy vendor properties and magic macros instead of using generic pinctrl properties.
- [Medium] The `maxItems: 32` constraint on the legacy array properties allows structurally incomplete configuration tuples.
- [Medium] The schema incorrectly uses `additionalProperties: false` alongside `$ref` inclusions, forcing redundant property definitions.
- [Low] The documentation incorrectly describes the legacy properties as a `phandle-array`.
- [Low] The schema fails to enforce the hardware limitation that `func1` is unsupported on `LPC` pins.
--

commit 20f50ac043d77ef903b805d0902034551c955519
Author: Jia Wang <wangjia@ultrarisc.com>

dt-bindings: pinctrl: Add UltraRISC DP1000 pinctrl bindings
    
Add bindings for the pin controllers on the UltraRISC DP1000 RISC-V SoC.

> diff --git a/Documentation/devicetree/bindings/pinctrl/ultrarisc,dp1000-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/ultrarisc,dp1000-pinctrl.yaml
> new file mode 100644
> index 0000000000000..c7ed1f96382af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/ultrarisc,dp1000-pinctrl.yaml
> @@ -0,0 +1,168 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/ultrarisc,dp1000-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: UltraRISC DP1000 Pin Controller
> +maintainers:
> +  - Jia Wang <wangjia@ultrarisc.com>
> +
> +description: |
> +  UltraRISC RISC-V SoC DP1000 pin controller.
> +
> +  The binding supports two child node styles under the same controller
> +  compatible:
> +
> +  - legacy DP1000-specific nodes using phandle-array properties
> +    `pinctrl-pins` and `pinconf-pins`

Is "phandle-array" accurate here? The schema defines these as flat uint32
arrays, and the examples show raw integer arrays without any phandles.

> +  - generic pinctrl nodes using `pins`, `function` and generic pin
> +    configuration properties
> +
> +properties:
> +  compatible:
> +    const: ultrarisc,dp1000-pinctrl
> +
> +  reg:
> +    maxItems: 1
> +
> +  "#pinctrl-cells":
> +    $ref: /schemas/types.yaml#/definitions/uint32

Should there be a top-level required block for compatible and reg?
Without it, a device tree node omitting the reg property or left entirely
empty could silently pass schema validation.

> +patternProperties:
> +  '.*-pins$':
> +    type: object
> +    allOf:
> +      - $ref: /schemas/pinctrl/pincfg-node.yaml#
> +      - $ref: /schemas/pinctrl/pinmux-node.yaml#
> +    additionalProperties: false

Should this use unevaluatedProperties: false instead? Using
additionalProperties: false alongside $ref ignores properties imported by
the references and forces redundantly redefining generic properties locally
in this file.

> +    properties:
> +      pinctrl-pins:
> +        description: |
> +          The list of pins and their mux settings that properties in the node
> +          apply to. The format: `PORT  PIN  FUNCTION`.
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        minItems: 1
> +        maxItems: 32

Since the format requires 3 integers per tuple, does a maxItems: 32
constraint allow structurally incomplete configuration tuples? 32 is not a
multiple of 3, so it rejects a valid 11-pin configuration (33 integers) while
permitting an invalid array ending with a partial tuple.

> +      pinconf-pins:
> +        description: |
> +          The list of pins and their pad configuration that properties in the
> +          node apply to. The format: `PORT  PIN  CONF`.
> +          CONF is a DP1000-specific encoding of pull and drive strength as
> +          defined in dt-bindings/pinctrl/ultrarisc,dp1000-pinctrl.h.
> +        $ref: /schemas/types.yaml#/definitions/uint32-array
> +        minItems: 1
> +        maxItems: 32

Is it appropriate to encode distinct physical characteristics like bias and
drive strength into a single integer using a macro? The pinctrl subsystem's
architectural design typically models these as individual, human-readable DT
properties.

For a new upstream driver, shouldn't it rely exclusively on the standard
generic pinctrl framework rather than introducing new custom vendor properties?

[ ... ]

> +      function:
> +        description: |
> +          Mux function to select for the listed pins.
> +          gpio maps to the hardware default mode. The default mode is
> +          GPIO for PA/PB/PC/PD pins and LPC for LPC pins.
> +          func1 is not supported on LPC pins.
> +        enum:
> +          - gpio
> +          - func0
> +          - func1

Could the schema enforce the hardware limitation that func1 is unsupported
on LPC pins? Adding an allOf with an if/then block would prevent this
invalid combination from passing schema validation.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-ultrarisc-pinctrl-v1-0-bf559589ea8a@ultrarisc.com?part=4