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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Tue, Jun 18, 2024 at 10:01:21AM -0500, Mario Limonciello wrote:
> 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.

That works for me. I'll change it to POWER_SUPPLY_CHARGE_TYPE_UNKNOWN
in that case.

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

thanks,

-- 
heikki