| Message ID | 20220111232309.377642-1-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: intel_crystal_cove_charger: Fix IRQ masking / unmasking | expand |
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
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 >
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 >> >
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"); > >
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");
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(-)