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

[Tzung-Bi Shih <tzungbi@kernel.org>]

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