[PATCH V2] usb: typec: Add sysfs node to show connector orientation

[PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Puma Hsu]

Export the Type-C connector orientation so that user space
can get this information.

Signed-off-by: Puma Hsu <pumahsu@google.com>
---
 Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
 drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
index d7647b258c3c..b22f71801671 100644
--- a/Documentation/ABI/testing/sysfs-class-typec
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -108,6 +108,17 @@ Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
 Description:
 		Revision number of the supported USB Type-C specification.
 
+What:		/sys/class/typec/<port>/connector_orientation
+Date:		October 2019
+Contact:	Puma Hsu <pumahsu@google.com>
+Description:
+		Indicates which typec connector orientation is configured now.
+		cc1 is defined as "normal" and cc2 is defined as "reversed".
+
+		Valid value:
+		- unknown (nothing configured)
+		- normal (configured in cc1 side)
+		- reversed (configured in cc2 side)
 
 USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
 
diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 94a3eda62add..911d06676aeb 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -1245,6 +1245,23 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(usb_power_delivery_revision);
 
+static const char * const typec_connector_orientation[] = {
+	[TYPEC_ORIENTATION_NONE]		= "unknown",
+	[TYPEC_ORIENTATION_NORMAL]		= "normal",
+	[TYPEC_ORIENTATION_REVERSE]		= "reversed",
+};
+
+static ssize_t connector_orientation_show(struct device *dev,
+						struct device_attribute *attr,
+						char *buf)
+{
+	struct typec_port *p = to_typec_port(dev);
+
+	return sprintf(buf, "%s\n",
+		       typec_connector_orientation[p->orientation]);
+}
+static DEVICE_ATTR_RO(connector_orientation);
+
 static struct attribute *typec_attrs[] = {
 	&dev_attr_data_role.attr,
 	&dev_attr_power_operation_mode.attr,
@@ -1255,6 +1272,7 @@ static struct attribute *typec_attrs[] = {
 	&dev_attr_usb_typec_revision.attr,
 	&dev_attr_vconn_source.attr,
 	&dev_attr_port_type.attr,
+	&dev_attr_connector_orientation.attr,
 	NULL,
 };
 ATTRIBUTE_GROUPS(typec);
-- 
2.23.0.866.gb869b98d4c-goog

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Greg KH]

On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
> Export the Type-C connector orientation so that user space
> can get this information.
> 
> Signed-off-by: Puma Hsu <pumahsu@google.com>
> ---
>  Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
>  drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index d7647b258c3c..b22f71801671 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -108,6 +108,17 @@ Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
>  		Revision number of the supported USB Type-C specification.
>  
> +What:		/sys/class/typec/<port>/connector_orientation
> +Date:		October 2019
> +Contact:	Puma Hsu <pumahsu@google.com>
> +Description:
> +		Indicates which typec connector orientation is configured now.
> +		cc1 is defined as "normal" and cc2 is defined as "reversed".

Why the blank line after "Description:"?  Shouldn't "Indicates..." be
right after it?
> +
> +		Valid value:
> +		- unknown (nothing configured)
> +		- normal (configured in cc1 side)
> +		- reversed (configured in cc2 side)
>  
>  USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
>  
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 94a3eda62add..911d06676aeb 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1245,6 +1245,23 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(usb_power_delivery_revision);
>  
> +static const char * const typec_connector_orientation[] = {
> +	[TYPEC_ORIENTATION_NONE]		= "unknown",
> +	[TYPEC_ORIENTATION_NORMAL]		= "normal",
> +	[TYPEC_ORIENTATION_REVERSE]		= "reversed",
> +};
> +
> +static ssize_t connector_orientation_show(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)

Can you line this up properly?

thanks,

greg k-h

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Puma Hsu]

Hi Greg,


On Wed, Oct 23, 2019 at 1:27 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
> > Export the Type-C connector orientation so that user space
> > can get this information.
> >
> > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
> >  drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
> >  2 files changed, 29 insertions(+)
> >
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > index d7647b258c3c..b22f71801671 100644
> > --- a/Documentation/ABI/testing/sysfs-class-typec
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -108,6 +108,17 @@ Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >  Description:
> >               Revision number of the supported USB Type-C specification.
> >
> > +What:                /sys/class/typec/<port>/connector_orientation
> > +Date:                October 2019
> > +Contact:     Puma Hsu <pumahsu@google.com>
> > +Description:
> > +             Indicates which typec connector orientation is configured now.
> > +             cc1 is defined as "normal" and cc2 is defined as "reversed".
>
> Why the blank line after "Description:"?  Shouldn't "Indicates..." be
> right after it?

I checked the coding style for sysfs-class-*, all of them put the
description at the next line behind "Description:"
Should I change it?

> > +
> > +             Valid value:
> > +             - unknown (nothing configured)
> > +             - normal (configured in cc1 side)
> > +             - reversed (configured in cc2 side)
> >
> >  USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
> >
> > diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> > index 94a3eda62add..911d06676aeb 100644
> > --- a/drivers/usb/typec/class.c
> > +++ b/drivers/usb/typec/class.c
> > @@ -1245,6 +1245,23 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
> >  }
> >  static DEVICE_ATTR_RO(usb_power_delivery_revision);
> >
> > +static const char * const typec_connector_orientation[] = {
> > +     [TYPEC_ORIENTATION_NONE]                = "unknown",
> > +     [TYPEC_ORIENTATION_NORMAL]              = "normal",
> > +     [TYPEC_ORIENTATION_REVERSE]             = "reversed",
> > +};
> > +
> > +static ssize_t connector_orientation_show(struct device *dev,
> > +                                             struct device_attribute *attr,
> > +                                             char *buf)
>
> Can you line this up properly?

Yes, I will update it in version3 once the previous problem is confirmed.

>
> thanks,
>
> greg k-h


Thanks in advance.
Puma Hsu

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Heikki Krogerus]

+Guenter

On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
> Export the Type-C connector orientation so that user space
> can get this information.
> 
> Signed-off-by: Puma Hsu <pumahsu@google.com>
> ---
>  Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
>  drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
>  2 files changed, 29 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> index d7647b258c3c..b22f71801671 100644
> --- a/Documentation/ABI/testing/sysfs-class-typec
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -108,6 +108,17 @@ Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>  Description:
>  		Revision number of the supported USB Type-C specification.
>  
> +What:		/sys/class/typec/<port>/connector_orientation
> +Date:		October 2019
> +Contact:	Puma Hsu <pumahsu@google.com>
> +Description:
> +		Indicates which typec connector orientation is configured now.
> +		cc1 is defined as "normal" and cc2 is defined as "reversed".
> +
> +		Valid value:
> +		- unknown (nothing configured)

"unknown" means we do not know the orientation.

> +		- normal (configured in cc1 side)
> +		- reversed (configured in cc2 side)

Guenter, do you think "connector_orientation" OK. I proposed it, but
I'm now wondering if something like "polarity" would be better?

>  USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
>  
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 94a3eda62add..911d06676aeb 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -1245,6 +1245,23 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(usb_power_delivery_revision);
>  
> +static const char * const typec_connector_orientation[] = {
> +	[TYPEC_ORIENTATION_NONE]		= "unknown",
> +	[TYPEC_ORIENTATION_NORMAL]		= "normal",
> +	[TYPEC_ORIENTATION_REVERSE]		= "reversed",
> +};
> +
> +static ssize_t connector_orientation_show(struct device *dev,
> +						struct device_attribute *attr,
> +						char *buf)
> +{
> +	struct typec_port *p = to_typec_port(dev);
> +
> +	return sprintf(buf, "%s\n",
> +		       typec_connector_orientation[p->orientation]);
> +}
> +static DEVICE_ATTR_RO(connector_orientation);
> +
>  static struct attribute *typec_attrs[] = {
>  	&dev_attr_data_role.attr,
>  	&dev_attr_power_operation_mode.attr,
> @@ -1255,6 +1272,7 @@ static struct attribute *typec_attrs[] = {
>  	&dev_attr_usb_typec_revision.attr,
>  	&dev_attr_vconn_source.attr,
>  	&dev_attr_port_type.attr,
> +	&dev_attr_connector_orientation.attr,
>  	NULL,
>  };
>  ATTRIBUTE_GROUPS(typec);

thanks,

-- 
heikki

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Guenter Roeck]

On 10/23/19 1:32 AM, Heikki Krogerus wrote:
> +Guenter
> 
> On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
>> Export the Type-C connector orientation so that user space
>> can get this information.
>>
>> Signed-off-by: Puma Hsu <pumahsu@google.com>
>> ---
>>   Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
>>   drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
>>   2 files changed, 29 insertions(+)
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
>> index d7647b258c3c..b22f71801671 100644
>> --- a/Documentation/ABI/testing/sysfs-class-typec
>> +++ b/Documentation/ABI/testing/sysfs-class-typec
>> @@ -108,6 +108,17 @@ Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>   Description:
>>   		Revision number of the supported USB Type-C specification.
>>   
>> +What:		/sys/class/typec/<port>/connector_orientation
>> +Date:		October 2019
>> +Contact:	Puma Hsu <pumahsu@google.com>
>> +Description:
>> +		Indicates which typec connector orientation is configured now.
>> +		cc1 is defined as "normal" and cc2 is defined as "reversed".
>> +
>> +		Valid value:
>> +		- unknown (nothing configured)
> 
> "unknown" means we do not know the orientation.
> 
>> +		- normal (configured in cc1 side)
>> +		- reversed (configured in cc2 side)
> 
> Guenter, do you think "connector_orientation" OK. I proposed it, but
> I'm now wondering if something like "polarity" would be better?
> 

Yes, or just "orientation". I don't see the value in the "connector_" prefix.
I also wonder if "unknown" is really correct. Is it really unknown, or
does it mean that the port is disconnected ?

Guenter

>>   USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
>>   
>> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
>> index 94a3eda62add..911d06676aeb 100644
>> --- a/drivers/usb/typec/class.c
>> +++ b/drivers/usb/typec/class.c >> @@ -1245,6 +1245,23 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
>>   }
>>   static DEVICE_ATTR_RO(usb_power_delivery_revision);
>>   
>> +static const char * const typec_connector_orientation[] = {
>> +	[TYPEC_ORIENTATION_NONE]		= "unknown",
>> +	[TYPEC_ORIENTATION_NORMAL]		= "normal",
>> +	[TYPEC_ORIENTATION_REVERSE]		= "reversed",
>> +};
>> +
>> +static ssize_t connector_orientation_show(struct device *dev,
>> +						struct device_attribute *attr,
>> +						char *buf)
>> +{
>> +	struct typec_port *p = to_typec_port(dev);
>> +
>> +	return sprintf(buf, "%s\n",
>> +		       typec_connector_orientation[p->orientation]);
>> +}
>> +static DEVICE_ATTR_RO(connector_orientation);
>> +
>>   static struct attribute *typec_attrs[] = {
>>   	&dev_attr_data_role.attr,
>>   	&dev_attr_power_operation_mode.attr,
>> @@ -1255,6 +1272,7 @@ static struct attribute *typec_attrs[] = {
>>   	&dev_attr_usb_typec_revision.attr,
>>   	&dev_attr_vconn_source.attr,
>>   	&dev_attr_port_type.attr,
>> +	&dev_attr_connector_orientation.attr,
>>   	NULL,
>>   };
>>   ATTRIBUTE_GROUPS(typec);
> 
> thanks,
> 

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Puma Hsu]

Hi Guenter,

I think "unknown" here means that the USB port is disconnected in real usage.

Thanks in advance.
Puma Hsu







On Wed, Oct 23, 2019 at 9:44 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 10/23/19 1:32 AM, Heikki Krogerus wrote:
> > +Guenter
> >
> > On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
> >> Export the Type-C connector orientation so that user space
> >> can get this information.
> >>
> >> Signed-off-by: Puma Hsu <pumahsu@google.com>
> >> ---
> >>   Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
> >>   drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
> >>   2 files changed, 29 insertions(+)
> >>
> >> diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> >> index d7647b258c3c..b22f71801671 100644
> >> --- a/Documentation/ABI/testing/sysfs-class-typec
> >> +++ b/Documentation/ABI/testing/sysfs-class-typec
> >> @@ -108,6 +108,17 @@ Contact:        Heikki Krogerus <heikki.krogerus@linux.intel.com>
> >>   Description:
> >>              Revision number of the supported USB Type-C specification.
> >>
> >> +What:               /sys/class/typec/<port>/connector_orientation
> >> +Date:               October 2019
> >> +Contact:    Puma Hsu <pumahsu@google.com>
> >> +Description:
> >> +            Indicates which typec connector orientation is configured now.
> >> +            cc1 is defined as "normal" and cc2 is defined as "reversed".
> >> +
> >> +            Valid value:
> >> +            - unknown (nothing configured)
> >
> > "unknown" means we do not know the orientation.
> >
> >> +            - normal (configured in cc1 side)
> >> +            - reversed (configured in cc2 side)
> >
> > Guenter, do you think "connector_orientation" OK. I proposed it, but
> > I'm now wondering if something like "polarity" would be better?
> >
>
> Yes, or just "orientation". I don't see the value in the "connector_" prefix.
> I also wonder if "unknown" is really correct. Is it really unknown, or
> does it mean that the port is disconnected ?
>
> Guenter
>
> >>   USB Type-C partner devices (eg. /sys/class/typec/port0-partner/)
> >>
> >> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> >> index 94a3eda62add..911d06676aeb 100644
> >> --- a/drivers/usb/typec/class.c
> >> +++ b/drivers/usb/typec/class.c >> @@ -1245,6 +1245,23 @@ static ssize_t usb_power_delivery_revision_show(struct device *dev,
> >>   }
> >>   static DEVICE_ATTR_RO(usb_power_delivery_revision);
> >>
> >> +static const char * const typec_connector_orientation[] = {
> >> +    [TYPEC_ORIENTATION_NONE]                = "unknown",
> >> +    [TYPEC_ORIENTATION_NORMAL]              = "normal",
> >> +    [TYPEC_ORIENTATION_REVERSE]             = "reversed",
> >> +};
> >> +
> >> +static ssize_t connector_orientation_show(struct device *dev,
> >> +                                            struct device_attribute *attr,
> >> +                                            char *buf)
> >> +{
> >> +    struct typec_port *p = to_typec_port(dev);
> >> +
> >> +    return sprintf(buf, "%s\n",
> >> +                   typec_connector_orientation[p->orientation]);
> >> +}
> >> +static DEVICE_ATTR_RO(connector_orientation);
> >> +
> >>   static struct attribute *typec_attrs[] = {
> >>      &dev_attr_data_role.attr,
> >>      &dev_attr_power_operation_mode.attr,
> >> @@ -1255,6 +1272,7 @@ static struct attribute *typec_attrs[] = {
> >>      &dev_attr_usb_typec_revision.attr,
> >>      &dev_attr_vconn_source.attr,
> >>      &dev_attr_port_type.attr,
> >> +    &dev_attr_connector_orientation.attr,
> >>      NULL,
> >>   };
> >>   ATTRIBUTE_GROUPS(typec);
> >
> > thanks,
> >
>

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Heikki Krogerus]

On Wed, Oct 23, 2019 at 06:44:39AM -0700, Guenter Roeck wrote:
> On 10/23/19 1:32 AM, Heikki Krogerus wrote:
> > +Guenter
> > 
> > On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
> > > Export the Type-C connector orientation so that user space
> > > can get this information.
> > > 
> > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > > ---
> > >   Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
> > >   drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
> > >   2 files changed, 29 insertions(+)
> > > 
> > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > > index d7647b258c3c..b22f71801671 100644
> > > --- a/Documentation/ABI/testing/sysfs-class-typec
> > > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > > @@ -108,6 +108,17 @@ Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > >   Description:
> > >   		Revision number of the supported USB Type-C specification.
> > > +What:		/sys/class/typec/<port>/connector_orientation
> > > +Date:		October 2019
> > > +Contact:	Puma Hsu <pumahsu@google.com>
> > > +Description:
> > > +		Indicates which typec connector orientation is configured now.
> > > +		cc1 is defined as "normal" and cc2 is defined as "reversed".
> > > +
> > > +		Valid value:
> > > +		- unknown (nothing configured)
> > 
> > "unknown" means we do not know the orientation.
> > 
> > > +		- normal (configured in cc1 side)
> > > +		- reversed (configured in cc2 side)
> > 
> > Guenter, do you think "connector_orientation" OK. I proposed it, but
> > I'm now wondering if something like "polarity" would be better?
> > 
> 
> Yes, or just "orientation". I don't see the value in the "connector_" prefix.
> I also wonder if "unknown" is really correct. Is it really unknown, or
> does it mean that the port is disconnected ?

Unknown means we don't know the orientation. We don't always have that
information available to us. With UCSI we simply do not know it.

I think this file needs to be hidden after all if we don't know the
cable plug orientation.

How about empty string instead of "unknown"?

thanks,

-- 
heikki

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Guenter Roeck]

On Wed, Oct 23, 2019 at 05:29:00PM +0300, Heikki Krogerus wrote:
> On Wed, Oct 23, 2019 at 06:44:39AM -0700, Guenter Roeck wrote:
> > On 10/23/19 1:32 AM, Heikki Krogerus wrote:
> > > +Guenter
> > > 
> > > On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
> > > > Export the Type-C connector orientation so that user space
> > > > can get this information.
> > > > 
> > > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > > > ---
> > > >   Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
> > > >   drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
> > > >   2 files changed, 29 insertions(+)
> > > > 
> > > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > > > index d7647b258c3c..b22f71801671 100644
> > > > --- a/Documentation/ABI/testing/sysfs-class-typec
> > > > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > > > @@ -108,6 +108,17 @@ Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > >   Description:
> > > >   		Revision number of the supported USB Type-C specification.
> > > > +What:		/sys/class/typec/<port>/connector_orientation
> > > > +Date:		October 2019
> > > > +Contact:	Puma Hsu <pumahsu@google.com>
> > > > +Description:
> > > > +		Indicates which typec connector orientation is configured now.
> > > > +		cc1 is defined as "normal" and cc2 is defined as "reversed".
> > > > +
> > > > +		Valid value:
> > > > +		- unknown (nothing configured)
> > > 
> > > "unknown" means we do not know the orientation.
> > > 
> > > > +		- normal (configured in cc1 side)
> > > > +		- reversed (configured in cc2 side)
> > > 
> > > Guenter, do you think "connector_orientation" OK. I proposed it, but
> > > I'm now wondering if something like "polarity" would be better?
> > > 
> > 
> > Yes, or just "orientation". I don't see the value in the "connector_" prefix.
> > I also wonder if "unknown" is really correct. Is it really unknown, or
> > does it mean that the port is disconnected ?
> 
> Unknown means we don't know the orientation. We don't always have that
> information available to us. With UCSI we simply do not know it.
> 
> I think this file needs to be hidden after all if we don't know the
> cable plug orientation.
> 
Making the attribute appear and disappear may cause difficulties for
userspace.

> How about empty string instead of "unknown"?
> 
An empty string might also be challenging for userspace.

"unknown" is fine if it is really unknown. With that in mind,
I wonder what value that attribute has for userspace, but presumably
there must be some use case. I assume it is purely informational.

In summary, I would suggest to name the attribute either "orientation"
or "polarity".

Thanks,
Guenter

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Heikki Krogerus]

On Wed, Oct 23, 2019 at 08:01:26AM -0700, Guenter Roeck wrote:
> On Wed, Oct 23, 2019 at 05:29:00PM +0300, Heikki Krogerus wrote:
> > On Wed, Oct 23, 2019 at 06:44:39AM -0700, Guenter Roeck wrote:
> > > On 10/23/19 1:32 AM, Heikki Krogerus wrote:
> > > > +Guenter
> > > > 
> > > > On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
> > > > > Export the Type-C connector orientation so that user space
> > > > > can get this information.
> > > > > 
> > > > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > > > > ---
> > > > >   Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
> > > > >   drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
> > > > >   2 files changed, 29 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > > > > index d7647b258c3c..b22f71801671 100644
> > > > > --- a/Documentation/ABI/testing/sysfs-class-typec
> > > > > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > > > > @@ -108,6 +108,17 @@ Contact:	Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > >   Description:
> > > > >   		Revision number of the supported USB Type-C specification.
> > > > > +What:		/sys/class/typec/<port>/connector_orientation
> > > > > +Date:		October 2019
> > > > > +Contact:	Puma Hsu <pumahsu@google.com>
> > > > > +Description:
> > > > > +		Indicates which typec connector orientation is configured now.
> > > > > +		cc1 is defined as "normal" and cc2 is defined as "reversed".
> > > > > +
> > > > > +		Valid value:
> > > > > +		- unknown (nothing configured)
> > > > 
> > > > "unknown" means we do not know the orientation.
> > > > 
> > > > > +		- normal (configured in cc1 side)
> > > > > +		- reversed (configured in cc2 side)
> > > > 
> > > > Guenter, do you think "connector_orientation" OK. I proposed it, but
> > > > I'm now wondering if something like "polarity" would be better?
> > > > 
> > > 
> > > Yes, or just "orientation". I don't see the value in the "connector_" prefix.
> > > I also wonder if "unknown" is really correct. Is it really unknown, or
> > > does it mean that the port is disconnected ?
> > 
> > Unknown means we don't know the orientation. We don't always have that
> > information available to us. With UCSI we simply do not know it.
> > 
> > I think this file needs to be hidden after all if we don't know the
> > cable plug orientation.
> > 
> Making the attribute appear and disappear may cause difficulties for
> userspace.
> 
> > How about empty string instead of "unknown"?
> > 
> An empty string might also be challenging for userspace.
> 
> "unknown" is fine if it is really unknown.

That's what I was thinking, but I realised that since the value may be
"unknown" even when the driver is able to tell the cable plug
orientation, there is no way for the userspace to know is the driver
able to supply the information or not. That is why I say the attribute
has to be hidden in cases where the driver really does not know the
orientation (like UCSI).

I'm really not a big fan of hidden attribute files, as they do make
things unpredictable for the userspace, but with information like
this, either we simply do not provide it to the userspace at all -
option that I'm all for if there is no real need for this - or we
hide the file with drivers that can not supply the information.

> With that in mind, I wonder what value that attribute has for
> userspace, but presumably there must be some use case. I assume it
> is purely informational.

Puma actually already answered to this one:
https://lkml.org/lkml/2019/10/22/198

If I understood correctly, it would be purely informational. Puma,
please correct me if I'm wrong!

But if that is the case, and it is purely informational, then I don't
think we should add this attribute at all.


thanks,

-- 
heikki

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Puma Hsu]

Yes, generally this might be purely informational or be a dynamically
debuggable
mechanism for end user as I mentioned in previous discussion
thread(https://lkml.org/lkml/2019/10/22/198).
Could I know if it is not suitable that we expose a file for
informational usage?


If everyone agreed above, about the definition of “unknown” and the condition
“don’t know the orientation”, what about adding additional return value?
  1. For original “unknown”, it is a generic unknown state which can
indicate no
      matter connector is disconnected, cannot specify which cc side
is configured(such as Ra-Ra),
      or even driver can not know the orientation.
  2. New additional value “unavailable”, it can be used to
specifically explicate that
      driver can not know the orientation.
Take UCSI as example, it can use generic “unknown” or “unavailable” if
it wants.
But if it exposes “unavailable”, then application in user space can
know that this attribute is not useful.



I summarize the proposal definition below:
 - unknown (generic unknown. driver don’t or can’t know the polarity,
                      e.g. disconnected, both cc1 and cc2 are the same, )
 - normal (configured in cc1 side)
 - reversed (configured in cc2 side)
 - unavailable (not support the polarity detection)


Thanks in advance.
Puma Hsu


On Wed, Oct 23, 2019 at 11:58 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> On Wed, Oct 23, 2019 at 08:01:26AM -0700, Guenter Roeck wrote:
> > On Wed, Oct 23, 2019 at 05:29:00PM +0300, Heikki Krogerus wrote:
> > > On Wed, Oct 23, 2019 at 06:44:39AM -0700, Guenter Roeck wrote:
> > > > On 10/23/19 1:32 AM, Heikki Krogerus wrote:
> > > > > +Guenter
> > > > >
> > > > > On Tue, Oct 22, 2019 at 04:59:24PM +0800, Puma Hsu wrote:
> > > > > > Export the Type-C connector orientation so that user space
> > > > > > can get this information.
> > > > > >
> > > > > > Signed-off-by: Puma Hsu <pumahsu@google.com>
> > > > > > ---
> > > > > >   Documentation/ABI/testing/sysfs-class-typec | 11 +++++++++++
> > > > > >   drivers/usb/typec/class.c                   | 18 ++++++++++++++++++
> > > > > >   2 files changed, 29 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/ABI/testing/sysfs-class-typec b/Documentation/ABI/testing/sysfs-class-typec
> > > > > > index d7647b258c3c..b22f71801671 100644
> > > > > > --- a/Documentation/ABI/testing/sysfs-class-typec
> > > > > > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > > > > > @@ -108,6 +108,17 @@ Contact: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > > > > >   Description:
> > > > > >               Revision number of the supported USB Type-C specification.
> > > > > > +What:                /sys/class/typec/<port>/connector_orientation
> > > > > > +Date:                October 2019
> > > > > > +Contact:     Puma Hsu <pumahsu@google.com>
> > > > > > +Description:
> > > > > > +             Indicates which typec connector orientation is configured now.
> > > > > > +             cc1 is defined as "normal" and cc2 is defined as "reversed".
> > > > > > +
> > > > > > +             Valid value:
> > > > > > +             - unknown (nothing configured)
> > > > >
> > > > > "unknown" means we do not know the orientation.
> > > > >
> > > > > > +             - normal (configured in cc1 side)
> > > > > > +             - reversed (configured in cc2 side)
> > > > >
> > > > > Guenter, do you think "connector_orientation" OK. I proposed it, but
> > > > > I'm now wondering if something like "polarity" would be better?
> > > > >
> > > >
> > > > Yes, or just "orientation". I don't see the value in the "connector_" prefix.
> > > > I also wonder if "unknown" is really correct. Is it really unknown, or
> > > > does it mean that the port is disconnected ?
> > >
> > > Unknown means we don't know the orientation. We don't always have that
> > > information available to us. With UCSI we simply do not know it.
> > >
> > > I think this file needs to be hidden after all if we don't know the
> > > cable plug orientation.
> > >
> > Making the attribute appear and disappear may cause difficulties for
> > userspace.
> >
> > > How about empty string instead of "unknown"?
> > >
> > An empty string might also be challenging for userspace.
> >
> > "unknown" is fine if it is really unknown.
>
> That's what I was thinking, but I realised that since the value may be
> "unknown" even when the driver is able to tell the cable plug
> orientation, there is no way for the userspace to know is the driver
> able to supply the information or not. That is why I say the attribute
> has to be hidden in cases where the driver really does not know the
> orientation (like UCSI).
>
> I'm really not a big fan of hidden attribute files, as they do make
> things unpredictable for the userspace, but with information like
> this, either we simply do not provide it to the userspace at all -
> option that I'm all for if there is no real need for this - or we
> hide the file with drivers that can not supply the information.
>
> > With that in mind, I wonder what value that attribute has for
> > userspace, but presumably there must be some use case. I assume it
> > is purely informational.
>
> Puma actually already answered to this one:
> https://lkml.org/lkml/2019/10/22/198
>
> If I understood correctly, it would be purely informational. Puma,
> please correct me if I'm wrong!
>
> But if that is the case, and it is purely informational, then I don't
> think we should add this attribute at all.
>
>
> thanks,
>
> --
> heikki

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Heikki Krogerus]

Hi,

On Thu, Oct 24, 2019 at 05:02:18PM +0800, Puma Hsu wrote:
> Yes, generally this might be purely informational or be a dynamically
> debuggable
> mechanism for end user as I mentioned in previous discussion
> thread(https://lkml.org/lkml/2019/10/22/198).
> Could I know if it is not suitable that we expose a file for
> informational usage?
> 
> If everyone agreed above, about the definition of “unknown” and the condition
> “don’t know the orientation”, what about adding additional return value?
>   1. For original “unknown”, it is a generic unknown state which can
> indicate no
>       matter connector is disconnected, cannot specify which cc side
> is configured(such as Ra-Ra),
>       or even driver can not know the orientation.
>   2. New additional value “unavailable”, it can be used to
> specifically explicate that
>       driver can not know the orientation.
> Take UCSI as example, it can use generic “unknown” or “unavailable” if
> it wants.
> But if it exposes “unavailable”, then application in user space can
> know that this attribute is not useful.
> 
> I summarize the proposal definition below:
>  - unknown (generic unknown. driver don’t or can’t know the polarity,
>                       e.g. disconnected, both cc1 and cc2 are the same, )
>  - normal (configured in cc1 side)
>  - reversed (configured in cc2 side)
>  - unavailable (not support the polarity detection)

Now the attribute would be supplying two types of information:

        1) Does the driver know the orientation
        2) The current orientation

Let's not do that! If you really need this, then just implement the
".is_visible" callback with it. You just need to add a flag to the
struct typec_capability that tells does the driver know the
orientation or not. Something like:

        unsigned int orientation_aware:1;

We already "hide" the identity information if the underlying driver
is unable to supply it. By making this attribute optional as well (by
hiding it when it's not known), the style of exposing the information
is kept the same throughout the class.

thanks,

-- 
heikki

Re: [PATCH V2] usb: typec: Add sysfs node to show connector orientation

[Puma Hsu]

Hi Heikki,

Sure, I will check about this.
Thanks for advising me.

Thanks in advance.
Puma Hsu



On Thu, Oct 24, 2019 at 8:06 PM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
>
> Hi,
>
> On Thu, Oct 24, 2019 at 05:02:18PM +0800, Puma Hsu wrote:
> > Yes, generally this might be purely informational or be a dynamically
> > debuggable
> > mechanism for end user as I mentioned in previous discussion
> > thread(https://lkml.org/lkml/2019/10/22/198).
> > Could I know if it is not suitable that we expose a file for
> > informational usage?
> >
> > If everyone agreed above, about the definition of “unknown” and the condition
> > “don’t know the orientation”, what about adding additional return value?
> >   1. For original “unknown”, it is a generic unknown state which can
> > indicate no
> >       matter connector is disconnected, cannot specify which cc side
> > is configured(such as Ra-Ra),
> >       or even driver can not know the orientation.
> >   2. New additional value “unavailable”, it can be used to
> > specifically explicate that
> >       driver can not know the orientation.
> > Take UCSI as example, it can use generic “unknown” or “unavailable” if
> > it wants.
> > But if it exposes “unavailable”, then application in user space can
> > know that this attribute is not useful.
> >
> > I summarize the proposal definition below:
> >  - unknown (generic unknown. driver don’t or can’t know the polarity,
> >                       e.g. disconnected, both cc1 and cc2 are the same, )
> >  - normal (configured in cc1 side)
> >  - reversed (configured in cc2 side)
> >  - unavailable (not support the polarity detection)
>
> Now the attribute would be supplying two types of information:
>
>         1) Does the driver know the orientation
>         2) The current orientation
>
> Let's not do that! If you really need this, then just implement the
> ".is_visible" callback with it. You just need to add a flag to the
> struct typec_capability that tells does the driver know the
> orientation or not. Something like:
>
>         unsigned int orientation_aware:1;
>
> We already "hide" the identity information if the underlying driver
> is unable to supply it. By making this attribute optional as well (by
> hiding it when it's not known), the style of exposing the information
> is kept the same throughout the class.
>
> thanks,
>
> --
> heikki