Re: [PATCH v2 03/10] mfd: wcd9335: add wcd irq support
From: Srinivas Kandagatla
Date: Thu Aug 02 2018 - 12:03:05 EST
Thanks for the review,
On 02/08/18 09:05, Lee Jones wrote:
On Fri, 27 Jul 2018, Srinivas Kandagatla wrote:
WCD9335 supports two lines of irqs INTR1 and INTR2.
Multiple interrupts are muxed via these lines.
INTR1 consists of all possible interrupt sources like:
Ear OCP, HPH OCP, MBHC, MAD, VBAT, and SVA
INTR2 is a subset of first interrupt sources like MAD, VBAT, and SVA
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>
---
drivers/mfd/Makefile | 2 +-
drivers/mfd/wcd9335-core.c | 9 ++
drivers/mfd/wcd9335-irq.c | 172 ++++++++++++++++++++++++++++++++++++
Any particular reason for separating out IRQ handling?
No particular reason, I tried to follow what other codecs like wm8994,
wm8350 and 14 others do.
include/dt-bindings/mfd/wcd9335.h | 43 +++++++++
include/linux/mfd/wcd9335/wcd9335.h | 3 +
5 files changed, 228 insertions(+), 1 deletion(-)
create mode 100644 drivers/mfd/wcd9335-irq.c
create mode 100644 include/dt-bindings/mfd/wcd9335.h
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index a4697370640b..210875afe78a 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -58,7 +58,7 @@ obj-$(CONFIG_MFD_ARIZONA) += cs47l24-tables.o
endif
obj-$(CONFIG_MFD_WCD9335) += wcd9335.o
-wcd9335-objs := wcd9335-core.o
+wcd9335-objs := wcd9335-core.o wcd9335-irq.o
obj-$(CONFIG_MFD_WM8400) += wm8400-core.o
wm831x-objs := wm831x-core.o wm831x-irq.o wm831x-otp.o
diff --git a/drivers/mfd/wcd9335-core.c b/drivers/mfd/wcd9335-core.c
index 8f746901f4e9..6299dfb63aca 100644
--- a/drivers/mfd/wcd9335-core.c
+++ b/drivers/mfd/wcd9335-core.c
@@ -243,12 +243,20 @@ static int wcd9335_slim_status(struct slim_device *sdev,
return ret;
}
+ wcd9335_irq_init(wcd);
Why don't you check the return value?
What happens if it defers?
It would not defer here as the regmaps are already setup in probe and
the status callback is only invoked when the SLIMbus device is up.
I will add the missing checks here.
wcd->slim_ifd = wcd->slim_ifd;
return mfd_add_devices(wcd->dev, 0, wcd9335_devices,
ARRAY_SIZE(wcd9335_devices), NULL, 0, NULL);
}
+static void wcd9335_slim_remove(struct slim_device *sdev)
+{
+ struct wcd9335 *wcd = dev_get_drvdata(&sdev->dev);
+
+ wcd9335_irq_exit(wcd);
+}
+
static const struct slim_device_id wcd9335_slim_id[] = {
{0x217, 0x1a0, 0x1, 0x0},
{}
@@ -259,6 +267,7 @@ static struct slim_driver wcd9335_slim_driver = {
.name = "wcd9335-slim",
},
.probe = wcd9335_slim_probe,
+ .remove = wcd9335_slim_remove,
.device_status = wcd9335_slim_status,
.id_table = wcd9335_slim_id,
};
diff --git a/drivers/mfd/wcd9335-irq.c b/drivers/mfd/wcd9335-irq.c
new file mode 100644
index 000000000000..84098c89419b
--- /dev/null
+++ b/drivers/mfd/wcd9335-irq.c
@@ -0,0 +1,172 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (c) 2018, Linaro Limited
+//
Blank line?
Yep.
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/regmap.h>
+#include <linux/of_irq.h>
+#include <dt-bindings/mfd/wcd9335.h>
+#include <linux/mfd/wcd9335/wcd9335.h>
+#include <linux/mfd/wcd9335/registers.h>
Please place in alphabetical order.
sure!
+static const struct regmap_irq wcd9335_irqs[] = {
+ /* INTR_REG 0 */
+ [WCD9335_IRQ_SLIMBUS] = {
+ .reg_offset = 0,
+ .mask = BIT(0),
+ },
[snipped approx 1,000,000 entries]
+ [WCD9335_IRQ_SVA_OUTBOX2] = {
+ .reg_offset = 3,
+ .mask = BIT(6),
+ },
+};
Please use REGMAP_IRQ_REG() to significantly reduce the diff.
Nice, that looks good, I will use that in next version.
+static const struct regmap_irq_chip wcd9335_regmap_irq1_chip = {
+ .name = "wcd9335_pin1_irq",
+ .status_base = WCD9335_INTR_PIN1_STATUS0,
+ .mask_base = WCD9335_INTR_PIN1_MASK0,
+ .ack_base = WCD9335_INTR_PIN1_CLEAR0,
+ .type_base = WCD9335_INTR_LEVEL0,
+ .num_regs = 4,
+ .irqs = wcd9335_irqs,
+ .num_irqs = ARRAY_SIZE(wcd9335_irqs),
+};
+
+int wcd9335_irq_init(struct wcd9335 *wcd)
+{
+ int ret;
'\n' here.
yep!
+ /*
+ * INTR1 consists of all possible interrupt sources Ear OCP,
+ * HPH OCP, MBHC, MAD, VBAT, and SVA
+ * INTR2 is a subset of first interrupt sources MAD, VBAT, and SVA
+ */
+ wcd->intr1 = of_irq_get_byname(wcd->dev->of_node, "intr1");
+ if (wcd->intr1 < 0 || wcd->intr1 == -EPROBE_DEFER) {
+ dev_err(wcd->dev, "Unable to configure irq\n");
Nit: s/irq/IRQ/
Do you really want to issue an error message for -EPROBE_DEFER?
This maybe confusing if it is initiated later.
Thanks for spotting this, I will take a closer look at this error path
and handle the DEFER case correctly in next version!
+ return wcd->intr1;
+ }
+
+ ret = regmap_add_irq_chip(wcd->regmap, wcd->intr1,
+ IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+ 0, &wcd9335_regmap_irq1_chip,
+ &wcd->irq_data);
+ if (ret != 0) {
"if (ret)"
Yep!
+ dev_err(wcd->dev, "Failed to register IRQ chip: %d\n", ret);
+ return ret;
+ }
+
+ return 0;
+}
+
+int wcd9335_irq_exit(struct wcd9335 *wcd)
+{
+ regmap_del_irq_chip(wcd->intr1, wcd->irq_data);
'\n' here.
yep!
+ return 0;
+}
Or better still, make this a void and remove the superfluous return.
Makes sense, I will give that a try in next version.