Re: [PATCH] usb: typec: pd: Add higher_capability sysfs for sink PDO

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

Hi,

On Sun, Feb 12, 2023 at 04:13:22PM +0000, Gopal, Saranya wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: Sunday, February 12, 2023 9:05 PM
> > To: Gopal, Saranya <saranya.gopal@intel.com>
> > Cc: linux-usb@vger.kernel.org; Heikki Krogerus
> > <heikki.krogerus@linux.intel.com>; Regupathy, Rajaram
> > <rajaram.regupathy@intel.com>
> > 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@linux.intel.com>
> > 
> > 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@intel.com>
> > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com>
> > > ---
> > >  .../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@linux.intel.com>
> > >  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@linux.intel.com>
> > > +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