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

[Conor Dooley <conor@kernel.org>]

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

On Wed, Jan 28, 2026 at 11:01:40AM +0100, Krzysztof Kozlowski wrote:
> On 28/01/2026 10:36, Gary Yang wrote:
> > There are two reset controllers on Cix sky1 Soc.
> > One is located in S0 domain, and the other is located
> > in S0 and S5 domain.
> 
> Please wrap commit message according to Linux coding style / submission
> process (neither too early nor over the limit):
> https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
> 
> > 
> > Signed-off-by: Gary Yang <gary.yang@cixtech.com>
> > Link: https://lore.kernel.org/r/20251124063235.952136-2-gary.yang@cixtech.com
> > Signed-off-by: Peter Chen <peter.chen@cixtech.com>
> > ---
> >  .../devicetree/bindings/mfd/syscon.yaml       |   7 +
> >  .../bindings/reset/cix,sky1-rst.yaml          |  46 +++++
> >  .../soc/cix/cix,sky1-system-controller.yaml   |  48 +++++
> >  include/dt-bindings/reset/cix,sky1-rst-fch.h  |  42 +++++
> >  include/dt-bindings/reset/cix,sky1-rst.h      | 164 ++++++++++++++++++
> >  5 files changed, 307 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml
> >  create mode 100644 Documentation/devicetree/bindings/soc/cix/cix,sky1-system-controller.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/mfd/syscon.yaml b/Documentation/devicetree/bindings/mfd/syscon.yaml
> > index 55efb83b1495..be864a6b8efb 100644
> > --- a/Documentation/devicetree/bindings/mfd/syscon.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/syscon.yaml
> > @@ -170,6 +170,8 @@ properties:
> >                - cirrus,ep7209-syscon1
> >                - cirrus,ep7209-syscon2
> >                - cirrus,ep7209-syscon3
> > +              - cix,sky1-system-controller
> > +              - cix,sky1-s5-system-controller
> >                - cnxt,cx92755-uc
> >                - freecom,fsg-cs2-system-controller
> >                - fsl,imx93-aonmix-ns-syscfg
> > @@ -254,6 +256,11 @@ properties:
> >            - const: microchip,pic64gx-sysreg-scb
> >            - const: microchip,mpfs-sysreg-scb
> >            - const: syscon
> > +      - items:
> > +          - enum:
> > +              - cix,sky1-system-controller
> > +              - cix,sky1-s5-system-controller
> > +          - const: syscon
> 
> Do not create your own style. Cix is not special. There is already an
> enum for EVERYONE.
> 
> >  
> >    reg:
> >      maxItems: 1
> 
> Independent patch. Please do not combine unrelated patches into one.

Was mid reply when I saw this mail come through, just wanted to add the
one thing I had to say in excess of what you've written:

All the changes to syscon.yaml should be deleted, as it adds a competing
definition to the one in the dedicated file, and I am surprised
something like this passed testing.. Perhaps it passes if
DT_SCHEMA_FILES=syscon.yaml but it'd fail a real dtbs_check, right?
At v4, I'd be hoping for more thorough testing to be done.

Cheers,
Conor.

> 
> 
> > 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..4323acdc2c45
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/reset/cix,sky1-rst.yaml
> > @@ -0,0 +1,46 @@
> > +# 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 S0 and S5 domain.
> > +
> > +  See also:
> > +  - include/dt-bindings/reset/cix,sky1-rst.h
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - cix,sky1-rst
> > +          - cix,sky1-rst-fch
> > +
> > +  '#reset-cells':
> > +    const: 1
> 
> No resources? Then not a dedicated child node, but part of the syscon.
> Fold this entire binding into the parent node.
> 
> > +
> > +required:
> > +  - compatible
> > +  - '#reset-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    syscon@16000000 {
> > +      compatible = "cix,sky1-s5-system-controller", "syscon",
> > +                "simple-mfd";
> 
> Mess alignment.
> 
> > +      reg = <0x0 0x16000000 0x0 0x1000>;
> 
> Does not belong here.
> 
> > +      src: reset-controller {
> > +        compatible = "cix,sky1-rst";
> > +        #reset-cells = <1>;
> > +      };
> 
> > +    };
> > diff --git a/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-controller.yaml b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-controller.yaml
> > new file mode 100644
> > index 000000000000..f6e2776e3f53
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/cix/cix,sky1-system-controller.yaml
> > @@ -0,0 +1,48 @@
> > +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/cix/cix,sky1-system-controller.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cix Sky1 SoC system controller register region
> > +
> > +maintainers:
> > +  - Gary Yang <gary.yang@cixtech.com>
> > +
> > +description:
> > +  An wide assortment of registers of the system controller on Sky1 SoC,
> > +  including resets and usb.
> > +
> > +allOf:
> > +  - $ref: /schemas/mfd/syscon.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - contains:
> 
> What? No, don't come with random code. There is no single code like this.
> 
> > +          - enum:
> > +              - cix,sky1-system-controller
> > +              - cix,sky1-s5-system-controller
> > +          - const: syscon
> > +          - const: simple-mfd
> 
> Huh? You already documented this.
> 
> NAK, you are making random changes without any concept.
> 
> Read previous feedback. Really carefully.
> 
> Best regards,
> Krzysztof

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