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

[Conor Dooley]

On Thu, Oct 01, 2026 at 11:10:34PM +0200, Linus Walleij wrote:
> Hi Yemike,
> 
> thanks for your patch!
> 
> On Wed, Sep 30, 2026 at 11:21 AM Yemike Abhilash Chandra
> <y-abhilashchandra@xxxxxx> 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

And I think it should probably be strings, so that there's some hope of
commonality, since obviously any user is going to invent different
mappings.

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

Is this not just an either or, so there should only be one property
here.

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

Additionally, if any of the custom properties are gonna be kept as
numerical values, they're all missing defaults. In the case where you're
advocating for multiple boolean properties, one of them is gonna have to
be required as otherwise we're looking at indeterminate states based on
reset values or what a prior stage may or may not have done? The reset
values should probably be considered default, but the cleanest way to
make this work with separate booleans is have a
oneOf:
 - required:
    - a
 - required:
    - b
 - required:
    - c

and you've also got to deal with people potentially making mistakes and
adding multiple properties for the same feature or trying to overwrite a
$som.dtsi/$soc-common.dtsi config and not realising that they've got to
do a /delete-property/ on the other property for the feature.

Attachment: signature.asc
Description: PGP signature