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.
