On 08/11/2016 16:49, Will Deacon wrote:
On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
On 08/11/2016 16:12, Will Deacon wrote:
On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
+static inline void arm64_set_extops(struct extio_ops *ops)
+{
+ if (ops)
+ WRITE_ONCE(arm64_extio_ops, ops);
Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
side. Also, what if multiple drivers want to set different ops for distinct
address ranges?
I think that the idea here is that we only have possibly one master in the
system which offers indirectIO backend, so another one could not possibly
re-set this value.
Why is that assumption valid, and why does WRITE_ONCE help there? It's not
ONCE as in WARN_ONCE, more ONCE as in exactly-once-per-invocation.
It's only valid based on the inherent assumption that all indirectIO is
redirected to one backend master, i.e. LPC driver.
Anyway, right, I don't think that WRITE_ONCE is correct. Zhichang was
looking for something which would only allow the pointer to be written
once ever.
diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
new file mode 100644
index 0000000..647b3fa
--- /dev/null
+++ b/arch/arm64/kernel/extio.c
@@ -0,0 +1,27 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang@xxxxxxxxxxxxx>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/io.h>
+
+struct extio_ops *arm64_extio_ops;
+
+
+BUILD_EXTIO(b, u8)
+
+BUILD_EXTIO(w, u16)
+
+BUILD_EXTIO(l, u32)
Is there no way to make this slightly more generic, so that it can be
re-used elsewhere? For example, if struct extio_ops was common, then
you could have the singleton (which maybe should be an interval tree?),
type definition, setter function and the BUILD_EXTIO invocations
somewhere generic, rather than squirelled away in the arch backend.
The concern would be that some architecture which uses generic higher-level
ISA accessor ops, but have IO space, could be affected.
You're already adding a Kconfig symbol for this stuff, so you can keep
that if you don't want it on other architectures. I'm just arguing that
plumbing drivers directly into arch code via arm64_set_extops is not
something I'm particularly fond of, especially when it looks like it
could be avoided with a small amount of effort.
We'll check this.
Cheers,
John
Will
.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
