RE: [PATCH v2 0/3] Improvement, fix and new entry for dwc3 debugfs

[Felipe Balbi]

HI

"Du, Changbin"  writes:
>> before I review your patches, one comment
>> 
>> changbin...@intel.com writes:
>> > From: "Du, Changbin" 
>> >
>> > The first patch removed unnecessary checking for debugfs api call;
>> > The second patch fix a memory leak issue;
>> > The third patch add one new entry to debufs.
>> >
>> > Du, Changbin (3):
>> >   usb: dwc3: make dwc3_debugfs_init return value be void
>> 
>> this is _not_ a fix
>> 
>> >   usb: dwc3: free dwc->regset on dwc3_debugfs_exit
>> 
>> but this is. Why isn't this, at least, the first patch in the list ? In
>> fact, it would be preferred that this patch be sent by itself and the
>> following two patches should be on another branch completely without any
>> dependencies to the memory leak fix.
>> 
>> --
>> Balbi
>
> Sure, Balbi. This will be better. I will send out patch v3 and another 
> independent
> patch. Also include changelog as Greg required. Thanks for checking.

thanks, that way we can get the fix during the -rc cycle and the other
two patches on next merge window ;-)

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v2 0/3] Improvement, fix and new entry for dwc3 debugfs

[Du, Changbin]

> before I review your patches, one comment
> 
> changbin...@intel.com writes:
> > From: "Du, Changbin" 
> >
> > The first patch removed unnecessary checking for debugfs api call;
> > The second patch fix a memory leak issue;
> > The third patch add one new entry to debufs.
> >
> > Du, Changbin (3):
> >   usb: dwc3: make dwc3_debugfs_init return value be void
> 
> this is _not_ a fix
> 
> >   usb: dwc3: free dwc->regset on dwc3_debugfs_exit
> 
> but this is. Why isn't this, at least, the first patch in the list ? In
> fact, it would be preferred that this patch be sent by itself and the
> following two patches should be on another branch completely without any
> dependencies to the memory leak fix.
> 
> --
> Balbi

Sure, Balbi. This will be better. I will send out patch v3 and another 
independent
patch. Also include changelog as Greg required. Thanks for checking.

Regards,
Du, Changbin
--
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: DWC3: Clock Domain Crossing erratum description.

[Felipe Balbi]

Hi,

Kirill Dronov  writes:
> I'm working on USB device bringup on Intel E3800 – based board. DWC3
> core configured as DRD in device mode. The only connected device phy
> is SMSC 3310 (USB2 ULPI). DWC3 core version is 2.10A. Gadget zero
> driver can be loaded, but device enumeration fails: device is detected
> by host, speed is negotiated (HS), host successfully reset device. On
> device side – conndone interrupt is followed by linksts change with
> link_state 0.  Then host sends USBREQ_GET_DESCRIPTOR and tries to set
> address but device does not react – no events generated.

which kernel are you using ? Please collect driver traces and send them
to us:

# mount -t debugfs none /sys/kernel/debug
# cd /sys/kernel/debug/tracing
# echo 2048 > buffer_size_kb
# echo 1 > events/dwc3/enable

(now trigger the problem)

# cp trace /path/to/non/volatile/media/

Send that trace file (you need to compress it, probably)

> I'm not sure if I hit “run/stop metastability” issue [“STAR#9000525659: 
> Clock Domain Crossing on DCTL in USB 2.0 Mode”]. I don't have DWC3 

we have a workaround for that, you shouldn't hit it unless you removed
the workaround.

> databook or erratum description (other than mentioned in driver code). 
> Can somebody provide more detailed description of STAR#9000525659?
> Where can I get DWC3 Databook?

you need to talk to a Synopsys representative for that.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 0/3] Improvement, fix and new entry for dwc3 debugfs

[Felipe Balbi]

Hi,

before I review your patches, one comment

changbin...@intel.com writes:
> From: "Du, Changbin" 
>
> The first patch removed unnecessary checking for debugfs api call;
> The second patch fix a memory leak issue;
> The third patch add one new entry to debufs.
>
> Du, Changbin (3):
>   usb: dwc3: make dwc3_debugfs_init return value be void

this is _not_ a fix

>   usb: dwc3: free dwc->regset on dwc3_debugfs_exit

but this is. Why isn't this, at least, the first patch in the list ? In
fact, it would be preferred that this patch be sent by itself and the
following two patches should be on another branch completely without any
dependencies to the memory leak fix.

-- 
balbi


signature.asc
Description: PGP signature

Re: raid 5 creation fails on usb3 connected drives kernel 4.4.x, 4.5

[Brian Chadwick]

On 06/04/16 19:53, Greg KH wrote:

On Tue, Apr 05, 2016 at 06:42:51PM +1000, Brian Chadwick wrote:

SETUP:

i7 16GB Computer.

1 x PCI-x USB3 adaptor card (i think uses xhci-hcd)04:00.0 USB controller:
Renesas Technology Corp. uPD720202 USB 3.0 Host Controller (rev 02)
 Kernel driver in use: xhci_hcd

2 x USB3 to dual SATA HDD docks. uses JMicron JMS56x Series controllers,
uses uas.ko kernel module
Bus 004 Device 002: ID 152d:9561 JMicron Technology Corp. / JMicron USA
Technology Corp.
Bus 004 Device 003: ID 152d:9561 JMicron Technology Corp. / JMicron USA
Technology Corp.

3 x Western Digital 320GB 3.5" SATA drives

USE:

RAID 5

KERNEL:
4.3.5

System works perfectly as expected.


Change to kernel 4.4.x or 4.5 and RAID 5 doesnt work. I am not sure whether
its actually RAID 5 at fault or if its the USB Attached SCSI driver uas.ko,
or maybe the host driver xhci-hcd.

Can you run 'git bisect' to try to track down the offending commit?

thanks,

greg k-h


Hi, I am unfamiliar with that command, when should it be run, and in 
what directory?


thanks

Brian C

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



--
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 v3 3/7] usb: mux: add common code for Intel dual role port mux

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
> On Wed, Apr 06, 2016 at 01:19:04PM +0300, Felipe Balbi wrote:
>> > What happened to getting internal help in designing this api?  This
>> > shouldn't be my job :)
>> 
>> I looked at this Baolu but, at least to me, it's unclear what you're
>> hinting here. Sure, the API is a bit odd in that we register a mux and
>> unregister a device (that has been fixed), but is there anything else ?
>
> I don't remember anymore, that was a few thousand emails ago...
>
> But the suggestion about using devm_* shows that the original author
> has very little understanding about how the driver model works at all,
> which doesn't give me much hope at the moment...

fair enough, that can also be 'fixed' with experience, right ? Just
takes time ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Peter Chen  writes:
>> >> > In below change of usb_gadget_vbus_draw(), already can get charger
>> >> > type via usb_charger_get_type().
>> >> >
>> >> > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget,
>> >> > unsigned mA)  {
>> >> > +   enum usb_charger_type type;
>> >> > +
>> >> > +   if (gadget->charger) {
>> >> > +   type = usb_charger_get_type(gadget->charger);
>> >> > +   usb_charger_set_cur_limit_by_type(gadget->charger, type,
>> >> mA);
>> >> > +   }
>> >> > +
>> >> > if (!gadget->ops->vbus_draw)
>> >> > return -EOPNOTSUPP;
>> >> > return gadget->ops->vbus_draw(gadget, mA);
>> >> >
>> >> > Could you detail in what situation gadget->ops-> get_charger_type() is
>> >> used?
>> >> 
>> >> isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling
>> >> it ? What did I miss here ?
>> >
>> > Well, that's true, so my real meaning is why gadget need get charger type
>> > via another new api gadget->ops->get_charger_type().
>> 
>> because of semantics. usb_gadget_vbus_draw() is *only* supposed to
>> connect a load across vbus and gnd so some battery can be charged. Also,
>> we need to abstract away this ->get_charger_type() operation because it
>> might be different for each UDC.
>
> In this patch set, there are two ->get_charger_type in below two
> structures, as my understanding, get_charger_type at struct usb_charger
> can be implemented at UDC drivers; But I don't see necessary we
> need to implement get_charger_type for usb_gadget_ops at UDC drivers
> again. What do you think?

I had missed that completely, nice catch. I agree with you, there should
be one place where this is implemented and struct usb_charger sounds
like it is that place.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

Hi,

Peter Chen  writes:
> On Wed, Apr 06, 2016 at 01:25:06PM +0300, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Peter Chen  writes:
>> > On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
>> >> Peter Chen  writes:
>> >> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>> >> >> Peter Chen  writes:
>> >> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> >> >> >  +
>> >> >> >> +static struct attribute *usb_charger_attrs[] = {
>> >> >> >> +   _attr_sdp_current.attr,
>> >> >> >> +   _attr_dcp_current.attr,
>> >> >> >> +   _attr_cdp_current.attr,
>> >> >> >> +   _attr_aca_current.attr,
>> >> >> >> +   _attr_charger_type.attr,
>> >> >> >> +   _attr_charger_state.attr,
>> >> >> >> +   NULL
>> >> >> >> +};
>> >> >> >
>> >> >> > The user may only care about current limit, type and state, why they
>> >> >> > need to care what type's current limit, it is the usb charger
>> >> >> > framework handles, the framework judge the current according to
>> >> >> > charger type and USB state (connect/configured/suspended).
>> >> >> 
>> >> >> it might be useful if we want to know that $this charger doesn't really
>> >> >> give us as much current as it advertises.
>> >> >> 
>> >> >
>> >> > As my understanding, the current limit is dynamic value, it should
>> >> > report the value the charger supports now, eg, it connects SDP, but
>> >> > the host is suspended now, then the value should be 2mA.
>> >> 
>> >> yes, and that's the limit. Now consider we connect to DCP or CDP and
>> >> limit is 2000mA but we're charging at 1000mA ;-)
>> >> 
>> >
>> > Does the user need to know the $this charger limit? Don't they only
>> > care about the current charging value? I have a USB cable which can
>> 
>> Why not ? UI might want to change the color of the battery charging icon
>> if we're charging @ 2000mA or @ 1000mA to give some visual feedback as
>> to "how fast" battery is supposed to be charged.
>> 
>> > show charging current value, it changes from time to time, when it
>> > connects to host pc, it shows 430mA; when it connects to dedicated
>> > charger, it shows 1000mA.
>> 
>> good for you, now what does that have to do with $subject ?
>> 
>
> +static struct attribute *usb_charger_attrs[] = {
> + _attr_sdp_current.attr,
> + _attr_dcp_current.attr,
> + _attr_cdp_current.attr,
> + _attr_aca_current.attr,
> + _attr_charger_type.attr,
> + _attr_charger_state.attr,
> + NULL
> +};
>
> Ok, even the users are interesting in current limit, we still have no
> necessary to know all kinds of chargers limit and current value, since
> we already have charger type user interface, the framework can show
> limit according to charger type.

Oh, now I get your comment and I totally agree. We already *know* the
detected charger type, there's no point in showing them all.

> I think below user interfaces are enough, who do you think?
>
> +static struct attribute *usb_charger_attrs[] = {
> + _attr_current.attr,
> + _attr_current_limit.attr,
> + _attr_charger_type.attr,
> + _attr_charger_state.attr,
> + NULL
> +};

agreed, const though.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Peter Chen]

On Wed, Apr 06, 2016 at 04:58:03PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Jun Li  writes:
> >> >> >> > Since we already have get_charger_type callback at usb_charger
> >> >> >> > structure, why we still need this API at usb_gadget_ops?
> >> >> >>
> >> >> >> In case some users want to get charger type at gadget level.
> >> >> >>
> >> >> > Why gadget needs to know charger type? I also don't catch the
> >> >> > intent of
> >> >>
> >> >> because some gadgets need to call usb_gadget_vbus_draw(), although
> >> >> for that they need power in mA rather.
> >> >
> >> > In below change of usb_gadget_vbus_draw(), already can get charger
> >> > type via usb_charger_get_type().
> >> >
> >> > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget,
> >> > unsigned mA)  {
> >> > +   enum usb_charger_type type;
> >> > +
> >> > +   if (gadget->charger) {
> >> > +   type = usb_charger_get_type(gadget->charger);
> >> > +   usb_charger_set_cur_limit_by_type(gadget->charger, type,
> >> mA);
> >> > +   }
> >> > +
> >> > if (!gadget->ops->vbus_draw)
> >> > return -EOPNOTSUPP;
> >> > return gadget->ops->vbus_draw(gadget, mA);
> >> >
> >> > Could you detail in what situation gadget->ops-> get_charger_type() is
> >> used?
> >> 
> >> isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling
> >> it ? What did I miss here ?
> >
> > Well, that's true, so my real meaning is why gadget need get charger type
> > via another new api gadget->ops->get_charger_type().
> 
> because of semantics. usb_gadget_vbus_draw() is *only* supposed to
> connect a load across vbus and gnd so some battery can be charged. Also,
> we need to abstract away this ->get_charger_type() operation because it
> might be different for each UDC.

In this patch set, there are two ->get_charger_type in below two
structures, as my understanding, get_charger_type at struct usb_charger
can be implemented at UDC drivers; But I don't see necessary we
need to implement get_charger_type for usb_gadget_ops at UDC drivers
again. What do you think?

struct usb_gadget_ops {

struct usb_ep *(*match_ep)(struct usb_gadget *,
+   /* get the charger type */
+   enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
};

struct usb_charger {
...
+   /* user can get charger type by implementing this callback */
+   enum usb_charger_type   (*get_charger_type)(struct usb_charger
*);
}
> 
> $subject has a fragility, however: It's assuming that we should always
> call ->get_charger_type() before ->vbus_draw(), but that's a good
> default, I'd say.
> 

-- 

Best Regards,
Peter Chen
--
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 v9 1/4] gadget: Introduce the usb charger framework

[Peter Chen]

On Wed, Apr 06, 2016 at 01:25:06PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Peter Chen  writes:
> > On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
> >> Peter Chen  writes:
> >> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
> >> >> Peter Chen  writes:
> >> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> >> >> >  +
> >> >> >> +static struct attribute *usb_charger_attrs[] = {
> >> >> >> +_attr_sdp_current.attr,
> >> >> >> +_attr_dcp_current.attr,
> >> >> >> +_attr_cdp_current.attr,
> >> >> >> +_attr_aca_current.attr,
> >> >> >> +_attr_charger_type.attr,
> >> >> >> +_attr_charger_state.attr,
> >> >> >> +NULL
> >> >> >> +};
> >> >> >
> >> >> > The user may only care about current limit, type and state, why they
> >> >> > need to care what type's current limit, it is the usb charger
> >> >> > framework handles, the framework judge the current according to
> >> >> > charger type and USB state (connect/configured/suspended).
> >> >> 
> >> >> it might be useful if we want to know that $this charger doesn't really
> >> >> give us as much current as it advertises.
> >> >> 
> >> >
> >> > As my understanding, the current limit is dynamic value, it should
> >> > report the value the charger supports now, eg, it connects SDP, but
> >> > the host is suspended now, then the value should be 2mA.
> >> 
> >> yes, and that's the limit. Now consider we connect to DCP or CDP and
> >> limit is 2000mA but we're charging at 1000mA ;-)
> >> 
> >
> > Does the user need to know the $this charger limit? Don't they only
> > care about the current charging value? I have a USB cable which can
> 
> Why not ? UI might want to change the color of the battery charging icon
> if we're charging @ 2000mA or @ 1000mA to give some visual feedback as
> to "how fast" battery is supposed to be charged.
> 
> > show charging current value, it changes from time to time, when it
> > connects to host pc, it shows 430mA; when it connects to dedicated
> > charger, it shows 1000mA.
> 
> good for you, now what does that have to do with $subject ?
> 

+static struct attribute *usb_charger_attrs[] = {
+   _attr_sdp_current.attr,
+   _attr_dcp_current.attr,
+   _attr_cdp_current.attr,
+   _attr_aca_current.attr,
+   _attr_charger_type.attr,
+   _attr_charger_state.attr,
+   NULL
+};

Ok, even the users are interesting in current limit, we still have no
necessary to know all kinds of chargers limit and current value, since
we already have charger type user interface, the framework can show
limit according to charger type.

I think below user interfaces are enough, who do you think?

+static struct attribute *usb_charger_attrs[] = {
+   _attr_current.attr,
+   _attr_current_limit.attr,
+   _attr_charger_type.attr,
+   _attr_charger_state.attr,
+   NULL
+};

-- 

Best Regards,
Peter Chen
--
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 v2 10/14] USB: ch341: fix coding style

[Grigori Goronzy]

On 04/06/2016 08:10 PM, Johan Hovold wrote:
> As Joe already said, we generally don't want indentation-only changes to
> existing code. Just try to stick to the style of the driver (even if
> it's inconsistent at times).
> 

Hm, I don't get it.  I understand that white-space-only changes are
discouraged if they are freestanding and contributors don't follow up
with any change to functionality (as outlined in
development-process/4.Coding), but this is not the case here.  IMHO, if
the style of a module is inconsistent, it should be fixed at some point.
 The kind of policy you are presenting here will in the long run lead to
messy code, and can't be found in any of the official documents (e.g.
CodingStyle, SubmitChecklist, development-process/) either.  It also
encourages mixing white-space changes with patches that change
functionality, which is a bad practice.

I'll just drop the indentation changes. The rest is fine, I guess?

Grigori



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Wed, Apr 06, 2016 at 01:19:04PM +0300, Felipe Balbi wrote:
> > What happened to getting internal help in designing this api?  This
> > shouldn't be my job :)
> 
> I looked at this Baolu but, at least to me, it's unclear what you're
> hinting here. Sure, the API is a bit odd in that we register a mux and
> unregister a device (that has been fixed), but is there anything else ?

I don't remember anymore, that was a few thousand emails ago...

But the suggestion about using devm_* shows that the original author has
very little understanding about how the driver model works at all, which
doesn't give me much hope at the moment...

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

Re: [Linux-v4.6-rc2] atkbd serio0: Unknown key pressed (translated set 2, code 0xa8 on isa0060/serio0).

[Jiri Kosina]

On Wed, 6 Apr 2016, Dmitry Torokhov wrote:

> >>> [  685.425634] atkbd serio0: Unknown key pressed (translated set 2,
> >>> code 0xa8 on isa0060/serio0).
> >>> [  685.425648] atkbd serio0: Use 'setkeycodes e028 ' to make it 
> >>> known.
> >>>
> >>> Known issue?
> >>>
> >>> Do you need more input?
> >>>
> >>> Linux-config and dmesg-output attached.
> >>>
> >>> Regards,
> >>> - Sedat Dilek -
> >>
> >> Attached is output of...
> >>
> >>$ xmodmap -pke > /tmp/xmodmap-pke.txt
> >>
> >
> > [ CC (USB)HID folks ]
> >
> > I forgot to tell that I have some patches on top of vanilla Linux v4.6-rc2.
> >
> > Dropping the patches from  makes
> > the issue go away.
> > Unsure what is the cause.
> 
> Hmm, this is quite curious, none of the recent patches in
> for-4.6/upstream-fixes touches atkbd. I'd be very interested in which
> one exactly causes this.

It's indeed odd.

Sedat, the kernel in question is compiled by gcc, right? (just making sure 
given the past experience :) ).

Thanks,

-- 
Jiri Kosina
SUSE Labs

--
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/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Alan Stern]

On Wed, 6 Apr 2016, Greg KH wrote:

> On Wed, Apr 06, 2016 at 04:08:04PM +0300, Mathias Nyman wrote:
> > We don't want to runtime suspend a bus if there is an event pending.
> > The roothub on a NEC uPD720200 host with a single USB3 device connected
> > might go back to runtime suspend immediately after runtime resume as
> > hub might not yet see any port changes in resume.
> > 
> > Prevent this by checking if there is a unhandled event pending when
> > calling runtime suspend.
> 
> As Alan points out, you didn't actually test this :(

Also as Björn pointed out, failing a system suspend isn't a good idea.  
In general, you should do it only if wakeup is enabled and there is a 
pending wakeup event.

In this case there may be alternatives.  For example, you could just 
delay a few ms until the pending event has been handled.  Or, if you 
really just want to prevent runtime suspend, you should check to see if 
this was a runtime suspend and not a system suspend.  Or you could 
prevent the bus from being runtime suspended in the first place by 
doing a pm_runtime_get_sync().

Alan Stern

--
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 v2 2/3] usb: dwc3: free dwc->regset on dwc3_debugfs_exit

[Greg KH]

On Wed, Apr 06, 2016 at 11:44:05PM +0800, changbin...@intel.com wrote:
> From: "Du, Changbin" 
> 
> Signed-off-by: Du, Changbin 
> ---

You need a changelog entry in order for a patch to be able to be
accepted.

thanks,

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

Oops in 4.6-rc2: NULL pointer dereference in cdc-acm

[Gabriele Mazzotta]

Hi,

I'm getting a kernel oops when I plug some smartphone via USB to my
laptop, which is currently running the v4.6-rc2.

The problem seems to be caused by a81cf9799ad7 ("cdc-acm: implement
put_char() and flush_chars()").

A simple NULL pointer check prevents the crash, but since I have no
use of cdc-acm and I didn't read the code, I don't know if some other
changes are required.

Here below you can find the change I did to prevent the crash and
the dmesg showing the problem.

Regards,
Gabriele


diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index 83fd30b..aa0c244 100644
--- a/drivers/usb/class/cdc-acm.c
+++ b/drivers/usb/class/cdc-acm.c
@@ -754,8 +754,9 @@ static void acm_tty_flush_chars(struct tty_struct *tty)
 
if (acm->susp_count)
usb_anchor_urb(cur->urb, >delayed);
-   else
+   else if (cur) {
acm_start_wb(acm, cur);
+   }
 out:
spin_unlock_irqrestore(>write_lock, flags);
return;


<6>[   46.942493] cdc_acm 2-1:1.0: ttyACM0: USB ACM device
<6>[   46.942716] usbcore: registered new interface driver cdc_acm
<6>[   46.942718] cdc_acm: USB Abstract Control Model driver for USB modems and 
ISDN adapters
<6>[   46.946245] usb-storage 2-1:1.3: USB Mass Storage device detected
<1>[   46.946751] BUG: unable to handle kernel NULL pointer dereference at 
0018
<1>[   46.946790] IP: [] acm_start_wb+0x18/0xb0 [cdc_acm]
<4>[   46.946824] PGD 0
<4>[   46.946836] Oops:  [#1] SMP
<4>[   46.946855] Modules linked in: usb_storage(+) cdc_acm rfcomm ccm bnep 
uvcvideo videobuf2_vmalloc videobuf2_memops btusb videobuf2_v4l2 btintel 
videobuf2_core videodev bluetooth hid_multitouch media usbhid vboxpci(O) 
vboxnetadp(O) vboxnetflt(O) vboxdrv(O) arc4 joydev binfmt_misc nls_utf8 
nls_cp437 dell_wmi sparse_keymap x86_pkg_temp_thermal intel_powerclamp coretemp 
kvm_intel kvm iTCO_wdt irqbypass iTCO_vendor_support hid_rmi crct10dif_pclmul 
crc32_pclmul crc32c_intel ghash_clmulni_intel dell_laptop dell_smbios dcdbas 
dell_smm_hwmon snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic 
mac80211 aesni_intel aes_x86_64 glue_helper lrw ablk_helper cryptd psmouse 
cfg80211 serio_raw sg snd_hda_intel rfkill snd_hda_codec snd_hwdep snd_hda_core 
xhci_pci ehci_pci xhci_hcd snd_pcm ehci_hcd usbcore snd_timer snd lpc_ich 
usb_common soundcore mfd_core shpchp i2c_i801 thermal wmi battery i2c_hid hid 
acpi_als kfifo_buf industrialio sdhci_acpi sdhci mmc_core
i2c_designware_platform i2c_designware_core evdev intel_rst ac parport_pc ppdev 
lp parport [last unloaded: iwlwifi]
<4>[   46.947425] CPU: 2 PID: 84 Comm: kworker/u8:3 Tainted: G U O
4.6.0-rc2+ #1
<4>[   46.947461] Hardware name: Dell Inc. XPS13 9333/0HP75V, BIOS A07 
03/27/2015
<4>[   46.947496] Workqueue: events_unbound flush_to_ldisc
<4>[   46.947521] task: 88021541c100 ti: 8800d04c8000 task.ti: 
8800d04c8000
<4>[   46.947555] RIP: 0010:[]  [] 
acm_start_wb+0x18/0xb0 [cdc_acm]
<4>[   46.947598] RSP: 0018:8800d04cbd10  EFLAGS: 00010006
<4>[   46.947623] RAX:  RBX:  RCX: 
0004
<4>[   46.947655] RDX: 0001 RSI:  RDI: 
8800d056d000
<4>[   46.947688] RBP: 8800d056d000 R08: 0002 R09: 

<4>[   46.947720] R10: 0002 R11: 8800ac270040 R12: 
0246
<4>[   46.947753] R13:  R14:  R15: 
c9f832a8
<4>[   46.947786] FS:  () GS:88021f30() 
knlGS:
<4>[   46.947823] CS:  0010 DS:  ES:  CR0: 80050033
<4>[   46.947850] CR2: 0018 CR3: cfc99000 CR4: 
001406e0
<4>[   46.947882] Stack:
<4>[   46.947892]  0009 8800d056d000 8800d056d744 
a03318ee
<4>[   46.947932]  8801dac26c00 c9f81000 c9f81000 
0009
<4>[   46.947971]   814032c5 8802123cb420 
c9f81000
<4>[   46.948010] Call Trace:
<4>[   46.948024]  [] ? acm_tty_flush_chars+0x5e/0x90 
[cdc_acm]
<4>[   46.948057]  [] ? n_tty_receive_buf_common+0x665/0xb30
<4>[   46.948090]  [] ? pick_next_task_fair+0xf0/0x440
<4>[   46.948120]  [] ? flush_to_ldisc+0xbe/0x130
<4>[   46.948148]  [] ? process_one_work+0x164/0x480
<4>[   46.948176]  [] ? worker_thread+0x4a/0x4f0
<4>[   46.948203]  [] ? process_one_work+0x480/0x480
<4>[   46.948232]  [] ? kthread+0xbd/0xe0
<4>[   46.948258]  [] ? ret_from_fork+0x22/0x40
<4>[   46.948285]  [] ? kthread_create_on_node+0x180/0x180
<4>[   46.948315] Code: 40 09 c8 09 d0 c3 0f 1f 00 66 2e 0f 1f 84 00 00 00 00 
00 0f 1f 44 00 00 55 48 89 fd 53 48 89 f3 48 83 ec 08 83 87 40 07 00 00 01 <48> 
8b 46 18 48 8b 16 48 89 50 68 48 8b 46 18 48 8b 56 08 48 89
<1>[   46.948488] RIP  [] acm_start_wb+0x18/0xb0 [cdc_acm]
<4>[   46.948517]  RSP 
<4>[   46.948532] CR2: 0018
<4>[   46.959254] ---[ end trace 006fe18e3212a836 ]---


--
To 

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Greg KH]

On Wed, Apr 06, 2016 at 04:08:04PM +0300, Mathias Nyman wrote:
> We don't want to runtime suspend a bus if there is an event pending.
> The roothub on a NEC uPD720200 host with a single USB3 device connected
> might go back to runtime suspend immediately after runtime resume as
> hub might not yet see any port changes in resume.
> 
> Prevent this by checking if there is a unhandled event pending when
> calling runtime suspend.

As Alan points out, you didn't actually test this :(

--
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 v2 10/14] USB: ch341: fix coding style

[Johan Hovold]

On Wed, Apr 06, 2016 at 07:58:36PM +0200, Grigori Goronzy wrote:
> On 04/02/2016 07:29 PM, Joe Perches wrote:
> > Most of the whitespace only changes are undesired.
>
> Well, the style wasn't very consistent.  I think consistency is
> important.  So I took the liberty of deciding for one style and stuck
> to it.
> 
> > Multi-line statements here are using alignment to
> > open parenthesis which for some is the preferred
> > style.
> 
> I didn't use alignment to open parentheses because that is often
> reducing the usable space per line too much.  So you have to break lines
> a lot and code becomes less readable.
> 
> Of course, I'm open to arguments if and why a particular style should be
> preferred.  Maybe we should try to mostly avoid these bikeshed
> discussions though. :)

As Joe already said, we generally don't want indentation-only changes to
existing code. Just try to stick to the style of the driver (even if
it's inconsistent at times).

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 v2 14/14] USB: ch341: implement tx_empty callback

[Grigori Goronzy]

On 04/03/2016 06:03 PM, Karl Palsson wrote:
> 
> Grigori Goronzy  wrote:
>> The status bit was found with USB captures of the Windows
>> driver and some luck. Tested on CH340G and CH341A.
> 
> By my reversing, bit 0x4, is "multiple status changes since last
> interrupt"
>

Thanks, I can add the definition.  However, I wonder what this is
actually good for.  We don't actually need this to see that there are
multiple status changes.  We can just look at the different bits.

Grigori



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v2 10/14] USB: ch341: fix coding style

[Grigori Goronzy]

On 04/02/2016 07:29 PM, Joe Perches wrote:
> Most of the whitespace only changes are undesired.
> 

Well, the style wasn't very consistent.  I think consistency is
important.  So I took the liberty of deciding for one style and stuck to it.

> Multi-line statements here are using alignment to
> open parenthesis which for some is the preferred
> style.

I didn't use alignment to open parentheses because that is often
reducing the usable space per line too much.  So you have to break lines
a lot and code becomes less readable.

Of course, I'm open to arguments if and why a particular style should be
preferred.  Maybe we should try to mostly avoid these bikeshed
discussions though. :)

Grigori



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v10 4/9] phy: Add Tegra XUSB pad controller support

[Thierry Reding]

On Wed, Apr 06, 2016 at 06:13:42PM +0530, Kishon Vijay Abraham I wrote:
> On Friday 04 March 2016 09:49 PM, Thierry Reding wrote:
[...]
> > +struct tegra124_xusb_fuse_calibration {
> > +   u32 hs_curr_level[3];
> > +   u32 hs_iref_cap;
> > +   u32 hs_term_range_adj;
> > +   u32 hs_squelch_level;
> > +};
> 
> All these calibration data can come from dt and a generic PHY function to set
> these data to registers.

This calibration data is actually read from fuses within the chip. As I
understand it the process is that these values are characterized during
chip development and written to the fuses at the fab (or perhaps they
are characterized even as late as at the fab). There should be no need
to read these from DT.

> > +static const char * const tegra124_ulpi_functions[] = {
> > +   "snps",
> > +   "xusb",
> > +};
> > +
> > +static const struct tegra_xusb_lane_soc tegra124_ulpi_lanes[] = {
> > +   TEGRA124_LANE("ulpi-0", 0x004, 12, 0x1, ulpi),
> > +};
> > +
> > +static struct tegra_xusb_lane *
> > +tegra124_ulpi_lane_probe(struct tegra_xusb_pad *pad, struct device_node 
> > *np,
> > +unsigned int index)
> > +{
> > +   struct tegra_xusb_ulpi_lane *ulpi;
> > +   int err;
> > +
> > +   ulpi = kzalloc(sizeof(*ulpi), GFP_KERNEL);
> > +   if (!ulpi)
> > +   return ERR_PTR(-ENOMEM);
> > +
> > +   INIT_LIST_HEAD(>base.list);
> > +   ulpi->base.soc = >soc->lanes[index];
> > +   ulpi->base.index = index;
> > +   ulpi->base.pad = pad;
> > +   ulpi->base.np = np;
> > +
> 
> ulpi PHY's can be found dynamically right? Should this use the ulpi
> phy library?

I don't think that would work here. The registered accessed by this code
are all very Tegra specific as far as I can tell. I doubt that any kind
of generic library would work here.

Perhaps you can point me at the exact code you're thinking of. I only
found drivers/phy/ulpi_phy.h and drivers/usb/common/ulpi.c in a quick
search, neither of which seem to provide anything that would be useful
in this context. The former contains a couple of small helpers that I
don't think are appropriate here, whereas the latter seems to want the
driver to implement a ULPI interface, something which the Tegra XUSB pad
controller doesn't expose.

Thierry


signature.asc
Description: PGP signature

Re: [PATCH v10 3/9] dt-bindings: phy: tegra-xusb-padctl: Add Tegra210 support

[Thierry Reding]

On Tue, Apr 05, 2016 at 03:10:16PM -0600, Stephen Warren wrote:
> On 04/05/2016 08:44 AM, Thierry Reding wrote:
> > On Wed, Mar 16, 2016 at 11:59:44AM -0600, Stephen Warren wrote:
> > > On 03/04/2016 09:19 AM, Thierry Reding wrote:
> > > > From: Thierry Reding 
> > > > 
> > > > Extend the binding to cover the set of feature found in Tegra210.
> > > 
> > > Acked-by: Stephen Warren 
> > > 
> > > > diff --git 
> > > > a/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt 
> > > > b/Documentation/devicetree/bindings/phy/nvidia,tegra124-xusb-padctl.txt
> > > 
> > > > +   padctl@0,7009f000 {
> > > ...
> > > > +   pads {
> > > ...
> > > > +   };
> > > > +
> > > > +   ports {
> > > ...
> > > > +   };
> > > 
> > > As a comment not affecting my ack in any way: At the top-level, we place 
> > > all
> > > the child nodes into "array container" nodes named "pads" and "ports". 
> > > This
> > > is nice since it separates different types of child nodes and allows 
> > > easily
> > > adding more types of child nodes in the future without interference, and 
> > > in
> > > a way that allows us to explicitly know what each node is without having 
> > > to
> > > interpret its name or compatible value to do so. However, we haven't done
> > > this with the per-lane child nodes inside each pad. If we were to rev the
> > > design, I'd be tempted to suggest:
> > > 
> > >   padctl@0,7009f000 {
> > >   pads {
> > >   usb2 {
> > >   lanes { // This level is new
> > >   usb2-0 {
> > 
> > I tried to make this work, but it's unfortunately not possible with the
> > current code. The reason is that the PHY subsystem assumes that either
> > the provider DT node corresponds to that of the device (the usb2 pad in
> > the above example) or one of its children. Hence, putting everything
> > into one more level further down would require some mechanism to tell
> > the subsystem about it so that it can be found.
> 
> When the padctl driver registers the PHY objects with the PHY subsystem, can
> it pass the lanes node as the DT node? That woulud mean each lane /was/ a
> child of the node registered with the PHY subsystem.
> 
> Perhaps the PHY subsystem requires a struct device rather than a DT node
> registered with it?

Yes, that's how the PHY subsystem works. You pass it a struct device *
that it will wrap a struct phy_provider around. It will then use that
struct device's ->of_node as the parent for the child lookup.

> If so, does it make sense to create a separate struct device with the
> of_node pointing at lanes{}?

I suspect that that would work, but I'm already somewhat uncomfortable
about how many devices the driver creates. Adding more dummy devices
seems like a workaround.

> > Arguably the current support code isn't a good argument for designing a
> > binding, so perhaps it'd be useful to add this mechanism in order to get
> > a better binding. On the other hand, I'm not sure it's really worth it,
> > since we already have generic bindings that specify the layout of child
> > devices, and those have been agreed upon broadly (presumably).
> > 
> > In light of the recent discussion on DPAUX vs. I2C, I see how having the
> > extra level would be useful to provide additional context. If you think
> > it's worth it I can spend the extra time to get this implemented in the
> > core.
> 
> Naively, it sounds like it'd be a good idea to fix the PHY core. It really
> shouldn't care about the parent of any object registered with it; it should
> only interact with the specific object it was given, and any other data such
> as "ops" callbacks. Do you have any inkling how much work that would be?

I attached what I came up with. It extends the OF PHY provider registry
by allowing an additional node to be specified that if specified will
serve as the parent for the child lookup (and hence overrides the
default node that's taken from the struct device).

It is a fairly trivial patch, and you'll notice the bulk of the changes
is adding the additional parameter in a number of different places. The
only thing I'm not quite happy about is that we start needing to pass a
fairly large number of arguments. But perhaps it's still okay.

An alternative would be to make struct phy_provider embeddable into
driver private structures. That way they could be completely initialized
by a driver before being passed to the __of_phy_provider_register()
function (much like struct gpio_chip and others). That would be a fairly
intrusive change, one that I'd be willing to take on, though I'd like to
have Kishon's opinion on this before going ahead.

For reference, here's what I'm imagining:

struct foo_phy_provider {
struct phy_provider base;
...
};

int foo_probe(struct platform_device *pdev)
{
struct 

Re: [Linux-v4.6-rc2] atkbd serio0: Unknown key pressed (translated set 2, code 0xa8 on isa0060/serio0).

[Dmitry Torokhov]

On Wed, Apr 6, 2016 at 3:27 AM, Sedat Dilek  wrote:
> On Wed, Apr 6, 2016 at 11:35 AM, Sedat Dilek  wrote:
>> On Wed, Apr 6, 2016 at 11:27 AM, Sedat Dilek  wrote:
>>> Hi,
>>>
>>> I cannot use cursor up and down on my notebook and see this in my dmesg.
>>>
>>> [  685.425634] atkbd serio0: Unknown key pressed (translated set 2,
>>> code 0xa8 on isa0060/serio0).
>>> [  685.425648] atkbd serio0: Use 'setkeycodes e028 ' to make it 
>>> known.
>>>
>>> Known issue?
>>>
>>> Do you need more input?
>>>
>>> Linux-config and dmesg-output attached.
>>>
>>> Regards,
>>> - Sedat Dilek -
>>
>> Attached is output of...
>>
>>$ xmodmap -pke > /tmp/xmodmap-pke.txt
>>
>
> [ CC (USB)HID folks ]
>
> I forgot to tell that I have some patches on top of vanilla Linux v4.6-rc2.
>
> Dropping the patches from  makes
> the issue go away.
> Unsure what is the cause.

Hmm, this is quite curious, none of the recent patches in
for-4.6/upstream-fixes touches atkbd. I'd be very interested in which
one exactly causes this.

Thanks.

-- 
Dmitry
--
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 v9 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Wed, Apr 06, 2016 at 09:15:26AM +0800, Peter Chen wrote:

> > > No, this comment is common one, but only for SW detection. Eg, when
> > > the PMIC tells you it is a SDP, you can't notify to charger IC about
> > > 500mA at once, you need to do it after host allows you to do it.

> > Note that this isn't just the charger device that needs to constrain
> > current consumption - it's the entire system.  You can't charge to the
> > limit for system power draw if the USB controller is supplying the main
> > system rail.

> Sorry, I can't catch up you. USB Host (SDP or CDP) supplies power for
> USB device (not only USB controller, it is the whole system), it can
> supply more power after set configuration. See 1.4.13 from BC 1.2.

You're saying we need to notify the charger.  The charger does not in
general control the overall system current draw.


signature.asc
Description: PGP signature

RE: [PATCH 1/2] usb: xhci: ring: simplify count_trbs() a little bit

[David Laight]

From: Felipe Balbi
> Sent: 06 April 2016 14:25
> If we _know_ length is 0, we can just return 1 early.

Does that happen often enough to be an optimisation?
Looks like a mispredicted branch to me.

David

--
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/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Bjørn Mork]

Alan Stern  writes:
> On Wed, 6 Apr 2016, Bjørn Mork wrote:
>> Alan Stern  writes:
>> > On Wed, 6 Apr 2016, Mathias Nyman wrote:
>> >
>> >> We don't want to runtime suspend a bus if there is an event pending.
>> >> The roothub on a NEC uPD720200 host with a single USB3 device connected
>> >> might go back to runtime suspend immediately after runtime resume as
>> >> hub might not yet see any port changes in resume.
>> >> 
>> >> Prevent this by checking if there is a unhandled event pending when
>> >> calling runtime suspend.
>> >> 
>> >> Cc: 
>> >> Tested-by: Mike Murdoch 
>> >> Signed-off-by: Mathias Nyman 
>> >> ---
>> >>  drivers/usb/host/xhci-hub.c | 6 ++
>> >>  drivers/usb/host/xhci.c | 1 +
>> >>  2 files changed, 7 insertions(+)
>> >> 
>> >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> >> index d61fcc4..7e8999a 100644
>> >> --- a/drivers/usb/host/xhci-hub.c
>> >> +++ b/drivers/usb/host/xhci-hub.c
>> >> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>> >>   __le32 __iomem **port_array;
>> >>   struct xhci_bus_state *bus_state;
>> >>   unsigned long flags;
>> >> + u32 status;
>> >>  
>> >>   max_ports = xhci_get_ports(hcd, _array);
>> >>   bus_state = >bus_state[hcd_index(hcd)];
>> >>  
>> >>   spin_lock_irqsave(>lock, flags);
>> >>  
>> >> + /* Don't suspend root hub if there's an event pending. */
>> >> + status = readl(>op_regs->status);
>> >> + if (status & STS_EINT)
>> >> + return -EBUSY;
>> >
>> > Does anybody else see a problem here?
>> 
>> Like the EBUSY being propagated all the way to usb_suspend(), which is
>> called on PMSG_HIBERNATE, PMSG_FREEZE and PMSG_SUSPEND?
>> 
>> This will not only prevent runtime suspend, but also system suspend,
>> wont it?  Or did I miss something on the way?  The call chain between
>> the USB dev_pm_ops .suspend and xhci_bus_suspend() is quite long..
>
> I was thinking of returning with a private spinlock held.

No, I didn't see that. So I am happy to support Mathias' case by
demonstrating that it wasn't *that* bloody obvious :)


Bjørn
--
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/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Alan Stern]

On Wed, 6 Apr 2016, Bjørn Mork wrote:

> Alan Stern  writes:
> 
> > On Wed, 6 Apr 2016, Mathias Nyman wrote:
> >
> >> We don't want to runtime suspend a bus if there is an event pending.
> >> The roothub on a NEC uPD720200 host with a single USB3 device connected
> >> might go back to runtime suspend immediately after runtime resume as
> >> hub might not yet see any port changes in resume.
> >> 
> >> Prevent this by checking if there is a unhandled event pending when
> >> calling runtime suspend.
> >> 
> >> Cc: 
> >> Tested-by: Mike Murdoch 
> >> Signed-off-by: Mathias Nyman 
> >> ---
> >>  drivers/usb/host/xhci-hub.c | 6 ++
> >>  drivers/usb/host/xhci.c | 1 +
> >>  2 files changed, 7 insertions(+)
> >> 
> >> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> >> index d61fcc4..7e8999a 100644
> >> --- a/drivers/usb/host/xhci-hub.c
> >> +++ b/drivers/usb/host/xhci-hub.c
> >> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
> >>__le32 __iomem **port_array;
> >>struct xhci_bus_state *bus_state;
> >>unsigned long flags;
> >> +  u32 status;
> >>  
> >>max_ports = xhci_get_ports(hcd, _array);
> >>bus_state = >bus_state[hcd_index(hcd)];
> >>  
> >>spin_lock_irqsave(>lock, flags);
> >>  
> >> +  /* Don't suspend root hub if there's an event pending. */
> >> +  status = readl(>op_regs->status);
> >> +  if (status & STS_EINT)
> >> +  return -EBUSY;
> >
> > Does anybody else see a problem here?
> 
> Like the EBUSY being propagated all the way to usb_suspend(), which is
> called on PMSG_HIBERNATE, PMSG_FREEZE and PMSG_SUSPEND?
> 
> This will not only prevent runtime suspend, but also system suspend,
> wont it?  Or did I miss something on the way?  The call chain between
> the USB dev_pm_ops .suspend and xhci_bus_suspend() is quite long..

I was thinking of returning with a private spinlock held.

Alan Stern


--
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 v2 1/3] usb: dwc3: make dwc3_debugfs_init return value be void

[changbin . du]

From: "Du, Changbin" 

Debugfs init failure is not so important. We can continue our job on
this failure. Also no need to check debugfs_create_file call results.

Signed-off-by: Du, Changbin 
---
 drivers/usb/dwc3/core.c| 10 +-
 drivers/usb/dwc3/debug.h   |  6 +++---
 drivers/usb/dwc3/debugfs.c | 50 ++
 3 files changed, 14 insertions(+), 52 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 17fd814..30f825c 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1062,19 +1062,11 @@ static int dwc3_probe(struct platform_device *pdev)
if (ret)
goto err5;
 
-   ret = dwc3_debugfs_init(dwc);
-   if (ret) {
-   dev_err(dev, "failed to initialize debugfs\n");
-   goto err6;
-   }
-
+   dwc3_debugfs_init(dwc);
pm_runtime_allow(dev);
 
return 0;
 
-err6:
-   dwc3_core_exit_mode(dwc);
-
 err5:
dwc3_event_buffers_cleanup(dwc);
 
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index 07fbc2d..71e3180 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -217,11 +217,11 @@ static inline const char 
*dwc3_gadget_event_type_string(u8 event)
 void dwc3_trace(void (*trace)(struct va_format *), const char *fmt, ...);
 
 #ifdef CONFIG_DEBUG_FS
-extern int dwc3_debugfs_init(struct dwc3 *);
+extern void dwc3_debugfs_init(struct dwc3 *);
 extern void dwc3_debugfs_exit(struct dwc3 *);
 #else
-static inline int dwc3_debugfs_init(struct dwc3 *d)
-{  return 0;  }
+static inline void dwc3_debugfs_init(struct dwc3 *d)
+{  }
 static inline void dwc3_debugfs_exit(struct dwc3 *d)
 {  }
 #endif
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 9ac37fe..071b286 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -618,69 +618,39 @@ static const struct file_operations dwc3_link_state_fops 
= {
.release= single_release,
 };
 
-int dwc3_debugfs_init(struct dwc3 *dwc)
+void dwc3_debugfs_init(struct dwc3 *dwc)
 {
struct dentry   *root;
-   struct dentry   *file;
-   int ret;
 
root = debugfs_create_dir(dev_name(dwc->dev), NULL);
-   if (!root) {
-   ret = -ENOMEM;
-   goto err0;
-   }
+   if (IS_ERR_OR_NULL(root))
+   return;
 
dwc->root = root;
 
dwc->regset = kzalloc(sizeof(*dwc->regset), GFP_KERNEL);
if (!dwc->regset) {
-   ret = -ENOMEM;
-   goto err1;
+   debugfs_remove_recursive(root);
+   return;
}
 
dwc->regset->regs = dwc3_regs;
dwc->regset->nregs = ARRAY_SIZE(dwc3_regs);
dwc->regset->base = dwc->regs;
 
-   file = debugfs_create_regset32("regdump", S_IRUGO, root, dwc->regset);
-   if (!file) {
-   ret = -ENOMEM;
-   goto err1;
-   }
+   debugfs_create_regset32("regdump", S_IRUGO, root, dwc->regset);
 
-   if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)) {
-   file = debugfs_create_file("mode", S_IRUGO | S_IWUSR, root,
+   if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE))
+   debugfs_create_file("mode", S_IRUGO | S_IWUSR, root,
dwc, _mode_fops);
-   if (!file) {
-   ret = -ENOMEM;
-   goto err1;
-   }
-   }
 
if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE) ||
IS_ENABLED(CONFIG_USB_DWC3_GADGET)) {
-   file = debugfs_create_file("testmode", S_IRUGO | S_IWUSR, root,
+   debugfs_create_file("testmode", S_IRUGO | S_IWUSR, root,
dwc, _testmode_fops);
-   if (!file) {
-   ret = -ENOMEM;
-   goto err1;
-   }
-
-   file = debugfs_create_file("link_state", S_IRUGO | S_IWUSR, 
root,
+   debugfs_create_file("link_state", S_IRUGO | S_IWUSR, root,
dwc, _link_state_fops);
-   if (!file) {
-   ret = -ENOMEM;
-   goto err1;
-   }
}
-
-   return 0;
-
-err1:
-   debugfs_remove_recursive(root);
-
-err0:
-   return ret;
 }
 
 void dwc3_debugfs_exit(struct dwc3 *dwc)
-- 
2.5.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 v2 3/3] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[changbin . du]

From: "Du, Changbin" 

For DWC3 USB controller, the Global Debug Queue/FIFO Space Available
Register(GDBGFIFOSPACE) can be used to dump FIFO/Queue available space.
This can be used to check some special issues, like whether data is
successfully copied from memory to fifo when a trb is blocked.

Signed-off-by: Du, Changbin 
---
 drivers/usb/dwc3/core.h|  5 +
 drivers/usb/dwc3/debugfs.c | 40 
 2 files changed, 45 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 6254b2f..899cf76 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -348,6 +348,11 @@
 #define DWC3_DSTS_LOWSPEED (2 << 0)
 #define DWC3_DSTS_FULLSPEED1   (3 << 0)
 
+/* Global Debug Queue/FIFO Space Available Register */
+#define DWC3_GDBGFIFOSPACE_NUM(x)  (((x) << 0) & 0x1F)
+#define DWC3_GDBGFIFOSPACE_TYPE(x) (((x) << 5) & 0xE0)
+#define DWC3_GDBGFIFOSPACE_GET_SPACE(x)(((x) >> 16) & 0x)
+
 /* Device Generic Command Register */
 #define DWC3_DGCMD_SET_LMP 0x01
 #define DWC3_DGCMD_SET_PERIODIC_PAR0x02
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 2d4f397..bb608e0 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -426,6 +426,45 @@ static const struct file_operations dwc3_mode_fops = {
.release= single_release,
 };
 
+static int dwc3_fifo_show(struct seq_file *s, void *unused)
+{
+   struct dwc3 *dwc = s->private;
+   unsigned long   flags;
+   unsigned inttype, index;
+   const char  *name;
+   u32 reg;
+
+   static const char * const fifo_names[] = {
+   "TxFIFO", "RxFIFO", "TxReqQ", "RxReqQ", "RxInfoQ",
+   "DescFetchQ", "EventQ", "ProtocolStatusQ"};
+   spin_lock_irqsave(>lock, flags);
+   for (type = 0; type < 8; type++) {
+   name = fifo_names[type];
+   for (index = 0; index < 32; index++) {
+   dwc3_writel(dwc->regs, DWC3_GDBGFIFOSPACE,
+   DWC3_GDBGFIFOSPACE_NUM(index) |
+   DWC3_GDBGFIFOSPACE_TYPE(type));
+   reg = dwc3_readl(dwc->regs, DWC3_GDBGFIFOSPACE);
+   seq_printf(s, "%s%02d = %d\n", name, index,
+   DWC3_GDBGFIFOSPACE_GET_SPACE(reg));
+   }
+   }
+   spin_unlock_irqrestore(>lock, flags);
+   return 0;
+}
+
+static int dwc3_fifo_open(struct inode *inode, struct file *file)
+{
+   return single_open(file, dwc3_fifo_show, inode->i_private);
+}
+
+static const struct file_operations dwc3_fifo_fops = {
+   .open   = dwc3_fifo_open,
+   .read   = seq_read,
+   .llseek = seq_lseek,
+   .release= single_release,
+};
+
 static int dwc3_testmode_show(struct seq_file *s, void *unused)
 {
struct dwc3 *dwc = s->private;
@@ -639,6 +678,7 @@ void dwc3_debugfs_init(struct dwc3 *dwc)
dwc->regset->base = dwc->regs;
 
debugfs_create_regset32("regdump", S_IRUGO, root, dwc->regset);
+   debugfs_create_file("fifo", S_IRUGO, root, dwc, _fifo_fops);
 
if (IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE))
debugfs_create_file("mode", S_IRUGO | S_IWUSR, root,
-- 
2.5.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 v2 2/3] usb: dwc3: free dwc->regset on dwc3_debugfs_exit

[changbin . du]

From: "Du, Changbin" 

Signed-off-by: Du, Changbin 
---
 drivers/usb/dwc3/debugfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 071b286..2d4f397 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -657,4 +657,7 @@ void dwc3_debugfs_exit(struct dwc3 *dwc)
 {
debugfs_remove_recursive(dwc->root);
dwc->root = NULL;
+
+   kfree(dwc->regset);
+   dwc->regset = NULL;
 }
-- 
2.5.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] option.c: Support for Gemalto's Cinterion PH8 and AHxx products added

[Schemmel Hans-Christoph]

Added support for Gemalto's Cinterion PH8 and AHxx products
with 2 RmNet Interfaces and products with 1 RmNet + 1 USB Audio interface.

The RmNet and USB Audio interfaces are blacklisted because they will be
handled by other drivers.

In addition some minor renaming and formatting.

Signed-off-by: Hans-Christoph Schemmel 
---
 drivers/usb/serial/option.c | 39 ---
 1 file changed, 32 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index 348e198..b1e7820 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -375,18 +375,22 @@ static void option_instat_callback(struct urb *urb);
 #define HAIER_PRODUCT_CE81B0x10f8
 #define HAIER_PRODUCT_CE1000x2009
 
-/* Cinterion (formerly Siemens) products */
-#define SIEMENS_VENDOR_ID  0x0681
-#define CINTERION_VENDOR_ID0x1e2d
+/* Gemalto's Cinterion products (formerly Siemens)*/
+#define SIEMENS_VENDOR_ID  0x0681
+#define CINTERION_VENDOR_ID0x1e2d
 #define CINTERION_PRODUCT_HC25_MDM 0x0047
-#define CINTERION_PRODUCT_HC25_MDMNET  0x0040
+#define CINTERION_PRODUCT_HC25_MDMNET  0x0040
 #define CINTERION_PRODUCT_HC28_MDM 0x004C
-#define CINTERION_PRODUCT_HC28_MDMNET  0x004A /* same for HC28J */
+#define CINTERION_PRODUCT_HC28_MDMNET  0x004A /* same for HC28J */
 #define CINTERION_PRODUCT_EU3_E0x0051
 #define CINTERION_PRODUCT_EU3_P0x0052
 #define CINTERION_PRODUCT_PH8  0x0053
 #define CINTERION_PRODUCT_AHXX 0x0055
 #define CINTERION_PRODUCT_PLXX 0x0060
+#define CINTERION_PRODUCT_PH8_2RMNET   0x0082
+#define CINTERION_PRODUCT_PH8_AUDIO0x0083
+#define CINTERION_PRODUCT_AHXX_2RMNET  0x0084
+#define CINTERION_PRODUCT_AHXX_AUDIO   0x0085
 
 /* Olivetti products */
 #define OLIVETTI_VENDOR_ID 0x0b3c
@@ -600,6 +604,18 @@ static const struct option_blacklist_info 
net_intf6_blacklist = {
.reserved = BIT(6),
 };
 
+static const struct option_blacklist_info cinterion_rmnet1_blacklist = {
+   .reserved = BIT(4) | BIT(5),
+};
+
+static const struct option_blacklist_info cinterion_rmnet2_blacklist = {
+   .reserved = BIT(4) | BIT(5) | BIT(6) | BIT(7),
+};
+
+static const struct option_blacklist_info cinterion_audio_blacklist = {
+   .reserved = BIT(4) | BIT(5) | BIT(6) | BIT(7) | BIT(8),
+};
+
 static const struct option_blacklist_info zte_mf626_blacklist = {
.sendsetup = BIT(0) | BIT(1),
.reserved = BIT(4),
@@ -1708,8 +1724,17 @@ static const struct usb_device_id option_ids[] = {
{ USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_EU3_E) },
{ USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_EU3_P) },
{ USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_PH8),
-   .driver_info = (kernel_ulong_t)_intf4_blacklist },
-   { USB_DEVICE_INTERFACE_CLASS(CINTERION_VENDOR_ID, 
CINTERION_PRODUCT_AHXX, 0xff) },
+   .driver_info = (kernel_ulong_t)_rmnet1_blacklist },
+   { USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_AHXX),
+   .driver_info = (kernel_ulong_t)_rmnet1_blacklist },
+   { USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_PH8_2RMNET),
+   .driver_info = (kernel_ulong_t)_rmnet2_blacklist },
+   { USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_PH8_AUDIO),
+   .driver_info = (kernel_ulong_t)_audio_blacklist },
+   { USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_AHXX_2RMNET),
+   .driver_info = (kernel_ulong_t)_rmnet2_blacklist },
+   { USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_AHXX_AUDIO),
+   .driver_info = (kernel_ulong_t)_audio_blacklist },
{ USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_PLXX),
.driver_info = (kernel_ulong_t)_intf4_blacklist },
{ USB_DEVICE(CINTERION_VENDOR_ID, CINTERION_PRODUCT_HC28_MDM) }, 



smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 3/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Bjørn Mork]

Alan Stern  writes:

> On Wed, 6 Apr 2016, Mathias Nyman wrote:
>
>> We don't want to runtime suspend a bus if there is an event pending.
>> The roothub on a NEC uPD720200 host with a single USB3 device connected
>> might go back to runtime suspend immediately after runtime resume as
>> hub might not yet see any port changes in resume.
>> 
>> Prevent this by checking if there is a unhandled event pending when
>> calling runtime suspend.
>> 
>> Cc: 
>> Tested-by: Mike Murdoch 
>> Signed-off-by: Mathias Nyman 
>> ---
>>  drivers/usb/host/xhci-hub.c | 6 ++
>>  drivers/usb/host/xhci.c | 1 +
>>  2 files changed, 7 insertions(+)
>> 
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index d61fcc4..7e8999a 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>>  __le32 __iomem **port_array;
>>  struct xhci_bus_state *bus_state;
>>  unsigned long flags;
>> +u32 status;
>>  
>>  max_ports = xhci_get_ports(hcd, _array);
>>  bus_state = >bus_state[hcd_index(hcd)];
>>  
>>  spin_lock_irqsave(>lock, flags);
>>  
>> +/* Don't suspend root hub if there's an event pending. */
>> +status = readl(>op_regs->status);
>> +if (status & STS_EINT)
>> +return -EBUSY;
>
> Does anybody else see a problem here?

Like the EBUSY being propagated all the way to usb_suspend(), which is
called on PMSG_HIBERNATE, PMSG_FREEZE and PMSG_SUSPEND?

This will not only prevent runtime suspend, but also system suspend,
wont it?  Or did I miss something on the way?  The call chain between
the USB dev_pm_ops .suspend and xhci_bus_suspend() is quite long..


Bjørn
--
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] usb: f_mass_storage: test whether thread is running before starting another

[Alan Stern]

On Wed, 6 Apr 2016, Michal Nazarewicz wrote:

> On Tue, Apr 05 2016, Alan Stern wrote:
> > Suppose one usb_function is carrying out an I/O operation while
> > another one in the same config gets a Set-Interface request from the
> > host.
> 
> That cannot happen.  A single instance of mass_storage cannot¹ be added
> twice to the same configuration.
> 
> ¹ To be more precise, not via configfs.  A legacy gadget could do that,
>   but that would be a bug in that legacy driver, not f_mass_storage.
>   Moreover, no current legacy gadgets do that though so IMO this is an
>   academic discussion.

Okay.  Then I suggest adding this explanation to the patch description.

BTW, is configfs capable of adding a single instance twice in different 
configs?  Or is that again something only legacy gadgets can do?

Alan Stern

--
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/7] xhci: Don't suspend a xhci usb bus if there is a pending event.

[Alan Stern]

On Wed, 6 Apr 2016, Mathias Nyman wrote:

> We don't want to runtime suspend a bus if there is an event pending.
> The roothub on a NEC uPD720200 host with a single USB3 device connected
> might go back to runtime suspend immediately after runtime resume as
> hub might not yet see any port changes in resume.
> 
> Prevent this by checking if there is a unhandled event pending when
> calling runtime suspend.
> 
> Cc: 
> Tested-by: Mike Murdoch 
> Signed-off-by: Mathias Nyman 
> ---
>  drivers/usb/host/xhci-hub.c | 6 ++
>  drivers/usb/host/xhci.c | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index d61fcc4..7e8999a 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1289,12 +1289,18 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
>   __le32 __iomem **port_array;
>   struct xhci_bus_state *bus_state;
>   unsigned long flags;
> + u32 status;
>  
>   max_ports = xhci_get_ports(hcd, _array);
>   bus_state = >bus_state[hcd_index(hcd)];
>  
>   spin_lock_irqsave(>lock, flags);
>  
> + /* Don't suspend root hub if there's an event pending. */
> + status = readl(>op_regs->status);
> + if (status & STS_EINT)
> + return -EBUSY;

Does anybody else see a problem here?

Alan Stern

--
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] net: cdc_ncm: update datagram size after changing mtu

[Bjørn Mork]

Robert Dobrowolski  writes:

> From: Rafal Redzimski 
>
> Current implementation updates the mtu size and notify cdc_ncm
> device using USB_CDC_SET_MAX_DATAGRAM_SIZE request about datagram
> size change instead of changing rx_urb_size.
>
> Whenever mtu is being changed, datagram size should also be
> updated.

Definitely!  Thanks for this.  But looking at the code I believe you
need to fix the calculation of maxmtu too.  It is currently:

  int maxmtu = ctx->max_datagram_size - cdc_ncm_eth_hlen(dev);

And cdc_ncm_set_dgram_size() updates ctx->max_datagram_size with the new
mtu, meaning that you can only reduce the mtu.  We should probably use
cdc_ncm_max_dgram_size() instead here.

And cdc_ncm_set_dgram_size() takes the datagram size with header as
input (ref the above maxmtu calucalution), so it probably needs to
called as

  cdc_ncm_set_dgram_size(dev, new_mtu + cdc_ncm_eth_hlen(dev));

to get it right.  I think.  None of this is tested on an actual device
yet...  Care to test if I'm right, and do a v2 if necessry?

> Cc: 

This should be dropped for net.  Ask David to queue it for stable
instead.  I usually do that by using a subject prefix like

 [PATCH net,stable v1] ...




Bjørn
--
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 v2 2/2] phy: Group vendor specific phy drivers

[Vivek Gautam]

Adding vendor specific directories in phy to group
phy drivers under their respective vendor umbrella.

Also updated the MAINTAINERS file to reflect the correct
directory structure for phy drivers.

Signed-off-by: Vivek Gautam 
Acked-by: Heiko Stuebner 
Acked-by: Viresh Kumar 
---

Changes from v1:
 - Updated the MAINTAINERS file to reflect the same change
   in directory structure.
 - Removed PHY_PLAT config as pointed out by Kishon.
   So we don't require the second patch in the v1 of this series:
   [PATCH 2/2] arm: mach-spear: Enable PHY_PLAT to meet dependency
 - Renamed sunxi --> allwinner; rcar --> renesas.
 - Fixed error coming with qcom Makefile.

 MAINTAINERS   |  22 +-
 drivers/phy/Kconfig   | 383 +-
 drivers/phy/Makefile  |  57 +---
 drivers/phy/allwinner/Kconfig |  28 ++
 drivers/phy/allwinner/Makefile|   2 +
 drivers/phy/{ => allwinner}/phy-sun4i-usb.c   |   0
 drivers/phy/{ => allwinner}/phy-sun9i-usb.c   |   0
 drivers/phy/bcom/Kconfig  |  27 ++
 drivers/phy/bcom/Makefile |   3 +
 drivers/phy/{ => bcom}/phy-bcm-cygnus-pcie.c  |   0
 drivers/phy/{ => bcom}/phy-bcm-kona-usb2.c|   0
 drivers/phy/{ => bcom}/phy-brcmstb-sata.c |   0
 drivers/phy/hisi/Kconfig  |  20 ++
 drivers/phy/hisi/Makefile |   2 +
 drivers/phy/{ => hisi}/phy-hi6220-usb.c   |   0
 drivers/phy/{ => hisi}/phy-hix5hd2-sata.c |   0
 drivers/phy/marvell/Kconfig   |  50 +++
 drivers/phy/marvell/Makefile  |   6 +
 drivers/phy/{ => marvell}/phy-armada375-usb2.c|   0
 drivers/phy/{ => marvell}/phy-berlin-sata.c   |   0
 drivers/phy/{ => marvell}/phy-berlin-usb.c|   0
 drivers/phy/{ => marvell}/phy-mvebu-sata.c|   0
 drivers/phy/{ => marvell}/phy-pxa-28nm-hsic.c |   0
 drivers/phy/{ => marvell}/phy-pxa-28nm-usb2.c |   0
 drivers/phy/qcom/Kconfig  |  23 ++
 drivers/phy/qcom/Makefile |   5 +
 drivers/phy/{ => qcom}/phy-qcom-apq8064-sata.c|   0
 drivers/phy/{ => qcom}/phy-qcom-ipq806x-sata.c|   0
 drivers/phy/{ => qcom}/phy-qcom-ufs-i.h   |   0
 drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-14nm.c|   0
 drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-14nm.h|   0
 drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-20nm.c|   0
 drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-20nm.h|   0
 drivers/phy/{ => qcom}/phy-qcom-ufs.c |   0
 drivers/phy/renesas/Kconfig   |  16 +
 drivers/phy/renesas/Makefile  |   2 +
 drivers/phy/{ => renesas}/phy-rcar-gen2.c |   0
 drivers/phy/{ => renesas}/phy-rcar-gen3-usb2.c|   0
 drivers/phy/rockchip/Kconfig  |  23 ++
 drivers/phy/rockchip/Makefile |   3 +
 drivers/phy/{ => rockchip}/phy-rockchip-dp.c  |   0
 drivers/phy/{ => rockchip}/phy-rockchip-emmc.c|   0
 drivers/phy/{ => rockchip}/phy-rockchip-usb.c |   0
 drivers/phy/samsung/Kconfig   |  87 +
 drivers/phy/samsung/Makefile  |  10 +
 drivers/phy/{ => samsung}/phy-exynos-dp-video.c   |   0
 drivers/phy/{ => samsung}/phy-exynos-mipi-video.c |   0
 drivers/phy/{ => samsung}/phy-exynos4210-usb2.c   |   0
 drivers/phy/{ => samsung}/phy-exynos4x12-usb2.c   |   0
 drivers/phy/{ => samsung}/phy-exynos5-usbdrd.c|   0
 drivers/phy/{ => samsung}/phy-exynos5250-sata.c   |   0
 drivers/phy/{ => samsung}/phy-exynos5250-usb2.c   |   0
 drivers/phy/{ => samsung}/phy-s5pv210-usb2.c  |   0
 drivers/phy/{ => samsung}/phy-samsung-usb2.c  |   0
 drivers/phy/{ => samsung}/phy-samsung-usb2.h  |   0
 drivers/phy/st/Kconfig|  51 +++
 drivers/phy/st/Makefile   |   6 +
 drivers/phy/{ => st}/phy-miphy28lp.c  |   0
 drivers/phy/{ => st}/phy-miphy365x.c  |   0
 drivers/phy/{ => st}/phy-spear1310-miphy.c|   0
 drivers/phy/{ => st}/phy-spear1340-miphy.c|   0
 drivers/phy/{ => st}/phy-stih407-usb.c|   0
 drivers/phy/{ => st}/phy-stih41x-usb.c|   0
 drivers/phy/ti/Kconfig|  67 
 drivers/phy/ti/Makefile   |   6 +
 drivers/phy/{ => ti}/phy-dm816x-usb.c |   0
 drivers/phy/{ => ti}/phy-omap-control.c   |   0
 drivers/phy/{ => ti}/phy-omap-usb2.c  |   0
 drivers/phy/{ => ti}/phy-ti-pipe3.c   |   0
 drivers/phy/{ => ti}/phy-tusb1210.c   |   0
 drivers/phy/{ => ti}/phy-twl4030-usb.c|   0
 71 files changed, 472 insertions(+), 427 deletions(-)
 create mode 100644 drivers/phy/allwinner/Kconfig
 create mode 100644 

[PATCH 1/2] phy: Move ULPI phy header out of driver to includes path

[Vivek Gautam]

Although ULPI phy is currently being used by tusb1210,
there can be other consumers too in future. So move this
to the includes path for phy.

Signed-off-by: Vivek Gautam 
---
 {drivers => include/linux}/phy/ulpi_phy.h | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename {drivers => include/linux}/phy/ulpi_phy.h (100%)

diff --git a/drivers/phy/ulpi_phy.h b/include/linux/phy/ulpi_phy.h
similarity index 100%
rename from drivers/phy/ulpi_phy.h
rename to include/linux/phy/ulpi_phy.h
-- 
1.9.1

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

DWC3: Clock Domain Crossing erratum description.

[Kirill Dronov]

Hi.
I'm working on USB device bringup on Intel E3800 – based board. DWC3 
core configured as DRD in device mode. The only connected device phy is 
SMSC 3310 (USB2 ULPI). DWC3 core version is 2.10A. Gadget zero driver 
can be loaded, but device enumeration fails: device is detected by host, 
speed is negotiated (HS), host successfully reset device. On device side 
– conndone interrupt is followed by linksts change with link_state 0. 
Then host sends USBREQ_GET_DESCRIPTOR and tries to set address but 
device does not react – no events generated.
I'm not sure if I hit “run/stop metastability” issue [“STAR#9000525659: 
Clock Domain Crossing on DCTL in USB 2.0 Mode”]. I don't have DWC3 
databook or erratum description (other than mentioned in driver code). 
Can somebody provide more detailed description of STAR#9000525659?

Where can I get DWC3 Databook?
–
Thanks in advance,
Kirill
--
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 v9 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Jun Li  writes:
>> >> >> > Since we already have get_charger_type callback at usb_charger
>> >> >> > structure, why we still need this API at usb_gadget_ops?
>> >> >>
>> >> >> In case some users want to get charger type at gadget level.
>> >> >>
>> >> > Why gadget needs to know charger type? I also don't catch the
>> >> > intent of
>> >>
>> >> because some gadgets need to call usb_gadget_vbus_draw(), although
>> >> for that they need power in mA rather.
>> >
>> > In below change of usb_gadget_vbus_draw(), already can get charger
>> > type via usb_charger_get_type().
>> >
>> > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget,
>> > unsigned mA)  {
>> > +   enum usb_charger_type type;
>> > +
>> > +   if (gadget->charger) {
>> > +   type = usb_charger_get_type(gadget->charger);
>> > +   usb_charger_set_cur_limit_by_type(gadget->charger, type,
>> mA);
>> > +   }
>> > +
>> > if (!gadget->ops->vbus_draw)
>> > return -EOPNOTSUPP;
>> > return gadget->ops->vbus_draw(gadget, mA);
>> >
>> > Could you detail in what situation gadget->ops-> get_charger_type() is
>> used?
>> 
>> isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling
>> it ? What did I miss here ?
>
> Well, that's true, so my real meaning is why gadget need get charger type
> via another new api gadget->ops->get_charger_type().

because of semantics. usb_gadget_vbus_draw() is *only* supposed to
connect a load across vbus and gnd so some battery can be charged. Also,
we need to abstract away this ->get_charger_type() operation because it
might be different for each UDC.

$subject has a fragility, however: It's assuming that we should always
call ->get_charger_type() before ->vbus_draw(), but that's a good
default, I'd say.

>> >> > This api, as my understanding, gadget only need report gadget state
>> >> changes.
>> >> > All information required for usb charger is charger type and gadget
>> >> state.
>> >>
>> >> you're making an assumption about how the HW is laid out which might
>> >> not be true.
>> >>
>> >
>> > What other information you refer to here? Or what I am not aware of?
>> 
>> what I'm trying to say is that you're assuming gadgets don't need to know
>> anything other than charger type and gadget state (suspended, resume,
>> enumerated, default state, addressed, etc), but that might not be true for
>> all UDCs. You can't make that assumption that charger type and gadget
>> state is enough. The real question is what do we need *now*, but still
>> keep in mind that what we need *now* might be valid 2 years from now, so
>> API needs to be a little flexible.
>
> Get your point, flexible, I just thought create an api without any user
> for existing case/spec, wouldn't it be better to let the real user add it
> later when it's needed.

that sure is a fair point.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Jun Li]

Hi

> -Original Message-
> From: Felipe Balbi [mailto:ba...@kernel.org]
> Sent: Wednesday, April 06, 2016 8:56 PM
> To: Jun Li ; Baolin Wang ; Peter
> Chen 
> Cc: Greg KH ; Sebastian Reichel
> ; Dmitry Eremin-Solenikov ; David
> Woodhouse ; Peter Chen ;
> Alan Stern ; r.bald...@samsung.com; Yoshihiro
> Shimoda ; Lee Jones
> ; Mark Brown ; Charles Keepax
> ; patc...@opensource.wolfsonmicro.com;
> Linux PM list ; USB ;
> device-mainlin...@lists.linuxfoundation.org; LKML  ker...@vger.kernel.org>
> Subject: RE: [PATCH v9 2/4] gadget: Support for the usb charger framework
> 
> 
> Hi,
> 
> Jun Li  writes:
> >> >> On 6 April 2016 at 15:19, Peter Chen  wrote:
> >> >> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote:
> >> >> >>
> >> >> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops {
> >> >> >>   struct usb_ep *(*match_ep)(struct usb_gadget *,
> >> >> >>   struct usb_endpoint_descriptor *,
> >> >> >>   struct usb_ss_ep_comp_descriptor *);
> >> >> >> + /* get the charger type */
> >> >> >> + enum usb_charger_type (*get_charger_type)(struct
> >> >> >> + usb_gadget *);
> >> >> >>  };
> >> >> >
> >> >> > Since we already have get_charger_type callback at usb_charger
> >> >> > structure, why we still need this API at usb_gadget_ops?
> >> >>
> >> >> In case some users want to get charger type at gadget level.
> >> >>
> >> > Why gadget needs to know charger type? I also don't catch the
> >> > intent of
> >>
> >> because some gadgets need to call usb_gadget_vbus_draw(), although
> >> for that they need power in mA rather.
> >
> > In below change of usb_gadget_vbus_draw(), already can get charger
> > type via usb_charger_get_type().
> >
> > static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget,
> > unsigned mA)  {
> > +   enum usb_charger_type type;
> > +
> > +   if (gadget->charger) {
> > +   type = usb_charger_get_type(gadget->charger);
> > +   usb_charger_set_cur_limit_by_type(gadget->charger, type,
> mA);
> > +   }
> > +
> > if (!gadget->ops->vbus_draw)
> > return -EOPNOTSUPP;
> > return gadget->ops->vbus_draw(gadget, mA);
> >
> > Could you detail in what situation gadget->ops-> get_charger_type() is
> used?
> 
> isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling
> it ? What did I miss here ?

Well, that's true, so my real meaning is why gadget need get charger type
via another new api gadget->ops->get_charger_type().
 
> 
> >> > This api, as my understanding, gadget only need report gadget state
> >> changes.
> >> > All information required for usb charger is charger type and gadget
> >> state.
> >>
> >> you're making an assumption about how the HW is laid out which might
> >> not be true.
> >>
> >
> > What other information you refer to here? Or what I am not aware of?
> 
> what I'm trying to say is that you're assuming gadgets don't need to know
> anything other than charger type and gadget state (suspended, resume,
> enumerated, default state, addressed, etc), but that might not be true for
> all UDCs. You can't make that assumption that charger type and gadget
> state is enough. The real question is what do we need *now*, but still
> keep in mind that what we need *now* might be valid 2 years from now, so
> API needs to be a little flexible.

Get your point, flexible, I just thought create an api without any user
for existing case/spec, wouldn't it be better to let the real user add it
later when it's needed.

> 
> cheers
> 
> --
> balbi
--
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 1/2] phy: Group vendor specific phy drivers

[Vivek Gautam]

Hi Kishon,


On Wed, Apr 6, 2016 at 5:51 AM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Friday 01 April 2016 07:05 PM, Vivek Gautam wrote:
>> Hi,
>>
>>
>> On Fri, Apr 1, 2016 at 6:05 AM, Kishon Vijay Abraham I  wrote:
>>> Hi,
>>>
>>> On Friday 01 April 2016 04:59 PM, Vivek Gautam wrote:
 Adding vendor specific directories in phy to group
 phy drivers under their respective vendor umbrella.

 Signed-off-by: Vivek Gautam 
 ---

 With growing number of phy drivers, it makes sense to
 group these drivers under their respective vendor/platform
 umbrella directory.

 Build-tested 'multi_v7_defconfig'.

  drivers/phy/Kconfig   | 386 
 +-
  drivers/phy/Makefile  |  57 +---
  drivers/phy/bcom/Kconfig  |  27 ++
  drivers/phy/bcom/Makefile |   3 +
  drivers/phy/{ => bcom}/phy-bcm-cygnus-pcie.c  |   0
  drivers/phy/{ => bcom}/phy-bcm-kona-usb2.c|   0
  drivers/phy/{ => bcom}/phy-brcmstb-sata.c |   0
  drivers/phy/hisi/Kconfig  |  20 ++
  drivers/phy/hisi/Makefile |   2 +
  drivers/phy/{ => hisi}/phy-hi6220-usb.c   |   0
  drivers/phy/{ => hisi}/phy-hix5hd2-sata.c |   0
  drivers/phy/marvell/Kconfig   |  50 +++
  drivers/phy/marvell/Makefile  |   6 +
  drivers/phy/{ => marvell}/phy-armada375-usb2.c|   0
  drivers/phy/{ => marvell}/phy-berlin-sata.c   |   0
  drivers/phy/{ => marvell}/phy-berlin-usb.c|   0
  drivers/phy/{ => marvell}/phy-mvebu-sata.c|   0
  drivers/phy/{ => marvell}/phy-pxa-28nm-hsic.c |   0
  drivers/phy/{ => marvell}/phy-pxa-28nm-usb2.c |   0
  drivers/phy/qcom/Kconfig  |  23 ++
  drivers/phy/qcom/Makefile |   5 +
  drivers/phy/{ => qcom}/phy-qcom-apq8064-sata.c|   0
  drivers/phy/{ => qcom}/phy-qcom-ipq806x-sata.c|   0
  drivers/phy/{ => qcom}/phy-qcom-ufs-i.h   |   0
  drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-14nm.c|   0
  drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-14nm.h|   0
  drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-20nm.c|   0
  drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-20nm.h|   0
  drivers/phy/{ => qcom}/phy-qcom-ufs.c |   0
  drivers/phy/rcar/Kconfig  |  16 +
  drivers/phy/rcar/Makefile |   2 +
  drivers/phy/{ => rcar}/phy-rcar-gen2.c|   0
  drivers/phy/{ => rcar}/phy-rcar-gen3-usb2.c   |   0
  drivers/phy/rockchip/Kconfig  |  23 ++
  drivers/phy/rockchip/Makefile |   3 +
  drivers/phy/{ => rockchip}/phy-rockchip-dp.c  |   0
  drivers/phy/{ => rockchip}/phy-rockchip-emmc.c|   0
  drivers/phy/{ => rockchip}/phy-rockchip-usb.c |   0
  drivers/phy/samsung/Kconfig   |  87 +
  drivers/phy/samsung/Makefile  |  10 +
  drivers/phy/{ => samsung}/phy-exynos-dp-video.c   |   0
  drivers/phy/{ => samsung}/phy-exynos-mipi-video.c |   0
  drivers/phy/{ => samsung}/phy-exynos4210-usb2.c   |   0
  drivers/phy/{ => samsung}/phy-exynos4x12-usb2.c   |   0
  drivers/phy/{ => samsung}/phy-exynos5-usbdrd.c|   0
  drivers/phy/{ => samsung}/phy-exynos5250-sata.c   |   0
  drivers/phy/{ => samsung}/phy-exynos5250-usb2.c   |   0
  drivers/phy/{ => samsung}/phy-s5pv210-usb2.c  |   0
  drivers/phy/{ => samsung}/phy-samsung-usb2.c  |   0
  drivers/phy/{ => samsung}/phy-samsung-usb2.h  |   0
  drivers/phy/st/Kconfig|  51 +++
  drivers/phy/st/Makefile   |   6 +
  drivers/phy/{ => st}/phy-miphy28lp.c  |   0
  drivers/phy/{ => st}/phy-miphy365x.c  |   0
  drivers/phy/{ => st}/phy-spear1310-miphy.c|   0
  drivers/phy/{ => st}/phy-spear1340-miphy.c|   0
  drivers/phy/{ => st}/phy-stih407-usb.c|   0
  drivers/phy/{ => st}/phy-stih41x-usb.c|   0
  drivers/phy/sunxi/Kconfig |  28 ++
  drivers/phy/sunxi/Makefile|   2 +
  drivers/phy/{ => sunxi}/phy-sun4i-usb.c   |   0
  drivers/phy/{ => sunxi}/phy-sun9i-usb.c   |   0
  drivers/phy/ti/Kconfig|  67 
  drivers/phy/ti/Makefile   |   6 +
  drivers/phy/{ => ti}/phy-dm816x-usb.c |   0
  drivers/phy/{ => ti}/phy-omap-control.c   |   0
  drivers/phy/{ => ti}/phy-omap-usb2.c  |   0
  drivers/phy/{ => ti}/phy-ti-pipe3.c 

Re: [PATCH] usb: gadget: f_fs: Fix EFAULT generation for async read operations

[Felipe Balbi]

Hi Al,

Lars-Peter Clausen  writes:
> In the current implementation functionfs generates a EFAULT for async read
> operations if the read buffer size is larger than the URB data size. Since
> a application does not necessarily know how much data the host side is
> going to send it typically supplies a buffer larger than the actual data,
> which will then result in a EFAULT error.
>
> This behaviour was introduced while refactoring the code to use iov_iter
> interface in commit c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter
> into io_data"). The original code took the minimum over the URB size and
> the user buffer size and then attempted to copy that many bytes using
> copy_to_user(). If copy_to_user() could not copy all data a EFAULT error
> was generated. Restore the original behaviour by only generating a EFAULT
> error when the number of bytes copied is not the size of the URB and the
> target buffer has not been fully filled.
>
> Commit 342f39a6c8d3 ("usb: gadget: f_fs: fix check in read operation")
> already fixed the same problem for the synchronous read path.
>
> Fixes: c993c39b8639 ("gadget/function/f_fs.c: use put iov_iter into io_data")
> Signed-off-by: Lars-Peter Clausen 
> ---
> Changes since v1:
>   * copy_to_iter() can fail, make sure that is handled. Test this case by
> mapping part of the target buffer read-only.
>
> Signed-off-by: Lars-Peter Clausen 
> ---
>  drivers/usb/gadget/function/f_fs.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 8cfce10..3b27aa0 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -650,7 +650,7 @@ static void ffs_user_copy_worker(struct work_struct *work)
>   if (io_data->read && ret > 0) {
>   use_mm(io_data->mm);
>   ret = copy_to_iter(io_data->buf, ret, _data->data);
> - if (iov_iter_count(_data->data))
> + if (ret != io_data->req->actual && 
> iov_iter_count(_data->data))
>   ret = -EFAULT;
>   unuse_mm(io_data->mm);
>   }

does this look good for you now ?

-- 
balbi


signature.asc
Description: PGP signature

[PATCH 0/2] usb: xhci: couple cleanups

[Felipe Balbi]

Hi,

The following two patches are tiny simplifications
two count_trbs() and count_sg_trbs_needed() which
make the code slightly easier to read.

Felipe Balbi (2):
  usb: xhci: ring: simplify count_trbs() a little bit
  usb: xhci: ring: simplify count_sg_trbs_needed()

 drivers/usb/host/xhci-ring.c | 35 ---
 1 file changed, 20 insertions(+), 15 deletions(-)

-- 
2.8.0.rc2

--
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: xhci: ring: simplify count_trbs() a little bit

[Felipe Balbi]

If we _know_ length is 0, we can just return 1 early.

While at that, we're also adding a comment
referencing the location where the 64KiB boundary is
mentioned in the xhci spec. This will prevent people
from thinking this isn't necessary in the future.

We're also changing the bitwise-and operator into a
mathematically equivalent (albeit slower) modulus
operator and trusting the compiler to optimize it
back to bitwise-and. The reason for this is that a
modulus operator states the intent with much more
clarity.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-ring.c | 21 +++--
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 2a1bb76f4d88..d7f9c0cfa047 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2940,14 +2940,23 @@ static int prepare_transfer(struct xhci_hcd *xhci,
 
 static unsigned int count_trbs(u64 addr, u64 len)
 {
-   unsigned int num_trbs;
+   u64 aligned_len = len + (addr % TRB_MAX_BUFF_SIZE);
 
-   num_trbs = DIV_ROUND_UP(len + (addr & (TRB_MAX_BUFF_SIZE - 1)),
-   TRB_MAX_BUFF_SIZE);
-   if (num_trbs == 0)
-   num_trbs++;
+   if (len == 0)
+   return 1;
 
-   return num_trbs;
+   /*
+* Note here that we _must_ make sure buffer pointers don't cross 64KiB
+* boundaries as stated on section 6.4.1 of the xHCI specification.
+*
+* We're trusting the compiler to optimize the mod operator down to a
+* bitwise and, considering we're dealing with a power-of-two constant.
+*
+* It's also good to note that some xHCI implementations don't seem to
+* sport this limitation, however for the sake of stability we will
+* maintain this check here.
+*/
+   return DIV_ROUND_UP(aligned_len, TRB_MAX_BUFF_SIZE);
 }
 
 static inline unsigned int count_trbs_needed(struct urb *urb)
-- 
2.8.0.rc2

--
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: xhci: ring: simplify count_sg_trbs_needed()

[Felipe Balbi]

we _know_ that for_each_sg() will only iterate over
number of mapped sgs, so there's no need to count
full_len and break out when it reaches zero.

Signed-off-by: Felipe Balbi 
---
 drivers/usb/host/xhci-ring.c | 14 +-
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index d7f9c0cfa047..c6b77282924d 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -2967,17 +2967,13 @@ static inline unsigned int count_trbs_needed(struct urb 
*urb)
 static unsigned int count_sg_trbs_needed(struct urb *urb)
 {
struct scatterlist *sg;
-   unsigned int i, len, full_len, num_trbs = 0;
-
-   full_len = urb->transfer_buffer_length;
+   unsigned int i, num_trbs = 0;
 
for_each_sg(urb->sg, sg, urb->num_mapped_sgs, i) {
-   len = sg_dma_len(sg);
-   num_trbs += count_trbs(sg_dma_address(sg), len);
-   len = min_t(unsigned int, len, full_len);
-   full_len -= len;
-   if (full_len == 0)
-   break;
+   unsigned int len = sg_dma_len(sg);
+   dma_addr_t dma = sg_dma_address(sg);
+
+   num_trbs += count_trbs(dma, len);
}
 
return num_trbs;
-- 
2.8.0.rc2

--
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 6/7] usb: xhci: fix wild pointers in xhci_mem_cleanup

[Mathias Nyman]

From: Lu Baolu 

This patch fixes some wild pointers produced by xhci_mem_cleanup.
These wild pointers will cause system crash if xhci_mem_cleanup()
is called twice.

Reported-and-tested-by: Pengcheng Li 
Signed-off-by: Lu Baolu 
Cc: sta...@vger.kernel.org
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 80c1de2..bad0d1f 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1861,6 +1861,12 @@ no_bw:
kfree(xhci->rh_bw);
kfree(xhci->ext_caps);
 
+   xhci->usb2_ports = NULL;
+   xhci->usb3_ports = NULL;
+   xhci->port_array = NULL;
+   xhci->rh_bw = NULL;
+   xhci->ext_caps = NULL;
+
xhci->page_size = 0;
xhci->page_shift = 0;
xhci->bus_state[0].bus_suspended = 0;
-- 
1.9.1

--
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 5/7] usb: host: xhci-plat: fix cannot work if R-Car Gen2/3 run on above 4GB phys

[Mathias Nyman]

From: Yoshihiro Shimoda 

This patch fixes an issue that cannot work if R-Car Gen2/3 run on
above 4GB physical memory environment to use a quirk XHCI_NO_64BIT_SUPPORT.

Cc: 
Signed-off-by: Yoshihiro Shimoda 
Reviewed-by: Felipe Balbi 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-plat.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 5c15e9b..474b5fa 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -39,12 +39,25 @@ static const struct xhci_driver_overrides 
xhci_plat_overrides __initconst = {
 
 static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 {
+   struct usb_hcd *hcd = xhci_to_hcd(xhci);
+
/*
 * As of now platform drivers don't provide MSI support so we ensure
 * here that the generic code does not try to make a pci_dev from our
 * dev struct in order to setup MSI
 */
xhci->quirks |= XHCI_PLAT;
+
+   /*
+* On R-Car Gen2 and Gen3, the AC64 bit (bit 0) of HCCPARAMS1 is set
+* to 1. However, these SoCs don't support 64-bit address memory
+* pointers. So, this driver clears the AC64 bit of xhci->hcc_params
+* to call dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) in
+* xhci_gen_setup().
+*/
+   if (xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN2) ||
+   xhci_plat_type_is(hcd, XHCI_PLAT_TYPE_RENESAS_RCAR_GEN3))
+   xhci->quirks |= XHCI_NO_64BIT_SUPPORT;
 }
 
 /* called during probe() after chip reset completes */
-- 
1.9.1

--
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/7] xhci: resume USB 3 roothub first

[Mathias Nyman]

Give USB3 devices a better chance to enumerate at USB 3 speeds if
they are connected to a suspended host.
Solves an issue with NEC uPD720200 host hanging when partially
enumerating a USB3 device as USB2 after host controller runtime resume.

Cc: 
Tested-by: Mike Murdoch 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index d51ee0c..b609288 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1108,8 +1108,8 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
/* Resume root hubs only when have pending events. */
status = readl(>op_regs->status);
if (status & STS_EINT) {
-   usb_hcd_resume_root_hub(hcd);
usb_hcd_resume_root_hub(xhci->shared_hcd);
+   usb_hcd_resume_root_hub(hcd);
}
}
 
@@ -1124,10 +1124,10 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 
/* Re-enable port polling. */
xhci_dbg(xhci, "%s: starting port polling.\n", __func__);
-   set_bit(HCD_FLAG_POLL_RH, >flags);
-   usb_hcd_poll_rh_status(hcd);
set_bit(HCD_FLAG_POLL_RH, >shared_hcd->flags);
usb_hcd_poll_rh_status(xhci->shared_hcd);
+   set_bit(HCD_FLAG_POLL_RH, >flags);
+   usb_hcd_poll_rh_status(hcd);
 
return retval;
 }
-- 
1.9.1

--
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 4/7] usb: host: xhci: add a new quirk XHCI_NO_64BIT_SUPPORT

[Mathias Nyman]

From: Yoshihiro Shimoda 

On some xHCI controllers (e.g. R-Car SoCs), the AC64 bit (bit 0) of
HCCPARAMS1 is set to 1. However, the xHCs don't support 64-bit
address memory pointers actually. So, in this case, this driver should
call dma_set_coherent_mask(dev, DMA_BIT_MASK(32)) in xhci_gen_setup().
Otherwise, the xHCI controller will be died after a usb device is
connected if it runs on above 4GB physical memory environment.

So, this patch adds a new quirk XHCI_NO_64BIT_SUPPORT to resolve
such an issue.

Cc: 
Signed-off-by: Yoshihiro Shimoda 
Reviewed-by: Felipe Balbi 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.c | 10 ++
 drivers/usb/host/xhci.h |  1 +
 2 files changed, 11 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 16366a9..31f73ed 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4949,6 +4949,16 @@ int xhci_gen_setup(struct usb_hcd *hcd, 
xhci_get_quirks_t get_quirks)
return retval;
xhci_dbg(xhci, "Reset complete\n");
 
+   /*
+* On some xHCI controllers (e.g. R-Car SoCs), the AC64 bit (bit 0)
+* of HCCPARAMS1 is set to 1. However, the xHCs don't support 64-bit
+* address memory pointers actually. So, this driver clears the AC64
+* bit of xhci->hcc_params to call dma_set_coherent_mask(dev,
+* DMA_BIT_MASK(32)) in this xhci_gen_setup().
+*/
+   if (xhci->quirks & XHCI_NO_64BIT_SUPPORT)
+   xhci->hcc_params &= ~BIT(0);
+
/* Set dma_mask and coherent_dma_mask to 64-bits,
 * if xHC supports 64-bit addressing */
if (HCC_64BIT_ADDR(xhci->hcc_params) &&
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e293e09..70f215c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1641,6 +1641,7 @@ struct xhci_hcd {
 #define XHCI_PME_STUCK_QUIRK   (1 << 20)
 #define XHCI_MTK_HOST  (1 << 21)
 #define XHCI_SSIC_PORT_UNUSED  (1 << 22)
+#define XHCI_NO_64BIT_SUPPORT  (1 << 23)
unsigned intnum_active_eps;
unsigned intlimit_active_eps;
/* There are two roothubs to keep track of bus suspend info for */
-- 
1.9.1

--
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 7/7] xhci: fix 10 second timeout on removal of PCI hotpluggable xhci controllers

[Mathias Nyman]

PCI hotpluggable xhci controllers such as some Alpine Ridge solutions will
remove the xhci controller from the PCI bus when the last USB device is
disconnected.

Add a flag to indicate that the host is being removed to avoid queueing
configure_endpoint commands for the dropped endpoints.
For PCI hotplugged controllers this will prevent 5 second command timeouts
For static xhci controllers the configure_endpoint command is not needed
in the removal case as everything will be returned, freed, and the
controller is reset.

For now the flag is only set for PCI connected host controllers.

Cc: 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-pci.c  | 1 +
 drivers/usb/host/xhci-ring.c | 3 ++-
 drivers/usb/host/xhci.c  | 8 +---
 drivers/usb/host/xhci.h  | 1 +
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 071b34a..48672fa 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -304,6 +304,7 @@ static void xhci_pci_remove(struct pci_dev *dev)
struct xhci_hcd *xhci;
 
xhci = hcd_to_xhci(pci_get_drvdata(dev));
+   xhci->xhc_state |= XHCI_STATE_REMOVING;
if (xhci->shared_hcd) {
usb_remove_hcd(xhci->shared_hcd);
usb_put_hcd(xhci->shared_hcd);
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 7cf6621..99b4ff4 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -4004,7 +4004,8 @@ static int queue_command(struct xhci_hcd *xhci, struct 
xhci_command *cmd,
int reserved_trbs = xhci->cmd_ring_reserved_trbs;
int ret;
 
-   if (xhci->xhc_state) {
+   if ((xhci->xhc_state & XHCI_STATE_DYING) ||
+   (xhci->xhc_state & XHCI_STATE_HALTED)) {
xhci_dbg(xhci, "xHCI dying or halted, can't queue_command\n");
return -ESHUTDOWN;
}
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 31f73ed..679266b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -147,7 +147,8 @@ static int xhci_start(struct xhci_hcd *xhci)
"waited %u microseconds.\n",
XHCI_MAX_HALT_USEC);
if (!ret)
-   xhci->xhc_state &= ~(XHCI_STATE_HALTED | XHCI_STATE_DYING);
+   /* clear state flags. Including dying, halted or removing */
+   xhci->xhc_state = 0;
 
return ret;
 }
@@ -2774,7 +2775,8 @@ int xhci_check_bandwidth(struct usb_hcd *hcd, struct 
usb_device *udev)
if (ret <= 0)
return ret;
xhci = hcd_to_xhci(hcd);
-   if (xhci->xhc_state & XHCI_STATE_DYING)
+   if ((xhci->xhc_state & XHCI_STATE_DYING) ||
+   (xhci->xhc_state & XHCI_STATE_REMOVING))
return -ENODEV;
 
xhci_dbg(xhci, "%s called for udev %p\n", __func__, udev);
@@ -3821,7 +3823,7 @@ static int xhci_setup_device(struct usb_hcd *hcd, struct 
usb_device *udev,
 
mutex_lock(>mutex);
 
-   if (xhci->xhc_state)/* dying or halted */
+   if (xhci->xhc_state)/* dying, removing or halted */
goto out;
 
if (!udev->slot_id) {
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 70f215c..6c629c9 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1605,6 +1605,7 @@ struct xhci_hcd {
  */
 #define XHCI_STATE_DYING   (1 << 0)
 #define XHCI_STATE_HALTED  (1 << 1)
+#define XHCI_STATE_REMOVING(1 << 2)
/* Statistics */
int error_bitmask;
unsigned intquirks;
-- 
1.9.1

--
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/7] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

These xhci fixes solve various small issues such as:
 * USB 3 device enumeration issues after runtime resume seen on NEC hosts
 * Add a PCI ID and needed quirk for a new controller
 * Remove extra 10 seconds driver timeouts on PCI hotplugged xhcis
 * R-Car SoC xhci quirks to solve their DMA addressing issues.

Lu Baolu (1):
  usb: xhci: fix wild pointers in xhci_mem_cleanup

Mathias Nyman (3):
  xhci: resume USB 3 roothub first
  xhci: Don't suspend a xhci usb bus if there is a pending event.
  xhci: fix 10 second timeout on removal of PCI hotpluggable xhci
controllers

Rafal Redzimski (1):
  usb: xhci: applying XHCI_PME_STUCK_QUIRK to Intel BXT B0 host

Yoshihiro Shimoda (2):
  usb: host: xhci: add a new quirk XHCI_NO_64BIT_SUPPORT
  usb: host: xhci-plat: fix cannot work if R-Car Gen2/3 run on above 4GB
phys

 drivers/usb/host/xhci-hub.c  |  6 ++
 drivers/usb/host/xhci-mem.c  |  6 ++
 drivers/usb/host/xhci-pci.c  |  5 -
 drivers/usb/host/xhci-plat.c | 13 +
 drivers/usb/host/xhci-ring.c |  3 ++-
 drivers/usb/host/xhci.c  | 25 +++--
 drivers/usb/host/xhci.h  |  2 ++
 7 files changed, 52 insertions(+), 8 deletions(-)

-- 
1.9.1

--
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/7] usb: xhci: applying XHCI_PME_STUCK_QUIRK to Intel BXT B0 host

[Mathias Nyman]

From: Rafal Redzimski 

Broxton B0 also requires XHCI_PME_STUCK_QUIRK.
Adding PCI device ID for Broxton B and adding to quirk.

Cc: 
Signed-off-by: Rafal Redzimski 
Signed-off-by: Robert Dobrowolski 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index f0640b7..071b34a 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -48,6 +48,7 @@
 #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_XHCI0xa12f
 #define PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI   0x9d2f
 #define PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI 0x0aa8
+#define PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI 0x1aa8
 
 static const char hcd_name[] = "xhci_hcd";
 
@@ -155,7 +156,8 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
(pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI ||
 pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_XHCI ||
 pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
-pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI)) {
+pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_M_XHCI ||
+pdev->device == PCI_DEVICE_ID_INTEL_BROXTON_B_XHCI)) {
xhci->quirks |= XHCI_PME_STUCK_QUIRK;
}
if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
-- 
1.9.1

--
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 v9 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Jun Li  writes:
>> >> On 6 April 2016 at 15:19, Peter Chen  wrote:
>> >> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote:
>> >> >>
>> >> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops {
>> >> >>   struct usb_ep *(*match_ep)(struct usb_gadget *,
>> >> >>   struct usb_endpoint_descriptor *,
>> >> >>   struct usb_ss_ep_comp_descriptor *);
>> >> >> + /* get the charger type */
>> >> >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget
>> >> >> + *);
>> >> >>  };
>> >> >
>> >> > Since we already have get_charger_type callback at usb_charger
>> >> > structure, why we still need this API at usb_gadget_ops?
>> >>
>> >> In case some users want to get charger type at gadget level.
>> >>
>> > Why gadget needs to know charger type? I also don't catch the intent
>> > of
>> 
>> because some gadgets need to call usb_gadget_vbus_draw(), although for
>> that they need power in mA rather.
>
> In below change of usb_gadget_vbus_draw(), already can get charger type
> via usb_charger_get_type().
>
> static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
>  {
> +   enum usb_charger_type type;
> +
> +   if (gadget->charger) {
> +   type = usb_charger_get_type(gadget->charger);
> +   usb_charger_set_cur_limit_by_type(gadget->charger, type, mA);
> +   }
> +
> if (!gadget->ops->vbus_draw)
> return -EOPNOTSUPP;
> return gadget->ops->vbus_draw(gadget, mA);
>
> Could you detail in what situation gadget->ops-> get_charger_type() is used?

isn't it right there in the code ? Isn't usb_gadget_vbus_draw() calling
it ? What did I miss here ?

>> > This api, as my understanding, gadget only need report gadget state
>> changes.
>> > All information required for usb charger is charger type and gadget
>> state.
>> 
>> you're making an assumption about how the HW is laid out which might not
>> be true.
>> 
>
> What other information you refer to here? Or what I am not aware of?

what I'm trying to say is that you're assuming gadgets don't need to
know anything other than charger type and gadget state (suspended,
resume, enumerated, default state, addressed, etc), but that might not
be true for all UDCs. You can't make that assumption that charger type
and gadget state is enough. The real question is what do we need *now*,
but still keep in mind that what we need *now* might be valid 2 years
from now, so API needs to be a little flexible.

cheers

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/2] phy: Group vendor specific phy drivers

[Kishon Vijay Abraham I]

Hi,

On Friday 01 April 2016 07:05 PM, Vivek Gautam wrote:
> Hi,
> 
> 
> On Fri, Apr 1, 2016 at 6:05 AM, Kishon Vijay Abraham I  wrote:
>> Hi,
>>
>> On Friday 01 April 2016 04:59 PM, Vivek Gautam wrote:
>>> Adding vendor specific directories in phy to group
>>> phy drivers under their respective vendor umbrella.
>>>
>>> Signed-off-by: Vivek Gautam 
>>> ---
>>>
>>> With growing number of phy drivers, it makes sense to
>>> group these drivers under their respective vendor/platform
>>> umbrella directory.
>>>
>>> Build-tested 'multi_v7_defconfig'.
>>>
>>>  drivers/phy/Kconfig   | 386 
>>> +-
>>>  drivers/phy/Makefile  |  57 +---
>>>  drivers/phy/bcom/Kconfig  |  27 ++
>>>  drivers/phy/bcom/Makefile |   3 +
>>>  drivers/phy/{ => bcom}/phy-bcm-cygnus-pcie.c  |   0
>>>  drivers/phy/{ => bcom}/phy-bcm-kona-usb2.c|   0
>>>  drivers/phy/{ => bcom}/phy-brcmstb-sata.c |   0
>>>  drivers/phy/hisi/Kconfig  |  20 ++
>>>  drivers/phy/hisi/Makefile |   2 +
>>>  drivers/phy/{ => hisi}/phy-hi6220-usb.c   |   0
>>>  drivers/phy/{ => hisi}/phy-hix5hd2-sata.c |   0
>>>  drivers/phy/marvell/Kconfig   |  50 +++
>>>  drivers/phy/marvell/Makefile  |   6 +
>>>  drivers/phy/{ => marvell}/phy-armada375-usb2.c|   0
>>>  drivers/phy/{ => marvell}/phy-berlin-sata.c   |   0
>>>  drivers/phy/{ => marvell}/phy-berlin-usb.c|   0
>>>  drivers/phy/{ => marvell}/phy-mvebu-sata.c|   0
>>>  drivers/phy/{ => marvell}/phy-pxa-28nm-hsic.c |   0
>>>  drivers/phy/{ => marvell}/phy-pxa-28nm-usb2.c |   0
>>>  drivers/phy/qcom/Kconfig  |  23 ++
>>>  drivers/phy/qcom/Makefile |   5 +
>>>  drivers/phy/{ => qcom}/phy-qcom-apq8064-sata.c|   0
>>>  drivers/phy/{ => qcom}/phy-qcom-ipq806x-sata.c|   0
>>>  drivers/phy/{ => qcom}/phy-qcom-ufs-i.h   |   0
>>>  drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-14nm.c|   0
>>>  drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-14nm.h|   0
>>>  drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-20nm.c|   0
>>>  drivers/phy/{ => qcom}/phy-qcom-ufs-qmp-20nm.h|   0
>>>  drivers/phy/{ => qcom}/phy-qcom-ufs.c |   0
>>>  drivers/phy/rcar/Kconfig  |  16 +
>>>  drivers/phy/rcar/Makefile |   2 +
>>>  drivers/phy/{ => rcar}/phy-rcar-gen2.c|   0
>>>  drivers/phy/{ => rcar}/phy-rcar-gen3-usb2.c   |   0
>>>  drivers/phy/rockchip/Kconfig  |  23 ++
>>>  drivers/phy/rockchip/Makefile |   3 +
>>>  drivers/phy/{ => rockchip}/phy-rockchip-dp.c  |   0
>>>  drivers/phy/{ => rockchip}/phy-rockchip-emmc.c|   0
>>>  drivers/phy/{ => rockchip}/phy-rockchip-usb.c |   0
>>>  drivers/phy/samsung/Kconfig   |  87 +
>>>  drivers/phy/samsung/Makefile  |  10 +
>>>  drivers/phy/{ => samsung}/phy-exynos-dp-video.c   |   0
>>>  drivers/phy/{ => samsung}/phy-exynos-mipi-video.c |   0
>>>  drivers/phy/{ => samsung}/phy-exynos4210-usb2.c   |   0
>>>  drivers/phy/{ => samsung}/phy-exynos4x12-usb2.c   |   0
>>>  drivers/phy/{ => samsung}/phy-exynos5-usbdrd.c|   0
>>>  drivers/phy/{ => samsung}/phy-exynos5250-sata.c   |   0
>>>  drivers/phy/{ => samsung}/phy-exynos5250-usb2.c   |   0
>>>  drivers/phy/{ => samsung}/phy-s5pv210-usb2.c  |   0
>>>  drivers/phy/{ => samsung}/phy-samsung-usb2.c  |   0
>>>  drivers/phy/{ => samsung}/phy-samsung-usb2.h  |   0
>>>  drivers/phy/st/Kconfig|  51 +++
>>>  drivers/phy/st/Makefile   |   6 +
>>>  drivers/phy/{ => st}/phy-miphy28lp.c  |   0
>>>  drivers/phy/{ => st}/phy-miphy365x.c  |   0
>>>  drivers/phy/{ => st}/phy-spear1310-miphy.c|   0
>>>  drivers/phy/{ => st}/phy-spear1340-miphy.c|   0
>>>  drivers/phy/{ => st}/phy-stih407-usb.c|   0
>>>  drivers/phy/{ => st}/phy-stih41x-usb.c|   0
>>>  drivers/phy/sunxi/Kconfig |  28 ++
>>>  drivers/phy/sunxi/Makefile|   2 +
>>>  drivers/phy/{ => sunxi}/phy-sun4i-usb.c   |   0
>>>  drivers/phy/{ => sunxi}/phy-sun9i-usb.c   |   0
>>>  drivers/phy/ti/Kconfig|  67 
>>>  drivers/phy/ti/Makefile   |   6 +
>>>  drivers/phy/{ => ti}/phy-dm816x-usb.c |   0
>>>  drivers/phy/{ => ti}/phy-omap-control.c   |   0
>>>  drivers/phy/{ => ti}/phy-omap-usb2.c  |   0
>>>  drivers/phy/{ => ti}/phy-ti-pipe3.c   |   0
>>>  drivers/phy/{ => ti}/phy-tusb1210.c   |   0
>>>  drivers/phy/{ => ti}/phy-twl4030-usb.c|   0
>>>  drivers/phy/{ => ti}/ulpi_phy.h

RE: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Jun Li]

> -Original Message-
> From: Felipe Balbi [mailto:ba...@kernel.org]
> Sent: Wednesday, April 06, 2016 8:22 PM
> To: Jun Li ; Baolin Wang ; Peter
> Chen 
> Cc: Greg KH ; Sebastian Reichel
> ; Dmitry Eremin-Solenikov ; David
> Woodhouse ; Peter Chen ;
> Alan Stern ; r.bald...@samsung.com; Yoshihiro
> Shimoda ; Lee Jones
> ; Mark Brown ; Charles Keepax
> ; patc...@opensource.wolfsonmicro.com;
> Linux PM list ; USB ;
> device-mainlin...@lists.linuxfoundation.org; LKML  ker...@vger.kernel.org>
> Subject: RE: [PATCH v9 2/4] gadget: Support for the usb charger framework
> 
> 
> Hi,
> 
> Jun Li  writes:
> >> -Original Message-
> >> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> >> ow...@vger.kernel.org] On Behalf Of Baolin Wang
> >> Sent: Wednesday, April 06, 2016 6:47 PM
> >> To: Peter Chen 
> >> Cc: Felipe Balbi ; Greg KH
> >> ; Sebastian Reichel ;
> >> Dmitry Eremin-Solenikov ; David Woodhouse
> >> ; Peter Chen ; Alan
> >> Stern ; r.bald...@samsung.com; Yoshihiro
> >> Shimoda ; Lee Jones
> >> ; Mark Brown ; Charles
> >> Keepax ;
> >> patc...@opensource.wolfsonmicro.com;
> >> Linux PM list ; USB
> >> ;
> >> device-mainlin...@lists.linuxfoundation.org; LKML  >> ker...@vger.kernel.org>
> >> Subject: Re: [PATCH v9 2/4] gadget: Support for the usb charger
> >> framework
> >>
> >> On 6 April 2016 at 15:19, Peter Chen  wrote:
> >> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote:
> >> >>
> >> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops {
> >> >>   struct usb_ep *(*match_ep)(struct usb_gadget *,
> >> >>   struct usb_endpoint_descriptor *,
> >> >>   struct usb_ss_ep_comp_descriptor *);
> >> >> + /* get the charger type */
> >> >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget
> >> >> + *);
> >> >>  };
> >> >
> >> > Since we already have get_charger_type callback at usb_charger
> >> > structure, why we still need this API at usb_gadget_ops?
> >>
> >> In case some users want to get charger type at gadget level.
> >>
> > Why gadget needs to know charger type? I also don't catch the intent
> > of
> 
> because some gadgets need to call usb_gadget_vbus_draw(), although for
> that they need power in mA rather.

In below change of usb_gadget_vbus_draw(), already can get charger type
via usb_charger_get_type().

static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA)
 {
+   enum usb_charger_type type;
+
+   if (gadget->charger) {
+   type = usb_charger_get_type(gadget->charger);
+   usb_charger_set_cur_limit_by_type(gadget->charger, type, mA);
+   }
+
if (!gadget->ops->vbus_draw)
return -EOPNOTSUPP;
return gadget->ops->vbus_draw(gadget, mA);

Could you detail in what situation gadget->ops-> get_charger_type() is used?

> 
> > This api, as my understanding, gadget only need report gadget state
> changes.
> > All information required for usb charger is charger type and gadget
> state.
> 
> you're making an assumption about how the HW is laid out which might not
> be true.
> 

What other information you refer to here? Or what I am not aware of?

Thanks.
Li Jun  

> --
> balbi
--
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 v10 4/9] phy: Add Tegra XUSB pad controller support

[Kishon Vijay Abraham I]

Hi,

On Friday 04 March 2016 09:49 PM, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Add a new driver for the XUSB pad controller found on NVIDIA Tegra SoCs.
> This hardware block used to be exposed as a pin controller, but it turns
> out that this isn't a good fit. The new driver and DT binding much more
> accurately describe the hardware and are more flexible in supporting new
> SoC generations.
> 
> Signed-off-by: Thierry Reding 
> ---
> Changes in v9:
> - export public API for direct use by the xHCI driver (replaces mailbox
>   API which had introduced a nasty circular dependency)
> 
>  drivers/phy/Kconfig|2 +
>  drivers/phy/Makefile   |2 +
>  drivers/phy/tegra/Kconfig  |8 +
>  drivers/phy/tegra/Makefile |5 +
>  drivers/phy/tegra/xusb-tegra124.c  | 1747 
> 
>  drivers/phy/tegra/xusb.c   | 1017 
>  drivers/phy/tegra/xusb.h   |  421 +++
>  drivers/pinctrl/tegra/pinctrl-tegra-xusb.c |   20 +-
>  include/linux/phy/tegra/xusb.h |   30 +
>  9 files changed, 3236 insertions(+), 16 deletions(-)
>  create mode 100644 drivers/phy/tegra/Kconfig
>  create mode 100644 drivers/phy/tegra/Makefile
>  create mode 100644 drivers/phy/tegra/xusb-tegra124.c
>  create mode 100644 drivers/phy/tegra/xusb.c
>  create mode 100644 drivers/phy/tegra/xusb.h
>  create mode 100644 include/linux/phy/tegra/xusb.h
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 0124d17bd9fe..4bf65ceb3250 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -407,4 +407,6 @@ config PHY_CYGNUS_PCIE
> Enable this to support the Broadcom Cygnus PCIe PHY.
> If unsure, say N.
>  
> +source "drivers/phy/tegra/Kconfig"
> +
>  endmenu
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index c80f09df3bb8..82709141d072 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -50,3 +50,5 @@ obj-$(CONFIG_PHY_TUSB1210)  += phy-tusb1210.o
>  obj-$(CONFIG_PHY_BRCMSTB_SATA)   += phy-brcmstb-sata.o
>  obj-$(CONFIG_PHY_PISTACHIO_USB)  += phy-pistachio-usb.o
>  obj-$(CONFIG_PHY_CYGNUS_PCIE)+= phy-bcm-cygnus-pcie.o
> +
> +obj-$(CONFIG_ARCH_TEGRA) += tegra/
> diff --git a/drivers/phy/tegra/Kconfig b/drivers/phy/tegra/Kconfig
> new file mode 100644
> index ..a3b1de953fb7
> --- /dev/null
> +++ b/drivers/phy/tegra/Kconfig
> @@ -0,0 +1,8 @@
> +config PHY_TEGRA_XUSB
> + tristate "NVIDIA Tegra XUSB pad controller driver"
> + depends on ARCH_TEGRA
> + help
> +   Choose this option if you have an NVIDIA Tegra SoC.
> +
> +   To compile this driver as a module, choose M here: the module will
> +   be called phy-tegra-xusb.
> diff --git a/drivers/phy/tegra/Makefile b/drivers/phy/tegra/Makefile
> new file mode 100644
> index ..31150b4337cd
> --- /dev/null
> +++ b/drivers/phy/tegra/Makefile
> @@ -0,0 +1,5 @@
> +obj-$(CONFIG_PHY_TEGRA_XUSB) += phy-tegra-xusb.o
> +
> +phy-tegra-xusb-y += xusb.o
> +phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_124_SOC) += xusb-tegra124.o
> +phy-tegra-xusb-$(CONFIG_ARCH_TEGRA_132_SOC) += xusb-tegra124.o
> diff --git a/drivers/phy/tegra/xusb-tegra124.c 
> b/drivers/phy/tegra/xusb-tegra124.c
> new file mode 100644
> index ..6340d43688d3
> --- /dev/null
> +++ b/drivers/phy/tegra/xusb-tegra124.c
> @@ -0,0 +1,1747 @@
> +/*
> + * Copyright (c) 2014, NVIDIA CORPORATION.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#include 
> +
> +#include "xusb.h"
> +
> +#define FUSE_SKU_CALIB_HS_CURR_LEVEL_PADX_SHIFT(x) ((x) ? 15 : 0)
> +#define FUSE_SKU_CALIB_HS_CURR_LEVEL_PAD_MASK 0x3f
> +#define FUSE_SKU_CALIB_HS_IREF_CAP_SHIFT 13
> +#define FUSE_SKU_CALIB_HS_IREF_CAP_MASK 0x3
> +#define FUSE_SKU_CALIB_HS_SQUELCH_LEVEL_SHIFT 11
> +#define FUSE_SKU_CALIB_HS_SQUELCH_LEVEL_MASK 0x3
> +#define FUSE_SKU_CALIB_HS_TERM_RANGE_ADJ_SHIFT 7
> +#define FUSE_SKU_CALIB_HS_TERM_RANGE_ADJ_MASK 0xf
> +
> +#define XUSB_PADCTL_USB2_PORT_CAP 0x008
> +#define XUSB_PADCTL_USB2_PORT_CAP_PORTX_CAP_SHIFT(x) ((x) * 4)
> +#define XUSB_PADCTL_USB2_PORT_CAP_PORT_CAP_MASK 0x3
> +#define XUSB_PADCTL_USB2_PORT_CAP_DISABLED 0x0
> +#define XUSB_PADCTL_USB2_PORT_CAP_HOST 0x1
> +#define XUSB_PADCTL_USB2_PORT_CAP_DEVICE 

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Sergei Shtylyov]

Hello.

On 4/6/2016 9:44 AM, Lu Baolu wrote:


+struct intel_mux_dev {
+   struct device   *dev;
+   char*extcon_name;
+   char*cable_name;
+   int (*cable_set_cb)(struct intel_mux_dev *mux);
+   int (*cable_unset_cb)(struct intel_mux_dev *mux);
+};

This is a device, why not make it one?  Don't just hold a reference.
And do you really even hold that reference?

It's not a device. It's just an encapsulation for parameters passed into
intel_usb_mux_register().

But you called it a device, so you can understand my confusion.

And why not make it a device?  Why isn't this one?  Hint, I really think
it should be...


And your api is horrid, think about what you want the "core" to do here,
it should be the one creating the device and returning it to the caller,
not forcing the caller to somehow create it first and then pass it in.

This isn't a layer or core. It doesn't create any new devices. It's actually
some shared code which can be used by all Intel dual role port drivers.

It should be a device, as you are treating it like one :)


I put it in a separated file because 1) this can avoid duplication; 2) this code
could be used for any architectures as long as a USB port is shared by
two components and it needs an OS response when event triggers.

It's a bit hard for other arches to be using something called "intel_"
:(


Are there any other implementations which need an external mux
to swap the usb roles?


   There are, of course, like Renesas R-Car gen2 SoCs. I've implemented the 
USB port multiplexing as a part of the PHY driver (see 
drivers/phy/phy-rcar-gen2.c).


MBR, Sergei

--
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] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Felipe Balbi]

Hi,

(please make sure to break your lines at
80-characters. Documentation/email-clients.txt has several tips for
different email clients ;-))

"Du, Changbin"  writes:
>> > @@ -648,6 +687,12 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
>> >goto err1;
>> >}
>> >
>> > +  file = debugfs_create_file("fifo", S_IRUGO, root, dwc,
>> _fifo_fops);
>> > +  if (!file) {
>> > +  ret = -ENOMEM;
>> 
>> Um, no, that's not the error here.  You shouldn't care at all about
>> debugfs api call results.  Just keep moving on here please.
>> 
>> thanks,
>> 
>> greg k-h
>
> Agree with you. I will create another patch to cleanup this piece of
> code.

cool, thanks

> And I found a memory leak issue there, dwc->regset never
> released. Will also fix it.

good catch, thanks for fixing it.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Felipe Balbi]

Hi,

Jun Li  writes:
>> -Original Message-
>> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
>> ow...@vger.kernel.org] On Behalf Of Baolin Wang
>> Sent: Wednesday, April 06, 2016 6:47 PM
>> To: Peter Chen 
>> Cc: Felipe Balbi ; Greg KH ;
>> Sebastian Reichel ; Dmitry Eremin-Solenikov
>> ; David Woodhouse ; Peter Chen
>> ; Alan Stern ;
>> r.bald...@samsung.com; Yoshihiro Shimoda
>> ; Lee Jones ; Mark
>> Brown ; Charles Keepax
>> ; patc...@opensource.wolfsonmicro.com;
>> Linux PM list ; USB ;
>> device-mainlin...@lists.linuxfoundation.org; LKML > ker...@vger.kernel.org>
>> Subject: Re: [PATCH v9 2/4] gadget: Support for the usb charger framework
>> 
>> On 6 April 2016 at 15:19, Peter Chen  wrote:
>> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote:
>> >>
>> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops {
>> >>   struct usb_ep *(*match_ep)(struct usb_gadget *,
>> >>   struct usb_endpoint_descriptor *,
>> >>   struct usb_ss_ep_comp_descriptor *);
>> >> + /* get the charger type */
>> >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
>> >>  };
>> >
>> > Since we already have get_charger_type callback at usb_charger
>> > structure, why we still need this API at usb_gadget_ops?
>> 
>> In case some users want to get charger type at gadget level.
>> 
> Why gadget needs to know charger type? I also don't catch the intent of

because some gadgets need to call usb_gadget_vbus_draw(), although for
that they need power in mA rather.

> This api, as my understanding, gadget only need report gadget state changes.
> All information required for usb charger is charger type and gadget state.

you're making an assumption about how the HW is laid out which might not
be true.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Jun Li]

Hi

> -Original Message-
> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> ow...@vger.kernel.org] On Behalf Of Baolin Wang
> Sent: Wednesday, April 06, 2016 6:47 PM
> To: Peter Chen 
> Cc: Felipe Balbi ; Greg KH ;
> Sebastian Reichel ; Dmitry Eremin-Solenikov
> ; David Woodhouse ; Peter Chen
> ; Alan Stern ;
> r.bald...@samsung.com; Yoshihiro Shimoda
> ; Lee Jones ; Mark
> Brown ; Charles Keepax
> ; patc...@opensource.wolfsonmicro.com;
> Linux PM list ; USB ;
> device-mainlin...@lists.linuxfoundation.org; LKML  ker...@vger.kernel.org>
> Subject: Re: [PATCH v9 2/4] gadget: Support for the usb charger framework
> 
> On 6 April 2016 at 15:19, Peter Chen  wrote:
> > On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote:
> >>
> >> @@ -563,6 +564,8 @@ struct usb_gadget_ops {
> >>   struct usb_ep *(*match_ep)(struct usb_gadget *,
> >>   struct usb_endpoint_descriptor *,
> >>   struct usb_ss_ep_comp_descriptor *);
> >> + /* get the charger type */
> >> + enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
> >>  };
> >
> > Since we already have get_charger_type callback at usb_charger
> > structure, why we still need this API at usb_gadget_ops?
> 
> In case some users want to get charger type at gadget level.
> 
Why gadget needs to know charger type? I also don't catch the intent of
This api, as my understanding, gadget only need report gadget state changes.
All information required for usb charger is charger type and gadget state.

Li Jun

Re: [PATCH v2 07/14] USB: ch341: add support for parity, frame length, stop bits

[One Thousand Gnomes]

On Sat,  2 Apr 2016 19:07:16 +0200
Grigori Goronzy  wrote:

> With the new reinitialization method, configuring parity, different
> frame lengths and different stop bit settings work as expected on
> both CH340G and CH341A.  This has been extensively tested with a
> logic analyzer.
> 
> Tested-by: Ryan Barber 
> Signed-off-by: Grigori Goronzy 
> ---
>  drivers/usb/serial/ch341.c | 36 ++--
>  1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c001773..4d66f0f 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -346,6 +346,7 @@ static void ch341_set_termios(struct tty_struct *tty,
>   unsigned baud_rate;
>   unsigned long flags;
>   unsigned char ctrl;
> + unsigned cflag = tty->termios.c_cflag;
>   int r;
>  
>   /* redundant changes may cause the chip to lose bytes */
> @@ -356,7 +357,35 @@ static void ch341_set_termios(struct tty_struct *tty,
>  
>   priv->baud_rate = baud_rate;
>  
> - ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX | CH341_LCR_CS8;
> + ctrl = CH341_LCR_ENABLE_RX | CH341_LCR_ENABLE_TX;
> +
> + switch (cflag & CSIZE) {
> + case CS5:
> + ctrl |= CH341_LCR_CS5;
> + break;
> + case CS6:
> + ctrl |= CH341_LCR_CS6;
> + break;
> + case CS7:
> + ctrl |= CH341_LCR_CS7;
> + break;
> + case CS8:
> + default:
> + ctrl |= CH341_LCR_CS8;

In the default case tty-.termios.c_cflag should also be updated to show
CS8

Alan
--
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 v2 01/14] USB: ch341: improve documentation

[One Thousand Gnomes]

On Sat,  2 Apr 2016 19:07:10 +0200
Grigori Goronzy  wrote:

> Signed-off-by: Grigori Goronzy 
> ---
>  drivers/usb/serial/ch341.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/serial/ch341.c b/drivers/usb/serial/ch341.c
> index c73808f..43e4594 100644
> --- a/drivers/usb/serial/ch341.c
> +++ b/drivers/usb/serial/ch341.c
> @@ -3,7 +3,8 @@
>   * Copyright 2007, Werner Cornelius 
>   * Copyright 2009, Boris Hajduk 
>   *
> - * ch341.c implements a serial port driver for the Winchiphead CH341.
> + * ch341.c implements a serial port driver for the Winchiphead CH341,
> + * the second-worst USB serial chip in the world.

Tempting as it may be it probably doesn't belong in the kernel.

--
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 v9 1/4] gadget: Introduce the usb charger framework

[Jun Li]

Hi

> -Original Message-
> From: Baolin Wang [mailto:baolin.w...@linaro.org]
> Sent: Wednesday, April 06, 2016 7:31 PM
> To: Jun Li 
> Cc: ba...@kernel.org; gre...@linuxfoundation.org; s...@kernel.org;
> dbarysh...@gmail.com; dw...@infradead.org; peter.c...@freescale.com;
> st...@rowland.harvard.edu; r.bald...@samsung.com;
> yoshihiro.shimoda...@renesas.com; lee.jo...@linaro.org; broo...@kernel.org;
> ckee...@opensource.wolfsonmicro.com; patc...@opensource.wolfsonmicro.com;
> linux...@vger.kernel.org; linux-usb@vger.kernel.org; device-
> mainlin...@lists.linuxfoundation.org; linux-ker...@vger.kernel.org
> Subject: Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework
> 
> On 6 April 2016 at 16:26, Jun Li  wrote:
> > Hi
> >
> >> + */
> >> +static enum usb_charger_type
> >> +usb_charger_get_type_by_others(struct usb_charger *uchger) {
> >> + if (uchger->type != UNKNOWN_TYPE)
> >> + return uchger->type;
> >> +
> >> + if (uchger->psy) {
> >> + union power_supply_propval val;
> >> +
> >> + power_supply_get_property(uchger->psy,
> >> +   POWER_SUPPLY_PROP_CHARGE_TYPE,
> >> +   );
> >> + switch (val.intval) {
> >> + case POWER_SUPPLY_TYPE_USB:
> >> + uchger->type = SDP_TYPE;
> >> + break;
> >> + case POWER_SUPPLY_TYPE_USB_DCP:
> >> + uchger->type = DCP_TYPE;
> >> + break;
> >> + case POWER_SUPPLY_TYPE_USB_CDP:
> >> + uchger->type = CDP_TYPE;
> >> + break;
> >> + case POWER_SUPPLY_TYPE_USB_ACA:
> >> + uchger->type = ACA_TYPE;
> >> + break;
> >> + default:
> >> + uchger->type = UNKNOWN_TYPE;
> >> + break;
> >> + }
> >> + } else if (uchger->get_charger_type) {
> >> + uchger->type = uchger->get_charger_type(uchger);
> >> + } else {
> >> + uchger->type = UNKNOWN_TYPE;
> >> + }
> >> +
> >> + return uchger->type;
> >> +}
> >> +
> >
> > I think we may don't need this usb_charger_get_type_by_others().
> > "uchger->type" is set in one place is enough, that is: by
> > uchger->charger_detect() in usb_charger_detect_type(), then
> > usb_charger_get_type_by_others() is replaced by usb_charger_get_type().
> >
> > uchger->charger_detect() can have diff implementations no matter
> > what kind of mechanism is used, for your PMIC case, you can just
> > directly get the type value by power_supply_get_property(); with that,
> > we can have one central place to set uchger->type.
> > After uchger->type is set, charger type detection is no need to be
> > involved until charger type changes.
> >
> > Then next question is where is to call usb_charger_detect_type(), We
> > need make sure it finished before usb gadget connect.
> 
> Yeah, that's the point: where? It is hard for usb charger framework to
> control, which will make it more complicated. The
> 'usb_charger_detect_type()' is used for detecting the charger type
> manually on user's platform, and user should call it at the right time to
> avoid affecting gadget enumeration. Otherwise user can implement some
> callbacks showed in 'usb_charger_get_type_by_others()' function to get
> charger type. I think this is controllable and simple for the usb charger
> framework.

As my suggested, Can this detection be in usb_udc_vbus_handler(), and
before usb_udc_connect_control()? We should be able to find some central
place where it is between vbus detection and gadget connect. 

> 
> >
> > Ideal is with your framework, diff users only need implement
> > uchger->charger_detect(). :)
> >
> 
> --
> Baolin.wang
> Best Regards
N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

[PATCH v1] net: cdc_ncm: update datagram size after changing mtu

[Robert Dobrowolski]

From: Rafal Redzimski 

Current implementation updates the mtu size and notify cdc_ncm
device using USB_CDC_SET_MAX_DATAGRAM_SIZE request about datagram
size change instead of changing rx_urb_size.

Whenever mtu is being changed, datagram size should also be
updated.

Cc: 
Signed-off-by: Rafal Redzimski 
Signed-off-by: Robert Dobrowolski 
---
 drivers/net/usb/cdc_ncm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index 2fb31ed..4a47656 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -746,6 +746,8 @@ int cdc_ncm_change_mtu(struct net_device *net, int new_mtu)
if (new_mtu <= 0 || new_mtu > maxmtu)
return -EINVAL;
net->mtu = new_mtu;
+   cdc_ncm_set_dgram_size(dev, new_mtu);
+
return 0;
 }
 EXPORT_SYMBOL_GPL(cdc_ncm_change_mtu);
-- 
1.9.1

--
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] usb: dwc3: add debugfs node to dump FIFO/Queue available space

[Du, Changbin]

> > This can be used to check some special issues, like whether data is
> > successfully copied from memory to fifo when a trb is blocked.
> >
> > Signed-off-by: Du, Changbin 
> > ---
> >  drivers/usb/dwc3/core.h|  5 +
> >  drivers/usb/dwc3/debugfs.c | 45
> +
> >  2 files changed, 50 insertions(+)
> 
> Why did you not include the linux-usb@vger mailing list?
> 
Just forget it :)

> >  static int dwc3_testmode_show(struct seq_file *s, void *unused)
> >  {
> > struct dwc3 *dwc = s->private;
> > @@ -648,6 +687,12 @@ int dwc3_debugfs_init(struct dwc3 *dwc)
> > goto err1;
> > }
> >
> > +   file = debugfs_create_file("fifo", S_IRUGO, root, dwc,
> _fifo_fops);
> > +   if (!file) {
> > +   ret = -ENOMEM;
> 
> Um, no, that's not the error here.  You shouldn't care at all about
> debugfs api call results.  Just keep moving on here please.
> 
> thanks,
> 
> greg k-h

Agree with you. I will create another patch to cleanup this piece of code. 
And I found a memory leak issue there, dwc->regset never released. Will also 
fix it.

Thanks,
Du, Changbin

--
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 v9 1/4] gadget: Introduce the usb charger framework

[Baolin Wang]

On 6 April 2016 at 16:26, Jun Li  wrote:
> Hi
>
>> + */
>> +static enum usb_charger_type
>> +usb_charger_get_type_by_others(struct usb_charger *uchger) {
>> + if (uchger->type != UNKNOWN_TYPE)
>> + return uchger->type;
>> +
>> + if (uchger->psy) {
>> + union power_supply_propval val;
>> +
>> + power_supply_get_property(uchger->psy,
>> +   POWER_SUPPLY_PROP_CHARGE_TYPE,
>> +   );
>> + switch (val.intval) {
>> + case POWER_SUPPLY_TYPE_USB:
>> + uchger->type = SDP_TYPE;
>> + break;
>> + case POWER_SUPPLY_TYPE_USB_DCP:
>> + uchger->type = DCP_TYPE;
>> + break;
>> + case POWER_SUPPLY_TYPE_USB_CDP:
>> + uchger->type = CDP_TYPE;
>> + break;
>> + case POWER_SUPPLY_TYPE_USB_ACA:
>> + uchger->type = ACA_TYPE;
>> + break;
>> + default:
>> + uchger->type = UNKNOWN_TYPE;
>> + break;
>> + }
>> + } else if (uchger->get_charger_type) {
>> + uchger->type = uchger->get_charger_type(uchger);
>> + } else {
>> + uchger->type = UNKNOWN_TYPE;
>> + }
>> +
>> + return uchger->type;
>> +}
>> +
>
> I think we may don't need this usb_charger_get_type_by_others().
> "uchger->type" is set in one place is enough, that is: by
> uchger->charger_detect() in usb_charger_detect_type(), then
> usb_charger_get_type_by_others() is replaced by usb_charger_get_type().
>
> uchger->charger_detect() can have diff implementations no matter
> what kind of mechanism is used, for your PMIC case, you can just
> directly get the type value by power_supply_get_property();
> with that, we can have one central place to set uchger->type.
> After uchger->type is set, charger type detection is no need to be
> involved until charger type changes.
>
> Then next question is where is to call usb_charger_detect_type(),
> We need make sure it finished before usb gadget connect.

Yeah, that's the point: where? It is hard for usb charger framework to
control, which will make it more complicated. The
'usb_charger_detect_type()' is used for detecting the charger type
manually on user's platform, and user should call it at the right time
to avoid affecting gadget enumeration. Otherwise user can implement
some callbacks showed in 'usb_charger_get_type_by_others()' function
to get charger type. I think this is controllable and simple for the
usb charger framework.

>
> Ideal is with your framework, diff users only need implement
> uchger->charger_detect(). :)
>

-- 
Baolin.wang
Best Regards
--
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: dwc3: wakeup request details (was: Re: [PATCH 2/2] usb: dwc3: gadget: put link to U0 before Start Transfer)

[Felipe Balbi]

Hi John,

Felipe Balbi  writes:
> Felipe Balbi  writes:
>> Synopsys Databook says we should move link to U0
>> before issuing a Start Transfer command. We could
>> require the gadget driver to call
>> usb_gadget_wakeup() however I feel that changing all
>> gagdget drivers to keep track of Link State and
>> conditionally call usb_gadget_wakeup() would be far
>> too much work. For now we will handle this at the
>> UDC level, but at some point composite.c should be
>> one handling this.
>>
>> Signed-off-by: Felipe Balbi 
>
> I have been looking at this for a while but I have two question which I
> can't seem to answer using Synopsys databook (I have here 2.60a).
>
> i) When is it legal to start Wakeup request ?
>
> ii) What is the proper way to start wakeup request ?
>
> IIRC current code was very much extracted from a BSD reference code
> which was sent to me (while back at TI) years ago and now I have been
> wondering if current code is actually correct or not. I can't find
> answers in the databook. Everytime I look for 'wakeup' on the databook
> it's always along the lines of "SW can issue wakeup request" but there's
> never a "here's how you start wakeup request and here's when is it legal
> to do so" section.
>
> Would you be able to request Synopsys to clarify this in the Databook ?
> (I know I'm using an older version of the databook, but I really don't
> have anything else right now, sorry).

A gentle ping about this question ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Baolin Wang]

On 6 April 2016 at 15:19, Peter Chen  wrote:
> On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote:
>>
>> @@ -563,6 +564,8 @@ struct usb_gadget_ops {
>>   struct usb_ep *(*match_ep)(struct usb_gadget *,
>>   struct usb_endpoint_descriptor *,
>>   struct usb_ss_ep_comp_descriptor *);
>> + /* get the charger type */
>> + enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
>>  };
>
> Since we already have get_charger_type callback at usb_charger
> structure, why we still need this API at usb_gadget_ops?

In case some users want to get charger type at gadget level.

>
>>
>>  /**
>> @@ -635,6 +638,8 @@ struct usb_gadget {
>>   unsignedout_epnum;
>>   unsignedin_epnum;
>>   struct usb_otg_caps *otg_caps;
>> + /* negotiate the power with the usb charger */
>> + struct usb_charger  *charger;
>>
>>   unsignedsg_supported:1;
>>   unsignedis_otg:1;
>> @@ -839,10 +844,20 @@ static inline int usb_gadget_vbus_connect(struct 
>> usb_gadget *gadget)
>>   * reporting how much power the device may consume.  For example, this
>>   * could affect how quickly batteries are recharged.
>>   *
>> + * It will also notify the USB charger how much power the device may
>> + * consume if there is a USB charger linking with the gadget.
>> + *
>>   * Returns zero on success, else negative errno.
>>   */
>>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned 
>> mA)
>>  {
>> + enum usb_charger_type type;
>> +
>> + if (gadget->charger) {
>> + type = usb_charger_get_type(gadget->charger);
>> + usb_charger_set_cur_limit_by_type(gadget->charger, type, mA);
>> + }
>> +
>
> You may do something redundant.
>
> - Charger detection only occurs at connection period, at other periods,
> we only change the current limit, and notify charger IC. That is to
> say, we may only need to save charger type and current limit at
> usb_charger structure, we don't need to distinguish all chargers
> type from time to time.

That's right. I just want to get the charger type as one parameter to
set current. The function is implemented as below:

enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger)
{
enum usb_charger_type type;

mutex_lock(>lock);
type = uchger->type;
mutex_unlock(>lock);

return type;
}

>
> - The purpose of usb_gadget_vbus_draw design is notify charger IC too,
> so you can do set current limit and notify charger IC together at this
> API together, it has already covered all situations. We don't need to
> notify charger IC again when the gadget status has changed again.

It did not notify charger IC again. You are right,
usb_gadget_vbus_draw design will notify charger IC, so we want to
record the current in usb charger framework at the same time.

>
>>   if (!gadget->ops->vbus_draw)
>>   return -EOPNOTSUPP;
>>   return gadget->ops->vbus_draw(gadget, mA);
>> --
>> 1.7.9.5
>>
>> --
>> 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
>
> --
>
> Best Regards,
> Peter Chen



-- 
Baolin.wang
Best Regards
--
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: [Linux-v4.6-rc2] atkbd serio0: Unknown key pressed (translated set 2, code 0xa8 on isa0060/serio0).

[Sedat Dilek]

On Wed, Apr 6, 2016 at 12:33 PM, Jiri Kosina  wrote:
> On Wed, 6 Apr 2016, Sedat Dilek wrote:
>
>> >> I cannot use cursor up and down on my notebook and see this in my dmesg.
>> >>
>> >> [  685.425634] atkbd serio0: Unknown key pressed (translated set 2,
>> >> code 0xa8 on isa0060/serio0).
>> >> [  685.425648] atkbd serio0: Use 'setkeycodes e028 ' to make it 
>> >> known.
>> >>
>> >> Known issue?
>> >>
>> >> Do you need more input?
>> >>
>> >> Linux-config and dmesg-output attached.
>> >>
>> >> Regards,
>> >> - Sedat Dilek -
>> >
>> > Attached is output of...
>> >
>> >$ xmodmap -pke > /tmp/xmodmap-pke.txt
>> >
>>
>> [ CC (USB)HID folks ]
>>
>> I forgot to tell that I have some patches on top of vanilla Linux v4.6-rc2.
>>
>> Dropping the patches from  makes
>> the issue go away.
>
> Thanks for the useful report!
>
>> Unsure what is the cause. On the way, try to look this evening or
>> tommorrow into this.
>
> I don't seem to see the beginning of this thread anywhere.
>
> What hardware is this?
>

OK, OK.
As said I am on the run - my train is going in 40mins-
I try to write some better report.

- Sedat -
--
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: [Linux-v4.6-rc2] atkbd serio0: Unknown key pressed (translated set 2, code 0xa8 on isa0060/serio0).

[Jiri Kosina]

On Wed, 6 Apr 2016, Sedat Dilek wrote:

> >> I cannot use cursor up and down on my notebook and see this in my dmesg.
> >>
> >> [  685.425634] atkbd serio0: Unknown key pressed (translated set 2,
> >> code 0xa8 on isa0060/serio0).
> >> [  685.425648] atkbd serio0: Use 'setkeycodes e028 ' to make it 
> >> known.
> >>
> >> Known issue?
> >>
> >> Do you need more input?
> >>
> >> Linux-config and dmesg-output attached.
> >>
> >> Regards,
> >> - Sedat Dilek -
> >
> > Attached is output of...
> >
> >$ xmodmap -pke > /tmp/xmodmap-pke.txt
> >
> 
> [ CC (USB)HID folks ]
> 
> I forgot to tell that I have some patches on top of vanilla Linux v4.6-rc2.
> 
> Dropping the patches from  makes
> the issue go away.

Thanks for the useful report!

> Unsure what is the cause. On the way, try to look this evening or 
> tommorrow into this.

I don't seem to see the beginning of this thread anywhere.

What hardware is this?

-- 
Jiri Kosina
SUSE Labs

--
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: [Linux-v4.6-rc2] atkbd serio0: Unknown key pressed (translated set 2, code 0xa8 on isa0060/serio0).

[Sedat Dilek]

On Wed, Apr 6, 2016 at 11:35 AM, Sedat Dilek  wrote:
> On Wed, Apr 6, 2016 at 11:27 AM, Sedat Dilek  wrote:
>> Hi,
>>
>> I cannot use cursor up and down on my notebook and see this in my dmesg.
>>
>> [  685.425634] atkbd serio0: Unknown key pressed (translated set 2,
>> code 0xa8 on isa0060/serio0).
>> [  685.425648] atkbd serio0: Use 'setkeycodes e028 ' to make it 
>> known.
>>
>> Known issue?
>>
>> Do you need more input?
>>
>> Linux-config and dmesg-output attached.
>>
>> Regards,
>> - Sedat Dilek -
>
> Attached is output of...
>
>$ xmodmap -pke > /tmp/xmodmap-pke.txt
>

[ CC (USB)HID folks ]

I forgot to tell that I have some patches on top of vanilla Linux v4.6-rc2.

Dropping the patches from  makes
the issue go away.
Unsure what is the cause.
On the way, try to look this evening or tommorrow into this.

- Sedat -
--
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 v9 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

Hi,

Peter Chen  writes:
> On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
>> Peter Chen  writes:
>> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>> >> Peter Chen  writes:
>> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> >> >  +
>> >> >> +static struct attribute *usb_charger_attrs[] = {
>> >> >> +  _attr_sdp_current.attr,
>> >> >> +  _attr_dcp_current.attr,
>> >> >> +  _attr_cdp_current.attr,
>> >> >> +  _attr_aca_current.attr,
>> >> >> +  _attr_charger_type.attr,
>> >> >> +  _attr_charger_state.attr,
>> >> >> +  NULL
>> >> >> +};
>> >> >
>> >> > The user may only care about current limit, type and state, why they
>> >> > need to care what type's current limit, it is the usb charger
>> >> > framework handles, the framework judge the current according to
>> >> > charger type and USB state (connect/configured/suspended).
>> >> 
>> >> it might be useful if we want to know that $this charger doesn't really
>> >> give us as much current as it advertises.
>> >> 
>> >
>> > As my understanding, the current limit is dynamic value, it should
>> > report the value the charger supports now, eg, it connects SDP, but
>> > the host is suspended now, then the value should be 2mA.
>> 
>> yes, and that's the limit. Now consider we connect to DCP or CDP and
>> limit is 2000mA but we're charging at 1000mA ;-)
>> 
>
> The user doesn't need to know the value which spec designs.

because... ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

Hi,

Peter Chen  writes:
> On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
>> Peter Chen  writes:
>> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>> >> Peter Chen  writes:
>> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> >> >  +
>> >> >> +static struct attribute *usb_charger_attrs[] = {
>> >> >> +  _attr_sdp_current.attr,
>> >> >> +  _attr_dcp_current.attr,
>> >> >> +  _attr_cdp_current.attr,
>> >> >> +  _attr_aca_current.attr,
>> >> >> +  _attr_charger_type.attr,
>> >> >> +  _attr_charger_state.attr,
>> >> >> +  NULL
>> >> >> +};
>> >> >
>> >> > The user may only care about current limit, type and state, why they
>> >> > need to care what type's current limit, it is the usb charger
>> >> > framework handles, the framework judge the current according to
>> >> > charger type and USB state (connect/configured/suspended).
>> >> 
>> >> it might be useful if we want to know that $this charger doesn't really
>> >> give us as much current as it advertises.
>> >> 
>> >
>> > As my understanding, the current limit is dynamic value, it should
>> > report the value the charger supports now, eg, it connects SDP, but
>> > the host is suspended now, then the value should be 2mA.
>> 
>> yes, and that's the limit. Now consider we connect to DCP or CDP and
>> limit is 2000mA but we're charging at 1000mA ;-)
>> 
>
> Does the user need to know the $this charger limit? Don't they only
> care about the current charging value? I have a USB cable which can

Why not ? UI might want to change the color of the battery charging icon
if we're charging @ 2000mA or @ 1000mA to give some visual feedback as
to "how fast" battery is supposed to be charged.

> show charging current value, it changes from time to time, when it
> connects to host pc, it shows 430mA; when it connects to dedicated
> charger, it shows 1000mA.

good for you, now what does that have to do with $subject ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
>> On 03/11/2016 07:57 AM, Greg Kroah-Hartman wrote:
>> > On Thu, Mar 10, 2016 at 01:39:43PM +0100, Oliver Neukum wrote:
>> >> On Tue, 2016-03-08 at 15:53 +0800, Lu Baolu wrote:
>> >>
>> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-platform 
>> >>> b/Documentation/ABI/testing/sysfs-bus-platform
>> >>> index 5172a61..a2261cb 100644
>> >>> --- a/Documentation/ABI/testing/sysfs-bus-platform
>> >>> +++ b/Documentation/ABI/testing/sysfs-bus-platform
>> >>> @@ -18,3 +18,18 @@ Description:
>> >>>  devices to opt-out of driver binding using a 
>> >>> driver_override
>> >>>  name such as "none".  Only a single driver may be 
>> >>> specified in
>> >>>  the override, there is no support for parsing 
>> >>> delimiters.
>> >>> +
>> >>> +What:   /sys/bus/platform/devices/.../intel_mux
>> >> Hi,
>> >>
>> >> is there any reason to call this "intel_mux"? We want a common interface
>> >> for such things. So how about "role_mux" or "master_slave_mux"?
>> > I agree, don't make this intel specific, as it shouldn't be.

right, we can easily change the name. What happens when
e.g. AMD/NVidia/TI/MediaTek/Allwinner/etc come up with a similar setup
which can't be supported by Intel's mux driver ? (see more below,
agreeing on a name here doesn't seem to be enough)

>> By the way, do you expect a class for port mux in sysfs?
>
> Why would you create a class?  What is that going to do here?

well, you said you didn't want this to be Intel-specific. We could just
change the name and remove "intel_" from it, but then we would fall into
this situation where possibly different mux drivers might be
needed. That almost requires some small abstraction between sysfs and
the actual implementation, does it not ?

A bus_type doesn't fit here and a simple 'agreement' that mux drivers
will create a sysfs file named 'mux' is too fragile. Thus...

> What happened to getting internal help in designing this api?  This
> shouldn't be my job :)

I looked at this Baolu but, at least to me, it's unclear what you're
hinting here. Sure, the API is a bit odd in that we register a mux and
unregister a device (that has been fixed), but is there anything else ?

We're not asking that you design the API for us, rather just make your
comments a little clearer.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Jacek Anaszewski]

On 04/06/2016 10:52 AM, Pavel Machek wrote:

Hi!


Lets say we have

/sys/class/pattern/lp5533::0
/sys/class/pattern/software::0

/sys/class/led/n900::red ; default trigger "lp5533::0:0"
/sys/class/led/n900::green ; default trigger "lp5533::0:1"
/sys/class/led/n900::blue ; default trigger "lp5533::0:2"

Normally, pattern would correspond to one RGB LED. We could have
attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
this pattern.


Could you give an example on how to set a color for RGB LED using
this interface? Would it be compatible with LED triggers?
Where the "pattern" class would be implemented?


Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
set the color for the led. 'echo "trigger-name" > trigger' would set
the trigger, probably just toggling between LED off and set color for
the old triggers.

Where to implement the patterns is different question, but for example
drivers/leds/pattern?


I'd rather leave the pattern issue for now, since it seems to be
different from the problem Heiner was trying to solve with his LED RGB
extension. Moreover, hardware patterns are device specific and it could
be hard to propose a generic interface.


Well, RGB leds are basically useless without pattern support. And I
believe we can do generic interface.


Drivers can always expose their custom sysfs attributes for configuring
the patterns.

Regardless of the above, some of your considerations brought me an idea
on how to add generic and backwards compatible support for setting RGB
color at one go.

Currently LED class drivers of RGB LED controllers expose three LED
class devices - one per R, G and B color component. I propose that
such drivers set LED_DEV_CAP_RGB flag for each LED class device they
register. LED core, seeing the flag, would create a generic "color"
sysfs attribute for each of the three LED class devices.

The "color" attribute would contain "R G B" values. Setting the "color"
attribute of any of the three LED class devices would affect brightness
properties (i.e. constituent colors) of the remaining two ones.
It would result in disabling any active triggers and writing all the
three color settings to the RGB LED controller at one go.


Having one attribute across three devices is rather ugly. And we'll
need to solve the pattern issue one day.

What's tricky about patterns is that you need to control 3 (or more)
leds at a time. Problem you are trying to solve here is ... control of
3 leds, at the same time.

So let's solve them together.


OK, now I've got your point. So we'd need to have a means for defining
patterns. The interface could be located at /sys/class/leds/patterns.

We'd need to have a flexible way for defining LED class devices involved
in a pattern. Since we cannot guarantee no space in a LED class device
name, then a single attribute containing space separated list is not an
option. We'd have to create a predefined set of attributes that would
contain LED class device name. Predefined implies that it would be
a fixed number, i.e. either some attributes would always remain unused
or, which is even worse, we could run out of free attributes for some
use cases.

The same constraints would appear if we wanted to be able to define
more than one pattern.

It would be best to work out more flexible solution. I wonder if
ioctl interface isn't the only option.

--
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: raid 5 creation fails on usb3 connected drives kernel 4.4.x, 4.5

[Greg KH]

On Tue, Apr 05, 2016 at 06:42:51PM +1000, Brian Chadwick wrote:
> SETUP:
> 
> i7 16GB Computer.
> 
> 1 x PCI-x USB3 adaptor card (i think uses xhci-hcd)04:00.0 USB controller:
> Renesas Technology Corp. uPD720202 USB 3.0 Host Controller (rev 02)
> Kernel driver in use: xhci_hcd
> 
> 2 x USB3 to dual SATA HDD docks. uses JMicron JMS56x Series controllers,
> uses uas.ko kernel module
> Bus 004 Device 002: ID 152d:9561 JMicron Technology Corp. / JMicron USA
> Technology Corp.
> Bus 004 Device 003: ID 152d:9561 JMicron Technology Corp. / JMicron USA
> Technology Corp.
> 
> 3 x Western Digital 320GB 3.5" SATA drives
> 
> USE:
> 
> RAID 5
> 
> KERNEL:
> 4.3.5
> 
> System works perfectly as expected.
> 
> 
> Change to kernel 4.4.x or 4.5 and RAID 5 doesnt work. I am not sure whether
> its actually RAID 5 at fault or if its the USB Attached SCSI driver uas.ko,
> or maybe the host driver xhci-hcd.

Can you run 'git bisect' to try to track down the offending commit?

thanks,

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

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Wed, Apr 06, 2016 at 01:58:52PM +0800, Lu Baolu wrote:
> Hi,
> 
> On 03/11/2016 07:57 AM, Greg Kroah-Hartman wrote:
> > On Thu, Mar 10, 2016 at 01:39:43PM +0100, Oliver Neukum wrote:
> >> On Tue, 2016-03-08 at 15:53 +0800, Lu Baolu wrote:
> >>
> >>> diff --git a/Documentation/ABI/testing/sysfs-bus-platform 
> >>> b/Documentation/ABI/testing/sysfs-bus-platform
> >>> index 5172a61..a2261cb 100644
> >>> --- a/Documentation/ABI/testing/sysfs-bus-platform
> >>> +++ b/Documentation/ABI/testing/sysfs-bus-platform
> >>> @@ -18,3 +18,18 @@ Description:
> >>>   devices to opt-out of driver binding using a driver_override
> >>>   name such as "none".  Only a single driver may be specified in
> >>>   the override, there is no support for parsing delimiters.
> >>> +
> >>> +What:/sys/bus/platform/devices/.../intel_mux
> >> Hi,
> >>
> >> is there any reason to call this "intel_mux"? We want a common interface
> >> for such things. So how about "role_mux" or "master_slave_mux"?
> > I agree, don't make this intel specific, as it shouldn't be.
> >
> 
> By the way, do you expect a class for port mux in sysfs?

Why would you create a class?  What is that going to do here?

What happened to getting internal help in designing this api?  This
shouldn't be my job :)

thanks,

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

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Greg Kroah-Hartman]

On Wed, Apr 06, 2016 at 02:44:55PM +0800, Lu Baolu wrote:
> Hi,
> 
> On 03/14/2016 11:27 AM, Greg Kroah-Hartman wrote:
> > On Mon, Mar 14, 2016 at 09:09:22AM +0800, Lu Baolu wrote:
> >> On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
> >>> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
>  +struct intel_mux_dev {
>  +struct device   *dev;
>  +char*extcon_name;
>  +char*cable_name;
>  +int (*cable_set_cb)(struct intel_mux_dev *mux);
>  +int (*cable_unset_cb)(struct intel_mux_dev *mux);
>  +};
> >>> This is a device, why not make it one?  Don't just hold a reference.
> >>> And do you really even hold that reference?
> >> It's not a device. It's just an encapsulation for parameters passed into
> >> intel_usb_mux_register().
> > But you called it a device, so you can understand my confusion.
> >
> > And why not make it a device?  Why isn't this one?  Hint, I really think
> > it should be...
> >
> >>> And your api is horrid, think about what you want the "core" to do here,
> >>> it should be the one creating the device and returning it to the caller,
> >>> not forcing the caller to somehow create it first and then pass it in.
> >> This isn't a layer or core. It doesn't create any new devices. It's 
> >> actually
> >> some shared code which can be used by all Intel dual role port drivers.
> > It should be a device, as you are treating it like one :)
> >
> >> I put it in a separated file because 1) this can avoid duplication; 2) 
> >> this code
> >> could be used for any architectures as long as a USB port is shared by
> >> two components and it needs an OS response when event triggers.
> > It's a bit hard for other arches to be using something called "intel_"
> > :(
> 
> Are there any other implementations which need an external mux
> to swap the usb roles?

Why wouldn't there be?  :)

> >> I guess intel_usb_mux_register/unregister() is a bit misleading. How about
> >> changing them to intel_usb_mux_probe/remove()?
> > You are going to probe/remove something that isn't a device?  Come on
> > now...
> >
> >>> And why is it not symmetrical, you are passing one thing into register
> >>> and another into unregister.
> >> struct intel_mux_dev is an encapsulation for parameters passed into
> >> intel_usb_mux_register().
> > Which is a device.
> >
> >> It's not a new device structure though the name
> >> is a bit misleading.
> > Yes it is, hint, you want it to be a device.
> >
> >> How about remove this structure and put these in function parameters?
> > How about making it a real device? :)
> 
> Hi Greg,
> 
> Just want to make myself clear about your expectation.
> Did you mean to create a port mux device and return it to the caller?

Yes, that's the correct way to do this type of thing.

> The interfaces look like:
> 
> struct portmux_dev *devm_portmux_register(struct device *dev,
>   const struct portmux_desc *desc);
> 
> void devm_portmux_unregister(struct device *dev,
>struct portmux_dev *pdev)
> 
> Do I get it right?

Why devm?  Your lifetime rules don't allow that at all.

thanks,

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >As I see it the current blinking support then would be one special case of a 
> >pattern.
> >As a consequence once having pattern support we might be able to switch 
> >users of blinking
> >to pattern and remove the blinking support.
> 
> Let's split patterns related discussion into a separate thread.
> It would be best if it began with a patch.

Lets design userland interface first, then decide how to implement it
in the kernel. Patches are useless at this point.

And actually... without patterns, existing interface works just
fine. Even if you can't "atomically" write values to three different
files, operation is so fast that user will not see the intermediate
state, anyway. So solving the "atomic" issue without solving the rest
is pretty much useless.

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> > We would probably need additional op in the LED core : color_set.
> > 
> > Having the color set to nonzero value would signify the the three LED
> > class devices are in sync and that setting a trigger on any of them
> > applies to the remaining two ones. It would have to be considered
> > whether existing triggers could be made compatible with synchronized
> > RGB LED class devices.
> > 
> > I'm curious what do you think about the idea.
> > 
> > Pavel, Heiner, others?
> > 
> Exposing "coupled LED devices" as separate LED devices most likely is ok
> when accessed from user space as the name of the led_classdev's indicates
> that they belong together.
> But how about a trigger wanting to set a RGB LED to a specific color?
> (That's not available yet but one possible use case for RGB LED's)
> A trigger is bound to a led_classdev currently. In addition we'd need
> to introduce some kind of super_led_classdev having links to the respective
> R/G/B led_classdev's (+ trigger functions dealing with this 
> super_led_classdev).
> 
> These changes / extensions are not needed if a RGB LED is exposed as one
> led_classdev, just with flag LED_DEV_CAP_RGB set.
> OK, we'd still have to change the sysfs interface as obviously setting
> hue/sat/brightness via one "brightness" attribute is not acceptable.
> However this constraint might not affect the kernel-internal trigger API
> (usage of parameter brightness in led_trigger_event).

Your proposal would break existing hardware. We already have RGB LEDs
exposed as three LEDs. It is too late to change interface there.

> I see Pavel's point that there might be different types of multi-color LED's.
> At least we have:
> - multi-color LED's where each single LED is visible even if all are switched 
> on
> - multi-color LED's like RGB LED's where you usually just see a
> uniform color

Well, I suggest we ignore that distinction. Yes, I can see different
colors coming from different directions, but the LED was clearly
designed to look like single light. 

> Last but not least regarding the patterns:
> Something like proposed by Pavel is e.g. (partially) supported by the blink(1)
> firmware. That would be an example of such a "hardware-accelerated" pattern.
> 
> As I see it the current blinking support then would be one special case of a 
> pattern.
> As a consequence once having pattern support we might be able to switch users 
> of blinking
> to pattern and remove the blinking support.

No, you can't remove existing blinking support, due to backwards
compatibility reasons.

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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 v9 1/4] gadget: Introduce the usb charger framework

[Jun Li]

Hi

> -Original Message-
> From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
> ow...@vger.kernel.org] On Behalf Of Baolin Wang
> Sent: Friday, April 01, 2016 3:22 PM
> To: ba...@kernel.org; gre...@linuxfoundation.org; s...@kernel.org;
> dbarysh...@gmail.com; dw...@infradead.org
> Cc: peter.c...@freescale.com; st...@rowland.harvard.edu;
> r.bald...@samsung.com; yoshihiro.shimoda...@renesas.com;
> lee.jo...@linaro.org; broo...@kernel.org;
> ckee...@opensource.wolfsonmicro.com; patc...@opensource.wolfsonmicro.com;
> baolin.w...@linaro.org; linux...@vger.kernel.org; linux-
> u...@vger.kernel.org; device-mainlin...@lists.linuxfoundation.org; linux-
> ker...@vger.kernel.org
> Subject: [PATCH v9 1/4] gadget: Introduce the usb charger framework
> 

...

> +/*
> + * usb_charger_get_type() - get the usb charger type with lock protection.
> + * @uchger - usb charger device
> + *
> + * Users can get the charger type by this safe API, rather than using
> +the
> + * usb_charger structure directly.
> + */
> +enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger)
> +{
> + enum usb_charger_type type;
> +
> + mutex_lock(>lock);
> + type = uchger->type;
> + mutex_unlock(>lock);
> +
> + return type;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get_type);
> +
> +/*
> + * usb_charger_detect_type() - detect the charger type manually.
> + * @uchger - usb charger device
> + *
> + * Note: You should ensure you need to detect the charger type manually
> +on your
> + * platform.
> + * You should call it at the right gadget state to avoid affecting
> +gadget
> + * enumeration.
> + */
> +int usb_charger_detect_type(struct usb_charger *uchger) {
> + enum usb_charger_type type;
> +
> + if (WARN(!uchger->charger_detect,
> +  "charger detection method should not be NULL"))
> + return -EINVAL;
> +
> + type = uchger->charger_detect(uchger);
> +
> + mutex_lock(>lock);
> + uchger->type = type;
> + mutex_unlock(>lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_detect_type);
> +
> +/*
> + * usb_charger_get_type_by_others() - Get the usb charger type by the
> +callback
> + * which is implemented by users.
> + * @uchger - the usb charger device.
> + *
> + * Note: This function is just used for getting the charger type, not
> +for
> + * detecting charger type which might affect the DP/DM line when gadget
> +is on
> + * enumeration state.
> + */
> +static enum usb_charger_type
> +usb_charger_get_type_by_others(struct usb_charger *uchger) {
> + if (uchger->type != UNKNOWN_TYPE)
> + return uchger->type;
> +
> + if (uchger->psy) {
> + union power_supply_propval val;
> +
> + power_supply_get_property(uchger->psy,
> +   POWER_SUPPLY_PROP_CHARGE_TYPE,
> +   );
> + switch (val.intval) {
> + case POWER_SUPPLY_TYPE_USB:
> + uchger->type = SDP_TYPE;
> + break;
> + case POWER_SUPPLY_TYPE_USB_DCP:
> + uchger->type = DCP_TYPE;
> + break;
> + case POWER_SUPPLY_TYPE_USB_CDP:
> + uchger->type = CDP_TYPE;
> + break;
> + case POWER_SUPPLY_TYPE_USB_ACA:
> + uchger->type = ACA_TYPE;
> + break;
> + default:
> + uchger->type = UNKNOWN_TYPE;
> + break;
> + }
> + } else if (uchger->get_charger_type) {
> + uchger->type = uchger->get_charger_type(uchger);
> + } else {
> + uchger->type = UNKNOWN_TYPE;
> + }
> +
> + return uchger->type;
> +}
> +

I think we may don't need this usb_charger_get_type_by_others().
"uchger->type" is set in one place is enough, that is: by
uchger->charger_detect() in usb_charger_detect_type(), then
usb_charger_get_type_by_others() is replaced by usb_charger_get_type().

uchger->charger_detect() can have diff implementations no matter
what kind of mechanism is used, for your PMIC case, you can just
directly get the type value by power_supply_get_property();
with that, we can have one central place to set uchger->type.
After uchger->type is set, charger type detection is no need to be
involved until charger type changes.

Then next question is where is to call usb_charger_detect_type(),
We need make sure it finished before usb gadget connect.

Ideal is with your framework, diff users only need implement
uchger->charger_detect(). :)

Li Jun

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

Re: [PATCH v5 1/4] leds: core: add generic support for RGB Color LED's

[Pavel Machek]

Hi!

> >Lets say we have
> >
> >/sys/class/pattern/lp5533::0
> >/sys/class/pattern/software::0
> >
> >/sys/class/led/n900::red ; default trigger "lp5533::0:0"
> >/sys/class/led/n900::green ; default trigger "lp5533::0:1"
> >/sys/class/led/n900::blue ; default trigger "lp5533::0:2"
> >
> >Normally, pattern would correspond to one RGB LED. We could have
> >attribute "/sys/class/pattern/lp5533::0/color" containing R,G,B for
> >this pattern.
> >>
> >>Could you give an example on how to set a color for RGB LED using
> >>this interface? Would it be compatible with LED triggers?
> >>Where the "pattern" class would be implemented?
> >
> >Well, 'echo "50 60 70" > /sys/class/pattern/lp5533::0/color' should
> >set the color for the led. 'echo "trigger-name" > trigger' would set
> >the trigger, probably just toggling between LED off and set color for
> >the old triggers.
> >
> >Where to implement the patterns is different question, but for example
> >drivers/leds/pattern?
> 
> I'd rather leave the pattern issue for now, since it seems to be
> different from the problem Heiner was trying to solve with his LED RGB
> extension. Moreover, hardware patterns are device specific and it could
> be hard to propose a generic interface.

Well, RGB leds are basically useless without pattern support. And I
believe we can do generic interface.

> Drivers can always expose their custom sysfs attributes for configuring
> the patterns.
> 
> Regardless of the above, some of your considerations brought me an idea
> on how to add generic and backwards compatible support for setting RGB
> color at one go.
> 
> Currently LED class drivers of RGB LED controllers expose three LED
> class devices - one per R, G and B color component. I propose that
> such drivers set LED_DEV_CAP_RGB flag for each LED class device they
> register. LED core, seeing the flag, would create a generic "color"
> sysfs attribute for each of the three LED class devices.
> 
> The "color" attribute would contain "R G B" values. Setting the "color"
> attribute of any of the three LED class devices would affect brightness
> properties (i.e. constituent colors) of the remaining two ones.
> It would result in disabling any active triggers and writing all the
> three color settings to the RGB LED controller at one go.

Having one attribute across three devices is rather ugly. And we'll
need to solve the pattern issue one day.

What's tricky about patterns is that you need to control 3 (or more)
leds at a time. Problem you are trying to solve here is ... control of
3 leds, at the same time.

So let's solve them together.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
--
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 v9 1/4] gadget: Introduce the usb charger framework

[Peter Chen]

On Wed, Apr 06, 2016 at 11:05:26AM +0300, Felipe Balbi wrote:
> Peter Chen  writes:
> > On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
> >> Peter Chen  writes:
> >> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> >> >  +
> >> >> +static struct attribute *usb_charger_attrs[] = {
> >> >> +   _attr_sdp_current.attr,
> >> >> +   _attr_dcp_current.attr,
> >> >> +   _attr_cdp_current.attr,
> >> >> +   _attr_aca_current.attr,
> >> >> +   _attr_charger_type.attr,
> >> >> +   _attr_charger_state.attr,
> >> >> +   NULL
> >> >> +};
> >> >
> >> > The user may only care about current limit, type and state, why they
> >> > need to care what type's current limit, it is the usb charger
> >> > framework handles, the framework judge the current according to
> >> > charger type and USB state (connect/configured/suspended).
> >> 
> >> it might be useful if we want to know that $this charger doesn't really
> >> give us as much current as it advertises.
> >> 
> >
> > As my understanding, the current limit is dynamic value, it should
> > report the value the charger supports now, eg, it connects SDP, but
> > the host is suspended now, then the value should be 2mA.
> 
> yes, and that's the limit. Now consider we connect to DCP or CDP and
> limit is 2000mA but we're charging at 1000mA ;-)
> 

The user doesn't need to know the value which spec designs.

-- 

Best Regards,
Peter Chen
--
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 v9 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

Peter Chen  writes:
> On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
>> Peter Chen  writes:
>> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>> >  +
>> >> +static struct attribute *usb_charger_attrs[] = {
>> >> + _attr_sdp_current.attr,
>> >> + _attr_dcp_current.attr,
>> >> + _attr_cdp_current.attr,
>> >> + _attr_aca_current.attr,
>> >> + _attr_charger_type.attr,
>> >> + _attr_charger_state.attr,
>> >> + NULL
>> >> +};
>> >
>> > The user may only care about current limit, type and state, why they
>> > need to care what type's current limit, it is the usb charger
>> > framework handles, the framework judge the current according to
>> > charger type and USB state (connect/configured/suspended).
>> 
>> it might be useful if we want to know that $this charger doesn't really
>> give us as much current as it advertises.
>> 
>
> As my understanding, the current limit is dynamic value, it should
> report the value the charger supports now, eg, it connects SDP, but
> the host is suspended now, then the value should be 2mA.

yes, and that's the limit. Now consider we connect to DCP or CDP and
limit is 2000mA but we're charging at 1000mA ;-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework

[Peter Chen]

On Wed, Apr 06, 2016 at 10:38:23AM +0300, Felipe Balbi wrote:
> Peter Chen  writes:
> > On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
> >  +
> >> +static struct attribute *usb_charger_attrs[] = {
> >> +  _attr_sdp_current.attr,
> >> +  _attr_dcp_current.attr,
> >> +  _attr_cdp_current.attr,
> >> +  _attr_aca_current.attr,
> >> +  _attr_charger_type.attr,
> >> +  _attr_charger_state.attr,
> >> +  NULL
> >> +};
> >
> > The user may only care about current limit, type and state, why they
> > need to care what type's current limit, it is the usb charger
> > framework handles, the framework judge the current according to
> > charger type and USB state (connect/configured/suspended).
> 
> it might be useful if we want to know that $this charger doesn't really
> give us as much current as it advertises.
> 

As my understanding, the current limit is dynamic value, it should
report the value the charger supports now, eg, it connects SDP, but
the host is suspended now, then the value should be 2mA.

-- 

Best Regards,
Peter Chen
--
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 v9 1/4] gadget: Introduce the usb charger framework

[Felipe Balbi]

Peter Chen  writes:
> On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
>  +
>> +static struct attribute *usb_charger_attrs[] = {
>> +_attr_sdp_current.attr,
>> +_attr_dcp_current.attr,
>> +_attr_cdp_current.attr,
>> +_attr_aca_current.attr,
>> +_attr_charger_type.attr,
>> +_attr_charger_state.attr,
>> +NULL
>> +};
>
> The user may only care about current limit, type and state, why they
> need to care what type's current limit, it is the usb charger
> framework handles, the framework judge the current according to
> charger type and USB state (connect/configured/suspended).

it might be useful if we want to know that $this charger doesn't really
give us as much current as it advertises.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v9 1/4] gadget: Introduce the usb charger framework

[Peter Chen]

On Fri, Apr 01, 2016 at 03:21:49PM +0800, Baolin Wang wrote:
 +
> +static struct attribute *usb_charger_attrs[] = {
> + _attr_sdp_current.attr,
> + _attr_dcp_current.attr,
> + _attr_cdp_current.attr,
> + _attr_aca_current.attr,
> + _attr_charger_type.attr,
> + _attr_charger_state.attr,
> + NULL
> +};

The user may only care about current limit, type and state, why they
need to care what type's current limit, it is the usb charger
framework handles, the framework judge the current according to
charger type and USB state (connect/configured/suspended).

Peter

> +ATTRIBUTE_GROUPS(usb_charger);
> +
> +/*
> + * usb_charger_find_by_name() - Get the usb charger device by name.
> + * @name - usb charger device name.
> + *
> + * return the instance of usb charger device, the device must be
> + * released with usb_charger_put().
> + */
> +struct usb_charger *usb_charger_find_by_name(const char *name)
> +{
> + struct class_dev_iter iter;
> + struct device *dev;
> +
> + if (WARN(!name, "can't work with NULL name"))
> + return ERR_PTR(-EINVAL);
> +
> + /* Iterate all usb charger devices in the class */
> + class_dev_iter_init(, usb_charger_class, NULL, NULL);
> + while ((dev = class_dev_iter_next())) {
> + if (!strcmp(dev_name(dev), name))
> + break;
> + }
> + class_dev_iter_exit();
> +
> + if (WARN(!dev, "can't find usb charger device"))
> + return ERR_PTR(-ENODEV);
> +
> + return dev_to_uchger(dev);
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_find_by_name);
> +
> +/*
> + * usb_charger_get() - Reference a usb charger.
> + * @uchger - usb charger
> + */
> +struct usb_charger *usb_charger_get(struct usb_charger *uchger)
> +{
> + return (uchger && get_device(>dev)) ? uchger : NULL;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get);
> +
> +/*
> + * usb_charger_put() - Dereference a usb charger.
> + * @uchger - charger to release
> + */
> +void usb_charger_put(struct usb_charger *uchger)
> +{
> + if (uchger)
> + put_device(>dev);
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_put);
> +
> +/*
> + * usb_charger_get_type() - get the usb charger type with lock protection.
> + * @uchger - usb charger device
> + *
> + * Users can get the charger type by this safe API, rather than using the
> + * usb_charger structure directly.
> + */
> +enum usb_charger_type usb_charger_get_type(struct usb_charger *uchger)
> +{
> + enum usb_charger_type type;
> +
> + mutex_lock(>lock);
> + type = uchger->type;
> + mutex_unlock(>lock);
> +
> + return type;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_get_type);
> +
> +/*
> + * usb_charger_detect_type() - detect the charger type manually.
> + * @uchger - usb charger device
> + *
> + * Note: You should ensure you need to detect the charger type manually on 
> your
> + * platform.
> + * You should call it at the right gadget state to avoid affecting gadget
> + * enumeration.
> + */
> +int usb_charger_detect_type(struct usb_charger *uchger)
> +{
> + enum usb_charger_type type;
> +
> + if (WARN(!uchger->charger_detect,
> +  "charger detection method should not be NULL"))
> + return -EINVAL;
> +
> + type = uchger->charger_detect(uchger);
> +
> + mutex_lock(>lock);
> + uchger->type = type;
> + mutex_unlock(>lock);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_charger_detect_type);
> +
> +/*
> + * usb_charger_get_type_by_others() - Get the usb charger type by the 
> callback
> + * which is implemented by users.
> + * @uchger - the usb charger device.
> + *
> + * Note: This function is just used for getting the charger type, not for
> + * detecting charger type which might affect the DP/DM line when gadget is on
> + * enumeration state.
> + */
> +static enum usb_charger_type
> +usb_charger_get_type_by_others(struct usb_charger *uchger)
> +{
> + if (uchger->type != UNKNOWN_TYPE)
> + return uchger->type;
> +
> + if (uchger->psy) {
> + union power_supply_propval val;
> +
> + power_supply_get_property(uchger->psy,
> +   POWER_SUPPLY_PROP_CHARGE_TYPE,
> +   );
> + switch (val.intval) {
> + case POWER_SUPPLY_TYPE_USB:
> + uchger->type = SDP_TYPE;
> + break;
> + case POWER_SUPPLY_TYPE_USB_DCP:
> + uchger->type = DCP_TYPE;
> + break;
> + case POWER_SUPPLY_TYPE_USB_CDP:
> + uchger->type = CDP_TYPE;
> + break;
> + case POWER_SUPPLY_TYPE_USB_ACA:
> + uchger->type = ACA_TYPE;
> + break;
> + default:
> + uchger->type = UNKNOWN_TYPE;
> + break;
> + }
> + } else if (uchger->get_charger_type) {
> + uchger->type = 

Re: [PATCH v9 2/4] gadget: Support for the usb charger framework

[Peter Chen]

On Fri, Apr 01, 2016 at 03:21:50PM +0800, Baolin Wang wrote:
> For supporting the usb charger, it adds the usb_charger_init() and
> usb_charger_exit() functions for usb charger initialization and exit.
> 
> It will report to the usb charger when the gadget state is changed,
> then the usb charger can do the power things.
> 
> Introduce a callback 'get_charger_type' which will implemented by
> user for usb gadget operations to get the usb charger type.
> 
> Signed-off-by: Baolin Wang 
> ---
>  drivers/usb/gadget/udc/udc-core.c |   11 +++
>  include/linux/usb/gadget.h|   15 +++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/usb/gadget/udc/udc-core.c 
> b/drivers/usb/gadget/udc/udc-core.c
> index b86a6f0..8d98c6b 100644
> --- a/drivers/usb/gadget/udc/udc-core.c
> +++ b/drivers/usb/gadget/udc/udc-core.c
> @@ -28,6 +28,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /**
>   * struct usb_udc - describes one usb device controller
> @@ -230,6 +231,9 @@ static void usb_gadget_state_work(struct work_struct 
> *work)
>   struct usb_gadget *gadget = work_to_gadget(work);
>   struct usb_udc *udc = gadget->udc;
>  
> + /* when the gadget state is changed, then report to USB charger */
> + usb_charger_plug_by_gadget(gadget, gadget->state);
> +
>   if (udc)
>   sysfs_notify(>dev.kobj, NULL, "state");
>  }
> @@ -423,8 +427,14 @@ int usb_add_gadget_udc_release(struct device *parent, 
> struct usb_gadget *gadget,
>  
>   mutex_unlock(_lock);
>  
> + ret = usb_charger_init(gadget);
> + if (ret)
> + goto err5;
> +
>   return 0;
>  
> +err5:
> + device_del(>dev);
>  err4:
>   list_del(>list);
>   mutex_unlock(_lock);
> @@ -503,6 +513,7 @@ void usb_del_gadget_udc(struct usb_gadget *gadget)
>   kobject_uevent(>dev.kobj, KOBJ_REMOVE);
>   flush_work(>work);
>   device_unregister(>dev);
> + usb_charger_exit(gadget);
>   device_unregister(>dev);
>  }
>  EXPORT_SYMBOL_GPL(usb_del_gadget_udc);
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index d82d006..054488a 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -24,6 +24,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  struct usb_ep;
>  
> @@ -563,6 +564,8 @@ struct usb_gadget_ops {
>   struct usb_ep *(*match_ep)(struct usb_gadget *,
>   struct usb_endpoint_descriptor *,
>   struct usb_ss_ep_comp_descriptor *);
> + /* get the charger type */
> + enum usb_charger_type (*get_charger_type)(struct usb_gadget *);
>  };

Since we already have get_charger_type callback at usb_charger
structure, why we still need this API at usb_gadget_ops?

>  
>  /**
> @@ -635,6 +638,8 @@ struct usb_gadget {
>   unsignedout_epnum;
>   unsignedin_epnum;
>   struct usb_otg_caps *otg_caps;
> + /* negotiate the power with the usb charger */
> + struct usb_charger  *charger;
>  
>   unsignedsg_supported:1;
>   unsignedis_otg:1;
> @@ -839,10 +844,20 @@ static inline int usb_gadget_vbus_connect(struct 
> usb_gadget *gadget)
>   * reporting how much power the device may consume.  For example, this
>   * could affect how quickly batteries are recharged.
>   *
> + * It will also notify the USB charger how much power the device may
> + * consume if there is a USB charger linking with the gadget.
> + *
>   * Returns zero on success, else negative errno.
>   */
>  static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned 
> mA)
>  {
> + enum usb_charger_type type;
> +
> + if (gadget->charger) {
> + type = usb_charger_get_type(gadget->charger);
> + usb_charger_set_cur_limit_by_type(gadget->charger, type, mA);
> + }
> +

You may do something redundant.

- Charger detection only occurs at connection period, at other periods,
we only change the current limit, and notify charger IC. That is to
say, we may only need to save charger type and current limit at
usb_charger structure, we don't need to distinguish all chargers
type from time to time.

- The purpose of usb_gadget_vbus_draw design is notify charger IC too,
so you can do set current limit and notify charger IC together at this
API together, it has already covered all situations. We don't need to
notify charger IC again when the gadget status has changed again.

>   if (!gadget->ops->vbus_draw)
>   return -EOPNOTSUPP;
>   return gadget->ops->vbus_draw(gadget, mA);
> -- 
> 1.7.9.5
> 
> --
> 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

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the 

Re: [PATCH v7 3/3] usb: otg-fsm: Prevent build warning "VDBG" redefined

[Roger Quadros]

On 06/04/16 09:46, Peter Chen wrote:
> On Tue, Apr 05, 2016 at 04:48:19PM +0300, Roger Quadros wrote:
>> Peter,
>>
>> On 05/04/16 15:52, Roger Quadros wrote:
>>> Peter,
>>>
>>> On 05/04/16 11:52, Peter Chen wrote:
 On Thu, Mar 31, 2016 at 12:41:19PM +0300, Roger Quadros wrote:
> If usb/otg-fsm.h and usb/composite.h are included together
> then it results in the build warning [1].
>
> Prevent that by using dev_vdbg() instead.
>

 After considering it more, I think it may not be a good solution
 that we delete VDBG at one header file, but keep it at another
 one. In future, we may add VDBG at another file, and cause the
 same problem. In fact, I find VDBG is defined at several files
 in USB folder (and only at USB folder), I plan to replace them
 with standard one (dev_vdbg) together.
>>>
>>> OK, please ignore this patch then.
>>
>> On second thoughts can you please retain this patch and post the
>> VDBG removal from composite.h cleanup separately?
>>
> 
> I find the struct usb_otg has a struct device pointer, and you changes
> all fsm stuffs under struct usb_otg (like otg.fsm) in your later patches,
> then, would you please refine this patch that just using otg->dev for
> print and move VDBG to phy-fsl-usb.c, of course, you need to move this
> patch in that patch series.

OK. I'll rework this patch and include it in the otg series.

cheers,
-roger

> 
> Peter
> 
>> I'm sending a revised usb-otg patchset today which depends on this series
>> and will break without this patch.
>>
>> cheers,
>> -roger
>>
>>>

> Also get rid of MPC_LOC which doesn't seem to be used
> by anyone.
>

 If you want, you can only delete MPC_LOC at this patch.

 Peter

> [1] - warning fixed by this patch:
>
>In file included from drivers/usb/dwc3/core.h:33,
> from drivers/usb/dwc3/ep0.c:33:
>include/linux/usb/otg-fsm.h:30:1: warning: "VDBG" redefined
>In file included from drivers/usb/dwc3/ep0.c:31:
>include/linux/usb/composite.h:615:1: warning: this is the location
>   of the previous definition
>
> Signed-off-by: Roger Quadros 
> ---
> v7: define VDBG locally in phy-fsl-usb.c
>
>  drivers/usb/chipidea/otg_fsm.c   |  1 +
>  drivers/usb/common/usb-otg-fsm.c | 12 +++-
>  drivers/usb/phy/phy-fsl-usb.c|  8 
>  include/linux/usb/otg-fsm.h  | 19 ---
>  4 files changed, 20 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/usb/chipidea/otg_fsm.c 
> b/drivers/usb/chipidea/otg_fsm.c
> index de8e22e..5f169b3 100644
> --- a/drivers/usb/chipidea/otg_fsm.c
> +++ b/drivers/usb/chipidea/otg_fsm.c
> @@ -805,6 +805,7 @@ int ci_hdrc_otg_fsm_init(struct ci_hdrc *ci)
>   ci->fsm.id = hw_read_otgsc(ci, OTGSC_ID) ? 1 : 0;
>   ci->fsm.otg->state = OTG_STATE_UNDEFINED;
>   ci->fsm.ops = _otg_ops;
> + ci->fsm.dev = ci->dev;
>   ci->gadget.hnp_polling_support = 1;
>   ci->fsm.host_req_flag = devm_kzalloc(ci->dev, 1, GFP_KERNEL);
>   if (!ci->fsm.host_req_flag)
> diff --git a/drivers/usb/common/usb-otg-fsm.c 
> b/drivers/usb/common/usb-otg-fsm.c
> index 9059b7d..c5a61fe 100644
> --- a/drivers/usb/common/usb-otg-fsm.c
> +++ b/drivers/usb/common/usb-otg-fsm.c
> @@ -36,8 +36,9 @@ static int otg_set_protocol(struct otg_fsm *fsm, int 
> protocol)
>   int ret = 0;
>  
>   if (fsm->protocol != protocol) {
> - VDBG("Changing role fsm->protocol= %d; new protocol= %d\n",
> - fsm->protocol, protocol);
> + dev_vdbg(fsm->dev,
> +  "Changing role fsm->protocol= %d; new protocol= %d\n",
> +  fsm->protocol, protocol);
>   /* stop old protocol */
>   if (fsm->protocol == PROTO_HOST)
>   ret = otg_start_host(fsm, 0);
> @@ -208,7 +209,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum 
> usb_otg_state new_state)
>  {
>   if (fsm->otg->state == new_state)
>   return 0;
> - VDBG("Set state: %s\n", usb_otg_state_string(new_state));
> + dev_vdbg(fsm->dev, "Set state: %s\n", usb_otg_state_string(new_state));
>   otg_leave_state(fsm, fsm->otg->state);
>   switch (new_state) {
>   case OTG_STATE_B_IDLE:
> @@ -338,7 +339,7 @@ int otg_statemachine(struct otg_fsm *fsm)
>  
>   switch (state) {
>   case OTG_STATE_UNDEFINED:
> - VDBG("fsm->id = %d\n", fsm->id);
> + dev_vdbg(fsm->dev, "fsm->id = %d\n", fsm->id);
>   if (fsm->id)
>   otg_set_state(fsm, OTG_STATE_B_IDLE);
>   else
> @@ -446,7 +447,8 @@ int otg_statemachine(struct otg_fsm *fsm)
>   }
>   mutex_unlock(>lock);
>  
> - VDBG("quit statemachine, changed = %d\n", fsm->state_changed);
> + 

Re: [PATCH v7 3/3] usb: otg-fsm: Prevent build warning "VDBG" redefined

[Peter Chen]

On Tue, Apr 05, 2016 at 04:48:19PM +0300, Roger Quadros wrote:
> Peter,
> 
> On 05/04/16 15:52, Roger Quadros wrote:
> > Peter,
> > 
> > On 05/04/16 11:52, Peter Chen wrote:
> >> On Thu, Mar 31, 2016 at 12:41:19PM +0300, Roger Quadros wrote:
> >>> If usb/otg-fsm.h and usb/composite.h are included together
> >>> then it results in the build warning [1].
> >>>
> >>> Prevent that by using dev_vdbg() instead.
> >>>
> >>
> >> After considering it more, I think it may not be a good solution
> >> that we delete VDBG at one header file, but keep it at another
> >> one. In future, we may add VDBG at another file, and cause the
> >> same problem. In fact, I find VDBG is defined at several files
> >> in USB folder (and only at USB folder), I plan to replace them
> >> with standard one (dev_vdbg) together.
> > 
> > OK, please ignore this patch then.
> 
> On second thoughts can you please retain this patch and post the
> VDBG removal from composite.h cleanup separately?
> 

I find the struct usb_otg has a struct device pointer, and you changes
all fsm stuffs under struct usb_otg (like otg.fsm) in your later patches,
then, would you please refine this patch that just using otg->dev for
print and move VDBG to phy-fsl-usb.c, of course, you need to move this
patch in that patch series.

Peter

> I'm sending a revised usb-otg patchset today which depends on this series
> and will break without this patch.
> 
> cheers,
> -roger
> 
> > 
> >>
> >>> Also get rid of MPC_LOC which doesn't seem to be used
> >>> by anyone.
> >>>
> >>
> >> If you want, you can only delete MPC_LOC at this patch.
> >>
> >> Peter
> >>
> >>> [1] - warning fixed by this patch:
> >>>
> >>>In file included from drivers/usb/dwc3/core.h:33,
> >>> from drivers/usb/dwc3/ep0.c:33:
> >>>include/linux/usb/otg-fsm.h:30:1: warning: "VDBG" redefined
> >>>In file included from drivers/usb/dwc3/ep0.c:31:
> >>>include/linux/usb/composite.h:615:1: warning: this is the location
> >>>   of the previous definition
> >>>
> >>> Signed-off-by: Roger Quadros 
> >>> ---
> >>> v7: define VDBG locally in phy-fsl-usb.c
> >>>
> >>>  drivers/usb/chipidea/otg_fsm.c   |  1 +
> >>>  drivers/usb/common/usb-otg-fsm.c | 12 +++-
> >>>  drivers/usb/phy/phy-fsl-usb.c|  8 
> >>>  include/linux/usb/otg-fsm.h  | 19 ---
> >>>  4 files changed, 20 insertions(+), 20 deletions(-)
> >>>
> >>> diff --git a/drivers/usb/chipidea/otg_fsm.c 
> >>> b/drivers/usb/chipidea/otg_fsm.c
> >>> index de8e22e..5f169b3 100644
> >>> --- a/drivers/usb/chipidea/otg_fsm.c
> >>> +++ b/drivers/usb/chipidea/otg_fsm.c
> >>> @@ -805,6 +805,7 @@ int ci_hdrc_otg_fsm_init(struct ci_hdrc *ci)
> >>>   ci->fsm.id = hw_read_otgsc(ci, OTGSC_ID) ? 1 : 0;
> >>>   ci->fsm.otg->state = OTG_STATE_UNDEFINED;
> >>>   ci->fsm.ops = _otg_ops;
> >>> + ci->fsm.dev = ci->dev;
> >>>   ci->gadget.hnp_polling_support = 1;
> >>>   ci->fsm.host_req_flag = devm_kzalloc(ci->dev, 1, GFP_KERNEL);
> >>>   if (!ci->fsm.host_req_flag)
> >>> diff --git a/drivers/usb/common/usb-otg-fsm.c 
> >>> b/drivers/usb/common/usb-otg-fsm.c
> >>> index 9059b7d..c5a61fe 100644
> >>> --- a/drivers/usb/common/usb-otg-fsm.c
> >>> +++ b/drivers/usb/common/usb-otg-fsm.c
> >>> @@ -36,8 +36,9 @@ static int otg_set_protocol(struct otg_fsm *fsm, int 
> >>> protocol)
> >>>   int ret = 0;
> >>>  
> >>>   if (fsm->protocol != protocol) {
> >>> - VDBG("Changing role fsm->protocol= %d; new protocol= %d\n",
> >>> - fsm->protocol, protocol);
> >>> + dev_vdbg(fsm->dev,
> >>> +  "Changing role fsm->protocol= %d; new protocol= %d\n",
> >>> +  fsm->protocol, protocol);
> >>>   /* stop old protocol */
> >>>   if (fsm->protocol == PROTO_HOST)
> >>>   ret = otg_start_host(fsm, 0);
> >>> @@ -208,7 +209,7 @@ static int otg_set_state(struct otg_fsm *fsm, enum 
> >>> usb_otg_state new_state)
> >>>  {
> >>>   if (fsm->otg->state == new_state)
> >>>   return 0;
> >>> - VDBG("Set state: %s\n", usb_otg_state_string(new_state));
> >>> + dev_vdbg(fsm->dev, "Set state: %s\n", usb_otg_state_string(new_state));
> >>>   otg_leave_state(fsm, fsm->otg->state);
> >>>   switch (new_state) {
> >>>   case OTG_STATE_B_IDLE:
> >>> @@ -338,7 +339,7 @@ int otg_statemachine(struct otg_fsm *fsm)
> >>>  
> >>>   switch (state) {
> >>>   case OTG_STATE_UNDEFINED:
> >>> - VDBG("fsm->id = %d\n", fsm->id);
> >>> + dev_vdbg(fsm->dev, "fsm->id = %d\n", fsm->id);
> >>>   if (fsm->id)
> >>>   otg_set_state(fsm, OTG_STATE_B_IDLE);
> >>>   else
> >>> @@ -446,7 +447,8 @@ int otg_statemachine(struct otg_fsm *fsm)
> >>>   }
> >>>   mutex_unlock(>lock);
> >>>  
> >>> - VDBG("quit statemachine, changed = %d\n", fsm->state_changed);
> >>> + dev_vdbg(fsm->dev, "quit statemachine, changed = %d\n",
> >>> +  fsm->state_changed);
> >>>   return fsm->state_changed;
> >>>  }
> >>>  

Re: [PATCH v3 3/7] usb: mux: add common code for Intel dual role port mux

[Lu Baolu]

Hi,

On 03/14/2016 11:27 AM, Greg Kroah-Hartman wrote:
> On Mon, Mar 14, 2016 at 09:09:22AM +0800, Lu Baolu wrote:
>> On 03/11/2016 08:06 AM, Greg Kroah-Hartman wrote:
>>> On Tue, Mar 08, 2016 at 03:53:44PM +0800, Lu Baolu wrote:
 +struct intel_mux_dev {
 +  struct device   *dev;
 +  char*extcon_name;
 +  char*cable_name;
 +  int (*cable_set_cb)(struct intel_mux_dev *mux);
 +  int (*cable_unset_cb)(struct intel_mux_dev *mux);
 +};
>>> This is a device, why not make it one?  Don't just hold a reference.
>>> And do you really even hold that reference?
>> It's not a device. It's just an encapsulation for parameters passed into
>> intel_usb_mux_register().
> But you called it a device, so you can understand my confusion.
>
> And why not make it a device?  Why isn't this one?  Hint, I really think
> it should be...
>
>>> And your api is horrid, think about what you want the "core" to do here,
>>> it should be the one creating the device and returning it to the caller,
>>> not forcing the caller to somehow create it first and then pass it in.
>> This isn't a layer or core. It doesn't create any new devices. It's actually
>> some shared code which can be used by all Intel dual role port drivers.
> It should be a device, as you are treating it like one :)
>
>> I put it in a separated file because 1) this can avoid duplication; 2) this 
>> code
>> could be used for any architectures as long as a USB port is shared by
>> two components and it needs an OS response when event triggers.
> It's a bit hard for other arches to be using something called "intel_"
> :(

Are there any other implementations which need an external mux
to swap the usb roles?

>> I guess intel_usb_mux_register/unregister() is a bit misleading. How about
>> changing them to intel_usb_mux_probe/remove()?
> You are going to probe/remove something that isn't a device?  Come on
> now...
>
>>> And why is it not symmetrical, you are passing one thing into register
>>> and another into unregister.
>> struct intel_mux_dev is an encapsulation for parameters passed into
>> intel_usb_mux_register().
> Which is a device.
>
>> It's not a new device structure though the name
>> is a bit misleading.
> Yes it is, hint, you want it to be a device.
>
>> How about remove this structure and put these in function parameters?
> How about making it a real device? :)

Hi Greg,

Just want to make myself clear about your expectation.
Did you mean to create a port mux device and return it to the caller?

The interfaces look like:

struct portmux_dev *devm_portmux_register(struct device *dev,
  const struct portmux_desc *desc);

void devm_portmux_unregister(struct device *dev,
   struct portmux_dev *pdev)

Do I get it right?

Best regards,
Baolu

>
> thanks,
>
> greg k-h
>

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

Re: [PATCH v6 12/12] usb: host: xhci-plat: Add otg device to platform data

[Roger Quadros]

Hi,

On 06/04/16 06:23, Yoshihiro Shimoda wrote:
> Hi,
> 
>> Sent: Tuesday, April 05, 2016 11:05 PM
>>
>> Host controllers that are part of an OTG/dual-role instance
>> need to somehow pass the OTG controller device information
>> to the HCD core.
>>
>> We use platform data to pass the OTG controller device.
>>
>> Signed-off-by: Roger Quadros 
>> ---
>>  drivers/usb/host/xhci-plat.c | 35 ---
>>  include/linux/usb/xhci_pdriver.h |  3 +++
>>  2 files changed, 31 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 5c15e9b..3d0f597 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -230,11 +230,20 @@ static int xhci_plat_probe(struct platform_device 
>> *pdev)
>>  goto put_usb3_hcd;
>>  }
>>
>> -ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>> +if (pdata->otg_dev)
> 
> I think that this should be changed like below:
>   if (pdata && pdata->otg_dev)

Indeed, I'll fix this and the remaining occurrences. Thanks.

cheers,
-roger

> 
> Otherwise, panic happened on my environment (arm64 / r8a7795 board) because 
> the pdata was NULL.
> 
>> +ret = usb_otg_add_hcd(hcd, irq, IRQF_SHARED, pdata->otg_dev);
>> +else
>> +ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
>> +
>>  if (ret)
>>  goto disable_usb_phy;
>>
>> -ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
>> +if (pdata->otg_dev)
> 
> Ditto.
> 
>> +ret = usb_otg_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED,
>> +  pdata->otg_dev);
>> +else
>> +ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
>> +
>>  if (ret)
>>  goto dealloc_usb2_hcd;
>>
>> @@ -242,7 +251,10 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>
>>
>>  dealloc_usb2_hcd:
>> -usb_remove_hcd(hcd);
>> +if (pdata->otg_dev)
> 
> Ditto.
> 
>> +usb_otg_remove_hcd(hcd);
>> +else
>> +usb_remove_hcd(hcd);
>>
>>  disable_usb_phy:
>>  usb_phy_shutdown(hcd->usb_phy);
>> @@ -260,16 +272,25 @@ put_hcd:
>>  return ret;
>>  }
>>
>> -static int xhci_plat_remove(struct platform_device *dev)
>> +static int xhci_plat_remove(struct platform_device *pdev)
>>  {
>> -struct usb_hcd  *hcd = platform_get_drvdata(dev);
>> +struct usb_hcd  *hcd = platform_get_drvdata(pdev);
>>  struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>  struct clk *clk = xhci->clk;
>> +struct usb_xhci_pdata *pdata = dev_get_platdata(>dev);
>> +
>> +if (pdata->otg_dev)
> 
> Ditto.
> 
>> +usb_otg_remove_hcd(xhci->shared_hcd);
>> +else
>> +usb_remove_hcd(xhci->shared_hcd);
>>
>> -usb_remove_hcd(xhci->shared_hcd);
>>  usb_phy_shutdown(hcd->usb_phy);
>>
>> -usb_remove_hcd(hcd);
>> +if (pdata->otg_dev)
> 
> Ditto.
> 
> Best regards,
> Yoshihiro Shimoda
> 
>> +usb_otg_remove_hcd(hcd);
>> +else
>> +usb_remove_hcd(hcd);
>> +
>>  usb_put_hcd(xhci->shared_hcd);
>>
>>  if (!IS_ERR(clk))
>> diff --git a/include/linux/usb/xhci_pdriver.h 
>> b/include/linux/usb/xhci_pdriver.h
>> index 376654b..5c68b83 100644
>> --- a/include/linux/usb/xhci_pdriver.h
>> +++ b/include/linux/usb/xhci_pdriver.h
>> @@ -18,10 +18,13 @@
>>   *
>>   * @usb3_lpm_capable:   determines if this xhci platform supports USB3
>>   *  LPM capability
>> + * @otg_dev:OTG controller device. Only requied if part of
>> + *  OTG/dual-role.
>>   *
>>   */
>>  struct usb_xhci_pdata {
>>  unsignedusb3_lpm_capable:1;
>> +struct device   *otg_dev;
>>  };
>>
>>  #endif /* __USB_CORE_XHCI_PDRIVER_H */
>> --
>> 2.5.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
--
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 v6 01/12] usb: hcd: Initialize hcd->flags to 0

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 2ca2cef..6b1930d 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2706,6 +2706,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>   int retval;
>   struct usb_device *rhdev;
>  
> + hcd->flags = 0;

seems like this would make more sense in usb_del_hcd() instead.

-- 
balbi


signature.asc
Description: PGP signature