Re: [PATCH v2 03/10] mfd: wcd9335: add wcd irq support

Srinivas Kandagatla
2018-08-02T08:54:08Z

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@linaro.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?
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.