Re: [PATCH v2] usb: typec: ucsi: psy: Add support for the charge type property

[Mario Limonciello <mario.limonciello@amd.com>]

On 6/18/2024 03:15, Heikki Krogerus wrote:
> Adding support for the charge type Linux power supply class
> property (POWER_SUPPLY_PROP_CHARGE_TYPE) to the UCSI driver.
> That will make the charge type visible in the charge_type
> power supply class sysfs attribute file.
> 
> UCSI has the charge type specified in the Battery Charging
> Status field of the response to the GET_CONNECTOR_STATUS
> command.
> 
> Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> ---
> Changed since the last version:
> The commit message is completely rewritten. The subject line was also changed.
> 
> The original patch:
> https://lore.kernel.org/linux-usb/20240617105554.1677285-1-heikki.krogerus@linux.intel.com/
> 
> ---
>   drivers/usb/typec/ucsi/psy.c  | 27 +++++++++++++++++++++++++++
>   drivers/usb/typec/ucsi/ucsi.c |  3 +++
>   2 files changed, 30 insertions(+)
> 
> diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
> index b35c6e07911e..b3910f37e171 100644
> --- a/drivers/usb/typec/ucsi/psy.c
> +++ b/drivers/usb/typec/ucsi/psy.c
> @@ -20,6 +20,7 @@ enum ucsi_psy_online_states {
>   };
>   
>   static enum power_supply_property ucsi_psy_props[] = {
> +	POWER_SUPPLY_PROP_CHARGE_TYPE,
>   	POWER_SUPPLY_PROP_USB_TYPE,
>   	POWER_SUPPLY_PROP_ONLINE,
>   	POWER_SUPPLY_PROP_VOLTAGE_MIN,
> @@ -194,6 +195,30 @@ static int ucsi_psy_get_usb_type(struct ucsi_connector *con,
>   	return 0;
>   }
>   
> +static int ucsi_psy_get_charge_type(struct ucsi_connector *con, union power_supply_propval *val)
> +{
> +	if (!(con->status.flags & UCSI_CONSTAT_CONNECTED) ||
> +	    (con->status.flags & UCSI_CONSTAT_PWR_DIR) != TYPEC_SINK) {
> +		val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;

The not connected state obviously makes sense for 
POWER_SUPPLY_CHARGE_TYPE_NONE, but what exactly is the other situation? 
A UCSI state machine failure?

I'm mostly wondering if POWER_SUPPLY_CHARGE_TYPE_UNKNOWN makes sense for 
that or not.

Besides this question the patch looks fine to me and you can add my tag 
with your decision.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> +		return 0;
> +	}
> +
> +	switch (UCSI_CONSTAT_BC_STATUS(con->status.pwr_status)) {
> +	case UCSI_CONSTAT_BC_NOMINAL_CHARGING:
> +		val->intval = POWER_SUPPLY_CHARGE_TYPE_STANDARD;
> +		break;
> +	case UCSI_CONSTAT_BC_SLOW_CHARGING:
> +	case UCSI_CONSTAT_BC_TRICKLE_CHARGING:
> +		val->intval = POWER_SUPPLY_CHARGE_TYPE_TRICKLE;
> +		break;
> +	default:
> +		val->intval = POWER_SUPPLY_CHARGE_TYPE_NONE;
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
>   static int ucsi_psy_get_prop(struct power_supply *psy,
>   			     enum power_supply_property psp,
>   			     union power_supply_propval *val)
> @@ -201,6 +226,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy,
>   	struct ucsi_connector *con = power_supply_get_drvdata(psy);
>   
>   	switch (psp) {
> +	case POWER_SUPPLY_PROP_CHARGE_TYPE:
> +		return ucsi_psy_get_charge_type(con, val);
>   	case POWER_SUPPLY_PROP_USB_TYPE:
>   		return ucsi_psy_get_usb_type(con, val);
>   	case POWER_SUPPLY_PROP_ONLINE:
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 6bbaad4b89a5..51135e3502cf 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -1257,6 +1257,9 @@ static void ucsi_handle_connector_change(struct work_struct *work)
>   	if (con->status.change & UCSI_CONSTAT_CAM_CHANGE)
>   		ucsi_partner_task(con, ucsi_check_altmodes, 1, HZ);
>   
> +	if (con->status.change & UCSI_CONSTAT_BC_CHANGE)
> +		ucsi_port_psy_changed(con);
> +
>   out_unlock:
>   	mutex_unlock(&con->lock);
>   }