* [PATCH AUTOSEL 6.7 15/39] power: supply: qcom_battmgr: Register the power supplies after PDR is up
[not found] <20240128161130.200783-1-sashal@kernel.org>
@ 2024-01-28 16:10 ` Sasha Levin
2024-01-29 13:03 ` Sebastian Reichel
0 siblings, 1 reply; 3+ messages in thread
From: Sasha Levin @ 2024-01-28 16:10 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Konrad Dybcio, Luca Weiss, Sebastian Reichel, Sasha Levin,
andersson, sre, linux-arm-msm, linux-pm
From: Konrad Dybcio <konrad.dybcio@linaro.org>
[ Upstream commit b43f7ddc2b7a5a90447d96cb4d3c6d142dd4a810 ]
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.
Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Tested-by: Luca Weiss <luca.weiss@fairphone.com>
Link: https://lore.kernel.org/r/20231218-topic-battmgr_fixture_attempt-v1-1-6145745f34fe@linaro.org
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
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,
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH AUTOSEL 6.7 15/39] power: supply: qcom_battmgr: Register the power supplies after PDR is up
2024-01-28 16:10 ` [PATCH AUTOSEL 6.7 15/39] power: supply: qcom_battmgr: Register the power supplies after PDR is up Sasha Levin
@ 2024-01-29 13:03 ` Sebastian Reichel
2024-01-30 23:01 ` Sasha Levin
0 siblings, 1 reply; 3+ messages in thread
From: Sebastian Reichel @ 2024-01-29 13:03 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-kernel, stable, Konrad Dybcio, Luca Weiss, andersson,
linux-arm-msm, linux-pm, Johan Hovold
[-- Attachment #1: Type: text/plain, Size: 7824 bytes --]
Hi,
On Sun, Jan 28, 2024 at 11:10:35AM -0500, Sasha Levin wrote:
> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>
> [ Upstream commit b43f7ddc2b7a5a90447d96cb4d3c6d142dd4a810 ]
>
> 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.
>
> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Tested-by: Luca Weiss <luca.weiss@fairphone.com>
> Link: https://lore.kernel.org/r/20231218-topic-battmgr_fixture_attempt-v1-1-6145745f34fe@linaro.org
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
Please drop it, I have a patch queued reverting this patch.
-- Sebastian
> 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,
> --
> 2.43.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH AUTOSEL 6.7 15/39] power: supply: qcom_battmgr: Register the power supplies after PDR is up
2024-01-29 13:03 ` Sebastian Reichel
@ 2024-01-30 23:01 ` Sasha Levin
0 siblings, 0 replies; 3+ messages in thread
From: Sasha Levin @ 2024-01-30 23:01 UTC (permalink / raw)
To: Sebastian Reichel
Cc: linux-kernel, stable, Konrad Dybcio, Luca Weiss, andersson,
linux-arm-msm, linux-pm, Johan Hovold
On Mon, Jan 29, 2024 at 02:03:14PM +0100, Sebastian Reichel wrote:
>Hi,
>
>On Sun, Jan 28, 2024 at 11:10:35AM -0500, Sasha Levin wrote:
>> From: Konrad Dybcio <konrad.dybcio@linaro.org>
>>
>> [ Upstream commit b43f7ddc2b7a5a90447d96cb4d3c6d142dd4a810 ]
>>
>> 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.
>>
>> Signed-off-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>> Tested-by: Luca Weiss <luca.weiss@fairphone.com>
>> Link: https://lore.kernel.org/r/20231218-topic-battmgr_fixture_attempt-v1-1-6145745f34fe@linaro.org
>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>> ---
>
>Please drop it, I have a patch queued reverting this patch.
Dropped, thanks!
--
Thanks,
Sasha
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-01-30 23:01 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20240128161130.200783-1-sashal@kernel.org>
2024-01-28 16:10 ` [PATCH AUTOSEL 6.7 15/39] power: supply: qcom_battmgr: Register the power supplies after PDR is up Sasha Levin
2024-01-29 13:03 ` Sebastian Reichel
2024-01-30 23:01 ` Sasha Levin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).