| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <f805c0d8-a7f7-4e03-8d8c-0c13baa02ac4@t-8ch.de>
Date: Fri, 22 Nov 2024 11:21:18 +0100
From: Thomas Weißschuh <thomas@...ch.de>
To: Sung-Chi Li <lschyi@...omium.org>
Cc: Benson Leung <bleung@...omium.org>,
Guenter Roeck <groeck@...omium.org>, Sebastian Reichel <sre@...nel.org>, Lee Jones <lee@...nel.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>, chrome-platform@...ts.linux.dev, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/2] power: supply: cros_usbpd-charger: extend as a
thermal of cooling device
On 2024-11-22 11:47:21+0800, Sung-Chi Li wrote:
> cros_usbpd-charger is the driver that takes care the system input power
> from the pd charger. This driver also exposes the functionality to limit
> input current.
>
> We can extend this driver to make it as a passive thermal cooling
> device by limiting the input current. As such, this commit implements
> the required cooling methods and OF style registration.
>
> Signed-off-by: Sung-Chi Li <lschyi@...omium.org>
> ---
> drivers/power/supply/cros_usbpd-charger.c | 98 +++++++++++++++++++++++++++++--
> 1 file changed, 93 insertions(+), 5 deletions(-)
A dependency from CHARGER_CROS_PCHG to THERMAL needs to be added to
drivers/power/supply/Kconfig.
>
> diff --git a/drivers/power/supply/cros_usbpd-charger.c b/drivers/power/supply/cros_usbpd-charger.c
> index 47d3f58aa15c..a0451630cdd7 100644
> --- a/drivers/power/supply/cros_usbpd-charger.c
> +++ b/drivers/power/supply/cros_usbpd-charger.c
> @@ -13,6 +13,9 @@
> #include <linux/platform_device.h>
> #include <linux/power_supply.h>
> #include <linux/slab.h>
> +#ifdef CONFIG_THERMAL_OF
Remove this ifdef. The header is perfectly usable in any case.
Actually the CONFIG_THERMAL_OF dependency is not needed at all.
It is only necessary for devm_thermal_of_zone_register() but not
devm_thermal_of_cooling_device_register() which you are using.
I am confused.
OTOH you are adding the #cooling-cells OF property which itself seems to
be only used by devm_thermal_of_zone_register(), so I'm now even more
confused.
In general, try to also test the driver configurations
!CONFIG_THERMAL_OF and !CONFIG_THERMAL.
> +#include <linux/thermal.h>
> +#endif /* CONFIG_THERMAL_OF */
>
> #define CHARGER_USBPD_DIR_NAME "CROS_USBPD_CHARGER%d"
> #define CHARGER_DEDICATED_DIR_NAME "CROS_DEDICATED_CHARGER"
> @@ -22,6 +25,7 @@
> sizeof(CHARGER_DEDICATED_DIR_NAME))
> #define CHARGER_CACHE_UPDATE_DELAY msecs_to_jiffies(500)
> #define CHARGER_MANUFACTURER_MODEL_LENGTH 32
> +#define CHARGER_COOLING_INTERVALS 10
>
> #define DRV_NAME "cros-usbpd-charger"
>
> @@ -76,6 +80,8 @@ static enum power_supply_property cros_usbpd_dedicated_charger_props[] = {
> /* Input voltage/current limit in mV/mA. Default to none. */
> static u16 input_voltage_limit = EC_POWER_LIMIT_NONE;
> static u16 input_current_limit = EC_POWER_LIMIT_NONE;
> +/* Cooling level interns of current limit */
> +static u16 input_current_cooling_level;
>
> static bool cros_usbpd_charger_port_is_dedicated(struct port_data *port)
> {
> @@ -459,13 +465,20 @@ static int cros_usbpd_charger_set_prop(struct power_supply *psy,
> break;
>
> input_current_limit = intval;
> - if (input_current_limit == EC_POWER_LIMIT_NONE)
> + if (input_current_limit == EC_POWER_LIMIT_NONE) {
> dev_info(dev,
> "External Current Limit cleared for all ports\n");
> - else
> - dev_info(dev,
> - "External Current Limit set to %dmA for all ports\n",
> - input_current_limit);
> + input_current_cooling_level = 0;
> + } else {
> + dev_info(
> + dev,
> + "External Current Limit set to %dmA for all ports\n",
> + input_current_limit);
> + input_current_cooling_level =
> + input_current_limit *
> + CHARGER_COOLING_INTERVALS /
> + port->psy_current_max;
This seems to be a very spammy driver...
> + }
> break;
> case POWER_SUPPLY_PROP_INPUT_VOLTAGE_LIMIT:
> ret = cros_usbpd_charger_set_ext_power_limit(charger,
> @@ -525,6 +538,66 @@ static void cros_usbpd_charger_unregister_notifier(void *data)
> cros_usbpd_unregister_notify(&charger->notifier);
> }
>
> +#ifdef CONFIG_THERMAL_OF
> +static int
> +cros_usbpd_charger_get_max_cooling_state(struct thermal_cooling_device *cdev,
> + unsigned long *cooling_level)
> +{
> + *cooling_level = CHARGER_COOLING_INTERVALS;
> + return 0;
> +}
> +
> +static int
> +cros_usbpd_charger_get_cur_cooling_state(struct thermal_cooling_device *cdev,
> + unsigned long *cooling_level)
> +{
> + *cooling_level = input_current_cooling_level;
> + return 0;
> +}
> +
> +static int
> +cros_usbpd_charger_set_cur_cooling_state(struct thermal_cooling_device *cdev,
> + unsigned long cooling_level)
> +{
> + struct charger_data *charger = cdev->devdata;
> + struct port_data *port;
> + int current_limit;
> + int idx = -1;
> + int ret;
> +
> + for (int i = 0; i < charger->num_registered_psy; i++) {
> + port = charger->ports[i];
> + if (port->psy_status == POWER_SUPPLY_STATUS_CHARGING) {
> + idx = i;
> + break;
> + }
> + }
Why not register one cooling device per charger?
It would make things more predictable.
I have no experience with the thermal subsystem, so this is just a
guess.
> +
> + if (idx == -1)
> + return -EINVAL;
> +
> + current_limit =
> + port->psy_current_max - (cooling_level * port->psy_current_max /
> + CHARGER_COOLING_INTERVALS);
> + ret = cros_usbpd_charger_set_ext_power_limit(charger, current_limit,
> + input_voltage_limit);
> + if (ret < 0)
> + return ret;
> +
> + input_current_limit = (current_limit == port->psy_current_max) ?
> + EC_POWER_LIMIT_NONE :
> + current_limit;
> + input_current_cooling_level = cooling_level;
> + return 0;
> +}
> +
> +static struct thermal_cooling_device_ops cros_usbpd_charger_cooling_ops = {
const
> + .get_max_state = cros_usbpd_charger_get_max_cooling_state,
> + .get_cur_state = cros_usbpd_charger_get_cur_cooling_state,
> + .set_cur_state = cros_usbpd_charger_set_cur_cooling_state,
> +};
> +#endif /* CONFIG_THERMAL_OF */
> +
> static int cros_usbpd_charger_probe(struct platform_device *pd)
> {
> struct cros_ec_dev *ec_dev = dev_get_drvdata(pd->dev.parent);
> @@ -534,6 +607,9 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
> struct charger_data *charger;
> struct power_supply *psy;
> struct port_data *port;
> +#ifdef CONFIG_THERMAL_OF
> + struct thermal_cooling_device *cdev;
> +#endif /* CONFIG_THERMAL_OF */
> int ret = -EINVAL;
> int i;
>
> @@ -674,6 +750,18 @@ static int cros_usbpd_charger_probe(struct platform_device *pd)
> goto fail;
> }
>
> +#ifdef CONFIG_THERMAL_OF
Avoid ifdef in .c files.
Use if (IS_ENABLED(CONFIG_THERMAL_OF)) in the normal code flow.
The compiler will optimize away all the unreachable code.
> + cdev = devm_thermal_of_cooling_device_register(
> + dev, ec_device->dev->of_node, DRV_NAME, charger,
> + &cros_usbpd_charger_cooling_ops);
> + if (IS_ERR(cdev)) {
> + dev_err(dev,
> + "Failing register thermal cooling device (err:%pe)\n",
> + cdev);
dev_err_probe().
> + goto fail;
Does the call to devm_thermal_of_cooling_device_register() work if there
is no OF configuration?
> + }
> +#endif /* CONFIG_THERMAL_OF */
> +
> return 0;
>
> fail:
>
> --
> 2.47.0.371.ga323438b13-goog
>
Powered by blists - more mailing lists