Re: [PATCH RFC] power: supply: qcom_battmgr: Register the power supplies after PDR is up

[Luca Weiss]

On Mon Dec 18, 2023 at 3:41 PM CET, Konrad Dybcio wrote:
> Currently, a not-yet-entirely-initialized battmgr (e.g. with pd-mapper
> not having yet started or ADSP not being up etc.) results in a couple of
> zombie power supply devices hanging around.
>
> This is particularly noticeable when trying to suspend the device (even
> s2idle): the PSY-internal thermal zone is inaccessible and returns
> -ENODEV, which causes log spam.
>
> Register the power supplies only after we received some notification
> indicating battmgr is ready to take off.

Hi Konrad,

This seems to fix an issue I'm seeing consistently on
qcm6490-fairphone-fp5 when having adsp driver built-in and manually
starting it with:
  echo start | sudo tee /sys/class/remoteproc/remoteproc0/state

Before upower was thinking line_power_qcom_battmgr_usb and
line_power_qcom_battmgr_wls were "battery" devices with 0% charge and
not "line-power" (which is what they're shown as now, and also before
with adsp =m).
That caused upower to have its "Display Device" only show a third of the
actual battery percent because it was including those fake batteries
also.

At least for this exact case (didn't test more tbh) I can offer my:

Tested-by: Luca Weiss <luca.weiss@xxxxxxxxxxxxx>

Regards
Luca

>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> ---
>  drivers/power/supply/qcom_battmgr.c | 109 ++++++++++++++++++++----------------
>  1 file changed, 60 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> index ec163d1bcd18..a12e2a66d516 100644
> --- a/drivers/power/supply/qcom_battmgr.c
> +++ b/drivers/power/supply/qcom_battmgr.c
> @@ -282,6 +282,7 @@ struct qcom_battmgr_wireless {
>  
>  struct qcom_battmgr {
>  	struct device *dev;
> +	struct auxiliary_device *adev;
>  	struct pmic_glink_client *client;
>  
>  	enum qcom_battmgr_variant variant;
> @@ -1293,11 +1294,69 @@ static void qcom_battmgr_enable_worker(struct work_struct *work)
>  		dev_err(battmgr->dev, "failed to request power notifications\n");
>  }
>  
> +static char *qcom_battmgr_battery[] = { "battery" };
> +
> +static void qcom_battmgr_register_psy(struct qcom_battmgr *battmgr)
> +{
> +	struct power_supply_config psy_cfg_supply = {};
> +	struct auxiliary_device *adev = battmgr->adev;
> +	struct power_supply_config psy_cfg = {};
> +	struct device *dev = &adev->dev;
> +
> +	psy_cfg.drv_data = battmgr;
> +	psy_cfg.of_node = adev->dev.of_node;
> +
> +	psy_cfg_supply.drv_data = battmgr;
> +	psy_cfg_supply.of_node = adev->dev.of_node;
> +	psy_cfg_supply.supplied_to = qcom_battmgr_battery;
> +	psy_cfg_supply.num_supplicants = 1;
> +
> +	if (battmgr->variant == QCOM_BATTMGR_SC8280XP) {
> +		battmgr->bat_psy = devm_power_supply_register(dev, &sc8280xp_bat_psy_desc, &psy_cfg);
> +		if (IS_ERR(battmgr->bat_psy))
> +			dev_err(dev, "failed to register battery power supply (%ld)\n",
> +				PTR_ERR(battmgr->bat_psy));
> +
> +		battmgr->ac_psy = devm_power_supply_register(dev, &sc8280xp_ac_psy_desc, &psy_cfg_supply);
> +		if (IS_ERR(battmgr->ac_psy))
> +			dev_err(dev, "failed to register AC power supply (%ld)\n",
> +				PTR_ERR(battmgr->ac_psy));
> +
> +		battmgr->usb_psy = devm_power_supply_register(dev, &sc8280xp_usb_psy_desc, &psy_cfg_supply);
> +		if (IS_ERR(battmgr->usb_psy))
> +			dev_err(dev, "failed to register USB power supply (%ld)\n",
> +				PTR_ERR(battmgr->usb_psy));
> +
> +		battmgr->wls_psy = devm_power_supply_register(dev, &sc8280xp_wls_psy_desc, &psy_cfg_supply);
> +		if (IS_ERR(battmgr->wls_psy))
> +			dev_err(dev, "failed to register wireless charing power supply (%ld)\n",
> +				PTR_ERR(battmgr->wls_psy));
> +	} else {
> +		battmgr->bat_psy = devm_power_supply_register(dev, &sm8350_bat_psy_desc, &psy_cfg);
> +		if (IS_ERR(battmgr->bat_psy))
> +			dev_err(dev, "failed to register battery power supply (%ld)\n",
> +				PTR_ERR(battmgr->bat_psy));
> +
> +		battmgr->usb_psy = devm_power_supply_register(dev, &sm8350_usb_psy_desc, &psy_cfg_supply);
> +		if (IS_ERR(battmgr->usb_psy))
> +			dev_err(dev, "failed to register USB power supply (%ld)\n",
> +				PTR_ERR(battmgr->usb_psy));
> +
> +		battmgr->wls_psy = devm_power_supply_register(dev, &sm8350_wls_psy_desc, &psy_cfg_supply);
> +		if (IS_ERR(battmgr->wls_psy))
> +			dev_err(dev, "failed to register wireless charing power supply (%ld)\n",
> +				PTR_ERR(battmgr->wls_psy));
> +	}
> +}
> +
>  static void qcom_battmgr_pdr_notify(void *priv, int state)
>  {
>  	struct qcom_battmgr *battmgr = priv;
>  
>  	if (state == SERVREG_SERVICE_STATE_UP) {
> +		if (!battmgr->bat_psy)
> +			qcom_battmgr_register_psy(battmgr);
> +
>  		battmgr->service_up = true;
>  		schedule_work(&battmgr->enable_work);
>  	} else {
> @@ -1312,13 +1371,9 @@ static const struct of_device_id qcom_battmgr_of_variants[] = {
>  	{}
>  };
>  
> -static char *qcom_battmgr_battery[] = { "battery" };
> -
>  static int qcom_battmgr_probe(struct auxiliary_device *adev,
>  			      const struct auxiliary_device_id *id)
>  {
> -	struct power_supply_config psy_cfg_supply = {};
> -	struct power_supply_config psy_cfg = {};
>  	const struct of_device_id *match;
>  	struct qcom_battmgr *battmgr;
>  	struct device *dev = &adev->dev;
> @@ -1328,14 +1383,7 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev,
>  		return -ENOMEM;
>  
>  	battmgr->dev = dev;
> -
> -	psy_cfg.drv_data = battmgr;
> -	psy_cfg.of_node = adev->dev.of_node;
> -
> -	psy_cfg_supply.drv_data = battmgr;
> -	psy_cfg_supply.of_node = adev->dev.of_node;
> -	psy_cfg_supply.supplied_to = qcom_battmgr_battery;
> -	psy_cfg_supply.num_supplicants = 1;
> +	battmgr->adev = adev;
>  
>  	INIT_WORK(&battmgr->enable_work, qcom_battmgr_enable_worker);
>  	mutex_init(&battmgr->lock);
> @@ -1347,43 +1395,6 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev,
>  	else
>  		battmgr->variant = QCOM_BATTMGR_SM8350;
>  
> -	if (battmgr->variant == QCOM_BATTMGR_SC8280XP) {
> -		battmgr->bat_psy = devm_power_supply_register(dev, &sc8280xp_bat_psy_desc, &psy_cfg);
> -		if (IS_ERR(battmgr->bat_psy))
> -			return dev_err_probe(dev, PTR_ERR(battmgr->bat_psy),
> -					     "failed to register battery power supply\n");
> -
> -		battmgr->ac_psy = devm_power_supply_register(dev, &sc8280xp_ac_psy_desc, &psy_cfg_supply);
> -		if (IS_ERR(battmgr->ac_psy))
> -			return dev_err_probe(dev, PTR_ERR(battmgr->ac_psy),
> -					     "failed to register AC power supply\n");
> -
> -		battmgr->usb_psy = devm_power_supply_register(dev, &sc8280xp_usb_psy_desc, &psy_cfg_supply);
> -		if (IS_ERR(battmgr->usb_psy))
> -			return dev_err_probe(dev, PTR_ERR(battmgr->usb_psy),
> -					     "failed to register USB power supply\n");
> -
> -		battmgr->wls_psy = devm_power_supply_register(dev, &sc8280xp_wls_psy_desc, &psy_cfg_supply);
> -		if (IS_ERR(battmgr->wls_psy))
> -			return dev_err_probe(dev, PTR_ERR(battmgr->wls_psy),
> -					     "failed to register wireless charing power supply\n");
> -	} else {
> -		battmgr->bat_psy = devm_power_supply_register(dev, &sm8350_bat_psy_desc, &psy_cfg);
> -		if (IS_ERR(battmgr->bat_psy))
> -			return dev_err_probe(dev, PTR_ERR(battmgr->bat_psy),
> -					     "failed to register battery power supply\n");
> -
> -		battmgr->usb_psy = devm_power_supply_register(dev, &sm8350_usb_psy_desc, &psy_cfg_supply);
> -		if (IS_ERR(battmgr->usb_psy))
> -			return dev_err_probe(dev, PTR_ERR(battmgr->usb_psy),
> -					     "failed to register USB power supply\n");
> -
> -		battmgr->wls_psy = devm_power_supply_register(dev, &sm8350_wls_psy_desc, &psy_cfg_supply);
> -		if (IS_ERR(battmgr->wls_psy))
> -			return dev_err_probe(dev, PTR_ERR(battmgr->wls_psy),
> -					     "failed to register wireless charing power supply\n");
> -	}
> -
>  	battmgr->client = devm_pmic_glink_register_client(dev,
>  							  PMIC_GLINK_OWNER_BATTMGR,
>  							  qcom_battmgr_callback,
>
> ---
> base-commit: ceb2fe0d438644e1de06b9a6468a1fb8e2199c70
> change-id: 20231218-topic-battmgr_fixture_attempt-ec86ef8df7e3
>
> Best regards,