[PATCH 0/2] Add Apple macsmc GPIO support

[PATCH 0/2] Add Apple macsmc GPIO support

[Russell King (Oracle)]

Hi,

This series adds support for the Apple Mac SMC GPIO driver that was
previously posted before the last merge window. I believe all previous
review points have been addressed.

This series is smaller than the previous posting because I'm splitting
this up to allow progress to be made; this driver is dependent on the
Apple SMC driver in order to be buildable and usable. It is expected
that this Apple SMC driver will be merged via Lee's MFD tree. The
Kconfig dependencies this driver from causing build issues until that
driver is also merged, so I believe it is appropriate to send this
separately.

There is no issue if we wish to delay applying this until the core
driver has been accepted by Lee.

The series is made up of two patches - one patch for the bindings,
which now include two compatibles, one specific and one generic. The
second patch adds the actual driver itself.

 .../devicetree/bindings/gpio/gpio-macsmc.yaml      |  28 +++
 drivers/gpio/Kconfig                               |  11 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-macsmc.c                         | 245 +++++++++++++++++++++
 4 files changed, 285 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
 create mode 100644 drivers/gpio/gpio-macsmc.c

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

[PATCH 1/2] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

Add the DT binding for the Apple Mac System Management Controller GPIOs.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml

diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
new file mode 100644
index 000000000000..a3883d62292d
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
@@ -0,0 +1,28 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Mac System Management Controller GPIO
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description:
+  This describes the binding for the Apple Mac System Management Controller
+  GPIO block.
+
+properties:
+  compatible:
+    allOf:
+      - enum:
+          - apple,t8103-smc
+      - const: apple,smc-gpio
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+additionalProperties: false
-- 
2.30.2

[PATCH 2/2] gpio: Add new gpio-macsmc driver for Apple Macs

[Russell King]

From: Hector Martin <marcan@marcan.st>

This driver implements the GPIO service on top of the SMC framework
on Apple Mac machines. In particular, these are the GPIOs present in the
PMU IC which are used to control power to certain on-board devices.

Although the underlying hardware supports various pin config settings
(input/output, open drain, etc.), this driver does not implement that
functionality and leaves it up to the firmware to configure things
properly. We also don't yet support interrupts/events. This is
sufficient for device power control, which is the only thing we need to
support at this point. More features will be implemented when needed.

To our knowledge, only Apple Silicon Macs implement this SMC feature.

Signed-off-by: Hector Martin <marcan@marcan.st>
Reviewed-by: Bartosz Golaszewski <brgl@bgdev.pl>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Sven Peter <sven@svenpeter.dev>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/gpio/Kconfig       |  11 ++
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-macsmc.c | 245 +++++++++++++++++++++++++++++++++++++
 3 files changed, 257 insertions(+)
 create mode 100644 drivers/gpio/gpio-macsmc.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0642f579196f..9b87f5ebe1b9 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1292,6 +1292,17 @@ config GPIO_LP87565
 	  This driver can also be built as a module. If so, the module will be
 	  called gpio-lp87565.
 
+config GPIO_MACSMC
+	tristate "Apple Mac SMC GPIO"
+	depends on APPLE_SMC
+	default ARCH_APPLE
+	help
+	  Support for GPIOs controlled by the SMC microcontroller on Apple Mac
+	  systems.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called gpio-macsmc.
+
 config GPIO_MADERA
 	tristate "Cirrus Logic Madera class codecs"
 	depends on PINCTRL_MADERA
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index a0985d30f51b..a401a467c6f4 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -83,6 +83,7 @@ obj-$(CONFIG_GPIO_LP873X)		+= gpio-lp873x.o
 obj-$(CONFIG_GPIO_LP87565)		+= gpio-lp87565.o
 obj-$(CONFIG_GPIO_LPC18XX)		+= gpio-lpc18xx.o
 obj-$(CONFIG_GPIO_LPC32XX)		+= gpio-lpc32xx.o
+obj-$(CONFIG_GPIO_MACSMC)		+= gpio-macsmc.o
 obj-$(CONFIG_GPIO_MADERA)		+= gpio-madera.o
 obj-$(CONFIG_GPIO_MAX3191X)		+= gpio-max3191x.o
 obj-$(CONFIG_GPIO_MAX7300)		+= gpio-max7300.o
diff --git a/drivers/gpio/gpio-macsmc.c b/drivers/gpio/gpio-macsmc.c
new file mode 100644
index 000000000000..24ec98ad18f7
--- /dev/null
+++ b/drivers/gpio/gpio-macsmc.c
@@ -0,0 +1,245 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SMC GPIO driver
+ * Copyright The Asahi Linux Contributors
+ *
+ * This driver implements basic SMC PMU GPIO support that can read inputs
+ * and write outputs. Mode changes and IRQ config are not yet implemented.
+ */
+
+#include <linux/bitmap.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/macsmc.h>
+
+#define MAX_GPIO 64
+
+/*
+ * Commands 0-6 are, presumably, the intended API.
+ * Command 0xff lets you get/set the pin configuration in detail directly,
+ * but the bit meanings seem not to be stable between devices/PMU hardware
+ * versions.
+ *
+ * We're going to try to make do with the low commands for now.
+ * We don't implement pin mode changes at this time.
+ */
+
+#define CMD_ACTION	(0 << 24)
+#define CMD_OUTPUT	(1 << 24)
+#define CMD_INPUT	(2 << 24)
+#define CMD_PINMODE	(3 << 24)
+#define CMD_IRQ_ENABLE	(4 << 24)
+#define CMD_IRQ_ACK	(5 << 24)
+#define CMD_IRQ_MODE	(6 << 24)
+#define CMD_CONFIG	(0xff << 24)
+
+#define MODE_INPUT	0
+#define MODE_OUTPUT	1
+#define MODE_VALUE_0	0
+#define MODE_VALUE_1	2
+
+#define IRQ_MODE_HIGH		0
+#define IRQ_MODE_LOW		1
+#define IRQ_MODE_RISING		2
+#define IRQ_MODE_FALLING	3
+#define IRQ_MODE_BOTH		4
+
+#define CONFIG_MASK	GENMASK(23, 16)
+#define CONFIG_VAL	GENMASK(7, 0)
+
+#define CONFIG_OUTMODE	GENMASK(7, 6)
+#define CONFIG_IRQMODE	GENMASK(5, 3)
+#define CONFIG_PULLDOWN	BIT(2)
+#define CONFIG_PULLUP	BIT(1)
+#define CONFIG_OUTVAL	BIT(0)
+
+/*
+ * Output modes seem to differ depending on the PMU in use... ?
+ * j274 / M1 (Sera PMU):
+ *   0 = input
+ *   1 = output
+ *   2 = open drain
+ *   3 = disable
+ * j314 / M1Pro (Maverick PMU):
+ *   0 = input
+ *   1 = open drain
+ *   2 = output
+ *   3 = ?
+ */
+
+struct macsmc_gpio {
+	struct device *dev;
+	struct apple_smc *smc;
+	struct gpio_chip gc;
+
+	int first_index;
+};
+
+static int macsmc_gpio_nr(smc_key key)
+{
+	int low = hex_to_bin(key & 0xff);
+	int high = hex_to_bin((key >> 8) & 0xff);
+
+	if (low < 0 || high < 0)
+		return -1;
+
+	return low | (high << 4);
+}
+
+static int macsmc_gpio_key(unsigned int offset)
+{
+	return _SMC_KEY("gP\0\0") | hex_asc_hi(offset) << 8 | hex_asc_lo(offset);
+}
+
+static int macsmc_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
+	smc_key key = macsmc_gpio_key(offset);
+	u32 val;
+	int ret;
+
+	/* First try reading the explicit pin mode register */
+	ret = apple_smc_rw_u32(smcgp->smc, key, CMD_PINMODE, &val);
+	if (!ret)
+		return (val & MODE_OUTPUT) ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+
+	/*
+	 * Less common IRQ configs cause CMD_PINMODE to fail, and so does open drain mode.
+	 * Fall back to reading IRQ mode, which will only succeed for inputs.
+	 */
+	ret = apple_smc_rw_u32(smcgp->smc, key, CMD_IRQ_MODE, &val);
+	return ret ? GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;
+}
+
+static int macsmc_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
+	smc_key key = macsmc_gpio_key(offset);
+	u32 cmd, val;
+	int ret;
+
+	ret = macsmc_gpio_get_direction(gc, offset);
+	if (ret < 0)
+		return ret;
+
+	if (ret == GPIO_LINE_DIRECTION_OUT)
+		cmd = CMD_OUTPUT;
+	else
+		cmd = CMD_INPUT;
+
+	ret = apple_smc_rw_u32(smcgp->smc, key, cmd, &val);
+	if (ret < 0)
+		return ret;
+
+	return val ? 1 : 0;
+}
+
+static void macsmc_gpio_set(struct gpio_chip *gc, unsigned int offset, int value)
+{
+	struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
+	smc_key key = macsmc_gpio_key(offset);
+	int ret;
+
+	value |= CMD_OUTPUT;
+	ret = apple_smc_write_u32(smcgp->smc, key, CMD_OUTPUT | value);
+	if (ret < 0)
+		dev_err(smcgp->dev, "GPIO set failed %p4ch = 0x%x\n", &key, value);
+}
+
+static int macsmc_gpio_init_valid_mask(struct gpio_chip *gc,
+				       unsigned long *valid_mask, unsigned int ngpios)
+{
+	struct macsmc_gpio *smcgp = gpiochip_get_data(gc);
+	int count;
+	int i;
+
+	count = apple_smc_get_key_count(smcgp->smc) - smcgp->first_index;
+	if (count > MAX_GPIO)
+		count = MAX_GPIO;
+
+	bitmap_zero(valid_mask, ngpios);
+
+	for (i = 0; i < count; i++) {
+		int ret, gpio_nr;
+		smc_key key;
+
+		ret = apple_smc_get_key_by_index(smcgp->smc, smcgp->first_index + i, &key);
+		if (ret < 0)
+			return ret;
+
+		if (key > SMC_KEY(gPff))
+			break;
+
+		gpio_nr = macsmc_gpio_nr(key);
+		if (gpio_nr < 0 || gpio_nr > MAX_GPIO) {
+			dev_err(smcgp->dev, "Bad GPIO key %p4ch\n", &key);
+			continue;
+		}
+
+		set_bit(gpio_nr, valid_mask);
+	}
+
+	return 0;
+}
+
+static int macsmc_gpio_probe(struct platform_device *pdev)
+{
+	struct macsmc_gpio *smcgp;
+	struct apple_smc *smc = dev_get_drvdata(pdev->dev.parent);
+	smc_key key;
+	int ret;
+
+	smcgp = devm_kzalloc(&pdev->dev, sizeof(*smcgp), GFP_KERNEL);
+	if (!smcgp)
+		return -ENOMEM;
+
+	smcgp->dev = &pdev->dev;
+	smcgp->smc = smc;
+	smcgp->first_index = apple_smc_find_first_key_index(smc, SMC_KEY(gP00));
+
+	if (smcgp->first_index >= apple_smc_get_key_count(smc))
+		return -ENODEV;
+
+	ret = apple_smc_get_key_by_index(smc, smcgp->first_index, &key);
+	if (ret < 0)
+		return ret;
+
+	if (key > macsmc_gpio_key(MAX_GPIO - 1))
+		return -ENODEV;
+
+	dev_info(smcgp->dev, "First GPIO key: %p4ch\n", &key);
+
+	smcgp->gc.label = "macsmc-pmu-gpio";
+	smcgp->gc.owner = THIS_MODULE;
+	smcgp->gc.get = macsmc_gpio_get;
+	smcgp->gc.set = macsmc_gpio_set;
+	smcgp->gc.get_direction = macsmc_gpio_get_direction;
+	smcgp->gc.init_valid_mask = macsmc_gpio_init_valid_mask;
+	smcgp->gc.can_sleep = true;
+	smcgp->gc.ngpio = MAX_GPIO;
+	smcgp->gc.base = -1;
+	smcgp->gc.parent = &pdev->dev;
+
+	return devm_gpiochip_add_data(&pdev->dev, &smcgp->gc, smcgp);
+}
+
+static const struct of_device_id macsmc_gpio_of_table[] = {
+	{ .compatible = "apple,smc-gpio", },
+	{}
+};
+MODULE_DEVICE_TABLE(of, macsmc_gpio_of_table);
+
+static struct platform_driver macsmc_gpio_driver = {
+	.driver = {
+		.name = "macsmc-gpio",
+		.of_match_table = macsmc_gpio_of_table,
+	},
+	.probe = macsmc_gpio_probe,
+};
+module_platform_driver(macsmc_gpio_driver);
+
+MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_DESCRIPTION("Apple SMC GPIO driver");
+MODULE_ALIAS("platform:macsmc-gpio");
-- 
2.30.2

Re: [PATCH 1/2] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 27/10/2022 13:03, Russell King (Oracle) wrote:
> Add the DT binding for the Apple Mac System Management Controller GPIOs.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> new file mode 100644
> index 000000000000..a3883d62292d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml


Filename based on compatible, so "apple,smc-gpio.yaml"

> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple Mac System Management Controller GPIO
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description:
> +  This describes the binding for the Apple Mac System Management Controller

Drop "This describes the binding for"

> +  GPIO block.
> +
> +properties:
> +  compatible:
> +    allOf:

That's not proper syntax. Look at other examples (e.g. Apple bindings)
doing it. Probably you wanted items here.

> +      - enum:
> +          - apple,t8103-smc
> +      - const: apple,smc-gpio
> +
> +  gpio-controller: true
> +
> +  '#gpio-cells':
> +    const: 2

Missing required properties. Start from new bindings or example-schema.

> +
> +additionalProperties: false

Missing example, it's necessary to validate these.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

On Thu, Oct 27, 2022 at 03:53:25PM -0400, Krzysztof Kozlowski wrote:
> On 27/10/2022 13:03, Russell King (Oracle) wrote:
> > Add the DT binding for the Apple Mac System Management Controller GPIOs.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
> >  1 file changed, 28 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> > new file mode 100644
> > index 000000000000..a3883d62292d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> 
> Filename based on compatible, so "apple,smc-gpio.yaml"

Many of the other yaml files in gpio/ are named as such.

> > @@ -0,0 +1,28 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Apple Mac System Management Controller GPIO
> > +
> > +maintainers:
> > +  - Hector Martin <marcan@marcan.st>
> > +
> > +description:
> > +  This describes the binding for the Apple Mac System Management Controller
> 
> Drop "This describes the binding for"
> 
> > +  GPIO block.
> > +
> > +properties:
> > +  compatible:
> > +    allOf:
> 
> That's not proper syntax. Look at other examples (e.g. Apple bindings)
> doing it. Probably you wanted items here.

Really? You're joking. I had sent an email to Rob to ask how this should
be done because my first guess spat out unhelpful error messages from
dt_bindings_check, and this is the best I could come up with based on
other "examples".

I tried "- items:" but that made no difference - dt_bindings_check spat
errors, so that's clearly incorrect. Specifically, I tried:

properties:
  compatible:
    - items:
        - enum:
	    - apple,t8103-smc
	- const: apple,smc-gpio

That doesn't work:

Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml: properties:compatible: [{'items': [{'const': 'apple,t8103-smc'}, {'const': 'apple,smc-gpio'}]}] is not of type 'object', 'boolean'  from schema $id: http://json-schema.org/draft-07/schema#

> > +      - enum:
> > +          - apple,t8103-smc
> > +      - const: apple,smc-gpio
> > +
> > +  gpio-controller: true
> > +
> > +  '#gpio-cells':
> > +    const: 2
> 
> Missing required properties. Start from new bindings or example-schema.

What's missing? All the other bindings that I see follow this pattern.

> > +
> > +additionalProperties: false
> 
> Missing example, it's necessary to validate these.

Documentation states that examples are optional according to the
"writing-schema" documentation.

Honestly, I find this YAML stuff extremely difficult, especially given
the lack of documentation on how to write it and the cryptic error
messages from the tooling. It's impossible to get it right before
submitting it - and I suspect from what I see above, it's impossible
for reviewers to know what is correct either, since some of what you've
said above appears to be wrong!

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/2] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 27/10/2022 17:00, Russell King (Oracle) wrote:
> On Thu, Oct 27, 2022 at 03:53:25PM -0400, Krzysztof Kozlowski wrote:
>> On 27/10/2022 13:03, Russell King (Oracle) wrote:
>>> Add the DT binding for the Apple Mac System Management Controller GPIOs.
>>>
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
>>>  1 file changed, 28 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>> new file mode 100644
>>> index 000000000000..a3883d62292d
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>
>> Filename based on compatible, so "apple,smc-gpio.yaml"
> 
> Many of the other yaml files in gpio/ are named as such.

Poor patterns, inconsistencies or even bugs like to copy themselves and
it is never an argument.

The convention for all bindings is to use vendor,device.yaml, matching
the compatible when applicable.

> 
>>> @@ -0,0 +1,28 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Apple Mac System Management Controller GPIO
>>> +
>>> +maintainers:
>>> +  - Hector Martin <marcan@marcan.st>
>>> +
>>> +description:
>>> +  This describes the binding for the Apple Mac System Management Controller
>>
>> Drop "This describes the binding for"
>>
>>> +  GPIO block.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    allOf:
>>
>> That's not proper syntax. Look at other examples (e.g. Apple bindings)
>> doing it. Probably you wanted items here.
> 
> Really? You're joking. 

No. If you look at example-schema then answer should be obvious, so why
do you think I am joking?

> I had sent an email to Rob to ask how this should
> be done because my first guess spat out unhelpful error messages from
> dt_bindings_check, and this is the best I could come up with based on
> other "examples".
> 
> I tried "- items:" but that made no difference - dt_bindings_check spat
> errors, so that's clearly incorrect. Specifically, I tried:
> 
> properties:
>   compatible:
>     - items:
>         - enum:
> 	    - apple,t8103-smc
> 	- const: apple,smc-gpio
> 
> That doesn't work:

Of course, because "-" means list, so "- items" is not correct.

Where do you see such pattern? Anywhere following compatible? No. There
is no. You just invented something instead of using many, many existing
examples.


> 
> Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml: properties:compatible: [{'items': [{'const': 'apple,t8103-smc'}, {'const': 'apple,smc-gpio'}]}] is not of type 'object', 'boolean'  from schema $id: http://json-schema.org/draft-07/schema#
> 
>>> +      - enum:
>>> +          - apple,t8103-smc
>>> +      - const: apple,smc-gpio
>>> +
>>> +  gpio-controller: true
>>> +
>>> +  '#gpio-cells':
>>> +    const: 2
>>
>> Missing required properties. Start from new bindings or example-schema.
> 
> What's missing? All the other bindings that I see follow this pattern.

No. All other bindings have list of required properties. Only yours do
not have.

> 
>>> +
>>> +additionalProperties: false
>>
>> Missing example, it's necessary to validate these.
> 
> Documentation states that examples are optional according to the
> "writing-schema" documentation.

Yes, but without it we cannot validate the bindings.

> 
> Honestly, I find this YAML stuff extremely difficult, especially given
> the lack of documentation on how to write it and the cryptic error
> messages from the tooling. It's impossible to get it right before
> submitting it - and I suspect from what I see above, it's impossible
> for reviewers to know what is correct either, since some of what you've
> said above appears to be wrong!

I would say it is doable - copy example-schema or recent device specific
schema and customize it... But you started adding some weird stuff which
was never, never in other bindings.

I would also say that C stuff is extremely difficult... Impossible to
get it right. Wait...

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

On Thu, Oct 27, 2022 at 05:31:49PM -0400, Krzysztof Kozlowski wrote:
> On 27/10/2022 17:00, Russell King (Oracle) wrote:
> > On Thu, Oct 27, 2022 at 03:53:25PM -0400, Krzysztof Kozlowski wrote:
> >> On 27/10/2022 13:03, Russell King (Oracle) wrote:
> >>> Add the DT binding for the Apple Mac System Management Controller GPIOs.
> >>>
> >>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >>> ---
> >>>  .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
> >>>  1 file changed, 28 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> >>> new file mode 100644
> >>> index 000000000000..a3883d62292d
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
> >>
> >> Filename based on compatible, so "apple,smc-gpio.yaml"
> > 
> > Many of the other yaml files in gpio/ are named as such.
> 
> Poor patterns, inconsistencies or even bugs like to copy themselves and
> it is never an argument.
> 
> The convention for all bindings is to use vendor,device.yaml, matching
> the compatible when applicable.
> 
> > 
> >>> @@ -0,0 +1,28 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
> >>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>> +
> >>> +title: Apple Mac System Management Controller GPIO
> >>> +
> >>> +maintainers:
> >>> +  - Hector Martin <marcan@marcan.st>
> >>> +
> >>> +description:
> >>> +  This describes the binding for the Apple Mac System Management Controller
> >>
> >> Drop "This describes the binding for"
> >>
> >>> +  GPIO block.
> >>> +
> >>> +properties:
> >>> +  compatible:
> >>> +    allOf:
> >>
> >> That's not proper syntax. Look at other examples (e.g. Apple bindings)
> >> doing it. Probably you wanted items here.
> > 
> > Really? You're joking. 
> 
> No. If you look at example-schema then answer should be obvious, so why
> do you think I am joking?
> 
> > I had sent an email to Rob to ask how this should
> > be done because my first guess spat out unhelpful error messages from
> > dt_bindings_check, and this is the best I could come up with based on
> > other "examples".
> > 
> > I tried "- items:" but that made no difference - dt_bindings_check spat
> > errors, so that's clearly incorrect. Specifically, I tried:
> > 
> > properties:
> >   compatible:
> >     - items:
> >         - enum:
> > 	    - apple,t8103-smc
> > 	- const: apple,smc-gpio
> > 
> > That doesn't work:
> 
> Of course, because "-" means list, so "- items" is not correct.
> 
> Where do you see such pattern? Anywhere following compatible? No. There
> is no. You just invented something instead of using many, many existing
> examples.

No, I did not "invent" something here. I tried to copy it from other
examples, but I couldn't find something that matched exactly.

In any case, relying on examples rather than a proper description of
how this should be done is utterly rediculous. There should be a formal
definition of the language used to describe this - but there doesn't
seem to be.

So, stuff like "-" means list is just not obvious, and the error
messages make it totally unobvious that's what the problem was.

> > Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml: properties:compatible: [{'items': [{'const': 'apple,t8103-smc'}, {'const': 'apple,smc-gpio'}]}] is not of type 'object', 'boolean'  from schema $id: http://json-schema.org/draft-07/schema#
> > 
> >>> +      - enum:
> >>> +          - apple,t8103-smc
> >>> +      - const: apple,smc-gpio
> >>> +
> >>> +  gpio-controller: true
> >>> +
> >>> +  '#gpio-cells':
> >>> +    const: 2
> >>
> >> Missing required properties. Start from new bindings or example-schema.
> > 
> > What's missing? All the other bindings that I see follow this pattern.
> 
> No. All other bindings have list of required properties. Only yours do
> not have.

Oh, you don't mean there are required properties missing from
#gpio-cells, you mean the list of required properties is missing,
which is something _entirely_ different. Your comment was utterly
ambiguous.

> >>> +
> >>> +additionalProperties: false
> >>
> >> Missing example, it's necessary to validate these.
> > 
> > Documentation states that examples are optional according to the
> > "writing-schema" documentation.
> 
> Yes, but without it we cannot validate the bindings.

Please update the writing-schema documentation to state that it's now
required to validate bindings, so that the documentation is no longer
stating something that's different from the required process.

> > Honestly, I find this YAML stuff extremely difficult, especially given
> > the lack of documentation on how to write it and the cryptic error
> > messages from the tooling. It's impossible to get it right before
> > submitting it - and I suspect from what I see above, it's impossible
> > for reviewers to know what is correct either, since some of what you've
> > said above appears to be wrong!
> 
> I would say it is doable - copy example-schema or recent device specific
> schema and customize it... But you started adding some weird stuff which
> was never, never in other bindings.

"weird stuff"? What weird stuff? What wasn't in other bindings? You
make no sense when you make this accusations, because they are totally
untrue.

I started with:

properties:
  compatible:
    - enum:
       - ...
    - const: ...

and dt_bindings_check thew it out. So I looked again at
Documentation/bindings/gpio/*.yaml. I decided maybe the - enum
containing one entry could be confusing matters, so I tried converting
that to a - const. Still failed.

So I had another look at other *.yaml files, and I then tried adding
- items: and indenting the following. Failed.

So then I tried allOf: which passed the checks. That's the evolution
there - trial and error.

Cryptic error messages, nothing else in gpio/ that follows the pattern
I wanted and trial and error led me to what I had in this patch. This
is *no* way to develop bindings.

There has to be a formal definition of this schema language - and
something better than pointing people at other bindings that may or
may not be correct.

So, I repeat myself: writing yaml stuff is utterly horrid and a total
hit and miss affair whether one gets it correct or not.

It seems to me that the problem of validating .dts files hasn't been
solved - the problem has merely been moved to a whole set of different
problems trying to write .yaml files that allow .dts files to be
validated, some of which could be solved by a better understanding of
the syntax, if only it were documented properly.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 1/2] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 27/10/2022 17:49, Russell King (Oracle) wrote:
> On Thu, Oct 27, 2022 at 05:31:49PM -0400, Krzysztof Kozlowski wrote:
>> On 27/10/2022 17:00, Russell King (Oracle) wrote:
>>> On Thu, Oct 27, 2022 at 03:53:25PM -0400, Krzysztof Kozlowski wrote:
>>>> On 27/10/2022 13:03, Russell King (Oracle) wrote:
>>>>> Add the DT binding for the Apple Mac System Management Controller GPIOs.
>>>>>
>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>>>> ---
>>>>>  .../devicetree/bindings/gpio/gpio-macsmc.yaml | 28 +++++++++++++++++++
>>>>>  1 file changed, 28 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..a3883d62292d
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-macsmc.yaml
>>>>
>>>> Filename based on compatible, so "apple,smc-gpio.yaml"
>>>
>>> Many of the other yaml files in gpio/ are named as such.
>>
>> Poor patterns, inconsistencies or even bugs like to copy themselves and
>> it is never an argument.
>>
>> The convention for all bindings is to use vendor,device.yaml, matching
>> the compatible when applicable.
>>
>>>
>>>>> @@ -0,0 +1,28 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Apple Mac System Management Controller GPIO
>>>>> +
>>>>> +maintainers:
>>>>> +  - Hector Martin <marcan@marcan.st>
>>>>> +
>>>>> +description:
>>>>> +  This describes the binding for the Apple Mac System Management Controller
>>>>
>>>> Drop "This describes the binding for"
>>>>
>>>>> +  GPIO block.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    allOf:
>>>>
>>>> That's not proper syntax. Look at other examples (e.g. Apple bindings)
>>>> doing it. Probably you wanted items here.
>>>
>>> Really? You're joking. 
>>
>> No. If you look at example-schema then answer should be obvious, so why
>> do you think I am joking?
>>
>>> I had sent an email to Rob to ask how this should
>>> be done because my first guess spat out unhelpful error messages from
>>> dt_bindings_check, and this is the best I could come up with based on
>>> other "examples".
>>>
>>> I tried "- items:" but that made no difference - dt_bindings_check spat
>>> errors, so that's clearly incorrect. Specifically, I tried:
>>>
>>> properties:
>>>   compatible:
>>>     - items:
>>>         - enum:
>>> 	    - apple,t8103-smc
>>> 	- const: apple,smc-gpio
>>>
>>> That doesn't work:
>>
>> Of course, because "-" means list, so "- items" is not correct.
>>
>> Where do you see such pattern? Anywhere following compatible? No. There
>> is no. You just invented something instead of using many, many existing
>> examples.
> 
> No, I did not "invent" something here. I tried to copy it from other
> examples, but I couldn't find something that matched exactly.
> 
> In any case, relying on examples rather than a proper description of
> how this should be done is utterly rediculous. There should be a formal
> definition of the language used to describe this - but there doesn't
> seem to be.

There is...

> 
> So, stuff like "-" means list is just not obvious, and the error

"-" is defined by YAML, so I do not understand what is here not obvious?

> messages make it totally unobvious that's what the problem was.

The error messages could indeed be improved, I agree.

> 
>>>>> +
>>>>> +additionalProperties: false
>>>>
>>>> Missing example, it's necessary to validate these.
>>>
>>> Documentation states that examples are optional according to the
>>> "writing-schema" documentation.
>>
>> Yes, but without it we cannot validate the bindings.
> 
> Please update the writing-schema documentation to state that it's now
> required to validate bindings, so that the documentation is no longer
> stating something that's different from the required process.
> 
>>> Honestly, I find this YAML stuff extremely difficult, especially given
>>> the lack of documentation on how to write it and the cryptic error
>>> messages from the tooling. It's impossible to get it right before
>>> submitting it - and I suspect from what I see above, it's impossible
>>> for reviewers to know what is correct either, since some of what you've
>>> said above appears to be wrong!
>>
>> I would say it is doable - copy example-schema or recent device specific
>> schema and customize it... But you started adding some weird stuff which
>> was never, never in other bindings.
> 
> "weird stuff"? What weird stuff? What wasn't in other bindings? You

The "allOf" in compatible is the weird stuff.

There is basically just one case with it, a special case. There are no
other bindings using such pattern.

> make no sense when you make this accusations, because they are totally
> untrue.
> 
> I started with:
> 
> properties:
>   compatible:
>     - enum:
>        - ...
>     - const: ...
> 
> and dt_bindings_check thew it out. So I looked again at
> Documentation/bindings/gpio/*.yaml. I decided maybe the - enum
> containing one entry could be confusing matters, so I tried converting
> that to a - const. Still failed.

Fourth YAML binding (counting alphabetically) in gpios has it:
Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.yaml

> 
> So I had another look at other *.yaml files, and I then tried adding
> - items: and indenting the following. Failed.
> 
> So then I tried allOf: which passed the checks. That's the evolution
> there - trial and error.

I understand your process. I still think that easier is to start from
example-schema as it has this case exactly.

> 
> Cryptic error messages, nothing else in gpio/ that follows the pattern
> I wanted and trial and error led me to what I had in this patch. This
> is *no* way to develop bindings.
> 
> There has to be a formal definition of this schema language - and
> something better than pointing people at other bindings that may or
> may not be correct.
> 
> So, I repeat myself: writing yaml stuff is utterly horrid and a total
> hit and miss affair whether one gets it correct or not.
> 
> It seems to me that the problem of validating .dts files hasn't been
> solved - the problem has merely been moved to a whole set of different
> problems trying to write .yaml files that allow .dts files to be
> validated, some of which could be solved by a better understanding of
> the syntax, if only it were documented properly.

I repeat also myself, writing C is also difficult and horrid :)

Anyway, your feedback is of course appreciated. Happy to help if you
have some more questions.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

On Thu, Oct 27, 2022 at 06:06:21PM -0400, Krzysztof Kozlowski wrote:
> On 27/10/2022 17:49, Russell King (Oracle) wrote:
> > In any case, relying on examples rather than a proper description of
> > how this should be done is utterly rediculous. There should be a formal
> > definition of the language used to describe this - but there doesn't
> > seem to be.
> 
> There is...
> 
> > 
> > So, stuff like "-" means list is just not obvious, and the error
> 
> "-" is defined by YAML, so I do not understand what is here not obvious?

I wonder which bit of "I don't understand YAML" isn't obvious to you?

The "-" means list isn't stated in yaml online tutorials, it seems
one is just supposed to know this. I guess if one been in JS and
similar landscapes it's obvious, but not everyone has.

C is damn easy in comparison! :)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!