[RFC,v4,0/4] Pinctrl: A4: Add pinctrl driver

Message

Xianwei Zhao via B4 Relay Nov. 1, 2024, 8:27 a.m. UTC

Add pinctrl driver support for Amloigc A4 SoC

I want to find out what kind of solution is feasible to
meet the needs of all parties. This RFC verion is one of them.

All of Amogic SoCs GPIO device requirement is met here by
adding GPIO bank definition instead of the pin definition.
Binding header files will no longer be added to future
SoCs's pin devices.

The pinctrl software only adds insterface of of_xlate to support
for transformation without affecting the overall framework and
is compatible with previous drivers.

The code in DTS file is also readable when using GPIO, as below:

reset-gpios = <&gpio AMLOGIC_GPIO(AMLOGIC_GPIO_X, 6) GPIO_ACTIVE_LOW>;

Signed-off-by: Xianwei Zhao <[e-mail]>
---
Changes in v4:
- Add interface of of_xlate support.
- Add const for some variable.
- Link to v3: https://lore.kernel.org/r/[e-mail]

Changes in v3:
- Remove head file from binding.
- Move GPIO define to file *.c.
- Link to v2: https://lore.kernel.org/r/[e-mail]

Changes in v2:
- Use one marco instead of all pin define.
- Add unit name for dts node.
- Link to v1: https://lore.kernel.org/all/[e-mail]/

---
Xianwei Zhao (4):
      dt-bindings: pinctrl: Add support for Amlogic A4 SoCs
      pinctrl: meson: add interface of of_xlate
      pinctrl: meson: Add driver support for Amlogic A4 SoCs
      arm64: dts: amlogic: a4: add pinctrl node

 .../bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml |    2 +
 arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi        |   36 +
 drivers/pinctrl/meson/Kconfig                      |    6 +
 drivers/pinctrl/meson/Makefile                     |    1 +
 drivers/pinctrl/meson/pinctrl-amlogic-a4.c         | 1321 ++++++++++++++++++++
 drivers/pinctrl/meson/pinctrl-meson.c              |    4 +
 drivers/pinctrl/meson/pinctrl-meson.h              |    4 +
 include/dt-bindings/gpio/amlogic-gpio.h            |   50 +
 8 files changed, 1424 insertions(+)
---
base-commit: 58e2d28ed28e5bc8836f8c14df1f94c27c1f9e2f
change-id: 20241012-a4_pinctrl-09d1b2a17e47

Best regards,

Comments

Neil Armstrong Nov. 10, 2024, 12:24 p.m. UTC | #1

Hi,

Le 01/11/2024 à 09:27, Xianwei Zhao via B4 Relay a écrit :
> Add pinctrl driver support for Amloigc A4 SoC
> 
> I want to find out what kind of solution is feasible to
> meet the needs of all parties. This RFC verion is one of them.
> 
> All of Amogic SoCs GPIO device requirement is met here by
> adding GPIO bank definition instead of the pin definition.
> Binding header files will no longer be added to future
> SoCs's pin devices.
> 
> The pinctrl software only adds insterface of of_xlate to support
> for transformation without affecting the overall framework and
> is compatible with previous drivers.
> 
> The code in DTS file is also readable when using GPIO, as below:
> 
> reset-gpios = <&gpio AMLOGIC_GPIO(AMLOGIC_GPIO_X, 6) GPIO_ACTIVE_LOW>;

Fine, but why not use 3 cells instead of this macro ? Since you introduced the
custom xlate, parsing the 3 cells would be easier that using a macro:

reset-gpios = <&gpio AMLOGIC_GPIO_X 6 GPIO_ACTIVE_LOW>;

Neil

> 
> Signed-off-by: Xianwei Zhao <[e-mail]>
> ---
> Changes in v4:
> - Add interface of of_xlate support.
> - Add const for some variable.
> - Link to v3: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v3:
> - Remove head file from binding.
> - Move GPIO define to file *.c.
> - Link to v2: https://lore.kernel.org/r/[e-mail]
> 
> Changes in v2:
> - Use one marco instead of all pin define.
> - Add unit name for dts node.
> - Link to v1: https://lore.kernel.org/all/[e-mail]/
> 
> ---
> Xianwei Zhao (4):
>        dt-bindings: pinctrl: Add support for Amlogic A4 SoCs
>        pinctrl: meson: add interface of of_xlate
>        pinctrl: meson: Add driver support for Amlogic A4 SoCs
>        arm64: dts: amlogic: a4: add pinctrl node
> 
>   .../bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml |    2 +
>   arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi        |   36 +
>   drivers/pinctrl/meson/Kconfig                      |    6 +
>   drivers/pinctrl/meson/Makefile                     |    1 +
>   drivers/pinctrl/meson/pinctrl-amlogic-a4.c         | 1321 ++++++++++++++++++++
>   drivers/pinctrl/meson/pinctrl-meson.c              |    4 +
>   drivers/pinctrl/meson/pinctrl-meson.h              |    4 +
>   include/dt-bindings/gpio/amlogic-gpio.h            |   50 +
>   8 files changed, 1424 insertions(+)
> ---
> base-commit: 58e2d28ed28e5bc8836f8c14df1f94c27c1f9e2f
> change-id: 20241012-a4_pinctrl-09d1b2a17e47
> 
> Best regards,

Xianwei Zhao Nov. 11, 2024, 3:26 a.m. UTC | #2

Hi Neil,
    Thanks for your reply.

On 2024/11/10 20:24, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> Hi,
> 
> Le 01/11/2024 à 09:27, Xianwei Zhao via B4 Relay a écrit :
>> Add pinctrl driver support for Amloigc A4 SoC
>>
>> I want to find out what kind of solution is feasible to
>> meet the needs of all parties. This RFC verion is one of them.
>>
>> All of Amogic SoCs GPIO device requirement is met here by
>> adding GPIO bank definition instead of the pin definition.
>> Binding header files will no longer be added to future
>> SoCs's pin devices.
>>
>> The pinctrl software only adds insterface of of_xlate to support
>> for transformation without affecting the overall framework and
>> is compatible with previous drivers.
>>
>> The code in DTS file is also readable when using GPIO, as below:
>>
>> reset-gpios = <&gpio AMLOGIC_GPIO(AMLOGIC_GPIO_X, 6) GPIO_ACTIVE_LOW>;
> 
> Fine, but why not use 3 cells instead of this macro ? Since you 
> introduced the
> custom xlate, parsing the 3 cells would be easier that using a macro:
> 
> reset-gpios = <&gpio AMLOGIC_GPIO_X 6 GPIO_ACTIVE_LOW>;
> 
> Neil

I was prepared to do this before, mainly later considering incompatible 
binding, using the original two parameter passing

If use three parameters, I  need to modify the corresponding binding 
property. in file:
Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-common.yaml

       "#gpio-cells":
         const: 2
It must be compatible with the current number of parameters(3)

> 
>>
>> Signed-off-by: Xianwei Zhao <[e-mail]>
>> ---
>> Changes in v4:
>> - Add interface of of_xlate support.
>> - Add const for some variable.
>> - Link to v3: 
>> https://lore.kernel.org/r/[e-mail]
>>
>> Changes in v3:
>> - Remove head file from binding.
>> - Move GPIO define to file *.c.
>> - Link to v2: 
>> https://lore.kernel.org/r/[e-mail]
>>
>> Changes in v2:
>> - Use one marco instead of all pin define.
>> - Add unit name for dts node.
>> - Link to v1: 
>> https://lore.kernel.org/all/[e-mail]/
>>
>> ---
>> Xianwei Zhao (4):
>>        dt-bindings: pinctrl: Add support for Amlogic A4 SoCs
>>        pinctrl: meson: add interface of of_xlate
>>        pinctrl: meson: Add driver support for Amlogic A4 SoCs
>>        arm64: dts: amlogic: a4: add pinctrl node
>>
>>   .../bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml |    2 +
>>   arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi        |   36 +
>>   drivers/pinctrl/meson/Kconfig                      |    6 +
>>   drivers/pinctrl/meson/Makefile                     |    1 +
>>   drivers/pinctrl/meson/pinctrl-amlogic-a4.c         | 1321 
>> ++++++++++++++++++++
>>   drivers/pinctrl/meson/pinctrl-meson.c              |    4 +
>>   drivers/pinctrl/meson/pinctrl-meson.h              |    4 +
>>   include/dt-bindings/gpio/amlogic-gpio.h            |   50 +
>>   8 files changed, 1424 insertions(+)
>> ---
>> base-commit: 58e2d28ed28e5bc8836f8c14df1f94c27c1f9e2f
>> change-id: 20241012-a4_pinctrl-09d1b2a17e47
>>
>> Best regards,
>

Neil Armstrong Nov. 12, 2024, 8:24 a.m. UTC | #3

On 11/11/2024 04:26, Xianwei Zhao wrote:
> Hi Neil,
>     Thanks for your reply.
> 
> On 2024/11/10 20:24, Neil Armstrong wrote:
>> [ EXTERNAL EMAIL ]
>>
>> Hi,
>>
>> Le 01/11/2024 à 09:27, Xianwei Zhao via B4 Relay a écrit :
>>> Add pinctrl driver support for Amloigc A4 SoC
>>>
>>> I want to find out what kind of solution is feasible to
>>> meet the needs of all parties. This RFC verion is one of them.
>>>
>>> All of Amogic SoCs GPIO device requirement is met here by
>>> adding GPIO bank definition instead of the pin definition.
>>> Binding header files will no longer be added to future
>>> SoCs's pin devices.
>>>
>>> The pinctrl software only adds insterface of of_xlate to support
>>> for transformation without affecting the overall framework and
>>> is compatible with previous drivers.
>>>
>>> The code in DTS file is also readable when using GPIO, as below:
>>>
>>> reset-gpios = <&gpio AMLOGIC_GPIO(AMLOGIC_GPIO_X, 6) GPIO_ACTIVE_LOW>;
>>
>> Fine, but why not use 3 cells instead of this macro ? Since you introduced the
>> custom xlate, parsing the 3 cells would be easier that using a macro:
>>
>> reset-gpios = <&gpio AMLOGIC_GPIO_X 6 GPIO_ACTIVE_LOW>;
>>
>> Neil
> 
> I was prepared to do this before, mainly later considering incompatible binding, using the original two parameter passing
> 
> If use three parameters, I  need to modify the corresponding binding property. in file:
> Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-common.yaml
> 
>        "#gpio-cells":
>          const: 2
> It must be compatible with the current number of parameters(3)

Yes, you may move the #gpio-cells definition out of the common yaml
and define them in the soc spefic yaml and set it to 3 for a4.

Neil

> 
>>
>>>
>>> Signed-off-by: Xianwei Zhao <[e-mail]>
>>> ---
>>> Changes in v4:
>>> - Add interface of of_xlate support.
>>> - Add const for some variable.
>>> - Link to v3: https://lore.kernel.org/r/[e-mail]
>>>
>>> Changes in v3:
>>> - Remove head file from binding.
>>> - Move GPIO define to file *.c.
>>> - Link to v2: https://lore.kernel.org/r/[e-mail]
>>>
>>> Changes in v2:
>>> - Use one marco instead of all pin define.
>>> - Add unit name for dts node.
>>> - Link to v1: https://lore.kernel.org/all/[e-mail]/
>>>
>>> ---
>>> Xianwei Zhao (4):
>>>        dt-bindings: pinctrl: Add support for Amlogic A4 SoCs
>>>        pinctrl: meson: add interface of of_xlate
>>>        pinctrl: meson: Add driver support for Amlogic A4 SoCs
>>>        arm64: dts: amlogic: a4: add pinctrl node
>>>
>>>   .../bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml |    2 +
>>>   arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi        |   36 +
>>>   drivers/pinctrl/meson/Kconfig                      |    6 +
>>>   drivers/pinctrl/meson/Makefile                     |    1 +
>>>   drivers/pinctrl/meson/pinctrl-amlogic-a4.c         | 1321 ++++++++++++++++++++
>>>   drivers/pinctrl/meson/pinctrl-meson.c              |    4 +
>>>   drivers/pinctrl/meson/pinctrl-meson.h              |    4 +
>>>   include/dt-bindings/gpio/amlogic-gpio.h            |   50 +
>>>   8 files changed, 1424 insertions(+)
>>> ---
>>> base-commit: 58e2d28ed28e5bc8836f8c14df1f94c27c1f9e2f
>>> change-id: 20241012-a4_pinctrl-09d1b2a17e47
>>>
>>> Best regards,
>>

Xianwei Zhao Nov. 12, 2024, 8:58 a.m. UTC | #4

Hi Neil,
    Thanks for your reply.

On 2024/11/12 16:24, Neil Armstrong wrote:
> [ EXTERNAL EMAIL ]
> 
> On 11/11/2024 04:26, Xianwei Zhao wrote:
>> Hi Neil,
>>     Thanks for your reply.
>>
>> On 2024/11/10 20:24, Neil Armstrong wrote:
>>> [ EXTERNAL EMAIL ]
>>>
>>> Hi,
>>>
>>> Le 01/11/2024 à 09:27, Xianwei Zhao via B4 Relay a écrit :
>>>> Add pinctrl driver support for Amloigc A4 SoC
>>>>
>>>> I want to find out what kind of solution is feasible to
>>>> meet the needs of all parties. This RFC verion is one of them.
>>>>
>>>> All of Amogic SoCs GPIO device requirement is met here by
>>>> adding GPIO bank definition instead of the pin definition.
>>>> Binding header files will no longer be added to future
>>>> SoCs's pin devices.
>>>>
>>>> The pinctrl software only adds insterface of of_xlate to support
>>>> for transformation without affecting the overall framework and
>>>> is compatible with previous drivers.
>>>>
>>>> The code in DTS file is also readable when using GPIO, as below:
>>>>
>>>> reset-gpios = <&gpio AMLOGIC_GPIO(AMLOGIC_GPIO_X, 6) GPIO_ACTIVE_LOW>;
>>>
>>> Fine, but why not use 3 cells instead of this macro ? Since you 
>>> introduced the
>>> custom xlate, parsing the 3 cells would be easier that using a macro:
>>>
>>> reset-gpios = <&gpio AMLOGIC_GPIO_X 6 GPIO_ACTIVE_LOW>;
>>>
>>> Neil
>>
>> I was prepared to do this before, mainly later considering 
>> incompatible binding, using the original two parameter passing
>>
>> If use three parameters, I  need to modify the corresponding binding 
>> property. in file:
>> Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-common.yaml
>>
>>        "#gpio-cells":
>>          const: 2
>> It must be compatible with the current number of parameters(3)
> 
> Yes, you may move the #gpio-cells definition out of the common yaml
> and define them in the soc spefic yaml and set it to 3 for a4.
> 

I can modify 
Documentation/devicetree/bindings/pinctrl/amlogic,meson-pinctrl-common.yaml 
file
change
         "#gpio-cells":
           const: 2
to
         "#gpio-cells":
           enum: [2, 3]

  and make it to compatible with subsequent ones. Do you feel OK?

> Neil
> 
>>
>>>
>>>>
>>>> Signed-off-by: Xianwei Zhao <[e-mail]>
>>>> ---
>>>> Changes in v4:
>>>> - Add interface of of_xlate support.
>>>> - Add const for some variable.
>>>> - Link to v3: 
>>>> https://lore.kernel.org/r/[e-mail]
>>>>
>>>> Changes in v3:
>>>> - Remove head file from binding.
>>>> - Move GPIO define to file *.c.
>>>> - Link to v2: 
>>>> https://lore.kernel.org/r/[e-mail]
>>>>
>>>> Changes in v2:
>>>> - Use one marco instead of all pin define.
>>>> - Add unit name for dts node.
>>>> - Link to v1: 
>>>> https://lore.kernel.org/all/[e-mail]/
>>>>
>>>> ---
>>>> Xianwei Zhao (4):
>>>>        dt-bindings: pinctrl: Add support for Amlogic A4 SoCs
>>>>        pinctrl: meson: add interface of of_xlate
>>>>        pinctrl: meson: Add driver support for Amlogic A4 SoCs
>>>>        arm64: dts: amlogic: a4: add pinctrl node
>>>>
>>>>   .../bindings/pinctrl/amlogic,meson-pinctrl-a1.yaml |    2 +
>>>>   arch/arm64/boot/dts/amlogic/amlogic-a4.dtsi        |   36 +
>>>>   drivers/pinctrl/meson/Kconfig                      |    6 +
>>>>   drivers/pinctrl/meson/Makefile                     |    1 +
>>>>   drivers/pinctrl/meson/pinctrl-amlogic-a4.c         | 1321 
>>>> ++++++++++++++++++++
>>>>   drivers/pinctrl/meson/pinctrl-meson.c              |    4 +
>>>>   drivers/pinctrl/meson/pinctrl-meson.h              |    4 +
>>>>   include/dt-bindings/gpio/amlogic-gpio.h            |   50 +
>>>>   8 files changed, 1424 insertions(+)
>>>> ---
>>>> base-commit: 58e2d28ed28e5bc8836f8c14df1f94c27c1f9e2f
>>>> change-id: 20241012-a4_pinctrl-09d1b2a17e47
>>>>
>>>> Best regards,
>>>
>