| Message ID | 20180222074521.23783-1-linus.walleij@linaro.org |
|---|---|
| State | Accepted |
| Commit | 4a9be940551ab664918ac089b92c47d74e6cb8e7 |
| Headers | show |
| Series | [v2] power: gemini-poweroff: Avoid spurious poweroff | expand |
Hi Linus, On Thu, Feb 22, 2018 at 08:45:21AM +0100, Linus Walleij wrote: > On the D-Link DIR-685 we get spurious poweroff from > infrared. Since that block (CIR) doesn't even have a > driver this can be safely ignored, we can revisit this > code once we have a device supporting CIR. > > On the D-Link DNS-313 we get spurious poweroff from > the power button. This appears to be an initialization > issue: we need to enable the block (start the state > machine) before we clear any dangling IRQ. > > This patch fixes both issues. > > Fixes: f7a388d6cd1c ("power: reset: Add a driver for the Gemini poweroff") > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > ChangeLog v1->v2: > - Fix both issues and rename the patch. > - Proper commit message with specifics. > --- Applying to power-supply's fixes branch. -- Sebastian > drivers/power/reset/gemini-poweroff.c | 30 +++++++++++++++++------------- > 1 file changed, 17 insertions(+), 13 deletions(-) > > diff --git a/drivers/power/reset/gemini-poweroff.c b/drivers/power/reset/gemini-poweroff.c > index ff75af5abbc5..2ac291af1265 100644 > --- a/drivers/power/reset/gemini-poweroff.c > +++ b/drivers/power/reset/gemini-poweroff.c > @@ -47,8 +47,12 @@ static irqreturn_t gemini_powerbutton_interrupt(int irq, void *data) > val &= 0x70U; > switch (val) { > case GEMINI_STAT_CIR: > - dev_info(gpw->dev, "infrared poweroff\n"); > - orderly_poweroff(true); > + /* > + * We do not yet have a driver for the infrared > + * controller so it can cause spurious poweroff > + * events. Ignore those for now. > + */ > + dev_info(gpw->dev, "infrared poweroff - ignored\n"); > break; > case GEMINI_STAT_RTC: > dev_info(gpw->dev, "RTC poweroff\n"); > @@ -116,7 +120,17 @@ static int gemini_poweroff_probe(struct platform_device *pdev) > return -ENODEV; > } > > - /* Clear the power management IRQ */ > + /* > + * Enable the power controller. This is crucial on Gemini > + * systems: if this is not done, pressing the power button > + * will result in unconditional poweroff without any warning. > + * This makes the kernel handle the poweroff. > + */ > + val = readl(gpw->base + GEMINI_PWC_CTRLREG); > + val |= GEMINI_CTRL_ENABLE; > + writel(val, gpw->base + GEMINI_PWC_CTRLREG); > + > + /* Now that the state machine is active, clear the IRQ */ > val = readl(gpw->base + GEMINI_PWC_CTRLREG); > val |= GEMINI_CTRL_IRQ_CLR; > writel(val, gpw->base + GEMINI_PWC_CTRLREG); > @@ -129,16 +143,6 @@ static int gemini_poweroff_probe(struct platform_device *pdev) > pm_power_off = gemini_poweroff; > gpw_poweroff = gpw; > > - /* > - * Enable the power controller. This is crucial on Gemini > - * systems: if this is not done, pressing the power button > - * will result in unconditional poweroff without any warning. > - * This makes the kernel handle the poweroff. > - */ > - val = readl(gpw->base + GEMINI_PWC_CTRLREG); > - val |= GEMINI_CTRL_ENABLE; > - writel(val, gpw->base + GEMINI_PWC_CTRLREG); > - > dev_info(dev, "Gemini poweroff driver registered\n"); > > return 0; > -- > 2.14.3 >
diff --git a/drivers/power/reset/gemini-poweroff.c b/drivers/power/reset/gemini-poweroff.c index ff75af5abbc5..2ac291af1265 100644 --- a/drivers/power/reset/gemini-poweroff.c +++ b/drivers/power/reset/gemini-poweroff.c @@ -47,8 +47,12 @@ static irqreturn_t gemini_powerbutton_interrupt(int irq, void *data) val &= 0x70U; switch (val) { case GEMINI_STAT_CIR: - dev_info(gpw->dev, "infrared poweroff\n"); - orderly_poweroff(true); + /* + * We do not yet have a driver for the infrared + * controller so it can cause spurious poweroff + * events. Ignore those for now. + */ + dev_info(gpw->dev, "infrared poweroff - ignored\n"); break; case GEMINI_STAT_RTC: dev_info(gpw->dev, "RTC poweroff\n"); @@ -116,7 +120,17 @@ static int gemini_poweroff_probe(struct platform_device *pdev) return -ENODEV; } - /* Clear the power management IRQ */ + /* + * Enable the power controller. This is crucial on Gemini + * systems: if this is not done, pressing the power button + * will result in unconditional poweroff without any warning. + * This makes the kernel handle the poweroff. + */ + val = readl(gpw->base + GEMINI_PWC_CTRLREG); + val |= GEMINI_CTRL_ENABLE; + writel(val, gpw->base + GEMINI_PWC_CTRLREG); + + /* Now that the state machine is active, clear the IRQ */ val = readl(gpw->base + GEMINI_PWC_CTRLREG); val |= GEMINI_CTRL_IRQ_CLR; writel(val, gpw->base + GEMINI_PWC_CTRLREG); @@ -129,16 +143,6 @@ static int gemini_poweroff_probe(struct platform_device *pdev) pm_power_off = gemini_poweroff; gpw_poweroff = gpw; - /* - * Enable the power controller. This is crucial on Gemini - * systems: if this is not done, pressing the power button - * will result in unconditional poweroff without any warning. - * This makes the kernel handle the poweroff. - */ - val = readl(gpw->base + GEMINI_PWC_CTRLREG); - val |= GEMINI_CTRL_ENABLE; - writel(val, gpw->base + GEMINI_PWC_CTRLREG); - dev_info(dev, "Gemini poweroff driver registered\n"); return 0;
On the D-Link DIR-685 we get spurious poweroff from infrared. Since that block (CIR) doesn't even have a driver this can be safely ignored, we can revisit this code once we have a device supporting CIR. On the D-Link DNS-313 we get spurious poweroff from the power button. This appears to be an initialization issue: we need to enable the block (start the state machine) before we clear any dangling IRQ. This patch fixes both issues. Fixes: f7a388d6cd1c ("power: reset: Add a driver for the Gemini poweroff") Signed-off-by: Linus Walleij <linus.walleij@linaro.org> --- ChangeLog v1->v2: - Fix both issues and rename the patch. - Proper commit message with specifics. --- drivers/power/reset/gemini-poweroff.c | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) -- 2.14.3