Re: [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking

From: Hans de Goede <hdegoede@redhat.com>
To: Mark Gross <markgross@kernel.org>, Andy Shevchenko <andy@kernel.org>
Cc: platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking
Date: Wed, 12 Jan 2022 13:29:38 +0100	[thread overview]
Message-ID: <10f4870d-660a-0181-6e0a-1196dc1f2e70@redhat.com> (raw)
In-Reply-To: <20220111232309.377642-1-hdegoede@redhat.com>

Hi,

On 1/12/22 00:23, Hans de Goede wrote:
> The driver as originally submitted accidentally relied on Android having
> run before and Android having unmasked the 2nd level IRQ-mask for the
> charger IRQ. This worked since these are PMIC registers which are only
> reset when the battery is fully drained or disconnected.
> 
> Fix the charger IRQ no longer working after loss of battery power by
> properly setting the 2nd level IRQ-mask for the charger IRQ.
> 
> Note this removes the need to enable/disable our parent IRQ which just
> sets the mask bit in the 1st level IRQ-mask register, setting one of
> the 2 level masks is enough to stop the IRQ from getting reported.
> 
> Fixes: 761db353d9e2 ("platform/x86: Add intel_crystal_cove_charger driver")
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  .../platform/x86/intel/crystal_cove_charger.c | 26 +++++++++----------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/platform/x86/intel/crystal_cove_charger.c b/drivers/platform/x86/intel/crystal_cove_charger.c
> index 0374bc742513..eeaa926d2058 100644
> --- a/drivers/platform/x86/intel/crystal_cove_charger.c
> +++ b/drivers/platform/x86/intel/crystal_cove_charger.c
> @@ -17,6 +17,7 @@
>  #include <linux/regmap.h>
>  
>  #define CHGRIRQ_REG					0x0a
> +#define MCHGRIRQ_REG					0x17
>  
>  struct crystal_cove_charger_data {
>  	struct mutex buslock; /* irq_bus_lock */
> @@ -25,8 +26,8 @@ struct crystal_cove_charger_data {
>  	struct irq_domain *irq_domain;
>  	int irq;
>  	int charger_irq;
> -	bool irq_enabled;
> -	bool irq_is_enabled;
> +	u8 mask;
> +	u8 new_mask;
>  };
>  
>  static irqreturn_t crystal_cove_charger_irq(int irq, void *data)
> @@ -53,13 +54,9 @@ static void crystal_cove_charger_irq_bus_sync_unlock(struct irq_data *data)
>  {
>  	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
>  
> -	if (charger->irq_is_enabled != charger->irq_enabled) {
> -		if (charger->irq_enabled)
> -			enable_irq(charger->irq);
> -		else
> -			disable_irq(charger->irq);
> -
> -		charger->irq_is_enabled = charger->irq_enabled;
> +	if (charger->mask != charger->new_mask) {
> +		regmap_write(charger->regmap, MCHGRIRQ_REG, charger->new_mask);
> +		charger->mask = charger->new_mask;
>  	}
>  
>  	mutex_unlock(&charger->buslock);
> @@ -69,14 +66,14 @@ static void crystal_cove_charger_irq_unmask(struct irq_data *data)
>  {
>  	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
>  
> -	charger->irq_enabled = true;
> +	charger->new_mask &= ~BIT(data->hwirq);
>  }
>  
>  static void crystal_cove_charger_irq_mask(struct irq_data *data)
>  {
>  	struct crystal_cove_charger_data *charger = irq_data_get_irq_chip_data(data);
>  
> -	charger->irq_enabled = false;
> +	charger->new_mask |= BIT(data->hwirq);
>  }
>  
>  static void crystal_cove_charger_rm_irq_domain(void *data)
> @@ -130,10 +127,13 @@ static int crystal_cove_charger_probe(struct platform_device *pdev)
>  	irq_set_nested_thread(charger->charger_irq, true);
>  	irq_set_noprobe(charger->charger_irq);
>  
> +	/* Mask the single 2nd level IRQ before enabling the 1st level IRQ */
> +	charger->mask = BIT(0);

I just realized that this also needs to set charger->new_mask. I will fix this
up when merging this.

> +	regmap_write(charger->regmap, MCHGRIRQ_REG, charger->mask);
> +
>  	ret = devm_request_threaded_irq(&pdev->dev, charger->irq, NULL,
>  					crystal_cove_charger_irq,
> -					IRQF_ONESHOT | IRQF_NO_AUTOEN,
> -					KBUILD_MODNAME, charger);
> +					IRQF_ONESHOT, KBUILD_MODNAME, charger);
>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "requesting irq\n");
>  
> 

Regards,

Hans