Re: 回复: [PATCH 2/3] dt-bindings: pinctrl: Add cix,sky1-pinctrl

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/08/2025 07:37, Gary Yang wrote:
> 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@cixtech.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@cixtech.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.


State != device. Please create bindings for devices, not states.

> 
>>> +
>>> +  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?

and pinmux. There is a standard pins property.


...

>>> 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 ?

No. I suggest to be named EXACTLY like compatible.

> 
>>> @@ -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?

Really, what is unlcear in "drop all this"? Drop means to remove.

You ask for confirmation for some really obvious comments.

BTW, if you disagree provide arguments (in terms of bindings) why these
are bindings.