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

[Conor Dooley <conor@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3836 bytes --]

On Tue, Oct 27, 2026 at 02:39:32PM +0530, Yemike Abhilash Chandra wrote:
> On 02/10/26 02:56, Conor Dooley wrote:
> > On Thu, Oct 01, 2026 at 11:10:34PM +0200, Linus Walleij wrote:
> > > > +      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?

Maybe this is a misguided belief, but I don't think that any node for
which pin configuration is being provided should be incomplete to the
point that it relies on earlier stages - or are you thinking about nodes
that provide only pinmuxing and having no way to differentiate between a
node that has only muxing (and should not modify configuration and a node
that is meant to do pinmuxing plus set the default isolation bypass
state?

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

Sure.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]