* Re: [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking
2022-01-11 23:23 [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking Hans de Goede
@ 2022-01-12 12:29 ` Hans de Goede
2022-01-15 0:05 ` Mark Gross
2022-01-17 10:12 ` Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2022-01-12 12:29 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko; +Cc: platform-driver-x86
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking
2022-01-11 23:23 [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking Hans de Goede
2022-01-12 12:29 ` Hans de Goede
@ 2022-01-15 0:05 ` Mark Gross
2022-01-17 10:10 ` Hans de Goede
2022-01-17 10:12 ` Hans de Goede
2 siblings, 1 reply; 5+ messages in thread
From: Mark Gross @ 2022-01-15 0:05 UTC (permalink / raw)
To: Hans de Goede; +Cc: Mark Gross, Andy Shevchenko, platform-driver-x86
On Wed, Jan 12, 2022 at 12:23:09AM +0100, 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.
Correct me if I'm wrong but Android is a usermode stack. It cannot unmask
IRQ's form user mode.
I'm guessing the persistant PMIC registeres where set when running a vendor
kernel that was in use with Anroid prior to installing one using this change
and a modern kernel, and after the batteries ran out or where removed the
charger didn't work.
--mark
>
> 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);
> + 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");
>
> --
> 2.33.1
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking
2022-01-15 0:05 ` Mark Gross
@ 2022-01-17 10:10 ` Hans de Goede
0 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2022-01-17 10:10 UTC (permalink / raw)
To: Mark Gross; +Cc: Andy Shevchenko, platform-driver-x86
Hi,
On 1/15/22 01:05, Mark Gross wrote:
> On Wed, Jan 12, 2022 at 12:23:09AM +0100, 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.
> Correct me if I'm wrong but Android is a usermode stack. It cannot unmask
> IRQ's form user mode.
Android is an OS, consisting of what in many cases amounts to a fork of
the Linux kernel + a usermode stack. With "Android" in the commit msg
before I was referring to the entire OS, including the custom kernel shipped
with the OS.
> I'm guessing the persistant PMIC registeres where set when running a vendor
> kernel that was in use with Anroid prior to installing one using this change
> and a modern kernel, and after the batteries ran out or where removed the
> charger didn't work.
Right.
Regards,
Hans
>> 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);
>> + 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");
>>
>> --
>> 2.33.1
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking
2022-01-11 23:23 [PATCH] platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking Hans de Goede
2022-01-12 12:29 ` Hans de Goede
2022-01-15 0:05 ` Mark Gross
@ 2022-01-17 10:12 ` Hans de Goede
2 siblings, 0 replies; 5+ messages in thread
From: Hans de Goede @ 2022-01-17 10:12 UTC (permalink / raw)
To: Mark Gross, Andy Shevchenko; +Cc: platform-driver-x86
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>
I've added this to my review-hans (soon to be for-next) branch now.
Regards,
Hans
> ---
> .../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);
> + 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");
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread