| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20180802080525.GM4662@dell>
Date: Thu, 2 Aug 2018 09:05:25 +0100
From: Lee Jones <lee.jones@...aro.org>
To: Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc: robh+dt@...nel.org, broonie@...nel.org, mark.rutland@....com,
lgirdwood@...il.com, tiwai@...e.com, bgoswami@...eaurora.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
vkoul@...nel.org, alsa-devel@...a-project.org
Subject: Re: [PATCH v2 03/10] mfd: wcd9335: add wcd irq support
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@...aro.org>
> ---
> drivers/mfd/Makefile | 2 +-
> drivers/mfd/wcd9335-core.c | 9 ++
> drivers/mfd/wcd9335-irq.c | 172 ++++++++++++++++++++++++++++++++++++
Any particular reason for separating out IRQ handling?
> 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?
> 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?
> +#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.
> +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.
> +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.
> + /*
> + * 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.
> + 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)"
> + 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.
> + return 0;
> +}
Or better still, make this a void and remove the superfluous return.
> diff --git a/include/dt-bindings/mfd/wcd9335.h b/include/dt-bindings/mfd/wcd9335.h
> new file mode 100644
> index 000000000000..61b6a11da00d
> --- /dev/null
> +++ b/include/dt-bindings/mfd/wcd9335.h
> @@ -0,0 +1,43 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * This header provides macros for WCD9335 device bindings.
> + *
> + * Copyright (c) 2018, Linaro Limited
> + */
> +
> +#ifndef _DT_BINDINGS_MFD_WCD9335_H
> +#define _DT_BINDINGS_MFD_WCD9335_H
> +
> +#define WCD9335_IRQ_SLIMBUS 0
> +#define WCD9335_IRQ_FLL_LOCK_LOSS 1
> +#define WCD9335_IRQ_HPH_PA_OCPL_FAULT 2
> +#define WCD9335_IRQ_HPH_PA_OCPR_FAULT 3
> +#define WCD9335_IRQ_EAR_PA_OCP_FAULT 4
> +#define WCD9335_IRQ_HPH_PA_CNPL_COMPLETE 5
> +#define WCD9335_IRQ_HPH_PA_CNPR_COMPLETE 6
> +#define WCD9335_IRQ_EAR_PA_CNP_COMPLETE 7
> +#define WCD9335_IRQ_MBHC_SW_DET 8
> +#define WCD9335_IRQ_MBHC_ELECT_INS_REM_DET 9
> +#define WCD9335_IRQ_MBHC_BUTTON_PRESS_DET 10
> +#define WCD9335_IRQ_MBHC_BUTTON_RELEASE_DET 11
> +#define WCD9335_IRQ_MBHC_ELECT_INS_REM_LEG_DET 12
> +#define WCD9335_IRQ_RESERVED_0 13
> +#define WCD9335_IRQ_RESERVED_1 14
> +#define WCD9335_IRQ_RESERVED_2 15
> +#define WCD9335_IRQ_LINE_PA1_CNP_COMPLETE 16
> +#define WCD9335_IRQ_LINE_PA2_CNP_COMPLETE 17
> +#define WCD9335_IRQ_LINE_PA3_CNP_COMPLETE 18
> +#define WCD9335_IRQ_LINE_PA4_CNP_COMPLETE 19
> +#define WCD9335_IRQ_SOUNDWIRE 20
> +#define WCD9335_IRQ_VDD_DIG_RAMP_COMPLETE 21
> +#define WCD9335_IRQ_RCO_ERROR 22
> +#define WCD9335_IRQ_SVA_ERROR 23
> +#define WCD9335_IRQ_MAD_AUDIO 24
> +#define WCD9335_IRQ_MAD_BEACON 25
> +#define WCD9335_IRQ_MAD_ULTRASOUND 26
> +#define WCD9335_IRQ_VBAT_ATTACK 27
> +#define WCD9335_IRQ_VBAT_RESTORE 28
> +#define WCD9335_IRQ_SVA_OUTBOX1 29
> +#define WCD9335_IRQ_SVA_OUTBOX2 30
> +
> +#endif /* _DT_BINDINGS_MFD_WCD9335_H */
> diff --git a/include/linux/mfd/wcd9335/wcd9335.h b/include/linux/mfd/wcd9335/wcd9335.h
> index b9d2f7af243a..1479bfe75f23 100644
> --- a/include/linux/mfd/wcd9335/wcd9335.h
> +++ b/include/linux/mfd/wcd9335/wcd9335.h
> @@ -39,4 +39,7 @@ struct wcd9335 {
> struct regulator_bulk_data supplies[WCD9335_MAX_SUPPLY];
> };
>
> +extern int wcd9335_irq_init(struct wcd9335 *wcd);
> +extern int wcd9335_irq_exit(struct wcd9335 *wcd);
> +
> #endif /* __WCD9335_H__ */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Powered by blists - more mailing lists