| Message ID | 20241122-extend_power_limit-v1-1-a3ecd87afa76@chromium.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Extend the cros_usbpd-charger to make it a passive thermal cooling device | expand |
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@chromium.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 >
On Fri, Nov 22, 2024 at 11:21:18AM +0100, Thomas Weißschuh wrote: > On 2024-11-22 11:47:21+0800, Sung-Chi Li wrote: > > > > 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. > Thank you, I removed the ifdef. Yes, it is confusing that devm_thermal_of_cooling_device_register() does not depend on CONFIG_THERMAL_OF. You can supply NULL to the device_node to devm_thermal_of_cooling_device_register(), and if you are going the OF route, you then fail at devm_thermal_of_zone_register(), because that call requires the supplied device_node to have property '#cooling-cells'. I would like to split the handling on thermal side to OF route and non-OF route, so I would use CONFIG_THERMAL_OF to decide which route to go. > > +#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, s ap> > 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... > Hmm, I did not add extra logs, just that I add more actions in these branches when the current limit is applied, so the clang format tool touches these lines. I think I can revert the formatting changes, and maybe I can make these logs to dev_dbg in a following commit. > > + } > > 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. > The driver has only one power limiting instance, so I treat the whole EC as a cooling device. This is also more convenient when crafting the thermal zone settings. Maybe we can see how other reviewers think? > > + .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. > Thank you, applied this approach when using CONFIG_THERMAL_OF. > > + 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 > > As the thermal functionality is later added to extend this driver, I think you are right, it would be better to make this behavior just make warnings, rather than directly failing this driver probe. Will use dev_warn_probe, and do not goto fail branch for registering it as a cooling device.
On 2024-11-25 10:37:47+0800, Sung-Chi, Li wrote: > On Fri, Nov 22, 2024 at 11:21:18AM +0100, Thomas Weißschuh wrote: > > On 2024-11-22 11:47:21+0800, Sung-Chi Li wrote: > > > > > > 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. > > > > Thank you, I removed the ifdef. Yes, it is confusing that > devm_thermal_of_cooling_device_register() does not depend on CONFIG_THERMAL_OF. > You can supply NULL to the device_node to > devm_thermal_of_cooling_device_register(), and if you are going the OF route, > you then fail at devm_thermal_of_zone_register(), because that call requires the > supplied device_node to have property '#cooling-cells'. > > I would like to split the handling on thermal side to OF route and non-OF route, > so I would use CONFIG_THERMAL_OF to decide which route to go. Thanks for the clarifications and thinking about this usecase. > > > +#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, > s ap> > 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... > > > > Hmm, I did not add extra logs, just that I add more actions in these branches > when the current limit is applied, so the clang format tool touches these lines. > > I think I can revert the formatting changes, and maybe I can make these logs to > dev_dbg in a following commit. This wasn't a real review comment for your patch, only a general observation. Maybe it's worth to have a dedicated commit trimming down the dev_info() to dev_dbg() and reducing the spam during probe (failures). > > > > + } > > > 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. > > > > The driver has only one power limiting instance, so I treat the whole EC as a > cooling device. This is also more convenient when crafting the thermal zone > settings. Maybe we can see how other reviewers think? Yes, I am don't know much about the thermal subsystem. > > > + .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. > > > > Thank you, applied this approach when using CONFIG_THERMAL_OF. > > > > + 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 > > > > > As the thermal functionality is later added to extend this driver, I think you > are right, it would be better to make this behavior just make warnings, rather > than directly failing this driver probe. Will use dev_warn_probe, and do not > goto fail branch for registering it as a cooling device. Can it also be an info? There should be existing devices in the field without the OF configuration which will run your mainline driver.
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 +#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; + } 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; + } + } + + 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 = { + .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 + 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); + goto fail; + } +#endif /* CONFIG_THERMAL_OF */ + return 0; fail:
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@chromium.org> --- drivers/power/supply/cros_usbpd-charger.c | 98 +++++++++++++++++++++++++++++-- 1 file changed, 93 insertions(+), 5 deletions(-)