login
Search
Hi,
This is a patch for the Compaq ASIC3 multi function chip, found in many PDAs
(iPAQs, HTCs...).
It is a simplified version of Paul Sokolovsky's first proposal [1]. With this
code, it is basically a GPIO and IRQ expander. My plan is to add more
features once this patch gets reviewed and accepted.[1] http://lkml.org/lkml/2007/5/1/46
Signed-off-by: Samuel Ortiz <sameo@openedhand.com>
---
drivers/mfd/Kconfig | 6
drivers/mfd/Makefile | 1
drivers/mfd/asic3.c | 572 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/asic3.h | 463 +++++++++++++++++++++++++++++++++++++
4 files changed, 1042 insertions(+)Index: linux-2.6-htc/drivers/mfd/asic3.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6-htc/drivers/mfd/asic3.c 2007-10-18 11:07:09.000000000 +0200
@@ -0,0 +1,572 @@
+/*
+ * driver/mfd/asic3.c
+ *
+ * Compaq ASIC3 support.
+ *
+ * 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.
+ *
+ * Copyright 2001 Compaq Computer Corporation.
+ * Copyright 2004-2005 Phil Blundell
+ * Copyright 2007 OpenedHand Ltd.
+ */
+
+#include <linux/module.h>
+#include <linux/version.h>
+#include <linux/irq.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+#include <linux/platform_device.h>
+
+#include <linux/mfd/asic3.h>
+
+static inline void asic3_write_register(struct asic3 *asic,
+ unsigned int reg, u32 value)
+{
+ iowrite16(value, (unsigned long)asic->mapping +
+ (reg >> (2 - asic->bus_shift)));
+}
+
+static inline u32 asic3_read_register(struct asic3 *asic,
+ unsigned int reg)
+{
+ return ioread16((unsigned long)asic->mapping +
+ (reg >> (2 - asic->bus_shift)));
+}
+
+/* IRQs */
+#define MAX_ASIC_ISR_LOOPS 20
+...
On Thu, 18 Oct 2007 11:12:41 +0200
You're not a big fan of checkpatch, I see.
total: 76 errors, 69 warnings, 1054 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, seePlease see the large comment at the top of linux/irq.h. I believe this
driver will fial to compile on at least arm.You'd get faster code if that "2 - asic->bus_shift" was cached in struct
Too large to be inlined. If it has only one callsite (it does) then the
compiler will inline it. If it has more than one callsite then we didn'thm, so this delves into the innards of the IRQ management. Does it work OK
with and without CONFIG_GENERIC_HARDIRQS? If not, some Kconfig work willAm wondering about the handling of asic->lock. As it is taken from hard
interrupts, it is a bug to take it with local interrupts enabled. afacit
that is what this code is doing and is hence deadlockable. But I didn't
look very hard.OK, there you go. Maybe I was wrong about that lock.
To what are these exported?
-
Well, now I am :-)
I fixed all the errors, there are only a couple lines being more than 80
It doesn't build as a module, since we need the irq.h symbols.
I changed MFD_ASIC3 to bool. I somehow feel that this is not the cleanest
solution, but OTOH I think that dynamically adding IRQs and GPIOs to an
As I explained to Thomas, asic3 defines an additional range of IRQs for the
board, so we really need to access the irq API.
No, I think you're right, the lock should be taken with local interrupts
Currently nothing, but the plan is to push several drivers (leds, MMC,
buttons...) based on the ASIC3, and they need to access the ASIC3 GPIOs.
Do you want me to remove the EXPORT_SYMBOL until we actually add those
drivers ?Thanks a lot for the review, here goes a new version of this patch:
Signed-off-by: Samuel Ortiz <sameo@openedhand.com>
---
drivers/mfd/Kconfig | 7
drivers/mfd/Makefile | 1
drivers/mfd/asic3.c | 588 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/asic3.h | 497 ++++++++++++++++++++++++++++++++++++++
4 files changed, 1093 insertions(+)Index: linux-2.6-htc/drivers/mfd/asic3.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6-htc/drivers/mfd/asic3.c 2007-10-19 12:45:56.000000000 +0200
@@ -0,0 +1,588 @@
+/*
+ * driver/mfd/asic3.c
+ *
+ * Compaq ASIC3 support.
+ *
+ * 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.
+ *
+ * Copyright 2001 Compaq Computer Corporation.
+ * Copyright 2004-2005 Phil Blundell
+ * Copyright 2007 OpenedHand Ltd.
+ *
+ * Authors: Phil Blundell <pb@handhelds.org>,
+ * Samuel Ortiz <sameo@openedhand.com>
+ *
+ */
+
+#include <linux/version.h>
+#include <linux/kernel.h>
+#include <linux/irq.h>
+#include <linux/io.h>
+#include &...
We seem to have miscommunicated here. <linux/irq.h> contains references to
things which only some architectures actually implement. I don't know
which architectures those are, but it includes common ones like x86, so
it's a real trap. I recall it does not include arm, so your code might
break on arm.At least, that's what's _supposed_ to happen: I just compiled and linked
this driver into an ARM kernel with no problems so now I'm all confused as
to what the problem was.Oh well, we'll see...
-
Yes, ARM implements the irq.h API. So, it will definitely builds on an ARM
kernel.
However, since there is no requirement for other architectures to implement
this API and since the asic3 is currently only found and tested on ARM
machines, I think it would be safer to make it depends on ARM.
Would that make sense to you ? Here goes a new revision of the patch in case
it does. That would be version 3.--
ASIC3 driver - v3.This is a patch for the ASIC3 silicon originally designed by Compaq.
This is a multifunction device, found in many PDAs (iPAQs, HTCs...).Changes from v3 to v2:
---------------------
- Depends on ARMChanges from v2 to v1:
---------------------
- Fixes checkpatch errors and warnings.
- Depends on GENERIC_HARDIRQS.
- MFD_ASIC3 is bool, as it needs non-exported irq.h symbols.
- Disable local interrupts when taking the lock from the irq_chip ops.
- Optimize bus shift handling.
- Coding style fixes.Signed-off-by: Samuel Ortiz <sameo@openedhand.com>
---
drivers/mfd/Kconfig | 7
drivers/mfd/Makefile | 1
drivers/mfd/asic3.c | 588 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/mfd/asic3.h | 497 ++++++++++++++++++++++++++++++++++++++
4 files changed, 1093 insertions(+)Index: linux-2.6-htc/drivers/mfd/asic3.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6-htc/drivers/mfd/asic3.c 2007-10-19 12:45:56.000000000 +0200
@@ -0,0 +1,588 @@
+/*
+ * driver/mfd/asic3.c
+ *
+ * Compaq ASIC3 support.
+ *
+ * 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.
+ *
+ * Copyright 2001 Compaq Computer Corporation.
+ * Copyright 2004-2005 Phil Blundell
+ * Copyright 2007 OpenedHand Ltd.
+ *
+ * Authors: Phil Blundell <pb@handhelds.org>,
+ * Samuel Ortiz <sameo@openedhand.com>
+ *...
We obviously never removed the big fat warning, which was valid before
the ARM to generic irq conversion.tglx
-
I'd love to see this converted to use the GPIO API before all the
drivers are going in.
Any chance we can use this as an opportunity to look at David
Brownell's gpiolib again? (http://lkml.org/lkml/2007/4/15/127).
I want to do the same for a similar chip (a GPIO/IRQ extender
implemented in a Xilinx CPLD used on several HTC phones), but I
couldn't figure out how to do GPIO<->IRQ conversion properly with
gpiolib and how to handle the CPU's internal GPIOs, which are more
than ARCH_GPIOS_PER_CHIP in gpiolib.regards
Philipp
-
drivers are fine with: #include <linux/interrupt.h>
If they need linux/irq.h, then they do probably something really
nasty.tglx
-
The asic3 adds irqs to a board, through GPIOs. Is that a valid case ?
Cheers,
-
