Re: [PATCH v1 2/8] dt-bindings: pinctrl: Add bindings for TI TAC5x1x pinctrl

[Krzysztof Kozlowski]

On Fri, Mar 13, 2026 at 12:18:27AM +0530, Niranjan H Y wrote:
> Add device tree bindings for the Texas Instruments TAC5x1x family
> pin controller. These bindings define the GPIO and pin control
> configuration interface for the device.

A nit, subject: drop second/last, redundant "bindings for". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Niranjan H Y <niranjan.hy@xxxxxx>
> ---
>  .../bindings/pinctrl/ti,tac5x1x-pinctrl.yaml  | 163 ++++++++++++++++++
>  include/dt-bindings/pinctrl/tac5x1x.h         |  44 +++++
>  2 files changed, 207 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/ti,tac5x1x-pinctrl.yaml
>  create mode 100644 include/dt-bindings/pinctrl/tac5x1x.h
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/ti,tac5x1x-pinctrl.yaml b/Documentation/devicetree/bindings/pinctrl/ti,tac5x1x-pinctrl.yaml
> new file mode 100644
> index 000000000000..3ccb262d6247
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/pinctrl/ti,tac5x1x-pinctrl.yaml
> @@ -0,0 +1,163 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/pinctrl/ti,tac5x1x-pinctrl.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI TAC5x1x Pin Controller
> +
> +maintainers:
> +  - Niranjan H Y <niranjan.hy@xxxxxx>
> +
> +description: |
> +  The TAC5x1x devices have 5 configurable pins that can be used for GPIO
> +  or alternate functions like PDM (Pulse Density Modulation) and interrupt
> +  generation. A subset of pins can be present in any variant of the HW.
> +
> +  This binding is used as a child node of the main TAC5x1x MFD device
> +  described in Documentation/devicetree/bindings/mfd/ti,tac5x1x.yaml
> +
> +  Pin capabilities:
> +  - GPIO1, GPIO2: Bidirectional, configurable as GPIO, PDM clock, or IRQ output
> +  - GPO1: Output only, configurable as GPIO, PDM clock, or IRQ output.
> +          Some variants use different name GPO1A.
> +  - GPI1: Input only, configurable as GPIO or PDM data input
> +          Some variants use different name GPI1A.
> +  - GPI2A: Input only, configurable as GPIO or PDM data input
> +
> +properties:
> +  compatible:
> +    const: ti,tac5x1x-pinctrl

No wildcards in compatibles and filenames.

> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2
> +    description: |
> +      First cell is the pin number (0-4 corresponding to GPIO1, GPIO2, GPO1, GPI1, GPI2A)
> +      Second cell is flags (GPIO_ACTIVE_HIGH or GPIO_ACTIVE_LOW)
> +
> +  gpio-ranges:
> +    maxItems: 1
> +    description: GPIO range mapping to pin controller
> +
> +  gpio-line-names:
> +    minItems: 1
> +    maxItems: 5
> +    description: Names for the GPIO lines
> +
> +patternProperties:
> +  '-state$':
> +    type: object
> +    description: Pin configuration state
> +    $ref: /schemas/pinctrl/pinmux-node.yaml#
> +
> +    patternProperties:
> +      '^.*$':

-group$ or -grp$

> +        type: object
> +        $ref: /schemas/pinctrl/pincfg-node.yaml#
> +
> +        properties:
> +          groups:
> +            description: Pin groups to configure
> +            items:
> +              enum:
> +                - gpio1
> +                - gpio2
> +                - gpo1
> +                - gpi1
> +                - gpi2a
> +                - pdm_gpio1_gpio2
> +                - pdm_gpio1_gpi1
> +                - pdm_gpio1_gpi2a
> +                - pdm_gpio2_gpio1
> +                - pdm_gpio2_gpi1
> +                - pdm_gpio2_gpi2a
> +                - pdm_gpo1_gpio1
> +                - pdm_gpo1_gpio2
> +                - pdm_gpo1_gpi1
> +                - pdm_gpo1_gpi2a
> +
> +          function:
> +            description: Function to assign
> +            enum:
> +              - gpio
> +              - pdm
> +              - irq
> +
> +          drive-push-pull:
> +            type: boolean
> +            description: Enable push-pull drive mode
> +
> +          drive-open-drain:
> +            type: boolean
> +            description: Enable open-drain drive mode
> +
> +          drive-open-source:
> +            type: boolean
> +            description: Enable open-source drive mode
> +
> +          bias-pull-up:
> +            type: boolean
> +            description: Enable pull-up bias
> +
> +          bias-pull-down:
> +            type: boolean
> +            description: Enable pull-down bias
> +
> +          bias-high-impedance:
> +            type: boolean
> +            description: Set pin to high impedance
> +
> +          input-enable:
> +            type: boolean
> +            description: Enable input buffer
> +
> +          output-enable:
> +            type: boolean
> +            description: Enable output buffer

Drop all descriptions and type. common schema defines them. You only
need :true if none of other properties are applicable here.


> +
> +        required:
> +          - groups
> +          - function
> +
> +        additionalProperties: false
> +
> +    additionalProperties: false
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - '#gpio-cells'
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pinctrl {
> +        compatible = "ti,tac5x1x-pinctrl";
> +        gpio-controller;
> +        #gpio-cells = <2>;
> +        gpio-ranges = <&pinctrl 0 0 5>;
> +        gpio-line-names = "GPIO1", "GPIO2", "GPO1", "GPI1", "GPI2A";
> +
> +        default_state: default-state {
> +            pdm_config {
> +                groups = "pdm_gpo1_gpi1";
> +                function = "pdm";
> +                drive-push-pull;
> +            };
> +
> +            gpio_config {
> +                groups = "gpio1", "gpio2";
> +                function = "gpio";
> +                bias-pull-up;
> +            };
> +
> +            irq_config {
> +                groups = "gpo1";
> +                function = "irq";
> +                drive-open-drain;
> +            };
> +        };
> +    };
> diff --git a/include/dt-bindings/pinctrl/tac5x1x.h b/include/dt-bindings/pinctrl/tac5x1x.h
> new file mode 100644
> index 000000000000..8cc3fa0b7946
> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/tac5x1x.h

Header always match bindings filename.

> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * Device Tree binding constants for TAC5x1x pinctrl
> + *
> + * Copyright (C) 2025 Texas Instruments Incorporated
> + * Author: Niranjan H Y <niranjan.hy@xxxxxx>
> + */
> +
> +#ifndef _DT_BINDINGS_PINCTRL_TAC5X1X_H
> +#define _DT_BINDINGS_PINCTRL_TAC5X1X_H
> +
> +/* Pin IDs */
> +#define TAC5X1X_PIN_GPIO1 0
> +#define TAC5X1X_PIN_GPIO2 1
> +#define TAC5X1X_PIN_GPO1 2
> +#define TAC5X1X_PIN_GPI1 3
> +#define TAC5X1X_PIN_GPI2A 4

Since when GPIOs are bindings?

> +
> +/* Pin functions */
> +#define TAC5X1X_FUNC_GPIO 0
> +#define TAC5X1X_FUNC_PDM 1
> +#define TAC5X1X_FUNC_IRQ 2

No, pin function is a string.

> +
> +/* Pin drive modes */
> +#define TAC5X1X_DRIVE_HIZ 0
> +#define TAC5X1X_DRIVE_PUSH_PULL 1
> +#define TAC5X1X_DRIVE_PULL_DOWN 2
> +#define TAC5X1X_DRIVE_OPEN_DRAIN 3
> +#define TAC5X1X_DRIVE_PULL_UP 4
> +#define TAC5X1X_DRIVE_OPEN_SOURCE 5

Are you sure this patch was extensively reviewed internally?

Best regards,
Krzysztof