[PATCH fixes 1/2] mfd: max77693: Use proper regmap for handling MUIC interrupts

[PATCH fixes 1/2] mfd: max77693: Use proper regmap for handling MUIC interrupts

[Krzysztof Kozlowski]

Interrupts coming from Maxim77693 MUIC block (MicroUSB Interface
Controller) were not handled at all because wrong regmap was used for
MUIC's regmap_irq_chip.

The MUIC component of Maxim 77693 uses different I2C address thus second
regmap is created and used by max77693 extcon driver. The registers for
MUIC interrupts are also in that block and should be handled by that
second regmap.

However the regmap irq chip for MUIC was configured with default regmap
which could not read MUIC registers.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: <stable@vger.kernel.org>
Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
---
 drivers/mfd/max77693.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
index 2277a11b6629..4b54da1ff7ab 100644
--- a/drivers/mfd/max77693.c
+++ b/drivers/mfd/max77693.c
@@ -243,7 +243,7 @@ static int max77693_i2c_probe(struct i2c_client *i2c,
 		goto err_irq_charger;
 	}
 
-	ret = regmap_add_irq_chip(max77693->regmap, max77693->irq,
+	ret = regmap_add_irq_chip(max77693->regmap_muic, max77693->irq,
 				IRQF_ONESHOT | IRQF_SHARED |
 				IRQF_TRIGGER_FALLING, 0,
 				&max77693_muic_irq_chip,
-- 
1.9.1

[PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Krzysztof Kozlowski]

All interrupts coming from MUIC were ignored because interrupt source
register was masked.

The Maxim 77693 has a "interrupt source" - a separate register and interrupts
which give information about PMIC block triggering the individual
interrupt (charger, topsys, MUIC, flash LED).

By default bootloader could initialize this register to "mask all"
value. In such case (observed on Trats2 board) MUIC interrupts won't be
generated regardless of their mask status. Regmap irq chip was unmasking
individual MUIC interrupts but the source was masked

Before introducing regmap irq chip this interrupt source was unmasked,
read and acked. Reading and acking is not necessary but unmasking is. Do
it in extcon driver.

Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: <stable@vger.kernel.org>
Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
---
 drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
 include/linux/mfd/max77693-private.h |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
index 490e27475bac..d3adacf3187f 100644
--- a/drivers/extcon/extcon-max77693.c
+++ b/drivers/extcon/extcon-max77693.c
@@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
 		}
 	}
 
+	if (i) {
+		/*
+		 * i>0, at least one MUIC interrupt requested so need to unmask
+		 * MUIC interrupt in INTSRC register. Without that MUIC
+		 * interrupts will not be reported.
+		 */
+		ret = regmap_update_bits(max77693->regmap,
+				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
+				~SRC_IRQ_MUIC);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
+					ret);
+			return ret;
+		}
+	}
+
 	/* Initialize extcon device */
 	info->edev = devm_extcon_dev_allocate(&pdev->dev,
 					      max77693_extcon_cable);
diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
index e1b2b61285b9..4f1a80b66ecf 100644
--- a/include/linux/mfd/max77693-private.h
+++ b/include/linux/mfd/max77693-private.h
@@ -438,6 +438,11 @@ enum max77693_irq_source {
 	MAX77693_IRQ_GROUP_NR,
 };
 
+#define SRC_IRQ_CHARGER			BIT(0)
+#define SRC_IRQ_TOP			BIT(1)
+#define SRC_IRQ_FLASH			BIT(2)
+#define SRC_IRQ_MUIC			BIT(3)
+
 #define LED_IRQ_FLED2_OPEN		BIT(0)
 #define LED_IRQ_FLED2_SHORT		BIT(1)
 #define LED_IRQ_FLED1_OPEN		BIT(2)
-- 
1.9.1

Re: [PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Chanwoo Choi]

On 10/10/2014 05:22 PM, Krzysztof Kozlowski wrote:
> All interrupts coming from MUIC were ignored because interrupt source
> register was masked.
> 
> The Maxim 77693 has a "interrupt source" - a separate register and interrupts
> which give information about PMIC block triggering the individual
> interrupt (charger, topsys, MUIC, flash LED).
> 
> By default bootloader could initialize this register to "mask all"
> value. In such case (observed on Trats2 board) MUIC interrupts won't be
> generated regardless of their mask status. Regmap irq chip was unmasking
> individual MUIC interrupts but the source was masked
> 
> Before introducing regmap irq chip this interrupt source was unmasked,
> read and acked. Reading and acking is not necessary but unmasking is. Do
> it in extcon driver.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: <stable@vger.kernel.org>
> Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> ---
>  drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
>  include/linux/mfd/max77693-private.h |  5 +++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
> index 490e27475bac..d3adacf3187f 100644
> --- a/drivers/extcon/extcon-max77693.c
> +++ b/drivers/extcon/extcon-max77693.c
> @@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (i) {
> +		/*
> +		 * i>0, at least one MUIC interrupt requested so need to unmask
> +		 * MUIC interrupt in INTSRC register. Without that MUIC
> +		 * interrupts will not be reported.
> +		 */
> +		ret = regmap_update_bits(max77693->regmap,
> +				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
> +				~SRC_IRQ_MUIC);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
> +					ret);
> +			return ret;
> +		}
> +	}
> +

No,
The extcon-max77693 driver cannot enable IRQ_CHARGER/TOP/FLASH
because MAX77693_PMIC_REG_INTSRC_MASK register is common register of MAX77693.

The IRQ_CHARGER/TOP/FLASH of MAX77693_PMIC_REG_INTSRC_MASK register should be enabled
in drivers/mfd/max77693.c.

>  	/* Initialize extcon device */
>  	info->edev = devm_extcon_dev_allocate(&pdev->dev,
>  					      max77693_extcon_cable);
> diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
> index e1b2b61285b9..4f1a80b66ecf 100644
> --- a/include/linux/mfd/max77693-private.h
> +++ b/include/linux/mfd/max77693-private.h
> @@ -438,6 +438,11 @@ enum max77693_irq_source {
>  	MAX77693_IRQ_GROUP_NR,
>  };
>  
> +#define SRC_IRQ_CHARGER			BIT(0)
> +#define SRC_IRQ_TOP			BIT(1)
> +#define SRC_IRQ_FLASH			BIT(2)
> +#define SRC_IRQ_MUIC			BIT(3)
> +
>  #define LED_IRQ_FLED2_OPEN		BIT(0)
>  #define LED_IRQ_FLED2_SHORT		BIT(1)
>  #define LED_IRQ_FLED1_OPEN		BIT(2)
> 

Re: [PATCH fixes 1/2] mfd: max77693: Use proper regmap for handling MUIC interrupts

[Chanwoo Choi]

On 10/10/2014 05:22 PM, Krzysztof Kozlowski wrote:
> Interrupts coming from Maxim77693 MUIC block (MicroUSB Interface
> Controller) were not handled at all because wrong regmap was used for
> MUIC's regmap_irq_chip.
> 
> The MUIC component of Maxim 77693 uses different I2C address thus second
> regmap is created and used by max77693 extcon driver. The registers for
> MUIC interrupts are also in that block and should be handled by that
> second regmap.
> 
> However the regmap irq chip for MUIC was configured with default regmap
> which could not read MUIC registers.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: <stable@vger.kernel.org>
> Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> ---
>  drivers/mfd/max77693.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
> index 2277a11b6629..4b54da1ff7ab 100644
> --- a/drivers/mfd/max77693.c
> +++ b/drivers/mfd/max77693.c
> @@ -243,7 +243,7 @@ static int max77693_i2c_probe(struct i2c_client *i2c,
>  		goto err_irq_charger;
>  	}
>  
> -	ret = regmap_add_irq_chip(max77693->regmap, max77693->irq,
> +	ret = regmap_add_irq_chip(max77693->regmap_muic, max77693->irq,
>  				IRQF_ONESHOT | IRQF_SHARED |
>  				IRQF_TRIGGER_FALLING, 0,
>  				&max77693_muic_irq_chip,
> 

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thanks,
Chanwoo Choi

Re: [PATCH fixes 1/2] mfd: max77693: Use proper regmap for handling MUIC interrupts

[Lee Jones]

On Fri, 10 Oct 2014, Krzysztof Kozlowski wrote:

> Interrupts coming from Maxim77693 MUIC block (MicroUSB Interface
> Controller) were not handled at all because wrong regmap was used for
> MUIC's regmap_irq_chip.
> 
> The MUIC component of Maxim 77693 uses different I2C address thus second
> regmap is created and used by max77693 extcon driver. The registers for
> MUIC interrupts are also in that block and should be handled by that
> second regmap.
> 
> However the regmap irq chip for MUIC was configured with default regmap
> which could not read MUIC registers.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: <stable@vger.kernel.org>
> Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> ---
>  drivers/mfd/max77693.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to -fixes.

> diff --git a/drivers/mfd/max77693.c b/drivers/mfd/max77693.c
> index 2277a11b6629..4b54da1ff7ab 100644
> --- a/drivers/mfd/max77693.c
> +++ b/drivers/mfd/max77693.c
> @@ -243,7 +243,7 @@ static int max77693_i2c_probe(struct i2c_client *i2c,
>  		goto err_irq_charger;
>  	}
>  
> -	ret = regmap_add_irq_chip(max77693->regmap, max77693->irq,
> +	ret = regmap_add_irq_chip(max77693->regmap_muic, max77693->irq,
>  				IRQF_ONESHOT | IRQF_SHARED |
>  				IRQF_TRIGGER_FALLING, 0,
>  				&max77693_muic_irq_chip,

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

Re: [PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Lee Jones]

On Fri, 10 Oct 2014, Krzysztof Kozlowski wrote:

> All interrupts coming from MUIC were ignored because interrupt source
> register was masked.
> 
> The Maxim 77693 has a "interrupt source" - a separate register and interrupts
> which give information about PMIC block triggering the individual
> interrupt (charger, topsys, MUIC, flash LED).
> 
> By default bootloader could initialize this register to "mask all"
> value. In such case (observed on Trats2 board) MUIC interrupts won't be
> generated regardless of their mask status. Regmap irq chip was unmasking
> individual MUIC interrupts but the source was masked
> 
> Before introducing regmap irq chip this interrupt source was unmasked,
> read and acked. Reading and acking is not necessary but unmasking is. Do
> it in extcon driver.
> 
> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: <stable@vger.kernel.org>
> Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> ---
>  drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
>  include/linux/mfd/max77693-private.h |  5 +++++
>  2 files changed, 21 insertions(+)

Did you run this through checkpatch?

> diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
> index 490e27475bac..d3adacf3187f 100644
> --- a/drivers/extcon/extcon-max77693.c
> +++ b/drivers/extcon/extcon-max77693.c
> @@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	if (i) {
> +		/*
> +		 * i>0, at least one MUIC interrupt requested so need to unmask
> +		 * MUIC interrupt in INTSRC register. Without that MUIC
> +		 * interrupts will not be reported.
> +		 */
> +		ret = regmap_update_bits(max77693->regmap,
> +				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
> +				~SRC_IRQ_MUIC);
> +		if (ret < 0) {
> +			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
> +					ret);
> +			return ret;
> +		}
> +	}
> +
>  	/* Initialize extcon device */
>  	info->edev = devm_extcon_dev_allocate(&pdev->dev,
>  					      max77693_extcon_cable);
> diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
> index e1b2b61285b9..4f1a80b66ecf 100644
> --- a/include/linux/mfd/max77693-private.h
> +++ b/include/linux/mfd/max77693-private.h
> @@ -438,6 +438,11 @@ enum max77693_irq_source {
>  	MAX77693_IRQ_GROUP_NR,
>  };
>  
> +#define SRC_IRQ_CHARGER			BIT(0)
> +#define SRC_IRQ_TOP			BIT(1)
> +#define SRC_IRQ_FLASH			BIT(2)
> +#define SRC_IRQ_MUIC			BIT(3)
> +
>  #define LED_IRQ_FLED2_OPEN		BIT(0)
>  #define LED_IRQ_FLED2_SHORT		BIT(1)
>  #define LED_IRQ_FLED1_OPEN		BIT(2)

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

Re: [PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Krzysztof Kozlowski]

On pią, 2014-10-10 at 09:58 +0100, Lee Jones wrote:
> On Fri, 10 Oct 2014, Krzysztof Kozlowski wrote:
> 
> > All interrupts coming from MUIC were ignored because interrupt source
> > register was masked.
> > 
> > The Maxim 77693 has a "interrupt source" - a separate register and interrupts
> > which give information about PMIC block triggering the individual
> > interrupt (charger, topsys, MUIC, flash LED).
> > 
> > By default bootloader could initialize this register to "mask all"
> > value. In such case (observed on Trats2 board) MUIC interrupts won't be
> > generated regardless of their mask status. Regmap irq chip was unmasking
> > individual MUIC interrupts but the source was masked
> > 
> > Before introducing regmap irq chip this interrupt source was unmasked,
> > read and acked. Reading and acking is not necessary but unmasking is. Do
> > it in extcon driver.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Cc: <stable@vger.kernel.org>
> > Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> > ---
> >  drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
> >  include/linux/mfd/max77693-private.h |  5 +++++
> >  2 files changed, 21 insertions(+)
> 
> Did you run this through checkpatch?

Yes, no complains.


> > diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
> > index 490e27475bac..d3adacf3187f 100644
> > --- a/drivers/extcon/extcon-max77693.c
> > +++ b/drivers/extcon/extcon-max77693.c
> > @@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
> >  		}
> >  	}
> >  
> > +	if (i) {
> > +		/*
> > +		 * i>0, at least one MUIC interrupt requested so need to unmask
> > +		 * MUIC interrupt in INTSRC register. Without that MUIC
> > +		 * interrupts will not be reported.
> > +		 */
> > +		ret = regmap_update_bits(max77693->regmap,
> > +				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
> > +				~SRC_IRQ_MUIC);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
> > +					ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	/* Initialize extcon device */
> >  	info->edev = devm_extcon_dev_allocate(&pdev->dev,
> >  					      max77693_extcon_cable);
> > diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
> > index e1b2b61285b9..4f1a80b66ecf 100644
> > --- a/include/linux/mfd/max77693-private.h
> > +++ b/include/linux/mfd/max77693-private.h
> > @@ -438,6 +438,11 @@ enum max77693_irq_source {
> >  	MAX77693_IRQ_GROUP_NR,
> >  };
> >  
> > +#define SRC_IRQ_CHARGER			BIT(0)
> > +#define SRC_IRQ_TOP			BIT(1)
> > +#define SRC_IRQ_FLASH			BIT(2)
> > +#define SRC_IRQ_MUIC			BIT(3)
> > +
> >  #define LED_IRQ_FLED2_OPEN		BIT(0)
> >  #define LED_IRQ_FLED2_SHORT		BIT(1)
> >  #define LED_IRQ_FLED1_OPEN		BIT(2)
> 

Re: [PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Krzysztof Kozlowski]

On pią, 2014-10-10 at 17:45 +0900, Chanwoo Choi wrote:
> On 10/10/2014 05:22 PM, Krzysztof Kozlowski wrote:
> > All interrupts coming from MUIC were ignored because interrupt source
> > register was masked.
> > 
> > The Maxim 77693 has a "interrupt source" - a separate register and interrupts
> > which give information about PMIC block triggering the individual
> > interrupt (charger, topsys, MUIC, flash LED).
> > 
> > By default bootloader could initialize this register to "mask all"
> > value. In such case (observed on Trats2 board) MUIC interrupts won't be
> > generated regardless of their mask status. Regmap irq chip was unmasking
> > individual MUIC interrupts but the source was masked
> > 
> > Before introducing regmap irq chip this interrupt source was unmasked,
> > read and acked. Reading and acking is not necessary but unmasking is. Do
> > it in extcon driver.
> > 
> > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > Cc: <stable@vger.kernel.org>
> > Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> > ---
> >  drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
> >  include/linux/mfd/max77693-private.h |  5 +++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
> > index 490e27475bac..d3adacf3187f 100644
> > --- a/drivers/extcon/extcon-max77693.c
> > +++ b/drivers/extcon/extcon-max77693.c
> > @@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
> >  		}
> >  	}
> >  
> > +	if (i) {
> > +		/*
> > +		 * i>0, at least one MUIC interrupt requested so need to unmask
> > +		 * MUIC interrupt in INTSRC register. Without that MUIC
> > +		 * interrupts will not be reported.
> > +		 */
> > +		ret = regmap_update_bits(max77693->regmap,
> > +				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
> > +				~SRC_IRQ_MUIC);
> > +		if (ret < 0) {
> > +			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
> > +					ret);
> > +			return ret;
> > +		}
> > +	}
> > +
> 
> No,
> The extcon-max77693 driver cannot enable IRQ_CHARGER/TOP/FLASH
> because MAX77693_PMIC_REG_INTSRC_MASK register is common register of MAX77693.
> 
> The IRQ_CHARGER/TOP/FLASH of MAX77693_PMIC_REG_INTSRC_MASK register should be enabled
> in drivers/mfd/max77693.c.

The code above unmasks only SRC_IRQ_MUIC. Other bits are not touched.
Why do you think that other IRQs will be enabled?

Best regards,
Krzysztof

> 
> >  	/* Initialize extcon device */
> >  	info->edev = devm_extcon_dev_allocate(&pdev->dev,
> >  					      max77693_extcon_cable);
> > diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
> > index e1b2b61285b9..4f1a80b66ecf 100644
> > --- a/include/linux/mfd/max77693-private.h
> > +++ b/include/linux/mfd/max77693-private.h
> > @@ -438,6 +438,11 @@ enum max77693_irq_source {
> >  	MAX77693_IRQ_GROUP_NR,
> >  };
> >  
> > +#define SRC_IRQ_CHARGER			BIT(0)
> > +#define SRC_IRQ_TOP			BIT(1)
> > +#define SRC_IRQ_FLASH			BIT(2)
> > +#define SRC_IRQ_MUIC			BIT(3)
> > +
> >  #define LED_IRQ_FLED2_OPEN		BIT(0)
> >  #define LED_IRQ_FLED2_SHORT		BIT(1)
> >  #define LED_IRQ_FLED1_OPEN		BIT(2)
> > 

Re: [PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Lee Jones]

On Fri, 10 Oct 2014, Krzysztof Kozlowski wrote:

> On pią, 2014-10-10 at 09:58 +0100, Lee Jones wrote:
> > On Fri, 10 Oct 2014, Krzysztof Kozlowski wrote:
> > 
> > > All interrupts coming from MUIC were ignored because interrupt source
> > > register was masked.
> > > 
> > > The Maxim 77693 has a "interrupt source" - a separate register and interrupts
> > > which give information about PMIC block triggering the individual
> > > interrupt (charger, topsys, MUIC, flash LED).
> > > 
> > > By default bootloader could initialize this register to "mask all"
> > > value. In such case (observed on Trats2 board) MUIC interrupts won't be
> > > generated regardless of their mask status. Regmap irq chip was unmasking
> > > individual MUIC interrupts but the source was masked
> > > 
> > > Before introducing regmap irq chip this interrupt source was unmasked,
> > > read and acked. Reading and acking is not necessary but unmasking is. Do
> > > it in extcon driver.
> > > 
> > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > Cc: <stable@vger.kernel.org>
> > > Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> > > ---
> > >  drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
> > >  include/linux/mfd/max77693-private.h |  5 +++++
> > >  2 files changed, 21 insertions(+)
> > 
> > Did you run this through checkpatch?
> 
> Yes, no complains.

Run it again!  One of those lines is clearly over 80 chars.

> > > diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
> > > index 490e27475bac..d3adacf3187f 100644
> > > --- a/drivers/extcon/extcon-max77693.c
> > > +++ b/drivers/extcon/extcon-max77693.c
> > > @@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
> > >  		}
> > >  	}
> > >  
> > > +	if (i) {
> > > +		/*
> > > +		 * i>0, at least one MUIC interrupt requested so need to unmask
> > > +		 * MUIC interrupt in INTSRC register. Without that MUIC
> > > +		 * interrupts will not be reported.
> > > +		 */
> > > +		ret = regmap_update_bits(max77693->regmap,
> > > +				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
> > > +				~SRC_IRQ_MUIC);
> > > +		if (ret < 0) {
> > > +			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
> > > +					ret);
> > > +			return ret;
> > > +		}
> > > +	}
> > > +
> > >  	/* Initialize extcon device */
> > >  	info->edev = devm_extcon_dev_allocate(&pdev->dev,
> > >  					      max77693_extcon_cable);
> > > diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
> > > index e1b2b61285b9..4f1a80b66ecf 100644
> > > --- a/include/linux/mfd/max77693-private.h
> > > +++ b/include/linux/mfd/max77693-private.h
> > > @@ -438,6 +438,11 @@ enum max77693_irq_source {
> > >  	MAX77693_IRQ_GROUP_NR,
> > >  };
> > >  
> > > +#define SRC_IRQ_CHARGER			BIT(0)
> > > +#define SRC_IRQ_TOP			BIT(1)
> > > +#define SRC_IRQ_FLASH			BIT(2)
> > > +#define SRC_IRQ_MUIC			BIT(3)
> > > +
> > >  #define LED_IRQ_FLED2_OPEN		BIT(0)
> > >  #define LED_IRQ_FLED2_SHORT		BIT(1)
> > >  #define LED_IRQ_FLED1_OPEN		BIT(2)
> > 
> 

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

Re: [PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Chanwoo Choi]

On 10/10/2014 06:03 PM, Krzysztof Kozlowski wrote:
> On pią, 2014-10-10 at 17:45 +0900, Chanwoo Choi wrote:
>> On 10/10/2014 05:22 PM, Krzysztof Kozlowski wrote:
>>> All interrupts coming from MUIC were ignored because interrupt source
>>> register was masked.
>>>
>>> The Maxim 77693 has a "interrupt source" - a separate register and interrupts
>>> which give information about PMIC block triggering the individual
>>> interrupt (charger, topsys, MUIC, flash LED).
>>>
>>> By default bootloader could initialize this register to "mask all"
>>> value. In such case (observed on Trats2 board) MUIC interrupts won't be
>>> generated regardless of their mask status. Regmap irq chip was unmasking
>>> individual MUIC interrupts but the source was masked
>>>
>>> Before introducing regmap irq chip this interrupt source was unmasked,
>>> read and acked. Reading and acking is not necessary but unmasking is. Do
>>> it in extcon driver.
>>>
>>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
>>> Cc: <stable@vger.kernel.org>
>>> Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
>>> ---
>>>  drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
>>>  include/linux/mfd/max77693-private.h |  5 +++++
>>>  2 files changed, 21 insertions(+)
>>>
>>> diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
>>> index 490e27475bac..d3adacf3187f 100644
>>> --- a/drivers/extcon/extcon-max77693.c
>>> +++ b/drivers/extcon/extcon-max77693.c
>>> @@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
>>>  		}
>>>  	}
>>>  
>>> +	if (i) {
>>> +		/*
>>> +		 * i>0, at least one MUIC interrupt requested so need to unmask
>>> +		 * MUIC interrupt in INTSRC register. Without that MUIC
>>> +		 * interrupts will not be reported.
>>> +		 */
>>> +		ret = regmap_update_bits(max77693->regmap,
>>> +				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
>>> +				~SRC_IRQ_MUIC);
>>> +		if (ret < 0) {
>>> +			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
>>> +					ret);
>>> +			return ret;
>>> +		}
>>> +	}
>>> +
>>
>> No,
>> The extcon-max77693 driver cannot enable IRQ_CHARGER/TOP/FLASH
>> because MAX77693_PMIC_REG_INTSRC_MASK register is common register of MAX77693.
>>
>> The IRQ_CHARGER/TOP/FLASH of MAX77693_PMIC_REG_INTSRC_MASK register should be enabled
>> in drivers/mfd/max77693.c.
> 
> The code above unmasks only SRC_IRQ_MUIC. Other bits are not touched.
> Why do you think that other IRQs will be enabled?

I think that MAX77693_PMIC_REG_INTSRC_MASK common register better to be controlled
in drivers/mfd/max77693.c. I prefer that MAX77693 extcon driver would read/write only MUIC registers.

Also, MAX77693 extcon driver have to be always unmasked.
If IRQ_MUIC is masked, MAX77693 extcon driver could not detect any external cables.

For example,
The max77836 enable MUIC_IRQ by un-masking MUIC_IRQ.
You can check it on drivers/mfd/max14577.c (line 251~258)

Best Regards,
Chanwoo Choi

> 
> Best regards,
> Krzysztof
> 
>>
>>>  	/* Initialize extcon device */
>>>  	info->edev = devm_extcon_dev_allocate(&pdev->dev,
>>>  					      max77693_extcon_cable);
>>> diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
>>> index e1b2b61285b9..4f1a80b66ecf 100644
>>> --- a/include/linux/mfd/max77693-private.h
>>> +++ b/include/linux/mfd/max77693-private.h
>>> @@ -438,6 +438,11 @@ enum max77693_irq_source {
>>>  	MAX77693_IRQ_GROUP_NR,
>>>  };
>>>  
>>> +#define SRC_IRQ_CHARGER			BIT(0)
>>> +#define SRC_IRQ_TOP			BIT(1)
>>> +#define SRC_IRQ_FLASH			BIT(2)
>>> +#define SRC_IRQ_MUIC			BIT(3)
>>> +
>>>  #define LED_IRQ_FLED2_OPEN		BIT(0)
>>>  #define LED_IRQ_FLED2_SHORT		BIT(1)
>>>  #define LED_IRQ_FLED1_OPEN		BIT(2)
>>>
> 
> 

Re: [PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Krzysztof Kozlowski]

On pią, 2014-10-10 at 10:48 +0100, Lee Jones wrote:
> On Fri, 10 Oct 2014, Krzysztof Kozlowski wrote:
> 
> > On pią, 2014-10-10 at 09:58 +0100, Lee Jones wrote:
> > > On Fri, 10 Oct 2014, Krzysztof Kozlowski wrote:
> > > 
> > > > All interrupts coming from MUIC were ignored because interrupt source
> > > > register was masked.
> > > > 
> > > > The Maxim 77693 has a "interrupt source" - a separate register and interrupts
> > > > which give information about PMIC block triggering the individual
> > > > interrupt (charger, topsys, MUIC, flash LED).
> > > > 
> > > > By default bootloader could initialize this register to "mask all"
> > > > value. In such case (observed on Trats2 board) MUIC interrupts won't be
> > > > generated regardless of their mask status. Regmap irq chip was unmasking
> > > > individual MUIC interrupts but the source was masked
> > > > 
> > > > Before introducing regmap irq chip this interrupt source was unmasked,
> > > > read and acked. Reading and acking is not necessary but unmasking is. Do
> > > > it in extcon driver.
> > > > 
> > > > Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> > > > Cc: <stable@vger.kernel.org>
> > > > Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> > > > ---
> > > >  drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
> > > >  include/linux/mfd/max77693-private.h |  5 +++++
> > > >  2 files changed, 21 insertions(+)
> > > 
> > > Did you run this through checkpatch?
> > 
> > Yes, no complains.
> 
> Run it again!  One of those lines is clearly over 80 chars.

>From the checkpatch still nothing. Really! :)

I believe you're mentioning the line:
dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",

Probably checkpatch does not complain about it because it is the string
who dares to exceed 80 chars. I can split this into:

dev_err(&pdev->dev,
	"Could not unmask MUIC interrupt in INTSRC: %d\n",
	ret);

Best regards,
Krzysztof

> 
> > > > diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
> > > > index 490e27475bac..d3adacf3187f 100644
> > > > --- a/drivers/extcon/extcon-max77693.c
> > > > +++ b/drivers/extcon/extcon-max77693.c
> > > > @@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
> > > >  		}
> > > >  	}
> > > >  
> > > > +	if (i) {
> > > > +		/*
> > > > +		 * i>0, at least one MUIC interrupt requested so need to unmask
> > > > +		 * MUIC interrupt in INTSRC register. Without that MUIC
> > > > +		 * interrupts will not be reported.
> > > > +		 */
> > > > +		ret = regmap_update_bits(max77693->regmap,
> > > > +				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
> > > > +				~SRC_IRQ_MUIC);
> > > > +		if (ret < 0) {
> > > > +			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
> > > > +					ret);
> > > > +			return ret;
> > > > +		}
> > > > +	}
> > > > +
> > > >  	/* Initialize extcon device */
> > > >  	info->edev = devm_extcon_dev_allocate(&pdev->dev,
> > > >  					      max77693_extcon_cable);
> > > > diff --git a/include/linux/mfd/max77693-private.h b/include/linux/mfd/max77693-private.h
> > > > index e1b2b61285b9..4f1a80b66ecf 100644
> > > > --- a/include/linux/mfd/max77693-private.h
> > > > +++ b/include/linux/mfd/max77693-private.h
> > > > @@ -438,6 +438,11 @@ enum max77693_irq_source {
> > > >  	MAX77693_IRQ_GROUP_NR,
> > > >  };
> > > >  
> > > > +#define SRC_IRQ_CHARGER			BIT(0)
> > > > +#define SRC_IRQ_TOP			BIT(1)
> > > > +#define SRC_IRQ_FLASH			BIT(2)
> > > > +#define SRC_IRQ_MUIC			BIT(3)
> > > > +
> > > >  #define LED_IRQ_FLED2_OPEN		BIT(0)
> > > >  #define LED_IRQ_FLED2_SHORT		BIT(1)
> > > >  #define LED_IRQ_FLED1_OPEN		BIT(2)
> > > 
> > 
> 

Re: [PATCH fixes 2/2] extcon: max77693: Fix always masked MUIC interrupts

[Krzysztof Kozlowski]

On pią, 2014-10-10 at 19:05 +0900, Chanwoo Choi wrote:
> On 10/10/2014 06:03 PM, Krzysztof Kozlowski wrote:
> > On pią, 2014-10-10 at 17:45 +0900, Chanwoo Choi wrote:
> >> On 10/10/2014 05:22 PM, Krzysztof Kozlowski wrote:
> >>> All interrupts coming from MUIC were ignored because interrupt source
> >>> register was masked.
> >>>
> >>> The Maxim 77693 has a "interrupt source" - a separate register and interrupts
> >>> which give information about PMIC block triggering the individual
> >>> interrupt (charger, topsys, MUIC, flash LED).
> >>>
> >>> By default bootloader could initialize this register to "mask all"
> >>> value. In such case (observed on Trats2 board) MUIC interrupts won't be
> >>> generated regardless of their mask status. Regmap irq chip was unmasking
> >>> individual MUIC interrupts but the source was masked
> >>>
> >>> Before introducing regmap irq chip this interrupt source was unmasked,
> >>> read and acked. Reading and acking is not necessary but unmasking is. Do
> >>> it in extcon driver.
> >>>
> >>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> >>> Cc: <stable@vger.kernel.org>
> >>> Fixes: 342d669c1ee4 ("mfd: max77693: Handle IRQs using regmap")
> >>> ---
> >>>  drivers/extcon/extcon-max77693.c     | 16 ++++++++++++++++
> >>>  include/linux/mfd/max77693-private.h |  5 +++++
> >>>  2 files changed, 21 insertions(+)
> >>>
> >>> diff --git a/drivers/extcon/extcon-max77693.c b/drivers/extcon/extcon-max77693.c
> >>> index 490e27475bac..d3adacf3187f 100644
> >>> --- a/drivers/extcon/extcon-max77693.c
> >>> +++ b/drivers/extcon/extcon-max77693.c
> >>> @@ -1176,6 +1176,22 @@ static int max77693_muic_probe(struct platform_device *pdev)
> >>>  		}
> >>>  	}
> >>>  
> >>> +	if (i) {
> >>> +		/*
> >>> +		 * i>0, at least one MUIC interrupt requested so need to unmask
> >>> +		 * MUIC interrupt in INTSRC register. Without that MUIC
> >>> +		 * interrupts will not be reported.
> >>> +		 */
> >>> +		ret = regmap_update_bits(max77693->regmap,
> >>> +				MAX77693_PMIC_REG_INTSRC_MASK, SRC_IRQ_MUIC,
> >>> +				~SRC_IRQ_MUIC);
> >>> +		if (ret < 0) {
> >>> +			dev_err(&pdev->dev, "Could not unmask MUIC interrupt in INTSRC: %d\n",
> >>> +					ret);
> >>> +			return ret;
> >>> +		}
> >>> +	}
> >>> +
> >>
> >> No,
> >> The extcon-max77693 driver cannot enable IRQ_CHARGER/TOP/FLASH
> >> because MAX77693_PMIC_REG_INTSRC_MASK register is common register of MAX77693.
> >>
> >> The IRQ_CHARGER/TOP/FLASH of MAX77693_PMIC_REG_INTSRC_MASK register should be enabled
> >> in drivers/mfd/max77693.c.
> > 
> > The code above unmasks only SRC_IRQ_MUIC. Other bits are not touched.
> > Why do you think that other IRQs will be enabled?
> 
> I think that MAX77693_PMIC_REG_INTSRC_MASK common register better to be controlled
> in drivers/mfd/max77693.c. I prefer that MAX77693 extcon driver would read/write only MUIC registers.
> 
> Also, MAX77693 extcon driver have to be always unmasked.
> If IRQ_MUIC is masked, MAX77693 extcon driver could not detect any external cables.
> 
> For example,
> The max77836 enable MUIC_IRQ by un-masking MUIC_IRQ.
> You can check it on drivers/mfd/max14577.c (line 251~258)

Sure, I get it. I'll do it the same as in max14577 driver.

Thanks for feedback!
Krzysztof