Re: [PATCH 1/3] dt-bindings: pinctrl: Add TI TDA54 pin controller

[Linus Walleij <linusw@kernel.org>]

Hi Yemike,

thanks for your patch!

On Wed, Sep 30, 2026 at 11:21 AM Yemike Abhilash Chandra
<y-abhilashchandra@ti.com> wrote:

> +      ti,debounce-select:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3, 4, 5, 6]
> +        description:
> +          Selects which DBOUNCE_CFGn period register in the control module
> +          drives the debounce filter for this pad. 0 disables debouncing,
> +          1 to 6 select DBOUNCE_CFG1 to DBOUNCE_CFG6.

What's wrong with the existing input-debounce property?

  input-debounce:
    $ref: /schemas/types.yaml#/definitions/uint32-array
    description: Takes the debounce time in usec as argument or 0 to disable
      debouncing

Just translate usec:s into your custom format in the code, problem solved.

> +      ti,virt-gpio-instance:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3, 4, 5, 6, 7]
> +        description:
> +          Selects which virtual GPIO instance controls this pad, allowing
> +          protection between multiple virtual views of the GPIO control
> +          registers. Has no effect unless the pad is muxed to GPIO mode
> +          (muxmode 7).

Wow crazy stuff. OK keep it :D

> +      ti,wakeup:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2, 3]
> +        description: |
> +          Wakeup event configuration for this pad.
> +          0 - wakeup disabled
> +          1 - wakeup triggered by any change of the pin input value
> +          2 - wakeup triggered by a low value on the pin
> +          3 - wakeup triggered by a high value on the pin

I just have the feeling this should be a generic property. It seems so useful.

Can you just add this as wakeup-mode = <custom value> in
Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml

> +      ti,retention-bias:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2]
> +        description: |
> +          Enables the internal I/O pullup/pulldown resistor when pin enters
> +          TDA54 I/O retention mode, and configures the I/O pull resistor
> +          direction.
> +          0 - OFF mode pad pull resistor disable
> +          1 - Select OFF mode pulldown resistor
> +          2 - Select OFF mode pullup resistor

Use names inspired by the generic config types and flags instead
of enums.

ti,retention-bias-disable;
ti,retention-bias-pull-down;
ti,retention-bias-pull-up;

> +      ti,retention-offmode:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1]
> +        description: |
> +          I/O behaviour when retention mode is active.
> +          0 - I/O maintains its previous state
> +          1 - I/O state is forced to the OFF mode value

Behaviour of *what*?

The driver stage?

I suspect you should make two bool flags
ti,retention-output-hold;
ti,retention-output-disable;

> +      ti,retention-output:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1, 2]
> +        description: |
> +          Output driver behaviour while the pad is in retention mode.
> +          0 - output driver disabled
> +          1 - output driver enabled, pad driven low
> +          2 - output driver enabled, pad driven high

Make three flags:
ti,retention-output-disable;
ti,retention-output-low;
ti,retention-output-high;

> +      ti,retention-force:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1]
> +        description: |
> +          I/O retention controls.
> +          0 - gated by the Device Manager logic
> +          1 - forced active, overriding the Device Manager gating logic

This is clearly a bool flag. It should contain device-manager as that
magic entity is involved.

ti,retention-device-manager-enable;
ti,retention-device-manager-forced-active;

Both seems to be related to the device manager whatever that is.

> +      ti,isolation-bypass:
> +        $ref: /schemas/types.yaml#/definitions/uint32
> +        enum: [0, 1]
> +        description: |
> +          I/O isolation for this pad.
> +          0 - isolation preserved
> +          1 - isolation bypassed

What does this even mean electronically speaking? Explain in a description:
statement.

ti,isolation-preserve;
ti.isolation-bypass-enable;

perhaps?

Some of the retention settings seem *very* generic, c.f. this
from include/dt-bindings/pinctrl/nomadik.h that has been
around forever:

#define SLPM_DISABLED           0
#define SLPM_ENABLED            1
#define SLPM_INPUT_NOPULL       0
#define SLPM_INPUT_PULLUP       1
#define SLPM_INPUT_PULLDOWN     2
#define SLPM_DIR_INPUT          3
#define SLPM_OUTPUT_LOW         0
#define SLPM_OUTPUT_HIGH        1
#define SLPM_DIR_OUTPUT         2
#define SLPM_WAKEUP_DISABLE     0
#define SLPM_WAKEUP_ENABLE      1

SLPM means "sleep mode", yeah pretty much retention...

I think the corresponding retention settings for things that are really
quite generic should just be added to the generic pin config bindings.

Yours,
Linus Walleij