Hi,
On Sun, Feb 12, 2023 at 04:13:22PM +0000, Gopal, Saranya wrote:
> Hi Greg,
>
> > -----Original Message-----
> > From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Sent: Sunday, February 12, 2023 9:05 PM
> > To: Gopal, Saranya <saranya.gopal@xxxxxxxxx>
> > Cc: linux-usb@xxxxxxxxxxxxxxx; Heikki Krogerus
> > <heikki.krogerus@xxxxxxxxxxxxxxx>; Regupathy, Rajaram
> > <rajaram.regupathy@xxxxxxxxx>
> > Subject: Re: [PATCH] usb: typec: pd: Add higher_capability sysfs for
> > sink PDO
> >
> > On Sun, Feb 12, 2023 at 08:18:38PM +0530, Saranya Gopal wrote:
> > > As per USB PD specification, 28th bit of sink fixed power supply
> > > PDO represents higher capability. If this bit is set, it indicates
> > > that the sink needs more than vsafe5V to provide full functionality.
> > > This patch replaces usb_suspend_supported sysfs with
> > higher_capability
> > > sysfs for sink PDO.
> > >
> > > Fixes: 662a60102c12 ("usb: typec: Separate USB Power Delivery
> > from USB Type-C")
> > > Reviewed-by: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> >
> > Where was this reviewed? I don't see that on the list, am I missing
> > it?
> It was reviewed internally in Intel's internal mailing list.
>
> >
> > > Reported-by: Rajaram Regupathy <rajaram.regupathy@xxxxxxxxx>
> > > Signed-off-by: Saranya Gopal <saranya.gopal@xxxxxxxxx>
> > > ---
> > > .../ABI/testing/sysfs-class-usb_power_delivery | 10
> > +++++++++-
> > > drivers/usb/typec/pd.c | 9 ++++++++-
> > > 2 files changed, 17 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/Documentation/ABI/testing/sysfs-class-
> > usb_power_delivery b/Documentation/ABI/testing/sysfs-class-
> > usb_power_delivery
> > > index ce2b1b563cb3..59757118b6a3 100644
> > > --- a/Documentation/ABI/testing/sysfs-class-usb_power_delivery
> > > +++ b/Documentation/ABI/testing/sysfs-class-usb_power_delivery
> > > @@ -69,7 +69,7 @@ Description:
> > > This file contains boolean value that tells does the
> > device
> > > support both source and sink power roles.
> > >
> > > -What:
> > /sys/class/usb_power_delivery/.../<capability>/1:fixed_sup
> > ply/usb_suspend_supported
> > > +What: /sys/class/usb_power_delivery/.../source-
> > capabilities/1:fixed_supply/usb_suspend_supported
> > > Date: May 2022
> > > Contact: Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > Description:
> > > @@ -78,6 +78,14 @@ Description:
> > > will follow the USB 2.0 and USB 3.2 rules for
> > suspend and
> > > resume.
> > >
> > > +What: /sys/class/usb_power_delivery/.../sink-
> > capabilities/1:fixed_supply/higher_capability
> > > +Date: February 2023
> > > +Contact: Saranya Gopal <saranya.gopal@xxxxxxxxxxxxxxx>
> > > +Description:
> > > + This file shows the value of the Higher capability bit
> > in vsafe5V Fixed Supply Object.
> > > + If the bit is set, then the sink needs more than
> > vsafe5V(eg. 12 V) to provide
> > > + full functionality.
> >
> > You don't explain what this file will show. 0/1? Y/N? J/N?
> >
> > Also, properly wrap your lines at 80 columns for documentation
> > please.
> This sysfs will show 0/1 value.
I think we need to fix this one. Although, the description does
clearly say that this file shows the value of a bit, it's still better
to separately show the possible values - so 0 or 1.
> I have tried to maintain consistency with the rest of the file.
> (https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-usb_power_delivery)
> That is why I did not add the 0/1 value and also did not wrap the lines to 80.
> I can fix these in v2 if you are not convinced.
But the descriptions are wrapped at 80 characters in that document?
> > And adding a new sysfs entry does not "Fix" anything, why is this
> > tagged
> > as such?
> Sink fixed supply PDO wrongly shows usb_suspend_supported sysfs instead of higher_capability sysfs.
> Sink PDO does not have this 'usb_suspend_supported' bit.
> Only source fixed supply PDO has this bit. So, this patch adds higher_capability bit support for sink PDO.
> That is why 'fixes' tag was added.
Yep. So the value was assigned to the wrong sysfs file. I do think
this is a fix.
Br,
--
heikki
