Message-ID:
 <PUZPR06MB5887D9A879D16DC6A8C8ED58EF3BA@PUZPR06MB5887.apcprd06.prod.outlook.com>
Date: Thu, 28 Aug 2025 05:37:19 +0000
From: Gary Yang <gary.yang@...tech.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, "linus.walleij@...aro.org"
	<linus.walleij@...aro.org>, "robh@...nel.org" <robh@...nel.org>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>, "conor+dt@...nel.org"
	<conor+dt@...nel.org>
CC: "linux-gpio@...r.kernel.org" <linux-gpio@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	cix-kernel-upstream <cix-kernel-upstream@...tech.com>
Subject:
 回复: [PATCH 2/3] dt-bindings: pinctrl: Add cix,sky1-pinctrl

Hi Krzysztof,

Thanks for your comments

> 
> On 27/08/2025 04:42, Gary Yang wrote:
> > Add dt-bindings docs
> 
> For what? Describe the hardware here in one, two sentences.
> 

OK, we will add some description for it next version

> >
> > Signed-off-by: Gary Yang <gary.yang@...tech.com>
> > ---
> >  .../bindings/pinctrl/cix,sky1-pinctrl.yaml    |  77 +++
> >  include/dt-bindings/pinctrl/pads-sky1.h       | 592
> ++++++++++++++++++
> >  2 files changed, 669 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/pinctrl/cix,sky1-pinctrl.yaml
> >  create mode 100644 include/dt-bindings/pinctrl/pads-sky1.h
> >
> > diff --git
> > a/Documentation/devicetree/bindings/pinctrl/cix,sky1-pinctrl.yaml
> > b/Documentation/devicetree/bindings/pinctrl/cix,sky1-pinctrl.yaml
> > new file mode 100644
> > index 000000000000..10a4a292188e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/pinctrl/cix,sky1-pinctrl.yaml
> > @@ -0,0 +1,77 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/pinctrl/cix,sky1-pinctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Cix Sky1 Pin Controller
> > +
> > +maintainers:
> > +  - Gary Yang <gary.yang@...tech.com>
> > +
> > +description:
> > +  Please refer to pinctrl-bindings.txt in this directory for common
> > +  binding part and usage.
> 
> Drop description, not desired really.
> 

Ok, this yaml file comes from other yaml file. If not needed, we remove it next version

> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - cix,sky1-iomuxc
> > +      - cix,sky1-iomuxc-s5
> 
> Whats the difference between? You have entire description field to explain this
> but instead you said something obvious there.
> 
Cix sky1 has three power states. S0 means work state. S3 means STR state. S5 means SD state.

The pin-controller on sky1 has two power states. They are S0 and S5.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +# Client device subnode's properties
> > +patternProperties:
> > +  '-pins$':
> > +    type: object
> > +    description:
> > +      Pinctrl node's client devices use subnodes for desired pin
> configuration.
> > +      Client device subnodes use below standard properties.
> > +
> > +    properties:
> > +      cix,pins:
> 
> No, use generic properties from pinmux schema.
> 
> You should also reference it.

Did you suggest us to refer to Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml?

Make us support drive-strength, bias-pull-down properties?

> 
> > +        description:
> > +          each entry consists of 3 integers and represents the mux and
> config
> > +          setting for one pin. The first 2 integers <mux_reg func_num> are
> > +          specified using a CIX_PAD_* macro.The last integer CONFIG is
> the pad
> > +          setting value like pull-up on this pin.
> > +        $ref: /schemas/types.yaml#/definitions/uint32-matrix
> > +        items:
> > +          items:
> > +            - description: |
> > +                "mux_reg" indicates the offset of register.
> > +            - description: |
> > +                "func_num" indicates the mux value to be applied.
> > +            - description: |
> > +                "pad_setting" indicates the pad configuration value to be
> > +                applied.
> > +
> > +    required:
> > +      - cix,pins
> > +
> > +    additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +allOf:
> > +  - $ref: pinctrl.yaml#
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  # Pinmux controller node
> > +  - |
> > +    #include <dt-bindings/pinctrl/pads-sky1.h>
> > +    iomuxc: pinctrl@...0000 {
> > +        compatible = "cix,sky1-iomuxc";
> > +        reg = <0x4170000 0x1000>;
> > +
> > +        pinctrl_hog: hog-pins {
> 
> Don't use hog.

OK, we use other group instead of it

> 
> > +            cix,pins =
> > +                <CIX_PAD_GPIO144_FUNC_GPIO144
> (PULL_DOWN|DS_LEVEL4)>;
> > +        };
> > +    };
> > diff --git a/include/dt-bindings/pinctrl/pads-sky1.h
> > b/include/dt-bindings/pinctrl/pads-sky1.h
> > new file mode 100644
> > index 000000000000..44550e4105b3
> > --- /dev/null
> > +++ b/include/dt-bindings/pinctrl/pads-sky1.h
> 
> Bindings follow compatible naming. See writing bindings.
> 

Did you suggest rename it to pinctrl-sky1.h ?

> > @@ -0,0 +1,592 @@
> > +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> > +/*
> > + * Copyright 2024-2025 Cix Technology Group Co., Ltd.
> > + */
> > +
> > +#ifndef __SKY1_PADS_H
> > +#define __SKY1_PADS_H
> > +
> > +#define CIX_PAD_GPIO001_OFFSET                       0x0
> > +#define CIX_PAD_GPIO002_OFFSET                       0x4
> 
> Not bindings. Drop all this.
> 

Do you mean those macros not used need to delete?

> 
> 
> Best regards,
> Krzysztof

Best wishes
Gary

Powered by blists - more mailing lists