Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 9 Jan 2023 17:11:52 +0000
From:   Lee Jones <lee@kernel.org>
To:     Samuel Holland <samuel@sholland.org>
Cc:     Chen-Yu Tsai <wens@csie.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mfd: axp20x: Switch to the sys-off handler API
Message-ID: <Y7xK2AkHlr7hIzwC@google.com>
References: <20221228162752.14204-1-samuel@sholland.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20221228162752.14204-1-samuel@sholland.org>
Precedence: bulk

On Wed, 28 Dec 2022, Samuel Holland wrote:

> This removes a layer of indirection through pm_power_off() and allows
> the PMIC handler to be used as a fallback when firmware power off fails.
> This happens on boards like the Clockwork DevTerm R-01 where OpenSBI
> does not know how to use the PMIC to power off the board.
> 
> Move the check for AXP288 to avoid registering a dummy handler.
> 
> Signed-off-by: Samuel Holland <samuel@sholland.org>
> ---
> Note: this patch depends on updating the PSCI driver first[1] to keep
> the existing callback priority.
> 
> [1]: https://lore.kernel.org/lkml/20221105214841.7828-1-samuel@sholland.org/
> 
>  drivers/mfd/axp20x.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 47fd700f284f..58f82f1f4b43 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -23,7 +23,7 @@
>  #include <linux/mfd/core.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> -#include <linux/pm_runtime.h>
> +#include <linux/reboot.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  
> @@ -825,17 +825,17 @@ static const struct mfd_cell axp813_cells[] = {
>  	},
>  };
>  
> -static struct axp20x_dev *axp20x_pm_power_off;
> -static void axp20x_power_off(void)
> +static int axp20x_power_off(struct sys_off_data *data)
>  {
> -	if (axp20x_pm_power_off->variant == AXP288_ID)
> -		return;
> +	struct axp20x_dev *axp20x = data->cb_data;
>  
> -	regmap_write(axp20x_pm_power_off->regmap, AXP20X_OFF_CTRL,
> +	regmap_write(axp20x->regmap, AXP20X_OFF_CTRL,
>  		     AXP20X_OFF);

Nit: Can you pop this on the line above now, thanks.

In fact, forget it, I'll do it.

>  	/* Give capacitors etc. time to drain to avoid kernel panic msg. */
>  	mdelay(500);
> +
> +	return NOTIFY_DONE;
>  }
>  
>  int axp20x_match_device(struct axp20x_dev *axp20x)
> @@ -1002,10 +1002,11 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
>  		return ret;
>  	}
>  
> -	if (!pm_power_off) {
> -		axp20x_pm_power_off = axp20x;
> -		pm_power_off = axp20x_power_off;
> -	}
> +	if (axp20x->variant != AXP288_ID)
> +		devm_register_sys_off_handler(axp20x->dev,
> +					      SYS_OFF_MODE_POWER_OFF,
> +					      SYS_OFF_PRIO_DEFAULT,
> +					      axp20x_power_off, axp20x);
>  
>  	dev_info(axp20x->dev, "AXP20X driver loaded\n");
>  
> @@ -1015,11 +1016,6 @@ EXPORT_SYMBOL(axp20x_device_probe);
>  
>  void axp20x_device_remove(struct axp20x_dev *axp20x)
>  {
> -	if (axp20x == axp20x_pm_power_off) {
> -		axp20x_pm_power_off = NULL;
> -		pm_power_off = NULL;
> -	}
> -
>  	mfd_remove_devices(axp20x->dev);
>  	regmap_del_irq_chip(axp20x->irq, axp20x->regmap_irqc);
>  }

Applied, thanks

-- 
Lee Jones [李琼斯]