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

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

Hi Conor,
Thanks for the review.

On 02/10/26 02:56, Conor Dooley wrote:
> 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@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
> 
> 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.
>

Makes sense, I'll make generic wakeup-mode a string property in 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;
>>
>>> +      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.
> 

It is an either/or, but with a single bool, leaving it out wouldn't
clear a bypass set by an earlier boot stage. So I'd prefer the two
bool properties Linus suggested, does that work for you?

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

In v2, I will switch most of them to boolean properties, with every
state available as its own flag, so a DT can always set a known value
and override whatever an earlier stage left. For each group I'd follow
the same pattern as the generic bias-* properties: at most one flag from
a group, which also catches the dtsi override case in dtbs_check. If
none is given, the driver leaves those bits untouched, same as for
bias-*.

Would that work for you, or would you still prefer one of them to be
required?

For the numerical properties that remain, I'll add a default.

Thanks and Regards,
Yemike Abhilash Chandra