[PATCH 0/2] Extend the cros_usbpd-charger to make it a passive thermal cooling device

[PATCH 0/2] Extend the cros_usbpd-charger to make it a passive thermal cooling device

[Sung-Chi Li]

The cros_usbpd-charger already supports limiting input current, so we
can easily extend it as a passive thermal cooling device. By limiting
the current flow into the system, we can reduce the heat generated by
related chips, which results in reserving more thermal budget for the
system.

This series will only works on making it a OF style thermal cooling
device, so related code are guarded by #ifdef macros.

---
Sung-Chi Li (2):
      power: supply: cros_usbpd-charger: extend as a thermal of cooling device
      dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

 .../devicetree/bindings/mfd/google,cros-ec.yaml    |  3 +
 drivers/power/supply/cros_usbpd-charger.c          | 98 ++++++++++++++++++++--
 2 files changed, 96 insertions(+), 5 deletions(-)
---
base-commit: ac24e26aa08fe026804f678599f805eb13374a5d
change-id: 20241122-extend_power_limit-62c2d9aabfa7

Best regards,
-- 
Sung-Chi Li <lschyi@chromium.org>

[PATCH 1/2] power: supply: cros_usbpd-charger: extend as a thermal of cooling device

[Sung-Chi Li]

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(-)

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:

-- 
2.47.0.371.ga323438b13-goog

[PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Sung-Chi Li]

The cros_ec supports limiting the input current to act as a passive
thermal cooling device. Add the property '#cooling-cells' bindings, such
that thermal framework can recognize cros_ec as a valid thermal cooling
device.

Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
---
 Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
index aac8819bd00b..2b6f098057af 100644
--- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
+++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
@@ -96,6 +96,9 @@ properties:
   '#gpio-cells':
     const: 2
 
+  '#cooling-cells':
+    const: 2
+
   gpio-controller: true
 
   typec:

-- 
2.47.0.371.ga323438b13-goog

Re: [PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Krzysztof Kozlowski]

On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote:
> The cros_ec supports limiting the input current to act as a passive
> thermal cooling device. Add the property '#cooling-cells' bindings, such
> that thermal framework can recognize cros_ec as a valid thermal cooling
> device.
> 
> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> ---
>  Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> index aac8819bd00b..2b6f098057af 100644
> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> @@ -96,6 +96,9 @@ properties:
>    '#gpio-cells':
>      const: 2
>  
> +  '#cooling-cells':
> +    const: 2

This is not a cooling device. BTW, your commit msg is somehow circular.
"Add cooling to make it a cooling device because it will be then cooling
device."

Power supply already provides necessary framework for managing charging
current and temperatures. If this is to stay, you need to explain why
this is suitable to be considered a thermal zone or system cooling
device (not power supply or input power cooling).

Best regards,
Krzysztof

Re: [PATCH 1/2] power: supply: cros_usbpd-charger: extend as a thermal of cooling device

[Thomas Weißschuh]

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
> 

Re: [PATCH 1/2] power: supply: cros_usbpd-charger: extend as a thermal of cooling device

[Sung-Chi, Li]

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.

Re: [PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Sung-Chi, Li]

On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote:
> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote:
> > The cros_ec supports limiting the input current to act as a passive
> > thermal cooling device. Add the property '#cooling-cells' bindings, such
> > that thermal framework can recognize cros_ec as a valid thermal cooling
> > device.
> > 
> > Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> > ---
> >  Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> > index aac8819bd00b..2b6f098057af 100644
> > --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> > +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> > @@ -96,6 +96,9 @@ properties:
> >    '#gpio-cells':
> >      const: 2
> >  
> > +  '#cooling-cells':
> > +    const: 2
> 
> This is not a cooling device. BTW, your commit msg is somehow circular.
> "Add cooling to make it a cooling device because it will be then cooling
> device."
> 
> Power supply already provides necessary framework for managing charging
> current and temperatures. If this is to stay, you need to explain why
> this is suitable to be considered a thermal zone or system cooling
> device (not power supply or input power cooling).
> 
> Best regards,
> Krzysztof
> 

Thank you, I will rephrase the commit message. The reason to not to use the
managing charging current and temperatures in the power supply framework is
that:

- The EC may not have the thermal sensor value for the charger, and there is no
  protocol for getting the thermal sensor value for the charger (there is
  command for reading thermal sensor values, but there is no specification for
  what sensor index is for the charger, if the charger provides thermal value).
- The managing mechanism only take the charger thermal value into account, and
  I would like to control the current based on the thermal condition of the
  whole device.

I will include these explanation in the following changes.

Re: [PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Krzysztof Kozlowski]

On 25/11/2024 03:50, Sung-Chi, Li wrote:
> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote:
>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote:
>>> The cros_ec supports limiting the input current to act as a passive
>>> thermal cooling device. Add the property '#cooling-cells' bindings, such
>>> that thermal framework can recognize cros_ec as a valid thermal cooling
>>> device.
>>>
>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
>>> ---
>>>  Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>> index aac8819bd00b..2b6f098057af 100644
>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>> @@ -96,6 +96,9 @@ properties:
>>>    '#gpio-cells':
>>>      const: 2
>>>  
>>> +  '#cooling-cells':
>>> +    const: 2
>>
>> This is not a cooling device. BTW, your commit msg is somehow circular.
>> "Add cooling to make it a cooling device because it will be then cooling
>> device."
>>
>> Power supply already provides necessary framework for managing charging
>> current and temperatures. If this is to stay, you need to explain why
>> this is suitable to be considered a thermal zone or system cooling
>> device (not power supply or input power cooling).
>>
>> Best regards,
>> Krzysztof
>>
> 
> Thank you, I will rephrase the commit message. The reason to not to use the
> managing charging current and temperatures in the power supply framework is
> that:
> 
> - The EC may not have the thermal sensor value for the charger, and there is no
>   protocol for getting the thermal sensor value for the charger (there is
>   command for reading thermal sensor values, but there is no specification for
>   what sensor index is for the charger, if the charger provides thermal value).
> - The managing mechanism only take the charger thermal value into account, and
>   I would like to control the current based on the thermal condition of the
>   whole device.
> 
> I will include these explanation in the following changes.


This does not explain me why this is supposed to be thermal zone. I
already said it, but let's repeat: This is not a thermal zone. This
isn't thermal zone sensor, either.


Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Krzysztof Kozlowski]

On 25/11/2024 08:32, Krzysztof Kozlowski wrote:
> On 25/11/2024 03:50, Sung-Chi, Li wrote:
>> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote:
>>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote:
>>>> The cros_ec supports limiting the input current to act as a passive
>>>> thermal cooling device. Add the property '#cooling-cells' bindings, such
>>>> that thermal framework can recognize cros_ec as a valid thermal cooling
>>>> device.
>>>>
>>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
>>>> ---
>>>>  Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
>>>>  1 file changed, 3 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>> index aac8819bd00b..2b6f098057af 100644
>>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>> @@ -96,6 +96,9 @@ properties:
>>>>    '#gpio-cells':
>>>>      const: 2
>>>>  
>>>> +  '#cooling-cells':
>>>> +    const: 2
>>>
>>> This is not a cooling device. BTW, your commit msg is somehow circular.


^^^ And here which you ignored: this is not a cooling device.

>>> "Add cooling to make it a cooling device because it will be then cooling
>>> device."
>>>
>>> Power supply already provides necessary framework for managing charging
>>> current and temperatures. If this is to stay, you need to explain why
>>> this is suitable to be considered a thermal zone or system cooling
>>> device (not power supply or input power cooling).
>>>
>>> Best regards,
>>> Krzysztof
>>>
>>
>> Thank you, I will rephrase the commit message. The reason to not to use the
>> managing charging current and temperatures in the power supply framework is
>> that:
>>
>> - The EC may not have the thermal sensor value for the charger, and there is no
>>   protocol for getting the thermal sensor value for the charger (there is
>>   command for reading thermal sensor values, but there is no specification for
>>   what sensor index is for the charger, if the charger provides thermal value).
>> - The managing mechanism only take the charger thermal value into account, and
>>   I would like to control the current based on the thermal condition of the
>>   whole device.
>>
>> I will include these explanation in the following changes.
> 
> 
> This does not explain me why this is supposed to be thermal zone. I
> already said it, but let's repeat: This is not a thermal zone. This
> isn't thermal zone sensor, either.


And nothing from your "revised" commit msg explains why something which
is not a cooling device is supposed to be a cooling device.


Best regards,
Krzysztof

Re: [PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Sung-Chi, Li]

On Mon, Nov 25, 2024 at 08:35:01AM +0100, Krzysztof Kozlowski wrote:
> On 25/11/2024 08:32, Krzysztof Kozlowski wrote:
> > On 25/11/2024 03:50, Sung-Chi, Li wrote:
> >> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote:
> >>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote:
> >>>> The cros_ec supports limiting the input current to act as a passive
> >>>> thermal cooling device. Add the property '#cooling-cells' bindings, such
> >>>> that thermal framework can recognize cros_ec as a valid thermal cooling
> >>>> device.
> >>>>
> >>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> >>>> ---
> >>>>  Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
> >>>>  1 file changed, 3 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> >>>> index aac8819bd00b..2b6f098057af 100644
> >>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> >>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> >>>> @@ -96,6 +96,9 @@ properties:
> >>>>    '#gpio-cells':
> >>>>      const: 2
> >>>>  
> >>>> +  '#cooling-cells':
> >>>> +    const: 2
> >>>
> >>> This is not a cooling device. BTW, your commit msg is somehow circular.
> 
> 
> ^^^ And here which you ignored: this is not a cooling device.
> 

Hi, I added the explanation in the commit message in the v2 version. Please have
a look, it should explains why it is not a cooling device.

> >>> "Add cooling to make it a cooling device because it will be then cooling
> >>> device."
> >>>
> >>> Power supply already provides necessary framework for managing charging
> >>> current and temperatures. If this is to stay, you need to explain why
> >>> this is suitable to be considered a thermal zone or system cooling
> >>> device (not power supply or input power cooling).
> >>>
> >>> Best regards,
> >>> Krzysztof
> >>>
> >>
> >> Thank you, I will rephrase the commit message. The reason to not to use the
> >> managing charging current and temperatures in the power supply framework is
> >> that:
> >>
> >> - The EC may not have the thermal sensor value for the charger, and there is no
> >>   protocol for getting the thermal sensor value for the charger (there is
> >>   command for reading thermal sensor values, but there is no specification for
> >>   what sensor index is for the charger, if the charger provides thermal value).
> >> - The managing mechanism only take the charger thermal value into account, and
> >>   I would like to control the current based on the thermal condition of the
> >>   whole device.
> >>
> >> I will include these explanation in the following changes.
> > 
> > 
> > This does not explain me why this is supposed to be thermal zone. I
> > already said it, but let's repeat: This is not a thermal zone. This
> > isn't thermal zone sensor, either.
> 
> 
> And nothing from your "revised" commit msg explains why something which
> is not a cooling device is supposed to be a cooling device.

The revised commit message is sent, please have a look.

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Sung-Chi, Li]

On Mon, Nov 25, 2024 at 08:32:19AM +0100, Krzysztof Kozlowski wrote:
> On 25/11/2024 03:50, Sung-Chi, Li wrote:
> > On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote:
> >> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote:
> >>> The cros_ec supports limiting the input current to act as a passive
> >>> thermal cooling device. Add the property '#cooling-cells' bindings, such
> >>> that thermal framework can recognize cros_ec as a valid thermal cooling
> >>> device.
> >>>
> >>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
> >>> ---
> >>>  Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> >>> index aac8819bd00b..2b6f098057af 100644
> >>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> >>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
> >>> @@ -96,6 +96,9 @@ properties:
> >>>    '#gpio-cells':
> >>>      const: 2
> >>>  
> >>> +  '#cooling-cells':
> >>> +    const: 2
> >>
> >> This is not a cooling device. BTW, your commit msg is somehow circular.
> >> "Add cooling to make it a cooling device because it will be then cooling
> >> device."
> >>
> >> Power supply already provides necessary framework for managing charging
> >> current and temperatures. If this is to stay, you need to explain why
> >> this is suitable to be considered a thermal zone or system cooling
> >> device (not power supply or input power cooling).
> >>
> >> Best regards,
> >> Krzysztof
> >>
> > 
> > Thank you, I will rephrase the commit message. The reason to not to use the
> > managing charging current and temperatures in the power supply framework is
> > that:
> > 
> > - The EC may not have the thermal sensor value for the charger, and there is no
> >   protocol for getting the thermal sensor value for the charger (there is
> >   command for reading thermal sensor values, but there is no specification for
> >   what sensor index is for the charger, if the charger provides thermal value).
> > - The managing mechanism only take the charger thermal value into account, and
> >   I would like to control the current based on the thermal condition of the
> >   whole device.
> > 
> > I will include these explanation in the following changes.
> 
> 
> This does not explain me why this is supposed to be thermal zone. I
> already said it, but let's repeat: This is not a thermal zone. This
> isn't thermal zone sensor, either.

Hi, I added the explanation in the commit message in v2, in short, I need to use
different thermal sensors, and need finer thermal controls, so I have to use
thermal zone. This is included in the v2 commit message.

> 
> 
> Best regards,
> Krzysztof

Re: [PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Krzysztof Kozlowski]

On 25/11/2024 09:43, Sung-Chi, Li wrote:
> On Mon, Nov 25, 2024 at 08:32:19AM +0100, Krzysztof Kozlowski wrote:
>> On 25/11/2024 03:50, Sung-Chi, Li wrote:
>>> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote:
>>>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote:
>>>>> The cros_ec supports limiting the input current to act as a passive
>>>>> thermal cooling device. Add the property '#cooling-cells' bindings, such
>>>>> that thermal framework can recognize cros_ec as a valid thermal cooling
>>>>> device.
>>>>>
>>>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
>>>>> ---
>>>>>  Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
>>>>>  1 file changed, 3 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>>> index aac8819bd00b..2b6f098057af 100644
>>>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>>> @@ -96,6 +96,9 @@ properties:
>>>>>    '#gpio-cells':
>>>>>      const: 2
>>>>>  
>>>>> +  '#cooling-cells':
>>>>> +    const: 2
>>>>
>>>> This is not a cooling device. BTW, your commit msg is somehow circular.
>>>> "Add cooling to make it a cooling device because it will be then cooling
>>>> device."
>>>>
>>>> Power supply already provides necessary framework for managing charging
>>>> current and temperatures. If this is to stay, you need to explain why
>>>> this is suitable to be considered a thermal zone or system cooling
>>>> device (not power supply or input power cooling).
>>>>
>>>> Best regards,
>>>> Krzysztof
>>>>
>>>
>>> Thank you, I will rephrase the commit message. The reason to not to use the
>>> managing charging current and temperatures in the power supply framework is
>>> that:
>>>
>>> - The EC may not have the thermal sensor value for the charger, and there is no
>>>   protocol for getting the thermal sensor value for the charger (there is
>>>   command for reading thermal sensor values, but there is no specification for
>>>   what sensor index is for the charger, if the charger provides thermal value).
>>> - The managing mechanism only take the charger thermal value into account, and
>>>   I would like to control the current based on the thermal condition of the
>>>   whole device.
>>>
>>> I will include these explanation in the following changes.
>>
>>
>> This does not explain me why this is supposed to be thermal zone. I
>> already said it, but let's repeat: This is not a thermal zone. This
>> isn't thermal zone sensor, either.
> 
> Hi, I added the explanation in the commit message in v2, in short, I need to use
> different thermal sensors, and need finer thermal controls, so I have to use
> thermal zone. This is included in the v2 commit message.
You resolved nothing there. I don't care that "you need to use thermal
sensors". That's not a valid reason. If next time you say "I need to
make it a current regulator", shall we accept incorrect description? No.

I repeat multiple times: this is not a SoC cooling device, this is not a
thermal zone and not a thermal sensor.

This is a power supply or charger or battery. Eventually it might be
hardware monitoring sensor. Use appropriate properties for this category
of device.

Best regards,
Krzysztof

Re: [PATCH 1/2] power: supply: cros_usbpd-charger: extend as a thermal of cooling device

[Thomas Weißschuh]

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.

Re: [PATCH 2/2] dt-bindings: mfd: cros-ec: add properties for thermal cooling cells

[Krzysztof Kozlowski]

On 25/11/2024 09:48, Krzysztof Kozlowski wrote:
> On 25/11/2024 09:43, Sung-Chi, Li wrote:
>> On Mon, Nov 25, 2024 at 08:32:19AM +0100, Krzysztof Kozlowski wrote:
>>> On 25/11/2024 03:50, Sung-Chi, Li wrote:
>>>> On Fri, Nov 22, 2024 at 08:49:14AM +0100, Krzysztof Kozlowski wrote:
>>>>> On Fri, Nov 22, 2024 at 11:47:22AM +0800, Sung-Chi Li wrote:
>>>>>> The cros_ec supports limiting the input current to act as a passive
>>>>>> thermal cooling device. Add the property '#cooling-cells' bindings, such
>>>>>> that thermal framework can recognize cros_ec as a valid thermal cooling
>>>>>> device.
>>>>>>
>>>>>> Signed-off-by: Sung-Chi Li <lschyi@chromium.org>
>>>>>> ---
>>>>>>  Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
>>>>>>  1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>>>> index aac8819bd00b..2b6f098057af 100644
>>>>>> --- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>>>> +++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
>>>>>> @@ -96,6 +96,9 @@ properties:
>>>>>>    '#gpio-cells':
>>>>>>      const: 2
>>>>>>  
>>>>>> +  '#cooling-cells':
>>>>>> +    const: 2
>>>>>
>>>>> This is not a cooling device. BTW, your commit msg is somehow circular.
>>>>> "Add cooling to make it a cooling device because it will be then cooling
>>>>> device."
>>>>>
>>>>> Power supply already provides necessary framework for managing charging
>>>>> current and temperatures. If this is to stay, you need to explain why
>>>>> this is suitable to be considered a thermal zone or system cooling
>>>>> device (not power supply or input power cooling).
>>>>>
>>>>> Best regards,
>>>>> Krzysztof
>>>>>
>>>>
>>>> Thank you, I will rephrase the commit message. The reason to not to use the
>>>> managing charging current and temperatures in the power supply framework is
>>>> that:
>>>>
>>>> - The EC may not have the thermal sensor value for the charger, and there is no
>>>>   protocol for getting the thermal sensor value for the charger (there is
>>>>   command for reading thermal sensor values, but there is no specification for
>>>>   what sensor index is for the charger, if the charger provides thermal value).
>>>> - The managing mechanism only take the charger thermal value into account, and
>>>>   I would like to control the current based on the thermal condition of the
>>>>   whole device.
>>>>
>>>> I will include these explanation in the following changes.
>>>
>>>
>>> This does not explain me why this is supposed to be thermal zone. I
>>> already said it, but let's repeat: This is not a thermal zone. This
>>> isn't thermal zone sensor, either.
>>
>> Hi, I added the explanation in the commit message in v2, in short, I need to use
>> different thermal sensors, and need finer thermal controls, so I have to use
>> thermal zone. This is included in the v2 commit message.
> You resolved nothing there. I don't care that "you need to use thermal
> sensors". That's not a valid reason. If next time you say "I need to
> make it a current regulator", shall we accept incorrect description? No.
> 
> I repeat multiple times: this is not a SoC cooling device, this is not a
> thermal zone and not a thermal sensor.
> 
> This is a power supply or charger or battery. Eventually it might be
> hardware monitoring sensor. Use appropriate properties for this category
> of device.

One more comment, inspired by re-thinking this why watching grey heron
nearby: you sent first binding patch for thermal-sensor-cells, then
later for cooling. Sorry, that's wrong process: you are supposed to send
complete bindings for your hardware. Sending it piece by piece is a
proof you do it for the driver, so again completely wrong
intentions/rationale of making DT changes. It also hides from us
complete picture of the hardware and makes decision difficult.

Another reason for not accepting this and your previous bindings
contribution.

To recap: send *complete* bindings for the hardware.
Best regards,
Krzysztof