[RFC PATCH 1/2] dt-bindings: pincfg-node: Introduce an overridable way to set bias on pins

[RFC PATCH 1/2] dt-bindings: pincfg-node: Introduce an overridable way to set bias on pins

[Konrad Dybcio]

We came to a point where we sometimes we support a few dozen boards
with a given SoC. Sometimes, we have to take into consideration
configurations which deviate rather significatly from the reference
or most common designs. In the context of pinctrl, this often comes
down to wildly different pin configurations. While pins, function and
drive-strength are easily overridable, the (mostly) boolean properties
associated with setting bias, aren't. This wouldn't be much of a
problem if they didn't differ between boards so often, preventing us
from having a "nice" baseline setup without inevitably having to go
with an ugly /delete-property/. Introduce bias-type, a bias-type-
specific property and clone the pinconf-generic type enum into
dt-bindings to allow for setting the bias in an easily overridable
manner such as:

// SoC DT
i2c0_pin: i2c0-pin-state {
	pins = "gpio10";
	function = "gpio";
	bias-type = <BIAS_PULL_UP>;
};

// Deviant board DT
&i2c0_pin {
	bias-type = <BIAS_HIGH_IMPEDANCE>;
};

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 .../bindings/pinctrl/pincfg-node.yaml         |  4 ++
 include/dt-bindings/pinctrl/pinconf-generic.h | 40 +++++++++++++++++++
 2 files changed, 44 insertions(+)
 create mode 100644 include/dt-bindings/pinctrl/pinconf-generic.h

diff --git a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
index be81ed22a036..d4ea563d283e 100644
--- a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
@@ -51,6 +51,10 @@ properties:
     description: use pin-default pull state. Takes as optional argument on
       hardware supporting it the pull strength in Ohm.
 
+  bias-type:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Use the specified bias type.
+
   drive-push-pull:
     oneOf:
       - type: boolean
diff --git a/include/dt-bindings/pinctrl/pinconf-generic.h b/include/dt-bindings/pinctrl/pinconf-generic.h
new file mode 100644
index 000000000000..7d9c7d8f9105
--- /dev/null
+++ b/include/dt-bindings/pinctrl/pinconf-generic.h
@@ -0,0 +1,40 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ */
+
+#ifndef _DT_BINDINGS_PINCTRL_PINCONF_GENERIC_H
+#define _DT_BINDINGS_PINCTRL_PINCONF_GENERIC_H
+
+#define BIAS_BUS_HOLD			0
+#define BIAS_DISABLE			1
+#define BIAS_HIGH_IMPEDANCE		2
+#define BIAS_PULL_DOWN			3
+#define BIAS_PULL_PIN_DEFAULT		4
+#define BIAS_PULL_UP			5
+#define DRIVE_OPEN_DRAIN		6
+#define DRIVE_OPEN_SOURCE		7
+#define DRIVE_PUSH_PULL			8
+#define DRIVE_STRENGTH			9
+#define DRIVE_STRENGTH_UA		10
+#define INPUT_DEBOUNCE			11
+#define INPUT_ENABLE			12
+#define INPUT_SCHMITT			13
+#define INPUT_SCHMITT_ENABLE		14
+#define MODE_LOW_POWER			15
+#define MODE_PWM			16
+#define OUTPUT				17
+#define OUTPUT_ENABLE			18
+#define OUTPUT_IMPEDANCE_OHMS		19
+#define PERSIST_STATE			20
+#define POWER_SOURCE			21
+#define SKEW_DELAY			22
+#define SLEEP_HARDWARE_STATE		23
+#define SLEW_RATE			24
+#define PIN_CONFIG_END			0x7F
+#define PIN_CONFIG_MAX			0xFF
+
+#endif
-- 
2.39.1

[RFC PATCH 2/2] pinctrl: pinconf-generic: Add an overridable way to set bias property

[Konrad Dybcio]

We came to a point where we sometimes we support a few dozen boards
with a given SoC. Sometimes, we have to take into consideration
configurations which deviate rather significatly from the reference
or most common designs. In the context of pinctrl, this often comes
down to wildly different pin configurations. While pins, function and
drive-strength are easily overridable, the (mostly) boolean properties
associated with setting bias, aren't. This wouldn't be much of a
problem if they didn't differ between boards so often, preventing us
from having a "nice" baseline setup without inevitably having to go
with an ugly /delete-property/.

Introduce logic to handle bias-type, a property which sets a single
boolean type of bias on the pin (more than one type of BIAS_ does not
make sense, anyway) to make it easily overridable.

Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
---
 drivers/pinctrl/pinconf-generic.c       | 35 ++++++++++++++++++++++---
 include/linux/pinctrl/pinconf-generic.h |  1 +
 2 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c
index 365c4b0ca465..b99c2a85486e 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -206,11 +206,38 @@ static void parse_dt_cfg(struct device_node *np,
 			 unsigned int count, unsigned long *cfg,
 			 unsigned int *ncfg)
 {
-	int i;
+	int i, ret;
+	u32 val;
+
+	/* Let's assume only one type of bias is used.. as it should be.. */
+	ret = of_property_read_u32(np, "bias-type", &val);
+	if (!ret) {
+		/* Bias properties end at idx PIN_CONFIG_BIAS_PULL_UP */
+		if (ret > PIN_CONFIG_BIAS_PULL_UP) {
+			pr_err("invalid type: %u\n", val);
+			goto generic_parse;
+		}
 
-	for (i = 0; i < count; i++) {
-		u32 val;
-		int ret;
+		pr_debug("found bias type %u\n", val);
+		/*
+		 * Properties between PIN_CONFIG_BIAS_PULL_DOWN and PIN_CONFIG_BIAS_PULL_UP
+		 * have a default value of one, others default to zero.
+		 */
+		cfg[*ncfg] = pinconf_to_config_packed(val, val >= PIN_CONFIG_BIAS_PULL_DOWN);
+		(*ncfg)++;
+
+		/* Start the generic property read loop where bias properties end. */
+		i = PIN_CONFIG_DRIVE_OPEN_DRAIN;
+	} else {
+		/*
+		 * If we don't set bias through bias-type, search for all DT
+		 * properties like nothing ever happened.
+		 */
+generic_parse:
+		i = 0;
+	}
+
+	for (; i < count; i++) {
 		const struct pinconf_generic_params *par = &params[i];
 
 		ret = of_property_read_u32(np, par->property, &val);
diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index d74b7a4ea154..bcf68ba1ea46 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -117,6 +117,7 @@ struct pinctrl_map;
  *	presented using the packed format.
  */
 enum pin_config_param {
+	/* Keep in sync with dt-bindings/pinctrl/pinconf-generic.h! */
 	PIN_CONFIG_BIAS_BUS_HOLD,
 	PIN_CONFIG_BIAS_DISABLE,
 	PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
-- 
2.39.1

Re: [RFC PATCH 1/2] dt-bindings: pincfg-node: Introduce an overridable way to set bias on pins

[Linus Walleij]

On Mon, Jan 30, 2023 at 5:54 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:

> We came to a point where we sometimes we support a few dozen boards
> with a given SoC. Sometimes, we have to take into consideration
> configurations which deviate rather significatly from the reference
> or most common designs. In the context of pinctrl, this often comes
> down to wildly different pin configurations. While pins, function and
> drive-strength are easily overridable, the (mostly) boolean properties
> associated with setting bias, aren't. This wouldn't be much of a
> problem if they didn't differ between boards so often, preventing us
> from having a "nice" baseline setup without inevitably having to go
> with an ugly /delete-property/.

I see what the problem is.

Have you considered pulling out *all* the pin config for a certain
reference design into its own .dtsi file, simply? And then not include
that to the next product.

This pattern is pretty common.

> Introduce bias-type, a bias-type-
> specific property and clone the pinconf-generic type enum into
> dt-bindings to allow for setting the bias in an easily overridable
> manner such as:
>
> // SoC DT
> i2c0_pin: i2c0-pin-state {
>         pins = "gpio10";
>         function = "gpio";
>         bias-type = <BIAS_PULL_UP>;
> };
>
> // Deviant board DT
> &i2c0_pin {
>         bias-type = <BIAS_HIGH_IMPEDANCE>;
> };
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>

The idea is pretty straight-forward.

But it applies to systems already using the bool flags. So what do
we do the day we manage to have:

{
    bias-type = <BIAS_HIGH_IMPEDANCE>;
    bias-pull-up;
};

As you see this makes it necessary to author some really nasty
YAML to make sure this cannot happen or everyone has to make
a runtime check for it.

Another problem is that I was just discussing with Bjorn for some
specific i2c pull-up, was actually using the argument for
bias-pull-up with a parameter:

bias-pull-up = <8000000>;  // 8kOhm pull-up

Not to mention that other platforms than qcom use this and
qcom use it for drive-strength I think?

+#define DRIVE_STRENGTH                 9
+#define DRIVE_STRENGTH_UA              10

drive-strength = <8>; // 8mA drive strength

bias-type = <DRIVE_STRENGTH>;

OK where do I put my 8 mA now?

Yours,
Linus Walleij

Re: [RFC PATCH 1/2] dt-bindings: pincfg-node: Introduce an overridable way to set bias on pins

[Konrad Dybcio]

On 31.01.2023 00:10, Linus Walleij wrote:
> On Mon, Jan 30, 2023 at 5:54 PM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> 
>> We came to a point where we sometimes we support a few dozen boards
>> with a given SoC. Sometimes, we have to take into consideration
>> configurations which deviate rather significatly from the reference
>> or most common designs. In the context of pinctrl, this often comes
>> down to wildly different pin configurations. While pins, function and
>> drive-strength are easily overridable, the (mostly) boolean properties
>> associated with setting bias, aren't. This wouldn't be much of a
>> problem if they didn't differ between boards so often, preventing us
>> from having a "nice" baseline setup without inevitably having to go
>> with an ugly /delete-property/.
> 
> I see what the problem is.
> 
> Have you considered pulling out *all* the pin config for a certain
> reference design into its own .dtsi file, simply? And then not include
> that to the next product.
> 
> This pattern is pretty common.
It's *a* solution, but we already have some soc-pmics.dtsi-s which
include PMICs and reference regulator-to-peripherals assignments.
Adding another soc-reference-pins.dtsi would open the doors for
other soc-abcxyz.dtsi-s which would in turn lead to exchanging the
problem of redefining the same sets of properties 50 times to
having to include 20 device trees for each and every device. Allow
that and we basically have msm-3.10 except in torvalds/linux.

Adding to that, some devices are 98% compliant with the reference
designs, but randomly swap out one pin etc.

> 
>> Introduce bias-type, a bias-type-
>> specific property and clone the pinconf-generic type enum into
>> dt-bindings to allow for setting the bias in an easily overridable
>> manner such as:
>>
>> // SoC DT
>> i2c0_pin: i2c0-pin-state {
>>         pins = "gpio10";
>>         function = "gpio";
>>         bias-type = <BIAS_PULL_UP>;
>> };
>>
>> // Deviant board DT
>> &i2c0_pin {
>>         bias-type = <BIAS_HIGH_IMPEDANCE>;
>> };
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> 
> The idea is pretty straight-forward.
> 
> But it applies to systems already using the bool flags. So what do
> we do the day we manage to have:
> 
> {
>     bias-type = <BIAS_HIGH_IMPEDANCE>;
>     bias-pull-up;
> };
> 
> As you see this makes it necessary to author some really nasty
> YAML to make sure this cannot happen or everyone has to make
> a runtime check for it.
oneOf:
- bias-type
- enum:
    - bias-pull-down
    - bias-...

Doesn't seem really nasty to me.. And a runtime check is basically
in place, as I made sure the code in 2/2 prefers bias-type and falls
back to the existing mechanism if it's not found.

> 
> Another problem is that I was just discussing with Bjorn for some
> specific i2c pull-up, was actually using the argument for
> bias-pull-up with a parameter:
> 
> bias-pull-up = <8000000>;  // 8kOhm pull-up
This is an easy fix. Count the elements and if there's more than
one, pass the second one as a value. Both of these things already
have common of_ functions made just for that.

> 
> Not to mention that other platforms than qcom use this and
> qcom use it for drive-strength I think?
> 
> +#define DRIVE_STRENGTH                 9
> +#define DRIVE_STRENGTH_UA              10
> 
> drive-strength = <8>; // 8mA drive strength
> 
> bias-type = <DRIVE_STRENGTH>;
> 
> OK where do I put my 8 mA now?
If you look at the 2/2 patch, this property only reads BIAS_
values, which can't coexist anyway. I copied the entire enum
for completeness. But if we were to go with this approach,
having one for (in|out)put-*, where only having one of them
at a time makes sense could be beneficial too..

Konrad
> 
> Yours,
> Linus Walleij

Re: [RFC PATCH 1/2] dt-bindings: pincfg-node: Introduce an overridable way to set bias on pins

[Linus Walleij]

On Tue, Jan 31, 2023 at 12:50 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:

> > +#define DRIVE_STRENGTH                 9
> > +#define DRIVE_STRENGTH_UA              10
> >
> > drive-strength = <8>; // 8mA drive strength
> >
> > bias-type = <DRIVE_STRENGTH>;
> >
> > OK where do I put my 8 mA now?
> >
> If you look at the 2/2 patch, this property only reads BIAS_
> values, which can't coexist anyway.

Well the DT bindings have to be consistent and clear on their
own, no matter how Linux implements it.

But I'm sure you can make YAML verification such that it is
impossible to use both schemes at the same time, and it's not
like I don't understand what you're getting at.

What I need as input is mainly the DT bindings people opinion
on introducing another orthogonal way of doing something
that is already possible to do another way, just more convenient.
Because that is essentially what is happening here.

Yours,
Linus Walleij

Re: [RFC PATCH 1/2] dt-bindings: pincfg-node: Introduce an overridable way to set bias on pins

[Rob Herring]

On Tue, Jan 31, 2023 at 02:21:38PM +0100, Linus Walleij wrote:
> On Tue, Jan 31, 2023 at 12:50 AM Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> 
> > > +#define DRIVE_STRENGTH                 9
> > > +#define DRIVE_STRENGTH_UA              10
> > >
> > > drive-strength = <8>; // 8mA drive strength
> > >
> > > bias-type = <DRIVE_STRENGTH>;
> > >
> > > OK where do I put my 8 mA now?
> > >
> > If you look at the 2/2 patch, this property only reads BIAS_
> > values, which can't coexist anyway.
> 
> Well the DT bindings have to be consistent and clear on their
> own, no matter how Linux implements it.
> 
> But I'm sure you can make YAML verification such that it is
> impossible to use both schemes at the same time, and it's not
> like I don't understand what you're getting at.

We already don't enforce mutually exclusive combinations. Perhaps 
someone wants to fix that first?

> What I need as input is mainly the DT bindings people opinion
> on introducing another orthogonal way of doing something
> that is already possible to do another way, just more convenient.
> Because that is essentially what is happening here.

It's really a 3rd way we're adding because the existing properties have 
2 forms which IMO is worse than 2 disjoint ways of doing it. And since 
this new way can't represent some cases, I don't think it is an 
improvement. 

Rob