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

[Yemike Abhilash Chandra <y-abhilashchandra@ti.com>]

Hi Linus,

On 02/10/26 02:40, Linus Walleij wrote:
> Hi Yemike,
> 
> thanks for your patch!
> 

Thank you for the review and apologies for the delay.

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

The pad register doesn't really store a debounce time. It just has a 0-6
selector that points to one of six DBOUNCE_CFGn registers, and the actual
periods are set in those registers, which sit in a separate control module,
not in the pin controller.

I did think about using input-debounce, but putting 1 to 6 in there to
mean "use DBOUNCE_CFG1..6" felt like misusing a property that's meant
to be in usec. That's why I went with a custom property.

Happy to go either way though, let me know what you'd prefer.

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

Understood. As suggested, I'm adding a few of these as generic properties
to the common bindings and the driver for v2.

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

I will do this change in v2 as generic retention bias settings.

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

Not just the driver stage, it's the whole pad. I checked this with our
HW team. When this bit is set and I/O retention kicks in, the pad
switches over to its retention settings (output driver, pull and the
pad level). I will add this detail in the description in v2.

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

Since it isn't output specific, and retention-output-disable
would clash with the retention output flags below, how about:

   retention-hold-state;
   retention-offmode-enable;

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

Will do in v2.

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

I will switch to boolean properties in v2.


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

When the SoC goes into low power, each pad is isolated, it latches its
current settings and is cut off from the core, so it holds its state
while the core supply is off. With bypass set, the pad skips this and
keeps working normally, which needs the core to stay powered. I'll add
this to the description in v2.

> ti,isolation-preserve;
> ti.isolation-bypass-enable;
> 
> perhaps?
> 

I will switch to boolean properties in v2.

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

Agreed. For v2 I plan to make wakeup-mode, retention-bias-*, 
retention-output-*
and the retention offmode generic, and keep isolation bypass and the 
device manager
force as ti, properties since those are TI specific. Let me know if you 
have any
other preferences.

Thanks and Regards,
Yemike Abhilash Chandra


> Yours,
> Linus Walleij