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

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

[Saranya Gopal]

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(-)

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,
-- 
2.25.1

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

[Greg KH]

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?

> 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_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.

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.

And adding a new sysfs entry does not "Fix" anything, why is this tagged
as such?

thanks,

greg k-h

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

[Gopal, Saranya]

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 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.

> 
> 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.

Thanks,
Saranya
> 
> thanks,
> 
> greg k-h

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

[Heikki Krogerus]

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