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:

quoted

On Tue, Oct 26, 2021 at 11:48:42PM -0700, Jack Pham wrote:

quoted

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

quoted

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