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

[Lee Jones]

On Thu, 02 Aug 2018, Srinivas Kandagatla wrote:

> 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.

I haven't look at them in a while, but maybe theirs are optional?

Either way, I don't think you need to do it.

> > >   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 guess now you have seen my comment below, you know that it can
defer, right?

> > > +// SPDX-License-Identifier: GPL-2.0
> > > +// Copyright (c) 2018, Linaro Limited
> > > +//
> > 
> > Blank line?
> > 
> Yep.

Does "yep" mean, "I'll fix it"?

-- 
Lee Jones [æçæ]
Linaro Services Technical Lead
Linaro.org â Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog