Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley <conor@kernel.org>]

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

On Fri, Nov 21, 2025 at 11:21:38AM +0000, Conor Dooley wrote:
> On Fri, Nov 21, 2025 at 10:46:54AM +0000, Conor Dooley wrote:
> > On Fri, Nov 21, 2025 at 12:13:21AM +0100, Linus Walleij wrote:
> > > On Thu, Nov 20, 2025 at 1:26 AM Conor Dooley <conor@kernel.org> wrote:
> > > > On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> > > > > On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> > > 
> > > > > I looked at the bindings that look like this and are not 1:1 to the
> > > > > in-kernel configs:
> > > > >
> > > > >   input-schmitt-enable:
> > > > >     type: boolean
> > > > >     description: enable schmitt-trigger mode
> > > > >
> > > > >   input-schmitt-disable:
> > > > >     type: boolean
> > > > >     description: disable schmitt-trigger mode
> > > > >
> > > > >   input-schmitt-microvolt:
> > > > >     description: threshold strength for schmitt-trigger
> > > > >
> > > > > 1. input-schmitt is missing! But it is right there in
> > > > > drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> > > > > using input-schmitt-enable/disable and -microvolt.
> > > > >
> > > > > 2. input-schmitt-microvolt should probably be used separately
> > > > > to set the voltage threshold and can be used in conjunction
> > > > > with input-schmitt-enable in the same node. In your case
> > > > > you probably don't want to use it at all and disallow it.
> > > > >
> > > > > They are all treated individually in the parser.
> > > > >
> > > > > Maybe we could patch the docs in pinconf-generic.h to make it clear that
> > > > > they are all mutually exclusive.
> > > > >
> > > > > The DT parser is a bit primitive for these.
> > > > > For example right now it is fine with the schema
> > > > > to set input-schmitt-enable and input-schmitt-disable at the same time, and
> > > > > the result will be enabled because of parse order :/
> > > >
> > > > > The real trick would be to also make the
> > > > > schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > > > > make them at least mutually exclusive and deprecate the
> > > > > input-schmitt that noone is using, maybe that is simpler than I think?
> > > >
> > > > I think that this is probably what to do. Mutual exclusion isn't
> > > > difficult to set up there and if there's no property for "input-schmitt"
> > > > then deprecating it sounds pretty reasonable?
> > > 
> > > Yeah I agree.
> > > 
> > > Do you want to look into it?
> > > 
> > > Otherwise it becomes my problem now that I've noticed it :D
> > 
> > Yeah, it's just a binding patch here I think, so yeah I'll do it.
> 
> ngl, I forget if there's a shorthand for the bias part, so I just want
> to know if is this an accurate summary of what's exclusive?
> 
> diff --git a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> index cbfcf215e571..6865472ac124 100644
> --- a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> +++ b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> @@ -153,4 +153,66 @@ properties:
>        pin. Typically indicates how many double-inverters are
>        used to delay the signal.
>  
> +allOf:
> +  - if:
> +      required:
> +        - output-disable
> +    then:
> +      properties:
> +        output-enable: false
> +        output-impedance-ohms: false
> +  - if:
> +      required:
> +        - output-low
> +    then:
> +      properties:
> +        output-high: false
> +  - if:
> +      required:
> +        - low-power-enable
> +    then:
> +      properties:
> +        low-power-disable: false
> +  - if:
> +      required:
> +        - input-schmitt-disable
> +    then:
> +      properties:
> +        input-schmitt-enable: false
> +        input-schmitt-microvolt: false
> +  - if:
> +      required:
> +        - drive-open-source
> +    then:
> +      properties:
> +        drive-open-drain: false
> +  - if:
> +      anyOf:
> +        - required:
> +            - bias-disable
> +        - required:
> +            - bias-high-impedance
> +        - required:
> +            - bias-hold
> +        - required:
> +            - bias-up
> +        - required:
> +            - bias-down
> +        - required:
> +            - bias-pull-pin-default
> +    then:
> +      oneOf:
> +        - required:
> +            - bias-disable
> +        - required:
> +            - bias-high-impedance
> +        - required:
> +            - bias-hold
> +        - required:
> +            - bias-up
> +        - required:
> +            - bias-down
> +        - required:
> +            - bias-pull-pin-default
> +
>  additionalProperties: true

I was looking at the kernel part of this today, trying to figure out
where it would make sense to actually check this, but I'm not super keen
on what has to be done. I think doing it in parse_dt_cfg() makes the
most sense, setting flags if the property is one we care about during
the loop and then checking mutual exclusion at the end based on the
flags? The gpio example you gave has it easy, since they already appear
to have these things stored in flag properties.
Is there somewhere else, in addition to creating the config from dt that
this would have to be checked?

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