Re: [PATCH v4 1/2] dt-bindings: pinctrl: qcom: Add SM8350 pinctrl bindings

[Bjorn Andersson <bjorn.andersson@linaro.org>]

On Tue 12 Jan 20:56 CST 2021, Rob Herring wrote:

> On Thu, Jan 07, 2021 at 11:17:22AM -0600, Bjorn Andersson wrote:
> > On Tue 05 Jan 23:49 CST 2021, Vinod Koul wrote:
> > 
> > > Add device tree binding Documentation details for Qualcomm SM8350
> > > pinctrl driver.
> > > 
> > > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > > ---
> > >  .../bindings/pinctrl/qcom,sm8350-tlmm.yaml    | 149 ++++++++++++++++++
> > >  1 file changed, 149 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > > new file mode 100644
> > > index 000000000000..abdafd25bfc2
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/pinctrl/qcom,sm8350-tlmm.yaml
> > > @@ -0,0 +1,149 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/pinctrl/qcom,sm8350-tlmm.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Qualcomm Technologies, Inc. SM8350 TLMM block
> > > +
> > > +maintainers:
> > > +  - Vinod Koul <vkoul@kernel.org>
> > > +
> > > +description: |
> > > +  This binding describes the Top Level Mode Multiplexer block found in the
> > > +  SM8350 platform.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: qcom,sm8350-tlmm
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  interrupt-controller: true
> > > +
> > > +  '#interrupt-cells':
> > > +    description: Specifies the PIN numbers and Flags, as defined in
> > > +      include/dt-bindings/interrupt-controller/irq.h
> > > +    const: 2
> > > +
> > > +  gpio-controller: true
> > > +
> > > +  '#gpio-cells':
> > > +    description: Specifying the pin number and flags, as defined in
> > > +      include/dt-bindings/gpio/gpio.h
> > > +    const: 2
> > > +
> > > +  gpio-ranges:
> > > +    maxItems: 1
> > > +
> > > +  gpio-reserved-ranges:
> > > +    maxItems: 1
> > > +
> > > +#PIN CONFIGURATION NODES
> > > +patternProperties:
> > > +  '-pinmux$':
> > 
> > I believe that what Rob was asking for was the matter of describing the
> > mux and config subnodes under this one. But I don't know really how to
> > express this, because the following are all valid:
> > 
> > default_state: default-state {
> > 	pins = "gpio1";
> > 	bias-disable;
> > };
> > 
> > default_state: default-state {
> > 	rx {
> > 		pins = "gpio1";
> > 		function = "gpio";
> > 		bias-disable;
> > 	};
> > };
> > 
> > default_state: default-state {
> > 	pinmux {
> > 		pins = "gpio1";
> > 		function = "gpio";
> > 	};
> > 
> > 	pinconf {
> > 		pins = "gpio1";
> > 		bias-disable;
> > 	};
> > };
> > 
> > I.e. the properties described here applies either to this node directly,
> > or any subnodes (1 level) down.
> 
> Why!?
> 

That's is how the generic pinctrl framework in Linux has parsed these
nodes for the last several years, so I expect all pinctrl bindings to
show this.

And in particular, rather than forcing people to use the third form
above the free naming of the subnodes allows to describe an entire 4 pin
UART in a single pin state (the last requires 3-4 nodes + duplicates for
sleep state), and for single gpio things the first one doesn't force
people to make up phony names - which typically lands them in the last
case (using pinmux/pinconf).

> You can create a definition and reuse it. Something like this:
> 
> $defs:
>   pin-node:
>     type: object
>     properties:
>       ...
> 
> patternProperties:
>   '-state$':
>     oneOf:
>       - $ref: #/$defs/pin-node
> 
>       - patternProperties:
>           '.*':
>             $ref: #/$defs/pin-node

So I presume then that it would make sense to convert
{pinmux,pinconf}-node.yaml this form and then in our binding we should
somehow just refer to these.

But what's the appropriate method of extending and limiting pin-node? We
have additional constraints and not all the properties are valid for the
Qualcomm TLMM.

Regards,
Bjorn

> 
> 
> Rob