[PATCH v3 0/7] Add Apple Mac System Management Controller GPIOs

[PATCH v3 0/7] Add Apple Mac System Management Controller GPIOs

[Russell King (Oracle)]

Hi,

This is version 3 of the series adds support for the Apple Mac GPIO
driver. These GPIOs are hadled via the System Management Controller.

The first patch adds the core SMC support to the MFD subsystem.

Patch 2 prepares the printf code for printing generic fourccs, as
required by patch 4 and 7.

Patch 3 adds the DT binding documentation for the rtkit backend.

Patch 4 adds the rtkit backend for the SMC core.

Patch 5 adds the device tree updates for the rtkit communication
channel and gpio sub device to DT.

Patch 6 adds the DT binding documentation for the gpio client.

Patch 7 adds the gpio client.

Patches mostly taken from the Asahi project and then modified due to
review comments from the previous postings.

I may have missed some comments as there's been quite a lot of
discussion on previous postings.

 Documentation/core-api/printk-formats.rst          |  32 ++
 .../devicetree/bindings/gpio/apple,smc-gpio.yaml   |  37 ++
 .../devicetree/bindings/mfd/apple,smc.yaml         |  67 +++
 arch/arm64/boot/dts/apple/t8103.dtsi               |  27 ++
 drivers/gpio/Kconfig                               |  11 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-macsmc.c                         | 245 +++++++++++
 drivers/mfd/Kconfig                                |   4 +
 drivers/mfd/Makefile                               |   1 +
 drivers/mfd/macsmc.c                               | 239 +++++++++++
 drivers/platform/Kconfig                           |   2 +
 drivers/platform/Makefile                          |   1 +
 drivers/platform/apple/Kconfig                     |  34 ++
 drivers/platform/apple/Makefile                    |   7 +
 drivers/platform/apple/macsmc-rtkit.c              | 455 +++++++++++++++++++++
 include/linux/mfd/macsmc.h                         | 104 +++++
 lib/test_printf.c                                  |  39 +-
 lib/vsprintf.c                                     |  35 +-
 18 files changed, 1328 insertions(+), 13 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
 create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
 create mode 100644 drivers/gpio/gpio-macsmc.c
 create mode 100644 drivers/mfd/macsmc.c
 create mode 100644 drivers/platform/apple/Kconfig
 create mode 100644 drivers/platform/apple/Makefile
 create mode 100644 drivers/platform/apple/macsmc-rtkit.c
 create mode 100644 include/linux/mfd/macsmc.h

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

[PATCH v3 1/7] mfd: Add core Apple Mac SMC driver

[Russell King]

From: Hector Martin <marcan@marcan.st>

This driver implements support for the SMC (System Management
Controller) in Apple Macs. In contrast to the existing applesmc driver,
it uses pluggable backends that allow it to support different SMC
implementations, and uses the MFD subsystem to expose the core SMC
functionality so that specific features (gpio, hwmon, battery, etc.) can
be implemented by separate drivers in their respective downstream
subsystems.

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/mfd/Kconfig        |   4 +
 drivers/mfd/Makefile       |   1 +
 drivers/mfd/macsmc.c       | 239 +++++++++++++++++++++++++++++++++++++
 include/linux/mfd/macsmc.h | 104 ++++++++++++++++
 4 files changed, 348 insertions(+)
 create mode 100644 drivers/mfd/macsmc.c
 create mode 100644 include/linux/mfd/macsmc.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 8b93856de432..f73e098b7228 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -224,6 +224,10 @@ config MFD_CROS_EC_DEV
 	  To compile this driver as a module, choose M here: the module will be
 	  called cros-ec-dev.
 
+config MFD_MACSMC
+	tristate
+	select MFD_CORE
+
 config MFD_MADERA
 	tristate "Cirrus Logic Madera codecs"
 	select MFD_CORE
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 7ed3ef4a698c..a5271b578d31 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
 obj-$(CONFIG_MFD_ENE_KB3930)	+= ene-kb3930.o
 obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
 obj-$(CONFIG_MFD_GATEWORKS_GSC)	+= gateworks-gsc.o
+obj-$(CONFIG_MFD_MACSMC)	+= macsmc.o
 
 obj-$(CONFIG_HTC_PASIC3)	+= htc-pasic3.o
 obj-$(CONFIG_HTC_I2CPLD)	+= htc-i2cpld.o
diff --git a/drivers/mfd/macsmc.c b/drivers/mfd/macsmc.c
new file mode 100644
index 000000000000..e5c3957efea4
--- /dev/null
+++ b/drivers/mfd/macsmc.c
@@ -0,0 +1,239 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SMC core framework
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include <linux/device.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/macsmc.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+
+struct apple_smc {
+	struct device *dev;
+
+	void *be_cookie;
+	const struct apple_smc_backend_ops *be;
+
+	struct mutex mutex;
+
+	u32 key_count;
+	smc_key first_key;
+	smc_key last_key;
+
+	struct blocking_notifier_head event_handlers;
+};
+
+static const struct mfd_cell apple_smc_devs[] = {
+	MFD_CELL_OF("macsmc-gpio", NULL, NULL, 0, 0, "apple,smc-gpio"),
+	MFD_CELL_NAME("macsmc-hid"),
+	MFD_CELL_NAME("macsmc-power"),
+	MFD_CELL_NAME("macsmc-reboot"),
+	MFD_CELL_OF("macsmc-rtc", NULL, NULL, 0, 0, "apple,smc-rtc"),
+};
+
+int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->read_key(smc->be_cookie, key, buf, size);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_read);
+
+int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->write_key(smc->be_cookie, key, buf, size);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_write);
+
+int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size)
+{
+	int ret;
+
+	/*
+	 * Will fail if SMC is busy. This is only used by SMC reboot/poweroff
+	 * final calls, so it doesn't really matter at that point.
+	 */
+	if (!mutex_trylock(&smc->mutex))
+		return -EBUSY;
+
+	ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_write_atomic);
+
+int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
+		 void *rbuf, size_t rsize)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->rw_key(smc->be_cookie, key, wbuf, wsize, rbuf, rsize);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_rw);
+
+int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->get_key_by_index(smc->be_cookie, index, key);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_get_key_by_index);
+
+int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info)
+{
+	int ret;
+
+	mutex_lock(&smc->mutex);
+	ret = smc->be->get_key_info(smc->be_cookie, key, info);
+	mutex_unlock(&smc->mutex);
+
+	return ret;
+}
+EXPORT_SYMBOL(apple_smc_get_key_info);
+
+int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key)
+{
+	int start = 0, count = smc->key_count;
+	int ret;
+
+	if (key <= smc->first_key)
+		return 0;
+	if (key > smc->last_key)
+		return smc->key_count;
+
+	while (count > 1) {
+		int pivot = start + ((count - 1) >> 1);
+		smc_key pkey;
+
+		ret = apple_smc_get_key_by_index(smc, pivot, &pkey);
+		if (ret < 0)
+			return ret;
+
+		if (pkey == key)
+			return pivot;
+
+		pivot++;
+
+		if (pkey < key) {
+			count -= pivot - start;
+			start = pivot;
+		} else {
+			count = pivot - start;
+		}
+	}
+
+	return start;
+}
+EXPORT_SYMBOL(apple_smc_find_first_key_index);
+
+int apple_smc_get_key_count(struct apple_smc *smc)
+{
+	return smc->key_count;
+}
+EXPORT_SYMBOL(apple_smc_get_key_count);
+
+void apple_smc_event_received(struct apple_smc *smc, uint32_t event)
+{
+	dev_dbg(smc->dev, "Event: 0x%08x\n", event);
+	blocking_notifier_call_chain(&smc->event_handlers, event, NULL);
+}
+EXPORT_SYMBOL(apple_smc_event_received);
+
+int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n)
+{
+	return blocking_notifier_chain_register(&smc->event_handlers, n);
+}
+EXPORT_SYMBOL(apple_smc_register_notifier);
+
+int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n)
+{
+	return blocking_notifier_chain_unregister(&smc->event_handlers, n);
+}
+EXPORT_SYMBOL(apple_smc_unregister_notifier);
+
+void *apple_smc_get_cookie(struct apple_smc *smc)
+{
+	return smc->be_cookie;
+}
+EXPORT_SYMBOL(apple_smc_get_cookie);
+
+struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, void *cookie)
+{
+	struct apple_smc *smc;
+	u32 count;
+	int ret;
+
+	smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL);
+	if (!smc)
+		return ERR_PTR(-ENOMEM);
+
+	smc->dev = dev;
+	smc->be_cookie = cookie;
+	smc->be = ops;
+	mutex_init(&smc->mutex);
+	BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers);
+
+	ret = apple_smc_read_u32(smc, SMC_KEY(#KEY), &count);
+	if (ret)
+		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get key count"));
+	smc->key_count = be32_to_cpu(count);
+
+	ret = apple_smc_get_key_by_index(smc, 0, &smc->first_key);
+	if (ret)
+		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get first key"));
+
+	ret = apple_smc_get_key_by_index(smc, smc->key_count - 1, &smc->last_key);
+	if (ret)
+		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get last key"));
+
+	/* Enable notifications */
+	apple_smc_write_flag(smc, SMC_KEY(NTAP), 1);
+
+	dev_info(dev, "Initialized (%d keys %p4ch..%p4ch)\n",
+		 smc->key_count, &smc->first_key, &smc->last_key);
+
+	dev_set_drvdata(dev, smc);
+
+	ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL);
+	if (ret)
+		return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization failed"));
+
+	return smc;
+}
+EXPORT_SYMBOL(apple_smc_probe);
+
+int apple_smc_remove(struct apple_smc *smc)
+{
+	mfd_remove_devices(smc->dev);
+
+	/* Disable notifications */
+	apple_smc_write_flag(smc, SMC_KEY(NTAP), 1);
+
+	return 0;
+}
+EXPORT_SYMBOL(apple_smc_remove);
+
+MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_DESCRIPTION("Apple SMC core");
diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h
new file mode 100644
index 000000000000..99cfa23f27bd
--- /dev/null
+++ b/include/linux/mfd/macsmc.h
@@ -0,0 +1,104 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SMC core definitions
+ * Copyright (C) The Asahi Linux Contributors
+ */
+
+#ifndef _LINUX_MFD_MACSMC_H
+#define _LINUX_MFD_MACSMC_H
+
+struct apple_smc;
+
+typedef u32 smc_key;
+
+#define SMC_KEY(s) (smc_key)(_SMC_KEY(#s))
+#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
+
+#define APPLE_SMC_READABLE BIT(7)
+#define APPLE_SMC_WRITABLE BIT(6)
+#define APPLE_SMC_FUNCTION BIT(4)
+
+struct apple_smc_key_info {
+	u8 size;
+	u32 type_code;
+	u8 flags;
+};
+
+int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size);
+int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size);
+int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size);
+int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
+		 void *rbuf, size_t rsize);
+
+int apple_smc_get_key_count(struct apple_smc *smc);
+int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key);
+int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key);
+int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info);
+
+static inline bool apple_smc_key_exists(struct apple_smc *smc, smc_key key)
+{
+	return apple_smc_get_key_info(smc, key, NULL) >= 0;
+}
+
+#define APPLE_SMC_TYPE_OPS(type) \
+	static inline int apple_smc_read_##type(struct apple_smc *smc, smc_key key, type *p) \
+	{ \
+		int ret = apple_smc_read(smc, key, p, sizeof(*p)); \
+		return (ret < 0) ? ret : ((ret != sizeof(*p)) ? -EINVAL : 0); \
+	} \
+	static inline int apple_smc_write_##type(struct apple_smc *smc, smc_key key, type p) \
+	{ \
+		return apple_smc_write(smc, key, &p, sizeof(p)); \
+	} \
+	static inline int apple_smc_write_##type##_atomic(struct apple_smc *smc, smc_key key, type p) \
+	{ \
+		return apple_smc_write_atomic(smc, key, &p, sizeof(p)); \
+	} \
+	static inline int apple_smc_rw_##type(struct apple_smc *smc, smc_key key, \
+					      type w, type *r) \
+	{ \
+		int ret = apple_smc_rw(smc, key, &w, sizeof(w), r, sizeof(*r)); \
+		return (ret < 0) ? ret : ((ret != sizeof(*r)) ? -EINVAL : 0); \
+	}
+
+APPLE_SMC_TYPE_OPS(u64)
+APPLE_SMC_TYPE_OPS(u32)
+APPLE_SMC_TYPE_OPS(u16)
+APPLE_SMC_TYPE_OPS(u8)
+APPLE_SMC_TYPE_OPS(s64)
+APPLE_SMC_TYPE_OPS(s32)
+APPLE_SMC_TYPE_OPS(s16)
+APPLE_SMC_TYPE_OPS(s8)
+
+static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key)
+{
+	u8 val;
+	int ret = apple_smc_read_u8(smc, key, &val);
+	if (ret < 0)
+		return ret;
+	return val ? 1 : 0;
+}
+#define apple_smc_write_flag apple_smc_write_u8
+
+int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n);
+int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n);
+
+/* backend interface */
+
+struct apple_smc_backend_ops {
+	int (*read_key)(void *cookie, smc_key key, void *buf, size_t size);
+	int (*write_key)(void *cookie, smc_key key, void *buf, size_t size);
+	int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size);
+	int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize,
+		      void *rbuf, size_t rsize);
+	int (*get_key_by_index)(void *cookie, int index, smc_key *key);
+	int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info);
+};
+
+struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops,
+				  void *cookie);
+void *apple_smc_get_cookie(struct apple_smc *smc);
+int apple_smc_remove(struct apple_smc *smc);
+void apple_smc_event_received(struct apple_smc *smc, uint32_t event);
+
+#endif
-- 
2.30.2

Re: [PATCH v3 1/7] mfd: Add core Apple Mac SMC driver

[Lee Jones]

On Tue, 08 Nov 2022, Russell King wrote:

> From: Hector Martin <marcan@marcan.st>
> 
> This driver implements support for the SMC (System Management
> Controller) in Apple Macs. In contrast to the existing applesmc driver,
> it uses pluggable backends that allow it to support different SMC
> implementations, and uses the MFD subsystem to expose the core SMC
> functionality so that specific features (gpio, hwmon, battery, etc.) can
> be implemented by separate drivers in their respective downstream
> subsystems.

Could we have Russell's ASCII simplified architecture model here please?

> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/mfd/Kconfig        |   4 +
>  drivers/mfd/Makefile       |   1 +
>  drivers/mfd/macsmc.c       | 239 +++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/macsmc.h | 104 ++++++++++++++++
>  4 files changed, 348 insertions(+)
>  create mode 100644 drivers/mfd/macsmc.c
>  create mode 100644 include/linux/mfd/macsmc.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 8b93856de432..f73e098b7228 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -224,6 +224,10 @@ config MFD_CROS_EC_DEV
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called cros-ec-dev.
>  
> +config MFD_MACSMC
> +	tristate

Is this selectable?

Worth having a description?

> +	select MFD_CORE

Help section?

Copy / paste from the commit log should be enough.

>  config MFD_MADERA
>  	tristate "Cirrus Logic Madera codecs"
>  	select MFD_CORE
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 7ed3ef4a698c..a5271b578d31 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
>  obj-$(CONFIG_MFD_ENE_KB3930)	+= ene-kb3930.o
>  obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
>  obj-$(CONFIG_MFD_GATEWORKS_GSC)	+= gateworks-gsc.o
> +obj-$(CONFIG_MFD_MACSMC)	+= macsmc.o
>  
>  obj-$(CONFIG_HTC_PASIC3)	+= htc-pasic3.o
>  obj-$(CONFIG_HTC_I2CPLD)	+= htc-i2cpld.o
> diff --git a/drivers/mfd/macsmc.c b/drivers/mfd/macsmc.c
> new file mode 100644
> index 000000000000..e5c3957efea4
> --- /dev/null
> +++ b/drivers/mfd/macsmc.c
> @@ -0,0 +1,239 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SMC core framework

"SMC (System Management Controller)"

Tiny nit: '\n'

> + * Copyright The Asahi Linux Contributors

Missing (C)

Would you like an Author(s) line here?

> + */
> +
> +#include <linux/device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/macsmc.h>
> +#include <linux/mutex.h>
> +#include <linux/notifier.h>
> +

Would you be kind enough to add a header here to describe the
attributes please.  Some of them are non-standard.

> +struct apple_smc {
> +	struct device *dev;
> +
> +	void *be_cookie;
> +	const struct apple_smc_backend_ops *be;
> +
> +	struct mutex mutex;
> +
> +	u32 key_count;
> +	smc_key first_key;
> +	smc_key last_key;
> +
> +	struct blocking_notifier_head event_handlers;
> +};
> +
> +static const struct mfd_cell apple_smc_devs[] = {
> +	MFD_CELL_OF("macsmc-gpio", NULL, NULL, 0, 0, "apple,smc-gpio"),
> +	MFD_CELL_NAME("macsmc-hid"),
> +	MFD_CELL_NAME("macsmc-power"),
> +	MFD_CELL_NAME("macsmc-reboot"),
> +	MFD_CELL_OF("macsmc-rtc", NULL, NULL, 0, 0, "apple,smc-rtc"),
> +};
> +
> +int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->read_key(smc->be_cookie, key, buf, size);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_read);
> +
> +int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->write_key(smc->be_cookie, key, buf, size);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_write);
> +
> +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size)
> +{
> +	int ret;
> +
> +	/*
> +	 * Will fail if SMC is busy. This is only used by SMC reboot/poweroff
> +	 * final calls, so it doesn't really matter at that point.
> +	 */
> +	if (!mutex_trylock(&smc->mutex))
> +		return -EBUSY;
> +
> +	ret = smc->be->write_key_atomic(smc->be_cookie, key, buf, size);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_write_atomic);
> +
> +int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
> +		 void *rbuf, size_t rsize)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->rw_key(smc->be_cookie, key, wbuf, wsize, rbuf, rsize);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_rw);
> +
> +int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->get_key_by_index(smc->be_cookie, index, key);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_get_key_by_index);
> +
> +int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info)
> +{
> +	int ret;
> +
> +	mutex_lock(&smc->mutex);
> +	ret = smc->be->get_key_info(smc->be_cookie, key, info);
> +	mutex_unlock(&smc->mutex);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(apple_smc_get_key_info);
> +
> +int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key)
> +{
> +	int start = 0, count = smc->key_count;
> +	int ret;
> +
> +	if (key <= smc->first_key)
> +		return 0;
> +	if (key > smc->last_key)
> +		return smc->key_count;
> +
> +	while (count > 1) {
> +		int pivot = start + ((count - 1) >> 1);
> +		smc_key pkey;
> +
> +		ret = apple_smc_get_key_by_index(smc, pivot, &pkey);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (pkey == key)
> +			return pivot;
> +
> +		pivot++;
> +
> +		if (pkey < key) {
> +			count -= pivot - start;
> +			start = pivot;
> +		} else {
> +			count = pivot - start;
> +		}
> +	}
> +
> +	return start;
> +}

Maybe a 1 or 2 line comment to provide an overview of what's happening
in here please.

> +EXPORT_SYMBOL(apple_smc_find_first_key_index);
> +
> +int apple_smc_get_key_count(struct apple_smc *smc)
> +{
> +	return smc->key_count;
> +}
> +EXPORT_SYMBOL(apple_smc_get_key_count);
> +
> +void apple_smc_event_received(struct apple_smc *smc, uint32_t event)
> +{
> +	dev_dbg(smc->dev, "Event: 0x%08x\n", event);
> +	blocking_notifier_call_chain(&smc->event_handlers, event, NULL);
> +}
> +EXPORT_SYMBOL(apple_smc_event_received);
> +
> +int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n)
> +{
> +	return blocking_notifier_chain_register(&smc->event_handlers, n);
> +}
> +EXPORT_SYMBOL(apple_smc_register_notifier);
> +
> +int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n)
> +{
> +	return blocking_notifier_chain_unregister(&smc->event_handlers, n);
> +}
> +EXPORT_SYMBOL(apple_smc_unregister_notifier);
> +
> +void *apple_smc_get_cookie(struct apple_smc *smc)
> +{
> +	return smc->be_cookie;
> +}
> +EXPORT_SYMBOL(apple_smc_get_cookie);

These parts seem like abstraction for the sake of abstraction.

Any reason why the caller can't use the blocking_notifier_* API and
look into the apple_smc for themselves.

> +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops, void *cookie)
> +{
> +	struct apple_smc *smc;
> +	u32 count;
> +	int ret;
> +
> +	smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL);
> +	if (!smc)
> +		return ERR_PTR(-ENOMEM);
> +
> +	smc->dev = dev;
> +	smc->be_cookie = cookie;
> +	smc->be = ops;
> +	mutex_init(&smc->mutex);
> +	BLOCKING_INIT_NOTIFIER_HEAD(&smc->event_handlers);
> +
> +	ret = apple_smc_read_u32(smc, SMC_KEY(#KEY), &count);
> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get key count"));
> +	smc->key_count = be32_to_cpu(count);
> +
> +	ret = apple_smc_get_key_by_index(smc, 0, &smc->first_key);
> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get first key"));
> +
> +	ret = apple_smc_get_key_by_index(smc, smc->key_count - 1, &smc->last_key);
> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Failed to get last key"));
> +
> +	/* Enable notifications */
> +	apple_smc_write_flag(smc, SMC_KEY(NTAP), 1);
> +
> +	dev_info(dev, "Initialized (%d keys %p4ch..%p4ch)\n",
> +		 smc->key_count, &smc->first_key, &smc->last_key);
> +
> +	dev_set_drvdata(dev, smc);
> +
> +	ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL);

Please replace the -1 with the defines provided.

> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization failed"));

"Failed to register sub-devices"

> +	return smc;
> +}
> +EXPORT_SYMBOL(apple_smc_probe);
> +
> +int apple_smc_remove(struct apple_smc *smc)
> +{
> +	mfd_remove_devices(smc->dev);

devm_*?

> +	/* Disable notifications */
> +	apple_smc_write_flag(smc, SMC_KEY(NTAP), 1);

The same command enables and disables notifications?

> +	return 0;
> +}
> +EXPORT_SYMBOL(apple_smc_remove);
> +
> +MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
> +MODULE_LICENSE("Dual MIT/GPL");
> +MODULE_DESCRIPTION("Apple SMC core");

SMC (System Management Controller)

> diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h
> new file mode 100644
> index 000000000000..99cfa23f27bd
> --- /dev/null
> +++ b/include/linux/mfd/macsmc.h
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SMC core definitions

SMC (System Management Controller)

> + * Copyright (C) The Asahi Linux Contributors
> + */
> +
> +#ifndef _LINUX_MFD_MACSMC_H
> +#define _LINUX_MFD_MACSMC_H
> +
> +struct apple_smc;

You can move the definition into here and omit this line.

> +typedef u32 smc_key;
> +
> +#define SMC_KEY(s) (smc_key)(_SMC_KEY(#s))
> +#define _SMC_KEY(s) (((s)[0] << 24) | ((s)[1] << 16) | ((s)[2] << 8) | (s)[3])
> +
> +#define APPLE_SMC_READABLE BIT(7)
> +#define APPLE_SMC_WRITABLE BIT(6)
> +#define APPLE_SMC_FUNCTION BIT(4)
> +
> +struct apple_smc_key_info {
> +	u8 size;
> +	u32 type_code;
> +	u8 flags;
> +};
> +
> +int apple_smc_read(struct apple_smc *smc, smc_key key, void *buf, size_t size);
> +int apple_smc_write(struct apple_smc *smc, smc_key key, void *buf, size_t size);
> +int apple_smc_write_atomic(struct apple_smc *smc, smc_key key, void *buf, size_t size);
> +int apple_smc_rw(struct apple_smc *smc, smc_key key, void *wbuf, size_t wsize,
> +		 void *rbuf, size_t rsize);
> +
> +int apple_smc_get_key_count(struct apple_smc *smc);
> +int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key);
> +int apple_smc_get_key_by_index(struct apple_smc *smc, int index, smc_key *key);
> +int apple_smc_get_key_info(struct apple_smc *smc, smc_key key, struct apple_smc_key_info *info);
> +
> +static inline bool apple_smc_key_exists(struct apple_smc *smc, smc_key key)
> +{
> +	return apple_smc_get_key_info(smc, key, NULL) >= 0;
> +}
> +
> +#define APPLE_SMC_TYPE_OPS(type) \
> +	static inline int apple_smc_read_##type(struct apple_smc *smc, smc_key key, type *p) \
> +	{ \
> +		int ret = apple_smc_read(smc, key, p, sizeof(*p)); \
> +		return (ret < 0) ? ret : ((ret != sizeof(*p)) ? -EINVAL : 0); \
> +	} \
> +	static inline int apple_smc_write_##type(struct apple_smc *smc, smc_key key, type p) \
> +	{ \
> +		return apple_smc_write(smc, key, &p, sizeof(p)); \
> +	} \
> +	static inline int apple_smc_write_##type##_atomic(struct apple_smc *smc, smc_key key, type p) \
> +	{ \
> +		return apple_smc_write_atomic(smc, key, &p, sizeof(p)); \
> +	} \
> +	static inline int apple_smc_rw_##type(struct apple_smc *smc, smc_key key, \
> +					      type w, type *r) \
> +	{ \
> +		int ret = apple_smc_rw(smc, key, &w, sizeof(w), r, sizeof(*r)); \
> +		return (ret < 0) ? ret : ((ret != sizeof(*r)) ? -EINVAL : 0); \
> +	}
> +
> +APPLE_SMC_TYPE_OPS(u64)
> +APPLE_SMC_TYPE_OPS(u32)
> +APPLE_SMC_TYPE_OPS(u16)
> +APPLE_SMC_TYPE_OPS(u8)
> +APPLE_SMC_TYPE_OPS(s64)
> +APPLE_SMC_TYPE_OPS(s32)
> +APPLE_SMC_TYPE_OPS(s16)
> +APPLE_SMC_TYPE_OPS(s8)
> +
> +static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key)
> +{
> +	u8 val;
> +	int ret = apple_smc_read_u8(smc, key, &val);

Nit: Please separate the declaration and assignment via function call
with a line break in between.

> +	if (ret < 0)
> +		return ret;
> +	return val ? 1 : 0;
> +}
> +#define apple_smc_write_flag apple_smc_write_u8
> +
> +int apple_smc_register_notifier(struct apple_smc *smc, struct notifier_block *n);
> +int apple_smc_unregister_notifier(struct apple_smc *smc, struct notifier_block *n);
> +
> +/* backend interface */
> +
> +struct apple_smc_backend_ops {
> +	int (*read_key)(void *cookie, smc_key key, void *buf, size_t size);
> +	int (*write_key)(void *cookie, smc_key key, void *buf, size_t size);
> +	int (*write_key_atomic)(void *cookie, smc_key key, void *buf, size_t size);
> +	int (*rw_key)(void *cookie, smc_key key, void *wbuf, size_t wsize,
> +		      void *rbuf, size_t rsize);
> +	int (*get_key_by_index)(void *cookie, int index, smc_key *key);
> +	int (*get_key_info)(void *cookie, smc_key key, struct apple_smc_key_info *info);
> +};
> +
> +struct apple_smc *apple_smc_probe(struct device *dev, const struct apple_smc_backend_ops *ops,
> +				  void *cookie);
> +void *apple_smc_get_cookie(struct apple_smc *smc);
> +int apple_smc_remove(struct apple_smc *smc);
> +void apple_smc_event_received(struct apple_smc *smc, uint32_t event);
> +
> +#endif

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 1/7] mfd: Add core Apple Mac SMC driver

[Andy Shevchenko]

On Tue, Nov 08, 2022 at 04:33:17PM +0000, Russell King wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> This driver implements support for the SMC (System Management
> Controller) in Apple Macs. In contrast to the existing applesmc driver,
> it uses pluggable backends that allow it to support different SMC
> implementations, and uses the MFD subsystem to expose the core SMC
> functionality so that specific features (gpio, hwmon, battery, etc.) can
> be implemented by separate drivers in their respective downstream
> subsystems.

...

> +	ret = mfd_add_devices(dev, -1, apple_smc_devs, ARRAY_SIZE(apple_smc_devs), NULL, 0, NULL);

PLATFORM_DEVID_NONE

> +	if (ret)
> +		return ERR_PTR(dev_err_probe(dev, ret, "Subdevice initialization failed"));

...

> +EXPORT_SYMBOL(apple_smc_probe);

Namespace?

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Russell King]

From: Hector Martin <marcan@marcan.st>

%p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
it's useful to be able to print generic 4-character codes formatted as
an integer. Extend it to add format specifiers for printing generic
32-bit FOURCCs with various endian semantics:

%p4ch   Host-endian
%p4cl	Little-endian
%p4cb	Big-endian
%p4cr	Reverse-endian

The endianness determines how bytes are interpreted as a u32, and the
FOURCC is then always printed MSByte-first (this is the opposite of
V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
allow printing LSByte-first FOURCCs stored in host endian order
(other than the hex form being in character order, not the integer
value).

Signed-off-by: Hector Martin <marcan@marcan.st>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
 lib/test_printf.c                         | 39 +++++++++++++++++++----
 lib/vsprintf.c                            | 35 ++++++++++++++++----
 3 files changed, 93 insertions(+), 13 deletions(-)

diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
index dbe1aacc79d0..92a488884cf8 100644
--- a/Documentation/core-api/printk-formats.rst
+++ b/Documentation/core-api/printk-formats.rst
@@ -625,6 +625,38 @@ Passed by reference.
 	%p4cc	Y10  little-endian (0x20303159)
 	%p4cc	NV12 big-endian (0xb231564e)
 
+Generic FourCC code
+-------------------
+
+::
+	%p4c[hrbl]	gP00 (0x67503030)
+
+Print a generic FourCC code, as both ASCII characters and its numerical
+value as hexadecimal.
+
+The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify
+host, reversed, big or little endian order data respectively. Host endian
+order means the data is interpreted as a 32-bit integer and the most
+significant byte is printed first; that is, the character code as printed
+matches the byte order stored in memory on big-endian systems, and is reversed
+on little-endian systems.
+
+Passed by reference.
+
+Examples for a little-endian machine, given &(u32)0x67503030::
+
+	%p4ch	gP00 (0x67503030)
+	%p4cr	00Pg (0x30305067)
+	%p4cb	00Pg (0x30305067)
+	%p4cl	gP00 (0x67503030)
+
+Examples for a big-endian machine, given &(u32)0x67503030::
+
+	%p4ch	gP00 (0x67503030)
+	%p4cr	00Pg (0x30305067)
+	%p4cb	gP00 (0x67503030)
+	%p4cl	00Pg (0x30305067)
+
 Rust
 ----
 
diff --git a/lib/test_printf.c b/lib/test_printf.c
index 4bd15a593fbd..77a9128a6b5a 100644
--- a/lib/test_printf.c
+++ b/lib/test_printf.c
@@ -731,21 +731,46 @@ static void __init fwnode_pointer(void)
 	software_node_unregister_nodes(softnodes);
 }
 
+struct fourcc_struct {
+	u32 code;
+	const char *str;
+};
+
+static void __init fourcc_pointer_test(const struct fourcc_struct *fc, size_t n,
+				       const char *fmt)
+{
+	size_t i;
+
+	for (i = 0; i < n; i++)
+		test(fc[i].str, fmt, &fc[i].code);
+}
+
 static void __init fourcc_pointer(void)
 {
-	struct {
-		u32 code;
-		char *str;
-	} const try[] = {
+	struct fourcc_struct const try_cc[] = {
 		{ 0x3231564e, "NV12 little-endian (0x3231564e)", },
 		{ 0xb231564e, "NV12 big-endian (0xb231564e)", },
 		{ 0x10111213, ".... little-endian (0x10111213)", },
 		{ 0x20303159, "Y10  little-endian (0x20303159)", },
 	};
-	unsigned int i;
+	struct fourcc_struct const try_ch = {
+		0x41424344, "ABCD (0x41424344)",
+	};
+	struct fourcc_struct const try_cr = {
+		0x41424344, "DCBA (0x44434241)",
+	};
+	struct fourcc_struct const try_cl = {
+		le32_to_cpu(0x41424344), "ABCD (0x41424344)",
+	};
+	struct fourcc_struct const try_cb = {
+		be32_to_cpu(0x41424344), "ABCD (0x41424344)",
+	};
 
-	for (i = 0; i < ARRAY_SIZE(try); i++)
-		test(try[i].str, "%p4cc", &try[i].code);
+	fourcc_pointer_test(try_cc, ARRAY_SIZE(try_cc), "%p4cc");
+	fourcc_pointer_test(&try_ch, 1, "%p4ch");
+	fourcc_pointer_test(&try_cr, 1, "%p4cr");
+	fourcc_pointer_test(&try_cl, 1, "%p4cl");
+	fourcc_pointer_test(&try_cb, 1, "%p4cb");
 }
 
 static void __init
diff --git a/lib/vsprintf.c b/lib/vsprintf.c
index 24f37bab8bc1..17064b839f19 100644
--- a/lib/vsprintf.c
+++ b/lib/vsprintf.c
@@ -1762,27 +1762,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
 	char output[sizeof("0123 little-endian (0x01234567)")];
 	char *p = output;
 	unsigned int i;
+	bool pixel_fmt = false;
 	u32 orig, val;
 
-	if (fmt[1] != 'c' || fmt[2] != 'c')
+	if (fmt[1] != 'c')
 		return error_string(buf, end, "(%p4?)", spec);
 
 	if (check_pointer(&buf, end, fourcc, spec))
 		return buf;
 
 	orig = get_unaligned(fourcc);
-	val = orig & ~BIT(31);
+	switch (fmt[2]) {
+	case 'h':
+		val = orig;
+		break;
+	case 'r':
+		val = orig = swab32(orig);
+		break;
+	case 'l':
+		val = orig = le32_to_cpu(orig);
+		break;
+	case 'b':
+		val = orig = be32_to_cpu(orig);
+		break;
+	case 'c':
+		/* Pixel formats are printed LSB-first */
+		val = swab32(orig & ~BIT(31));
+		pixel_fmt = true;
+		break;
+	default:
+		return error_string(buf, end, "(%p4?)", spec);
+	}
 
 	for (i = 0; i < sizeof(u32); i++) {
-		unsigned char c = val >> (i * 8);
+		unsigned char c = val >> ((3 - i) * 8);
 
 		/* Print non-control ASCII characters as-is, dot otherwise */
 		*p++ = isascii(c) && isprint(c) ? c : '.';
 	}
 
-	*p++ = ' ';
-	strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
-	p += strlen(p);
+	if (pixel_fmt) {
+		*p++ = ' ';
+		strcpy(p, orig & BIT(31) ? "big-endian" : "little-endian");
+		p += strlen(p);
+	}
 
 	*p++ = ' ';
 	*p++ = '(';
-- 
2.30.2

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Petr Mladek]

On Tue 2022-11-08 16:33:22, Russell King wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> it's useful to be able to print generic 4-character codes formatted as
> an integer. Extend it to add format specifiers for printing generic
> 32-bit FOURCCs with various endian semantics:
> 
> %p4ch   Host-endian
> %p4cl	Little-endian
> %p4cb	Big-endian
> %p4cr	Reverse-endian
> 
> The endianness determines how bytes are interpreted as a u32, and the
> FOURCC is then always printed MSByte-first (this is the opposite of
> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> allow printing LSByte-first FOURCCs stored in host endian order
> (other than the hex form being in character order, not the integer
> value).
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Petr Mladek <pmladek@suse.com>

See one nit below.

> --- a/lib/vsprintf.c
> +++ b/lib/vsprintf.c
> @@ -1762,27 +1762,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
>  	char output[sizeof("0123 little-endian (0x01234567)")];
>  	char *p = output;
>  	unsigned int i;
> +	bool pixel_fmt = false;
>  	u32 orig, val;
>  
> -	if (fmt[1] != 'c' || fmt[2] != 'c')
> +	if (fmt[1] != 'c')
>  		return error_string(buf, end, "(%p4?)", spec);
>  
>  	if (check_pointer(&buf, end, fourcc, spec))
>  		return buf;
>  
>  	orig = get_unaligned(fourcc);
> -	val = orig & ~BIT(31);
> +	switch (fmt[2]) {
> +	case 'h':
> +		val = orig;
> +		break;
> +	case 'r':
> +		val = orig = swab32(orig);

I do not like much these multi assignments. I think that the result
was not even defined in some older C standards. Though, I can't find
it now. And even make W=3 does not warn about it.

> +		break;
> +	case 'l':
> +		val = orig = le32_to_cpu(orig);
> +		break;
> +	case 'b':
> +		val = orig = be32_to_cpu(orig);
> +		break;

Best Regards,
Petr

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Andy Shevchenko]

On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> On Tue 2022-11-08 16:33:22, Russell King wrote:

...

> >  	orig = get_unaligned(fourcc);
> > -	val = orig & ~BIT(31);
> > +	switch (fmt[2]) {
> > +	case 'h':
> > +		val = orig;
> > +		break;
> > +	case 'r':
> > +		val = orig = swab32(orig);
> 
> I do not like much these multi assignments. I think that the result
> was not even defined in some older C standards. Though, I can't find
> it now. And even make W=3 does not warn about it.
> 
> > +		break;
> > +	case 'l':
> > +		val = orig = le32_to_cpu(orig);
> > +		break;
> > +	case 'b':
> > +		val = orig = be32_to_cpu(orig);
> > +		break;

Isn't easy to fix? Something like below?

	switch (fmt[2]) {
	case 'h':
		break;
	case 'r':
		orig = swab32(orig);
		break;
	case 'l':
		orig = le32_to_cpu(orig);
		break;
	case 'b':
		orig = be32_to_cpu(orig);
		break;

		...
	}
	val = orig;


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Petr Mladek]

On Mon 2022-11-14 17:46:28, Andy Shevchenko wrote:
> On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> > On Tue 2022-11-08 16:33:22, Russell King wrote:
> 
> ...
> 
> > >  	orig = get_unaligned(fourcc);
> > > -	val = orig & ~BIT(31);
> > > +	switch (fmt[2]) {
> > > +	case 'h':
> > > +		val = orig;
> > > +		break;
> > > +	case 'r':
> > > +		val = orig = swab32(orig);
> > 
> > I do not like much these multi assignments. I think that the result
> > was not even defined in some older C standards. Though, I can't find
> > it now. And even make W=3 does not warn about it.
> > 
> > > +		break;
> > > +	case 'l':
> > > +		val = orig = le32_to_cpu(orig);
> > > +		break;
> > > +	case 'b':
> > > +		val = orig = be32_to_cpu(orig);
> > > +		break;
> 
> Isn't easy to fix? Something like below?
> 
> 	switch (fmt[2]) {
> 	case 'h':
> 		break;
> 	case 'r':
> 		orig = swab32(orig);
> 		break;
> 	case 'l':
> 		orig = le32_to_cpu(orig);
> 		break;
> 	case 'b':
> 		orig = be32_to_cpu(orig);
> 		break;
> 
> 		...
> 	}
> 	val = orig;

I though the same. Unfortunately, this is not valid for the "case c:"
path where "orig" stays untouched:

	case 'c':
		/* Pixel formats are printed LSB-first */
		val = swab32(orig & ~BIT(31));
		pixel_fmt = true;
		break;

It is pity that "orig" is handled differently for the pixel and the generic
formats.

But I am afraid that there is no good solution. The code will
always be a mess when it tries to implement a messy definition.

It would be nice if the the FourCC format was used consistently
in all subsystems in the first place.


IMPORTANT: This brings the questions.

	   Is there actually a standard how to print the original
	   number in FourCC?

	   Do we really want to modify "orig" in the generic
	   implementation?

Best Regards,
Petr

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Russell King (Oracle)]

On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> On Tue 2022-11-08 16:33:22, Russell King wrote:
> > From: Hector Martin <marcan@marcan.st>
> > 
> > %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> > it's useful to be able to print generic 4-character codes formatted as
> > an integer. Extend it to add format specifiers for printing generic
> > 32-bit FOURCCs with various endian semantics:
> > 
> > %p4ch   Host-endian
> > %p4cl	Little-endian
> > %p4cb	Big-endian
> > %p4cr	Reverse-endian
> > 
> > The endianness determines how bytes are interpreted as a u32, and the
> > FOURCC is then always printed MSByte-first (this is the opposite of
> > V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> > allow printing LSByte-first FOURCCs stored in host endian order
> > (other than the hex form being in character order, not the integer
> > value).
> > 
> > Signed-off-by: Hector Martin <marcan@marcan.st>
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> Reviewed-by: Petr Mladek <pmladek@suse.com>
> 
> See one nit below.
> 
> > --- a/lib/vsprintf.c
> > +++ b/lib/vsprintf.c
> > @@ -1762,27 +1762,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> >  	char output[sizeof("0123 little-endian (0x01234567)")];
> >  	char *p = output;
> >  	unsigned int i;
> > +	bool pixel_fmt = false;
> >  	u32 orig, val;
> >  
> > -	if (fmt[1] != 'c' || fmt[2] != 'c')
> > +	if (fmt[1] != 'c')
> >  		return error_string(buf, end, "(%p4?)", spec);
> >  
> >  	if (check_pointer(&buf, end, fourcc, spec))
> >  		return buf;
> >  
> >  	orig = get_unaligned(fourcc);
> > -	val = orig & ~BIT(31);
> > +	switch (fmt[2]) {
> > +	case 'h':
> > +		val = orig;
> > +		break;
> > +	case 'r':
> > +		val = orig = swab32(orig);
> 
> I do not like much these multi assignments. I think that the result
> was not even defined in some older C standards. Though, I can't find
> it now. And even make W=3 does not warn about it.

Err.

It's been supported for decades. I learnt about it back in 1992 when
I was introduced to C by another experienced C programmer. It's been
supported in ANSI C compilers. The Norcroft C compiler (which is
strict ANSI) on Acorn platforms back in the late 1980s/1990s even
supported it.

I think you're a bit out of date.

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

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Russell King (Oracle)]

On Mon, Nov 14, 2022 at 04:15:50PM +0000, Russell King (Oracle) wrote:
> On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> > >  	orig = get_unaligned(fourcc);
> > > -	val = orig & ~BIT(31);
> > > +	switch (fmt[2]) {
> > > +	case 'h':
> > > +		val = orig;
> > > +		break;
> > > +	case 'r':
> > > +		val = orig = swab32(orig);
> > 
> > I do not like much these multi assignments. I think that the result
> > was not even defined in some older C standards. Though, I can't find
> > it now. And even make W=3 does not warn about it.
> 
> Err.
> 
> It's been supported for decades. I learnt about it back in 1992 when
> I was introduced to C by another experienced C programmer. It's been
> supported in ANSI C compilers. The Norcroft C compiler (which is
> strict ANSI) on Acorn platforms back in the late 1980s/1990s even
> supported it.
> 
> I think you're a bit out of date.

Oh, and it's not like there isn't precedent for doing this in
lib/vsprintf.c:

841a915d20c7 vsprintf: Do not have bprintf dereference pointers

+                                       len = copy = strlen(args);

If you grep lib/, there's many more examples. So, what is in Hectors
patch is in no way any different from lots of other examples already
merged into the kernel code.

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

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Petr Mladek]

On Mon 2022-11-14 16:15:50, Russell King (Oracle) wrote:
> On Mon, Nov 14, 2022 at 04:34:07PM +0100, Petr Mladek wrote:
> > On Tue 2022-11-08 16:33:22, Russell King wrote:
> > > From: Hector Martin <marcan@marcan.st>
> > > 
> > > %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> > > it's useful to be able to print generic 4-character codes formatted as
> > > an integer. Extend it to add format specifiers for printing generic
> > > 32-bit FOURCCs with various endian semantics:
> > > 
> > > %p4ch   Host-endian
> > > %p4cl	Little-endian
> > > %p4cb	Big-endian
> > > %p4cr	Reverse-endian
> > > 
> > > The endianness determines how bytes are interpreted as a u32, and the
> > > FOURCC is then always printed MSByte-first (this is the opposite of
> > > V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> > > allow printing LSByte-first FOURCCs stored in host endian order
> > > (other than the hex form being in character order, not the integer
> > > value).
> > > 
> > > Signed-off-by: Hector Martin <marcan@marcan.st>
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > 
> > Reviewed-by: Petr Mladek <pmladek@suse.com>
> > 
> > See one nit below.
> > 
> > > --- a/lib/vsprintf.c
> > > +++ b/lib/vsprintf.c
> > > @@ -1762,27 +1762,50 @@ char *fourcc_string(char *buf, char *end, const u32 *fourcc,
> > >  	char output[sizeof("0123 little-endian (0x01234567)")];
> > >  	char *p = output;
> > >  	unsigned int i;
> > > +	bool pixel_fmt = false;
> > >  	u32 orig, val;
> > >  
> > > -	if (fmt[1] != 'c' || fmt[2] != 'c')
> > > +	if (fmt[1] != 'c')
> > >  		return error_string(buf, end, "(%p4?)", spec);
> > >  
> > >  	if (check_pointer(&buf, end, fourcc, spec))
> > >  		return buf;
> > >  
> > >  	orig = get_unaligned(fourcc);
> > > -	val = orig & ~BIT(31);
> > > +	switch (fmt[2]) {
> > > +	case 'h':
> > > +		val = orig;
> > > +		break;
> > > +	case 'r':
> > > +		val = orig = swab32(orig);
> > 
> > I do not like much these multi assignments. I think that the result
> > was not even defined in some older C standards. Though, I can't find
> > it now. And even make W=3 does not warn about it.
> 
> Err.
> 
> It's been supported for decades. I learnt about it back in 1992 when
> I was introduced to C by another experienced C programmer. It's been
> supported in ANSI C compilers. The Norcroft C compiler (which is
> strict ANSI) on Acorn platforms back in the late 1980s/1990s even
> supported it.

Ah, the problem probably was with a more complicated assignment.
For example, the result of the following code is not obvious:

	  a = b = a++;

Best Regards,
Petr

Re: [PATCH v3 2/7] lib/vsprintf: Add support for generic FOURCCs by extending %p4cc

[Petr Mladek]

On Tue 2022-11-08 16:33:22, Russell King wrote:
> From: Hector Martin <marcan@marcan.st>
> 
> %p4cc is designed for DRM/V4L2 FOURCCs with their specific quirks, but
> it's useful to be able to print generic 4-character codes formatted as
> an integer. Extend it to add format specifiers for printing generic
> 32-bit FOURCCs with various endian semantics:
> 
> %p4ch   Host-endian
> %p4cl	Little-endian
> %p4cb	Big-endian
> %p4cr	Reverse-endian
> 
> The endianness determines how bytes are interpreted as a u32, and the
> FOURCC is then always printed MSByte-first (this is the opposite of
> V4L/DRM FOURCCs). This covers most practical cases, e.g. %p4cr would
> allow printing LSByte-first FOURCCs stored in host endian order
> (other than the hex form being in character order, not the integer
> value).
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  Documentation/core-api/printk-formats.rst | 32 +++++++++++++++++++
>  lib/test_printf.c                         | 39 +++++++++++++++++++----
>  lib/vsprintf.c                            | 35 ++++++++++++++++----
>  3 files changed, 93 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst
> index dbe1aacc79d0..92a488884cf8 100644
> --- a/Documentation/core-api/printk-formats.rst
> +++ b/Documentation/core-api/printk-formats.rst
> @@ -625,6 +625,38 @@ Passed by reference.
>  	%p4cc	Y10  little-endian (0x20303159)
>  	%p4cc	NV12 big-endian (0xb231564e)
>  
> +Generic FourCC code
> +-------------------
> +
> +::
> +	%p4c[hrbl]	gP00 (0x67503030)
> +
> +Print a generic FourCC code, as both ASCII characters and its numerical
> +value as hexadecimal.
> +
> +The additional ``h``, ``r``, ``b``, and ``l`` specifiers are used to specify
> +host, reversed, big or little endian order data respectively. Host endian
> +order means the data is interpreted as a 32-bit integer and the most
> +significant byte is printed first; that is, the character code as printed
> +matches the byte order stored in memory on big-endian systems, and is reversed
> +on little-endian systems.

I though a bit more about the semantic and got a bit confused.
It might be because I am not familiar with FourCC. Anyway,
the description in the commit message provided some more clues.

The following documentation looks be more clear to me:

<proposal>
Generic FourCC code
-------------------

::
	%p4c[hrbl]	gP00 (0x67503030)

Print a generic FourCC code, as both ASCII characters and its numerical
value as hexadecimal.

The generic FourCC code is always printed in the the big-endian format,
the most significant byte first. This is the opposite of V4L/DRM
FOURCCs.

The additional ``h``, ``r``, ``b``, and ``l`` specifiers define what
endianes is used to load the stored value as 32-bit integer. The value
might be stored as host-endian, reverse-host-endian, big-endian,
or little endian.

Examples for a little-endian machine, host native load &(u32)0x67503030::

	%p4ch	gP00 (0x67503030)
	%p4cr	00Pg (0x30305067)
	%p4cb	00Pg (0x30305067)
	%p4cl	gP00 (0x67503030)

Examples for a big-endian machine, host native load &(u32)0x67503030::

	%p4ch	gP00 (0x67503030)
	%p4cr	00Pg (0x30305067)
	%p4cb	gP00 (0x67503030)
	%p4cl	00Pg (0x30305067)
</proposal>

Best Regards,
Petr

[PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

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

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

diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
new file mode 100644
index 000000000000..014eba5a1bbc
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
@@ -0,0 +1,67 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Apple Mac System Management Controller
+
+maintainers:
+  - Hector Martin <marcan@marcan.st>
+
+description:
+  Apple Mac System Management Controller implements various functions
+  such as GPIO, RTC, power, reboot.
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - apple,t6000-smc
+          - apple,t8103-smc
+          - apple,t8112-smc
+      - const: apple,smc
+
+  reg:
+    items:
+      - description: SMC area
+      - description: SRAM area
+
+  reg-names:
+    items:
+      - const: smc
+      - const: sram
+
+  mboxes:
+    maxItems: 1
+
+  gpio:
+    $ref: /schemas/gpio/gpio-macsmc.yaml
+
+additionalProperties: false
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - mboxes
+
+examples:
+  - |
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      smc@23e400000 {
+        compatible = "apple,t8103-smc", "apple,smc";
+        reg = <0x2 0x3e400000 0x0 0x4000>,
+               <0x2 0x3fe00000 0x0 0x100000>;
+        reg-names = "smc", "sram";
+        mboxes = <&smc_mbox>;
+
+        smc_gpio: gpio {
+          gpio-controller;
+          #gpio-cells = <2>;
+        };
+      };
+    };
-- 
2.30.2

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Linus Walleij]

On Tue, Nov 8, 2022 at 5:33 PM Russell King (Oracle)
<rmk+kernel@armlinux.org.uk> wrote:

> Add a DT binding for the Apple Mac System Management Controller.
>
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Looks good to me!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 08/11/2022 17:33, Russell King (Oracle) wrote:
> Add a DT binding for the Apple Mac System Management Controller.

Drop the second, redundant "binding" from subject. It's already in prefix.

> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> new file mode 100644
> index 000000000000..014eba5a1bbc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple Mac System Management Controller
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description:
> +  Apple Mac System Management Controller implements various functions
> +  such as GPIO, RTC, power, reboot.
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - apple,t6000-smc
> +          - apple,t8103-smc
> +          - apple,t8112-smc
> +      - const: apple,smc
> +
> +  reg:
> +    items:
> +      - description: SMC area
> +      - description: SRAM area
> +
> +  reg-names:
> +    items:
> +      - const: smc
> +      - const: sram
> +
> +  mboxes:
> +    maxItems: 1
> +
> +  gpio:
> +    $ref: /schemas/gpio/gpio-macsmc.yaml

So this depends on other patch, so:
1. You need mention the dependency in cover letter (nothing there),
2. Re-order patches.

The GPIO cannot go separate tree and this must be explicitly communicated.

> +
> +additionalProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - mboxes
> +
> +examples:
> +  - |
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      smc@23e400000 {

Usually these are called system-controller, to have a generic name (as
asked by DT spec).

> +        compatible = "apple,t8103-smc", "apple,smc";
> +        reg = <0x2 0x3e400000 0x0 0x4000>,
> +               <0x2 0x3fe00000 0x0 0x100000>;

Align the items (opening <).

With three above:
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > Add a DT binding for the Apple Mac System Management Controller.
> 
> Drop the second, redundant "binding" from subject. It's already in prefix.

Yet another thing that's been there from the start... how many more
things are you going to pick up in subsequent versions of the patch?
When does this stop?

In any case, taking your comment literally,

"dt-bindings: mfd: add for Apple Mac System Management Controller"

makes no sense, so presumably you want something more than that.

In any case, I see several recent cases already merged which follow
the pattern that I've used and that you've reviewed.

> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > new file mode 100644
> > index 000000000000..014eba5a1bbc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > @@ -0,0 +1,67 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Apple Mac System Management Controller
> > +
> > +maintainers:
> > +  - Hector Martin <marcan@marcan.st>
> > +
> > +description:
> > +  Apple Mac System Management Controller implements various functions
> > +  such as GPIO, RTC, power, reboot.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - apple,t6000-smc
> > +          - apple,t8103-smc
> > +          - apple,t8112-smc
> > +      - const: apple,smc
> > +
> > +  reg:
> > +    items:
> > +      - description: SMC area
> > +      - description: SRAM area
> > +
> > +  reg-names:
> > +    items:
> > +      - const: smc
> > +      - const: sram
> > +
> > +  mboxes:
> > +    maxItems: 1
> > +
> > +  gpio:
> > +    $ref: /schemas/gpio/gpio-macsmc.yaml
> 
> So this depends on other patch, so:
> 1. You need mention the dependency in cover letter (nothing there),
> 2. Re-order patches.
> 
> The GPIO cannot go separate tree and this must be explicitly communicated.

Sigh, getting an order that is sensible is really bloody difficult.
I'm quite sure Lee is only going to want to apply the mfd bits. Then
what do we do with the other bits? GPIO stuff via the GPIO tree, then
wait a cycle before the rest can be merged. Or what?

> > +
> > +additionalProperties: false
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - mboxes
> > +
> > +examples:
> > +  - |
> > +    soc {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      smc@23e400000 {
> 
> Usually these are called system-controller, to have a generic name (as
> asked by DT spec).

I'll defer to Hector for his response on this one, but you've had
had plenty of opportunities to bring this up in the past - it's been
there since the first posting.

Frustrating is definitely the word for this drip-drip-drip review.

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

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 08/11/2022 23:22, Russell King (Oracle) wrote:
> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>> Add a DT binding for the Apple Mac System Management Controller.
>>
>> Drop the second, redundant "binding" from subject. It's already in prefix.
> 
> Yet another thing that's been there from the start... how many more
> things are you going to pick up in subsequent versions of the patch?
> When does this stop?
> 
> In any case, taking your comment literally,
> 
> "dt-bindings: mfd: add for Apple Mac System Management Controller"
> 
> makes no sense, so presumably you want something more than that.

dt-bindings: mfd: add Apple Mac System Management Controller

> 
> In any case, I see several recent cases already merged which follow
> the pattern that I've used and that you've reviewed.

Any many received comments to fix. I wouldn't mention it if there was no
second issue - your patchset is non-bisectable. Since you must resend,
please adjust the subject.

> 
>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>> ---
>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>>>  1 file changed, 67 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>> new file mode 100644
>>> index 000000000000..014eba5a1bbc
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>> @@ -0,0 +1,67 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Apple Mac System Management Controller
>>> +
>>> +maintainers:
>>> +  - Hector Martin <marcan@marcan.st>
>>> +
>>> +description:
>>> +  Apple Mac System Management Controller implements various functions
>>> +  such as GPIO, RTC, power, reboot.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - apple,t6000-smc
>>> +          - apple,t8103-smc
>>> +          - apple,t8112-smc
>>> +      - const: apple,smc
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: SMC area
>>> +      - description: SRAM area
>>> +
>>> +  reg-names:
>>> +    items:
>>> +      - const: smc
>>> +      - const: sram
>>> +
>>> +  mboxes:
>>> +    maxItems: 1
>>> +
>>> +  gpio:
>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
>>
>> So this depends on other patch, so:
>> 1. You need mention the dependency in cover letter (nothing there),
>> 2. Re-order patches.
>>
>> The GPIO cannot go separate tree and this must be explicitly communicated.
> 
> Sigh, getting an order that is sensible is really bloody difficult.
> I'm quite sure Lee is only going to want to apply the mfd bits. Then
> what do we do with the other bits? GPIO stuff via the GPIO tree, then
> wait a cycle before the rest can be merged. Or what?

It's nothing new... bindings headers, drivers and DTS all have it since
years. Your case is actually easy to solve:
1. Re-order patches, describe dependency, get ack from one maintainer so
it can go via other.
2. Squash patches and describe dependency. Only one maintainer will need
to pick it up (with second's ack).

I don't understand why we talk about such basics, you are far way more
experienced in kernel development than many of us...

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> > On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > > Add a DT binding for the Apple Mac System Management Controller.
> > 
> > Drop the second, redundant "binding" from subject. It's already in prefix.
> 
> Yet another thing that's been there from the start... how many more
> things are you going to pick up in subsequent versions of the patch?
> When does this stop?
> 
> In any case, taking your comment literally,
> 
> "dt-bindings: mfd: add for Apple Mac System Management Controller"
> 
> makes no sense, so presumably you want something more than that.
> 
> In any case, I see several recent cases already merged which follow
> the pattern that I've used and that you've reviewed.
> 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > ---
> > >  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> > >  1 file changed, 67 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > new file mode 100644
> > > index 000000000000..014eba5a1bbc
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > @@ -0,0 +1,67 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Apple Mac System Management Controller
> > > +
> > > +maintainers:
> > > +  - Hector Martin <marcan@marcan.st>
> > > +
> > > +description:
> > > +  Apple Mac System Management Controller implements various functions
> > > +  such as GPIO, RTC, power, reboot.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    items:
> > > +      - enum:
> > > +          - apple,t6000-smc
> > > +          - apple,t8103-smc
> > > +          - apple,t8112-smc
> > > +      - const: apple,smc
> > > +
> > > +  reg:
> > > +    items:
> > > +      - description: SMC area
> > > +      - description: SRAM area
> > > +
> > > +  reg-names:
> > > +    items:
> > > +      - const: smc
> > > +      - const: sram
> > > +
> > > +  mboxes:
> > > +    maxItems: 1
> > > +
> > > +  gpio:
> > > +    $ref: /schemas/gpio/gpio-macsmc.yaml
> > 
> > So this depends on other patch, so:
> > 1. You need mention the dependency in cover letter (nothing there),
> > 2. Re-order patches.
> > 
> > The GPIO cannot go separate tree and this must be explicitly communicated.
> 
> Sigh, getting an order that is sensible is really bloody difficult.

It's not. Sub-devices before the MFD. The only time that doesn't work is 
when the sub-devices put the parent MFD in their example. The solution 
there is don't do that. Just 1 complete example in the MFD schema and no 
examples in the sub-devices.

> I'm quite sure Lee is only going to want to apply the mfd bits. 

Indeed. I can't seem to make Lee care... All the schemas should really 
be applied together.

> Then
> what do we do with the other bits? GPIO stuff via the GPIO tree, then
> wait a cycle before the rest can be merged. Or what?

The schemas must be picked up in the same cycle. I don't care so much 
if subsystem maintainers' trees have warnings if they don't care, but I 
do care for linux-next. If the subsystem bits aren't picked up, then 
I'll pick them up if it comes to that.

Rob

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Hector Martin]

On 10/11/2022 07.17, Rob Herring wrote:
> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>>> Add a DT binding for the Apple Mac System Management Controller.
>>>
>>> Drop the second, redundant "binding" from subject. It's already in prefix.
>>
>> Yet another thing that's been there from the start... how many more
>> things are you going to pick up in subsequent versions of the patch?
>> When does this stop?
>>
>> In any case, taking your comment literally,
>>
>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
>>
>> makes no sense, so presumably you want something more than that.
>>
>> In any case, I see several recent cases already merged which follow
>> the pattern that I've used and that you've reviewed.
>>
>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>>> ---
>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>>>>  1 file changed, 67 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>> new file mode 100644
>>>> index 000000000000..014eba5a1bbc
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>> @@ -0,0 +1,67 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Apple Mac System Management Controller
>>>> +
>>>> +maintainers:
>>>> +  - Hector Martin <marcan@marcan.st>
>>>> +
>>>> +description:
>>>> +  Apple Mac System Management Controller implements various functions
>>>> +  such as GPIO, RTC, power, reboot.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    items:
>>>> +      - enum:
>>>> +          - apple,t6000-smc
>>>> +          - apple,t8103-smc
>>>> +          - apple,t8112-smc
>>>> +      - const: apple,smc
>>>> +
>>>> +  reg:
>>>> +    items:
>>>> +      - description: SMC area
>>>> +      - description: SRAM area
>>>> +
>>>> +  reg-names:
>>>> +    items:
>>>> +      - const: smc
>>>> +      - const: sram
>>>> +
>>>> +  mboxes:
>>>> +    maxItems: 1
>>>> +
>>>> +  gpio:
>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
>>>
>>> So this depends on other patch, so:
>>> 1. You need mention the dependency in cover letter (nothing there),
>>> 2. Re-order patches.
>>>
>>> The GPIO cannot go separate tree and this must be explicitly communicated.
>>
>> Sigh, getting an order that is sensible is really bloody difficult.
> 
> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> when the sub-devices put the parent MFD in their example. The solution 
> there is don't do that. Just 1 complete example in the MFD schema and no 
> examples in the sub-devices.
> 
>> I'm quite sure Lee is only going to want to apply the mfd bits. 
> 
> Indeed. I can't seem to make Lee care... All the schemas should really 
> be applied together.
> 
>> Then
>> what do we do with the other bits? GPIO stuff via the GPIO tree, then
>> wait a cycle before the rest can be merged. Or what?
> 
> The schemas must be picked up in the same cycle. I don't care so much 
> if subsystem maintainers' trees have warnings if they don't care, but I 
> do care for linux-next. If the subsystem bits aren't picked up, then 
> I'll pick them up if it comes to that.
> 

We can take all the schemas and DT changes via asahi-soc if that works
for you. This also lets us move forward with more related DT changes
that would apply on top of things already in that branch. Then Lee only
has to take the mfd core bits (and possibly the RTKit platform part, or
we can figure something else out for that).


- Hector

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> > > On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > > > Add a DT binding for the Apple Mac System Management Controller.
> > > 
> > > Drop the second, redundant "binding" from subject. It's already in prefix.
> > 
> > Yet another thing that's been there from the start... how many more
> > things are you going to pick up in subsequent versions of the patch?
> > When does this stop?
> > 
> > In any case, taking your comment literally,
> > 
> > "dt-bindings: mfd: add for Apple Mac System Management Controller"
> > 
> > makes no sense, so presumably you want something more than that.
> > 
> > In any case, I see several recent cases already merged which follow
> > the pattern that I've used and that you've reviewed.
> > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > new file mode 100644
> > > > index 000000000000..014eba5a1bbc
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Apple Mac System Management Controller
> > > > +
> > > > +maintainers:
> > > > +  - Hector Martin <marcan@marcan.st>
> > > > +
> > > > +description:
> > > > +  Apple Mac System Management Controller implements various functions
> > > > +  such as GPIO, RTC, power, reboot.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - enum:
> > > > +          - apple,t6000-smc
> > > > +          - apple,t8103-smc
> > > > +          - apple,t8112-smc
> > > > +      - const: apple,smc
> > > > +
> > > > +  reg:
> > > > +    items:
> > > > +      - description: SMC area
> > > > +      - description: SRAM area
> > > > +
> > > > +  reg-names:
> > > > +    items:
> > > > +      - const: smc
> > > > +      - const: sram
> > > > +
> > > > +  mboxes:
> > > > +    maxItems: 1
> > > > +
> > > > +  gpio:
> > > > +    $ref: /schemas/gpio/gpio-macsmc.yaml
> > > 
> > > So this depends on other patch, so:
> > > 1. You need mention the dependency in cover letter (nothing there),
> > > 2. Re-order patches.
> > > 
> > > The GPIO cannot go separate tree and this must be explicitly communicated.
> > 
> > Sigh, getting an order that is sensible is really bloody difficult.
> 
> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> when the sub-devices put the parent MFD in their example. The solution 
> there is don't do that. Just 1 complete example in the MFD schema and no 
> examples in the sub-devices.

Meanwhile, I was told by Krzysztof that DT schemas must always have an
example. So, different person, different story.

Anyway, I've washed my hands of this farce with this series. I'm not
planning to post another version of it. I've had enough of this crap.

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

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 10/11/2022 12:48, Russell King (Oracle) wrote:
> On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
>> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
>>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
>>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>>>> Add a DT binding for the Apple Mac System Management Controller.
>>>>
>>>> Drop the second, redundant "binding" from subject. It's already in prefix.
>>>
>>> Yet another thing that's been there from the start... how many more
>>> things are you going to pick up in subsequent versions of the patch?
>>> When does this stop?
>>>
>>> In any case, taking your comment literally,
>>>
>>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
>>>
>>> makes no sense, so presumably you want something more than that.
>>>
>>> In any case, I see several recent cases already merged which follow
>>> the pattern that I've used and that you've reviewed.
>>>
>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>>>> ---
>>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>>>>>  1 file changed, 67 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..014eba5a1bbc
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>> @@ -0,0 +1,67 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>> +
>>>>> +title: Apple Mac System Management Controller
>>>>> +
>>>>> +maintainers:
>>>>> +  - Hector Martin <marcan@marcan.st>
>>>>> +
>>>>> +description:
>>>>> +  Apple Mac System Management Controller implements various functions
>>>>> +  such as GPIO, RTC, power, reboot.
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +          - apple,t6000-smc
>>>>> +          - apple,t8103-smc
>>>>> +          - apple,t8112-smc
>>>>> +      - const: apple,smc
>>>>> +
>>>>> +  reg:
>>>>> +    items:
>>>>> +      - description: SMC area
>>>>> +      - description: SRAM area
>>>>> +
>>>>> +  reg-names:
>>>>> +    items:
>>>>> +      - const: smc
>>>>> +      - const: sram
>>>>> +
>>>>> +  mboxes:
>>>>> +    maxItems: 1
>>>>> +
>>>>> +  gpio:
>>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
>>>>
>>>> So this depends on other patch, so:
>>>> 1. You need mention the dependency in cover letter (nothing there),
>>>> 2. Re-order patches.
>>>>
>>>> The GPIO cannot go separate tree and this must be explicitly communicated.
>>>
>>> Sigh, getting an order that is sensible is really bloody difficult.
>>
>> It's not. Sub-devices before the MFD. The only time that doesn't work is 
>> when the sub-devices put the parent MFD in their example. The solution 
>> there is don't do that. Just 1 complete example in the MFD schema and no 
>> examples in the sub-devices.
> 
> Meanwhile, I was told by Krzysztof that DT schemas must always have an
> example. So, different person, different story.

Hm, where do you see a message I told you to always have examples? Maybe
in some discussion I mentioned that examples are desired, but not
always. There is no point in having example in MFD child device schema
if it is already part of the parent MFD binding, where it is actually
required for complete picture.

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Nov 10, 2022 at 03:00:16PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2022 12:48, Russell King (Oracle) wrote:
> > On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
> >> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> >>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> >>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
> >>>>> Add a DT binding for the Apple Mac System Management Controller.
> >>>>
> >>>> Drop the second, redundant "binding" from subject. It's already in prefix.
> >>>
> >>> Yet another thing that's been there from the start... how many more
> >>> things are you going to pick up in subsequent versions of the patch?
> >>> When does this stop?
> >>>
> >>> In any case, taking your comment literally,
> >>>
> >>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
> >>>
> >>> makes no sense, so presumably you want something more than that.
> >>>
> >>> In any case, I see several recent cases already merged which follow
> >>> the pattern that I've used and that you've reviewed.
> >>>
> >>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >>>>> ---
> >>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> >>>>>  1 file changed, 67 insertions(+)
> >>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>> new file mode 100644
> >>>>> index 000000000000..014eba5a1bbc
> >>>>> --- /dev/null
> >>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>> @@ -0,0 +1,67 @@
> >>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>> +%YAML 1.2
> >>>>> +---
> >>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> >>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>> +
> >>>>> +title: Apple Mac System Management Controller
> >>>>> +
> >>>>> +maintainers:
> >>>>> +  - Hector Martin <marcan@marcan.st>
> >>>>> +
> >>>>> +description:
> >>>>> +  Apple Mac System Management Controller implements various functions
> >>>>> +  such as GPIO, RTC, power, reboot.
> >>>>> +
> >>>>> +properties:
> >>>>> +  compatible:
> >>>>> +    items:
> >>>>> +      - enum:
> >>>>> +          - apple,t6000-smc
> >>>>> +          - apple,t8103-smc
> >>>>> +          - apple,t8112-smc
> >>>>> +      - const: apple,smc
> >>>>> +
> >>>>> +  reg:
> >>>>> +    items:
> >>>>> +      - description: SMC area
> >>>>> +      - description: SRAM area
> >>>>> +
> >>>>> +  reg-names:
> >>>>> +    items:
> >>>>> +      - const: smc
> >>>>> +      - const: sram
> >>>>> +
> >>>>> +  mboxes:
> >>>>> +    maxItems: 1
> >>>>> +
> >>>>> +  gpio:
> >>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
> >>>>
> >>>> So this depends on other patch, so:
> >>>> 1. You need mention the dependency in cover letter (nothing there),
> >>>> 2. Re-order patches.
> >>>>
> >>>> The GPIO cannot go separate tree and this must be explicitly communicated.
> >>>
> >>> Sigh, getting an order that is sensible is really bloody difficult.
> >>
> >> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> >> when the sub-devices put the parent MFD in their example. The solution 
> >> there is don't do that. Just 1 complete example in the MFD schema and no 
> >> examples in the sub-devices.
> > 
> > Meanwhile, I was told by Krzysztof that DT schemas must always have an
> > example. So, different person, different story.
> 
> Hm, where do you see a message I told you to always have examples? Maybe
> in some discussion I mentioned that examples are desired, but not
> always. There is no point in having example in MFD child device schema
> if it is already part of the parent MFD binding, where it is actually
> required for complete picture.

35ed6e48-40e6-eb14-72de-9a0a4f5b38f8@linaro.org

and

2e2356f2-ded1-3cbf-4456-20054a8defda@linaro.org

For the GPIO macsec binding. So I'm getting contradictory information.
First you say that I need an example in the gpio macsec DT binding
yaml document.

Now I'm told it should go in the parent.

Make up your bloody minds and stop pissing me about. This is why I've
given up trying to get this in.

Getting a consistent message would be nice, but it seems impossible.

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

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 10/11/2022 15:14, Russell King (Oracle) wrote:
> On Thu, Nov 10, 2022 at 03:00:16PM +0100, Krzysztof Kozlowski wrote:
>> On 10/11/2022 12:48, Russell King (Oracle) wrote:
>>> On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
>>>> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
>>>>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
>>>>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
>>>>>>> Add a DT binding for the Apple Mac System Management Controller.
>>>>>>
>>>>>> Drop the second, redundant "binding" from subject. It's already in prefix.
>>>>>
>>>>> Yet another thing that's been there from the start... how many more
>>>>> things are you going to pick up in subsequent versions of the patch?
>>>>> When does this stop?
>>>>>
>>>>> In any case, taking your comment literally,
>>>>>
>>>>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
>>>>>
>>>>> makes no sense, so presumably you want something more than that.
>>>>>
>>>>> In any case, I see several recent cases already merged which follow
>>>>> the pattern that I've used and that you've reviewed.
>>>>>
>>>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
>>>>>>> ---
>>>>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>>>>>>>  1 file changed, 67 insertions(+)
>>>>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>>>> new file mode 100644
>>>>>>> index 000000000000..014eba5a1bbc
>>>>>>> --- /dev/null
>>>>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
>>>>>>> @@ -0,0 +1,67 @@
>>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>>> +%YAML 1.2
>>>>>>> +---
>>>>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
>>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>>> +
>>>>>>> +title: Apple Mac System Management Controller
>>>>>>> +
>>>>>>> +maintainers:
>>>>>>> +  - Hector Martin <marcan@marcan.st>
>>>>>>> +
>>>>>>> +description:
>>>>>>> +  Apple Mac System Management Controller implements various functions
>>>>>>> +  such as GPIO, RTC, power, reboot.
>>>>>>> +
>>>>>>> +properties:
>>>>>>> +  compatible:
>>>>>>> +    items:
>>>>>>> +      - enum:
>>>>>>> +          - apple,t6000-smc
>>>>>>> +          - apple,t8103-smc
>>>>>>> +          - apple,t8112-smc
>>>>>>> +      - const: apple,smc
>>>>>>> +
>>>>>>> +  reg:
>>>>>>> +    items:
>>>>>>> +      - description: SMC area
>>>>>>> +      - description: SRAM area
>>>>>>> +
>>>>>>> +  reg-names:
>>>>>>> +    items:
>>>>>>> +      - const: smc
>>>>>>> +      - const: sram
>>>>>>> +
>>>>>>> +  mboxes:
>>>>>>> +    maxItems: 1
>>>>>>> +
>>>>>>> +  gpio:
>>>>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
>>>>>>
>>>>>> So this depends on other patch, so:
>>>>>> 1. You need mention the dependency in cover letter (nothing there),
>>>>>> 2. Re-order patches.
>>>>>>
>>>>>> The GPIO cannot go separate tree and this must be explicitly communicated.
>>>>>
>>>>> Sigh, getting an order that is sensible is really bloody difficult.
>>>>
>>>> It's not. Sub-devices before the MFD. The only time that doesn't work is 
>>>> when the sub-devices put the parent MFD in their example. The solution 
>>>> there is don't do that. Just 1 complete example in the MFD schema and no 
>>>> examples in the sub-devices.
>>>
>>> Meanwhile, I was told by Krzysztof that DT schemas must always have an
>>> example. So, different person, different story.
>>
>> Hm, where do you see a message I told you to always have examples? Maybe
>> in some discussion I mentioned that examples are desired, but not
>> always. There is no point in having example in MFD child device schema
>> if it is already part of the parent MFD binding, where it is actually
>> required for complete picture.
> 
> 35ed6e48-40e6-eb14-72de-9a0a4f5b38f8@linaro.org
> 
> and

That was independent schema, no references to MFD, thus my comment. If
you post such stuff alone without indication it is part of MFD, what do
you expect from reviewers?

> 
> 2e2356f2-ded1-3cbf-4456-20054a8defda@linaro.org

Which was comment about MFD, right? It is expected to have example for
MFD. I never said it is mandatory for every schema, which you implied in
previous mailing.


> 
> For the GPIO macsec binding. So I'm getting contradictory information.
> First you say that I need an example in the gpio macsec DT binding
> yaml document.

First you split patches making reviewers life difficult. Then reviewers
don't get entire concept and they answer based what they got.

> 
> Now I'm told it should go in the parent.

After posting entire patchset with context you can get better review,
yes, that's right.

> 
> Make up your bloody minds and stop pissing me about. This is why I've
> given up trying to get this in.

I don't think it is constructive to discuss this with you anymore.

> 
> Getting a consistent message would be nice, but it seems impossible.
> 

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Nov 10, 2022 at 03:21:35PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2022 15:14, Russell King (Oracle) wrote:
> > On Thu, Nov 10, 2022 at 03:00:16PM +0100, Krzysztof Kozlowski wrote:
> >> On 10/11/2022 12:48, Russell King (Oracle) wrote:
> >>> On Wed, Nov 09, 2022 at 04:17:23PM -0600, Rob Herring wrote:
> >>>> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> >>>>> On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> >>>>>> On 08/11/2022 17:33, Russell King (Oracle) wrote:
> >>>>>>> Add a DT binding for the Apple Mac System Management Controller.
> >>>>>>
> >>>>>> Drop the second, redundant "binding" from subject. It's already in prefix.
> >>>>>
> >>>>> Yet another thing that's been there from the start... how many more
> >>>>> things are you going to pick up in subsequent versions of the patch?
> >>>>> When does this stop?
> >>>>>
> >>>>> In any case, taking your comment literally,
> >>>>>
> >>>>> "dt-bindings: mfd: add for Apple Mac System Management Controller"
> >>>>>
> >>>>> makes no sense, so presumably you want something more than that.
> >>>>>
> >>>>> In any case, I see several recent cases already merged which follow
> >>>>> the pattern that I've used and that you've reviewed.
> >>>>>
> >>>>>>> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> >>>>>>> ---
> >>>>>>>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> >>>>>>>  1 file changed, 67 insertions(+)
> >>>>>>>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>>>>
> >>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>>>> new file mode 100644
> >>>>>>> index 000000000000..014eba5a1bbc
> >>>>>>> --- /dev/null
> >>>>>>> +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> >>>>>>> @@ -0,0 +1,67 @@
> >>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>>>>>> +%YAML 1.2
> >>>>>>> +---
> >>>>>>> +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> >>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >>>>>>> +
> >>>>>>> +title: Apple Mac System Management Controller
> >>>>>>> +
> >>>>>>> +maintainers:
> >>>>>>> +  - Hector Martin <marcan@marcan.st>
> >>>>>>> +
> >>>>>>> +description:
> >>>>>>> +  Apple Mac System Management Controller implements various functions
> >>>>>>> +  such as GPIO, RTC, power, reboot.
> >>>>>>> +
> >>>>>>> +properties:
> >>>>>>> +  compatible:
> >>>>>>> +    items:
> >>>>>>> +      - enum:
> >>>>>>> +          - apple,t6000-smc
> >>>>>>> +          - apple,t8103-smc
> >>>>>>> +          - apple,t8112-smc
> >>>>>>> +      - const: apple,smc
> >>>>>>> +
> >>>>>>> +  reg:
> >>>>>>> +    items:
> >>>>>>> +      - description: SMC area
> >>>>>>> +      - description: SRAM area
> >>>>>>> +
> >>>>>>> +  reg-names:
> >>>>>>> +    items:
> >>>>>>> +      - const: smc
> >>>>>>> +      - const: sram
> >>>>>>> +
> >>>>>>> +  mboxes:
> >>>>>>> +    maxItems: 1
> >>>>>>> +
> >>>>>>> +  gpio:
> >>>>>>> +    $ref: /schemas/gpio/gpio-macsmc.yaml
> >>>>>>
> >>>>>> So this depends on other patch, so:
> >>>>>> 1. You need mention the dependency in cover letter (nothing there),
> >>>>>> 2. Re-order patches.
> >>>>>>
> >>>>>> The GPIO cannot go separate tree and this must be explicitly communicated.
> >>>>>
> >>>>> Sigh, getting an order that is sensible is really bloody difficult.
> >>>>
> >>>> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> >>>> when the sub-devices put the parent MFD in their example. The solution 
> >>>> there is don't do that. Just 1 complete example in the MFD schema and no 
> >>>> examples in the sub-devices.
> >>>
> >>> Meanwhile, I was told by Krzysztof that DT schemas must always have an
> >>> example. So, different person, different story.
> >>
> >> Hm, where do you see a message I told you to always have examples? Maybe
> >> in some discussion I mentioned that examples are desired, but not
> >> always. There is no point in having example in MFD child device schema
> >> if it is already part of the parent MFD binding, where it is actually
> >> required for complete picture.
> > 
> > 35ed6e48-40e6-eb14-72de-9a0a4f5b38f8@linaro.org
> > 
> > and
> 
> That was independent schema, no references to MFD, thus my comment. If
> you post such stuff alone without indication it is part of MFD, what do
> you expect from reviewers?
> 
> > 
> > 2e2356f2-ded1-3cbf-4456-20054a8defda@linaro.org
> 
> Which was comment about MFD, right? It is expected to have example for
> MFD. I never said it is mandatory for every schema, which you implied in
> previous mailing.
> 
> 
> > 
> > For the GPIO macsec binding. So I'm getting contradictory information.
> > First you say that I need an example in the gpio macsec DT binding
> > yaml document.
> 
> First you split patches making reviewers life difficult. Then reviewers
> don't get entire concept and they answer based what they got.
> 
> > 
> > Now I'm told it should go in the parent.
> 
> After posting entire patchset with context you can get better review,
> yes, that's right.
> 
> > 
> > Make up your bloody minds and stop pissing me about. This is why I've
> > given up trying to get this in.
> 
> I don't think it is constructive to discuss this with you anymore.
> 
> > 
> > Getting a consistent message would be nice, but it seems impossible.
> > 
> 

In which case you CLEARLY didn't read the cover message to that two
patch series.

Again, YOU are giving contradictory information.

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

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Krzysztof Kozlowski]

On 10/11/2022 15:23, Russell King (Oracle) wrote:
>>>
>>> Make up your bloody minds and stop pissing me about. This is why I've
>>> given up trying to get this in.
>>
>> I don't think it is constructive to discuss this with you anymore.
>>
>>>
>>> Getting a consistent message would be nice, but it seems impossible.
>>>
>>
> 
> In which case you CLEARLY didn't read the cover message to that two
> patch series.

I have every day 100-200 new patches, so not every cover letter gets
enough attention. Your did not say this is part of MFD, just mentioned
some build dependency.

Anyway, job of submitter is to make the patch and its context readable
to the reviewers.

There is literally nothing about including in parent MFD node in DT.
Just take a look:

https://lore.kernel.org/all/Y1q5jW8ff0aUdjPd@shell.armlinux.org.uk/

Or maybe you refer to this:
"previously posted before the last merge window."
Yes, so I will go through hundreds of emails from a week or months to
satisfy you?

"this driver is dependent on the
Apple SMC driver in order to be buildable and usable."

The only hint... it's about driver, not MFD. Make it obvious for
readers, not obvious for you.

"It is expected
that this Apple SMC driver will be merged via Lee's MFD tree."

Maybe there is dependency, maybe not, who the heck knows.

> Again, YOU are giving contradictory information.

Best regards,
Krzysztof

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Russell King (Oracle)]

On Thu, Nov 10, 2022 at 03:36:17PM +0100, Krzysztof Kozlowski wrote:
> On 10/11/2022 15:23, Russell King (Oracle) wrote:
> >>>
> >>> Make up your bloody minds and stop pissing me about. This is why I've
> >>> given up trying to get this in.
> >>
> >> I don't think it is constructive to discuss this with you anymore.
> >>
> >>>
> >>> Getting a consistent message would be nice, but it seems impossible.
> >>>
> >>
> > 
> > In which case you CLEARLY didn't read the cover message to that two
> > patch series.
> 
> I have every day 100-200 new patches, so not every cover letter gets
> enough attention. Your did not say this is part of MFD, just mentioned
> some build dependency.
> 
> Anyway, job of submitter is to make the patch and its context readable
> to the reviewers.
> 
> There is literally nothing about including in parent MFD node in DT.
> Just take a look:
> 
> https://lore.kernel.org/all/Y1q5jW8ff0aUdjPd@shell.armlinux.org.uk/
> 
> Or maybe you refer to this:
> "previously posted before the last merge window."
> Yes, so I will go through hundreds of emails from a week or months to
> satisfy you?
> 
> "this driver is dependent on the
> Apple SMC driver in order to be buildable and usable."
> 
> The only hint... it's about driver, not MFD. Make it obvious for
> readers, not obvious for you.
> 
> "It is expected
> that this Apple SMC driver will be merged via Lee's MFD tree."
> 
> Maybe there is dependency, maybe not, who the heck knows.

Everything that needed to be said was said in the cover message.

I'm not going to bother continuing with this thread because there is
nothing more to be said. This is getting utterly idiotic.

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

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Lee Jones]

On Wed, 09 Nov 2022, Rob Herring wrote:

> On Tue, Nov 08, 2022 at 10:22:31PM +0000, Russell King (Oracle) wrote:
> > On Tue, Nov 08, 2022 at 09:55:58PM +0100, Krzysztof Kozlowski wrote:
> > > On 08/11/2022 17:33, Russell King (Oracle) wrote:
> > > > Add a DT binding for the Apple Mac System Management Controller.
> > > 
> > > Drop the second, redundant "binding" from subject. It's already in prefix.
> > 
> > Yet another thing that's been there from the start... how many more
> > things are you going to pick up in subsequent versions of the patch?
> > When does this stop?
> > 
> > In any case, taking your comment literally,
> > 
> > "dt-bindings: mfd: add for Apple Mac System Management Controller"
> > 
> > makes no sense, so presumably you want something more than that.
> > 
> > In any case, I see several recent cases already merged which follow
> > the pattern that I've used and that you've reviewed.
> > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > > ---
> > > >  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
> > > >  1 file changed, 67 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mfd/apple,smc.yaml b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > new file mode 100644
> > > > index 000000000000..014eba5a1bbc
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/mfd/apple,smc.yaml
> > > > @@ -0,0 +1,67 @@
> > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > +%YAML 1.2
> > > > +---
> > > > +$id: http://devicetree.org/schemas/mfd/apple,smc.yaml#
> > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > +
> > > > +title: Apple Mac System Management Controller
> > > > +
> > > > +maintainers:
> > > > +  - Hector Martin <marcan@marcan.st>
> > > > +
> > > > +description:
> > > > +  Apple Mac System Management Controller implements various functions
> > > > +  such as GPIO, RTC, power, reboot.
> > > > +
> > > > +properties:
> > > > +  compatible:
> > > > +    items:
> > > > +      - enum:
> > > > +          - apple,t6000-smc
> > > > +          - apple,t8103-smc
> > > > +          - apple,t8112-smc
> > > > +      - const: apple,smc
> > > > +
> > > > +  reg:
> > > > +    items:
> > > > +      - description: SMC area
> > > > +      - description: SRAM area
> > > > +
> > > > +  reg-names:
> > > > +    items:
> > > > +      - const: smc
> > > > +      - const: sram
> > > > +
> > > > +  mboxes:
> > > > +    maxItems: 1
> > > > +
> > > > +  gpio:
> > > > +    $ref: /schemas/gpio/gpio-macsmc.yaml
> > > 
> > > So this depends on other patch, so:
> > > 1. You need mention the dependency in cover letter (nothing there),
> > > 2. Re-order patches.
> > > 
> > > The GPIO cannot go separate tree and this must be explicitly communicated.
> > 
> > Sigh, getting an order that is sensible is really bloody difficult.
> 
> It's not. Sub-devices before the MFD. The only time that doesn't work is 
> when the sub-devices put the parent MFD in their example. The solution 
> there is don't do that. Just 1 complete example in the MFD schema and no 
> examples in the sub-devices.
> 
> > I'm quite sure Lee is only going to want to apply the mfd bits. 
> 
> Indeed. I can't seem to make Lee care... All the schemas should really 
> be applied together.

I care about drivers.  Happy to take the set as a whole if it helps.

-- 
Lee Jones [李琼斯]

Re: [PATCH v3 3/7] dt-bindings: mfd: add binding for Apple Mac System Management Controller

[Rob Herring]

On Tue, 08 Nov 2022 16:33:27 +0000, Russell King (Oracle) wrote:
> Add a DT binding for the Apple Mac System Management Controller.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/mfd/apple,smc.yaml    | 67 +++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mfd/apple,smc.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/mfd/apple,smc.yaml: Unable to find schema file matching $id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/apple,smc.example.dtb: smc@23e400000: gpio: False schema does not allow {'gpio-controller': True, '#gpio-cells': [[2]]}
	From schema: /builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/mfd/apple,smc.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v3 4/7] platform/apple: Add new Apple Mac SMC driver

[Russell King]

From: Hector Martin <marcan@marcan.st>

This driver implements support for the SMC (System Management
Controller) in Apple Macs. In contrast to the existing applesmc driver,
it uses pluggable backends that allow it to support different SMC
implementations, and uses the MFD subsystem to expose the core SMC
functionality so that specific features (gpio, hwmon, battery, etc.) can
be implemented by separate drivers in their respective downstream
subsystems.

The initial RTKit backend adds support for Apple Silicon Macs (M1 et
al). We hope a backend for T2 Macs will be written in the future
(since those are not supported by applesmc), and eventually an x86
backend would allow us to fully deprecate applesmc in favor of this
driver.

Signed-off-by: Hector Martin <marcan@marcan.st>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/platform/Kconfig              |   2 +
 drivers/platform/Makefile             |   1 +
 drivers/platform/apple/Kconfig        |  34 ++
 drivers/platform/apple/Makefile       |   7 +
 drivers/platform/apple/macsmc-rtkit.c | 455 ++++++++++++++++++++++++++
 5 files changed, 499 insertions(+)
 create mode 100644 drivers/platform/apple/Kconfig
 create mode 100644 drivers/platform/apple/Makefile
 create mode 100644 drivers/platform/apple/macsmc-rtkit.c

diff --git a/drivers/platform/Kconfig b/drivers/platform/Kconfig
index dbd327712205..31a361ecfc5e 100644
--- a/drivers/platform/Kconfig
+++ b/drivers/platform/Kconfig
@@ -15,4 +15,6 @@ source "drivers/platform/olpc/Kconfig"
 
 source "drivers/platform/surface/Kconfig"
 
+source "drivers/platform/apple/Kconfig"
+
 source "drivers/platform/x86/Kconfig"
diff --git a/drivers/platform/Makefile b/drivers/platform/Makefile
index 41640172975a..d2baa4eb4f13 100644
--- a/drivers/platform/Makefile
+++ b/drivers/platform/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OLPC_EC)		+= olpc/
 obj-$(CONFIG_GOLDFISH)		+= goldfish/
 obj-$(CONFIG_CHROME_PLATFORMS)	+= chrome/
 obj-$(CONFIG_SURFACE_PLATFORMS)	+= surface/
+obj-$(CONFIG_APPLE_PLATFORMS)	+= apple/
diff --git a/drivers/platform/apple/Kconfig b/drivers/platform/apple/Kconfig
new file mode 100644
index 000000000000..35963c348d05
--- /dev/null
+++ b/drivers/platform/apple/Kconfig
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Apple Platform-Specific Drivers
+#
+
+menuconfig APPLE_PLATFORMS
+	bool "Apple Mac Platform-Specific Device Drivers"
+	depends on ARCH_APPLE || X86 || COMPILE_TEST
+	default ARCH_APPLE
+	help
+	  Say Y here to get to see options for platform-specific device drivers
+	  for Apple devices. This option alone does not add any kernel code.
+
+	  If you say N, all options in this submenu will be skipped and disabled.
+
+if APPLE_PLATFORMS
+
+config APPLE_SMC_RTKIT
+	tristate "Apple SMC RTKit backend"
+	depends on ARCH_APPLE || (COMPILE_TEST && 64BIT)
+	depends on APPLE_RTKIT
+	default ARCH_APPLE
+	select MFD_MACSMC
+	help
+	  Build support for the Apple System Management Controller found in
+	  Apple Macs, communicated with via the RTKit backend. This is required
+	  for Apple Silicon Macs.
+
+	  Say Y here if you have an Apple Silicon Mac.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called macsmc-rtkit.
+
+endif
diff --git a/drivers/platform/apple/Makefile b/drivers/platform/apple/Makefile
new file mode 100644
index 000000000000..16cce9061ee8
--- /dev/null
+++ b/drivers/platform/apple/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for linux/drivers/platform/apple
+# Apple Platform-Specific Drivers
+#
+
+obj-$(CONFIG_APPLE_SMC_RTKIT)		+= macsmc-rtkit.o
diff --git a/drivers/platform/apple/macsmc-rtkit.c b/drivers/platform/apple/macsmc-rtkit.c
new file mode 100644
index 000000000000..81560fb73e06
--- /dev/null
+++ b/drivers/platform/apple/macsmc-rtkit.c
@@ -0,0 +1,455 @@
+// SPDX-License-Identifier: GPL-2.0-only OR MIT
+/*
+ * Apple SMC RTKit backend
+ * Copyright The Asahi Linux Contributors
+ */
+
+#include <linux/bitfield.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/mfd/macsmc.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/soc/apple/rtkit.h>
+#include <asm/unaligned.h>
+
+#define SMC_ENDPOINT			0x20
+
+/* Guess */
+#define SMC_SHMEM_SIZE			0x1000
+#define SMC_MAX_SIZE			255
+
+#define SMC_MSG_READ_KEY		0x10
+#define SMC_MSG_WRITE_KEY		0x11
+#define SMC_MSG_GET_KEY_BY_INDEX	0x12
+#define SMC_MSG_GET_KEY_INFO		0x13
+#define SMC_MSG_INITIALIZE		0x17
+#define SMC_MSG_NOTIFICATION		0x18
+#define SMC_MSG_RW_KEY			0x20
+
+#define SMC_DATA			GENMASK(63, 32)
+#define SMC_WSIZE			GENMASK(31, 24)
+#define SMC_SIZE			GENMASK(23, 16)
+#define SMC_ID				GENMASK(15, 12)
+#define SMC_MSG				GENMASK(7, 0)
+#define SMC_RESULT			SMC_MSG
+
+#define SMC_RECV_TIMEOUT		500
+
+struct apple_smc_rtkit {
+	struct device *dev;
+	struct apple_smc *core;
+	struct apple_rtkit *rtk;
+
+	struct completion init_done;
+	bool initialized;
+	bool alive;
+
+	struct resource *sram;
+	void __iomem *sram_base;
+	struct apple_rtkit_shmem shmem;
+
+	unsigned int msg_id;
+
+	bool atomic_pending;
+	struct completion cmd_done;
+	u64 cmd_ret;
+};
+
+static int apple_smc_rtkit_write_key_atomic(void *cookie, smc_key key, void *buf, size_t size)
+{
+	struct apple_smc_rtkit *smc = cookie;
+	int ret;
+	u64 msg;
+	u8 result;
+
+	if (size > SMC_MAX_SIZE || size == 0)
+		return -EINVAL;
+
+	if (!smc->alive)
+		return -EIO;
+
+	memcpy_toio(smc->shmem.iomem, buf, size);
+	smc->msg_id = (smc->msg_id + 1) & 0xf;
+	msg = (FIELD_PREP(SMC_MSG, SMC_MSG_WRITE_KEY) |
+	       FIELD_PREP(SMC_SIZE, size) |
+	       FIELD_PREP(SMC_ID, smc->msg_id) |
+	       FIELD_PREP(SMC_DATA, key));
+	smc->atomic_pending = true;
+
+	ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, true);
+	if (ret < 0) {
+		dev_err(smc->dev, "Failed to send command (%d)\n", ret);
+		return ret;
+	}
+
+	while (smc->atomic_pending) {
+		ret = apple_rtkit_poll(smc->rtk);
+		if (ret < 0) {
+			dev_err(smc->dev, "RTKit poll failed (%llx)", msg);
+			return ret;
+		}
+		udelay(100);
+	}
+
+	if (FIELD_GET(SMC_ID, smc->cmd_ret) != smc->msg_id) {
+		dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n",
+			smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret));
+		return -EIO;
+	}
+
+	result = FIELD_GET(SMC_RESULT, smc->cmd_ret);
+	if (result != 0)
+		return -result;
+
+	return FIELD_GET(SMC_SIZE, smc->cmd_ret);
+}
+
+static int apple_smc_cmd(struct apple_smc_rtkit *smc, u64 cmd, u64 arg,
+			 u64 size, u64 wsize, u32 *ret_data)
+{
+	int ret;
+	u64 msg;
+	u8 result;
+
+	if (!smc->alive)
+		return -EIO;
+
+	reinit_completion(&smc->cmd_done);
+
+	smc->msg_id = (smc->msg_id + 1) & 0xf;
+	msg = (FIELD_PREP(SMC_MSG, cmd) |
+	       FIELD_PREP(SMC_SIZE, size) |
+	       FIELD_PREP(SMC_WSIZE, wsize) |
+	       FIELD_PREP(SMC_ID, smc->msg_id) |
+	       FIELD_PREP(SMC_DATA, arg));
+
+	ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT, msg, NULL, false);
+	if (ret < 0) {
+		dev_err(smc->dev, "Failed to send command\n");
+		return ret;
+	}
+
+	do {
+		if (wait_for_completion_timeout(&smc->cmd_done,
+						msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) {
+			dev_err(smc->dev, "Command timed out (%llx)", msg);
+			return -ETIMEDOUT;
+		}
+		if (FIELD_GET(SMC_ID, smc->cmd_ret) == smc->msg_id)
+			break;
+		dev_err(smc->dev, "Command sequence mismatch (expected %d, got %d)\n",
+			smc->msg_id, (unsigned int)FIELD_GET(SMC_ID, smc->cmd_ret));
+	} while(1);
+
+	result = FIELD_GET(SMC_RESULT, smc->cmd_ret);
+	if (result != 0)
+		return -result;
+
+	if (ret_data)
+		*ret_data = FIELD_GET(SMC_DATA, smc->cmd_ret);
+
+	return FIELD_GET(SMC_SIZE, smc->cmd_ret);
+}
+
+static int _apple_smc_rtkit_read_key(struct apple_smc_rtkit *smc, smc_key key,
+				     void *buf, size_t size, size_t wsize)
+{
+	int ret;
+	u32 rdata;
+	u64 cmd;
+
+	if (size > SMC_MAX_SIZE || size == 0)
+		return -EINVAL;
+
+	cmd = wsize ? SMC_MSG_RW_KEY : SMC_MSG_READ_KEY;
+
+	ret = apple_smc_cmd(smc, cmd, key, size, wsize, &rdata);
+	if (ret < 0)
+		return ret;
+
+	if (size <= 4)
+		memcpy(buf, &rdata, size);
+	else
+		memcpy_fromio(buf, smc->shmem.iomem, size);
+
+	return ret;
+}
+
+static int apple_smc_rtkit_read_key(void *cookie, smc_key key, void *buf, size_t size)
+{
+	return _apple_smc_rtkit_read_key(cookie, key, buf, size, 0);
+}
+
+static int apple_smc_rtkit_write_key(void *cookie, smc_key key, void *buf, size_t size)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	if (size > SMC_MAX_SIZE || size == 0)
+		return -EINVAL;
+
+	memcpy_toio(smc->shmem.iomem, buf, size);
+	return apple_smc_cmd(smc, SMC_MSG_WRITE_KEY, key, size, 0, NULL);
+}
+
+static int apple_smc_rtkit_rw_key(void *cookie, smc_key key,
+				  void *wbuf, size_t wsize, void *rbuf, size_t rsize)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	if (wsize > SMC_MAX_SIZE || wsize == 0)
+		return -EINVAL;
+
+	memcpy_toio(smc->shmem.iomem, wbuf, wsize);
+	return _apple_smc_rtkit_read_key(smc, key, rbuf, rsize, wsize);
+}
+
+static int apple_smc_rtkit_get_key_by_index(void *cookie, int index, smc_key *key)
+{
+	struct apple_smc_rtkit *smc = cookie;
+	int ret;
+
+	ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_BY_INDEX, index, 0, 0, key);
+
+	*key = swab32(*key);
+	return ret;
+}
+
+static int apple_smc_rtkit_get_key_info(void *cookie, smc_key key, struct apple_smc_key_info *info)
+{
+	struct apple_smc_rtkit *smc = cookie;
+	u8 key_info[6];
+	int ret;
+
+	ret = apple_smc_cmd(smc, SMC_MSG_GET_KEY_INFO, key, 0, 0, NULL);
+	if (ret >= 0 && info) {
+		memcpy_fromio(key_info, smc->shmem.iomem, sizeof(key_info));
+		info->size = key_info[0];
+		info->type_code = get_unaligned_be32(&key_info[1]);
+		info->flags = key_info[5];
+	}
+	return ret;
+}
+
+static const struct apple_smc_backend_ops apple_smc_rtkit_be_ops = {
+	.read_key = apple_smc_rtkit_read_key,
+	.write_key = apple_smc_rtkit_write_key,
+	.write_key_atomic = apple_smc_rtkit_write_key_atomic,
+	.rw_key = apple_smc_rtkit_rw_key,
+	.get_key_by_index = apple_smc_rtkit_get_key_by_index,
+	.get_key_info = apple_smc_rtkit_get_key_info,
+};
+
+static void apple_smc_rtkit_crashed(void *cookie)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	dev_err(smc->dev, "SMC crashed! Your system will reboot in a few seconds...\n");
+	smc->alive = false;
+}
+
+static int apple_smc_rtkit_shmem_setup(void *cookie, struct apple_rtkit_shmem *bfr)
+{
+	struct apple_smc_rtkit *smc = cookie;
+	struct resource res = {
+		.start = bfr->iova,
+		.end = bfr->iova + bfr->size - 1,
+		.name = "rtkit_map",
+		.flags = smc->sram->flags,
+	};
+
+	if (!bfr->iova) {
+		dev_err(smc->dev, "RTKit wants a RAM buffer\n");
+		return -EIO;
+	}
+
+	if (res.end < res.start || !resource_contains(smc->sram, &res)) {
+		dev_err(smc->dev,
+			"RTKit buffer request outside SRAM region: %pR", &res);
+		return -EFAULT;
+	}
+
+	bfr->iomem = smc->sram_base + (res.start - smc->sram->start);
+	bfr->is_mapped = true;
+
+	return 0;
+}
+
+static void apple_smc_rtkit_shmem_destroy(void *cookie, struct apple_rtkit_shmem *bfr)
+{
+	// no-op
+}
+
+static bool apple_smc_rtkit_recv_early(void *cookie, u8 endpoint, u64 message)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	if (endpoint != SMC_ENDPOINT) {
+		dev_err(smc->dev, "Received message for unknown endpoint 0x%x\n", endpoint);
+		return false;
+	}
+
+	if (!smc->initialized) {
+		int ret;
+
+		smc->shmem.iova = message;
+		smc->shmem.size = SMC_SHMEM_SIZE;
+		ret = apple_smc_rtkit_shmem_setup(smc, &smc->shmem);
+		if (ret < 0)
+			dev_err(smc->dev, "Failed to initialize shared memory\n");
+		else
+			smc->alive = true;
+		smc->initialized = true;
+		complete(&smc->init_done);
+	} else if (FIELD_GET(SMC_MSG, message) == SMC_MSG_NOTIFICATION) {
+		/* Handle these in the RTKit worker thread */
+		return false;
+	} else {
+		smc->cmd_ret = message;
+		if (smc->atomic_pending) {
+			smc->atomic_pending = false;
+		} else {
+			complete(&smc->cmd_done);
+		}
+	}
+
+	return true;
+}
+
+static void apple_smc_rtkit_recv(void *cookie, u8 endpoint, u64 message)
+{
+	struct apple_smc_rtkit *smc = cookie;
+
+	if (endpoint != SMC_ENDPOINT) {
+		dev_err(smc->dev, "Received message for unknown endpoint 0x%x\n", endpoint);
+		return;
+	}
+
+	if (FIELD_GET(SMC_MSG, message) != SMC_MSG_NOTIFICATION) {
+		dev_err(smc->dev, "Received unknown message from worker: 0x%llx\n", message);
+		return;
+	}
+
+	apple_smc_event_received(smc->core, FIELD_GET(SMC_DATA, message));
+}
+
+static const struct apple_rtkit_ops apple_smc_rtkit_ops = {
+	.crashed = apple_smc_rtkit_crashed,
+	.recv_message = apple_smc_rtkit_recv,
+	.recv_message_early = apple_smc_rtkit_recv_early,
+	.shmem_setup = apple_smc_rtkit_shmem_setup,
+	.shmem_destroy = apple_smc_rtkit_shmem_destroy,
+};
+
+static int apple_smc_rtkit_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct apple_smc_rtkit *smc;
+	int ret;
+
+	smc = devm_kzalloc(dev, sizeof(*smc), GFP_KERNEL);
+	if (!smc)
+		return -ENOMEM;
+
+	smc->dev = dev;
+
+	smc->sram = platform_get_resource_byname(pdev, IORESOURCE_MEM, "sram");
+	if (!smc->sram)
+		return dev_err_probe(dev, EIO,
+				     "No SRAM region");
+
+	smc->sram_base = devm_ioremap_resource(dev, smc->sram);
+	if (IS_ERR(smc->sram_base))
+		return dev_err_probe(dev, PTR_ERR(smc->sram_base),
+				     "Failed to map SRAM region");
+
+	smc->rtk =
+		devm_apple_rtkit_init(dev, smc, NULL, 0, &apple_smc_rtkit_ops);
+	if (IS_ERR(smc->rtk))
+		return dev_err_probe(dev, PTR_ERR(smc->rtk),
+				     "Failed to intialize RTKit");
+
+	ret = apple_rtkit_wake(smc->rtk);
+	if (ret != 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to wake up SMC");
+
+	ret = apple_rtkit_start_ep(smc->rtk, SMC_ENDPOINT);
+	if (ret != 0) {
+		dev_err(dev, "Failed to start endpoint");
+		goto cleanup;
+	}
+
+	init_completion(&smc->init_done);
+	init_completion(&smc->cmd_done);
+
+	ret = apple_rtkit_send_message(smc->rtk, SMC_ENDPOINT,
+				       FIELD_PREP(SMC_MSG, SMC_MSG_INITIALIZE), NULL, false);
+	if (ret < 0) {
+		ret = dev_err_probe(dev, ret, "Failed to send init message");
+		goto cleanup;
+	}
+
+	if (wait_for_completion_timeout(&smc->init_done,
+					msecs_to_jiffies(SMC_RECV_TIMEOUT)) == 0) {
+		ret = -ETIMEDOUT;
+		dev_err(dev, "Timed out initializing SMC");
+		goto cleanup;
+	}
+
+	if (!smc->alive) {
+		ret = -EIO;
+		goto cleanup;
+	}
+
+	smc->core = apple_smc_probe(dev, &apple_smc_rtkit_be_ops, smc);
+	if (IS_ERR(smc->core)) {
+		ret = PTR_ERR(smc->core);
+		goto cleanup;
+	}
+
+	return 0;
+
+cleanup:
+	/* Try to shut down RTKit, if it's not completely wedged */
+	if (apple_rtkit_is_running(smc->rtk))
+		apple_rtkit_quiesce(smc->rtk);
+
+	return ret;
+}
+
+static int apple_smc_rtkit_remove(struct platform_device *pdev)
+{
+	struct apple_smc *core = platform_get_drvdata(pdev);
+	struct apple_smc_rtkit *smc = apple_smc_get_cookie(core);
+
+	apple_smc_remove(core);
+
+	if (apple_rtkit_is_running(smc->rtk))
+		apple_rtkit_quiesce(smc->rtk);
+
+	return 0;
+}
+
+static const struct of_device_id apple_smc_rtkit_of_match[] = {
+	{ .compatible = "apple,smc" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, apple_smc_rtkit_of_match);
+
+static struct platform_driver apple_smc_rtkit_driver = {
+	.driver = {
+		.name = "macsmc-rtkit",
+		.owner = THIS_MODULE,
+		.of_match_table = apple_smc_rtkit_of_match,
+	},
+	.probe = apple_smc_rtkit_probe,
+	.remove = apple_smc_rtkit_remove,
+};
+module_platform_driver(apple_smc_rtkit_driver);
+
+MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
+MODULE_LICENSE("Dual MIT/GPL");
+MODULE_DESCRIPTION("Apple SMC RTKit backend driver");
-- 
2.30.2

[PATCH v3 5/7] arm64: dts: apple: Add SMC node to t8103 devicetrees

[Russell King]

From: Hector Martin <marcan@marcan.st>

Signed-off-by: Hector Martin <marcan@marcan.st>
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>
---
 arch/arm64/boot/dts/apple/t8103.dtsi | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/arch/arm64/boot/dts/apple/t8103.dtsi b/arch/arm64/boot/dts/apple/t8103.dtsi
index 51a63b29d404..e44ba4e541f6 100644
--- a/arch/arm64/boot/dts/apple/t8103.dtsi
+++ b/arch/arm64/boot/dts/apple/t8103.dtsi
@@ -336,6 +336,33 @@ wdt: watchdog@23d2b0000 {
 			interrupts = <AIC_IRQ 338 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		smc: smc@23e400000 {
+			compatible = "apple,t8103-smc", "apple,smc";
+			reg = <0x2 0x3e400000 0x0 0x4000>,
+				<0x2 0x3fe00000 0x0 0x100000>;
+			reg-names = "smc", "sram";
+			mboxes = <&smc_mbox>;
+
+			smc_gpio: gpio {
+				compatible = "apple,smc-gpio";
+				gpio-controller;
+				#gpio-cells = <2>;
+			};
+		};
+
+		smc_mbox: mbox@23e408000 {
+			compatible = "apple,t8103-asc-mailbox", "apple,asc-mailbox-v4";
+			reg = <0x2 0x3e408000 0x0 0x4000>;
+			interrupt-parent = <&aic>;
+			interrupts = <AIC_IRQ 400 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_IRQ 401 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_IRQ 402 IRQ_TYPE_LEVEL_HIGH>,
+				<AIC_IRQ 403 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "send-empty", "send-not-empty",
+				"recv-empty", "recv-not-empty";
+			#mbox-cells = <0>;
+		};
+
 		pinctrl_smc: pinctrl@23e820000 {
 			compatible = "apple,t8103-pinctrl", "apple,pinctrl";
 			reg = <0x2 0x3e820000 0x0 0x4000>;
-- 
2.30.2

[PATCH v3 6/7] 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>
---
 .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml

diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
new file mode 100644
index 000000000000..1a415b78760b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
@@ -0,0 +1,37 @@
+# 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:
+  Apple Mac System Management Controller GPIO block.
+
+properties:
+  compatible:
+    const: apple,smc-gpio
+
+  gpio-controller: true
+
+  '#gpio-cells':
+    const: 2
+
+required:
+  - compatible
+  - gpio-controller
+  - '#gpio-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    gpio {
+        compatible = "apple,smc-gpio";
+        gpio-controller;
+        #gpio-cells = <2>;
+    };
-- 
2.30.2

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 08/11/2022 17:33, 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>
> ---
>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> new file mode 100644
> index 000000000000..1a415b78760b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> @@ -0,0 +1,37 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#

Does not look like you tested the bindings. Please run `make
dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Apple Mac System Management Controller GPIO
> +
> +maintainers:
> +  - Hector Martin <marcan@marcan.st>
> +
> +description:
> +  Apple Mac System Management Controller GPIO block.
> +

Best regards,
Krzysztof

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
> On 08/11/2022 17:33, 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>
> > ---
> >  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
> >  1 file changed, 37 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> > new file mode 100644
> > index 000000000000..1a415b78760b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> > @@ -0,0 +1,37 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
> 
> Does not look like you tested the bindings. Please run `make
> dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).

Oh ffs. DT bindings are utterly impossible to get correct.

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

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Hector Martin]

On 09/11/2022 07.09, Russell King (Oracle) wrote:
> On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
>> On 08/11/2022 17:33, 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>
>>> ---
>>>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>>>  1 file changed, 37 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>> new file mode 100644
>>> index 000000000000..1a415b78760b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>> @@ -0,0 +1,37 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>
>> Does not look like you tested the bindings. Please run `make
>> dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> 
> Oh ffs. DT bindings are utterly impossible to get correct.

I'd be happy to wrap the bindings up in another cycle & take them via
asahi-soc, if you want. That will also allow us to add the nodes to the
t6000 DTs which are on that tree for this cycle, and unblock merging the
driver bits of this series. Bindings updates are not a hard dependency
for drivers, only for the DTs themselves.

That is:

1-2,4 via mfd tree (if Lee agrees to merging the RTKit driver platform
bits that way)
3,5-6 via asahi-soc
7 via GPIO (I think)

- Hector

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 08/11/2022 23:09, Russell King (Oracle) wrote:
> On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
>> On 08/11/2022 17:33, 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>
>>> ---
>>>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>>>  1 file changed, 37 insertions(+)
>>>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>> new file mode 100644
>>> index 000000000000..1a415b78760b
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>> @@ -0,0 +1,37 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>
>> Does not look like you tested the bindings. Please run `make
>> dt_binding_check` (see
>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> 
> Oh ffs. DT bindings are utterly impossible to get correct.

Why? You write, you test and then you see the error. Srsly...

Best regards,
Krzysztof

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Russell King (Oracle)]

On Wed, Nov 09, 2022 at 09:36:12AM +0100, Krzysztof Kozlowski wrote:
> On 08/11/2022 23:09, Russell King (Oracle) wrote:
> > On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
> >> On 08/11/2022 17:33, 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>
> >>> ---
> >>>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
> >>>  1 file changed, 37 insertions(+)
> >>>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> >>> new file mode 100644
> >>> index 000000000000..1a415b78760b
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> >>> @@ -0,0 +1,37 @@
> >>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >>> +%YAML 1.2
> >>> +---
> >>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
> >>
> >> Does not look like you tested the bindings. Please run `make
> >> dt_binding_check` (see
> >> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> > 
> > Oh ffs. DT bindings are utterly impossible to get correct.
> 
> Why? You write, you test and then you see the error. Srsly...

It's utterly impossible for me to remember the dependencies. It's
utterly impossible for me to remember the make incantation to test
this bloody things - especially as I need to limit it otherwise it
takes a very long time to test.

The whole bloody thing is just too sodding difficult.

I'm not going to bother with this patch set any more. Too frustrated
with this piecemeal approach and the crappy slow tooling and the
idiotic incomprehensible error messages it spits out.

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

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Krzysztof Kozlowski]

On 09/11/2022 10:12, Russell King (Oracle) wrote:
> On Wed, Nov 09, 2022 at 09:36:12AM +0100, Krzysztof Kozlowski wrote:
>> On 08/11/2022 23:09, Russell King (Oracle) wrote:
>>> On Tue, Nov 08, 2022 at 09:56:40PM +0100, Krzysztof Kozlowski wrote:
>>>> On 08/11/2022 17:33, 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>
>>>>> ---
>>>>>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>>>>>  1 file changed, 37 insertions(+)
>>>>>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>>> new file mode 100644
>>>>> index 000000000000..1a415b78760b
>>>>> --- /dev/null
>>>>> +++ b/Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
>>>>> @@ -0,0 +1,37 @@
>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>> +%YAML 1.2
>>>>> +---
>>>>> +$id: http://devicetree.org/schemas/gpio/gpio-macsmc.yaml#
>>>>
>>>> Does not look like you tested the bindings. Please run `make
>>>> dt_binding_check` (see
>>>> Documentation/devicetree/bindings/writing-schema.rst for instructions).
>>>
>>> Oh ffs. DT bindings are utterly impossible to get correct.
>>
>> Why? You write, you test and then you see the error. Srsly...
> 
> It's utterly impossible for me to remember the dependencies. It's
> utterly impossible for me to remember the make incantation to test
> this bloody things - especially as I need to limit it otherwise it
> takes a very long time to test.

Then this is where I can help - you can test just one, specific binding
or few of them:

make dt_binding_check DT_SCHEMA_FILES=apple,smc-gpio.yaml

make dt_binding_check DT_SCHEMA_FILES=apple,smc

So before you format patches, just do `git rebase -i HEAD~7`, change to
re-word or edit and  go one-by-one testing your bisectability. The same
we are always doing for regular kernel builds when having a big
patchset, right? Each commit must compile, not just the last one in
patchset.

Or push each of the commit to a service which can do it for you. For
example, for kernel building (not yet ready for DT binding check) I am
using something like:
https://github.com/krzk/tools/tree/master/git-build-kernel

> 
> The whole bloody thing is just too sodding difficult.
> 
> I'm not going to bother with this patch set any more. Too frustrated
> with this piecemeal approach and the crappy slow tooling and the
> idiotic incomprehensible error messages it spits out.

Best regards,
Krzysztof

Re: [PATCH v3 6/7] dt-bindings: gpio: add binding for the GPIO block for Apple Mac SMC

[Rob Herring]

On Tue, 08 Nov 2022 16:33:42 +0000, 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>
> ---
>  .../bindings/gpio/apple,smc-gpio.yaml         | 37 +++++++++++++++++++
>  1 file changed, 37 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
./Documentation/devicetree/bindings/gpio/apple,smc-gpio.yaml: $id: relative path/filename doesn't match actual path or filename
	expected: http://devicetree.org/schemas/gpio/apple,smc-gpio.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

[PATCH v3 7/7] 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 a01af1180616..08138322010a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1283,6 +1283,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 MFD_MACSMC
+	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 29e3beb6548c..467833cc9a9d 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