Re: [PATCH v2 1/4] usb: typec: ucsi: Add status to UCSI power supply driver

[Sebastian Reichel]

Hi,

On Thu, Jul 25, 2024 at 06:31:00AM GMT, Dmitry Baryshkov wrote:
> On Wed, Jul 24, 2024 at 08:11:13PM GMT, Jameson Thies wrote:
> > Add status to UCSI power supply driver properties based on the port's
> > connection and power direction states.
> > 
> > Signed-off-by: Jameson Thies <jthies@xxxxxxxxxx>
> 
> Please CC Power Supply maintainers for this patchset (added to cc).

Thanks. I guess I should add something like this to MAINTAINERS
considering the power-supply bits are in its own file for UCSI:

UCSI POWER-SUPPLY API
R:	Sebastian Reichel <sre@xxxxxxxxxx>
L:	linux-pm@xxxxxxxxxxxxxxx
F:	drivers/usb/typec/ucsi/psy.c

> At least per the Documentation/ABI/testing/sysfs-class-power, the status
> property applies to batteries, while UCSI psy device is a charger. This
> is logical, as there might be multiple reasons why the battery is not
> being charging even when the supply is online.

Correct. These status bits are not meant for chargers. E.g.
POWER_SUPPLY_STATUS_NOT_CHARGING has a very specific meaning, that a
battery is neither charged nor discharged. Since the device is
running that can only happen when a charger is connected, but not
charging (or the device has two batteries).

For AC the power-supply API has been designed only taking the SINK
role into account. At some point USB was added and some time later
people added reverse mode to their USB chargers for OTG mode. So
far this has been handled by registering a regulator and using that
to switch the mode. This made sense for OTG, but USB-C PD makes
things even more complex.

I am open to ideas how to properly handle the source role in the
power-supply API, but let's not overload the status property for
it. Please make sure to CC me on any follow-up series.

-- Sebastian

> 
> > ---
> > Changes in V2:
> > - None.
> > 
> >  drivers/usb/typec/ucsi/psy.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/usb/typec/ucsi/psy.c b/drivers/usb/typec/ucsi/psy.c
> > index e623d80e177c..d0b52cee41d2 100644
> > --- a/drivers/usb/typec/ucsi/psy.c
> > +++ b/drivers/usb/typec/ucsi/psy.c
> > @@ -29,6 +29,7 @@ static enum power_supply_property ucsi_psy_props[] = {
> >  	POWER_SUPPLY_PROP_CURRENT_MAX,
> >  	POWER_SUPPLY_PROP_CURRENT_NOW,
> >  	POWER_SUPPLY_PROP_SCOPE,
> > +	POWER_SUPPLY_PROP_STATUS,
> >  };
> >  
> >  static int ucsi_psy_get_scope(struct ucsi_connector *con,
> > @@ -51,6 +52,20 @@ static int ucsi_psy_get_scope(struct ucsi_connector *con,
> >  	return 0;
> >  }
> >  
> > +static int ucsi_psy_get_status(struct ucsi_connector *con,
> > +			       union power_supply_propval *val)
> > +{
> > +	val->intval = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > +	if (con->status.flags & UCSI_CONSTAT_CONNECTED) {
> > +		if ((con->status.flags & UCSI_CONSTAT_PWR_DIR) == TYPEC_SINK)
> > +			val->intval = POWER_SUPPLY_STATUS_CHARGING;
> > +		else
> > +			val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int ucsi_psy_get_online(struct ucsi_connector *con,
> >  			       union power_supply_propval *val)
> >  {
> > @@ -249,6 +264,8 @@ static int ucsi_psy_get_prop(struct power_supply *psy,
> >  		return ucsi_psy_get_current_now(con, val);
> >  	case POWER_SUPPLY_PROP_SCOPE:
> >  		return ucsi_psy_get_scope(con, val);
> > +	case POWER_SUPPLY_PROP_STATUS:
> > +		return ucsi_psy_get_status(con, val);
> >  	default:
> >  		return -EINVAL;
> >  	}
> > -- 
> > 2.45.2.1089.g2a221341d9-goog
> > 
> 
> -- 
> With best wishes
> Dmitry

Attachment: signature.asc
Description: PGP signature