| Message ID | 20230213140522.171578-1-saranya.gopal@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] usb: typec: pd: Add higher_capability sysfs for sink PDO | expand |
On Mon, Feb 13, 2023 at 07:35:22PM +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") > Reported-by: Rajaram Regupathy <rajaram.regupathy@intel.com> > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > --- > Changed from v1: > - Added the valid values for the sysfs > - Wrapped the description alone to 80 characters > > .../ABI/testing/sysfs-class-usb_power_delivery | 11 ++++++++++- > drivers/usb/typec/pd.c | 9 ++++++++- > 2 files changed, 18 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..1bf9d1d7902c 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 How does this relate to this specific change? You didn't mention it in the changelog at all :( > Date: May 2022 > Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Description: > @@ -78,6 +78,15 @@ 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. > + Valid values: 0, 1 > + > 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, So you deleted an attribute from this device, ok, but again, I don't understand how this is considered a "fix" other than perhaps the old attribute does not relate to this device? And if so, then make that a single patch, with a Fixes: tag, and we can properly backport that one, and then have a second patch that adds the new attribute. Again, adding a new attribute is a "new feature" not a fix, right? thanks, greg k-h
Hi Greg, > -----Original Message----- > From: Greg KH <gregkh@linuxfoundation.org> > Sent: Monday, February 13, 2023 8:50 PM > To: Gopal, Saranya <saranya.gopal@intel.com> > Cc: linux-usb@vger.kernel.org; heikki.krogerus@linux.intel.com; > Regupathy, Rajaram <rajaram.regupathy@intel.com> > Subject: Re: [PATCH v2] usb: typec: pd: Add higher_capability sysfs > for sink PDO > > On Mon, Feb 13, 2023 at 07:35:22PM +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") > > Reported-by: Rajaram Regupathy <rajaram.regupathy@intel.com> > > Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> > > --- > > Changed from v1: > > - Added the valid values for the sysfs > > - Wrapped the description alone to 80 characters > > > > .../ABI/testing/sysfs-class-usb_power_delivery | 11 > ++++++++++- > > drivers/usb/typec/pd.c | 9 ++++++++- > > 2 files changed, 18 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..1bf9d1d7902c 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 > > How does this relate to this specific change? You didn't mention it > in > the changelog at all :( It is related because source PDO still needs this sysfs. The sink PDO needs the new sysfs. I wanted to make it clear by specifying source_capabilities here. Sorry that my commit message was not clear. I will fix it in v3. > > > Date: May 2022 > > Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Description: > > @@ -78,6 +78,15 @@ 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. > > + Valid values: 0, 1 > > + > > What: > /sys/class/usb_power_delivery/.../<capability>/1:fixed_sup > ply/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, > > So you deleted an attribute from this device, ok, but again, I don't > understand how this is considered a "fix" other than perhaps the old > attribute does not relate to this device? As per USB PD specification: 28th bit of the fixed supply "sink" PDO represents higher capability (whether sink device needs more than vSafe5V (eg:12 V) for full functionality). 28th bit of the fixed supply "source" PDO represents usb_suspend_supported attribute. Before this patch, 28th bit of sink PDO was wrongly representing usb_suspend_supported attribute. We had to add the new attribute to correctly represent the 28th bit of sink PDO. Does it make sense to add fixes tag now if I add these additional details in commit message? Or do you still prefer not to add fixes tag for this patch? Thanks, Saranya > > And if so, then make that a single patch, with a Fixes: tag, and we > can > properly backport that one, and then have a second patch that adds > the > new attribute. > > Again, adding a new attribute is a "new feature" not a fix, right? > > thanks, > > greg k-h
On Mon, Feb 13, 2023 at 06:00:30PM +0000, Gopal, Saranya wrote: > > So you deleted an attribute from this device, ok, but again, I don't > > understand how this is considered a "fix" other than perhaps the old > > attribute does not relate to this device? > As per USB PD specification: > 28th bit of the fixed supply "sink" PDO represents higher capability (whether sink device needs > more than vSafe5V (eg:12 V) for full functionality). > 28th bit of the fixed supply "source" PDO represents usb_suspend_supported attribute. > Before this patch, 28th bit of sink PDO was wrongly representing usb_suspend_supported > attribute. We had to add the new attribute to correctly represent the 28th bit of sink PDO. > Does it make sense to add fixes tag now if I add these additional details in commit message? > Or do you still prefer not to add fixes tag for this patch? > > Thanks, > Saranya > > > > > And if so, then make that a single patch, with a Fixes: tag, and we > > can > > properly backport that one, and then have a second patch that adds > > the > > new attribute. > > > > Again, adding a new attribute is a "new feature" not a fix, right? This is true. Saranya, please split the patch in two like Greg pointed out. thanks,
diff --git a/Documentation/ABI/testing/sysfs-class-usb_power_delivery b/Documentation/ABI/testing/sysfs-class-usb_power_delivery index ce2b1b563cb3..1bf9d1d7902c 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,15 @@ 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. + Valid values: 0, 1 + 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") Reported-by: Rajaram Regupathy <rajaram.regupathy@intel.com> Signed-off-by: Saranya Gopal <saranya.gopal@intel.com> --- Changed from v1: - Added the valid values for the sysfs - Wrapped the description alone to 80 characters .../ABI/testing/sysfs-class-usb_power_delivery | 11 ++++++++++- drivers/usb/typec/pd.c | 9 ++++++++- 2 files changed, 18 insertions(+), 2 deletions(-)