Re: [PATCH 4/6] platform/apple: Add new Apple Mac SMC driver

["Russell King (Oracle)" <linux@xxxxxxxxxxxxxxx>]

Hi Lee,

One of the things we discussed on IRC was moving the shim to a header
file. Moving code into functions in a header file requires the
functions to be marked "static inline", and when I mentioned that
you seemed to be rather surprised.

They need to be static to give them compilation-unit scope so two
different users of them don't expose the functions to each other,
creating a linker error. They also need to be "inline" so the
compiler knows that they're supposed to be shim-style functions and
avoids the compiler warning that static functions aren't being used.

Most can be moved out, but we end up with one rather large function
which feels way too bulky to move into a header file, that being
apple_smc_find_first_key_index().

So, this is what it looks like moving everything except that function,
and it means we need somewhere for that function to live outside of a
header file. This raises the question whether there really is much
point to moving the shim out.

If we do manage to move the shim out, we're left with the shim
initialisation and the MFD bits left in smc_core.c, and we've already
established that oving the MFD bits isn't what you're after. So, we
could move that initialisation and MFD bits to smc_rtkit.c... but then
what happens when the next backend comes along (which will be for Intel
based MACs), we would need to duplicate that code in another backend.

To me, making this change just feels wrong - not just mildly wrong -
because it exposes things that consumers of the SMC API offered by the
core shouldn't really know about, such as the contents of struct
apple_smc and struct apple_smc_backend_ops. I'm really not sure doing
this results in an improvement. Here's the changes on top of this
patch set anyway.

>From what I understand that you want for MFD, you want a single .c file
that is a device driver itself, which then talks directly to MFD with
nothing else inbetween.

The only way I can think of achieving that is to essentially move the
contents of smc_rtkit.c into smc_core.c, and when it comes to merging
the Intel SMC support, that also has to go into smc_core.c as well -
which means we end up with a lot of dead code and #ifdef's in that .c
file - which is not nice.

 drivers/platform/apple/smc.h      |  24 +++---
 drivers/platform/apple/smc_core.c | 127 ----------------------------
 include/linux/mfd/macsmc.h        | 136 +++++++++++++++++++++++++++---
 3 files changed, 138 insertions(+), 149 deletions(-)

diff --git a/drivers/platform/apple/smc.h b/drivers/platform/apple/smc.h
index 8ae51887b2c5..fa86411d5044 100644
--- a/drivers/platform/apple/smc.h
+++ b/drivers/platform/apple/smc.h
@@ -7,22 +7,24 @@
 #ifndef _SMC_H
 #define _SMC_H
 
+#include <linux/device.h>
 #include <linux/mfd/macsmc.h>
 
-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);
-};
+static inline 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);
+}
+
+static inline void *apple_smc_get_cookie(struct apple_smc *smc)
+{
+	return smc->be_cookie;
+}
 
 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
diff --git a/drivers/platform/apple/smc_core.c b/drivers/platform/apple/smc_core.c
index 148a3f8173d3..0902d2f811f4 100644
--- a/drivers/platform/apple/smc_core.c
+++ b/drivers/platform/apple/smc_core.c
@@ -6,25 +6,8 @@
 
 #include <linux/device.h>
 #include <linux/mfd/core.h>
-#include <linux/mutex.h>
-#include <linux/notifier.h>
 #include "smc.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[] = {
 	{
 		.name = "macsmc-gpio",
@@ -44,85 +27,6 @@ static const struct mfd_cell apple_smc_devs[] = {
 	},
 };
 
-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;
@@ -158,37 +62,6 @@ int apple_smc_find_first_key_index(struct apple_smc *smc, smc_key key)
 }
 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;
diff --git a/include/linux/mfd/macsmc.h b/include/linux/mfd/macsmc.h
index 39b4dc4ca881..147d6f957cb1 100644
--- a/include/linux/mfd/macsmc.h
+++ b/include/linux/mfd/macsmc.h
@@ -7,7 +7,10 @@
 #ifndef _LINUX_MFD_MACSMC_H
 #define _LINUX_MFD_MACSMC_H
 
-struct apple_smc;
+#include <linux/errno.h>
+#include <linux/mutex.h>
+#include <linux/notifier.h>
+#include <linux/types.h>
 
 typedef u32 smc_key;
 
@@ -24,16 +27,118 @@ struct apple_smc_key_info {
 	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);
+struct device;
+
+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 {
+	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 inline 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;
+}
+
+static inline 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;
+}
+
+static inline 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 shouldn'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;
+}
+
+static inline 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;
+}
+
+static inline int apple_smc_get_key_count(struct apple_smc *smc)
+{
+	return smc->key_count;
+}
+
+static inline 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;
+}
+
+static inline 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;
+}
 
-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)
 {
@@ -80,7 +185,16 @@ static inline int apple_smc_read_flag(struct apple_smc *smc, smc_key key)
 }
 #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);
+static inline int apple_smc_register_notifier(struct apple_smc *smc,
+					      struct notifier_block *n)
+{
+	return blocking_notifier_chain_register(&smc->event_handlers, n);
+}
+
+static inline int apple_smc_unregister_notifier(struct apple_smc *smc,
+						struct notifier_block *n)
+{
+	return blocking_notifier_chain_unregister(&smc->event_handlers, n);
+}
 
 #endif
-- 
2.30.2



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