Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Wed, Nov 16, 2016 at 04:31:07PM +0100, Greg KH wrote:
> On Wed, Nov 16, 2016 at 05:20:24PM +0200, Heikki Krogerus wrote:
> > Hi Greg,
> > 
> > On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > > > +static int sysfs_strmatch(const char * const *array, size_t n, const 
> > > > char *str)
> > > > +{
> > > > +   const char *item;
> > > > +   int index;
> > > > +
> > > > +   for (index = 0; index < n; index++) {
> > > > +   item = array[index];
> > > > +   if (!item)
> > > > +   break;
> > > > +   if (sysfs_streq(item, str))
> > > > +   return index;
> > > > +   }
> > > > +
> > > > +   return -EINVAL;
> > > > +}
> > > 
> > > should we make this a core sysfs function?
> > 
> > Last question before I send v11. Is the following (the helper) OK?
> > 
> > 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 26b6f6a..5606810 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
> >  }
> >  
> >  int match_string(const char * const *array, size_t n, const char *string);
> > +int __sysfs_strmatch(const char * const *array, size_t n, const char 
> > *string);
> > +
> > +/**
> > + * sysfs_strmatch - matches given string in an array
> > + * @a: array of strings
> > + * @s: string to match with
> > + *
> > + * Helper for __sysfs_strmatch(). Calculates the size of @a automatically.
> > + */
> > +#define sysfs_strmatch(a, s) __sysfs_strmatch(a, ARRAY_SIZE(a), s)
> 
> People will bikeshed the name.  Why not just use sysfs_match_string() as
> this does the same as match_string, but calls sysfs_string instead of
> strcmp().

Makes sense. I'll change the name.

Thanks,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Wed, Nov 16, 2016 at 04:31:07PM +0100, Greg KH wrote:
> On Wed, Nov 16, 2016 at 05:20:24PM +0200, Heikki Krogerus wrote:
> > Hi Greg,
> > 
> > On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > > > +static int sysfs_strmatch(const char * const *array, size_t n, const 
> > > > char *str)
> > > > +{
> > > > +   const char *item;
> > > > +   int index;
> > > > +
> > > > +   for (index = 0; index < n; index++) {
> > > > +   item = array[index];
> > > > +   if (!item)
> > > > +   break;
> > > > +   if (sysfs_streq(item, str))
> > > > +   return index;
> > > > +   }
> > > > +
> > > > +   return -EINVAL;
> > > > +}
> > > 
> > > should we make this a core sysfs function?
> > 
> > Last question before I send v11. Is the following (the helper) OK?
> > 
> > 
> > diff --git a/include/linux/string.h b/include/linux/string.h
> > index 26b6f6a..5606810 100644
> > --- a/include/linux/string.h
> > +++ b/include/linux/string.h
> > @@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
> >  }
> >  
> >  int match_string(const char * const *array, size_t n, const char *string);
> > +int __sysfs_strmatch(const char * const *array, size_t n, const char 
> > *string);
> > +
> > +/**
> > + * sysfs_strmatch - matches given string in an array
> > + * @a: array of strings
> > + * @s: string to match with
> > + *
> > + * Helper for __sysfs_strmatch(). Calculates the size of @a automatically.
> > + */
> > +#define sysfs_strmatch(a, s) __sysfs_strmatch(a, ARRAY_SIZE(a), s)
> 
> People will bikeshed the name.  Why not just use sysfs_match_string() as
> this does the same as match_string, but calls sysfs_string instead of
> strcmp().

Makes sense. I'll change the name.

Thanks,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Wed, Nov 16, 2016 at 05:20:24PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > > +static int sysfs_strmatch(const char * const *array, size_t n, const 
> > > char *str)
> > > +{
> > > + const char *item;
> > > + int index;
> > > +
> > > + for (index = 0; index < n; index++) {
> > > + item = array[index];
> > > + if (!item)
> > > + break;
> > > + if (sysfs_streq(item, str))
> > > + return index;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > 
> > should we make this a core sysfs function?
> 
> Last question before I send v11. Is the following (the helper) OK?
> 
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 26b6f6a..5606810 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
>  }
>  
>  int match_string(const char * const *array, size_t n, const char *string);
> +int __sysfs_strmatch(const char * const *array, size_t n, const char 
> *string);
> +
> +/**
> + * sysfs_strmatch - matches given string in an array
> + * @a: array of strings
> + * @s: string to match with
> + *
> + * Helper for __sysfs_strmatch(). Calculates the size of @a automatically.
> + */
> +#define sysfs_strmatch(a, s) __sysfs_strmatch(a, ARRAY_SIZE(a), s)

People will bikeshed the name.  Why not just use sysfs_match_string() as
this does the same as match_string, but calls sysfs_string instead of
strcmp().

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Wed, Nov 16, 2016 at 05:20:24PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > > +static int sysfs_strmatch(const char * const *array, size_t n, const 
> > > char *str)
> > > +{
> > > + const char *item;
> > > + int index;
> > > +
> > > + for (index = 0; index < n; index++) {
> > > + item = array[index];
> > > + if (!item)
> > > + break;
> > > + if (sysfs_streq(item, str))
> > > + return index;
> > > + }
> > > +
> > > + return -EINVAL;
> > > +}
> > 
> > should we make this a core sysfs function?
> 
> Last question before I send v11. Is the following (the helper) OK?
> 
> 
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 26b6f6a..5606810 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
>  }
>  
>  int match_string(const char * const *array, size_t n, const char *string);
> +int __sysfs_strmatch(const char * const *array, size_t n, const char 
> *string);
> +
> +/**
> + * sysfs_strmatch - matches given string in an array
> + * @a: array of strings
> + * @s: string to match with
> + *
> + * Helper for __sysfs_strmatch(). Calculates the size of @a automatically.
> + */
> +#define sysfs_strmatch(a, s) __sysfs_strmatch(a, ARRAY_SIZE(a), s)

People will bikeshed the name.  Why not just use sysfs_match_string() as
this does the same as match_string, but calls sysfs_string instead of
strcmp().

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Badhri Jagan Sridharan]

> Yes, though I don't KOBJ_ADD separately with the partners and cables.
> That uevent is sent when the device for them is registered, so it's
> already there

Makes sense. Thanks Heikki.

Regards,
Badhri

On Wed, Nov 16, 2016 at 7:20 AM, Heikki Krogerus
 wrote:
> Hi Greg,
>
> On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
>> > +static int sysfs_strmatch(const char * const *array, size_t n, const char 
>> > *str)
>> > +{
>> > +   const char *item;
>> > +   int index;
>> > +
>> > +   for (index = 0; index < n; index++) {
>> > +   item = array[index];
>> > +   if (!item)
>> > +   break;
>> > +   if (sysfs_streq(item, str))
>> > +   return index;
>> > +   }
>> > +
>> > +   return -EINVAL;
>> > +}
>>
>> should we make this a core sysfs function?
>
> Last question before I send v11. Is the following (the helper) OK?
>
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 26b6f6a..5606810 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
>  }
>
>  int match_string(const char * const *array, size_t n, const char *string);
> +int __sysfs_strmatch(const char * const *array, size_t n, const char 
> *string);
> +
> +/**
> + * sysfs_strmatch - matches given string in an array
> + * @a: array of strings
> + * @s: string to match with
> + *
> + * Helper for __sysfs_strmatch(). Calculates the size of @a automatically.
> + */
> +#define sysfs_strmatch(a, s) __sysfs_strmatch(a, ARRAY_SIZE(a), s)
>
>  #ifdef CONFIG_BINARY_PRINTF
>  int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
> diff --git a/lib/string.c b/lib/string.c
> index ed83562..a4fe035 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -656,6 +656,32 @@ int match_string(const char * const *array, size_t n, 
> const char *string)
>  }
>  EXPORT_SYMBOL(match_string);
>
> +/**
> + * __sysfs_strmatch - matches given string in an array
> + * @array: array of strings
> + * @n: number of strings in the array or -1 for NULL terminated arrays
> + * @str: string to match with
> + *
> + * Returns index of @str in the @array or -EINVAL, just like match_string().
> + * Uses sysfs_streq() instead of strcmp for matching.
> + */
> +int __sysfs_strmatch(const char * const *array, size_t n, const char *str)
> +{
> +   const char *item;
> +   int index;
> +
> +   for (index = 0; index < n; index++) {
> +   item = array[index];
> +   if (!item)
> +   break;
> +   if (!sysfs_streq(item, str))
> +   return index;
> +   }
> +
> +   return -EINVAL;
> +}
> +EXPORT_SYMBOL(__sysfs_strmatch);
> +
>  #ifndef __HAVE_ARCH_MEMSET
>  /**
>   * memset - Fill a region of memory with the given value
>
>
> --
> 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: [PATHCv10 1/2] usb: USB Type-C connector class

[Badhri Jagan Sridharan]

> Yes, though I don't KOBJ_ADD separately with the partners and cables.
> That uevent is sent when the device for them is registered, so it's
> already there

Makes sense. Thanks Heikki.

Regards,
Badhri

On Wed, Nov 16, 2016 at 7:20 AM, Heikki Krogerus
 wrote:
> Hi Greg,
>
> On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
>> > +static int sysfs_strmatch(const char * const *array, size_t n, const char 
>> > *str)
>> > +{
>> > +   const char *item;
>> > +   int index;
>> > +
>> > +   for (index = 0; index < n; index++) {
>> > +   item = array[index];
>> > +   if (!item)
>> > +   break;
>> > +   if (sysfs_streq(item, str))
>> > +   return index;
>> > +   }
>> > +
>> > +   return -EINVAL;
>> > +}
>>
>> should we make this a core sysfs function?
>
> Last question before I send v11. Is the following (the helper) OK?
>
>
> diff --git a/include/linux/string.h b/include/linux/string.h
> index 26b6f6a..5606810 100644
> --- a/include/linux/string.h
> +++ b/include/linux/string.h
> @@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
>  }
>
>  int match_string(const char * const *array, size_t n, const char *string);
> +int __sysfs_strmatch(const char * const *array, size_t n, const char 
> *string);
> +
> +/**
> + * sysfs_strmatch - matches given string in an array
> + * @a: array of strings
> + * @s: string to match with
> + *
> + * Helper for __sysfs_strmatch(). Calculates the size of @a automatically.
> + */
> +#define sysfs_strmatch(a, s) __sysfs_strmatch(a, ARRAY_SIZE(a), s)
>
>  #ifdef CONFIG_BINARY_PRINTF
>  int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
> diff --git a/lib/string.c b/lib/string.c
> index ed83562..a4fe035 100644
> --- a/lib/string.c
> +++ b/lib/string.c
> @@ -656,6 +656,32 @@ int match_string(const char * const *array, size_t n, 
> const char *string)
>  }
>  EXPORT_SYMBOL(match_string);
>
> +/**
> + * __sysfs_strmatch - matches given string in an array
> + * @array: array of strings
> + * @n: number of strings in the array or -1 for NULL terminated arrays
> + * @str: string to match with
> + *
> + * Returns index of @str in the @array or -EINVAL, just like match_string().
> + * Uses sysfs_streq() instead of strcmp for matching.
> + */
> +int __sysfs_strmatch(const char * const *array, size_t n, const char *str)
> +{
> +   const char *item;
> +   int index;
> +
> +   for (index = 0; index < n; index++) {
> +   item = array[index];
> +   if (!item)
> +   break;
> +   if (!sysfs_streq(item, str))
> +   return index;
> +   }
> +
> +   return -EINVAL;
> +}
> +EXPORT_SYMBOL(__sysfs_strmatch);
> +
>  #ifndef __HAVE_ARCH_MEMSET
>  /**
>   * memset - Fill a region of memory with the given value
>
>
> --
> 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: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > +static int sysfs_strmatch(const char * const *array, size_t n, const char 
> > *str)
> > +{
> > +   const char *item;
> > +   int index;
> > +
> > +   for (index = 0; index < n; index++) {
> > +   item = array[index];
> > +   if (!item)
> > +   break;
> > +   if (sysfs_streq(item, str))
> > +   return index;
> > +   }
> > +
> > +   return -EINVAL;
> > +}
> 
> should we make this a core sysfs function?

Last question before I send v11. Is the following (the helper) OK?


diff --git a/include/linux/string.h b/include/linux/string.h
index 26b6f6a..5606810 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
 }
 
 int match_string(const char * const *array, size_t n, const char *string);
+int __sysfs_strmatch(const char * const *array, size_t n, const char *string);
+
+/**
+ * sysfs_strmatch - matches given string in an array
+ * @a: array of strings
+ * @s: string to match with
+ *
+ * Helper for __sysfs_strmatch(). Calculates the size of @a automatically.
+ */
+#define sysfs_strmatch(a, s) __sysfs_strmatch(a, ARRAY_SIZE(a), s)
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/string.c b/lib/string.c
index ed83562..a4fe035 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -656,6 +656,32 @@ int match_string(const char * const *array, size_t n, 
const char *string)
 }
 EXPORT_SYMBOL(match_string);
 
+/**
+ * __sysfs_strmatch - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq() instead of strcmp for matching.
+ */
+int __sysfs_strmatch(const char * const *array, size_t n, const char *str)
+{
+   const char *item;
+   int index;
+
+   for (index = 0; index < n; index++) {
+   item = array[index];
+   if (!item)
+   break;
+   if (!sysfs_streq(item, str))
+   return index;
+   }
+
+   return -EINVAL;
+}
+EXPORT_SYMBOL(__sysfs_strmatch);
+
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value


-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > +static int sysfs_strmatch(const char * const *array, size_t n, const char 
> > *str)
> > +{
> > +   const char *item;
> > +   int index;
> > +
> > +   for (index = 0; index < n; index++) {
> > +   item = array[index];
> > +   if (!item)
> > +   break;
> > +   if (sysfs_streq(item, str))
> > +   return index;
> > +   }
> > +
> > +   return -EINVAL;
> > +}
> 
> should we make this a core sysfs function?

Last question before I send v11. Is the following (the helper) OK?


diff --git a/include/linux/string.h b/include/linux/string.h
index 26b6f6a..5606810 100644
--- a/include/linux/string.h
+++ b/include/linux/string.h
@@ -135,6 +135,16 @@ static inline int strtobool(const char *s, bool *res)
 }
 
 int match_string(const char * const *array, size_t n, const char *string);
+int __sysfs_strmatch(const char * const *array, size_t n, const char *string);
+
+/**
+ * sysfs_strmatch - matches given string in an array
+ * @a: array of strings
+ * @s: string to match with
+ *
+ * Helper for __sysfs_strmatch(). Calculates the size of @a automatically.
+ */
+#define sysfs_strmatch(a, s) __sysfs_strmatch(a, ARRAY_SIZE(a), s)
 
 #ifdef CONFIG_BINARY_PRINTF
 int vbin_printf(u32 *bin_buf, size_t size, const char *fmt, va_list args);
diff --git a/lib/string.c b/lib/string.c
index ed83562..a4fe035 100644
--- a/lib/string.c
+++ b/lib/string.c
@@ -656,6 +656,32 @@ int match_string(const char * const *array, size_t n, 
const char *string)
 }
 EXPORT_SYMBOL(match_string);
 
+/**
+ * __sysfs_strmatch - matches given string in an array
+ * @array: array of strings
+ * @n: number of strings in the array or -1 for NULL terminated arrays
+ * @str: string to match with
+ *
+ * Returns index of @str in the @array or -EINVAL, just like match_string().
+ * Uses sysfs_streq() instead of strcmp for matching.
+ */
+int __sysfs_strmatch(const char * const *array, size_t n, const char *str)
+{
+   const char *item;
+   int index;
+
+   for (index = 0; index < n; index++) {
+   item = array[index];
+   if (!item)
+   break;
+   if (!sysfs_streq(item, str))
+   return index;
+   }
+
+   return -EINVAL;
+}
+EXPORT_SYMBOL(__sysfs_strmatch);
+
 #ifndef __HAVE_ARCH_MEMSET
 /**
  * memset - Fill a region of memory with the given value


-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Wed, Nov 16, 2016 at 06:30:23AM -0800, Badhri Jagan Sridharan wrote:
> > IMHO the uevent is cheaper. User space cannot just poll without further
> > infrastructure. A task needs to run to poll. A uevent can be handled
> > through established infrastructure.
> 
> Thanks Oliver for stating this. This is exactly what I was facing.
> 
> > OK, I'll add KOBJ_CHANGE for those.
> >
> > So is it OK to everybody if I remove the KOBJ_CHANGE in
> > typec_connect()? We will see uevent KOBJ_ADD since the partner (or
> > cable) is added in any case. Badhri, Oliver?
> 
> Yes Heikki.. That's OK for me as well.
> Just to get my understanding right. You are planning to add
> KOBJ_CHANGE uevents when current_power_role or
> current_data_role changes and KOBJ_ADD when new port-partner
> or the cable is attached. Is that right ?

Yes, though I don't KOBJ_ADD separately with the partners and cables.
That uevent is sent when the device for them is registered, so it's
already there.


Br,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Wed, Nov 16, 2016 at 06:30:23AM -0800, Badhri Jagan Sridharan wrote:
> > IMHO the uevent is cheaper. User space cannot just poll without further
> > infrastructure. A task needs to run to poll. A uevent can be handled
> > through established infrastructure.
> 
> Thanks Oliver for stating this. This is exactly what I was facing.
> 
> > OK, I'll add KOBJ_CHANGE for those.
> >
> > So is it OK to everybody if I remove the KOBJ_CHANGE in
> > typec_connect()? We will see uevent KOBJ_ADD since the partner (or
> > cable) is added in any case. Badhri, Oliver?
> 
> Yes Heikki.. That's OK for me as well.
> Just to get my understanding right. You are planning to add
> KOBJ_CHANGE uevents when current_power_role or
> current_data_role changes and KOBJ_ADD when new port-partner
> or the cable is attached. Is that right ?

Yes, though I don't KOBJ_ADD separately with the partners and cables.
That uevent is sent when the device for them is registered, so it's
already there.


Br,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Badhri Jagan Sridharan]

> IMHO the uevent is cheaper. User space cannot just poll without further
> infrastructure. A task needs to run to poll. A uevent can be handled
> through established infrastructure.

Thanks Oliver for stating this. This is exactly what I was facing.

> OK, I'll add KOBJ_CHANGE for those.
>
> So is it OK to everybody if I remove the KOBJ_CHANGE in
> typec_connect()? We will see uevent KOBJ_ADD since the partner (or
> cable) is added in any case. Badhri, Oliver?

Yes Heikki.. That's OK for me as well.
Just to get my understanding right. You are planning to add
KOBJ_CHANGE uevents when current_power_role or
current_data_role changes and KOBJ_ADD when new port-partner
or the cable is attached. Is that right ?

Thanks,
Badhri.

On Wed, Nov 16, 2016 at 3:27 AM, Oliver Neukum  wrote:
> On Wed, 2016-11-16 at 13:09 +0200, Heikki Krogerus wrote:
>
>> OK, I'll add KOBJ_CHANGE for those.
>>
>> So is it OK to everybody if I remove the KOBJ_CHANGE in
>> typec_connect()? We will see uevent KOBJ_ADD since the partner (or
>> cable) is added in any case. Badhri, Oliver?
>
> OK by me.
>
> Regards
> Oliver
>
>

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Badhri Jagan Sridharan]

> IMHO the uevent is cheaper. User space cannot just poll without further
> infrastructure. A task needs to run to poll. A uevent can be handled
> through established infrastructure.

Thanks Oliver for stating this. This is exactly what I was facing.

> OK, I'll add KOBJ_CHANGE for those.
>
> So is it OK to everybody if I remove the KOBJ_CHANGE in
> typec_connect()? We will see uevent KOBJ_ADD since the partner (or
> cable) is added in any case. Badhri, Oliver?

Yes Heikki.. That's OK for me as well.
Just to get my understanding right. You are planning to add
KOBJ_CHANGE uevents when current_power_role or
current_data_role changes and KOBJ_ADD when new port-partner
or the cable is attached. Is that right ?

Thanks,
Badhri.

On Wed, Nov 16, 2016 at 3:27 AM, Oliver Neukum  wrote:
> On Wed, 2016-11-16 at 13:09 +0200, Heikki Krogerus wrote:
>
>> OK, I'll add KOBJ_CHANGE for those.
>>
>> So is it OK to everybody if I remove the KOBJ_CHANGE in
>> typec_connect()? We will see uevent KOBJ_ADD since the partner (or
>> cable) is added in any case. Badhri, Oliver?
>
> OK by me.
>
> Regards
> Oliver
>
>

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Oliver Neukum]

On Wed, 2016-11-16 at 13:09 +0200, Heikki Krogerus wrote:

> OK, I'll add KOBJ_CHANGE for those.
> 
> So is it OK to everybody if I remove the KOBJ_CHANGE in
> typec_connect()? We will see uevent KOBJ_ADD since the partner (or
> cable) is added in any case. Badhri, Oliver?

OK by me.

Regards
Oliver

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Oliver Neukum]

On Wed, 2016-11-16 at 13:09 +0200, Heikki Krogerus wrote:

> OK, I'll add KOBJ_CHANGE for those.
> 
> So is it OK to everybody if I remove the KOBJ_CHANGE in
> typec_connect()? We will see uevent KOBJ_ADD since the partner (or
> cable) is added in any case. Badhri, Oliver?

OK by me.

Regards
Oliver

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Wed, Nov 16, 2016 at 10:49:49AM +0100, Greg KH wrote:
> On Wed, Nov 16, 2016 at 11:30:35AM +0200, Heikki Krogerus wrote:
> > On Tue, Nov 15, 2016 at 04:19:10PM -0800, Badhri Jagan Sridharan wrote:
> > > Hi,
> > > 
> > > At present I am using the uevent in the userspace to infer
> > > the Presence of a port on the remote end through the
> > > appearance of usbc*-partner.
> > > 
> > > Userspace uses this info to decide on when to show a USB
> > > notification on the screen and what should be the options
> > > provided in the dialog.
> > > 
> > > I was assuming that this is not something that would be dropped.
> > > 
> > > Coding using events was relatively easier to program from userspace ..
> > > 
> > > Is it possible to use POLL for identifying the appearance of port partner 
> > > ?
> > > I did not notice sysfs_notify call in typec_connect/typec_disconnect.
> > > 
> > > It would also be nice to have uevent notifications when the contents
> > > of current_data_role or current_power_role changes.
> > > 
> > > Is that too costly to have ?
> > 
> > Greg, could you give your opinion. In this case we do have attribute
> > files that the user space can poll. Data role is the USB data role, so
> > host or device, and it can change for example if the partner executes
> > a swap. The same can happen with the power role.
> 
> So the same 'struct device' switches roles and attribute files are
> updated that need to be re-read?  If so, yes KOBJ_CHANGE is correct, if
> a struct device is added/removed for this, then no, it doesn't make
> sense.

OK, I'll add KOBJ_CHANGE for those.

So is it OK to everybody if I remove the KOBJ_CHANGE in
typec_connect()? We will see uevent KOBJ_ADD since the partner (or
cable) is added in any case. Badhri, Oliver?


Thanks,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Wed, Nov 16, 2016 at 10:49:49AM +0100, Greg KH wrote:
> On Wed, Nov 16, 2016 at 11:30:35AM +0200, Heikki Krogerus wrote:
> > On Tue, Nov 15, 2016 at 04:19:10PM -0800, Badhri Jagan Sridharan wrote:
> > > Hi,
> > > 
> > > At present I am using the uevent in the userspace to infer
> > > the Presence of a port on the remote end through the
> > > appearance of usbc*-partner.
> > > 
> > > Userspace uses this info to decide on when to show a USB
> > > notification on the screen and what should be the options
> > > provided in the dialog.
> > > 
> > > I was assuming that this is not something that would be dropped.
> > > 
> > > Coding using events was relatively easier to program from userspace ..
> > > 
> > > Is it possible to use POLL for identifying the appearance of port partner 
> > > ?
> > > I did not notice sysfs_notify call in typec_connect/typec_disconnect.
> > > 
> > > It would also be nice to have uevent notifications when the contents
> > > of current_data_role or current_power_role changes.
> > > 
> > > Is that too costly to have ?
> > 
> > Greg, could you give your opinion. In this case we do have attribute
> > files that the user space can poll. Data role is the USB data role, so
> > host or device, and it can change for example if the partner executes
> > a swap. The same can happen with the power role.
> 
> So the same 'struct device' switches roles and attribute files are
> updated that need to be re-read?  If so, yes KOBJ_CHANGE is correct, if
> a struct device is added/removed for this, then no, it doesn't make
> sense.

OK, I'll add KOBJ_CHANGE for those.

So is it OK to everybody if I remove the KOBJ_CHANGE in
typec_connect()? We will see uevent KOBJ_ADD since the partner (or
cable) is added in any case. Badhri, Oliver?


Thanks,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Wed, Nov 16, 2016 at 11:30:35AM +0200, Heikki Krogerus wrote:
> On Tue, Nov 15, 2016 at 04:19:10PM -0800, Badhri Jagan Sridharan wrote:
> > Hi,
> > 
> > At present I am using the uevent in the userspace to infer
> > the Presence of a port on the remote end through the
> > appearance of usbc*-partner.
> > 
> > Userspace uses this info to decide on when to show a USB
> > notification on the screen and what should be the options
> > provided in the dialog.
> > 
> > I was assuming that this is not something that would be dropped.
> > 
> > Coding using events was relatively easier to program from userspace ..
> > 
> > Is it possible to use POLL for identifying the appearance of port partner ?
> > I did not notice sysfs_notify call in typec_connect/typec_disconnect.
> > 
> > It would also be nice to have uevent notifications when the contents
> > of current_data_role or current_power_role changes.
> > 
> > Is that too costly to have ?
> 
> Greg, could you give your opinion. In this case we do have attribute
> files that the user space can poll. Data role is the USB data role, so
> host or device, and it can change for example if the partner executes
> a swap. The same can happen with the power role.

So the same 'struct device' switches roles and attribute files are
updated that need to be re-read?  If so, yes KOBJ_CHANGE is correct, if
a struct device is added/removed for this, then no, it doesn't make
sense.

Again, document this to describe what is happening and it might be more
obvious to people and so these questions would not come up :)

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Wed, Nov 16, 2016 at 11:30:35AM +0200, Heikki Krogerus wrote:
> On Tue, Nov 15, 2016 at 04:19:10PM -0800, Badhri Jagan Sridharan wrote:
> > Hi,
> > 
> > At present I am using the uevent in the userspace to infer
> > the Presence of a port on the remote end through the
> > appearance of usbc*-partner.
> > 
> > Userspace uses this info to decide on when to show a USB
> > notification on the screen and what should be the options
> > provided in the dialog.
> > 
> > I was assuming that this is not something that would be dropped.
> > 
> > Coding using events was relatively easier to program from userspace ..
> > 
> > Is it possible to use POLL for identifying the appearance of port partner ?
> > I did not notice sysfs_notify call in typec_connect/typec_disconnect.
> > 
> > It would also be nice to have uevent notifications when the contents
> > of current_data_role or current_power_role changes.
> > 
> > Is that too costly to have ?
> 
> Greg, could you give your opinion. In this case we do have attribute
> files that the user space can poll. Data role is the USB data role, so
> host or device, and it can change for example if the partner executes
> a swap. The same can happen with the power role.

So the same 'struct device' switches roles and attribute files are
updated that need to be re-read?  If so, yes KOBJ_CHANGE is correct, if
a struct device is added/removed for this, then no, it doesn't make
sense.

Again, document this to describe what is happening and it might be more
obvious to people and so these questions would not come up :)

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Oliver Neukum]

On Wed, 2016-11-16 at 11:30 +0200, Heikki Krogerus wrote:
> On Tue, Nov 15, 2016 at 04:19:10PM -0800, Badhri Jagan Sridharan wrote:

> > Is that too costly to have ?
> 
> Greg, could you give your opinion. In this case we do have attribute
> files that the user space can poll. Data role is the USB data role, so
> host or device, and it can change for example if the partner executes
> a swap. The same can happen with the power role.

IMHO the uevent is cheaper. User space cannot just poll without further
infrastructure. A task needs to run to poll. A uevent can be handled
through established infrastructure.
Sure from a kernel level it is the heavier gun, but I think this is
the wrong angle of looking at this issue.

Regards
Oliver

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Oliver Neukum]

On Wed, 2016-11-16 at 11:30 +0200, Heikki Krogerus wrote:
> On Tue, Nov 15, 2016 at 04:19:10PM -0800, Badhri Jagan Sridharan wrote:

> > Is that too costly to have ?
> 
> Greg, could you give your opinion. In this case we do have attribute
> files that the user space can poll. Data role is the USB data role, so
> host or device, and it can change for example if the partner executes
> a swap. The same can happen with the power role.

IMHO the uevent is cheaper. User space cannot just poll without further
infrastructure. A task needs to run to poll. A uevent can be handled
through established infrastructure.
Sure from a kernel level it is the heavier gun, but I think this is
the wrong angle of looking at this issue.

Regards
Oliver

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Tue, Nov 15, 2016 at 04:19:10PM -0800, Badhri Jagan Sridharan wrote:
> Hi,
> 
> At present I am using the uevent in the userspace to infer
> the Presence of a port on the remote end through the
> appearance of usbc*-partner.
> 
> Userspace uses this info to decide on when to show a USB
> notification on the screen and what should be the options
> provided in the dialog.
> 
> I was assuming that this is not something that would be dropped.
> 
> Coding using events was relatively easier to program from userspace ..
> 
> Is it possible to use POLL for identifying the appearance of port partner ?
> I did not notice sysfs_notify call in typec_connect/typec_disconnect.
> 
> It would also be nice to have uevent notifications when the contents
> of current_data_role or current_power_role changes.
> 
> Is that too costly to have ?

Greg, could you give your opinion. In this case we do have attribute
files that the user space can poll. Data role is the USB data role, so
host or device, and it can change for example if the partner executes
a swap. The same can happen with the power role.


Thanks,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Tue, Nov 15, 2016 at 04:19:10PM -0800, Badhri Jagan Sridharan wrote:
> Hi,
> 
> At present I am using the uevent in the userspace to infer
> the Presence of a port on the remote end through the
> appearance of usbc*-partner.
> 
> Userspace uses this info to decide on when to show a USB
> notification on the screen and what should be the options
> provided in the dialog.
> 
> I was assuming that this is not something that would be dropped.
> 
> Coding using events was relatively easier to program from userspace ..
> 
> Is it possible to use POLL for identifying the appearance of port partner ?
> I did not notice sysfs_notify call in typec_connect/typec_disconnect.
> 
> It would also be nice to have uevent notifications when the contents
> of current_data_role or current_power_role changes.
> 
> Is that too costly to have ?

Greg, could you give your opinion. In this case we do have attribute
files that the user space can poll. Data role is the USB data role, so
host or device, and it can change for example if the partner executes
a swap. The same can happen with the power role.


Thanks,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Oliver Neukum]

On Mon, 2016-11-14 at 06:34 -0800, Guenter Roeck wrote:
> >>> +int typec_connect(struct typec_port *port, struct
> typec_connection *con)
> >>> +{
> >>> +   int ret;
> >>> +
> >>> +   if (!con->partner && !con->cable)
> >>> +   return -EINVAL;
> >>> +
> >>> +   port->connected = 1;
> >>> +   port->data_role = con->data_role;
> >>> +   port->pwr_role = con->pwr_role;
> >>> +   port->vconn_role = con->vconn_role;
> >>> +   port->pwr_opmode = con->pwr_opmode;
> >>> +
> >>> +   kobject_uevent(>dev.kobj, KOBJ_CHANGE);
> >>
> >> This worries me.  Who is listening for it?  What will you do with
> it?
> >> Shouldn't you just poll on an attribute file instead?
> >
> > Oliver! Did you need this or can we remove it?
> >
> 
> I'll also have to make sure that the Android folks don't use it.

How then do we notify user space? poll()? Yet another demon.

I do not specifically need this, but I note that uevents are the
general tool we use to notify user space of that kind of events.

Regards
Oliver

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Oliver Neukum]

On Mon, 2016-11-14 at 06:34 -0800, Guenter Roeck wrote:
> >>> +int typec_connect(struct typec_port *port, struct
> typec_connection *con)
> >>> +{
> >>> +   int ret;
> >>> +
> >>> +   if (!con->partner && !con->cable)
> >>> +   return -EINVAL;
> >>> +
> >>> +   port->connected = 1;
> >>> +   port->data_role = con->data_role;
> >>> +   port->pwr_role = con->pwr_role;
> >>> +   port->vconn_role = con->vconn_role;
> >>> +   port->pwr_opmode = con->pwr_opmode;
> >>> +
> >>> +   kobject_uevent(>dev.kobj, KOBJ_CHANGE);
> >>
> >> This worries me.  Who is listening for it?  What will you do with
> it?
> >> Shouldn't you just poll on an attribute file instead?
> >
> > Oliver! Did you need this or can we remove it?
> >
> 
> I'll also have to make sure that the Android folks don't use it.

How then do we notify user space? poll()? Yet another demon.

I do not specifically need this, but I note that uevents are the
general tool we use to notify user space of that kind of events.

Regards
Oliver

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Badhri Jagan Sridharan]

Hi,

At present I am using the uevent in the userspace to infer
the Presence of a port on the remote end through the
appearance of usbc*-partner.

Userspace uses this info to decide on when to show a USB
notification on the screen and what should be the options
provided in the dialog.

I was assuming that this is not something that would be dropped.

Coding using events was relatively easier to program from userspace ..

Is it possible to use POLL for identifying the appearance of port partner ?
I did not notice sysfs_notify call in typec_connect/typec_disconnect.

It would also be nice to have uevent notifications when the contents
of current_data_role or current_power_role changes.

Is that too costly to have ?

Thanks,
Badhri.


On Tue, Nov 15, 2016 at 1:25 AM, Guenter Roeck  wrote:
> On 11/14/2016 11:07 PM, Greg KH wrote:
>>
>> On Mon, Nov 14, 2016 at 12:46:50PM -0800, Guenter Roeck wrote:
>>>
>>> On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:

 Hi Greg,

 On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
>
> On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
>>
>> Reviewed-by: Guenter Roeck 
>> Tested-by: Guenter Roeck 
>> Signed-off-by: Heikki Krogerus 
>> ---
>>  Documentation/ABI/testing/sysfs-class-typec |  218 ++
>>  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   | 1075
>> +++
>>  include/linux/usb/typec.h   |  252 +++
>>  9 files changed, 1669 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
>
>
>>> [ ... ]
>>>
>> +
>> +int typec_connect(struct typec_port *port, struct typec_connection
>> *con)
>> +{
>> +   int ret;
>> +
>> +   if (!con->partner && !con->cable)
>> +   return -EINVAL;
>> +
>> +   port->connected = 1;
>> +   port->data_role = con->data_role;
>> +   port->pwr_role = con->pwr_role;
>> +   port->vconn_role = con->vconn_role;
>> +   port->pwr_opmode = con->pwr_opmode;
>> +
>> +   kobject_uevent(>dev.kobj, KOBJ_CHANGE);
>
>
> This worries me.  Who is listening for it?  What will you do with it?
> Shouldn't you just poll on an attribute file instead?


 Oliver! Did you need this or can we remove it?

 I remember I removed the "connected" attribute because you did not see
 any use for it at one point. I don't remember the reason exactly why?

>>>
>>> The Android team tells me that they are currently using the udev events
>>> to track port role changes, and to detect presence of port partner.
>>>
>>> Also, there are plans to track changes on usbc*cable to differentiate
>>> between cable attach vs. device being attached on the remote end.
>>>
>>> What is the problem with using kobject_uevent() and thus presumably
>>> udev events ?
>>
>>
>> It's not a "normal" thing to do and is pretty "heavy" to do.  What does
>> userspace do with that change event?  Does it read specific attributes?
>> What causes the event to happen in the kernel, is it really just a
>> change in the specific object, or do new ones get added/removed?
>>
>> In short, document the heck out of this please so people know how to use
>> it, and what is happening when the event happens.
>>
>
> Badhri,
>
> can you clarify which events you are using in detail, and what for ?
>
> Thanks,
> Guenter
>

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Badhri Jagan Sridharan]

Hi,

At present I am using the uevent in the userspace to infer
the Presence of a port on the remote end through the
appearance of usbc*-partner.

Userspace uses this info to decide on when to show a USB
notification on the screen and what should be the options
provided in the dialog.

I was assuming that this is not something that would be dropped.

Coding using events was relatively easier to program from userspace ..

Is it possible to use POLL for identifying the appearance of port partner ?
I did not notice sysfs_notify call in typec_connect/typec_disconnect.

It would also be nice to have uevent notifications when the contents
of current_data_role or current_power_role changes.

Is that too costly to have ?

Thanks,
Badhri.


On Tue, Nov 15, 2016 at 1:25 AM, Guenter Roeck  wrote:
> On 11/14/2016 11:07 PM, Greg KH wrote:
>>
>> On Mon, Nov 14, 2016 at 12:46:50PM -0800, Guenter Roeck wrote:
>>>
>>> On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:

 Hi Greg,

 On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
>
> On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
>>
>> Reviewed-by: Guenter Roeck 
>> Tested-by: Guenter Roeck 
>> Signed-off-by: Heikki Krogerus 
>> ---
>>  Documentation/ABI/testing/sysfs-class-typec |  218 ++
>>  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   | 1075
>> +++
>>  include/linux/usb/typec.h   |  252 +++
>>  9 files changed, 1669 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
>
>
>>> [ ... ]
>>>
>> +
>> +int typec_connect(struct typec_port *port, struct typec_connection
>> *con)
>> +{
>> +   int ret;
>> +
>> +   if (!con->partner && !con->cable)
>> +   return -EINVAL;
>> +
>> +   port->connected = 1;
>> +   port->data_role = con->data_role;
>> +   port->pwr_role = con->pwr_role;
>> +   port->vconn_role = con->vconn_role;
>> +   port->pwr_opmode = con->pwr_opmode;
>> +
>> +   kobject_uevent(>dev.kobj, KOBJ_CHANGE);
>
>
> This worries me.  Who is listening for it?  What will you do with it?
> Shouldn't you just poll on an attribute file instead?


 Oliver! Did you need this or can we remove it?

 I remember I removed the "connected" attribute because you did not see
 any use for it at one point. I don't remember the reason exactly why?

>>>
>>> The Android team tells me that they are currently using the udev events
>>> to track port role changes, and to detect presence of port partner.
>>>
>>> Also, there are plans to track changes on usbc*cable to differentiate
>>> between cable attach vs. device being attached on the remote end.
>>>
>>> What is the problem with using kobject_uevent() and thus presumably
>>> udev events ?
>>
>>
>> It's not a "normal" thing to do and is pretty "heavy" to do.  What does
>> userspace do with that change event?  Does it read specific attributes?
>> What causes the event to happen in the kernel, is it really just a
>> change in the specific object, or do new ones get added/removed?
>>
>> In short, document the heck out of this please so people know how to use
>> it, and what is happening when the event happens.
>>
>
> Badhri,
>
> can you clarify which events you are using in detail, and what for ?
>
> Thanks,
> Guenter
>

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On 11/14/2016 11:07 PM, Greg KH wrote:

On Mon, Nov 14, 2016 at 12:46:50PM -0800, Guenter Roeck wrote:

On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:

On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.

Reviewed-by: Guenter Roeck 
Tested-by: Guenter Roeck 
Signed-off-by: Heikki Krogerus 
---
 Documentation/ABI/testing/sysfs-class-typec |  218 ++
 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   | 1075 +++
 include/linux/usb/typec.h   |  252 +++
 9 files changed, 1669 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



[ ... ]


+
+int typec_connect(struct typec_port *port, struct typec_connection *con)
+{
+   int ret;
+
+   if (!con->partner && !con->cable)
+   return -EINVAL;
+
+   port->connected = 1;
+   port->data_role = con->data_role;
+   port->pwr_role = con->pwr_role;
+   port->vconn_role = con->vconn_role;
+   port->pwr_opmode = con->pwr_opmode;
+
+   kobject_uevent(>dev.kobj, KOBJ_CHANGE);


This worries me.  Who is listening for it?  What will you do with it?
Shouldn't you just poll on an attribute file instead?


Oliver! Did you need this or can we remove it?

I remember I removed the "connected" attribute because you did not see
any use for it at one point. I don't remember the reason exactly why?



The Android team tells me that they are currently using the udev events
to track port role changes, and to detect presence of port partner.

Also, there are plans to track changes on usbc*cable to differentiate
between cable attach vs. device being attached on the remote end.

What is the problem with using kobject_uevent() and thus presumably
udev events ?


It's not a "normal" thing to do and is pretty "heavy" to do.  What does
userspace do with that change event?  Does it read specific attributes?
What causes the event to happen in the kernel, is it really just a
change in the specific object, or do new ones get added/removed?

In short, document the heck out of this please so people know how to use
it, and what is happening when the event happens.



Badhri,

can you clarify which events you are using in detail, and what for ?

Thanks,
Guenter

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On 11/14/2016 11:07 PM, Greg KH wrote:

On Mon, Nov 14, 2016 at 12:46:50PM -0800, Guenter Roeck wrote:

On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:

On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.

Reviewed-by: Guenter Roeck 
Tested-by: Guenter Roeck 
Signed-off-by: Heikki Krogerus 
---
 Documentation/ABI/testing/sysfs-class-typec |  218 ++
 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   | 1075 +++
 include/linux/usb/typec.h   |  252 +++
 9 files changed, 1669 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



[ ... ]


+
+int typec_connect(struct typec_port *port, struct typec_connection *con)
+{
+   int ret;
+
+   if (!con->partner && !con->cable)
+   return -EINVAL;
+
+   port->connected = 1;
+   port->data_role = con->data_role;
+   port->pwr_role = con->pwr_role;
+   port->vconn_role = con->vconn_role;
+   port->pwr_opmode = con->pwr_opmode;
+
+   kobject_uevent(>dev.kobj, KOBJ_CHANGE);


This worries me.  Who is listening for it?  What will you do with it?
Shouldn't you just poll on an attribute file instead?


Oliver! Did you need this or can we remove it?

I remember I removed the "connected" attribute because you did not see
any use for it at one point. I don't remember the reason exactly why?



The Android team tells me that they are currently using the udev events
to track port role changes, and to detect presence of port partner.

Also, there are plans to track changes on usbc*cable to differentiate
between cable attach vs. device being attached on the remote end.

What is the problem with using kobject_uevent() and thus presumably
udev events ?


It's not a "normal" thing to do and is pretty "heavy" to do.  What does
userspace do with that change event?  Does it read specific attributes?
What causes the event to happen in the kernel, is it really just a
change in the specific object, or do new ones get added/removed?

In short, document the heck out of this please so people know how to use
it, and what is happening when the event happens.



Badhri,

can you clarify which events you are using in detail, and what for ?

Thanks,
Guenter

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Mon, Nov 14, 2016 at 12:46:50PM -0800, Guenter Roeck wrote:
> On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> > Hi Greg,
> > 
> > On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > > On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
> > > > 
> > > > Reviewed-by: Guenter Roeck 
> > > > Tested-by: Guenter Roeck 
> > > > Signed-off-by: Heikki Krogerus 
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-class-typec |  218 ++
> > > >  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   | 1075 
> > > > +++
> > > >  include/linux/usb/typec.h   |  252 +++
> > > >  9 files changed, 1669 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
> > > 
> [ ... ]
> 
> > > > +
> > > > +int typec_connect(struct typec_port *port, struct typec_connection 
> > > > *con)
> > > > +{
> > > > +   int ret;
> > > > +
> > > > +   if (!con->partner && !con->cable)
> > > > +   return -EINVAL;
> > > > +
> > > > +   port->connected = 1;
> > > > +   port->data_role = con->data_role;
> > > > +   port->pwr_role = con->pwr_role;
> > > > +   port->vconn_role = con->vconn_role;
> > > > +   port->pwr_opmode = con->pwr_opmode;
> > > > +
> > > > +   kobject_uevent(>dev.kobj, KOBJ_CHANGE);
> > > 
> > > This worries me.  Who is listening for it?  What will you do with it?
> > > Shouldn't you just poll on an attribute file instead?
> > 
> > Oliver! Did you need this or can we remove it?
> > 
> > I remember I removed the "connected" attribute because you did not see
> > any use for it at one point. I don't remember the reason exactly why?
> > 
> 
> The Android team tells me that they are currently using the udev events
> to track port role changes, and to detect presence of port partner.
> 
> Also, there are plans to track changes on usbc*cable to differentiate
> between cable attach vs. device being attached on the remote end. 
> 
> What is the problem with using kobject_uevent() and thus presumably
> udev events ?

It's not a "normal" thing to do and is pretty "heavy" to do.  What does
userspace do with that change event?  Does it read specific attributes?
What causes the event to happen in the kernel, is it really just a
change in the specific object, or do new ones get added/removed?

In short, document the heck out of this please so people know how to use
it, and what is happening when the event happens.

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Mon, Nov 14, 2016 at 12:46:50PM -0800, Guenter Roeck wrote:
> On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> > Hi Greg,
> > 
> > On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > > On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
> > > > 
> > > > Reviewed-by: Guenter Roeck 
> > > > Tested-by: Guenter Roeck 
> > > > Signed-off-by: Heikki Krogerus 
> > > > ---
> > > >  Documentation/ABI/testing/sysfs-class-typec |  218 ++
> > > >  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   | 1075 
> > > > +++
> > > >  include/linux/usb/typec.h   |  252 +++
> > > >  9 files changed, 1669 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
> > > 
> [ ... ]
> 
> > > > +
> > > > +int typec_connect(struct typec_port *port, struct typec_connection 
> > > > *con)
> > > > +{
> > > > +   int ret;
> > > > +
> > > > +   if (!con->partner && !con->cable)
> > > > +   return -EINVAL;
> > > > +
> > > > +   port->connected = 1;
> > > > +   port->data_role = con->data_role;
> > > > +   port->pwr_role = con->pwr_role;
> > > > +   port->vconn_role = con->vconn_role;
> > > > +   port->pwr_opmode = con->pwr_opmode;
> > > > +
> > > > +   kobject_uevent(>dev.kobj, KOBJ_CHANGE);
> > > 
> > > This worries me.  Who is listening for it?  What will you do with it?
> > > Shouldn't you just poll on an attribute file instead?
> > 
> > Oliver! Did you need this or can we remove it?
> > 
> > I remember I removed the "connected" attribute because you did not see
> > any use for it at one point. I don't remember the reason exactly why?
> > 
> 
> The Android team tells me that they are currently using the udev events
> to track port role changes, and to detect presence of port partner.
> 
> Also, there are plans to track changes on usbc*cable to differentiate
> between cable attach vs. device being attached on the remote end. 
> 
> What is the problem with using kobject_uevent() and thus presumably
> udev events ?

It's not a "normal" thing to do and is pretty "heavy" to do.  What does
userspace do with that change event?  Does it read specific attributes?
What causes the event to happen in the kernel, is it really just a
change in the specific object, or do new ones get added/removed?

In short, document the heck out of this please so people know how to use
it, and what is happening when the event happens.

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
> > > 
> > > Reviewed-by: Guenter Roeck 
> > > Tested-by: Guenter Roeck 
> > > Signed-off-by: Heikki Krogerus 
> > > ---
> > >  Documentation/ABI/testing/sysfs-class-typec |  218 ++
> > >  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   | 1075 
> > > +++
> > >  include/linux/usb/typec.h   |  252 +++
> > >  9 files changed, 1669 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
> > 
[ ... ]

> > > +
> > > +int typec_connect(struct typec_port *port, struct typec_connection *con)
> > > +{
> > > + int ret;
> > > +
> > > + if (!con->partner && !con->cable)
> > > + return -EINVAL;
> > > +
> > > + port->connected = 1;
> > > + port->data_role = con->data_role;
> > > + port->pwr_role = con->pwr_role;
> > > + port->vconn_role = con->vconn_role;
> > > + port->pwr_opmode = con->pwr_opmode;
> > > +
> > > + kobject_uevent(>dev.kobj, KOBJ_CHANGE);
> > 
> > This worries me.  Who is listening for it?  What will you do with it?
> > Shouldn't you just poll on an attribute file instead?
> 
> Oliver! Did you need this or can we remove it?
> 
> I remember I removed the "connected" attribute because you did not see
> any use for it at one point. I don't remember the reason exactly why?
> 

The Android team tells me that they are currently using the udev events
to track port role changes, and to detect presence of port partner.

Also, there are plans to track changes on usbc*cable to differentiate
between cable attach vs. device being attached on the remote end. 

What is the problem with using kobject_uevent() and thus presumably
udev events ?

Thanks,
Guenter

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> Hi Greg,
> 
> On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> > On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
> > > 
> > > Reviewed-by: Guenter Roeck 
> > > Tested-by: Guenter Roeck 
> > > Signed-off-by: Heikki Krogerus 
> > > ---
> > >  Documentation/ABI/testing/sysfs-class-typec |  218 ++
> > >  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   | 1075 
> > > +++
> > >  include/linux/usb/typec.h   |  252 +++
> > >  9 files changed, 1669 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
> > 
[ ... ]

> > > +
> > > +int typec_connect(struct typec_port *port, struct typec_connection *con)
> > > +{
> > > + int ret;
> > > +
> > > + if (!con->partner && !con->cable)
> > > + return -EINVAL;
> > > +
> > > + port->connected = 1;
> > > + port->data_role = con->data_role;
> > > + port->pwr_role = con->pwr_role;
> > > + port->vconn_role = con->vconn_role;
> > > + port->pwr_opmode = con->pwr_opmode;
> > > +
> > > + kobject_uevent(>dev.kobj, KOBJ_CHANGE);
> > 
> > This worries me.  Who is listening for it?  What will you do with it?
> > Shouldn't you just poll on an attribute file instead?
> 
> Oliver! Did you need this or can we remove it?
> 
> I remember I removed the "connected" attribute because you did not see
> any use for it at one point. I don't remember the reason exactly why?
> 

The Android team tells me that they are currently using the udev events
to track port role changes, and to detect presence of port partner.

Also, there are plans to track changes on usbc*cable to differentiate
between cable attach vs. device being attached on the remote end. 

What is the problem with using kobject_uevent() and thus presumably
udev events ?

Thanks,
Guenter

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Mon, Nov 14, 2016 at 04:39:10PM +0200, Heikki Krogerus wrote:
> On Mon, Nov 14, 2016 at 03:11:23PM +0100, Greg KH wrote:
> > On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> > > > > +static void __exit typec_exit(void)
> > > > > +{
> > > > > + class_unregister(_class);
> > > > 
> > > > You forgot to clean up your idr :(
> > > 
> > > Sorry, what idr? The port ids get removed in typec_release().
> > 
> > You have a static idr structure in the driver, right?  You have to clean
> > it up when your code is going away so that it will free any memory it
> > had allocated with a call to idr_destroy() on module exit.
> 
> Ok.
> 
> Regarding the DEVICE_ATTR* macros. So I have attributes with same
> names for different device types. I may be able to identify the device
> types and deal with the correct attribute based on that, but for
> example the attribute "active" with alternate modes is writable, but
> with cables it's not. How do I handle those?

The attribute init callback should let you handle this, right?  You'll
have to add it for your dynamic attributes.

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Mon, Nov 14, 2016 at 04:39:10PM +0200, Heikki Krogerus wrote:
> On Mon, Nov 14, 2016 at 03:11:23PM +0100, Greg KH wrote:
> > On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> > > > > +static void __exit typec_exit(void)
> > > > > +{
> > > > > + class_unregister(_class);
> > > > 
> > > > You forgot to clean up your idr :(
> > > 
> > > Sorry, what idr? The port ids get removed in typec_release().
> > 
> > You have a static idr structure in the driver, right?  You have to clean
> > it up when your code is going away so that it will free any memory it
> > had allocated with a call to idr_destroy() on module exit.
> 
> Ok.
> 
> Regarding the DEVICE_ATTR* macros. So I have attributes with same
> names for different device types. I may be able to identify the device
> types and deal with the correct attribute based on that, but for
> example the attribute "active" with alternate modes is writable, but
> with cables it's not. How do I handle those?

The attribute init callback should let you handle this, right?  You'll
have to add it for your dynamic attributes.

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Mon, Nov 14, 2016 at 03:11:23PM +0100, Greg KH wrote:
> On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> > > > +static void __exit typec_exit(void)
> > > > +{
> > > > +   class_unregister(_class);
> > > 
> > > You forgot to clean up your idr :(
> > 
> > Sorry, what idr? The port ids get removed in typec_release().
> 
> You have a static idr structure in the driver, right?  You have to clean
> it up when your code is going away so that it will free any memory it
> had allocated with a call to idr_destroy() on module exit.

Ok.

Regarding the DEVICE_ATTR* macros. So I have attributes with same
names for different device types. I may be able to identify the device
types and deal with the correct attribute based on that, but for
example the attribute "active" with alternate modes is writable, but
with cables it's not. How do I handle those?


Thanks,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

On Mon, Nov 14, 2016 at 03:11:23PM +0100, Greg KH wrote:
> On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> > > > +static void __exit typec_exit(void)
> > > > +{
> > > > +   class_unregister(_class);
> > > 
> > > You forgot to clean up your idr :(
> > 
> > Sorry, what idr? The port ids get removed in typec_release().
> 
> You have a static idr structure in the driver, right?  You have to clean
> it up when your code is going away so that it will free any memory it
> had allocated with a call to idr_destroy() on module exit.

Ok.

Regarding the DEVICE_ATTR* macros. So I have attributes with same
names for different device types. I may be able to identify the device
types and deal with the correct attribute based on that, but for
example the attribute "active" with alternate modes is writable, but
with cables it's not. How do I handle those?


Thanks,

-- 
heikki

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On 11/14/2016 04:32 AM, Heikki Krogerus wrote:

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:

On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.

Reviewed-by: Guenter Roeck 
Tested-by: Guenter Roeck 
Signed-off-by: Heikki Krogerus 


[ ... ]


+
+int typec_connect(struct typec_port *port, struct typec_connection *con)
+{
+   int ret;
+
+   if (!con->partner && !con->cable)
+   return -EINVAL;
+
+   port->connected = 1;
+   port->data_role = con->data_role;
+   port->pwr_role = con->pwr_role;
+   port->vconn_role = con->vconn_role;
+   port->pwr_opmode = con->pwr_opmode;
+
+   kobject_uevent(>dev.kobj, KOBJ_CHANGE);


This worries me.  Who is listening for it?  What will you do with it?
Shouldn't you just poll on an attribute file instead?


Oliver! Did you need this or can we remove it?



I'll also have to make sure that the Android folks don't use it.

Guenter

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Guenter Roeck]

On 11/14/2016 04:32 AM, Heikki Krogerus wrote:

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:

On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.

Reviewed-by: Guenter Roeck 
Tested-by: Guenter Roeck 
Signed-off-by: Heikki Krogerus 


[ ... ]


+
+int typec_connect(struct typec_port *port, struct typec_connection *con)
+{
+   int ret;
+
+   if (!con->partner && !con->cable)
+   return -EINVAL;
+
+   port->connected = 1;
+   port->data_role = con->data_role;
+   port->pwr_role = con->pwr_role;
+   port->vconn_role = con->vconn_role;
+   port->pwr_opmode = con->pwr_opmode;
+
+   kobject_uevent(>dev.kobj, KOBJ_CHANGE);


This worries me.  Who is listening for it?  What will you do with it?
Shouldn't you just poll on an attribute file instead?


Oliver! Did you need this or can we remove it?



I'll also have to make sure that the Android folks don't use it.

Guenter

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> > > +static void __exit typec_exit(void)
> > > +{
> > > + class_unregister(_class);
> > 
> > You forgot to clean up your idr :(
> 
> Sorry, what idr? The port ids get removed in typec_release().

You have a static idr structure in the driver, right?  You have to clean
it up when your code is going away so that it will free any memory it
had allocated with a call to idr_destroy() on module exit.

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Mon, Nov 14, 2016 at 02:32:35PM +0200, Heikki Krogerus wrote:
> > > +static void __exit typec_exit(void)
> > > +{
> > > + class_unregister(_class);
> > 
> > You forgot to clean up your idr :(
> 
> Sorry, what idr? The port ids get removed in typec_release().

You have a static idr structure in the driver, right?  You have to clean
it up when your code is going away so that it will free any memory it
had allocated with a call to idr_destroy() on module exit.

thanks,

greg k-h

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
> > 
> > Reviewed-by: Guenter Roeck 
> > Tested-by: Guenter Roeck 
> > Signed-off-by: Heikki Krogerus 
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec |  218 ++
> >  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   | 1075 
> > +++
> >  include/linux/usb/typec.h   |  252 +++
> >  9 files changed, 1669 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
> 
> Overall, this looks good, just a few minor comments that will require
> you to respin this...
> 
> 
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec 
> > b/Documentation/ABI/testing/sysfs-class-typec
> > new file mode 100644
> > index 000..dcca6bd
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -0,0 +1,218 @@
> > +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> > +
> > +What:  /sys/class/typec//current_data_role
> > +Date:  June 2016
> 
> It's no longer June.  I don't know why we have these dates, sorry, just
> make it be December and you should be fine.

OK

> > --- /dev/null
> > +++ b/Documentation/usb/typec.txt
> 
> We want to use .rst formats now, but this should be fine as-is for now.
> 
> > +static int sysfs_strmatch(const char * const *array, size_t n, const char 
> > *str)
> > +{
> > +   const char *item;
> > +   int index;
> > +
> > +   for (index = 0; index < n; index++) {
> > +   item = array[index];
> > +   if (!item)
> > +   break;
> > +   if (sysfs_streq(item, str))
> > +   return index;
> > +   }
> > +
> > +   return -EINVAL;
> > +}
> 
> should we make this a core sysfs function?

I already have the patch. I was planning on proposing that separately
after this series. This turned out to be something that can be used in
many drivers, so I was thinking about proposing it for at least few
other drivers.

But if you prefer, I can also introduce the function alone as new
sysfs core function in this series, and just use it in this driver.

> > +
> > +/* 
> > - */
> > +/* Type-C Partners */
> > +
> > +static void typec_dev_release(struct device *dev)
> > +{
> > +}
> 
> Yeah, thanks to the in-kernel documentation, I now get to make fun of
> you
> 
> Please, NEVER DO THIS EVER!!!  You are trying to tell the kernel "hey,
> shut up for your stupid warning about an empty release function!"  Did
> you ever think about _why_ I made the kernel warn about that?  Don't
> think you are smarter than the kernel, you will always loose that bet...

Point taken.

> > +
> > +static ssize_t partner_usb_pd_show(struct device *dev,
> > +  struct device_attribute *attr, char *buf)
> > +{
> > +   struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> > +
> > +   return sprintf(buf, "%d\n", p->usb_pd);
> > +}
> > +
> > +static struct device_attribute dev_attr_partner_usb_pd = {
> > +   .attr = {
> > +   .name = "supports_usb_power_delivery",
> > +   .mode = S_IRUGO,
> > +   },
> > +   .show = partner_usb_pd_show,
> > +};
> 
> DEVICE_ATTR_RO()?

I'm using the same attribute names with different types of devises. It
felt more wrong to use shared functions for some of the attributes
(but not all), and try to identify the device type in them, then to
simply ignore the macros and name the functions how ever I wanted. At
least it made the driver look less messy for sure.

But I'll try change these.

> > +
> > +static ssize_t
> > +partner_accessory_mode_show(struct device *dev, struct device_attribute 
> > *attr,
> > +   char *buf)
> > +{
> > +   struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> > +
> > +   return sprintf(buf, "%s\n", 

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Heikki Krogerus]

Hi Greg,

On Mon, Nov 14, 2016 at 10:51:48AM +0100, Greg KH wrote:
> On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
> > 
> > Reviewed-by: Guenter Roeck 
> > Tested-by: Guenter Roeck 
> > Signed-off-by: Heikki Krogerus 
> > ---
> >  Documentation/ABI/testing/sysfs-class-typec |  218 ++
> >  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   | 1075 
> > +++
> >  include/linux/usb/typec.h   |  252 +++
> >  9 files changed, 1669 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
> 
> Overall, this looks good, just a few minor comments that will require
> you to respin this...
> 
> 
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-class-typec 
> > b/Documentation/ABI/testing/sysfs-class-typec
> > new file mode 100644
> > index 000..dcca6bd
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-class-typec
> > @@ -0,0 +1,218 @@
> > +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> > +
> > +What:  /sys/class/typec//current_data_role
> > +Date:  June 2016
> 
> It's no longer June.  I don't know why we have these dates, sorry, just
> make it be December and you should be fine.

OK

> > --- /dev/null
> > +++ b/Documentation/usb/typec.txt
> 
> We want to use .rst formats now, but this should be fine as-is for now.
> 
> > +static int sysfs_strmatch(const char * const *array, size_t n, const char 
> > *str)
> > +{
> > +   const char *item;
> > +   int index;
> > +
> > +   for (index = 0; index < n; index++) {
> > +   item = array[index];
> > +   if (!item)
> > +   break;
> > +   if (sysfs_streq(item, str))
> > +   return index;
> > +   }
> > +
> > +   return -EINVAL;
> > +}
> 
> should we make this a core sysfs function?

I already have the patch. I was planning on proposing that separately
after this series. This turned out to be something that can be used in
many drivers, so I was thinking about proposing it for at least few
other drivers.

But if you prefer, I can also introduce the function alone as new
sysfs core function in this series, and just use it in this driver.

> > +
> > +/* 
> > - */
> > +/* Type-C Partners */
> > +
> > +static void typec_dev_release(struct device *dev)
> > +{
> > +}
> 
> Yeah, thanks to the in-kernel documentation, I now get to make fun of
> you
> 
> Please, NEVER DO THIS EVER!!!  You are trying to tell the kernel "hey,
> shut up for your stupid warning about an empty release function!"  Did
> you ever think about _why_ I made the kernel warn about that?  Don't
> think you are smarter than the kernel, you will always loose that bet...

Point taken.

> > +
> > +static ssize_t partner_usb_pd_show(struct device *dev,
> > +  struct device_attribute *attr, char *buf)
> > +{
> > +   struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> > +
> > +   return sprintf(buf, "%d\n", p->usb_pd);
> > +}
> > +
> > +static struct device_attribute dev_attr_partner_usb_pd = {
> > +   .attr = {
> > +   .name = "supports_usb_power_delivery",
> > +   .mode = S_IRUGO,
> > +   },
> > +   .show = partner_usb_pd_show,
> > +};
> 
> DEVICE_ATTR_RO()?

I'm using the same attribute names with different types of devises. It
felt more wrong to use shared functions for some of the attributes
(but not all), and try to identify the device type in them, then to
simply ignore the macros and name the functions how ever I wanted. At
least it made the driver look less messy for sure.

But I'll try change these.

> > +
> > +static ssize_t
> > +partner_accessory_mode_show(struct device *dev, struct device_attribute 
> > *attr,
> > +   char *buf)
> > +{
> > +   struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> > +
> > +   return sprintf(buf, "%s\n", typec_accessory_modes[p->accessory]);
> > +}
> > +
> > +static struct device_attribute 

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
> 
> Reviewed-by: Guenter Roeck 
> Tested-by: Guenter Roeck 
> Signed-off-by: Heikki Krogerus 
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  218 ++
>  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   | 1075 
> +++
>  include/linux/usb/typec.h   |  252 +++
>  9 files changed, 1669 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

Overall, this looks good, just a few minor comments that will require
you to respin this...


> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec 
> b/Documentation/ABI/testing/sysfs-class-typec
> new file mode 100644
> index 000..dcca6bd
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -0,0 +1,218 @@
> +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> +
> +What:/sys/class/typec//current_data_role
> +Date:June 2016

It's no longer June.  I don't know why we have these dates, sorry, just
make it be December and you should be fine.

> --- /dev/null
> +++ b/Documentation/usb/typec.txt

We want to use .rst formats now, but this should be fine as-is for now.

> +static int sysfs_strmatch(const char * const *array, size_t n, const char 
> *str)
> +{
> + const char *item;
> + int index;
> +
> + for (index = 0; index < n; index++) {
> + item = array[index];
> + if (!item)
> + break;
> + if (sysfs_streq(item, str))
> + return index;
> + }
> +
> + return -EINVAL;
> +}

should we make this a core sysfs function?

> +
> +/* - 
> */
> +/* Type-C Partners */
> +
> +static void typec_dev_release(struct device *dev)
> +{
> +}

Yeah, thanks to the in-kernel documentation, I now get to make fun of
you

Please, NEVER DO THIS EVER!!!  You are trying to tell the kernel "hey,
shut up for your stupid warning about an empty release function!"  Did
you ever think about _why_ I made the kernel warn about that?  Don't
think you are smarter than the kernel, you will always loose that bet...

> +
> +static ssize_t partner_usb_pd_show(struct device *dev,
> +struct device_attribute *attr, char *buf)
> +{
> + struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> +
> + return sprintf(buf, "%d\n", p->usb_pd);
> +}
> +
> +static struct device_attribute dev_attr_partner_usb_pd = {
> + .attr = {
> + .name = "supports_usb_power_delivery",
> + .mode = S_IRUGO,
> + },
> + .show = partner_usb_pd_show,
> +};

DEVICE_ATTR_RO()?

> +
> +static ssize_t
> +partner_accessory_mode_show(struct device *dev, struct device_attribute 
> *attr,
> + char *buf)
> +{
> + struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> +
> + return sprintf(buf, "%s\n", typec_accessory_modes[p->accessory]);
> +}
> +
> +static struct device_attribute dev_attr_partner_accessory = {
> + .attr = {
> + .name = "accessory_mode",
> + .mode = S_IRUGO,
> + },
> + .show = partner_accessory_mode_show,
> +};

DEVICE_ATTR_RO()?

> +
> +static struct attribute *typec_partner_attrs[] = {
> + _attr_partner_accessory.attr,
> + _attr_partner_usb_pd.attr,
> + NULL
> +};
> +
> +static struct attribute_group typec_partner_group = {
> + .attrs = typec_partner_attrs,
> +};
> +
> +static const struct attribute_group *typec_partner_groups[] = {
> + _partner_group,
> + NULL
> +};

ATTRIBUTE_GROUPS()?


> +
> +static struct device_type typec_partner_dev_type = {
> + .name = "typec_partner_device",
> + .groups = typec_partner_groups,
> + .release = typec_dev_release,
> +};
> +
> +static int
> +typec_add_partner(struct typec_port *port, struct typec_partner *partner)
> +{
> + struct device *dev = >dev;
> +   

Re: [PATHCv10 1/2] usb: USB Type-C connector class

[Greg KH]

On Mon, Sep 19, 2016 at 02:16:56PM +0300, 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.
> 
> Reviewed-by: Guenter Roeck 
> Tested-by: Guenter Roeck 
> Signed-off-by: Heikki Krogerus 
> ---
>  Documentation/ABI/testing/sysfs-class-typec |  218 ++
>  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   | 1075 
> +++
>  include/linux/usb/typec.h   |  252 +++
>  9 files changed, 1669 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

Overall, this looks good, just a few minor comments that will require
you to respin this...


> 
> diff --git a/Documentation/ABI/testing/sysfs-class-typec 
> b/Documentation/ABI/testing/sysfs-class-typec
> new file mode 100644
> index 000..dcca6bd
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-typec
> @@ -0,0 +1,218 @@
> +USB Type-C port devices (eg. /sys/class/typec/usbc0/)
> +
> +What:/sys/class/typec//current_data_role
> +Date:June 2016

It's no longer June.  I don't know why we have these dates, sorry, just
make it be December and you should be fine.

> --- /dev/null
> +++ b/Documentation/usb/typec.txt

We want to use .rst formats now, but this should be fine as-is for now.

> +static int sysfs_strmatch(const char * const *array, size_t n, const char 
> *str)
> +{
> + const char *item;
> + int index;
> +
> + for (index = 0; index < n; index++) {
> + item = array[index];
> + if (!item)
> + break;
> + if (sysfs_streq(item, str))
> + return index;
> + }
> +
> + return -EINVAL;
> +}

should we make this a core sysfs function?

> +
> +/* - 
> */
> +/* Type-C Partners */
> +
> +static void typec_dev_release(struct device *dev)
> +{
> +}

Yeah, thanks to the in-kernel documentation, I now get to make fun of
you

Please, NEVER DO THIS EVER!!!  You are trying to tell the kernel "hey,
shut up for your stupid warning about an empty release function!"  Did
you ever think about _why_ I made the kernel warn about that?  Don't
think you are smarter than the kernel, you will always loose that bet...

> +
> +static ssize_t partner_usb_pd_show(struct device *dev,
> +struct device_attribute *attr, char *buf)
> +{
> + struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> +
> + return sprintf(buf, "%d\n", p->usb_pd);
> +}
> +
> +static struct device_attribute dev_attr_partner_usb_pd = {
> + .attr = {
> + .name = "supports_usb_power_delivery",
> + .mode = S_IRUGO,
> + },
> + .show = partner_usb_pd_show,
> +};

DEVICE_ATTR_RO()?

> +
> +static ssize_t
> +partner_accessory_mode_show(struct device *dev, struct device_attribute 
> *attr,
> + char *buf)
> +{
> + struct typec_partner *p = container_of(dev, struct typec_partner, dev);
> +
> + return sprintf(buf, "%s\n", typec_accessory_modes[p->accessory]);
> +}
> +
> +static struct device_attribute dev_attr_partner_accessory = {
> + .attr = {
> + .name = "accessory_mode",
> + .mode = S_IRUGO,
> + },
> + .show = partner_accessory_mode_show,
> +};

DEVICE_ATTR_RO()?

> +
> +static struct attribute *typec_partner_attrs[] = {
> + _attr_partner_accessory.attr,
> + _attr_partner_usb_pd.attr,
> + NULL
> +};
> +
> +static struct attribute_group typec_partner_group = {
> + .attrs = typec_partner_attrs,
> +};
> +
> +static const struct attribute_group *typec_partner_groups[] = {
> + _partner_group,
> + NULL
> +};

ATTRIBUTE_GROUPS()?


> +
> +static struct device_type typec_partner_dev_type = {
> + .name = "typec_partner_device",
> + .groups = typec_partner_groups,
> + .release = typec_dev_release,
> +};
> +
> +static int
> +typec_add_partner(struct typec_port *port, struct typec_partner *partner)
> +{
> + struct device *dev = >dev;
> + int ret;
> +
> + dev->class = _class;
> + dev->parent = >dev;

[PATHCv10 1/2] usb: USB Type-C connector class

[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.

Reviewed-by: Guenter Roeck 
Tested-by: Guenter Roeck 
Signed-off-by: Heikki Krogerus 
---
 Documentation/ABI/testing/sysfs-class-typec |  218 ++
 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   | 1075 +++
 include/linux/usb/typec.h   |  252 +++
 9 files changed, 1669 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..dcca6bd
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -0,0 +1,218 @@
+USB Type-C port devices (eg. /sys/class/typec/usbc0/)
+
+What:  /sys/class/typec//current_data_role
+Date:  June 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.
+
+   Valid values:
+   - host
+   - device
+
+What:  /sys/class/typec//current_power_role
+Date:  June 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.
+
+   Valid values:
+   - source
+   - sink
+
+What:  /sys/class/typec//vconn_source
+Date:  June 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.
+
+   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:  June 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:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   The user space can notify the driver about the preferred role.
+   It should be handled as enabling of Try.SRC or Try.SNK, as
+ 

[PATHCv10 1/2] usb: USB Type-C connector class

[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.

Reviewed-by: Guenter Roeck 
Tested-by: Guenter Roeck 
Signed-off-by: Heikki Krogerus 
---
 Documentation/ABI/testing/sysfs-class-typec |  218 ++
 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   | 1075 +++
 include/linux/usb/typec.h   |  252 +++
 9 files changed, 1669 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..dcca6bd
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-class-typec
@@ -0,0 +1,218 @@
+USB Type-C port devices (eg. /sys/class/typec/usbc0/)
+
+What:  /sys/class/typec//current_data_role
+Date:  June 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.
+
+   Valid values:
+   - host
+   - device
+
+What:  /sys/class/typec//current_power_role
+Date:  June 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.
+
+   Valid values:
+   - source
+   - sink
+
+What:  /sys/class/typec//vconn_source
+Date:  June 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.
+
+   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:  June 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:  June 2016
+Contact:   Heikki Krogerus 
+Description:
+   The user space can notify the driver about the preferred role.
+   It should be handled as enabling of Try.SRC or Try.SNK, as
+   defined in USB Type-C specification, in the port drivers. By
+   default there is no preferred role.
+
+   Valid values:
+   - source
+   - sink
+   - none - to remove