Hi Greg,
> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Monday, February 13, 2023 8:50 PM
> To: Gopal, Saranya <saranya.gopal@xxxxxxxxx>
> Cc: linux-usb@xxxxxxxxxxxxxxx; heikki.krogerus@xxxxxxxxxxxxxxx;
> Regupathy, Rajaram <rajaram.regupathy@xxxxxxxxx>
> 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@xxxxxxxxx>
> > Signed-off-by: Saranya Gopal <saranya.gopal@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxx>
> > 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@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.
> > + 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@xxxxxxxxxxxxxxx>
> > 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
