Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-22 Thread Heikki Krogerus 
On Mon, Nov 21, 2016 at 03:45:06PM +0100, Greg KH wrote:
> Again, free the device for which this release function is being called
> for, that is why it is there.

I will.

Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-22 Thread Heikki Krogerus 
On Tue, Nov 22, 2016 at 12:51:11PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 21, 2016 at 03:45:06PM +0100, Greg KH wrote:
> > > We could allocate an extra structure for the partner when
> > > typec_connect() is called, but we would do that just for the sake of
> > > having something to free in the release hook. It would not be useful
> > > for anything. It would not help us increase/decrease the reference
> > > count of the device, and the port driver would still have to provide
> > > details about the partner capabilities the moment it tells us the
> > > partner was connected.
> > 
> > Again, free the device for which this release function is being called
> > for, that is why it is there.
> 
> The struct device is now member of struct typec_partner. This is what
> a typical port driver would have (I hope it's readable):
> 
> struct my_port {
> /* This structure is provided by the class */
> struct typec_port *port;
> 
> /*
>  * Don't forget, there can only be one partner at a time
>  */
> struct typec_partner partner; /* NOTE: this is not a pointer */
> };
> 
> int my_interrupt(...)
> {
> ...
> /*
>  * Connection happened (I'm skipping the typec_connection
>  * wrapper in this example)
>  */
> my_port->partner.usb_pd = ...
> ...
> ret = typec_connect(my_port->port, _port->partner);
> ...
> /*
>  * Disconnect
>  */
> typec_disconnect(my_port->port);
> memset(_port->partner, 0, sizeof(struct typec_partner));
> ...
> }
> 
> int my_probe(...)
> {
> struct my_port *my_port;
> ...
> my_port = devm_kzalloc(...
> ...
> my_port->port = typec_register_port(...
> ...
> }
> 
> To have something to free when the partner device's reference counter
> goes to zero and release is called (this happens after all the
> alternate modes, so the children, and the device are unregistered), we
> will need an extra structure, just for the fun of having something to
> free in release.
> 
> struct internal_partner_structure {
> struct device dev;
> struct typec_partner *partner_capabilities; /* port driver provides */
> };
> 
> Why is this necessary in this case? It is just something extra we have
> to do, just so we can allocate that when connection happens and the
> partner device is generated, and so we can then free that thing when
> release gets called? It does not give us anything. It does not affect
> anything.

Blah, ignore that message. I'm talking about the wrong thing here.

Sorry about that.

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-22 Thread Heikki Krogerus 
Hi Greg,

On Mon, Nov 21, 2016 at 03:45:06PM +0100, Greg KH wrote:
> > We could allocate an extra structure for the partner when
> > typec_connect() is called, but we would do that just for the sake of
> > having something to free in the release hook. It would not be useful
> > for anything. It would not help us increase/decrease the reference
> > count of the device, and the port driver would still have to provide
> > details about the partner capabilities the moment it tells us the
> > partner was connected.
> 
> Again, free the device for which this release function is being called
> for, that is why it is there.

The struct device is now member of struct typec_partner. This is what
a typical port driver would have (I hope it's readable):

struct my_port {
/* This structure is provided by the class */
struct typec_port *port;

/*
 * Don't forget, there can only be one partner at a time
 */
struct typec_partner partner; /* NOTE: this is not a pointer */
};

int my_interrupt(...)
{
...
/*
 * Connection happened (I'm skipping the typec_connection
 * wrapper in this example)
 */
my_port->partner.usb_pd = ...
...
ret = typec_connect(my_port->port, _port->partner);
...
/*
 * Disconnect
 */
typec_disconnect(my_port->port);
memset(_port->partner, 0, sizeof(struct typec_partner));
...
}

int my_probe(...)
{
struct my_port *my_port;
...
my_port = devm_kzalloc(...
...
my_port->port = typec_register_port(...
...
}

To have something to free when the partner device's reference counter
goes to zero and release is called (this happens after all the
alternate modes, so the children, and the device are unregistered), we
will need an extra structure, just for the fun of having something to
free in release.

struct internal_partner_structure {
struct device dev;
struct typec_partner *partner_capabilities; /* port driver provides */
};

Why is this necessary in this case? It is just something extra we have
to do, just so we can allocate that when connection happens and the
partner device is generated, and so we can then free that thing when
release gets called? It does not give us anything. It does not affect
anything.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-22 Thread Heikki Krogerus 
On Mon, Nov 21, 2016 at 07:33:45AM -0800, Guenter Roeck wrote:
> On 11/21/2016 06:23 AM, Heikki Krogerus wrote:
> > On Mon, Nov 21, 2016 at 03:11:03PM +0200, Heikki Krogerus wrote:
> > > Hi Greg,
> > > 
> > > On Mon, Nov 21, 2016 at 11:35:28AM +0100, Greg KH wrote:
> > > > > +static void typec_partner_release(struct device *dev)
> > > > > +{
> > > > > + struct typec_port *port = to_typec_port(dev->parent);
> > > > > +
> > > > > + typec_unregister_altmodes(dev);
> > > > > + port->partner = NULL;
> > > > > +}
> > > > 
> > > > This doesn't feel right, why are you both exporting
> > > > typec_unregister_altmodes() and also calling it from release callbacks?
> > > > That implies there is two way to clean stuff up, so what is a driver
> > > > writer to use?  Please simplify and force it to be one way or the other.
> > > 
> > > OK.
> > 
> > Guenter did you need to also remove the alternate modes in drivers, or
> > can we just do it here?
> > 
> 
> It is currently called explicitly on a data role change, when executing
> a hard reset, on detach, and during error recovery. Most of those would
> also unregister the partner, so I should be able to drop those calls
> (or maybe I'll have to - I will see when testing), but I am not sure
> how to handle data role changes.

Let's keep this in the drivers. Actually, we can't unregister them
here. The partner is the parent of the alternate modes devices. Sorry
about the hassle.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-22 Thread Greg KH 
On Tue, Nov 22, 2016 at 09:58:13AM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 21, 2016 at 03:46:08PM +0100, Greg KH wrote:
> > > > > +
> > > > > +config TYPEC
> > > > > + tristate
> > > > 
> > > > Hah, that says NOTHING about what this code is at all.
> > > 
> > > Alone the class driver does nothing. Why would the user need to be
> > > aware of if it when selecting the Type-C drivers, and what can the
> > > user use that information for?
> > 
> > If you see a blank Kconfig option, what are you supposed to do with it?
> > How do you know if you need to enable it or not?  Are you just supposed
> > to guess?
> 
> But you don't see anything when you are selecting the drivers and that
> is the point. You now can't select this separately. There is now no
> option for it.
> 
> Why should we bother the user with this? The user is most likely only
> interested in the drivers and by selecting those the user will get the
> interface. The drivers will need to have the dependency to the class
> set correctly in any case.

So with this patch, no code gets built or asked about in Kconfig?

Odd, if so, sorry for the noise...

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-22 Thread Heikki Krogerus 
Hi Greg,

On Mon, Nov 21, 2016 at 03:46:08PM +0100, Greg KH wrote:
> > > > +
> > > > +config TYPEC
> > > > +   tristate
> > > 
> > > Hah, that says NOTHING about what this code is at all.
> > 
> > Alone the class driver does nothing. Why would the user need to be
> > aware of if it when selecting the Type-C drivers, and what can the
> > user use that information for?
> 
> If you see a blank Kconfig option, what are you supposed to do with it?
> How do you know if you need to enable it or not?  Are you just supposed
> to guess?

But you don't see anything when you are selecting the drivers and that
is the point. You now can't select this separately. There is now no
option for it.

Why should we bother the user with this? The user is most likely only
interested in the drivers and by selecting those the user will get the
interface. The drivers will need to have the dependency to the class
set correctly in any case.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-21 Thread Guenter Roeck 

On 11/21/2016 06:23 AM, Heikki Krogerus wrote:

On Mon, Nov 21, 2016 at 03:11:03PM +0200, Heikki Krogerus wrote:

Hi Greg,

On Mon, Nov 21, 2016 at 11:35:28AM +0100, Greg KH wrote:

+static void typec_partner_release(struct device *dev)
+{
+   struct typec_port *port = to_typec_port(dev->parent);
+
+   typec_unregister_altmodes(dev);
+   port->partner = NULL;
+}


This doesn't feel right, why are you both exporting
typec_unregister_altmodes() and also calling it from release callbacks?
That implies there is two way to clean stuff up, so what is a driver
writer to use?  Please simplify and force it to be one way or the other.


OK.


Guenter did you need to also remove the alternate modes in drivers, or
can we just do it here?



It is currently called explicitly on a data role change, when executing
a hard reset, on detach, and during error recovery. Most of those would
also unregister the partner, so I should be able to drop those calls
(or maybe I'll have to - I will see when testing), but I am not sure
how to handle data role changes.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-21 Thread Greg KH 
On Mon, Nov 21, 2016 at 03:11:03PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 21, 2016 at 11:35:28AM +0100, Greg KH wrote:
> > > +static void typec_partner_release(struct device *dev)
> > > +{
> > > + struct typec_port *port = to_typec_port(dev->parent);
> > > +
> > > + typec_unregister_altmodes(dev);
> > > + port->partner = NULL;
> > > +}
> > 
> > This doesn't feel right, why are you both exporting
> > typec_unregister_altmodes() and also calling it from release callbacks?
> > That implies there is two way to clean stuff up, so what is a driver
> > writer to use?  Please simplify and force it to be one way or the other.
> 
> OK.
> 
> > Also typec_unregister_altmodes() doesn't free memory, which release is
> > supposed to be doing, which also implies that the reference counting
> > logic is all wrong here.  Please fix, like I asked you to previously.
> 
> There is nothing wrong with the reference counting, and nothing has
> been allocated so there is nothing to free.

The device structure itself that this release call is for needs to
be freed, right?  If not, something is really wrong...

> Please note that the partner device is meant to just represent the
> partner in user space and not to be actually used for anything. And
> please also note that there can only be one partner for a port at a
> time.

Ok, but these are still reference counted devices, you need to handle
that properly.

> We could allocate an extra structure for the partner when
> typec_connect() is called, but we would do that just for the sake of
> having something to free in the release hook. It would not be useful
> for anything. It would not help us increase/decrease the reference
> count of the device, and the port driver would still have to provide
> details about the partner capabilities the moment it tells us the
> partner was connected.

Again, free the device for which this release function is being called
for, that is why it is there.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-21 Thread Greg KH 
On Mon, Nov 21, 2016 at 03:27:41PM +0200, Heikki Krogerus wrote:
> On Mon, Nov 21, 2016 at 11:33:11AM +0100, Greg KH wrote:
> > On Thu, Nov 17, 2016 at 12:50:35PM +0200, Heikki Krogerus wrote:
> > > The purpose of USB Type-C connector class is to provide
> > > unified interface for the user space to get the status and
> > > basic information about USB Type-C connectors on a system,
> > > control over data role swapping, and when the port supports
> > > USB Power Delivery, also control over power role swapping
> > > and Alternate Modes.
> > > 
> > > Signed-off-by: Heikki Krogerus 
> > 
> > I'd like to get some acks or reviewed-by from others, but one thing
> > jumped out at me, which implies you did not run checkpatch.pl on the
> > patch:
> > 
> > 644
> 
> OK.
> 
> > > --- a/drivers/usb/Kconfig
> > > +++ b/drivers/usb/Kconfig
> > > @@ -152,6 +152,8 @@ source "drivers/usb/phy/Kconfig"
> > >  
> > >  source "drivers/usb/gadget/Kconfig"
> > >  
> > > +source "drivers/usb/typec/Kconfig"
> > > +
> > >  config USB_LED_TRIG
> > >   bool "USB LED Triggers"
> > >   depends on LEDS_CLASS && LEDS_TRIGGERS
> > > diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> > > index 7791af6..c7f4098 100644
> > > --- a/drivers/usb/Makefile
> > > +++ b/drivers/usb/Makefile
> > > @@ -62,3 +62,5 @@ obj-$(CONFIG_USB_GADGET)+= gadget/
> > >  obj-$(CONFIG_USB_COMMON) += common/
> > >  
> > >  obj-$(CONFIG_USBIP_CORE) += usbip/
> > > +
> > > +obj-$(CONFIG_TYPEC)  += typec/
> > > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > > new file mode 100644
> > > index 000..b229fb9
> > > --- /dev/null
> > > +++ b/drivers/usb/typec/Kconfig
> > > @@ -0,0 +1,7 @@
> > > +
> > > +menu "USB PD and Type-C drivers"
> > 
> > What is "PD"?  (yes, I know, but very few people do...)  Spell it out.
> 
> OK.
> 
> > > +
> > > +config TYPEC
> > > + tristate
> > 
> > Hah, that says NOTHING about what this code is at all.
> 
> Alone the class driver does nothing. Why would the user need to be
> aware of if it when selecting the Type-C drivers, and what can the
> user use that information for?

If you see a blank Kconfig option, what are you supposed to do with it?
How do you know if you need to enable it or not?  Are you just supposed
to guess?

That is what the help text is for...

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-21 Thread Heikki Krogerus 
On Mon, Nov 21, 2016 at 03:11:03PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 21, 2016 at 11:35:28AM +0100, Greg KH wrote:
> > > +static void typec_partner_release(struct device *dev)
> > > +{
> > > + struct typec_port *port = to_typec_port(dev->parent);
> > > +
> > > + typec_unregister_altmodes(dev);
> > > + port->partner = NULL;
> > > +}
> > 
> > This doesn't feel right, why are you both exporting
> > typec_unregister_altmodes() and also calling it from release callbacks?
> > That implies there is two way to clean stuff up, so what is a driver
> > writer to use?  Please simplify and force it to be one way or the other.
> 
> OK.

Guenter did you need to also remove the alternate modes in drivers, or
can we just do it here?

Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-21 Thread Heikki Krogerus 
On Mon, Nov 21, 2016 at 11:33:11AM +0100, Greg KH wrote:
> On Thu, Nov 17, 2016 at 12:50:35PM +0200, Heikki Krogerus wrote:
> > The purpose of USB Type-C connector class is to provide
> > unified interface for the user space to get the status and
> > basic information about USB Type-C connectors on a system,
> > control over data role swapping, and when the port supports
> > USB Power Delivery, also control over power role swapping
> > and Alternate Modes.
> > 
> > Signed-off-by: Heikki Krogerus 
> 
> I'd like to get some acks or reviewed-by from others, but one thing
> jumped out at me, which implies you did not run checkpatch.pl on the
> patch:
> 
> 644

OK.

> > --- a/drivers/usb/Kconfig
> > +++ b/drivers/usb/Kconfig
> > @@ -152,6 +152,8 @@ source "drivers/usb/phy/Kconfig"
> >  
> >  source "drivers/usb/gadget/Kconfig"
> >  
> > +source "drivers/usb/typec/Kconfig"
> > +
> >  config USB_LED_TRIG
> > bool "USB LED Triggers"
> > depends on LEDS_CLASS && LEDS_TRIGGERS
> > diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> > index 7791af6..c7f4098 100644
> > --- a/drivers/usb/Makefile
> > +++ b/drivers/usb/Makefile
> > @@ -62,3 +62,5 @@ obj-$(CONFIG_USB_GADGET)  += gadget/
> >  obj-$(CONFIG_USB_COMMON)   += common/
> >  
> >  obj-$(CONFIG_USBIP_CORE)   += usbip/
> > +
> > +obj-$(CONFIG_TYPEC)+= typec/
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > new file mode 100644
> > index 000..b229fb9
> > --- /dev/null
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -0,0 +1,7 @@
> > +
> > +menu "USB PD and Type-C drivers"
> 
> What is "PD"?  (yes, I know, but very few people do...)  Spell it out.

OK.

> > +
> > +config TYPEC
> > +   tristate
> 
> Hah, that says NOTHING about what this code is at all.

Alone the class driver does nothing. Why would the user need to be
aware of if it when selecting the Type-C drivers, and what can the
user use that information for?


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-21 Thread Heikki Krogerus 
Hi Greg,

On Mon, Nov 21, 2016 at 11:35:28AM +0100, Greg KH wrote:
> > +static void typec_partner_release(struct device *dev)
> > +{
> > +   struct typec_port *port = to_typec_port(dev->parent);
> > +
> > +   typec_unregister_altmodes(dev);
> > +   port->partner = NULL;
> > +}
> 
> This doesn't feel right, why are you both exporting
> typec_unregister_altmodes() and also calling it from release callbacks?
> That implies there is two way to clean stuff up, so what is a driver
> writer to use?  Please simplify and force it to be one way or the other.

OK.

> Also typec_unregister_altmodes() doesn't free memory, which release is
> supposed to be doing, which also implies that the reference counting
> logic is all wrong here.  Please fix, like I asked you to previously.

There is nothing wrong with the reference counting, and nothing has
been allocated so there is nothing to free.

Please note that the partner device is meant to just represent the
partner in user space and not to be actually used for anything. And
please also note that there can only be one partner for a port at a
time.

We could allocate an extra structure for the partner when
typec_connect() is called, but we would do that just for the sake of
having something to free in the release hook. It would not be useful
for anything. It would not help us increase/decrease the reference
count of the device, and the port driver would still have to provide
details about the partner capabilities the moment it tells us the
partner was connected.


Thanks,

-- 
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-21 Thread Greg KH 
On Thu, Nov 17, 2016 at 12:50:35PM +0200, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus 
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  222 ++
>  Documentation/usb/typec.txt |  103 +++
>  MAINTAINERS |9 +
>  drivers/usb/Kconfig |2 +
>  drivers/usb/Makefile|2 +
>  drivers/usb/typec/Kconfig   |7 +
>  drivers/usb/typec/Makefile  |1 +
>  drivers/usb/typec/typec.c   | 1012 
> +++
>  include/linux/usb/typec.h   |  252 +++
>  9 files changed, 1610 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-class-typec
>  create mode 100644 Documentation/usb/typec.txt
>  create mode 100644 drivers/usb/typec/Kconfig
>  create mode 100644 drivers/usb/typec/Makefile
>  create mode 100644 drivers/usb/typec/typec.c
>  create mode 100644 include/linux/usb/typec.h
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec 
> b/Documentation/ABI/testing/sysfs-class-typec
> new file mode 100644
> index 000..4fac77c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -0,0 +1,222 @@
> +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> +
> +What:/sys/class/typec//current_data_role
> +Date:December 2016
> +Contact: Heikki Krogerus 
> +Description:
> + The current USB data role the port is operating in. This
> + attribute can be used for requesting data role swapping on the
> + port. Swapping is supported as synchronous operation, so
> + write(2) to the attribute will not return until the operation
> + has finished. The attribute is notified about role changes so
> + that poll(2) on the attribute wakes up. Change on the role will
> + also generate uevent KOBJ_CHANGE on the port.
> +
> + Valid values:
> + - host
> + - device
> +
> +What:/sys/class/typec//current_power_role
> +Date:December 2016
> +Contact: Heikki Krogerus 
> +Description:
> + The current power role of the port. This attribute can be used
> + to request power role swap on the port when the port supports
> + USB Power Delivery. Swapping is supported as synchronous
> + operation, so write(2) to the attribute will not return until
> + the operation has finished. The attribute is notified about role
> + changes so that poll(2) on the attribute wakes up. Change on the
> + role will also generate uevent KOBJ_CHANGE.
> +
> + Valid values:
> + - source
> + - sink
> +
> +What:/sys/class/typec//vconn_source
> +Date:December 2016
> +Contact: Heikki Krogerus 
> +Description:
> + Shows is the port VCONN Source. This attribute can be used to
> + request VCONN swap to change the VCONN Source during connection
> + when both the port and the partner support USB Power Delivery.
> + Swapping is supported as synchronous operation, so write(2) to
> + the attribute will not return until the operation has finished.
> + The attribute is notified about VCONN source changes so that
> + poll(2) on the attribute wakes up. Change on VCONN source also
> + generates uevent KOBJ_CHANGE.
> +
> + Valid values are:
> + - 0 when the port is not the VCONN Source
> + - 1 when the port is the VCONN Source
> +
> +What:/sys/class/typec//power_operation_mode
> +Date:December 2016
> +Contact: Heikki Krogerus 
> +Description:
> + Shows the current power operational mode the port is in.
> +
> + Valid values:
> + - USB - Normal power levels defined in USB specifications
> + - BC1.2 - Power levels defined in Battery Charging Specification
> +   v1.2
> + - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
> + specification.
> + - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
> + specification.
> +- USB Power Delivery - The voltages and currents defined in 
> USB

Re: [PATCHv11 2/3] usb: USB Type-C connector class

2016-11-21 Thread Greg KH 
On Thu, Nov 17, 2016 at 12:50:35PM +0200, Heikki Krogerus wrote:
> The purpose of USB Type-C connector class is to provide
> unified interface for the user space to get the status and
> basic information about USB Type-C connectors on a system,
> control over data role swapping, and when the port supports
> USB Power Delivery, also control over power role swapping
> and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus 

I'd like to get some acks or reviewed-by from others, but one thing
jumped out at me, which implies you did not run checkpatch.pl on the
patch:

644
> --- a/drivers/usb/Kconfig
> +++ b/drivers/usb/Kconfig
> @@ -152,6 +152,8 @@ source "drivers/usb/phy/Kconfig"
>  
>  source "drivers/usb/gadget/Kconfig"
>  
> +source "drivers/usb/typec/Kconfig"
> +
>  config USB_LED_TRIG
>   bool "USB LED Triggers"
>   depends on LEDS_CLASS && LEDS_TRIGGERS
> diff --git a/drivers/usb/Makefile b/drivers/usb/Makefile
> index 7791af6..c7f4098 100644
> --- a/drivers/usb/Makefile
> +++ b/drivers/usb/Makefile
> @@ -62,3 +62,5 @@ obj-$(CONFIG_USB_GADGET)+= gadget/
>  obj-$(CONFIG_USB_COMMON) += common/
>  
>  obj-$(CONFIG_USBIP_CORE) += usbip/
> +
> +obj-$(CONFIG_TYPEC)  += typec/
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> new file mode 100644
> index 000..b229fb9
> --- /dev/null
> +++ b/drivers/usb/typec/Kconfig
> @@ -0,0 +1,7 @@
> +
> +menu "USB PD and Type-C drivers"

What is "PD"?  (yes, I know, but very few people do...)  Spell it out.

> +
> +config TYPEC
> + tristate

Hah, that says NOTHING about what this code is at all.

Please fix.

thanks,

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv11 2/3] usb: USB Type-C connector class

2016-11-17 Thread Heikki Krogerus 
The purpose of USB Type-C connector class is to provide
unified interface for the user space to get the status and
basic information about USB Type-C connectors on a system,
control over data role swapping, and when the port supports
USB Power Delivery, also control over power role swapping
and Alternate Modes.

Signed-off-by: Heikki Krogerus 
---
 Documentation/ABI/testing/sysfs-class-typec |  222 ++
 Documentation/usb/typec.txt |  103 +++
 MAINTAINERS |9 +
 drivers/usb/Kconfig |2 +
 drivers/usb/Makefile|2 +
 drivers/usb/typec/Kconfig   |7 +
 drivers/usb/typec/Makefile  |1 +
 drivers/usb/typec/typec.c   | 1012 +++
 include/linux/usb/typec.h   |  252 +++
 9 files changed, 1610 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-class-typec
 create mode 100644 Documentation/usb/typec.txt
 create mode 100644 drivers/usb/typec/Kconfig
 create mode 100644 drivers/usb/typec/Makefile
 create mode 100644 drivers/usb/typec/typec.c
 create mode 100644 include/linux/usb/typec.h

diff --git a/Documentation/ABI/testing/sysfs-class-typec 
b/Documentation/ABI/testing/sysfs-class-typec
new file mode 100644
index 000..4fac77c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -0,0 +1,222 @@
+USB Type-C port devices (eg. /sys/class/typec/usbc0/)
+
+What:  /sys/class/typec//current_data_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   The current USB data role the port is operating in. This
+   attribute can be used for requesting data role swapping on the
+   port. Swapping is supported as synchronous operation, so
+   write(2) to the attribute will not return until the operation
+   has finished. The attribute is notified about role changes so
+   that poll(2) on the attribute wakes up. Change on the role will
+   also generate uevent KOBJ_CHANGE on the port.
+
+   Valid values:
+   - host
+   - device
+
+What:  /sys/class/typec//current_power_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   The current power role of the port. This attribute can be used
+   to request power role swap on the port when the port supports
+   USB Power Delivery. Swapping is supported as synchronous
+   operation, so write(2) to the attribute will not return until
+   the operation has finished. The attribute is notified about role
+   changes so that poll(2) on the attribute wakes up. Change on the
+   role will also generate uevent KOBJ_CHANGE.
+
+   Valid values:
+   - source
+   - sink
+
+What:  /sys/class/typec//vconn_source
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows is the port VCONN Source. This attribute can be used to
+   request VCONN swap to change the VCONN Source during connection
+   when both the port and the partner support USB Power Delivery.
+   Swapping is supported as synchronous operation, so write(2) to
+   the attribute will not return until the operation has finished.
+   The attribute is notified about VCONN source changes so that
+   poll(2) on the attribute wakes up. Change on VCONN source also
+   generates uevent KOBJ_CHANGE.
+
+   Valid values are:
+   - 0 when the port is not the VCONN Source
+   - 1 when the port is the VCONN Source
+
+What:  /sys/class/typec//power_operation_mode
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description:
+   Shows the current power operational mode the port is in.
+
+   Valid values:
+   - USB - Normal power levels defined in USB specifications
+   - BC1.2 - Power levels defined in Battery Charging Specification
+ v1.2
+   - USB Type-C 1.5A - Higher 1.5A current defined in USB Type-C
+   specification.
+   - USB Type-C 3.0A - Higher 3A current defined in USB Type-C
+   specification.
+- USB Power Delivery - The voltages and currents defined in USB
+  Power Delivery specification
+
+What:  /sys/class/typec//preferred_role
+Date:  December 2016
+Contact:   Heikki Krogerus 
+Description: