[PATCH] power: supply: cros_charge-control: adopt EC charge state on probe

[PATCH] power: supply: cros_charge-control: adopt EC charge state on probe

[Matt DeVillier]

The driver previously always forced AUTO with no charge limits at
probe, which discarded sustainer thresholds and modes set by firmware
or firmware setup before the kernel loaded.

For command versions that support GET (v2+), read the EC state into
the driver cache instead. Valid sustainer limits are adopted as AUTO
with those thresholds: while the sustainer is active the EC may report
IDLE or DISCHARGE as a transient hold/discharge step, which must not
be exposed as inhibit-charge or force-discharge. Sustainer off
(-1/-1) still maps to Linux "no limit" (0/100); other invalid limit
pairs are remapped the same way with a warning.

If GET fails, fall back to the previous defaults and SET them on the
EC. Command version 1 still cannot report state and keeps forcing a
well-known configuration.

Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
---
 drivers/power/supply/cros_charge-control.c | 159 ++++++++++++++++++---
 1 file changed, 142 insertions(+), 17 deletions(-)

diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
index e0f168624807..fd620317cfba 100644
--- a/drivers/power/supply/cros_charge-control.c
+++ b/drivers/power/supply/cros_charge-control.c
@@ -21,12 +21,22 @@
 
 /*
  * Semantics of data *returned* from the EC API and Linux sysfs differ
- * slightly, also the v1 API can not return any data.
- * To match the expected sysfs API, data is never read back from the EC but
- * cached in the driver.
+ * slightly:
+ *  - EC sustainer off is lower=upper=-1
+ *  - Linux "no limit" is start=0, end=100
+ * Also the v1 API can not return any data.
  *
- * Changes to the EC bypassing the driver will not be reflected in sysfs.
- * Any change to "charge_behaviour" will synchronize the EC with the driver state.
+ * While the sustainer is active the EC may report IDLE or DISCHARGE as the
+ * current charge-control mode; that is an internal hold/discharge step, not a
+ * Linux inhibit-charge / force-discharge request. Valid sustainer limits are
+ * therefore adopted as AUTO with the reported thresholds.
+ *
+ * Sysfs reads come from a driver-side cache. On probe, command versions that
+ * support GET (v2+) are initialized from the EC so firmware or firmware-setup
+ * programmed limits and modes are preserved (unlike earlier behaviour that
+ * always forced AUTO with no limits). v1 still forces a well-known EC state.
+ * Subsequent sysfs writes keep the cache and EC in sync; changes that bypass
+ * the driver are not reflected until the next probe.
  */
 
 struct cros_chctl_priv {
@@ -44,18 +54,20 @@ struct cros_chctl_priv {
 };
 
 static int cros_chctl_send_charge_control_cmd(struct cros_ec_device *cros_ec,
-					      u8 cmd_version, struct ec_params_charge_control *req)
+					      u8 cmd_version,
+					      struct ec_params_charge_control *req,
+					      struct ec_response_charge_control *resp)
 {
-	int ret;
 	static const u8 outsizes[] = {
 		[1] = offsetof(struct ec_params_charge_control, cmd),
 		[2] = sizeof(struct ec_params_charge_control),
 		[3] = sizeof(struct ec_params_charge_control),
 	};
+	size_t insize = resp ? sizeof(*resp) : 0;
+	int ret;
 
 	ret = cros_ec_cmd(cros_ec, cmd_version, EC_CMD_CHARGE_CONTROL, req,
-			  outsizes[cmd_version], NULL, 0);
-
+			  outsizes[cmd_version], resp, insize);
 	if (ret < 0)
 		return ret;
 
@@ -94,7 +106,126 @@ static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
 		req.sustain_soc.upper = -1;
 	}
 
-	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
+	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
+						  &req, NULL);
+}
+
+static int cros_chctl_get_ec_status(struct cros_chctl_priv *priv,
+				    struct ec_response_charge_control *resp)
+{
+	struct ec_params_charge_control req = {
+		.cmd = EC_CHARGE_CONTROL_CMD_GET,
+	};
+
+	lockdep_assert_held(&priv->lock);
+
+	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
+						  &req, resp);
+}
+
+static void cros_chctl_set_default_state(struct cros_chctl_priv *priv)
+{
+	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+	priv->current_start_threshold = 0;
+	priv->current_end_threshold = 100;
+}
+
+static bool cros_chctl_sustainer_limits_valid(s8 lower, s8 upper)
+{
+	return lower >= 0 && upper >= 0 && lower <= 100 && upper <= 100 &&
+	       lower <= upper;
+}
+
+static int cros_chctl_adopt_ec_mode(struct cros_chctl_priv *priv, u32 mode)
+{
+	switch (mode) {
+	case CHARGE_CONTROL_NORMAL:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+		return 0;
+	case CHARGE_CONTROL_IDLE:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
+		return 0;
+	case CHARGE_CONTROL_DISCHARGE:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+		return 0;
+	default:
+		dev_warn(priv->dev, "unknown charge control mode %u\n", mode);
+		return -EINVAL;
+	}
+}
+
+static int cros_chctl_adopt_ec_state(struct cros_chctl_priv *priv)
+{
+	struct ec_response_charge_control resp = {};
+	s8 lower, upper;
+	int ret;
+
+	lockdep_assert_held(&priv->lock);
+
+	ret = cros_chctl_get_ec_status(priv, &resp);
+	if (ret < 0)
+		return ret;
+
+	lower = resp.sustain_soc.lower;
+	upper = resp.sustain_soc.upper;
+
+	/*
+	 * Valid sustainer limits mean "auto with thresholds". The EC mode may
+	 * be IDLE/DISCHARGE while the sustainer holds or bleeds SoC; do not
+	 * expose that as inhibit-charge / force-discharge.
+	 */
+	if (cros_chctl_sustainer_limits_valid(lower, upper)) {
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+		priv->current_start_threshold = lower;
+		priv->current_end_threshold = upper;
+	} else {
+		ret = cros_chctl_adopt_ec_mode(priv, resp.mode);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * Sustainer off is lower=upper=-1 → Linux "no limit" (0/100).
+		 * Any other non-valid pair is unexpected; remap and warn.
+		 */
+		if (!(lower == -1 && upper == -1))
+			dev_warn(priv->dev,
+				 "invalid EC sustainer limits (%d/%d), treating as no limit\n",
+				 lower, upper);
+
+		priv->current_start_threshold = 0;
+		priv->current_end_threshold = 100;
+	}
+
+	dev_dbg(priv->dev,
+		"adopted EC charge state: behaviour=%d start=%u end=%u (ec mode=%u)\n",
+		priv->current_behaviour, priv->current_start_threshold,
+		priv->current_end_threshold, resp.mode);
+
+	return 0;
+}
+
+static int cros_chctl_init_state(struct cros_chctl_priv *priv)
+{
+	int ret;
+
+	lockdep_assert_held(&priv->lock);
+
+	cros_chctl_set_default_state(priv);
+
+	/* v1 cannot report current state; force a well-known EC configuration. */
+	if (priv->cmd_version < 2)
+		return cros_chctl_configure_ec(priv);
+
+	ret = cros_chctl_adopt_ec_state(priv);
+	if (ret < 0) {
+		dev_warn(priv->dev,
+			 "failed to read EC charge state (%d), applying defaults\n",
+			 ret);
+		cros_chctl_set_default_state(priv);
+		return cros_chctl_configure_ec(priv);
+	}
+
+	return 0;
 }
 
 static int cros_chctl_psy_ext_get_prop(struct power_supply *psy,
@@ -152,7 +283,6 @@ static int cros_chctl_psy_ext_set_threshold(struct cros_chctl_priv *priv,
 	return 0;
 }
 
-
 static int cros_chctl_psy_ext_set_prop(struct power_supply *psy,
 				       const struct power_supply_ext *ext,
 				       void *data,
@@ -305,13 +435,8 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	priv->battery_hook.add_battery = cros_chctl_add_battery;
 	priv->battery_hook.remove_battery = cros_chctl_remove_battery;
 
-	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
-	priv->current_start_threshold = 0;
-	priv->current_end_threshold = 100;
-
-	/* Bring EC into well-known state */
 	scoped_guard(mutex, &priv->lock)
-		ret = cros_chctl_configure_ec(priv);
+		ret = cros_chctl_init_state(priv);
 	if (ret < 0)
 		return ret;
 
-- 
2.53.0

Re: [PATCH] power: supply: cros_charge-control: adopt EC charge state on probe

[Tzung-Bi Shih]

On Mon, Aug 24, 2026 at 10:57:18AM -0500, Matt DeVillier wrote:
> +static int cros_chctl_get_ec_status(struct cros_chctl_priv *priv,
> +				    struct ec_response_charge_control *resp)
> +{
> +	struct ec_params_charge_control req = {
> +		.cmd = EC_CHARGE_CONTROL_CMD_GET,
> +	};
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
> +						  &req, resp);
> +}

This lockdep_assert_held() is unnecessary.  While it might appear that the
lock is needed here to protect concurrent command transmissions through
the struct cros_ec_device, the underlying EC communication framework
already handles its own locking for that.

> +static void cros_chctl_set_default_state(struct cros_chctl_priv *priv)
> +{
> +	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +	priv->current_start_threshold = 0;
> +	priv->current_end_threshold = 100;
> +}

Please add a lockdep_assert_held().  This function modifies the core driver
state fields that the lock is explicitly designed to protect.

> +static int cros_chctl_adopt_ec_mode(struct cros_chctl_priv *priv, u32 mode)
> +{
> +	switch (mode) {
> +	case CHARGE_CONTROL_NORMAL:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +		return 0;
> +	case CHARGE_CONTROL_IDLE:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
> +		return 0;
> +	case CHARGE_CONTROL_DISCHARGE:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
> +		return 0;
> +	default:
> +		dev_warn(priv->dev, "unknown charge control mode %u\n", mode);
> +		return -EINVAL;
> +	}
> +}

Same here, please add a lockdep_assert_held().

> +static int cros_chctl_init_state(struct cros_chctl_priv *priv)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	cros_chctl_set_default_state(priv);
> +
> +	/* v1 cannot report current state; force a well-known EC configuration. */
> +	if (priv->cmd_version < 2)
> +		return cros_chctl_configure_ec(priv);
> +
> +	ret = cros_chctl_adopt_ec_state(priv);
> +	if (ret < 0) {
> +		dev_warn(priv->dev,
> +			 "failed to read EC charge state (%d), applying defaults\n",
> +			 ret);
> +		cros_chctl_set_default_state(priv);
> +		return cros_chctl_configure_ec(priv);
> +	}
> +
> +	return 0;
>  }

Instead of asserting the lock in the function, it would be cleaner to handle
the lock acquisition directly inside this function (e.g., by using guard()).

> @@ -305,13 +435,8 @@ static int cros_chctl_probe(struct platform_device *pdev)
>  	priv->battery_hook.add_battery = cros_chctl_add_battery;
>  	priv->battery_hook.remove_battery = cros_chctl_remove_battery;
>  
> -	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> -	priv->current_start_threshold = 0;
> -	priv->current_end_threshold = 100;
> -
> -	/* Bring EC into well-known state */
>  	scoped_guard(mutex, &priv->lock)
> -		ret = cros_chctl_configure_ec(priv);
> +		ret = cros_chctl_init_state(priv);

Following the suggestion above, please move the guard() into
cros_chctl_init_state().

Re: [PATCH] power: supply: cros_charge-control: adopt EC charge state on probe

[Thomas Weißschuh]

On 2026-08-24 10:57:18-0500, Matt DeVillier wrote:
> The driver previously always forced AUTO with no charge limits at
> probe, which discarded sustainer thresholds and modes set by firmware
> or firmware setup before the kernel loaded.
> 
> For command versions that support GET (v2+), read the EC state into
> the driver cache instead. Valid sustainer limits are adopted as AUTO
> with those thresholds: while the sustainer is active the EC may report
> IDLE or DISCHARGE as a transient hold/discharge step, which must not
> be exposed as inhibit-charge or force-discharge. Sustainer off
> (-1/-1) still maps to Linux "no limit" (0/100); other invalid limit
> pairs are remapped the same way with a warning.
> 
> If GET fails, fall back to the previous defaults and SET them on the
> EC. Command version 1 still cannot report state and keeps forcing a
> well-known configuration.
> 
> Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
> ---
>  drivers/power/supply/cros_charge-control.c | 159 ++++++++++++++++++---
>  1 file changed, 142 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
> index e0f168624807..fd620317cfba 100644
> --- a/drivers/power/supply/cros_charge-control.c
> +++ b/drivers/power/supply/cros_charge-control.c
> @@ -21,12 +21,22 @@
>  
>  /*
>   * Semantics of data *returned* from the EC API and Linux sysfs differ
> - * slightly, also the v1 API can not return any data.
> - * To match the expected sysfs API, data is never read back from the EC but
> - * cached in the driver.
> + * slightly:
> + *  - EC sustainer off is lower=upper=-1
> + *  - Linux "no limit" is start=0, end=100
> + * Also the v1 API can not return any data.
>   *
> - * Changes to the EC bypassing the driver will not be reflected in sysfs.
> - * Any change to "charge_behaviour" will synchronize the EC with the driver state.
> + * While the sustainer is active the EC may report IDLE or DISCHARGE as the
> + * current charge-control mode; that is an internal hold/discharge step, not a
> + * Linux inhibit-charge / force-discharge request. Valid sustainer limits are
> + * therefore adopted as AUTO with the reported thresholds.
> + *
> + * Sysfs reads come from a driver-side cache. On probe, command versions that
> + * support GET (v2+) are initialized from the EC so firmware or firmware-setup
> + * programmed limits and modes are preserved (unlike earlier behaviour that
> + * always forced AUTO with no limits). v1 still forces a well-known EC state.
> + * Subsequent sysfs writes keep the cache and EC in sync; changes that bypass
> + * the driver are not reflected until the next probe.
>   */
>  
>  struct cros_chctl_priv {
> @@ -44,18 +54,20 @@ struct cros_chctl_priv {
>  };
>  
>  static int cros_chctl_send_charge_control_cmd(struct cros_ec_device *cros_ec,
> -					      u8 cmd_version, struct ec_params_charge_control *req)
> +					      u8 cmd_version,
> +					      struct ec_params_charge_control *req,
> +					      struct ec_response_charge_control *resp)
>  {
> -	int ret;
>  	static const u8 outsizes[] = {
>  		[1] = offsetof(struct ec_params_charge_control, cmd),
>  		[2] = sizeof(struct ec_params_charge_control),
>  		[3] = sizeof(struct ec_params_charge_control),
>  	};
> +	size_t insize = resp ? sizeof(*resp) : 0;

This function is not guaranteed to write all bytes of the response
structure. This forces callers to pass in a pre-initialized structure.
I'd prefer this function to guarantee a fully initialized response.
Either by unconditionally zeroing out the resp parameter before calling
cros_ec_cmd() or only zeroing the trailing fields, using the return
value of cros_ec_cmd().

> +	int ret;

>  
>  	ret = cros_ec_cmd(cros_ec, cmd_version, EC_CMD_CHARGE_CONTROL, req,
> -			  outsizes[cmd_version], NULL, 0);
> -
> +			  outsizes[cmd_version], resp, insize);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -94,7 +106,126 @@ static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
>  		req.sustain_soc.upper = -1;
>  	}
>  
> -	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
> +	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
> +						  &req, NULL);

You could split the addition of the resp argument to cros_chctl_send_charge_control_cmd()
into its own patch.

> +}
> +
> +static int cros_chctl_get_ec_status(struct cros_chctl_priv *priv,
> +				    struct ec_response_charge_control *resp)
> +{
> +	struct ec_params_charge_control req = {
> +		.cmd = EC_CHARGE_CONTROL_CMD_GET,
> +	};
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
> +						  &req, resp);
> +}
> +
> +static void cros_chctl_set_default_state(struct cros_chctl_priv *priv)
> +{
> +	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +	priv->current_start_threshold = 0;
> +	priv->current_end_threshold = 100;
> +}
> +
> +static bool cros_chctl_sustainer_limits_valid(s8 lower, s8 upper)
> +{
> +	return lower >= 0 && upper >= 0 && lower <= 100 && upper <= 100 &&
> +	       lower <= upper;

No need to break so aggressively, you have 100 columns.

> +}
> +
> +static int cros_chctl_adopt_ec_mode(struct cros_chctl_priv *priv, u32 mode)
> +{
> +	switch (mode) {
> +	case CHARGE_CONTROL_NORMAL:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +		return 0;
> +	case CHARGE_CONTROL_IDLE:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
> +		return 0;
> +	case CHARGE_CONTROL_DISCHARGE:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
> +		return 0;
> +	default:
> +		dev_warn(priv->dev, "unknown charge control mode %u\n", mode);
> +		return -EINVAL;
> +	}
> +}
> +
> +static int cros_chctl_adopt_ec_state(struct cros_chctl_priv *priv)
> +{
> +	struct ec_response_charge_control resp = {};

No need to initialize this with the changes requested above.

> +	s8 lower, upper;
> +	int ret;
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	ret = cros_chctl_get_ec_status(priv, &resp);
> +	if (ret < 0)
> +		return ret;
> +
> +	lower = resp.sustain_soc.lower;
> +	upper = resp.sustain_soc.upper;
> +
> +	/*
> +	 * Valid sustainer limits mean "auto with thresholds". The EC mode may
> +	 * be IDLE/DISCHARGE while the sustainer holds or bleeds SoC; do not
> +	 * expose that as inhibit-charge / force-discharge.
> +	 */
> +	if (cros_chctl_sustainer_limits_valid(lower, upper)) {
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +		priv->current_start_threshold = lower;
> +		priv->current_end_threshold = upper;
> +	} else {
> +		ret = cros_chctl_adopt_ec_mode(priv, resp.mode);
> +		if (ret < 0)
> +			return ret;
> +
> +		/*
> +		 * Sustainer off is lower=upper=-1 → Linux "no limit" (0/100).
> +		 * Any other non-valid pair is unexpected; remap and warn.
> +		 */
> +		if (!(lower == -1 && upper == -1))
> +			dev_warn(priv->dev,
> +				 "invalid EC sustainer limits (%d/%d), treating as no limit\n",
> +				 lower, upper);

Here the view between the EC and the driver goes out of sync.
I'd prefer to reconfigure the EC here, too.

> +
> +		priv->current_start_threshold = 0;
> +		priv->current_end_threshold = 100;
> +	}
> +
> +	dev_dbg(priv->dev,
> +		"adopted EC charge state: behaviour=%d start=%u end=%u (ec mode=%u)\n",
> +		priv->current_behaviour, priv->current_start_threshold,
> +		priv->current_end_threshold, resp.mode);
> +
> +	return 0;
> +}
> +
> +static int cros_chctl_init_state(struct cros_chctl_priv *priv)
> +{
> +	int ret;
> +
> +	lockdep_assert_held(&priv->lock);
> +
> +	cros_chctl_set_default_state(priv);

This can be under the 'cmd_version < 2' below.

> +
> +	/* v1 cannot report current state; force a well-known EC configuration. */
> +	if (priv->cmd_version < 2)
> +		return cros_chctl_configure_ec(priv);
> +
> +	ret = cros_chctl_adopt_ec_state(priv);

I have the feeling that the code would be easier to understand if
cros_chctl_adopt_ec_state() was inlined here.

> +	if (ret < 0) {
> +		dev_warn(priv->dev,
> +			 "failed to read EC charge state (%d), applying defaults\n",

Use %pe to log error numbers.

> +			 ret);
> +		cros_chctl_set_default_state(priv);
> +		return cros_chctl_configure_ec(priv);
> +	}
> +
> +	return 0;
>  }
>  
>  static int cros_chctl_psy_ext_get_prop(struct power_supply *psy,
> @@ -152,7 +283,6 @@ static int cros_chctl_psy_ext_set_threshold(struct cros_chctl_priv *priv,
>  	return 0;
>  }
>  
> -
>  static int cros_chctl_psy_ext_set_prop(struct power_supply *psy,
>  				       const struct power_supply_ext *ext,
>  				       void *data,
> @@ -305,13 +435,8 @@ static int cros_chctl_probe(struct platform_device *pdev)
>  	priv->battery_hook.add_battery = cros_chctl_add_battery;
>  	priv->battery_hook.remove_battery = cros_chctl_remove_battery;
>  
> -	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> -	priv->current_start_threshold = 0;
> -	priv->current_end_threshold = 100;
> -
> -	/* Bring EC into well-known state */
>  	scoped_guard(mutex, &priv->lock)
> -		ret = cros_chctl_configure_ec(priv);
> +		ret = cros_chctl_init_state(priv);
>  	if (ret < 0)
>  		return ret;
>  
> -- 
> 2.53.0
> 

[PATCH v2] power: supply: cros_charge-control: adopt EC charge state on probe

[Matt DeVillier]

The driver previously always forced AUTO with no charge limits at
probe, which discarded sustainer thresholds and modes set by firmware
or firmware setup before the kernel loaded.

For command versions that support GET (v2+), read the EC state into
the driver cache instead. Valid sustainer limits are adopted as AUTO
with those thresholds: while the sustainer is active the EC may report
IDLE or DISCHARGE as a transient hold/discharge step, which must not
be exposed as inhibit-charge or force-discharge. Sustainer off
(-1/-1) still maps to Linux "no limit" (0/100); other invalid limit
pairs are remapped the same way with a warning.

If GET fails, fall back to the previous defaults and SET them on the
EC. Command version 1 still cannot report state and keeps forcing a
well-known configuration.

Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
---
v2:
- zero EC response in send helper; take mutex inside init_state
- reconfigure EC on invalid sustainer limits; use %pe for errors
- address lockdep placement feedback

 drivers/power/supply/cros_charge-control.c | 159 ++++++++++++++++++---
 1 file changed, 142 insertions(+), 17 deletions(-)

diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
index e0f168624807..fd620317cfba 100644
--- a/drivers/power/supply/cros_charge-control.c
+++ b/drivers/power/supply/cros_charge-control.c
@@ -21,12 +21,22 @@
 
 /*
  * Semantics of data *returned* from the EC API and Linux sysfs differ
- * slightly, also the v1 API can not return any data.
- * To match the expected sysfs API, data is never read back from the EC but
- * cached in the driver.
+ * slightly:
+ *  - EC sustainer off is lower=upper=-1
+ *  - Linux "no limit" is start=0, end=100
+ * Also the v1 API can not return any data.
  *
- * Changes to the EC bypassing the driver will not be reflected in sysfs.
- * Any change to "charge_behaviour" will synchronize the EC with the driver state.
+ * While the sustainer is active the EC may report IDLE or DISCHARGE as the
+ * current charge-control mode; that is an internal hold/discharge step, not a
+ * Linux inhibit-charge / force-discharge request. Valid sustainer limits are
+ * therefore adopted as AUTO with the reported thresholds.
+ *
+ * Sysfs reads come from a driver-side cache. On probe, command versions that
+ * support GET (v2+) are initialized from the EC so firmware or firmware-setup
+ * programmed limits and modes are preserved (unlike earlier behaviour that
+ * always forced AUTO with no limits). v1 still forces a well-known EC state.
+ * Subsequent sysfs writes keep the cache and EC in sync; changes that bypass
+ * the driver are not reflected until the next probe.
  */
 
 struct cros_chctl_priv {
@@ -44,18 +54,20 @@ struct cros_chctl_priv {
 };
 
 static int cros_chctl_send_charge_control_cmd(struct cros_ec_device *cros_ec,
-					      u8 cmd_version, struct ec_params_charge_control *req)
+					      u8 cmd_version,
+					      struct ec_params_charge_control *req,
+					      struct ec_response_charge_control *resp)
 {
-	int ret;
 	static const u8 outsizes[] = {
 		[1] = offsetof(struct ec_params_charge_control, cmd),
 		[2] = sizeof(struct ec_params_charge_control),
 		[3] = sizeof(struct ec_params_charge_control),
 	};
+	size_t insize = resp ? sizeof(*resp) : 0;
+	int ret;
 
 	ret = cros_ec_cmd(cros_ec, cmd_version, EC_CMD_CHARGE_CONTROL, req,
-			  outsizes[cmd_version], NULL, 0);
-
+			  outsizes[cmd_version], resp, insize);
 	if (ret < 0)
 		return ret;
 
@@ -94,7 +106,126 @@ static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
 		req.sustain_soc.upper = -1;
 	}
 
-	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
+	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
+						  &req, NULL);
+}
+
+static int cros_chctl_get_ec_status(struct cros_chctl_priv *priv,
+				    struct ec_response_charge_control *resp)
+{
+	struct ec_params_charge_control req = {
+		.cmd = EC_CHARGE_CONTROL_CMD_GET,
+	};
+
+	lockdep_assert_held(&priv->lock);
+
+	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
+						  &req, resp);
+}
+
+static void cros_chctl_set_default_state(struct cros_chctl_priv *priv)
+{
+	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+	priv->current_start_threshold = 0;
+	priv->current_end_threshold = 100;
+}
+
+static bool cros_chctl_sustainer_limits_valid(s8 lower, s8 upper)
+{
+	return lower >= 0 && upper >= 0 && lower <= 100 && upper <= 100 &&
+	       lower <= upper;
+}
+
+static int cros_chctl_adopt_ec_mode(struct cros_chctl_priv *priv, u32 mode)
+{
+	switch (mode) {
+	case CHARGE_CONTROL_NORMAL:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+		return 0;
+	case CHARGE_CONTROL_IDLE:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
+		return 0;
+	case CHARGE_CONTROL_DISCHARGE:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+		return 0;
+	default:
+		dev_warn(priv->dev, "unknown charge control mode %u\n", mode);
+		return -EINVAL;
+	}
+}
+
+static int cros_chctl_adopt_ec_state(struct cros_chctl_priv *priv)
+{
+	struct ec_response_charge_control resp = {};
+	s8 lower, upper;
+	int ret;
+
+	lockdep_assert_held(&priv->lock);
+
+	ret = cros_chctl_get_ec_status(priv, &resp);
+	if (ret < 0)
+		return ret;
+
+	lower = resp.sustain_soc.lower;
+	upper = resp.sustain_soc.upper;
+
+	/*
+	 * Valid sustainer limits mean "auto with thresholds". The EC mode may
+	 * be IDLE/DISCHARGE while the sustainer holds or bleeds SoC; do not
+	 * expose that as inhibit-charge / force-discharge.
+	 */
+	if (cros_chctl_sustainer_limits_valid(lower, upper)) {
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+		priv->current_start_threshold = lower;
+		priv->current_end_threshold = upper;
+	} else {
+		ret = cros_chctl_adopt_ec_mode(priv, resp.mode);
+		if (ret < 0)
+			return ret;
+
+		/*
+		 * Sustainer off is lower=upper=-1 → Linux "no limit" (0/100).
+		 * Any other non-valid pair is unexpected; remap and warn.
+		 */
+		if (!(lower == -1 && upper == -1))
+			dev_warn(priv->dev,
+				 "invalid EC sustainer limits (%d/%d), treating as no limit\n",
+				 lower, upper);
+
+		priv->current_start_threshold = 0;
+		priv->current_end_threshold = 100;
+	}
+
+	dev_dbg(priv->dev,
+		"adopted EC charge state: behaviour=%d start=%u end=%u (ec mode=%u)\n",
+		priv->current_behaviour, priv->current_start_threshold,
+		priv->current_end_threshold, resp.mode);
+
+	return 0;
+}
+
+static int cros_chctl_init_state(struct cros_chctl_priv *priv)
+{
+	int ret;
+
+	lockdep_assert_held(&priv->lock);
+
+	cros_chctl_set_default_state(priv);
+
+	/* v1 cannot report current state; force a well-known EC configuration. */
+	if (priv->cmd_version < 2)
+		return cros_chctl_configure_ec(priv);
+
+	ret = cros_chctl_adopt_ec_state(priv);
+	if (ret < 0) {
+		dev_warn(priv->dev,
+			 "failed to read EC charge state (%d), applying defaults\n",
+			 ret);
+		cros_chctl_set_default_state(priv);
+		return cros_chctl_configure_ec(priv);
+	}
+
+	return 0;
 }
 
 static int cros_chctl_psy_ext_get_prop(struct power_supply *psy,
@@ -152,7 +283,6 @@ static int cros_chctl_psy_ext_set_threshold(struct cros_chctl_priv *priv,
 	return 0;
 }
 
-
 static int cros_chctl_psy_ext_set_prop(struct power_supply *psy,
 				       const struct power_supply_ext *ext,
 				       void *data,
@@ -305,13 +435,8 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	priv->battery_hook.add_battery = cros_chctl_add_battery;
 	priv->battery_hook.remove_battery = cros_chctl_remove_battery;
 
-	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
-	priv->current_start_threshold = 0;
-	priv->current_end_threshold = 100;
-
-	/* Bring EC into well-known state */
 	scoped_guard(mutex, &priv->lock)
-		ret = cros_chctl_configure_ec(priv);
+		ret = cros_chctl_init_state(priv);
 	if (ret < 0)
 		return ret;
 
-- 
2.53.0

[PATCH v2] power: supply: cros_charge-control: adopt EC charge state on probe

[Matt DeVillier]

The driver previously always forced AUTO with no charge limits at
probe, which discarded sustainer thresholds and modes set by firmware
or firmware setup before the kernel loaded.

For command versions that support GET (v2+), read the EC state into
the driver cache instead. Valid sustainer limits are adopted as AUTO
with those thresholds: while the sustainer is active the EC may report
IDLE or DISCHARGE as a transient hold/discharge step, which must not
be exposed as inhibit-charge or force-discharge. Sustainer off
(-1/-1) still maps to Linux "no limit" (0/100); other invalid limit
pairs are remapped the same way with a warning and pushed back to
the EC so the cache stays in sync.

If GET fails, fall back to the previous defaults and SET them on the
EC. Command version 1 still cannot report state and keeps forcing a
well-known configuration.

Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>
---
 drivers/power/supply/cros_charge-control.c | 157 ++++++++++++++++++---
 1 file changed, 139 insertions(+), 18 deletions(-)

diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
index e0f168624807..07bcc6a9aeef 100644
--- a/drivers/power/supply/cros_charge-control.c
+++ b/drivers/power/supply/cros_charge-control.c
@@ -21,12 +21,22 @@
 
 /*
  * Semantics of data *returned* from the EC API and Linux sysfs differ
- * slightly, also the v1 API can not return any data.
- * To match the expected sysfs API, data is never read back from the EC but
- * cached in the driver.
+ * slightly:
+ *  - EC sustainer off is lower=upper=-1
+ *  - Linux "no limit" is start=0, end=100
+ * Also the v1 API can not return any data.
  *
- * Changes to the EC bypassing the driver will not be reflected in sysfs.
- * Any change to "charge_behaviour" will synchronize the EC with the driver state.
+ * While the sustainer is active the EC may report IDLE or DISCHARGE as the
+ * current charge-control mode; that is an internal hold/discharge step, not a
+ * Linux inhibit-charge / force-discharge request. Valid sustainer limits are
+ * therefore adopted as AUTO with the reported thresholds.
+ *
+ * Sysfs reads come from a driver-side cache. On probe, command versions that
+ * support GET (v2+) are initialized from the EC so firmware or firmware-setup
+ * programmed limits and modes are preserved (unlike earlier behaviour that
+ * always forced AUTO with no limits). v1 still forces a well-known EC state.
+ * Subsequent sysfs writes keep the cache and EC in sync; changes that bypass
+ * the driver are not reflected until the next probe.
  */
 
 struct cros_chctl_priv {
@@ -44,18 +54,23 @@ struct cros_chctl_priv {
 };
 
 static int cros_chctl_send_charge_control_cmd(struct cros_ec_device *cros_ec,
-					      u8 cmd_version, struct ec_params_charge_control *req)
+					      u8 cmd_version,
+					      struct ec_params_charge_control *req,
+					      struct ec_response_charge_control *resp)
 {
-	int ret;
 	static const u8 outsizes[] = {
 		[1] = offsetof(struct ec_params_charge_control, cmd),
 		[2] = sizeof(struct ec_params_charge_control),
 		[3] = sizeof(struct ec_params_charge_control),
 	};
+	size_t insize = resp ? sizeof(*resp) : 0;
+	int ret;
 
-	ret = cros_ec_cmd(cros_ec, cmd_version, EC_CMD_CHARGE_CONTROL, req,
-			  outsizes[cmd_version], NULL, 0);
+	if (resp)
+		*resp = (struct ec_response_charge_control){};
 
+	ret = cros_ec_cmd(cros_ec, cmd_version, EC_CMD_CHARGE_CONTROL, req,
+			  outsizes[cmd_version], resp, insize);
 	if (ret < 0)
 		return ret;
 
@@ -94,7 +109,120 @@ static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
 		req.sustain_soc.upper = -1;
 	}
 
-	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
+	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
+						  &req, NULL);
+}
+
+static int cros_chctl_get_ec_status(struct cros_chctl_priv *priv,
+				    struct ec_response_charge_control *resp)
+{
+	struct ec_params_charge_control req = {
+		.cmd = EC_CHARGE_CONTROL_CMD_GET,
+	};
+
+	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
+						  &req, resp);
+}
+
+static void cros_chctl_set_default_state(struct cros_chctl_priv *priv)
+{
+	lockdep_assert_held(&priv->lock);
+
+	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+	priv->current_start_threshold = 0;
+	priv->current_end_threshold = 100;
+}
+
+static bool cros_chctl_sustainer_limits_valid(s8 lower, s8 upper)
+{
+	return lower >= 0 && upper >= 0 && lower <= 100 && upper <= 100 && lower <= upper;
+}
+
+static int cros_chctl_adopt_ec_mode(struct cros_chctl_priv *priv, u32 mode)
+{
+	lockdep_assert_held(&priv->lock);
+
+	switch (mode) {
+	case CHARGE_CONTROL_NORMAL:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+		return 0;
+	case CHARGE_CONTROL_IDLE:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
+		return 0;
+	case CHARGE_CONTROL_DISCHARGE:
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
+		return 0;
+	default:
+		dev_warn(priv->dev, "unknown charge control mode %u\n", mode);
+		return -EINVAL;
+	}
+}
+
+static int cros_chctl_init_state(struct cros_chctl_priv *priv)
+{
+	struct ec_response_charge_control resp;
+	s8 lower, upper;
+	int ret;
+
+	guard(mutex)(&priv->lock);
+
+	/* v1 cannot report current state; force a well-known EC configuration. */
+	if (priv->cmd_version < 2) {
+		cros_chctl_set_default_state(priv);
+		return cros_chctl_configure_ec(priv);
+	}
+
+	ret = cros_chctl_get_ec_status(priv, &resp);
+	if (ret < 0) {
+		dev_warn(priv->dev,
+			 "failed to read EC charge state (%pe), applying defaults\n",
+			 ERR_PTR(ret));
+		goto defaults;
+	}
+
+	lower = resp.sustain_soc.lower;
+	upper = resp.sustain_soc.upper;
+
+	/*
+	 * Valid sustainer limits mean "auto with thresholds". The EC mode may
+	 * be IDLE/DISCHARGE while the sustainer holds or bleeds SoC; do not
+	 * expose that as inhibit-charge / force-discharge.
+	 */
+	if (cros_chctl_sustainer_limits_valid(lower, upper)) {
+		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
+		priv->current_start_threshold = lower;
+		priv->current_end_threshold = upper;
+	} else {
+		ret = cros_chctl_adopt_ec_mode(priv, resp.mode);
+		if (ret < 0)
+			goto defaults;
+
+		/*
+		 * Sustainer off is lower=upper=-1 → Linux "no limit" (0/100).
+		 * Any other non-valid pair is unexpected; remap, warn, and
+		 * push the cleaned state back to the EC.
+		 */
+		priv->current_start_threshold = 0;
+		priv->current_end_threshold = 100;
+
+		if (!(lower == -1 && upper == -1)) {
+			dev_warn(priv->dev,
+				 "invalid EC sustainer limits (%d/%d), treating as no limit\n",
+				 lower, upper);
+			return cros_chctl_configure_ec(priv);
+		}
+	}
+
+	dev_dbg(priv->dev,
+		"adopted EC charge state: behaviour=%d start=%u end=%u (ec mode=%u)\n",
+		priv->current_behaviour, priv->current_start_threshold,
+		priv->current_end_threshold, resp.mode);
+
+	return 0;
+
+defaults:
+	cros_chctl_set_default_state(priv);
+	return cros_chctl_configure_ec(priv);
 }
 
 static int cros_chctl_psy_ext_get_prop(struct power_supply *psy,
@@ -152,7 +280,6 @@ static int cros_chctl_psy_ext_set_threshold(struct cros_chctl_priv *priv,
 	return 0;
 }
 
-
 static int cros_chctl_psy_ext_set_prop(struct power_supply *psy,
 				       const struct power_supply_ext *ext,
 				       void *data,
@@ -305,13 +432,7 @@ static int cros_chctl_probe(struct platform_device *pdev)
 	priv->battery_hook.add_battery = cros_chctl_add_battery;
 	priv->battery_hook.remove_battery = cros_chctl_remove_battery;
 
-	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
-	priv->current_start_threshold = 0;
-	priv->current_end_threshold = 100;
-
-	/* Bring EC into well-known state */
-	scoped_guard(mutex, &priv->lock)
-		ret = cros_chctl_configure_ec(priv);
+	ret = cros_chctl_init_state(priv);
 	if (ret < 0)
 		return ret;
 
-- 
2.53.0

Re: [PATCH v2] power: supply: cros_charge-control: adopt EC charge state on probe

[Thomas Weißschuh]

On 2026-08-25 07:09:00-0500, Matt DeVillier wrote:
> The driver previously always forced AUTO with no charge limits at
> probe, which discarded sustainer thresholds and modes set by firmware
> or firmware setup before the kernel loaded.
> 
> For command versions that support GET (v2+), read the EC state into
> the driver cache instead. Valid sustainer limits are adopted as AUTO
> with those thresholds: while the sustainer is active the EC may report
> IDLE or DISCHARGE as a transient hold/discharge step, which must not
> be exposed as inhibit-charge or force-discharge. Sustainer off
> (-1/-1) still maps to Linux "no limit" (0/100); other invalid limit
> pairs are remapped the same way with a warning and pushed back to
> the EC so the cache stays in sync.
> 
> If GET fails, fall back to the previous defaults and SET them on the
> EC. Command version 1 still cannot report state and keeps forcing a
> well-known configuration.
> 
> Signed-off-by: Matt DeVillier <matt.devillier@gmail.com>

Something seems to have gone wrong when sending the v2.
Two mails are sent, the first has the old patch and the changelog,
the second one has no changelog but the correct v2.

I have a few small nitpicks left, but in any case:

Acked-by: Thomas Weißschuh <linux@weissschuh.net>

> ---
>  drivers/power/supply/cros_charge-control.c | 157 ++++++++++++++++++---
>  1 file changed, 139 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/power/supply/cros_charge-control.c b/drivers/power/supply/cros_charge-control.c
> index e0f168624807..07bcc6a9aeef 100644
> --- a/drivers/power/supply/cros_charge-control.c
> +++ b/drivers/power/supply/cros_charge-control.c
> @@ -21,12 +21,22 @@

(...)
  
>  struct cros_chctl_priv {
> @@ -44,18 +54,23 @@ struct cros_chctl_priv {
>  };
>  
>  static int cros_chctl_send_charge_control_cmd(struct cros_ec_device *cros_ec,
> -					      u8 cmd_version, struct ec_params_charge_control *req)
> +					      u8 cmd_version,
> +					      struct ec_params_charge_control *req,
> +					      struct ec_response_charge_control *resp)
>  {
> -	int ret;
>  	static const u8 outsizes[] = {
>  		[1] = offsetof(struct ec_params_charge_control, cmd),
>  		[2] = sizeof(struct ec_params_charge_control),
>  		[3] = sizeof(struct ec_params_charge_control),
>  	};
> +	size_t insize = resp ? sizeof(*resp) : 0;
> +	int ret;
>  
> -	ret = cros_ec_cmd(cros_ec, cmd_version, EC_CMD_CHARGE_CONTROL, req,
> -			  outsizes[cmd_version], NULL, 0);
> +	if (resp)
> +		*resp = (struct ec_response_charge_control){};
>  
> +	ret = cros_ec_cmd(cros_ec, cmd_version, EC_CMD_CHARGE_CONTROL, req,
> +			  outsizes[cmd_version], resp, insize);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -94,7 +109,120 @@ static int cros_chctl_configure_ec(struct cros_chctl_priv *priv)
>  		req.sustain_soc.upper = -1;
>  	}
>  
> -	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version, &req);
> +	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
> +						  &req, NULL);

This can also go on a single line.

> +}
> +
> +static int cros_chctl_get_ec_status(struct cros_chctl_priv *priv,
> +				    struct ec_response_charge_control *resp)
> +{
> +	struct ec_params_charge_control req = {
> +		.cmd = EC_CHARGE_CONTROL_CMD_GET,
> +	};
> +
> +	return cros_chctl_send_charge_control_cmd(priv->cros_ec, priv->cmd_version,
> +						  &req, resp);

Ditto.

> +}
> +
> +static void cros_chctl_set_default_state(struct cros_chctl_priv *priv)
> +{
> +	lockdep_assert_held(&priv->lock);
> +
> +	priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +	priv->current_start_threshold = 0;
> +	priv->current_end_threshold = 100;
> +}
> +
> +static bool cros_chctl_sustainer_limits_valid(s8 lower, s8 upper)
> +{
> +	return lower >= 0 && upper >= 0 && lower <= 100 && upper <= 100 && lower <= upper;
> +}
> +
> +static int cros_chctl_adopt_ec_mode(struct cros_chctl_priv *priv, u32 mode)
> +{
> +	lockdep_assert_held(&priv->lock);
> +
> +	switch (mode) {
> +	case CHARGE_CONTROL_NORMAL:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_AUTO;
> +		return 0;
> +	case CHARGE_CONTROL_IDLE:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_INHIBIT_CHARGE;
> +		return 0;
> +	case CHARGE_CONTROL_DISCHARGE:
> +		priv->current_behaviour = POWER_SUPPLY_CHARGE_BEHAVIOUR_FORCE_DISCHARGE;
> +		return 0;
> +	default:
> +		dev_warn(priv->dev, "unknown charge control mode %u\n", mode);
> +		return -EINVAL;

The error number is never used. The function could return a bool instead.

> +	}
> +}
> +
> +static int cros_chctl_init_state(struct cros_chctl_priv *priv)
> +{

(...)

> +defaults:
> +	cros_chctl_set_default_state(priv);
> +	return cros_chctl_configure_ec(priv);

This might as well be inline instead of an extra label.

>  }
>  
>  static int cros_chctl_psy_ext_get_prop(struct power_supply *psy,
> @@ -152,7 +280,6 @@ static int cros_chctl_psy_ext_set_threshold(struct cros_chctl_priv *priv,
>  	return 0;
>  }

(...)