Re: [PATCH v3 1/3] dt-bindings: reset: add sky1 reset controller

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2896 bytes --]

On Mon, Nov 24, 2025 at 02:32:33PM +0800, Gary Yang wrote:
> There are two reset controllers on Cix sky1 Soc.
> One is located in S0 domain, and the other is located
> in S5 domain.
> 
> Signed-off-by: Gary Yang <gary.yang@cixtech.com>
> ---
>  .../bindings/reset/cix,sky1-rst.yaml          |  50 ++++++
>  include/dt-bindings/reset/cix,sky1-rst-fch.h  |  42 +++++
>  include/dt-bindings/reset/cix,sky1-rst.h      | 164 ++++++++++++++++++
>  3 files changed, 256 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml
>  create mode 100644 include/dt-bindings/reset/cix,sky1-rst-fch.h
>  create mode 100644 include/dt-bindings/reset/cix,sky1-rst.h
> 
> diff --git a/Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml b/Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml
> new file mode 100644
> index 000000000000..a28f938a283d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/cix,sky1-rst.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CIX Sky1 Reset Controller
> +
> +maintainers:
> +  - Gary Yang <gary.yang@cixtech.com>
> +
> +description: |
> +  CIX Sky1 reset controller can be used to reset various set of peripherals.
> +  There are two reset controllers, one is located in S0 domain, the other
> +  is located in S5 domain.
> +
> +  See also:
> +  - include/dt-bindings/reset/cix,sky1-rst.h
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - cix,sky1-rst
> +          - cix,sky1-rst-fch

You've not addressed my v2 commentary:
https://lore.kernel.org/all/20251114-problem-overbook-383f8e45cd0b@spud/
I asked what else the device does, but you didn't answer me. Dropping the
syscon doesn't make sense if the device genuinely has other functions.

> +
> +  reg:
> +    minItems: 1
> +    maxItems: 3
> +
> +  '#reset-cells':
> +    const: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - '#reset-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/reset/cix,sky1-rst.h>
> +    reset-controller@16000304 {
> +      compatible = "cix,sky1-rst";

> +      reg = <0x16000304 0xc>,
> +            <0x16000400 0x10>,
> +            <0x16000800 0x8>;

This is also highly suspect, and I believe what you had before was
probably much more realistic.
Do things properly and fully *now*, rather than pay the price of
unravelling it all later. I just did this for one of my own platforms,
and putting in the effort to completely describe stuff up front is
actually worth it rather than having to refactor years down the line.

Cheers,
Conor.

pw-bot: changes-requested

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]