| Message ID | 20230212144838.128595-1-saranya.gopal@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: typec: pd: Add higher_capability sysfs for sink PDO | expand |
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,
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_supply/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. + What: /sys/class/usb_power_delivery/.../<capability>/1:fixed_supply/unconstrained_power Date: May 2022 Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com> diff --git a/drivers/usb/typec/pd.c b/drivers/usb/typec/pd.c index dc72005d68db..59c537a5e600 100644 --- a/drivers/usb/typec/pd.c +++ b/drivers/usb/typec/pd.c @@ -48,6 +48,13 @@ usb_suspend_supported_show(struct device *dev, struct device_attribute *attr, ch } static DEVICE_ATTR_RO(usb_suspend_supported); +static ssize_t +higher_capability_show(struct device *dev, struct device_attribute *attr, char *buf) +{ + return sysfs_emit(buf, "%u\n", !!(to_pdo(dev)->pdo & PDO_FIXED_HIGHER_CAP)); +} +static DEVICE_ATTR_RO(higher_capability); + static ssize_t unconstrained_power_show(struct device *dev, struct device_attribute *attr, char *buf) { @@ -161,7 +168,7 @@ static struct device_type source_fixed_supply_type = { static struct attribute *sink_fixed_supply_attrs[] = { &dev_attr_dual_role_power.attr, - &dev_attr_usb_suspend_supported.attr, + &dev_attr_higher_capability.attr, &dev_attr_unconstrained_power.attr, &dev_attr_usb_communication_capable.attr, &dev_attr_dual_role_data.attr,
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> 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(-)