Re: [PATCH v2] usb: typec: Expose Product Type VDOs via sysfs

[Benson Leung]

Hi Heikki,

On Thu, Oct 22, 2020 at 5:43 AM Heikki Krogerus
<heikki.krogerus@xxxxxxxxxxxxxxx> wrote:
>
> On Thu, Oct 22, 2020 at 12:25:07AM -0700, Prashant Malani wrote:
> > Hi Greg,
> >
> > On Thu, Oct 22, 2020 at 12:17 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > > > > +What:                /sys/class/typec/<port>-partner/identity/product_type_vdo
> > > > > > +Date:                October 2020
> > > > > > +Contact:     Prashant Malani <pmalani@xxxxxxxxxxxx>
> > > > > > +Description:
> > > > > > +             Product Type VDOs part of Discover Identity command result. 3 values
> > > > > > +             are displayed (for the 3 possible Product Type VDOs), one per line.
> > > > >
> > > > > sysfs is "one value per file", not "one value per line".  This is not
> > > > > ok.
> > > >
> > > > I see. Would listing these out as three separate vdos (i.e vdo0, vdo1,
> > > > vdo2) be better?
> > >
> > > Given that your current implementation is not acceptable, something has
> > > to change :)
> >
> > Got it. I'd like to see if Heikki has any suggestions on naming these
> > entries better.
>
> Why not have product type specific attribute files?
>
> So if the partner is UFP, then we expose ufp1 and ufp2 files that
> return the UFP1 and UFP2 VDO values and hide the other files:
>
>         % ls /sys/class/typec/port0-partner/identity/
>         id_header cert_stat product ufp1 ufp2
>
> If the partner is DFP, then you expose the dfp file and hide
> everything else:
>
>         % ls /sys/class/typec/port0-partner/identity/
>         id_header cert_stat product dfp
>
> And so on.

I would caution against any decoding of the VDO contents in the kernel
and making assumptions about the # or the names of these three
individual objects.

Since PD 2.0 through PD 3.0, and PD 3.0's different subrevisions (1.0,
1.3, 2.0), the # of VDOs that have been supported has changed in the
various spec versions.

PD R3.0 V2.0 actually added extra objects here (UFP VDO1 UFP VDO2, DFP
VDO), but thanks to some troublemaker (me, actually...), the PD spec's
next version deprecates and deletes two of them (the AMA VDO and the
UFP VDO2 are gone, thanks to an ECR I put into USB PD).

(If you've got USB PD working group access, the two ECRs in question
are: https://groups.usb.org/wg/powerdelivery/document/11007 and
https://groups.usb.org/wg/powerdelivery/document/10967).

Since the different spec versions need to all be supported (since the
firmware of PD devices are baked for a particular version of the PD
spec at the time they are released and don't change in practice), the
software on USB PD hosts should provide these objects up to the next
layer without adding any extra decoding, and the upper layer
(userspace) can figure out the specifics based on comparing different
revision and version fields to figure out what vdo1, vdo2, and vdo3
are.

Anyway, hope this helps, and sorry in advance for making this section
of the PD spec more complicated to handle over time...

Benson
>
> thanks,
>
> --
> heikki



-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@xxxxxxxxxx
Chromium OS Project
bleung@xxxxxxxxxxxx