Re: uas failing on multiple disk access on a jmicron JMS567 bridge

2017-05-29 Thread Christoph Gohle 

> On 24 May 2017, at 21:40, Christoph Gohle  wrote:
> 
>> 
>>> On 22 May 2017, at 14:37, Mathias Nyman  
>>> wrote:
>>> 
>>> On 22.05.2017 11:48, Christoph Gohle wrote:
 Hey Mathias,
 
 
> On 19 May 2017, at 09:58, Mathias Nyman  
> wrote:
> 
>> 
> 
> 4.11 kernel has xhci traces enabled, could you try to reproduce it with 
> 4.11?
> xhci traces can be enabled with:
> 
> echo 1 > /sys/kernel/debug/tracing/events/xhci-hcd/enable
> 
> If you know how to reliably reproduce this then please enable tracing 
> just before
> triggering this. It generates a lot of data.
> 
> -Mathias
 I would be able to do xhci traces with a 4.10 kernel (4.10.0-21-generic 
 #23~16.04.1-Ubuntu SMP). Don’t really want to go to 4.11. . Still 
 interested?
>>> 
>>> xhci got more useful tracing support in 4.11,
>>> Sure, you can try if there's something in 4.10
> OK. Now I managed (had to update-initramfs, stupid…)
> 
… I decided to give up on this. It seems like neither usb-storage nor uas is 
really a reliable driver for this device. Even though usb-storage did not show 
the above errors in the log, in the end I found that storage access was not 
reliable.
hotplugging or unplugging a disk from the device did not work as expected: all 
disks were initialized again and recieved new device nodes. smartd got confused 
for some reason (disks changing their identification?).

Now i am using the eSATA interface provided by the device instead. That seems 
to work OK for a couple of days now and has not expressed any of the above 
symptoms.

Anyway, thanks for your help.
Christoph
--
Christoph Gohle
christ...@gohle.org








signature.asc
Description: Message signed with OpenPGP using GPGMail

Re: [PATCH v1] usb: host: xhci: reliable endpoint reset after halt

2017-05-29 Thread Gavin Li 
You are correct; it seems like this issue arises from the fact that
stalled no-op trbs are ignored and the controller remains stuck in the
Halted state because the Reset Endpoint command never gets run.

>From what I understand, this issue is not caused by losing track of
where the endpoint ring stopped, but by a race condition between
killing URBs and stall errors. As such, the STALL_ERROR handling
happens right after the endpoint ring is stopped. Since the trb is
no-op'ed in xhci_handle_cmd_stop_ep(), the endpoint recovery code
never gets run.

The cdc-acm driver triggers the "perpetually halted endpoint" behavior
via (what I believe are) the following operations:
1. The cdc-acm driver submits and queues 16 bulk IN URBs.
2. A full-speed device times out and fails to respond to its IN token
(including retries), causing the CSPLIT transaction with its upstream
hub to return a STALL. The first URB thus completes with -EPIPE.
3. Upon completion of the first URB, the cdc-acm driver kills the
other 15 URBs, attempts to clear the halt condition, and resubmits all
16 URBs.
4. Every time a URB is killed, the endpoint ring is restarted and URBs
that are not killed yet are executed. If we are unlucky, the URB's TRB
is gets cancelled right after it has been executed but before
xhci_irq() gets to run, causing this condition.

My patch is simply a workaround for this specific case; it may be
worth considering if there are any other tasks that need to be done in
the no-op trb case.

Best,
Gavin

On Mon, May 29, 2017 at 7:06 AM, Mathias Nyman
 wrote:
> On 26.05.2017 22:12, gavi...@thegavinli.com wrote:
>>
>> From: Gavin Li 
>>
>> If a stalling TRB is cancelled and NOOP'ed in xhci_handle_cmd_stop_ep(),
>> finish_td() never gets called to reset the halted endpoint and the
>> endpoint remains indefinitely stalled. This patch ensures that
>> xhci_cleanup_halted_endpoint() is called after a TRB completion if the
>> endpoint is halted, irregardless of the status of any pending TRBs.
>>
>> Signed-off-by: Gavin Li 
>
>
> Interesting, I'm trying to understand the details of what's going on here.
>
> Does the event ring first have a stopped transfer event, then a stop
> endpoint
> command completion event, and finally a transfer event with STALL_ERROR
> completion
> code pointing to a no-op trb?
>
> If this is the case do yo have any idea if the STALL_ERROR transfer event
> was issued while the ring was stopping, or only after ring was restarted
> again?
>
> current code relies on the the stopped transfer event to know where on the
> endpoint ring hardware stopped, sometimes this might not be reliable.
>
> I have a change for that, just pushed to my for-usb-next branch in my tree
> at:
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git
>
> Your patch basically adds a check for STALL_ERROR for no-op trbs, it reveals
> that the driver currently skips handling all endpoint state related transfer
> events if that TRB was canceled. This needs to investigated more
>
> Thanks
> -Mathias
>
--
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: [PATCH v4 3/3] usb: gadget: add f_uac1 variant based on a new u_audio api

2017-05-29 Thread Ruslan Bilovol 
On Fri, May 26, 2017 at 6:52 PM, Julian Scheel  wrote:
> On 18.05.2017 00:37, Ruslan Bilovol wrote:
>>
>> This patch adds a new function 'f_uac1_acard'
>> (f_uac1 with virtual "ALSA card") that
>> uses recently created u_audio API. Comparing
>> to legacy f_uac1 function implementation it
>> doesn't require any real Audio codec to be
>> present on the device. In f_uac1_acard audio
>> streams are simply sinked to and sourced
>> from a virtual ALSA sound card created
>> using u_audio API.
>>
>> Legacy f_uac1 approach is to write audio
>> samples directly to existing ALSA sound
>> card
>>
>> f_uac1_acard approach is more generic/flexible
>> one - create an ALSA sound card that
>> represents USB Audio function and allows to
>> be used by userspace application that
>> may choose to do whatever it wants with the
>> data received from the USB Host and choose
>> to provide whatever it wants as audio data
>> to the USB Host.
>>
>> f_uac1_acard also has capture support (gadget->host)
>> thanks to easy implementation via u_audio.
>> By default, capture interface has 48000kHz/2ch
>> configuration, same as playback channel has.
>>
>> f_uac1_acard descriptors naming convention
>> uses f_uac2 driver naming convention that
>> makes it more common and meaningful.
>>
>> Comparing to f_uac1, the f_uac1_acard doesn't
>> have volume/mute functionality. This is because
>> the f_uac1 volume/mute feature unit was dummy
>> implementation since that driver creation (2009)
>> and never had any real volume control or mute
>> functionality, so there is no any difference
>> here.
>>
>> Since f_uac1_acard functionality, exposed
>> interface to userspace (virtual ALSA card),
>> input parameters are so different comparing
>> to legace f_uac1, that there is no any
>> reason to keep them in the same file/module,
>> and separate function was created.
>>
>> g_audio can be built using one of existing
>> UAC functions (f_uac1, f_uac1_acard or f_uac2)
>>
>> Signed-off-by: Ruslan Bilovol 
>> ---
>>   .../ABI/testing/configfs-usb-gadget-uac1_acard |  14 +
>>   Documentation/usb/gadget-testing.txt   |  45 ++
>>   drivers/usb/gadget/Kconfig |  21 +
>>   drivers/usb/gadget/function/Makefile   |   2 +
>>   drivers/usb/gadget/function/f_uac1_acard.c | 803
>> +
>>   drivers/usb/gadget/function/u_uac1_acard.h |  41 ++
>>   drivers/usb/gadget/legacy/Kconfig  |  15 +-
>>   drivers/usb/gadget/legacy/audio.c  |  53 ++
>>   8 files changed, 992 insertions(+), 2 deletions(-)
>>   create mode 100644
>> Documentation/ABI/testing/configfs-usb-gadget-uac1_acard
>>   create mode 100644 drivers/usb/gadget/function/f_uac1_acard.c
>>   create mode 100644 drivers/usb/gadget/function/u_uac1_acard.h
>>
>
> Tested on iMX7D using chipidea usb gadget controller. Tested Windows 10 and
> Linux 4.11 as host. Both work fine.
>
> Tested-by: Julian Scheel 

Thanks for testing it.

I'll wait for additional reviews/comments next few days and than will
send new patchset with addressed comments.

Also current patchset have minor conflict when applied on top of
Felipe's testing/next branch which is moved to 4.12-rc1. I've
resolved conflict but did only build test so far.
Anyway I uploaded rebased patches to github [1] if anybody
wants to try them.

[1] https://github.com/rbilovol/kernel/commits/usbaudio-v4.12-balbi-testing-next

Best regards,
Ruslan
--
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: [PATCH v4 2/3] usb: gadget: f_uac2: split out audio core

2017-05-29 Thread Ruslan Bilovol 
On Mon, May 22, 2017 at 6:58 PM, Jassi Brar  wrote:
> On Thu, May 18, 2017 at 4:07 AM, Ruslan Bilovol
>  wrote:
>> Abstract the peripheral side ALSA sound card code from
>> the f_uac2 function into a component that can be called
>> by various functions, so the various flavors can be split
>> apart and selectively reused.
>>
>> Visible changes:
>>  - add uac_params structure to pass audio paramteres for
>>g_audio_setup
>>  - make ALSA sound card's name configurable
>>  - add [in/out]_ep_maxpsize
>>  - allocate snd_uac_chip structure during g_audio_setup
>>  - add u_audio_[start/stop]_[capture/playback] functions
>>
>> Signed-off-by: Ruslan Bilovol 
>> ---
>>  drivers/usb/gadget/Kconfig|   4 +
>>  drivers/usb/gadget/function/Makefile  |   1 +
>>  drivers/usb/gadget/function/f_uac2.c  | 721 
>> --
>>  drivers/usb/gadget/function/u_audio.c | 661 +++
>>  drivers/usb/gadget/function/u_audio.h |  95 +
>>  drivers/usb/gadget/legacy/Kconfig |   1 +
>>  6 files changed, 846 insertions(+), 637 deletions(-)
>>  create mode 100644 drivers/usb/gadget/function/u_audio.c
>>  create mode 100644 drivers/usb/gadget/function/u_audio.h
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index c164d6b..2ba0ace 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -158,6 +158,9 @@ config USB_U_SERIAL
>>  config USB_U_ETHER
>> tristate
>>
>> +config USB_U_AUDIO
>> +   tristate
>> +
>>  config USB_F_SERIAL
>> tristate
>>
>> @@ -381,6 +384,7 @@ config USB_CONFIGFS_F_UAC2
>> depends on SND
>> select USB_LIBCOMPOSITE
>> select SND_PCM
>> +   select USB_U_AUDIO
>> select USB_F_UAC2
>> help
>>   This Audio function is compatible with USB Audio Class
>> diff --git a/drivers/usb/gadget/function/Makefile 
>> b/drivers/usb/gadget/function/Makefile
>> index cb8c225..b29f2ae 100644
>> --- a/drivers/usb/gadget/function/Makefile
>> +++ b/drivers/usb/gadget/function/Makefile
>> @@ -32,6 +32,7 @@ usb_f_mass_storage-y  := f_mass_storage.o 
>> storage_common.o
>>  obj-$(CONFIG_USB_F_MASS_STORAGE)+= usb_f_mass_storage.o
>>  usb_f_fs-y := f_fs.o
>>  obj-$(CONFIG_USB_F_FS) += usb_f_fs.o
>> +obj-$(CONFIG_USB_U_AUDIO)  += u_audio.o
>>  usb_f_uac1-y   := f_uac1.o u_uac1.o
>>  obj-$(CONFIG_USB_F_UAC1)   += usb_f_uac1.o
>>  usb_f_uac2-y   := f_uac2.o
>> diff --git a/drivers/usb/gadget/function/f_uac2.c 
>> b/drivers/usb/gadget/function/f_uac2.c
>> index d4565b5..059a14a 100644
>> --- a/drivers/usb/gadget/function/f_uac2.c
>> +++ b/drivers/usb/gadget/function/f_uac2.c
>> @@ -15,10 +15,7 @@
>>  #include 
>>  #include 
>>
>> -#include 
>> -#include 
>> -#include 
>> -
>> +#include "u_audio.h"
>>  #include "u_uac2.h"
>>
>>  /*
>> @@ -50,455 +47,23 @@
>>  #define UNFLW_CTRL 8
>>  #define OVFLW_CTRL 10
>>
>> -struct uac2_req {
>> -   struct uac2_rtd_params *pp; /* parent param */
>> -   struct usb_request *req;
>> -};
>> -
>> -struct uac2_rtd_params {
>> -   struct snd_uac2_chip *uac2; /* parent chip */
>> -   bool ep_enabled; /* if the ep is enabled */
>> -   /* Size of the ring buffer */
>> -   size_t dma_bytes;
>> -   unsigned char *dma_area;
>> -
>> -   struct snd_pcm_substream *ss;
>> -
>> -   /* Ring buffer */
>> -   ssize_t hw_ptr;
>> -
>> -   void *rbuf;
>> -
>> -   size_t period_size;
>> -
>> -   unsigned max_psize;
>> -   struct uac2_req *ureq;
>> -
>> -   spinlock_t lock;
>> -};
>> -
>> -struct snd_uac2_chip {
>> -   struct uac2_rtd_params p_prm;
>> -   struct uac2_rtd_params c_prm;
>> -
>> -   struct snd_card *card;
>> -   struct snd_pcm *pcm;
>> -
>> -   /* timekeeping for the playback endpoint */
>> -   unsigned int p_interval;
>> -   unsigned int p_residue;
>> -
>> -   /* pre-calculated values for playback iso completion */
>> -   unsigned int p_pktsize;
>> -   unsigned int p_pktsize_residue;
>> -   unsigned int p_framesize;
>> +struct f_uac2 {
>> +   struct g_audio g_audio;
>> +   u8 ac_intf, as_in_intf, as_out_intf;
>> +   u8 ac_alt, as_in_alt, as_out_alt;   /* needed for get_alt() */
>>  };
>>
>> -#define BUFF_SIZE_MAX  (PAGE_SIZE * 16)
>> -#define PRD_SIZE_MAX   PAGE_SIZE
>> -#define MIN_PERIODS4
>> -
>> -static struct snd_pcm_hardware uac2_pcm_hardware = {
>> -   .info = SNDRV_PCM_INFO_INTERLEAVED | SNDRV_PCM_INFO_BLOCK_TRANSFER
>> -| SNDRV_PCM_INFO_MMAP | SNDRV_PCM_INFO_MMAP_VALID
>> -| SNDRV_PCM_INFO_PAUSE | SNDRV_PCM_INFO_RESUME,
>> -   .rates = SNDRV_PCM_RATE_CONTINUOUS,
>> -   .periods_max = BUFF_SIZE_MAX / PRD_SIZE_MAX,
>> -   .buffer_bytes_max = BUFF_SIZE_MAX,
>> -   .period_bytes_max =

Re: [PATCH V3 1/2] dt-bindings: leds: document new trigger-sources property

2017-05-29 Thread Rafał Miłecki 
On 29 May 2017 at 21:52, Jacek Anaszewski  wrote:
> On 05/29/2017 04:01 PM, Rafał Miłecki wrote:
>> From: Rafał Miłecki 
>>
>> Some LEDs can be related to a specific device(s) described in the DT.
>> This property allows specifying such relations. E.g. USB LED should
>> usually be used to indicate some USB port(s) state.
>>
>> Please note this binding is designed to be generic and not influenced by
>> any operating system design. Linux developers may find "trigger" part a
>> bit confusing since in Linux triggers are separated drivers. It
>> shouldn't define the binding though (we shouldn't add an extra level of
>> indirection).
>>
>> Signed-off-by: Rafał Miłecki 
>> ---
>> V2: Replace "usb-ports" with "led-triggers" property which is more generic 
>> and
>> allows specifying other devices as well.
>> V3: Use "trigger-sources" which is even more accurate as devices aren't
>> precisely triggers.
>> ---
>>  Documentation/devicetree/bindings/leds/common.txt | 18 ++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/leds/common.txt 
>> b/Documentation/devicetree/bindings/leds/common.txt
>> index 24b656014089..e6e300975a4c 100644
>> --- a/Documentation/devicetree/bindings/leds/common.txt
>> +++ b/Documentation/devicetree/bindings/leds/common.txt
>> @@ -49,6 +49,19 @@ Optional properties for child nodes:
>>  - panic-indicator : This property specifies that the LED should be used,
>>   if at all possible, as a panic indicator.
>>
>> +- trigger-sources : List of devices which should be used as a source 
>> triggering
>> + this LED activity. Some LEDs can be related to a specific
>> + device and should somehow indicate its state. E.g. USB 2.0
>> + LED may react to device(s) in a USB 2.0 port(s).
>> + Another common example is switch or router with multiple
>> + Ethernet ports each of them having its own LED assigned
>> + (assuming they are not hardwired). In such cases this
>> + property should contain phandle(s) of related source
>> + device(s).
>> + In many cases LED can be related to more than one device
>> + (e.g. one USB LED vs. multiple USB ports) so a list of
>> + sources can be specified.
>> +
>>  Required properties for flash LED child nodes:
>>  - flash-max-microamp : Maximum flash LED supply current in microamperes.
>>  - flash-max-timeout-us : Maximum timeout in microseconds after which the 
>> flash
>> @@ -69,6 +82,11 @@ gpio-leds {
>>   linux,default-trigger = "heartbeat";
>>   gpios = < 0 GPIO_ACTIVE_HIGH>;
>>   };
>> +
>> + usb {
>> + gpios = < 1 GPIO_ACTIVE_HIGH>;
>> + led-triggers = <_port1>, <_port1>;
>
> Didn't you mean "trigger-sources" here instead?

Oh, thanks for catching that.

-- 
Rafał
--
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: [PATCH v5] usb: typec: Add a sysfs node to manage port type

2017-05-29 Thread Badhri Jagan Sridharan 
Thanks for all the reviews Guenter & Heikki.
Heikki, Is there anything else that I need to address in this patch ?
Or is the patch ready to be merged ?

Thanks,
Badhri

On Mon, May 29, 2017 at 12:56 PM, Badhri Jagan Sridharan
 wrote:
> User space applications in some cases have the need to enforce a
> specific port type(DFP/UFP/DRP). This change allows userspace to
> attempt setting the desired port type. Low level drivers can
> however reject the request if the specific port type is not supported.
>
> Signed-off-by: Badhri Jagan Sridharan 
> Reviewed-by: Guenter Roeck 
> ---
> Changelog since v1:
> - introduced a new variable port_type in struct typec_port to track
>   the current port type instead of changing type member in
>   typec_capability to address Heikki Krogerus comments.
> - changed the output format and strings that would be displayed as
>   suggested by Heikki Krogerus.
>
> Changelog since v2:
> - introduced a new mutex lock to protect port_type for addressing
>   the race conditions identified by Guenter Roeck
> - added typec_port_types_drp for printing port_type when cap->type
>   is TYPE_PORT_DRP as suggested by Guenter Roeck
> - Power role swap and data role swaps would be rejected unless
>   port port_type == TYPE_PORT_DRP
> - port_type_store would return immediately if the current port_type
>   is same as the new port_type as suggested by Guenter Roeck
>
> Changelog since v3:
> - Moved as much as code outside port_type_lock as suggested by
>   Guenter Roeck
> - Removed Change-Id line from commit message identified by
>   Greg Kroah-Hartman
>
> Changelog since v4:
> - Corrected return value and moved moved one more line of code
>   outside port_type_lock in the port_type_store function as
>   suggested by Guenter Roeck
>
>  drivers/usb/typec/typec.c | 106 
> +-
>  include/linux/usb/typec.h |   4 ++
>  2 files changed, 100 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> index 89e540bb7ff3..63644785ce31 100644
> --- a/drivers/usb/typec/typec.c
> +++ b/drivers/usb/typec/typec.c
> @@ -11,6 +11,7 @@
>
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>
> @@ -69,6 +70,8 @@ struct typec_port {
> enum typec_role pwr_role;
> enum typec_role vconn_role;
> enum typec_pwr_opmode   pwr_opmode;
> +   enum typec_port_typeport_type;
> +   struct mutexport_type_lock;
>
> const struct typec_capability   *cap;
>  };
> @@ -789,6 +792,18 @@ static const char * const typec_data_roles[] = {
> [TYPEC_HOST]= "host",
>  };
>
> +static const char * const typec_port_types[] = {
> +   [TYPEC_PORT_DFP] = "source",
> +   [TYPEC_PORT_UFP] = "sink",
> +   [TYPEC_PORT_DRP] = "dual",
> +};
> +
> +static const char * const typec_port_types_drp[] = {
> +   [TYPEC_PORT_DFP] = "dual [source] sink",
> +   [TYPEC_PORT_UFP] = "dual source [sink]",
> +   [TYPEC_PORT_DRP] = "[dual] source sink",
> +};
> +
>  static ssize_t
>  preferred_role_store(struct device *dev, struct device_attribute *attr,
>  const char *buf, size_t size)
> @@ -846,11 +861,6 @@ static ssize_t data_role_store(struct device *dev,
> struct typec_port *port = to_typec_port(dev);
> int ret;
>
> -   if (port->cap->type != TYPEC_PORT_DRP) {
> -   dev_dbg(dev, "data role swap only supported with DRP 
> ports\n");
> -   return -EOPNOTSUPP;
> -   }
> -
> if (!port->cap->dr_set) {
> dev_dbg(dev, "data role swapping not supported\n");
> return -EOPNOTSUPP;
> @@ -860,11 +870,22 @@ static ssize_t data_role_store(struct device *dev,
> if (ret < 0)
> return ret;
>
> +   mutex_lock(>port_type_lock);
> +   if (port->port_type != TYPEC_PORT_DRP) {
> +   dev_dbg(dev, "port type fixed at \"%s\"",
> +typec_port_types[port->port_type]);
> +   ret = -EOPNOTSUPP;
> +   goto unlock_and_ret;
> +   }
> +
> ret = port->cap->dr_set(port->cap, ret);
> if (ret)
> -   return ret;
> +   goto unlock_and_ret;
>
> -   return size;
> +   ret = size;
> +unlock_and_ret:
> +   mutex_unlock(>port_type_lock);
> +   return ret;
>  }
>
>  static ssize_t data_role_show(struct device *dev,
> @@ -885,7 +906,7 @@ static ssize_t power_role_store(struct device *dev,
> const char *buf, size_t size)
>  {
> struct typec_port *port = to_typec_port(dev);
> -   int ret = size;
> +   int ret;
>
> if (!port->cap->pd_revision) {
> dev_dbg(dev, "USB Power Delivery not supported\n");
> @@ -906,11 +927,22 @@ static ssize_t power_role_store(struct device *dev,
> if

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

2017-05-29 Thread Badhri Jagan Sridharan 
User space applications in some cases have the need to enforce a
specific port type(DFP/UFP/DRP). This change allows userspace to
attempt setting the desired port type. Low level drivers can
however reject the request if the specific port type is not supported.

Signed-off-by: Badhri Jagan Sridharan 
Reviewed-by: Guenter Roeck 
---
Changelog since v1:
- introduced a new variable port_type in struct typec_port to track
  the current port type instead of changing type member in
  typec_capability to address Heikki Krogerus comments.
- changed the output format and strings that would be displayed as
  suggested by Heikki Krogerus.

Changelog since v2:
- introduced a new mutex lock to protect port_type for addressing
  the race conditions identified by Guenter Roeck
- added typec_port_types_drp for printing port_type when cap->type
  is TYPE_PORT_DRP as suggested by Guenter Roeck
- Power role swap and data role swaps would be rejected unless
  port port_type == TYPE_PORT_DRP
- port_type_store would return immediately if the current port_type
  is same as the new port_type as suggested by Guenter Roeck

Changelog since v3:
- Moved as much as code outside port_type_lock as suggested by
  Guenter Roeck
- Removed Change-Id line from commit message identified by
  Greg Kroah-Hartman

Changelog since v4:
- Corrected return value and moved moved one more line of code
  outside port_type_lock in the port_type_store function as
  suggested by Guenter Roeck

 drivers/usb/typec/typec.c | 106 +-
 include/linux/usb/typec.h |   4 ++
 2 files changed, 100 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
index 89e540bb7ff3..63644785ce31 100644
--- a/drivers/usb/typec/typec.c
+++ b/drivers/usb/typec/typec.c
@@ -11,6 +11,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -69,6 +70,8 @@ struct typec_port {
enum typec_role pwr_role;
enum typec_role vconn_role;
enum typec_pwr_opmode   pwr_opmode;
+   enum typec_port_typeport_type;
+   struct mutexport_type_lock;
 
const struct typec_capability   *cap;
 };
@@ -789,6 +792,18 @@ static const char * const typec_data_roles[] = {
[TYPEC_HOST]= "host",
 };
 
+static const char * const typec_port_types[] = {
+   [TYPEC_PORT_DFP] = "source",
+   [TYPEC_PORT_UFP] = "sink",
+   [TYPEC_PORT_DRP] = "dual",
+};
+
+static const char * const typec_port_types_drp[] = {
+   [TYPEC_PORT_DFP] = "dual [source] sink",
+   [TYPEC_PORT_UFP] = "dual source [sink]",
+   [TYPEC_PORT_DRP] = "[dual] source sink",
+};
+
 static ssize_t
 preferred_role_store(struct device *dev, struct device_attribute *attr,
 const char *buf, size_t size)
@@ -846,11 +861,6 @@ static ssize_t data_role_store(struct device *dev,
struct typec_port *port = to_typec_port(dev);
int ret;
 
-   if (port->cap->type != TYPEC_PORT_DRP) {
-   dev_dbg(dev, "data role swap only supported with DRP ports\n");
-   return -EOPNOTSUPP;
-   }
-
if (!port->cap->dr_set) {
dev_dbg(dev, "data role swapping not supported\n");
return -EOPNOTSUPP;
@@ -860,11 +870,22 @@ static ssize_t data_role_store(struct device *dev,
if (ret < 0)
return ret;
 
+   mutex_lock(>port_type_lock);
+   if (port->port_type != TYPEC_PORT_DRP) {
+   dev_dbg(dev, "port type fixed at \"%s\"",
+typec_port_types[port->port_type]);
+   ret = -EOPNOTSUPP;
+   goto unlock_and_ret;
+   }
+
ret = port->cap->dr_set(port->cap, ret);
if (ret)
-   return ret;
+   goto unlock_and_ret;
 
-   return size;
+   ret = size;
+unlock_and_ret:
+   mutex_unlock(>port_type_lock);
+   return ret;
 }
 
 static ssize_t data_role_show(struct device *dev,
@@ -885,7 +906,7 @@ static ssize_t power_role_store(struct device *dev,
const char *buf, size_t size)
 {
struct typec_port *port = to_typec_port(dev);
-   int ret = size;
+   int ret;
 
if (!port->cap->pd_revision) {
dev_dbg(dev, "USB Power Delivery not supported\n");
@@ -906,11 +927,22 @@ static ssize_t power_role_store(struct device *dev,
if (ret < 0)
return ret;
 
+   mutex_lock(>port_type_lock);
+   if (port->port_type != TYPEC_PORT_DRP) {
+   dev_dbg(dev, "port type fixed at \"%s\"",
+typec_port_types[port->port_type]);
+   ret = -EOPNOTSUPP;
+   goto unlock_and_ret;
+   }
+
ret = port->cap->pr_set(port->cap, ret);
if (ret)
-   return ret;
+   goto unlock_and_ret;
 
-   return size;
+

Re: [PATCH V3 1/2] dt-bindings: leds: document new trigger-sources property

2017-05-29 Thread Jacek Anaszewski 
Hi Rafał,

On 05/29/2017 04:01 PM, Rafał Miłecki wrote:
> From: Rafał Miłecki 
> 
> Some LEDs can be related to a specific device(s) described in the DT.
> This property allows specifying such relations. E.g. USB LED should
> usually be used to indicate some USB port(s) state.
> 
> Please note this binding is designed to be generic and not influenced by
> any operating system design. Linux developers may find "trigger" part a
> bit confusing since in Linux triggers are separated drivers. It
> shouldn't define the binding though (we shouldn't add an extra level of
> indirection).
> 
> Signed-off-by: Rafał Miłecki 
> ---
> V2: Replace "usb-ports" with "led-triggers" property which is more generic and
> allows specifying other devices as well.
> V3: Use "trigger-sources" which is even more accurate as devices aren't
> precisely triggers.
> ---
>  Documentation/devicetree/bindings/leds/common.txt | 18 ++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.txt 
> b/Documentation/devicetree/bindings/leds/common.txt
> index 24b656014089..e6e300975a4c 100644
> --- a/Documentation/devicetree/bindings/leds/common.txt
> +++ b/Documentation/devicetree/bindings/leds/common.txt
> @@ -49,6 +49,19 @@ Optional properties for child nodes:
>  - panic-indicator : This property specifies that the LED should be used,
>   if at all possible, as a panic indicator.
>  
> +- trigger-sources : List of devices which should be used as a source 
> triggering
> + this LED activity. Some LEDs can be related to a specific
> + device and should somehow indicate its state. E.g. USB 2.0
> + LED may react to device(s) in a USB 2.0 port(s).
> + Another common example is switch or router with multiple
> + Ethernet ports each of them having its own LED assigned
> + (assuming they are not hardwired). In such cases this
> + property should contain phandle(s) of related source
> + device(s).
> + In many cases LED can be related to more than one device
> + (e.g. one USB LED vs. multiple USB ports) so a list of
> + sources can be specified.
> +
>  Required properties for flash LED child nodes:
>  - flash-max-microamp : Maximum flash LED supply current in microamperes.
>  - flash-max-timeout-us : Maximum timeout in microseconds after which the 
> flash
> @@ -69,6 +82,11 @@ gpio-leds {
>   linux,default-trigger = "heartbeat";
>   gpios = < 0 GPIO_ACTIVE_HIGH>;
>   };
> +
> + usb {
> + gpios = < 1 GPIO_ACTIVE_HIGH>;
> + led-triggers = <_port1>, <_port1>;

Didn't you mean "trigger-sources" here instead?

> + };
>  };
>  
>  max77693-led {
> 

-- 
Best regards,
Jacek Anaszewski
--
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: [PATCH 2/3] usb: typec: Add support for UCSI interface

2017-05-29 Thread Guenter Roeck 

On 05/29/2017 05:33 AM, Heikki Krogerus wrote:

On Fri, May 26, 2017 at 06:26:01AM -0700, Guenter Roeck wrote:

What happens if trace is not enabled ? If I recall discussions around CLANG
correctly, it complains about unused static inline functions.


Nothing happens if trace is not enable. Everything continues to
compile just like before.

Maybe you were trying to define the tracepoints in C code? That won't
work.


Anyway, is it really necessary to have an include file for this instead of
keeping the code in trace.c ? I am somewhat wary of variable declarations
in include files - include twice and there will be two instances.


The tracepoint documentation actually states that you need to place the
definitions of the tracepoints in a header. I have the trace.c file only
because it was a convenient place to put the tracepoint statement in.

I don't know the inner workings of tracepoints, but they do work fine
for me. In case you want more info on the topic, I never read these
articles, but they were mentioned in Documentation/trace/tracepoints.txt:

http://lwn.net/Articles/379903
http://lwn.net/Articles/381064
http://lwn.net/Articles/383362

I believe they provide at least some kind of description on how
tracepoints actually work.


Sorry for not being clear; that was a question about your code, not mine.
If it builds without warnings with trace disabled, no problem.


Ah, of course. Silly me. Sorry about that.


+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 


Any preference in usb about alphabetic order of include files ?


I don't see any mention of it in the documentation, so this is
probable just matter of taste in the end. One likes the mother, one
likes the daughter.



I like alphabetic because it makes life easier later on, if/when additional
include files are added, and it is easier to find a specific include file in
an alphabetic list. Some upstream maintainers ask for it nowadays (including me 
:-).


It's good to keep the headers in some order just for the sake of being
organized, but I don't see how alphabetical order is any better then
for example descending.



Now we are really down to bikeshedding, which is where I tend to sign off.


I normally would not care about this topic and I would have just
ordered the headers alphabetically if anybody asked for it, but I've
actually proposed ordering the headers alphabetically myself ones, and
this is what the upstream maintainer said: "Can we stick to serious
critiques"

I think some maintainers do not see the need to dictate styling rules
on everything, and leave some things to be decided by the developer. I
kind of like that.

But if you still feel strongly about this, I'll change the style, but
for now, I'll keep it the way it is. Actually, I'll move the last one
on top.



I am not the maintainer, so, no, I don't have a strong opinion.

Guenter


+/**
+ * ucsi_notify - PPM notification handler
+ * @ucsi: Source UCSI Interface for the notifications
+ *
+ * Handle notifications from PPM of @ucsi.
+ */
+void ucsi_notify(struct ucsi *ucsi)
+{
+   struct ucsi_cci *cci;
+
+   /* There is no requirement to sync here, so only warning if it fails */
+   if (ucsi_sync(ucsi))
+   dev_warn(ucsi->dev, "%s: sync failed\n", __func__);
+

Why even a warning if it is not a requirement ?


It still should not "ever" fail. Why not create the warning? It is
after all indication that something is really wrong, and we probable
want to know about it.


Your call, of course. I just dislike noisy drivers, and I am always concerned
about flooding the kernel log.


Fair enough, I'll remove the warning.


+static int
+ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role role)
+{
+   struct ucsi_connector *con = to_ucsi_connector(cap);
+   struct ucsi_control ctrl;
+   int ret = 0;
+
+   if (!con->partner)
+   return -EAGAIN;


Doesn't this result in an immediate retry by user space ?


Isn't that what tcpm.c reports? Maybe I misunderstood the code.
I'll change that to something else.


Good point; tcpm returns -EAGAIN if the port is not in a READY state.
Maybe I should change the tcpm code ? No idea what error to use instead,
though.


I'll use -ENOTCONN for now. Let me know if it's not good.


+/**
+ * ucsi_register_ppm - Register UCSI PPM Interface
+ * @dev: Device interface to the PPM
+ * @ppm: The PPM interface
+ *
+ * Allocates UCSI instance, associates it with @ppm and returns it to the
+ * caller, and schedules initialization of the interface.
+ */
+struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm *ppm)
+{
+   struct ucsi *ucsi;
+
+   ucsi = kzalloc(sizeof(*ucsi), GFP_KERNEL);
+   if (!ucsi)
+   return ERR_PTR(-ENOMEM);
+
+   INIT_WORK(>work, ucsi_init);
+   init_completion(>complete);
+   mutex_init(>ppm_lock);
+
+   ucsi->dev = dev;
+   ucsi->ppm = ppm;
+
+   queue_work(system_long_wq, >work);

Re: Kernel / USB bug

2017-05-29 Thread Nicolas Repentin 
Sorry Bjorn,

your solution seems good!

I had to regenerate initramfs.

To understand better, is it a real bug on kernel about uas ?


Le 22/05/2017 à 08:00, Bjørn Forsman a écrit :
> On 3 May 2017 at 10:07, Oliver Neukum  wrote:
>> Am Dienstag, den 02.05.2017, 20:06 +0200 schrieb Nicolas Repentin:
>>> Hi
>>>
>>> I got this bug : using last kernel on Archlinux (4.10.13-1, since 4.4),
>>> an USB 3.0 external HDD dies all the time when writing on it.
>> Hi,
>>
>> did 4.4 use uas?
> Yes.
>
> I have a Seagate Expansion Portable 4TB USB 3.0 disk. The 4.4 kernel
> use UAS against this disk and it corrupted the NTFS filesystem that
> the disk came with. I was able to fix the filesystem with Windows but
> the errors happened again. Linux utilities do not have a chkdsk
> equivalent that can operate on NTFS.
>
> An ugly workaround I'm using now is to disable UAS for this disk with
> modprobe "options usb-storage quirks=0bc2:2322:u".
>
> Best regards,
> Bjørn Forsman

--
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: Kernel / USB bug

2017-05-29 Thread Nicolas Repentin 
More info:


Bus 002 Device 003: ID 152d:0578 JMicron Technology Corp. / JMicron USA
Technology Corp.
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   3.00
  bDeviceClass0
  bDeviceSubClass 0
  bDeviceProtocol 0
  bMaxPacketSize0 9
  idVendor   0x152d JMicron Technology Corp. / JMicron USA
Technology Corp.
  idProduct  0x0578
  bcdDevice2.03
  iManufacturer   1 USB to ATA/ATAPI Brid
  iProduct2 USB Device
  iSerial 3 DB9876543211167
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength  121
bNumInterfaces  1
bConfigurationValue 1
iConfiguration  0
bmAttributes 0x80
  (Bus Powered)
MaxPower  224mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass 8 Mass Storage
  bInterfaceSubClass  6 SCSI
  bInterfaceProtocol 80 Bulk-Only
  iInterface  0
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0400  1x 1024 bytes
bInterval   0
bMaxBurst  15
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02  EP 2 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0400  1x 1024 bytes
bInterval   0
bMaxBurst  15
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   1
  bNumEndpoints   4
  bInterfaceClass 8 Mass Storage
  bInterfaceSubClass  6 SCSI
  bInterfaceProtocol 98
  iInterface  0
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01  EP 1 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0400  1x 1024 bytes
bInterval   0
bMaxBurst   0
Command pipe (0x01)
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82  EP 2 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0400  1x 1024 bytes
bInterval   0
bMaxBurst   0
MaxStreams 32
Status pipe (0x02)
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83  EP 3 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0400  1x 1024 bytes
bInterval   0
bMaxBurst  15
MaxStreams 32
Data-in pipe (0x03)
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x04  EP 4 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0400  1x 1024 bytes
bInterval   0
bMaxBurst  15
MaxStreams 32
Data-out pipe (0x04)
Binary Object Store Descriptor:
  bLength 5
  bDescriptorType15
  wTotalLength   22
  bNumDeviceCaps  2
  USB 2.0 Extension Device Capability:
bLength 7
bDescriptorType16
bDevCapabilityType  2
bmAttributes   0x0f0e
  BESL Link Power Management (LPM) Supported
BESL value 3840 us
  SuperSpeed USB Device Capability:
bLength10
bDescriptorType16
bDevCapabilityType  3
bmAttributes 0x00
wSpeedsSupported   0x000e
  Device can operate at Full Speed (12Mbps)
  Device can operate at High Speed (480Mbps)
  Device can operate at SuperSpeed (5Gbps)
bFunctionalitySupport   1

Re: Kernel / USB bug

2017-05-29 Thread Nicolas Repentin 
Hi

I doesn't works for me.

Device is Bus 002 Device 003: ID 152d:0578 JMicron Technology Corp. /
JMicron USA Technology Corp.

Always got this when plugged:

[  211.577287] xhci_hcd :00:14.0: ERROR Transfer event for disabled
endpoint or incorrect stream ring
[  211.577317] xhci_hcd :00:14.0: @c490 
 0400 07088000



Le 22/05/2017 à 08:00, Bjørn Forsman a écrit :
> On 3 May 2017 at 10:07, Oliver Neukum  wrote:
>> Am Dienstag, den 02.05.2017, 20:06 +0200 schrieb Nicolas Repentin:
>>> Hi
>>>
>>> I got this bug : using last kernel on Archlinux (4.10.13-1, since 4.4),
>>> an USB 3.0 external HDD dies all the time when writing on it.
>> Hi,
>>
>> did 4.4 use uas?
> Yes.
>
> I have a Seagate Expansion Portable 4TB USB 3.0 disk. The 4.4 kernel
> use UAS against this disk and it corrupted the NTFS filesystem that
> the disk came with. I was able to fix the filesystem with Windows but
> the errors happened again. Linux utilities do not have a chkdsk
> equivalent that can operate on NTFS.
>
> An ugly workaround I'm using now is to disable UAS for this disk with
> modprobe "options usb-storage quirks=0bc2:2322:u".
>
> Best regards,
> Bjørn Forsman

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

[PATCH 1/2] USB: core: fix device node leak

2017-05-29 Thread Johan Hovold 
Make sure to release any OF device-node reference taken when creating
the USB device.

Note that we currently do not hold a reference to the root hub
device-tree node (i.e. the parent controller node).

Fixes: 69bec7259853 ("USB: core: let USB device know device node")
Cc: stable  # v4.6
Cc: Peter Chen 
Signed-off-by: Johan Hovold 
---
 drivers/usb/core/usb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 28b053cacc90..62e1906bb2f3 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -416,6 +416,8 @@ static void usb_release_dev(struct device *dev)
 
usb_destroy_configuration(udev);
usb_release_bos_descriptor(udev);
+   if (udev->parent)
+   of_node_put(dev->of_node);
usb_put_hcd(hcd);
kfree(udev->product);
kfree(udev->manufacturer);
-- 
2.13.0

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

[PATCH 2/2] USB: of: document reference taken by child-loopkup helper

2017-05-29 Thread Johan Hovold 
Document that the child-node lookup helper takes a reference to the
device-tree node which needs to be dropped after use.

Signed-off-by: Johan Hovold 
---
 drivers/usb/core/of.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
index d563cbcf76cf..17a4af02cf5b 100644
--- a/drivers/usb/core/of.c
+++ b/drivers/usb/core/of.c
@@ -28,6 +28,9 @@
  *
  * Find the node from device tree according to its port number.
  *
+ * Takes a reference to the returned device-tree node, which needs to be
+ * dropped after use.
+ *
  * Return: On success, a pointer to the device node, %NULL on failure.
  */
 struct device_node *usb_of_get_child_node(struct device_node *parent,
-- 
2.13.0

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

[PATCH 0/2] USB: core: fix device-tree node leak

2017-05-29 Thread Johan Hovold 
A reference is taken when looking up a device-tree node when allocating
a USB device, but the reference is never released.

There are some issues related to how we deal with the root hub and its
node which I'll get back to in a follow up series, but this can go in
meanwhile.

Johan


Johan Hovold (2):
  USB: core: fix device node leak
  USB: of: document reference taken by child-loopkup helper

 drivers/usb/core/of.c  | 3 +++
 drivers/usb/core/usb.c | 2 ++
 2 files changed, 5 insertions(+)

-- 
2.13.0

--
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: Device tree nodes for USB devices

2017-05-29 Thread Johan Hovold 
On Mon, May 29, 2017 at 04:15:36PM +0200, Johan Hovold wrote:
> On Mon, May 29, 2017 at 11:01:52AM +0200, Martin Fuzzey wrote:

> > However my larger question is that I don't see how to associate a DT 
> > node with a USB *interface* rather than a USB *device*.
> 
> I started looking into this a while back but got interrupted. I have
> some preliminary code, mostly lacking associated documentation.

> I'll post my work as an RFC shortly, and add you on CC.

Here's a preview.

Johan


>From 5db86d55975ef0e047ab3157d59de0869aa88acf Mon Sep 17 00:00:00 2001
From: Johan Hovold 
Date: Fri, 21 Apr 2017 19:55:59 +0200
Subject: [RFC] USB: of: add support for interface nodes

Add support for USB-interface OF-nodes. Interfaces are children of
USB-device nodes and are identified by a configuration value and an
interface number:

 { /* root hub */
device@1 {  /* device at port 1 */
compatible = ;
reg = <1>;

device@1,0 { /* interface 0 of configuration 1 */
compatible = ;
reg = <1 0>;
};
};
};

FIXME: documentation, mention spec

Not-Signed-off-yet-by: Johan Hovold 
---
 drivers/usb/core/message.c | 14 +-
 drivers/usb/core/of.c  | 30 ++
 include/linux/usb/of.h |  9 +
 3 files changed, 48 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
index 4c38ea41ae96..921b66e34a03 100644
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -18,6 +18,7 @@
 #include 
 #include 
 #include  /* for usbcore internals */
+#include 
 #include 
 
 #include "usb.h"
@@ -1548,6 +1549,7 @@ static void usb_release_interface(struct device *dev)
 
kref_put(>ref, usb_release_interface_cache);
usb_put_dev(interface_to_usbdev(intf));
+   of_node_put(dev->of_node);
kfree(intf);
 }
 
@@ -1833,6 +1835,7 @@ int usb_set_configuration(struct usb_device *dev, int 
configuration)
struct usb_interface_cache *intfc;
struct usb_interface *intf;
struct usb_host_interface *alt;
+   u8 ifnum;
 
cp->interface[i] = intf = new_interfaces[i];
intfc = cp->intf_cache[i];
@@ -1851,11 +1854,13 @@ int usb_set_configuration(struct usb_device *dev, int 
configuration)
if (!alt)
alt = >altsetting[0];
 
-   intf->intf_assoc =
-   find_iad(dev, cp, alt->desc.bInterfaceNumber);
+   ifnum = alt->desc.bInterfaceNumber;
+   intf->intf_assoc = find_iad(dev, cp, ifnum);
intf->cur_altsetting = alt;
usb_enable_interface(dev, intf, true);
intf->dev.parent = >dev;
+   intf->dev.of_node = usb_of_find_interface_node(dev,
+   configuration, ifnum);
intf->dev.driver = NULL;
intf->dev.bus = _bus_type;
intf->dev.type = _if_device_type;
@@ -1870,9 +1875,8 @@ int usb_set_configuration(struct usb_device *dev, int 
configuration)
intf->minor = -1;
device_initialize(>dev);
pm_runtime_no_callbacks(>dev);
-   dev_set_name(>dev, "%d-%s:%d.%d",
-   dev->bus->busnum, dev->devpath,
-   configuration, alt->desc.bInterfaceNumber);
+   dev_set_name(>dev, "%d-%s:%d.%d", dev->bus->busnum,
+   dev->devpath, configuration, ifnum);
usb_get_dev(dev);
}
kfree(new_interfaces);
diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
index 17a4af02cf5b..87e257eb8eeb 100644
--- a/drivers/usb/core/of.c
+++ b/drivers/usb/core/of.c
@@ -51,6 +51,36 @@ struct device_node *usb_of_get_child_node(struct device_node 
*parent,
 EXPORT_SYMBOL_GPL(usb_of_get_child_node);
 
 /**
+ * usb_of_find_interface_node() - find a USB-interface device node
+ * @udev: USB device of interface
+ * @config: configuration value
+ * @ifnum: interface number
+ *
+ * Look up the device node of an interface given its USB device, configuration
+ * value, and interface number.
+ *
+ * Return: A pointer to the node with incremented refcount if found, or
+ * %NULL otherwise.
+ */
+struct device_node *usb_of_find_interface_node(struct usb_device *udev,
+   u8 config, u8 ifnum)
+{
+   struct device_node *node;
+   u32 reg[2];
+
+   for_each_child_of_node(udev->dev.of_node, node) {
+   if (of_property_read_u32_array(node, "reg", reg, 2))
+   continue;
+
+   if (reg[0] == config && reg[1] == ifnum)
+   return node;
+   }
+
+

Re: Device tree nodes for USB devices

2017-05-29 Thread Johan Hovold 
On Mon, May 29, 2017 at 11:01:52AM +0200, Martin Fuzzey wrote:
> Hi Peter,
> 
> I'm trying to create device tree nodes for USB devices (use case a USB 
> device providing GPIO and I2C controllers)
> 
> Your patch (now merged)
>  69bec725  "USB: core: let USB device know device node"
> 
> looks just like what I need but I have a couple of problems.
> 
> First it doesn't work at all on an i.MX53 because the actual struct 
> device for the USB host controller is a "ci_hdrc" which is a child of 
> the device having the DT node (ci_hdrc_imx)

I believe you should set the new sysdev pointer to the parent controller
device, which has the device tree node. Take a look at a8c06e407ef9
("usb: separate out sysdev pointer from usb_bus").

> However my larger question is that I don't see how to associate a DT 
> node with a USB *interface* rather than a USB *device*.

I started looking into this a while back but got interrupted. I have
some preliminary code, mostly lacking associated documentation.

> For instance in my use case I will have a single USB device providing 
> several USB interfaces (a composite device), including a GPIO controller 
> and a I2C controller. So I need 2 DT nodes...
> 
> There may be something in the the URL referenced in the binding doc 
> (http://www.firmware.org/1275/bindings/usb/usb-1_0.ps)
> but that seems to be down (at least has been for the last few days).

Yes, it's been down for weeks (at least) now.

> Is this something missing, still in progress,  or is each driver 
> supposed to handle it on its own?

So yes, it's in progress.

> That would be *possible* something like:
> 
> static int usb_gpio_probe(struct usb_interface *interface,
>const struct usb_device_id *id)
> {
>  struct device *dev = >dev;
>  struct device_node *usb_dev_np = dev->parent->of_node;
> 
>  struct device_node *usb_itf_np = of_get_child_by_name(usb_dev_np, 
> "gpio");
> 
>  ...
> }
> 
> With
>  {
>  #address-cells = <1>;
>  #size-cells = <0>;
> 
>  hub_board {
>  #address-cells = <1>;
>  #size-cells = <0>;
>  compatible = "pid,vid";
>  reg = <1>;
> 
>  ext_usb {
>  compatible = "pid2,vid2";
>  reg = <2>;
> 
>  usb_gpio: gpio {
>  }
>  };
>  };
> };
> 
> 
> But this doesn't seem like a very good idea since we're then likely to 
> grow lots of different driver specific ways of doing the same thing.
> Eg above I used a child node named "gpio" but it would probably have 
> been better to use the interface number in the node name in case there 
> are several. And what about matching by interface class...

Indeed. We should declare the interfaces in DT instead.

> I think a "struct device_node *" should be added to struct usb_interface 
> and the matching be done in the usb core code.
> Which means expressing in the DT as well all the different ways we can 
> match a USB interface.

struct usb_interface already has a device-node pointer in it's embedded
struct device. All that is missing is to set it up.

I'll post my work as an RFC shortly, and add you on CC.

Thanks,
Johan
--
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: [PATCH v1] usb: host: xhci: reliable endpoint reset after halt

2017-05-29 Thread Mathias Nyman 

On 26.05.2017 22:12, gavi...@thegavinli.com wrote:

From: Gavin Li 

If a stalling TRB is cancelled and NOOP'ed in xhci_handle_cmd_stop_ep(),
finish_td() never gets called to reset the halted endpoint and the
endpoint remains indefinitely stalled. This patch ensures that
xhci_cleanup_halted_endpoint() is called after a TRB completion if the
endpoint is halted, irregardless of the status of any pending TRBs.

Signed-off-by: Gavin Li 


Interesting, I'm trying to understand the details of what's going on here.

Does the event ring first have a stopped transfer event, then a stop endpoint
command completion event, and finally a transfer event with STALL_ERROR 
completion
code pointing to a no-op trb?

If this is the case do yo have any idea if the STALL_ERROR transfer event
was issued while the ring was stopping, or only after ring was restarted again?

current code relies on the the stopped transfer event to know where on the
endpoint ring hardware stopped, sometimes this might not be reliable.

I have a change for that, just pushed to my for-usb-next branch in my tree at:
git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git

Your patch basically adds a check for STALL_ERROR for no-op trbs, it reveals
that the driver currently skips handling all endpoint state related transfer
events if that TRB was canceled. This needs to investigated more

Thanks
-Mathias

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

[PATCH V3 2/2] usb: core: read USB ports from DT in the usbport LED trigger driver

2017-05-29 Thread Rafał Miłecki 
From: Rafał Miłecki 

This uses DT info to read relation description of LEDs and USB ports. If
DT has properly described LEDs, trigger will know when to turn them on.

Signed-off-by: Rafał Miłecki 
---
V2: Update to use "led-triggers"
V3: Update to use "trigger-sources"
---
 drivers/usb/core/ledtrig-usbport.c | 55 ++
 1 file changed, 55 insertions(+)

diff --git a/drivers/usb/core/ledtrig-usbport.c 
b/drivers/usb/core/ledtrig-usbport.c
index 1713248ab15a..29c9202226bf 100644
--- a/drivers/usb/core/ledtrig-usbport.c
+++ b/drivers/usb/core/ledtrig-usbport.c
@@ -11,8 +11,10 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
+#include 
 
 struct usbport_trig_data {
struct led_classdev *led_cdev;
@@ -123,6 +125,56 @@ static const struct attribute_group ports_group = {
  * Adding & removing ports
  ***/
 
+/**
+ * usbport_trig_port_observed - Check if port should be observed
+ */
+static bool usbport_trig_port_observed(struct usbport_trig_data *usbport_data,
+  struct usb_device *usb_dev, int port1)
+{
+   struct device *dev = usbport_data->led_cdev->dev;
+   struct device_node *led_np = dev->of_node;
+   struct of_phandle_args args;
+   struct device_node *port_np;
+   int count, i;
+
+   if (!led_np)
+   return false;
+
+   /* Get node of port being added */
+   port_np = usb_of_get_child_node(usb_dev->dev.of_node, port1);
+   if (!port_np)
+   return false;
+
+   /* Amount of trigger sources for this LED */
+   count = of_count_phandle_with_args(led_np, "trigger-sources",
+  "#source-cells");
+   if (count < 0) {
+   dev_warn(dev, "Failed to get trigger sources for %s\n",
+led_np->full_name);
+   return false;
+   }
+
+   /* Check list of sources for this specific port */
+   for (i = 0; i < count; i++) {
+   int err;
+
+   err = of_parse_phandle_with_args(led_np, "trigger-sources",
+"#source-cells", i, );
+   if (err) {
+   dev_err(dev, "Failed to get trigger source phandle at 
index %d: %d\n",
+   i, err);
+   continue;
+   }
+
+   of_node_put(args.np);
+
+   if (args.np == port_np)
+   return true;
+   }
+
+   return false;
+}
+
 static int usbport_trig_add_port(struct usbport_trig_data *usbport_data,
 struct usb_device *usb_dev,
 const char *hub_name, int portnum)
@@ -141,6 +193,8 @@ static int usbport_trig_add_port(struct usbport_trig_data 
*usbport_data,
port->data = usbport_data;
port->hub = usb_dev;
port->portnum = portnum;
+   port->observed = usbport_trig_port_observed(usbport_data, usb_dev,
+   portnum);
 
len = strlen(hub_name) + 8;
port->port_name = kzalloc(len, GFP_KERNEL);
@@ -255,6 +309,7 @@ static void usbport_trig_activate(struct led_classdev 
*led_cdev)
if (err)
goto err_free;
usb_for_each_dev(usbport_data, usbport_trig_add_usb_dev_ports);
+   usbport_trig_update_count(usbport_data);
 
/* Notifications */
usbport_data->nb.notifier_call = usbport_trig_notify,
-- 
2.11.0

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

[EXAMPLE V3 3/2] ARM: BCM53573: Specify ports for USB LED for Tenda AC9

2017-05-29 Thread Rafał Miłecki 
From: Rafał Miłecki 

Signed-off-by: Rafał Miłecki 
---
This patch *should not* be applied. It's only an EXAMPLE and that's why it uses
that weird 3/2 number.

It's a proof of concept, it was tested & will be submitted through ARM tree if
previous patches get accepted.

V3: Switch to the new binding
---
 arch/arm/boot/dts/bcm47189-tenda-ac9.dts | 1 +
 arch/arm/boot/dts/bcm53573.dtsi  | 4 
 2 files changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/bcm47189-tenda-ac9.dts 
b/arch/arm/boot/dts/bcm47189-tenda-ac9.dts
index 34417dac1cd0..ee44dec7d7ec 100644
--- a/arch/arm/boot/dts/bcm47189-tenda-ac9.dts
+++ b/arch/arm/boot/dts/bcm47189-tenda-ac9.dts
@@ -26,6 +26,7 @@
usb {
label = "bcm53xx:blue:usb";
gpios = < 1 GPIO_ACTIVE_HIGH>;
+   trigger-sources = <_port1>, <_port1>;
};
 
wps {
diff --git a/arch/arm/boot/dts/bcm53573.dtsi b/arch/arm/boot/dts/bcm53573.dtsi
index eae623f76401..5c0b9b4d679a 100644
--- a/arch/arm/boot/dts/bcm53573.dtsi
+++ b/arch/arm/boot/dts/bcm53573.dtsi
@@ -138,10 +138,12 @@
 
ehci_port1: port@1 {
reg = <1>;
+   #source-cells = <0>;
};
 
ehci_port2: port@2 {
reg = <2>;
+   #source-cells = <0>;
};
};
 
@@ -158,10 +160,12 @@
 
ohci_port1: port@1 {
reg = <1>;
+   #source-cells = <0>;
};
 
ohci_port2: port@2 {
reg = <2>;
+   #source-cells = <0>;
};
};
};
-- 
2.11.0

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

[PATCH V3 1/2] dt-bindings: leds: document new trigger-sources property

2017-05-29 Thread Rafał Miłecki 
From: Rafał Miłecki 

Some LEDs can be related to a specific device(s) described in the DT.
This property allows specifying such relations. E.g. USB LED should
usually be used to indicate some USB port(s) state.

Please note this binding is designed to be generic and not influenced by
any operating system design. Linux developers may find "trigger" part a
bit confusing since in Linux triggers are separated drivers. It
shouldn't define the binding though (we shouldn't add an extra level of
indirection).

Signed-off-by: Rafał Miłecki 
---
V2: Replace "usb-ports" with "led-triggers" property which is more generic and
allows specifying other devices as well.
V3: Use "trigger-sources" which is even more accurate as devices aren't
precisely triggers.
---
 Documentation/devicetree/bindings/leds/common.txt | 18 ++
 1 file changed, 18 insertions(+)

diff --git a/Documentation/devicetree/bindings/leds/common.txt 
b/Documentation/devicetree/bindings/leds/common.txt
index 24b656014089..e6e300975a4c 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -49,6 +49,19 @@ Optional properties for child nodes:
 - panic-indicator : This property specifies that the LED should be used,
if at all possible, as a panic indicator.
 
+- trigger-sources : List of devices which should be used as a source triggering
+   this LED activity. Some LEDs can be related to a specific
+   device and should somehow indicate its state. E.g. USB 2.0
+   LED may react to device(s) in a USB 2.0 port(s).
+   Another common example is switch or router with multiple
+   Ethernet ports each of them having its own LED assigned
+   (assuming they are not hardwired). In such cases this
+   property should contain phandle(s) of related source
+   device(s).
+   In many cases LED can be related to more than one device
+   (e.g. one USB LED vs. multiple USB ports) so a list of
+   sources can be specified.
+
 Required properties for flash LED child nodes:
 - flash-max-microamp : Maximum flash LED supply current in microamperes.
 - flash-max-timeout-us : Maximum timeout in microseconds after which the flash
@@ -69,6 +82,11 @@ gpio-leds {
linux,default-trigger = "heartbeat";
gpios = < 0 GPIO_ACTIVE_HIGH>;
};
+
+   usb {
+   gpios = < 1 GPIO_ACTIVE_HIGH>;
+   led-triggers = <_port1>, <_port1>;
+   };
 };
 
 max77693-led {
-- 
2.11.0

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

[PATCH V3 0/2] usb: introduce "trigger-sources" DT property for usbport trigger

2017-05-29 Thread Rafał Miłecki 
From: Rafał Miłecki 

Hi,

This is another try of adding relation between LEDs and devices to the DT (and
usbport). I followed discussions in all old threads and came with this V3.

I'd epxect both patches to go through Greg's usb.git if accepted.

For a reference (and before someome comes with already rejected solution) see
below history of this work:

1) [PATCH V2] leds: trigger: Introduce an USB port trigger
My initial try of adding "usb-ports" property. Rob said it should be more
generic.

2) [PATCH 1/2] dt-bindings: leds: document new usb-ports property
Rob still didn't like it due to being USB specific.

3) [PATCH V2 1/2] dt-bindings: leds: document new led-triggers property
Jacek didn't like it's more generic than what Linux can already support. Later
we agreed (?) that Linux limitations shouldn't influence DT structure.
Jacek still wanted a specific trigger but Rob said he won't accept something
specific to the USB.
Jacek suggested "trigger-sources" and Rob agreed on the name.
Jacek suggested trying triggers as separated nodes.

4) [PATCH 1/4] dt-bindings: leds: document property for LED triggers
This patch was trying to use separated nodes for triggers, e.g.:
foo-trigger { trigger-type = "foo"; };
Rob didn't like this extra level of indirection. He said Linux drivers shouldn't
define/influence the binding.

So finally I came with this patchset. It doesn't try to use separated nodes for
triggers anymore and it uses the name that was mostly accepted I think. It
should be generic and not influenced by Linux design.

Rafał Miłecki (3):
  dt-bindings: leds: document new trigger-sources property
  usb: core: read USB ports from DT in the usbport LED trigger driver
  ARM: BCM53573: Specify ports for USB LED for Tenda AC9

 Documentation/devicetree/bindings/leds/common.txt | 18 
 arch/arm/boot/dts/bcm47189-tenda-ac9.dts  |  1 +
 arch/arm/boot/dts/bcm53573.dtsi   |  4 ++
 drivers/usb/core/ledtrig-usbport.c| 55 +++
 4 files changed, 78 insertions(+)

-- 
2.11.0

--
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: [PATCH 3/3] usb: typec: ucsi: Add ACPI driver

2017-05-29 Thread Heikki Krogerus 
On Fri, May 26, 2017 at 06:30:38AM -0700, Guenter Roeck wrote:
> On 05/26/2017 04:08 AM, Heikki Krogerus wrote:
> > Hi,
> > 
> > On Thu, May 25, 2017 at 06:23:30AM -0700, Guenter Roeck wrote:
> > > > +   /*
> > > > +* NOTE: The memory region for the data structures is used also 
> > > > in an
> > > > +* operation region, which means ACPI has already reserved it. 
> > > > Therefore
> > > > +* it can not be requested here.
> > > > +*/
> > > > +   ua->ppm.data = devm_ioremap(>dev, res->start, 
> > > > resource_size(res));
> > > 
> > > devm_ioremap_resource() ?
> > 
> > devm_ioremap_resource() will try to request the region, and like I
> > tried to explain in the comment, it will fail as the region has
> > already been reserved by ACPI.
> > 
> 
> Maybe add this to the comment above ?
> 
>   .. it can not be requested here, and we can not use devm_ioremap_resource().

OK.

Thanks,

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

Re: [PATCH 2/3] usb: typec: Add support for UCSI interface

2017-05-29 Thread Heikki Krogerus 
On Fri, May 26, 2017 at 06:26:01AM -0700, Guenter Roeck wrote:
> > > What happens if trace is not enabled ? If I recall discussions around 
> > > CLANG
> > > correctly, it complains about unused static inline functions.
> > 
> > Nothing happens if trace is not enable. Everything continues to
> > compile just like before.
> > 
> > Maybe you were trying to define the tracepoints in C code? That won't
> > work.
> > 
> > > Anyway, is it really necessary to have an include file for this instead of
> > > keeping the code in trace.c ? I am somewhat wary of variable declarations
> > > in include files - include twice and there will be two instances.
> > 
> > The tracepoint documentation actually states that you need to place the
> > definitions of the tracepoints in a header. I have the trace.c file only
> > because it was a convenient place to put the tracepoint statement in.
> > 
> > I don't know the inner workings of tracepoints, but they do work fine
> > for me. In case you want more info on the topic, I never read these
> > articles, but they were mentioned in Documentation/trace/tracepoints.txt:
> > 
> > http://lwn.net/Articles/379903
> > http://lwn.net/Articles/381064
> > http://lwn.net/Articles/383362
> > 
> > I believe they provide at least some kind of description on how
> > tracepoints actually work.
> > 
> Sorry for not being clear; that was a question about your code, not mine.
> If it builds without warnings with trace disabled, no problem.

Ah, of course. Silly me. Sorry about that.

> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > > +#include 
> > > 
> > > Any preference in usb about alphabetic order of include files ?
> > 
> > I don't see any mention of it in the documentation, so this is
> > probable just matter of taste in the end. One likes the mother, one
> > likes the daughter.
> > 
> 
> I like alphabetic because it makes life easier later on, if/when additional
> include files are added, and it is easier to find a specific include file in
> an alphabetic list. Some upstream maintainers ask for it nowadays (including 
> me :-).

It's good to keep the headers in some order just for the sake of being
organized, but I don't see how alphabetical order is any better then
for example descending.

I normally would not care about this topic and I would have just
ordered the headers alphabetically if anybody asked for it, but I've
actually proposed ordering the headers alphabetically myself ones, and
this is what the upstream maintainer said: "Can we stick to serious
critiques"

I think some maintainers do not see the need to dictate styling rules
on everything, and leave some things to be decided by the developer. I
kind of like that.

But if you still feel strongly about this, I'll change the style, but
for now, I'll keep it the way it is. Actually, I'll move the last one
on top.

> > > > +/**
> > > > + * ucsi_notify - PPM notification handler
> > > > + * @ucsi: Source UCSI Interface for the notifications
> > > > + *
> > > > + * Handle notifications from PPM of @ucsi.
> > > > + */
> > > > +void ucsi_notify(struct ucsi *ucsi)
> > > > +{
> > > > +   struct ucsi_cci *cci;
> > > > +
> > > > +   /* There is no requirement to sync here, so only warning if it 
> > > > fails */
> > > > +   if (ucsi_sync(ucsi))
> > > > +   dev_warn(ucsi->dev, "%s: sync failed\n", __func__);
> > > > +
> > > Why even a warning if it is not a requirement ?
> > 
> > It still should not "ever" fail. Why not create the warning? It is
> > after all indication that something is really wrong, and we probable
> > want to know about it.
> > 
> Your call, of course. I just dislike noisy drivers, and I am always concerned
> about flooding the kernel log.

Fair enough, I'll remove the warning.

> > > > +static int
> > > > +ucsi_dr_swap(const struct typec_capability *cap, enum typec_data_role 
> > > > role)
> > > > +{
> > > > +   struct ucsi_connector *con = to_ucsi_connector(cap);
> > > > +   struct ucsi_control ctrl;
> > > > +   int ret = 0;
> > > > +
> > > > +   if (!con->partner)
> > > > +   return -EAGAIN;
> > > 
> > > Doesn't this result in an immediate retry by user space ?
> > 
> > Isn't that what tcpm.c reports? Maybe I misunderstood the code.
> > I'll change that to something else.
> > 
> Good point; tcpm returns -EAGAIN if the port is not in a READY state.
> Maybe I should change the tcpm code ? No idea what error to use instead,
> though.

I'll use -ENOTCONN for now. Let me know if it's not good.

> > > > +/**
> > > > + * ucsi_register_ppm - Register UCSI PPM Interface
> > > > + * @dev: Device interface to the PPM
> > > > + * @ppm: The PPM interface
> > > > + *
> > > > + * Allocates UCSI instance, associates it with @ppm and returns it to 
> > > > the
> > > > + * caller, and schedules initialization of the interface.
> > > > + */
> > > > +struct ucsi *ucsi_register_ppm(struct device *dev, struct ucsi_ppm

Re: [PATCH] usb: xhci: ASMedia ASM1042A chipset need shorts TX quirk

2017-05-29 Thread Mathias Nyman 

On 27.05.2017 22:26, Corentin Labbe wrote:

When plugging an USB webcam I see the following message:
[106385.615559] xhci_hcd :04:00.0: WARN Successful completion on short TX: 
needs XHCI_TRUST_TX_LENGTH quirk?
[106390.583860] handle_tx_event: 913 callbacks suppressed

With this patch applied, I get no more printing of this message.

Signed-off-by: Corentin Labbe 
---



Thanks, adding

-Mathias

--
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: [PATCH 3/3] mfd: twl: move header file out of I2C realm

2017-05-29 Thread Bartlomiej Zolnierkiewicz 
On Monday, May 22, 2017 12:02:10 AM Wolfram Sang wrote:
> include/linux/i2c is not for client devices. Move the header file to a
> more appropriate location.
> 
> Signed-off-by: Wolfram Sang 

Acked-by: Bartlomiej Zolnierkiewicz 

> ---
>  arch/arm/mach-omap2/common.h| 2 +-
>  arch/arm/mach-omap2/omap_twl.c  | 2 +-
>  drivers/gpio/gpio-twl4030.c | 2 +-
>  drivers/iio/adc/twl4030-madc.c  | 2 +-
>  drivers/iio/adc/twl6030-gpadc.c | 2 +-
>  drivers/input/keyboard/twl4030_keypad.c | 2 +-
>  drivers/input/misc/twl4030-pwrbutton.c  | 2 +-
>  drivers/input/misc/twl4030-vibra.c  | 2 +-
>  drivers/mfd/twl-core.c  | 6 +++---
>  drivers/mfd/twl4030-audio.c | 2 +-
>  drivers/mfd/twl4030-irq.c   | 2 +-
>  drivers/mfd/twl4030-power.c | 2 +-
>  drivers/mfd/twl6030-irq.c   | 2 +-
>  drivers/phy/phy-twl4030-usb.c   | 2 +-
>  drivers/power/supply/twl4030_charger.c  | 2 +-
>  drivers/pwm/pwm-twl-led.c   | 2 +-
>  drivers/pwm/pwm-twl.c   | 2 +-
>  drivers/regulator/twl-regulator.c   | 2 +-
>  drivers/regulator/twl6030-regulator.c   | 2 +-
>  drivers/rtc/rtc-twl.c   | 2 +-
>  drivers/usb/phy/phy-twl6030-usb.c   | 2 +-
>  drivers/video/backlight/pandora_bl.c| 2 +-
>  drivers/watchdog/twl4030_wdt.c  | 2 +-
>  include/linux/{i2c => mfd}/twl.h| 0
>  sound/soc/codecs/twl4030.c  | 2 +-
>  25 files changed, 26 insertions(+), 26 deletions(-)
>  rename include/linux/{i2c => mfd}/twl.h (100%)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics

--
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: [PATCH 2/3] mfd: tps65010: move header file out of I2C realm

2017-05-29 Thread Bartlomiej Zolnierkiewicz 
On Monday, May 22, 2017 12:02:09 AM Wolfram Sang wrote:
> include/linux/i2c is not for client devices. Move the header file to a
> more appropriate location.
> 
> Signed-off-by: Wolfram Sang 

Acked-by: Bartlomiej Zolnierkiewicz 

> ---
>  arch/arm/mach-omap1/board-h2-mmc.c  | 2 +-
>  arch/arm/mach-omap1/board-h2.c  | 2 +-
>  arch/arm/mach-omap1/board-h3-mmc.c  | 2 +-
>  arch/arm/mach-omap1/board-h3.c  | 2 +-
>  arch/arm/mach-omap1/board-osk.c | 2 +-
>  arch/arm/mach-s3c24xx/mach-osiris-dvs.c | 2 +-
>  arch/arm/mach-s3c24xx/mach-osiris.c | 2 +-
>  drivers/mfd/tps65010.c  | 2 +-
>  drivers/usb/host/ohci-omap.c| 2 +-
>  drivers/usb/phy/phy-isp1301-omap.c  | 2 +-
>  drivers/video/fbdev/omap/lcd_h3.c   | 2 +-
>  include/linux/{i2c => mfd}/tps65010.h   | 2 +-
>  12 files changed, 12 insertions(+), 12 deletions(-)
>  rename include/linux/{i2c => mfd}/tps65010.h (99%)

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R Institute Poland
Samsung Electronics

--
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: USB hub clear POWER feature -> device still in 'lsusb' ?!

2017-05-29 Thread Ladislav Michl 
On Tue, May 23, 2017 at 11:40:58PM +0200, Harald Welte wrote:
[snip]
> But this weird combination between sysfs on the one hand side and
> directly issuing USB hub control requests via uhubctl on the other side
> really looks ugly.  Is this really how one is supposed to power-cycle a
> USB device?  It doesn't seam like such a completely odd requirement,
> does it?

I knew I'm not the only one having to deal with ever-s(t)ucking USB modems...
For the time being issue was "solved" either using dedicated USB controler
for modem, where toggling VBUS does the trick or putting modem into the
HUB with devices which can be safely restarted randomly (from their point
of view) together with modem - powering whole HUB down for a while.

But, in your case, wouldn't it be easiest solution to wire USB device
unplug in hardware? I hope I understood well you built your own HUB
device.

Best regards,
ladis
--
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

Device tree nodes for USB devices

2017-05-29 Thread Martin Fuzzey 

Hi Peter,

I'm trying to create device tree nodes for USB devices (use case a USB 
device providing GPIO and I2C controllers)


Your patch (now merged)
69bec725  "USB: core: let USB device know device node"

looks just like what I need but I have a couple of problems.

First it doesn't work at all on an i.MX53 because the actual struct 
device for the USB host controller is a "ci_hdrc" which is a child of 
the device having the DT node (ci_hdrc_imx)


I can fix that by accepting parent devices too like this:


--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -466,12 +466,23 @@ struct usb_device *usb_alloc_dev(struct usb_device 
*parent,

 * cardbus or pci hotplugging, and so on.
 */
if (unlikely(!parent)) {
+   struct device *cdev = bus->controller;
+
dev->devpath[0] = '0';
dev->route = 0;

dev->dev.parent = bus->controller;
-   dev->dev.of_node = bus->controller->of_node;
+
+   do {
+   dev->dev.of_node = cdev->of_node;
+   cdev = cdev->parent;
+   } while(!dev->dev.of_node && cdev);
+
dev_set_name(>dev, "usb%d", bus->busnum);


(I can submit this properly if we agree it's the right way - I'm not 
sure if this should be fixed in the core or if ci_hdrc should set its DT 
node to be the same as its parent)


However my larger question is that I don't see how to associate a DT 
node with a USB *interface* rather than a USB *device*.


For instance in my use case I will have a single USB device providing 
several USB interfaces (a composite device), including a GPIO controller 
and a I2C controller. So I need 2 DT nodes...


There may be something in the the URL referenced in the binding doc 
(http://www.firmware.org/1275/bindings/usb/usb-1_0.ps)

but that seems to be down (at least has been for the last few days).
I also don't see any code that actually uses the compatible property 
anyway (even the VID/PID compatible)


Is this something missing, still in progress,  or is each driver 
supposed to handle it on its own?


That would be *possible* something like:

static int usb_gpio_probe(struct usb_interface *interface,
  const struct usb_device_id *id)
{
struct device *dev = >dev;
struct device_node *usb_dev_np = dev->parent->of_node;

struct device_node *usb_itf_np = of_get_child_by_name(usb_dev_np, 
"gpio");


...
}

With
 {
#address-cells = <1>;
#size-cells = <0>;

hub_board {
#address-cells = <1>;
#size-cells = <0>;
compatible = "pid,vid";
reg = <1>;

ext_usb {
compatible = "pid2,vid2";
reg = <2>;

usb_gpio: gpio {
}
};
};
};


But this doesn't seem like a very good idea since we're then likely to 
grow lots of different driver specific ways of doing the same thing.
Eg above I used a child node named "gpio" but it would probably have 
been better to use the interface number in the node name in case there 
are several. And what about matching by interface class...


I think a "struct device_node *" should be added to struct usb_interface 
and the matching be done in the usb core code.
Which means expressing in the DT as well all the different ways we can 
match a USB interface.


Regards,

Martin



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