Re: [PATCH] usb: typec: Add a sysfs node to manage port type

[Guenter Roeck]

On 05/23/2017 06:39 AM, Heikki Krogerus wrote:
> On Tue, May 23, 2017 at 06:16:28AM -0700, Guenter Roeck wrote:
> > On 05/23/2017 03:46 AM, Heikki Krogerus wrote:
> > > Hi,
> > >
> > > On Mon, May 22, 2017 at 01:05:42PM -0700, Badhri Jagan Sridharan wrote:
> > > > User space applications in some cases have the need to enforce a
> > > > specific port type(DFP/UFP/DRP). This change allows userspace to
> > > > attempt setting the desired port type. Low level drivers can
> > > > however reject the request if the specific port type is not supported.
> > > >
> > > > Signed-off-by: Badhri Jagan Sridharan <Badhri@xxxxxxxxxx>
> > > > ---
> > > >    Documentation/ABI/testing/sysfs-class-typec | 13 ++++++++++
> > > >    drivers/usb/typec/typec.c                   | 40 +++++++++++++++++++++++++++++
> > > >    include/linux/usb/typec.h                   |  4 +++
> > > >    3 files changed, 57 insertions(+)
> > > >
> > > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > > > index d4a3d23eb09c..853b2ef73641 100644
> > > > --- a/Documentation/ABI/testing/sysfs-class-typec
> > > > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > > > @@ -73,6 +73,19 @@ Description:
> > > >    		Valid values: source, sink, none (to remove preference)
> > > > +What:           /sys/class/typec/<port>/port_type
> > > > +Date:           May 2017
> > > > +Description:
> > > > +		Indicates the type of the port. This attribute can be used for
> > > > +		requesting a change in the port type. Port type change is
> > > > +		supported as a synchronous operation, so write(2) to the
> > > > +		attribute will not return until the operation has finished.
> > > > +
> > > > +		Valid values:
> > > > +		- DRP
> > > > +		- DFP
> > > > +		- UFP
> > > >
> > > >    What:		/sys/class/typec/<port>/supported_accessory_modes
> > > >    Date:		April 2017
> > > >    Contact:	Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>
> > > > diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> > > > index 89e540bb7ff3..684a13bb744d 100644
> > > > --- a/drivers/usb/typec/typec.c
> > > > +++ b/drivers/usb/typec/typec.c
> > > > @@ -789,6 +789,12 @@ static const char * const typec_data_roles[] = {
> > > >    	[TYPEC_HOST]	= "host",
> > > >    };
> > > > +static const char * const typec_port_types[] = {
> > > > +	[TYPEC_PORT_DFP] = "dfp",
> > > > +	[TYPEC_PORT_UFP] = "ufp",
> > > > +	[TYPEC_PORT_DRP] = "drp",
> > > > +};
> > >
> > > The meaning of those abbreviations has changed in every version of the
> > > spec since v1.0 which makes me a bit uncomfortable using them with the
> > > attributes. In USB Type-C specification v1.2, DRP now means
> > > Dual-Role-Power, but DFP and UFP are used with USB data operation.
> > >
> > > I would prefer "source, "sink" and "drp". Actually, I don't even like
> > > "drp". How about "dual" instead?
> > >
> > > >    static ssize_t
> > > >    preferred_role_store(struct device *dev, struct device_attribute *attr,
> > > >    		     const char *buf, size_t size)
> > > > @@ -926,6 +932,39 @@ static ssize_t power_role_show(struct device *dev,
> > > >    }
> > > >    static DEVICE_ATTR_RW(power_role);
> > > > +static ssize_t
> > > > +port_type_store(struct device *dev, struct device_attribute *attr,
> > > > +			const char *buf, size_t size)
> > > > +{
> > > > +	struct typec_port *port = to_typec_port(dev);
> > > > +	int ret;
> > > > +
> > > > +	if (!port->cap->port_type_set) {
> > > > +		dev_dbg(dev, "changing port type not supported\n");
> > > > +		return -EOPNOTSUPP;
> > > > +	}
> > > > +
> > > > +	ret = sysfs_match_string(typec_port_types, buf);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	ret = port->cap->port_type_set(port->cap, ret);
> > > > +	if (ret)
> > > > +		return ret;
> > > > +
> > > > +	return size;
> > > > +}
> > > > +
> > > > +static ssize_t
> > > > +port_type_show(struct device *dev, struct device_attribute *attr,
> > > > +		char *buf)
> > > > +{
> > > > +	struct typec_port *port = to_typec_port(dev);
> > > > +
> > > > +	return sprintf(buf, "%s\n", typec_port_types[port->cap->type]);
> > > > +}
> > > > +static DEVICE_ATTR_RW(port_type);
> > >
> > > This doesn't tell the user the capabilities of the port. All the
> > > supported roles should be listed here like with the other attributes,
> > > the active one in brackets. This probable means we need a small
> > > addition/change to the API too.
>
> Sorry, not the API..
>
> > typec_capability already lists the port type. Presumably it can be
> > restricted to TYPEC_PORT_DFP or TYPEC_PORT_UFP only if it is reported
> > as TYPEC_PORT_DRP. Am I missing something ?
>
> I mean, we should not overwrite the type member in typec_capability.
> DRP capable port is still DRP capable even if we fix it to DFP or UFP.
> So the active, fixed role, should be stored to struct typec_port.
Yes, you are right. Makes sense.

Thanks,
Guenter