linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: typec: Add a sysfs node to manage port type
@ 2017-05-22 20:05 Badhri Jagan Sridharan
  2017-05-23 10:46 ` Heikki Krogerus
  0 siblings, 1 reply; 5+ messages in thread
From: Badhri Jagan Sridharan @ 2017-05-22 20:05 UTC (permalink / raw)
  To: Heikki Krogerus, Greg Kroah-Hartman
  Cc: linux-usb, linux-kernel, Guenter Roeck, Oliver Neukum,
	Badhri Jagan Sridharan

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@google.com>
---
 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@linux.intel.com>
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",
+};
+
 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);
+
 static const char * const typec_pwr_opmodes[] = {
 	[TYPEC_PWR_MODE_USB]	= "default",
 	[TYPEC_PWR_MODE_1_5A]	= "1.5A",
@@ -1035,6 +1074,7 @@ static struct attribute *typec_attrs[] = {
 	&dev_attr_usb_power_delivery_revision.attr,
 	&dev_attr_usb_typec_revision.attr,
 	&dev_attr_vconn_source.attr,
+	&dev_attr_port_type.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(typec);
diff --git a/include/linux/usb/typec.h b/include/linux/usb/typec.h
index ec78204964ab..5badf6f66479 100644
--- a/include/linux/usb/typec.h
+++ b/include/linux/usb/typec.h
@@ -190,6 +190,7 @@ struct typec_partner_desc {
  * @pr_set: Set Power Role
  * @vconn_set: Set VCONN Role
  * @activate_mode: Enter/exit given Alternate Mode
+ * @port_type_set: Set port type
  *
  * Static capabilities of a single USB Type-C port.
  */
@@ -214,6 +215,9 @@ struct typec_capability {
 
 	int		(*activate_mode)(const struct typec_capability *,
 					 int mode, int activate);
+
+	int		(*port_type_set)(const struct typec_capability *,
+					enum typec_port_type);
 };
 
 /* Specific to try_role(). Indicates the user want's to clear the preference. */
-- 
2.13.0.303.g4ebf302169-goog

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: Add a sysfs node to manage port type
  2017-05-22 20:05 [PATCH] usb: typec: Add a sysfs node to manage port type Badhri Jagan Sridharan
@ 2017-05-23 10:46 ` Heikki Krogerus
  2017-05-23 13:16   ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: Heikki Krogerus @ 2017-05-23 10:46 UTC (permalink / raw)
  To: Badhri Jagan Sridharan
  Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Guenter Roeck,
	Oliver Neukum

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@google.com>
> ---
>  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@linux.intel.com>
> 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.

I do like the idea of being able to fix the role, assuming others are
OK with it too.


Thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: Add a sysfs node to manage port type
  2017-05-23 10:46 ` Heikki Krogerus
@ 2017-05-23 13:16   ` Guenter Roeck
  2017-05-23 13:39     ` Heikki Krogerus
  0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2017-05-23 13:16 UTC (permalink / raw)
  To: Heikki Krogerus, Badhri Jagan Sridharan
  Cc: Greg Kroah-Hartman, linux-usb, linux-kernel, Oliver Neukum

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@google.com>
>> ---
>>   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@linux.intel.com>
>> 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.
> 
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 do like the idea of being able to fix the role, assuming others are
> OK with it too.
> 

I am definitely ok with it.

Thanks,
Guenter

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: Add a sysfs node to manage port type
  2017-05-23 13:16   ` Guenter Roeck
@ 2017-05-23 13:39     ` Heikki Krogerus
  2017-05-23 14:06       ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: Heikki Krogerus @ 2017-05-23 13:39 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Badhri Jagan Sridharan, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Oliver Neukum

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@google.com>
> > > ---
> > >   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@linux.intel.com>
> > > 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.


Thanks,

-- 
heikki

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] usb: typec: Add a sysfs node to manage port type
  2017-05-23 13:39     ` Heikki Krogerus
@ 2017-05-23 14:06       ` Guenter Roeck
  0 siblings, 0 replies; 5+ messages in thread
From: Guenter Roeck @ 2017-05-23 14:06 UTC (permalink / raw)
  To: Heikki Krogerus
  Cc: Badhri Jagan Sridharan, Greg Kroah-Hartman, linux-usb,
	linux-kernel, Oliver Neukum

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@google.com>
>>>> ---
>>>>    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@linux.intel.com>
>>>> 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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2017-05-23 14:06 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-22 20:05 [PATCH] usb: typec: Add a sysfs node to manage port type Badhri Jagan Sridharan
2017-05-23 10:46 ` Heikki Krogerus
2017-05-23 13:16   ` Guenter Roeck
2017-05-23 13:39     ` Heikki Krogerus
2017-05-23 14:06       ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).