Re: [PATCH] usb: typec: ucsi: Only get source PDOs from the actual source

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

On Tue, Nov 02, 2021 at 12:01:31AM -0700, Jack Pham wrote:
> Hi Heikki,
> 
> On Thu, Oct 28, 2021 at 02:55:32PM +0300, Heikki Krogerus wrote:
> > On Tue, Oct 26, 2021 at 11:48:42PM -0700, Jack Pham wrote:
> > > The intent of ucsi_get_src_pdos() is to obtain the source's PDOs
> > > in order to provide the power_supply object the required data to
> > > report the mininum, maximum and currently operating voltage &
> > > current should a PD contract be in place.
> > > 
> > > If however, the port is operating as a PD source, this call would
> > > invoke GET_PDOS on the partner causing the PPM to send a
> > > Get_Source_Caps PD message to the port partner which may not make
> > > sense especially if the partner is not a dual-power role capable
> > > device.  Further, it has been observed that certain DisplayPort
> > > adapter cables (which are power sink-only devices) even fail to
> > > bring up the display link after receiving a Get_Source_Caps
> > > message, suggesting they can't cope well with an unsupported PD
> > > message to the point that it renders them functionally inoperable.
> > > 
> > > Fix this by checking the connector status flags for the power
> > > direction and use this to decide whether to send the GET_PDOs
> > > query to the partner or the port.  This also helps to make the
> > > power_supply VOLTAGE_{MIN,MAX,NOW} and CURRENT_{MAX,NOW}
> > > properties more consistent when the port is in source mode.
> > > 
> > > Signed-off-by: Jack Pham <quic_jackp@quicinc.com>
> > > ---
> > > Hi Heikki,
> > > 
> > > Was wrestling with how exactly to do this.  The other approach I was
> > > thinking was to not even do GET_PDOs at all if operating as a source,
> > > but that would also mean we'd need to add similar checking to the
> > > VOLTAGE/CURRENT property getters in psy.c so that they would not
> > > return incorrect/stale data.  Since the ONLINE property will already
> > > be 0 anyway it may make more sense to invalidate the rest of the props?
> > > 
> > > The patch below is concise though...so that's what I went with ;)
> > 
> > Would it still make sense / help if we had separate power supplies
> > registered for the port-source, port-sink, partner-source and
> > partner-sink?
> 
> The name "power_supply" of the class to me implies a source :). So I
> can see perhaps having separate port-source and partner-source supplies
> making sense; they would effectively be mutually exclusive as only one
> of the pair would be "online" at a time when a partner is present
> depending on the power direction.
> 
> But I'm not sure I see a use for having port-sink/partner-sink objects
> though.  While the VOLTAGE_NOW / CURRENT_NOW properties might be
> redundantly reporting the same value from corresponding -source objects,
> would there be different MIN/MAX values which are obtained from
> Get_Sink_Caps?

A power-supply can be a supply, supplicant or both, and that aligns
perfectly with source/sink IMO. By taking advantage of that, you could
display the whole power-supply chain to the user space.

So if you had a port-sink power-supply online and partner-source
online - the partner supplying the port - then in reality the
port-sink power-supply would/could also be the "supply" for another
power-supply, for example the battery. That's the idea with the
power-supply chain.

Nevertheless, I guess there does not have to be separate source and
sink power-supplies. If we just had separate port and partner
power-supplies, we could also possibly use the "status" property to
see which one is sink and which is the source (charging or
discharging). Though, I don't think that would be ideal.

> > I also think we need to unify these power supplies so that tcpm and
> > ucsi (and others) always register the same power supplies.
> 
> I like that idea too!


thanks,

-- 
heikki