[PATCH 1/4] mfd: arizona: Add additional dummy IRQ callbacks

[PATCH 1/4] mfd: arizona: Add additional dummy IRQ callbacks

[Charles Keepax]

We use a dummy IRQ chip to dispatch interrupts to the two seperate IRQ
domains on the Arizona devices. Currently only the enable and disable
callbacks are defined however, there are some situations where additional
callbacks will be used from the IRQ core, which currently results in an
NULL pointer deference. Add handlers for more of the IRQ callbacks and
combine these into a single function since they are all identical.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-irq.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
index d420dbc..71e8f06 100644
--- a/drivers/mfd/arizona-irq.c
+++ b/drivers/mfd/arizona-irq.c
@@ -144,18 +144,17 @@ static irqreturn_t arizona_irq_thread(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void arizona_irq_enable(struct irq_data *data)
-{
-}
-
-static void arizona_irq_disable(struct irq_data *data)
+static void arizona_irq_dummy(struct irq_data *data)
 {
 }
 
 static struct irq_chip arizona_irq_chip = {
 	.name			= "arizona",
-	.irq_disable		= arizona_irq_disable,
-	.irq_enable		= arizona_irq_enable,
+	.irq_disable		= arizona_irq_dummy,
+	.irq_enable		= arizona_irq_dummy,
+	.irq_ack		= arizona_irq_dummy,
+	.irq_mask		= arizona_irq_dummy,
+	.irq_unmask		= arizona_irq_dummy,
 };
 
 static int arizona_irq_map(struct irq_domain *h, unsigned int virq,
-- 
1.7.2.5

[PATCH 2/4] mfd: arizona: Propagate irq_wake through to parent IRQ

[Charles Keepax]

If one of the internal Arizona IRQs is set as a wake source this needs
to be propogated back to the actual IRQ line that the Arizona device is
attached to.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-irq.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
index 71e8f06..fac00b2 100644
--- a/drivers/mfd/arizona-irq.c
+++ b/drivers/mfd/arizona-irq.c
@@ -148,6 +148,13 @@ static void arizona_irq_dummy(struct irq_data *data)
 {
 }
 
+static int arizona_irq_set_wake(struct irq_data *data, unsigned int on)
+{
+	struct arizona *arizona = irq_data_get_irq_chip_data(data);
+
+	return irq_set_irq_wake(arizona->irq, on);
+}
+
 static struct irq_chip arizona_irq_chip = {
 	.name			= "arizona",
 	.irq_disable		= arizona_irq_dummy,
@@ -155,6 +162,7 @@ static struct irq_chip arizona_irq_chip = {
 	.irq_ack		= arizona_irq_dummy,
 	.irq_mask		= arizona_irq_dummy,
 	.irq_unmask		= arizona_irq_dummy,
+	.irq_set_wake		= arizona_irq_set_wake,
 };
 
 static int arizona_irq_map(struct irq_domain *h, unsigned int virq,
-- 
1.7.2.5

Re: [PATCH 2/4] mfd: arizona: Propagate irq_wake through to parent IRQ

[Lee Jones]

On Tue, 12 Aug 2014, Charles Keepax wrote:

> If one of the internal Arizona IRQs is set as a wake source this needs
> to be propogated back to the actual IRQ line that the Arizona device is
> attached to.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-irq.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)

Acked-by: Lee Jones <lee.jones@linaro.org>

... but obviously this won't apply, as it's based on the previous
NACKed patch.

> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index 71e8f06..fac00b2 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -148,6 +148,13 @@ static void arizona_irq_dummy(struct irq_data *data)
>  {
>  }
>  
> +static int arizona_irq_set_wake(struct irq_data *data, unsigned int on)
> +{
> +	struct arizona *arizona = irq_data_get_irq_chip_data(data);
> +
> +	return irq_set_irq_wake(arizona->irq, on);
> +}
> +
>  static struct irq_chip arizona_irq_chip = {
>  	.name			= "arizona",
>  	.irq_disable		= arizona_irq_dummy,
> @@ -155,6 +162,7 @@ static struct irq_chip arizona_irq_chip = {
>  	.irq_ack		= arizona_irq_dummy,
>  	.irq_mask		= arizona_irq_dummy,
>  	.irq_unmask		= arizona_irq_dummy,
> +	.irq_set_wake		= arizona_irq_set_wake,
>  };
>  
>  static int arizona_irq_map(struct irq_domain *h, unsigned int virq,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 3/4] mfd: arizona: Avoid use of legacy IRQ mapping

[Charles Keepax]

regmap_add_irq_chip is called from arizona_irq_init with the irq_base
specified as -1 and regmap_add_irq_chip uses if (irq_base) to check if
it should use legacy IRQ mapping. As such the irq mappings are currently
added with irq_domain_add_legacy, rather than irq_domain_add_linear.
This is clearly a typo as there is no reason why this driver can't use
irq_domain_add_linear.

This patch corrects this by passing the irq_base as zero to
regmap_add_irq_chip.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-irq.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
index fac00b2..b235e59 100644
--- a/drivers/mfd/arizona-irq.c
+++ b/drivers/mfd/arizona-irq.c
@@ -289,7 +289,7 @@ int arizona_irq_init(struct arizona *arizona)
 
 	ret = regmap_add_irq_chip(arizona->regmap,
 				  irq_create_mapping(arizona->virq, 0),
-				  IRQF_ONESHOT, -1, aod,
+				  IRQF_ONESHOT, 0, aod,
 				  &arizona->aod_irq_chip);
 	if (ret != 0) {
 		dev_err(arizona->dev, "Failed to add AOD IRQs: %d\n", ret);
@@ -298,7 +298,7 @@ int arizona_irq_init(struct arizona *arizona)
 
 	ret = regmap_add_irq_chip(arizona->regmap,
 				  irq_create_mapping(arizona->virq, 1),
-				  IRQF_ONESHOT, -1, irq,
+				  IRQF_ONESHOT, 0, irq,
 				  &arizona->irq_chip);
 	if (ret != 0) {
 		dev_err(arizona->dev, "Failed to add main IRQs: %d\n", ret);
-- 
1.7.2.5

Re: [PATCH 3/4] mfd: arizona: Avoid use of legacy IRQ mapping

[Lee Jones]

On Tue, 12 Aug 2014, Charles Keepax wrote:

> regmap_add_irq_chip is called from arizona_irq_init with the irq_base
> specified as -1 and regmap_add_irq_chip uses if (irq_base) to check if
> it should use legacy IRQ mapping. As such the irq mappings are currently
> added with irq_domain_add_legacy, rather than irq_domain_add_linear.
> This is clearly a typo as there is no reason why this driver can't use
> irq_domain_add_linear.
> 
> This patch corrects this by passing the irq_base as zero to
> regmap_add_irq_chip.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-irq.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index fac00b2..b235e59 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -289,7 +289,7 @@ int arizona_irq_init(struct arizona *arizona)
>  
>  	ret = regmap_add_irq_chip(arizona->regmap,
>  				  irq_create_mapping(arizona->virq, 0),
> -				  IRQF_ONESHOT, -1, aod,
> +				  IRQF_ONESHOT, 0, aod,
>  				  &arizona->aod_irq_chip);
>  	if (ret != 0) {
>  		dev_err(arizona->dev, "Failed to add AOD IRQs: %d\n", ret);
> @@ -298,7 +298,7 @@ int arizona_irq_init(struct arizona *arizona)
>  
>  	ret = regmap_add_irq_chip(arizona->regmap,
>  				  irq_create_mapping(arizona->virq, 1),
> -				  IRQF_ONESHOT, -1, irq,
> +				  IRQF_ONESHOT, 0, irq,
>  				  &arizona->irq_chip);
>  	if (ret != 0) {
>  		dev_err(arizona->dev, "Failed to add main IRQs: %d\n", ret);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 4/4] mfd: arizona: Mark additional registers as volatile

[Charles Keepax]

Mark some additional registers as volatile. The write sequencer control
registers should not be cached, as we don't ever want their value
synchronised as this might cause a write sequence to be accidentally
initiated.

Additionally, the DAC_COMP registers require special preconditions to
write so there values wouldn't be updated accurately during a register
sync.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/wm5102-tables.c |   11 ++++++++---
 drivers/mfd/wm5110-tables.c |    6 +++---
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/wm5102-tables.c b/drivers/mfd/wm5102-tables.c
index fb4d4bb..9004488 100644
--- a/drivers/mfd/wm5102-tables.c
+++ b/drivers/mfd/wm5102-tables.c
@@ -245,9 +245,6 @@ const struct regmap_irq_chip wm5102_irq = {
 static const struct reg_default wm5102_reg_default[] = {
 	{ 0x00000008, 0x0019 },   /* R8     - Ctrl IF SPI CFG 1 */ 
 	{ 0x00000009, 0x0001 },   /* R9     - Ctrl IF I2C1 CFG 1 */ 
-	{ 0x00000016, 0x0000 },   /* R22    - Write Sequencer Ctrl 0 */ 
-	{ 0x00000017, 0x0000 },   /* R23    - Write Sequencer Ctrl 1 */ 
-	{ 0x00000018, 0x0000 },   /* R24    - Write Sequencer Ctrl 2 */ 
 	{ 0x00000020, 0x0000 },   /* R32    - Tone Generator 1 */ 
 	{ 0x00000021, 0x1000 },   /* R33    - Tone Generator 2 */ 
 	{ 0x00000022, 0x0000 },   /* R34    - Tone Generator 3 */ 
@@ -1880,6 +1877,10 @@ static bool wm5102_volatile_register(struct device *dev, unsigned int reg)
 	switch (reg) {
 	case ARIZONA_SOFTWARE_RESET:
 	case ARIZONA_DEVICE_REVISION:
+	case ARIZONA_WRITE_SEQUENCER_CTRL_0:
+	case ARIZONA_WRITE_SEQUENCER_CTRL_1:
+	case ARIZONA_WRITE_SEQUENCER_CTRL_2:
+	case ARIZONA_WRITE_SEQUENCER_CTRL_3:
 	case ARIZONA_OUTPUT_STATUS_1:
 	case ARIZONA_RAW_OUTPUT_STATUS_1:
 	case ARIZONA_SLIMBUS_RX_PORT_STATUS:
@@ -1891,6 +1892,10 @@ static bool wm5102_volatile_register(struct device *dev, unsigned int reg)
 	case ARIZONA_ASYNC_SAMPLE_RATE_1_STATUS:
 	case ARIZONA_FLL1_NCO_TEST_0:
 	case ARIZONA_FLL2_NCO_TEST_0:
+	case ARIZONA_DAC_COMP_1:
+	case ARIZONA_DAC_COMP_2:
+	case ARIZONA_DAC_COMP_3:
+	case ARIZONA_DAC_COMP_4:
 	case ARIZONA_FX_CTRL2:
 	case ARIZONA_INTERRUPT_STATUS_1:
 	case ARIZONA_INTERRUPT_STATUS_2:
diff --git a/drivers/mfd/wm5110-tables.c b/drivers/mfd/wm5110-tables.c
index 9b98ee5..beae0a3 100644
--- a/drivers/mfd/wm5110-tables.c
+++ b/drivers/mfd/wm5110-tables.c
@@ -666,9 +666,6 @@ static const struct reg_default wm5110_reg_default[] = {
 	{ 0x0000000A, 0x0001 },    /* R10    - Ctrl IF I2C2 CFG 1 */
 	{ 0x0000000B, 0x0036 },    /* R11    - Ctrl IF I2C1 CFG 2 */
 	{ 0x0000000C, 0x0036 },    /* R12    - Ctrl IF I2C2 CFG 2 */
-	{ 0x00000016, 0x0000 },    /* R22    - Write Sequencer Ctrl 0 */
-	{ 0x00000017, 0x0000 },    /* R23    - Write Sequencer Ctrl 1 */
-	{ 0x00000018, 0x0000 },    /* R24    - Write Sequencer Ctrl 2 */
 	{ 0x00000020, 0x0000 },    /* R32    - Tone Generator 1 */
 	{ 0x00000021, 0x1000 },    /* R33    - Tone Generator 2 */
 	{ 0x00000022, 0x0000 },    /* R34    - Tone Generator 3 */
@@ -2815,6 +2812,9 @@ static bool wm5110_volatile_register(struct device *dev, unsigned int reg)
 	switch (reg) {
 	case ARIZONA_SOFTWARE_RESET:
 	case ARIZONA_DEVICE_REVISION:
+	case ARIZONA_WRITE_SEQUENCER_CTRL_0:
+	case ARIZONA_WRITE_SEQUENCER_CTRL_1:
+	case ARIZONA_WRITE_SEQUENCER_CTRL_2:
 	case ARIZONA_HAPTICS_STATUS:
 	case ARIZONA_SAMPLE_RATE_1_STATUS:
 	case ARIZONA_SAMPLE_RATE_2_STATUS:
-- 
1.7.2.5

Re: [PATCH 4/4] mfd: arizona: Mark additional registers as volatile

[Charles Keepax]

On Tue, Aug 12, 2014 at 02:51:23PM +0100, Charles Keepax wrote:
> Mark some additional registers as volatile. The write sequencer control
> registers should not be cached, as we don't ever want their value
> synchronised as this might cause a write sequence to be accidentally
> initiated.
> 
> Additionally, the DAC_COMP registers require special preconditions to
> write so there values wouldn't be updated accurately during a register
> sync.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---

Apologies please ignore this patch, I forgot I was carrying a
patch from Mark that it depends on that doesn't appear to be in
your MFD tree. I will resend Mark's patch first.

Thanks,
Charles

Re: [PATCH 1/4] mfd: arizona: Add additional dummy IRQ callbacks

[Lee Jones]

On Tue, 12 Aug 2014, Charles Keepax wrote:

> We use a dummy IRQ chip to dispatch interrupts to the two seperate IRQ
> domains on the Arizona devices. Currently only the enable and disable
> callbacks are defined however, there are some situations where additional
> callbacks will be used from the IRQ core, which currently results in an
> NULL pointer deference. Add handlers for more of the IRQ callbacks and
> combine these into a single function since they are all identical.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-irq.c |   13 ++++++-------
>  1 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-irq.c b/drivers/mfd/arizona-irq.c
> index d420dbc..71e8f06 100644
> --- a/drivers/mfd/arizona-irq.c
> +++ b/drivers/mfd/arizona-irq.c
> @@ -144,18 +144,17 @@ static irqreturn_t arizona_irq_thread(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -static void arizona_irq_enable(struct irq_data *data)
> -{
> -}
> -
> -static void arizona_irq_disable(struct irq_data *data)
> +static void arizona_irq_dummy(struct irq_data *data)
>  {
>  }
>  
>  static struct irq_chip arizona_irq_chip = {
>  	.name			= "arizona",
> -	.irq_disable		= arizona_irq_disable,
> -	.irq_enable		= arizona_irq_enable,
> +	.irq_disable		= arizona_irq_dummy,
> +	.irq_enable		= arizona_irq_dummy,
> +	.irq_ack		= arizona_irq_dummy,
> +	.irq_mask		= arizona_irq_dummy,
> +	.irq_unmask		= arizona_irq_dummy,

If you provide .irq_enable(), then .irq_unmask becomes redundant
and/or is checked for before invoking.  There is a chance of
.irq_mask() being called, but if this is a problem, it should be fixed
in the IRQ Chip code.  There is also one unprotected invocation of
.irq_ack(), but I think this should be fixed rather than forcing each
user of IRQ Chip to provide all of these call-backs.

>  };
>  
>  static int arizona_irq_map(struct irq_domain *h, unsigned int virq,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 1/4] mfd: arizona: Add additional dummy IRQ callbacks

[Charles Keepax]

On Thu, Aug 21, 2014 at 12:56:31PM +0100, Lee Jones wrote:
> On Tue, 12 Aug 2014, Charles Keepax wrote:
> >  static struct irq_chip arizona_irq_chip = {
> >  	.name			= "arizona",
> > -	.irq_disable		= arizona_irq_disable,
> > -	.irq_enable		= arizona_irq_enable,
> > +	.irq_disable		= arizona_irq_dummy,
> > +	.irq_enable		= arizona_irq_dummy,
> > +	.irq_ack		= arizona_irq_dummy,
> > +	.irq_mask		= arizona_irq_dummy,
> > +	.irq_unmask		= arizona_irq_dummy,
> 
> If you provide .irq_enable(), then .irq_unmask becomes redundant
> and/or is checked for before invoking.  There is a chance of
> .irq_mask() being called, but if this is a problem, it should be fixed
> in the IRQ Chip code.  There is also one unprotected invocation of
> .irq_ack(), but I think this should be fixed rather than forcing each
> user of IRQ Chip to provide all of these call-backs.

Cool I will look at doing some fixups in the IRQ code and see
where that gets me to.

Thanks,
Charles

Re: [PATCH 1/4] mfd: arizona: Add additional dummy IRQ callbacks

[Charles Keepax]

On Thu, Aug 21, 2014 at 12:56:31PM +0100, Lee Jones wrote:
> On Tue, 12 Aug 2014, Charles Keepax wrote:
> 
> > We use a dummy IRQ chip to dispatch interrupts to the two seperate IRQ
> > domains on the Arizona devices. Currently only the enable and disable
> > callbacks are defined however, there are some situations where additional
> > callbacks will be used from the IRQ core, which currently results in an
> > NULL pointer deference. Add handlers for more of the IRQ callbacks and
> > combine these into a single function since they are all identical.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> 
> If you provide .irq_enable(), then .irq_unmask becomes redundant
> and/or is checked for before invoking.  There is a chance of
> .irq_mask() being called, but if this is a problem, it should be fixed
> in the IRQ Chip code.  There is also one unprotected invocation of
> .irq_ack(), but I think this should be fixed rather than forcing each
> user of IRQ Chip to provide all of these call-backs.

So I have been looking at this further and going back over things
from when the issue was discovered and it looks like it was
probably the unprotected irq_ack call (in handle_edge_irq) that
was the problem. But thinking about this more I am fairly
convinced that the call is unprotected because it is expected that
an edge IRQ will always have an ack, as it doesn't really make
sense for an edge IRQ to not have an ack.

The IRQ chip here is just a software device to distribute the IRQ
to the two sub-domains. As such I think the problem lies here:

	irq_set_chip_and_handler(virq, &arizona_irq_chip, handle_edge_irq);

I am pretty sure the correct fix is to change this to a
handle_simple_irq as it is just a software dummy and there is
nothing really edge triggered about it. Do you see any problems
with that as a solution?

Thanks,
Charles
\x16

Re: [PATCH 1/4] mfd: arizona: Add additional dummy IRQ callbacks

[Lee Jones]

On Tue, 02 Sep 2014, Charles Keepax wrote:

> On Thu, Aug 21, 2014 at 12:56:31PM +0100, Lee Jones wrote:
> > On Tue, 12 Aug 2014, Charles Keepax wrote:
> > 
> > > We use a dummy IRQ chip to dispatch interrupts to the two seperate IRQ
> > > domains on the Arizona devices. Currently only the enable and disable
> > > callbacks are defined however, there are some situations where additional
> > > callbacks will be used from the IRQ core, which currently results in an
> > > NULL pointer deference. Add handlers for more of the IRQ callbacks and
> > > combine these into a single function since they are all identical.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > ---
> > 
> > If you provide .irq_enable(), then .irq_unmask becomes redundant
> > and/or is checked for before invoking.  There is a chance of
> > .irq_mask() being called, but if this is a problem, it should be fixed
> > in the IRQ Chip code.  There is also one unprotected invocation of
> > .irq_ack(), but I think this should be fixed rather than forcing each
> > user of IRQ Chip to provide all of these call-backs.
> 
> So I have been looking at this further and going back over things
> from when the issue was discovered and it looks like it was
> probably the unprotected irq_ack call (in handle_edge_irq) that
> was the problem. But thinking about this more I am fairly
> convinced that the call is unprotected because it is expected that
> an edge IRQ will always have an ack, as it doesn't really make
> sense for an edge IRQ to not have an ack.
> 
> The IRQ chip here is just a software device to distribute the IRQ
> to the two sub-domains. As such I think the problem lies here:
> 
> 	irq_set_chip_and_handler(virq, &arizona_irq_chip, handle_edge_irq);
> 
> I am pretty sure the correct fix is to change this to a
> handle_simple_irq as it is just a software dummy and there is
> nothing really edge triggered about it. Do you see any problems
> with that as a solution?

I don't pretend to be an expert on the IRQ framework, but this
certainly looks a lot less hacky than your previous submission.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog