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

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

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

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?