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

[Jack Pham <quic_jackp@quicinc.com>]

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?

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