Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20201022061554.3418060-1-pmalani@chromium.org> <20201022065719.GA1440360@kroah.com>
In-Reply-To: <20201022065719.GA1440360@kroah.com>
From:   Prashant Malani <pmalani@chromium.org>
Date:   Thu, 22 Oct 2020 00:13:54 -0700
Message-ID: <CACeCKacvhtSfQ=hGYHi3AdrTT+XY2RpKmPHuYWoxNVmRWMeBBA@mail.gmail.com>
Subject: Re: [PATCH v2] usb: typec: Expose Product Type VDOs via sysfs
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:USB NETWORKING DRIVERS" <linux-usb@vger.kernel.org>,
        Benson Leung <bleung@chromium.org>,
        Heikki Krogerus <heikki.krogerus@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thanks for reviewing the patch, Greg.

On Wed, Oct 21, 2020 at 11:56 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 21, 2020 at 11:15:54PM -0700, Prashant Malani wrote:
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > index b834671522d6..16440a236b66 100644
> > --- a/Documentation/ABI/testing/sysfs-class-typec
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -170,6 +170,14 @@ Description:
> >               will show 0 until Discover Identity command result becomes
> >               available. The value can be polled.
> >
> > +What:                /sys/class/typec/<port>-partner/identity/product_type_vdo
> > +Date:                October 2020
> > +Contact:     Prashant Malani <pmalani@chromium.org>
> > +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?
>
> > +             The values will show 0s until Discover Identity command result becomes
> > +             available. The values can be polled.
>
> It can be polled?  Did you try that?  I don't see the logic for that in
> your patch.

To be honest, no. I followed the pattern used by the other identity
VDOs (cert_stat, id_header and product),
and re-used their description assuming it to be accurate.

>
>
> >
> >  USB Type-C cable devices (eg. /sys/class/typec/port0-cable/)
> >
> > @@ -230,6 +238,15 @@ Description:
> >               will show 0 until Discover Identity command result becomes
> >               available. The value can be polled.
> >
> > +What:                /sys/class/typec/<port>-cable/identity/product_type_vdo
> > +Date:                October 2020
> > +Contact:     Prashant Malani <pmalani@chromium.org>
> > +Description:
> > +             Product Type VDOs part of Discover Identity command result. 3 values
> > +             are displayed (for the 3 possible Product Type VDOs), one per line.
> > +             The values will show 0s until Discover Identity command result becomes
> > +             available. The values can be polled.
>
> Why are you describing the same value in two different locations?

Both cable and partner can have Discover Identity VDOs and they are
listed separately in sysfs.
The other VDOs (id_header, cert_stat and product) have separate
descriptions for cable and partner too.
Perhaps there is a case for consolidating the listings here (factor
out the ones which are common to cable and partner).

>
> > +
> >
> >  USB Type-C port alternate mode devices.
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 35eec707cb51..37fa4501e75f 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -122,10 +122,20 @@ static ssize_t product_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(product);
> >
> > +static ssize_t product_type_vdo_show(struct device *dev, struct device_attribute *attr,
> > +                                  char *buf)
> > +{
> > +     struct usb_pd_identity *id = get_pd_identity(dev);
> > +
> > +     return sprintf(buf, "0x%08x\n0x%08x\n0x%08x\n", id->vdo[0], id->vdo[1], id->vdo[2]);
>
> Note, for future sysfs stuff, always use sysfs_emit().
>
> But again, this is not allowed as you have multiple values per a single
> file.

Noted. I will keep that in mind for future versions.

Best regards,

>
> thanks,
>
> greg k-h