Re: [PATCH v2 2/3] spi: add FTDI MPSSE SPI controller driver

[Mark Brown]

On Sat, Nov 10, 2018 at 11:39:58AM +0100, Anatolij Gustschin wrote:

> This v2 patch is the only patch of the original series that was
> changed, I didn't resend whole series to avoid spamming the lists.

Please don't do this, the numbering for a patch series only makes sense
for ordering the patches within a series.  If you're just sending a
single patch then there's no need to number anything, or the patch
series hasn't been applied then resend the whole thing so people don't
have to work out which versions of which patches are the current ones.


signature.asc
Description: PGP signature

Re: [PATCH v2 2/3] spi: add FTDI MPSSE SPI controller driver

[Mark Brown]

On Fri, Nov 09, 2018 at 08:53:56AM +0100, Anatolij Gustschin wrote:
> Add SPI bus controller driver for FTDI MPSSE mode. This driver
> is supposed to be used together with the FT232H interface driver
> for FPGA configuration in drivers/usb/misc/ft232h-intf.c which
> adds an mpsse spi platform device describing USB SPI bus with
> attached SPI slave devices.

I don't have the rest of this series or a cover letter - what's the
story with dependencies?


signature.asc
Description: PGP signature

Re: [PATCH/RFC 0/6] Allow compile-testing NO_DMA

[Mark Brown]

On Tue, Feb 06, 2018 at 11:14:46AM +0100, Geert Uytterhoeven wrote:

> The intention of this is twofold:
>   1. To catch users of the DMA API on systems that do no support the DMA
>  mapping API,
>   2. To avoid building drivers that cannot work on such systems anyway.
> 
> However, the disadvantage is that we have to keep on adding dependencies
> on HAS_DMA all over the place.

> Thanks to the COMPILE_TEST symbol, lots of drivers now depend on one or
> more platform dependencies (that imply HAS_DMA) || COMPILE_TEST, thus
> already covering intention #2.  Having to add an explicit dependency on
> HAS_DMA here is cumbersome, and hinders compile-testing.

Thanks for doing this, hopefully it'll make everyone's life easier!

Reviwed-by: Mark Brown <broo...@kernel.org>


signature.asc
Description: PGP signature

Re: next/master boot: 264 boots: 62 failed, 199 passed with 3 conflicts (next-20171109)

[Mark Brown]

On Thu, Nov 09, 2017 at 01:45:02PM +, Mark Brown wrote:
> On Thu, Nov 09, 2017 at 04:14:26AM -0800, kernelci.org bot wrote:
> 
> Today's -next fails to boot a bcm2836-rpi-2-b on any config in kernelci,
> it was working yesterday:
> 
> > bcm2835_defconfig:
> > bcm2836-rpi-2-b:
> > lab-collabora: new failure (last pass: next-20171107)
> 
> The boot log looks like something went badly wrong with USB:
> 
> [1.643488] dwc2 3f98.usb: DWC OTG Controller
> [1.648225] dwc2 3f98.usb: new USB bus registered, assigned bus number 
> 1
> [1.655328] dwc2 3f98.usb: irq 39, io mem 0x3f98
> [1.661495] hub 1-0:1.0: USB hub found
> [1.665301] hub 1-0:1.0: 1 port detected
> [1.669239] hub 1-0:1.0: config failed, can't get hub status (err 2)
> [1.676763] hub 1-0:1.0: activate --> -22
> [1.680806] hub 1-0:1.0: activate --> -22
> [1.684855] hub 1-0:1.0: activate --> -22
> 
> and so on until the tester gets bored.  There's nothing obvious changed
> in arch/arm or drivers/usb though.
> 
> More info and full log here:
> 
>https://kernelci.org/boot/id/5a04231159b514cb291cdd1a/

There's similar issues reported for Odroid c1 with the same DesignWare
controller so it's unlikely to be something Raspberry Pi related:

https://kernelci.org/boot/id/5a0416c059b514c4931cdd1b/


signature.asc
Description: PGP signature

Re: next/master boot: 264 boots: 62 failed, 199 passed with 3 conflicts (next-20171109)

[Mark Brown]

On Thu, Nov 09, 2017 at 04:14:26AM -0800, kernelci.org bot wrote:

Today's -next fails to boot a bcm2836-rpi-2-b on any config in kernelci,
it was working yesterday:

> bcm2835_defconfig:
> bcm2836-rpi-2-b:
> lab-collabora: new failure (last pass: next-20171107)

The boot log looks like something went badly wrong with USB:

[1.643488] dwc2 3f98.usb: DWC OTG Controller
[1.648225] dwc2 3f98.usb: new USB bus registered, assigned bus number 1
[1.655328] dwc2 3f98.usb: irq 39, io mem 0x3f98
[1.661495] hub 1-0:1.0: USB hub found
[1.665301] hub 1-0:1.0: 1 port detected
[1.669239] hub 1-0:1.0: config failed, can't get hub status (err 2)
[1.676763] hub 1-0:1.0: activate --> -22
[1.680806] hub 1-0:1.0: activate --> -22
[1.684855] hub 1-0:1.0: activate --> -22

and so on until the tester gets bored.  There's nothing obvious changed
in arch/arm or drivers/usb though.

More info and full log here:

   https://kernelci.org/boot/id/5a04231159b514cb291cdd1a/


signature.asc
Description: PGP signature

Re: linux-next: manual merge of the usb-gadget tree with the usb tree

[Mark Brown]

On Wed, Oct 18, 2017 at 08:40:04AM -0700, Kees Cook wrote:
> On Wed, Oct 18, 2017 at 7:49 AM, Mark Brown <broo...@kernel.org> wrote:

> > I fixed it up with the USB version and can carry the fix as necessary. This
> > is now fixed as far as linux-next is concerned, but any non trivial
> > conflicts should be mentioned to your upstream maintainer when your tree
> > is submitted for merging.  You may also want to consider cooperating
> > with the maintainer of the conflicting tree to minimise any particularly
> > complex conflicts.

> FWIW, timer_setup() should be preferred over setup_timer().

Bah, that's unfortunate.  When more of these come up I'll do that.  It
would be better to sort these out in the relevant trees though, there's
quite a few of these conflicts and it's not super obvious if you're not
into the timer code what's preferred.


signature.asc
Description: PGP signature

linux-next: manual merge of the usb-gadget tree with the usb tree

[Mark Brown]

Hi Felipe,

Today's linux-next merge of the usb-gadget tree got a conflict in:

  drivers/usb/phy/phy-isp1301-omap.c

between commit:

  4c13fec1ba55595 ("usb: isp1301-omap: Convert timers to use timer_setup()")

from the usb tree and commit:

  687f272c6b0e89d ("usb: phy: omap: use setup_timer() helper.")

from the usb-gadget tree.

I fixed it up with the USB version and can carry the fix as necessary. This
is now fixed as far as linux-next is concerned, but any non trivial
conflicts should be mentioned to your upstream maintainer when your tree
is submitted for merging.  You may also want to consider cooperating
with the maintainer of the conflicting tree to minimise any particularly
complex conflicts.


signature.asc
Description: PGP signature

linux-next: manual merge of the usb-gadget tree with the usb tree

[Mark Brown]

Hi Felipe,

Today's linux-next merge of the usb-gadget tree got a conflict in:

  drivers/usb/gadget/udc/snps_udc_core.c

between commit:

  29bce57723351f63d ("usb/gadget/snps_udc_core: Convert timers to use 
timer_setup()")

from the usb tree and commit:

  46b614affda8667f9 ("usb: gadget: udc: snps_udc_core: use setup_timer() 
helper.")

from the usb-gadget tree.

I fixed it up by taking the USB version and can carry the fix as
necessary. This is now fixed as far as linux-next is concerned, but any
non trivial conflicts should be mentioned to your upstream maintainer
when your tree is submitted for merging.  You may also want to consider
cooperating with the maintainer of the conflicting tree to minimise any
particularly complex conflicts.


signature.asc
Description: PGP signature

Re: [PATCH v2 14/27] ASoC: blackfin: Convert to the new PCM ops

[Mark Brown]

On Thu, Jun 01, 2017 at 10:58:37PM +0200, Takashi Iwai wrote:
> Replace the copy and the silence ops with the new PCM ops.
> In AC97 and I2S-TDM mode, we need to convert back to frames, but
> otherwise the conversion is pretty straightforward.

Acked-by: Mark Brown <broo...@kernel.org>


signature.asc
Description: PGP signature

Re: [PATCH v2 02/27] ALSA: pcm: Introduce copy_user, copy_kernel and fill_silence ops

[Mark Brown]

On Thu, Jun 01, 2017 at 10:58:25PM +0200, Takashi Iwai wrote:
> For supporting the explicit in-kernel copy of PCM buffer data, and
> also for further code refactoring, three new PCM ops, copy_user,
> copy_kernel and fill_silence, are introduced.  The old copy and
> silence ops will be deprecated and removed later once when all callers
> are converted.

Acked-by: Mark Brown <broo...@kernel.org>


signature.asc
Description: PGP signature

Re: [PATCH 3/3] mfd: twl: move header file out of I2C realm

[Mark Brown]

On Mon, May 22, 2017 at 12:28:12PM +0200, Wolfram Sang wrote:

> If you were not CCed on the other patches, then you are likely not
> listed as a maintainer for them?

Right, but it does mean that the people who only get a subset of patches
are missing context for how things are expected to be handled.
Sometimes it's clear but often it isn't.


signature.asc
Description: PGP signature

Re: [PATCH 3/3] mfd: twl: move header file out of I2C realm

[Mark Brown]

On Mon, May 22, 2017 at 12:02:10AM +0200, Wolfram Sang wrote:
> include/linux/i2c is not for client devices. Move the header file to a
> more appropriate location.

Acked-by: Mark Brown <broo...@kernel.org>

I'm missing the rest of the series and/or the cover letter...


signature.asc
Description: PGP signature

Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Tue, Nov 22, 2016 at 09:40:07AM +1100, NeilBrown wrote:

> I agree that the question of where the responsibility for information
> aggregation lies is open for discussion. If fact all details on how
> things should work are always open for discussion.
> I don't agree that this is the main different between our positions,
> though I can see how you might get that impression.

> You could even fix them so they look *exactly* like the notifiers that
> Baolin is proposing.  This is my key point.  It is not the end result
> that I particularly object to (though I do object to some details).  It

Ah, OK.  This really hadn't been at all clear - both Baolin and I had
the impression that the it was both that were blockers for you.  What
were the details here?

> is the process of getting to the end result that I don't like.  If the
> current system doesn't work and something different is needed, then the
> correct thing to do is to transform the existing system into something
> new that works better.  This should be a clear series of steps.  Each

Sometimes there's something to be said for working out what we want
things to look like before setting out to make these gradual
refactorings and sometimes the refactorings are just more pain than
they're worth, especially when they go across subsystems.  In this case
I do worry about the cross subsystem aspect causing hassle, it may be
more practical to do anything that represents an interface change by
adding the new interface, converting the users to it and then removing
the old interface.

At the very least the series should grow to incorporate conversion of
the existing users though.  Baolin, I think this does need adding to the
series but probably best to think about how to do it - some of Neil's
suggestions for incremental steps do seem like they should be useful
for organizing things here, probably we can get some things that can be
done internally within individual drivers merged while everything else
is under discussion.

> But I think here my key point got lost too, in part because it was hard
> to refer to an actual instance.
> My point was that in the present patch set, the "usb charger" is given
> a name which is dependant on discovery order, and only supports
> lookup-by-name.  This cannot work.

There's two bits here: one is the way names are assigned and the other
is the lookup by name.  I agree that the lookup by name isn't
particularly useful as things stand, that could just be dropped until
some naming mechanism is added.  We'd be more likely to use phandles in
DT systems, I don't know what ACPI systems would look like but I guess
it'd be something similar.

> If they supported lookup by phy-name or lookup-by-active (i.e. "find me
> any usb-charger which has power available"), or look up by some other
> attribute, then discover-order naming could work.  But the only
> lookup-mechanism is by-name, and the names aren't reliably stable.  So
> the name/lookup system proposed cannot possibly do anything useful
> with more than one usb_charger.

Baolin, I think adding a DT binding and lookup mechanism makes sense
here - do you agree?


signature.asc
Description: PGP signature

Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Thu, Nov 17, 2016 at 05:46:13PM +1100, NeilBrown wrote:
> On Thu, Nov 17 2016, Mark Brown wrote:

> > To me that's pretty much what's being done here, the code just happens
> > to sit in USB instead but fundamentally it's just a blob of helper code,
> > you could replace the notifier with a callback if that's the big concern
> > here.

> It is a lot more than "just a blob of helper code".  It duplicates
> existing infrastructure instead of fixing and using the
> infrastructure but I've said all this before.  Repeatedly.

My read on that is that the question of what we want to be responsible
for aggregating the information about what power the system is allowed
to draw from a given USB port hasn't been resolved yet and that apart
from that you're fairly close.  It seems to me like that's really what
the difference between your two positions is.  Fixing the existing
notifiers implies that things have to be aggregated in the power supply
drivers but Baolin is proposing doing that in the USB code instead.  It
does seem at least worth considering if that's a good idea since the
current situation doesn't look terribly thought through.

There are a whole bunch of things that need to be sorted out whatever
the decision is like the extcon related cleanups you mentioned in your
mail the other day (steps 1 and 2), it seems like those could be moved
forwards independently.

By the way it occurred to me recently that we have a use case for
multiple USB ports that could supply power - USB C.  Things with more
than one port like things in a laptop form factor are going to want to
be able to use all of them interchangably for power support (likely only
one at a time, at least initially, but still more than one port).


signature.asc
Description: PGP signature

Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Tue, Nov 15, 2016 at 08:35:13AM +1100, NeilBrown wrote:
> On Mon, Nov 14 2016, Mark Brown wrote:

> > Conflating the two seems like the whole point here.  We're looking for
> > something that sits between the power supply code and the USB code and
> > tells the power supply code what it's allowed to do which is the result
> > of a combination of physical cable detection and USB protocol.  It seems
> > reasonable that extcon drivers ought to be part of this but it doesn't
> > seem like they are the whole story.

> I don't think "between the power supply code and the USB code" is where
> this thing sits. I think it sits inside the power-supply driver.
> We already have extcon which sits between the phy and the power_supply
> code, and the usb_notifier which sits between the USB code and the
> power supply code.  We don't need another go-between.

...

> correct determinations and set the current limits itself.  All this
> could be done entirely internally, without the help of any new
> subsystem.
> Do you agree?

> Clearly doing it that way would result in lots of code duplication and
> would mean that each driver probably gets its own private set of bugs,
> but it would be possible.  Just undesirable.

I think this is the key here - the fact that it's technically possible
to implement doesn't really matter if it's sufficiently fiddly and non
obvious that nobody is actually joining everything up (bits are done
intermittently but not as a whole as far as I can see).

> So yes, it makes perfect to provide common code which handles the
> registrations, and captures the events, and translates the different
> events into current levels and feeds those back to the driver.  This
> isn't some new subsystem, this is just a resource, provided by a
> library, that power drivers can allocate and initialize if the want to.

To me that's pretty much what's being done here, the code just happens
to sit in USB instead but fundamentally it's just a blob of helper code,
you could replace the notifier with a callback if that's the big concern
here.


signature.asc
Description: PGP signature

Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Mon, Nov 14, 2016 at 03:21:13PM +1100, NeilBrown wrote:
> On Thu, Nov 10 2016, Baolin Wang wrote:

> > Fourth, we need integrate all charger plugin/out
> > event in one framework, not from extcon, maybe type-c in future.

> Why not extcon?  Given that a charger is connected by an external
> connector, extcon seems like exactly the right thing to use.

> Obviously extcon doesn't report the current that was negotiated, but
> that is best kept separate.  The battery charger can be advised of the
> available current either via extcon or separately via the usb
> subsystem.  Don't conflate the two.

Conflating the two seems like the whole point here.  We're looking for
something that sits between the power supply code and the USB code and
tells the power supply code what it's allowed to do which is the result
of a combination of physical cable detection and USB protocol.  It seems
reasonable that extcon drivers ought to be part of this but it doesn't
seem like they are the whole story.

> > word, we need one standard integration of this feature we need, though
> > like you said we should do some clean up or fix to make it better.

> But really, I'm not the person you need to convince.  I'm just a vaguely
> interested bystander who is rapidly losing interest.  You need to
> convince a maintainer, but they have so far shown remarkably little
> interest.  I don't know why, but I'd guess that reviewing a complex new
> subsystem isn't much fun.  Reviewing and applying clear bugfixes and
> incremental improvements is much easier and more enjoyable.  But that is
> just a guess.

OTOH having someone else having reviewed might help them apply something
they don't care about.


signature.asc
Description: PGP signature

Re: [PATCH v18 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Fri, Oct 28, 2016 at 08:51:41PM +0800, Baolin Wang wrote:
> On 28 October 2016 at 06:00, NeilBrown  wrote:

> > 1/ I think we agreed that it doesn't make sense for there to be
> >  two chargers registered in a system.

> Yes, until now...

> >  However usb_charger_register() still allows that, and assigns
> >  and arbitrary name to each based on discovery order.
> >  This *cannot* make sense.

> Fine, I can change that to allow only one charger to register.

Yeah, it's a reasonable change.  I'm not sure the prior discussion was
100% conclusive on the issue (I remember there being some debate about
leaving things there to avoid any need for future refactoring to touch
the interface).

> > 2/ Why do you have usb_charger_set_current()??
> >   No code ever calls it.
> >   This updates the min and max current which are defined in a
> >   standard.  It never makes sense to change the min and max
> >   for a particular cable type.

> Mark, do we have some scenarios which want to change the current
> limitation? If not, okay, I agree with you to remove this function.

I'm not aware of any, we can always add it back if the need arises.

> >  Related:  I don't like charger_type_show().  I don't think
> >  the usb-charger should export that information to user-space because
> >  extcon already does that, and duplication is confusing and pointless.

> I think we should combine all charger related information into one
> place for user. Moreover if we don't get charger type from extcon, we
> should also need one place to export the charger type.

I had also thought there was some software negotation as well as the
physical charger in cases where the device is plugged into an active
host?  I could be wrong.

> > 5/ There is no convincing example usage of this framework.
> >   wm8931x_power.c just scratches the surface.
> >   If it is so good, it should be easy to convert a lot of other
> >   drivers over to it.  If you did that it would be much easier
> >   to see how it works and what the strengths/weaknesses were.

> Jun have send out one patchset[1] based on my patchset, and he tested
> mypatchset. Thanks for your comments.
> [1]http://www.spinics.net/lists/linux-usb/msg139809.html

I think it's a good idea to pick up Jun's patches into your patch set,
that way Jun doesn't need to rebase and it might help with review of
your patches too.


signature.asc
Description: PGP signature

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Mark Brown]

On Tue, Oct 25, 2016 at 02:55:48PM +0200, Axel Haslam wrote:

> To be able to use regulator to handle the overcurrent pin, i need to be able
> to somehow retrieve the over current pin state from the regulator driver.

What makes you say that, none of the existing users need this?  

> As i was trying your suggestion, i remembered why i thought i should use
> mode instead of status: Status seems to be for internal regulator driver use,
> there is no regulator_get_status, function and REGULATOR_STATUS_* are defined
> in driver.h and not in consumer.h as  REGULATOR_MODE_*

> Would you be ok if i allow consumers to get the status via a new
> "regulator_get_status" call?

What would they do with this information that they can't do with the
existing error notification?


signature.asc
Description: PGP signature

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Mark Brown]

On Mon, Oct 24, 2016 at 08:11:40PM +0200, Axel Haslam wrote:
> On Mon, Oct 24, 2016 at 7:53 PM, Mark Brown <broo...@kernel.org> wrote:

> > does it make sense to report this as a mode, we don't report other error
> > conditions as modes but instead use REGULATOR_STATUS_ with the
> > get_status() operation?

> I used mode, because when the regulator toggles the overcurrent
> line, it means that it has entered a current limited mode, at least the
> regulator im looking at. ill change to STATUS

That's not what regulator modes are - please look at the documentation
for the defines here.  They're about the quality of regulation.


signature.asc
Description: PGP signature

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Mark Brown]

On Mon, Oct 24, 2016 at 06:46:26PM +0200, ahas...@baylibre.com wrote:

> + if (ret) {
> + pr_err("Failed to request irq: %d\n", ret);

dev_err()

> +++ b/include/linux/regulator/consumer.h
> @@ -74,6 +74,10 @@
>   * the most noisy and may not be able to handle fast load
>   * switching.
>   *
> + * OVERCURRENT Regulator has detected an overcurrent condition, and
> + * may be limiting the supply output.
> + *
> + *
>   * NOTE: Most regulators will only support a subset of these modes. Some
>   * will only just support NORMAL.
>   *
> @@ -84,6 +88,7 @@
>  #define REGULATOR_MODE_NORMAL0x2
>  #define REGULATOR_MODE_IDLE  0x4
>  #define REGULATOR_MODE_STANDBY   0x8
> +#define REGULATOR_MODE_OVERCURRENT   0x10

This is adding a new core feature with a new mode and should have been
split out of the driver specific change with a spearate changelog.  Why
does it make sense to report this as a mode, we don't report other error
conditions as modes but instead use REGULATOR_STATUS_ with the
get_status() operation?


signature.asc
Description: PGP signature

Re: [PATCH/RFT v2 09/17] regulator: fixed: Add over current event

[Mark Brown]

On Mon, Oct 24, 2016 at 06:46:26PM +0200, ahas...@baylibre.com wrote:
> From: Axel Haslam 
> 
> Some regulator supplies have an over-current pin that is
> activated when the hw detects an over current condition.
> When this happens, the hardware enters a current limited
> mode.

Please don't mix random enhancements like this into bigger system
specific RFC serieses, send them separately so they're easier to spot
and there's no confusion with dependencies and then reference them from
the system specific series when you post that.


signature.asc
Description: PGP signature

Re: [PATCH v2] musb: Export musb_root_disconnect for use in modules

[Mark Brown]

On Thu, Sep 22, 2016 at 07:28:42PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Sep 22, 2016 at 06:15:26PM +0100, Mark Brown wrote:
> > On Thu, Sep 22, 2016 at 08:30:16AM -0500, Bin Liu wrote:

> > > Removed it from my tree, since Greg already picked it.

> > It's not showing in today's -next...

> It probably missed the cut-off, should be there tomorrow.

Ah, excellent thanks!  Down to only one build break in all*config in -next :/


signature.asc
Description: PGP signature

Re: [PATCH v2] musb: Export musb_root_disconnect for use in modules

[Mark Brown]

On Thu, Sep 22, 2016 at 08:30:16AM -0500, Bin Liu wrote:
> On Wed, Sep 21, 2016 at 03:51:33PM -0500, Bin Liu wrote:

> > Applied. Thanks.

> Removed it from my tree, since Greg already picked it.

It's not showing in today's -next...


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: gadget: Add uevent to notify userspace

[Mark Brown]

On Thu, Sep 22, 2016 at 03:38:30PM +0200, Greg KH wrote:
> On Thu, Sep 22, 2016 at 08:41:09PM +0800, Baolin Wang wrote:

> > OK. I will talk with Badhri if I can upstream these.

> That's not an issue, you can keep the "From:" line on it, if you got it
> in a legal way, and then just have your signed off by on it, go read the
> DCO for the specifics.  I don't know why someone else told you
> otherwise.

Given the weakness people have in understanding the legal part of
signoffs it seems very unwise to just trust that they've actually
got everything clearly lined up and IME when they're missing they've
often just been removed or sometimes there's a very good reason why the
original author didn't provide a signoff.  It seems safer for everyone
to query what's going on and it's been fairly unusual that it's anything
other than a mistake.


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: Add uevent to notify userspace

[Mark Brown]

On Thu, Sep 22, 2016 at 06:53:12PM +0800, Baolin Wang wrote:
> From: Badhri Jagan Sridharan 
> 
> Some USB managament on userspace (like Android system) rely on the uevents
> generated by the composition driver to generate user notifications. Thus this
> patch adds uevents to be generated whenever USB changes its state: connected,
> disconnected, configured.
> 
> The original code was created by Badhri Jagan Sridharan, and I did some
> optimization.
> 
> CC: Badhri Jagan Sridharan 
> Signed-off-by: Baolin Wang 

If you're sending a patch someone else wrote you need their
Signed-off-by - if they didn't provide one you need to talk to them
about that and get it.

> +config USB_CONFIGFS_UEVENT
> + boolean "Uevent notification of Gadget state"
> + depends on USB_CONFIGFS
> + help
> +   Enable uevent notifications to userspace when the gadget
> +   state changes. The gadget can be in any of the following
> +   three states: "CONNECTED/DISCONNECTED/CONFIGURED"

Why not just generate the events unconditionally?


signature.asc
Description: PGP signature

Re: [PATCH] musb: Export musb_root_disconnect for use in modules

[Mark Brown]

On Fri, Sep 16, 2016 at 05:47:01PM +0200, Greg Kroah-Hartman wrote:
> On Fri, Sep 16, 2016 at 04:59:36PM +0200, Hans de Goede wrote:

> > +EXPORT_SYMBOL_GPL(musb_root_disconnect);

> Does this fix a build error somehow?  Who reported it?

Yes, the sunxi driver uses this symbol and the build bots reported it
yesterday.


signature.asc
Description: PGP signature

Re: next-20160915 build: 2 failures 16 warnings (next-20160915)

[Mark Brown]

On Thu, Sep 15, 2016 at 12:43:41PM +0100, Build bot for Mark Brown wrote:

Today's -next fails to build both arm and arm64 allmodconfigs due to:

>   arm64-allmodconfig
> ERROR: "musb_root_disconnect" [drivers/usb/musb/sunxi.ko] undefined!

>   arm-allmodconfig
> ERROR: "musb_root_disconnect" [drivers/usb/musb/sunxi.ko] undefined!

due to 7cba17ec9adc8cf (musb: sunxi: Add support for platform_set_mode)
which adds a call to that non-exported function to a module.


signature.asc
Description: PGP signature

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Wed, Sep 14, 2016 at 07:50:00PM +0200, NeilBrown wrote:
> On Wed, Sep 14 2016, Mark Brown wrote:

> Ah my mistake, sorry.
> When earlier you said:

> > It's a
> > current limiter intended to sit in line with the USB power lines to

> I assumed that all it did was limit the current to number given.
> If it also limits the current to ensure that voltage doesn't drop
> unduly, then I agree with your assertion that it just needs to be told
> the upper limit.

Oh, I see the gap here - the USB specific bit is only a current limiter
but it works in concert with other bits of the system that try to stop
the voltage from whatever supply is in use from dropping and can't be
used independently of them.  That's why I wasn't clear in what I said!

> I hope you'll agree that other drivers might need to know the lower
> limit, so reporting both to all drivers is sensible.

Yes, absolutely.


signature.asc
Description: PGP signature

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Wed, Sep 14, 2016 at 04:11:58PM +0200, NeilBrown wrote:
> On Wed, Sep 14 2016, Mark Brown wrote:

> > Yes, the idea is that the charger will back off charging and stop
> > entirely if the rest of the system is consuming too much power to allow
> > it to continue effectively.  The same thing happens with wall power, if
> > a wall supply isn't able to power the charger (eg, because the rest of
> > the system is running flat out) it'll have to cope with that.

> Maybe you are correct.  I don't find your argument convincing, but maybe
> that is because I don't want to...

There's a *huge* variation in how chargers are designed, some are
designed to be dumb and won't function without software while the wm831x
is more at the opposite end of the spectrum and will quite happily run
all the charging and power source selection logic with no software
intervention at all - the parameters it uses can be changed at runtime
but that's about it.  Software implementations are obviously more
flexible but hardware implementations can be more responsive to changes
in system state like drooping supplies and aren't vulnerable to things
like software lockups.

>  1/ I had a report once from someone whose device stopped charging
>  because it was pulling more current than the charger could supply.
>  The voltage dropped below the 3.5V (I think) that the battery
>  charging hardware needed, so it switched off.  It wouldn't switch
>  back on again until explicitly told too.  It would then overload the
>  charger again and switch off.

That's just one charger's algorithm though, other options are available.

> Which seems to say the maximum is just for safety, implying that the
> minimum is the important value.

This is what I was saying about a sensible reading being for the supply
and consumer side to directly target the maximum and minimum limits
respectively (though for the battery charger spec it's a bit different
as the range is so wide).

>  3/  Felipe Balbi <ba...@kernel.org>  appears to agree with my
>perspective.
>   http://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1224904.html
>does argument-by-authority work?

TI do a lot of the more software managed chargers (which I suspect are
the main thing Felipe will have looked at) if that's what you're
referring to here?  The device is implementing pretty much the algorithm
you're describing in that e-mail so I'm a bit confused as to what you're
saying here.


signature.asc
Description: PGP signature

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Tue, Sep 13, 2016 at 10:00:28AM +0200, NeilBrown wrote:
> On Mon, Sep 12 2016, Mark Brown wrote:

> > That's not actually 100% clear to me - for what the wm831x is doing it
> > probably *does* want the higher limit.  This is a system inflow limit
> > (as it should be for this), at least the charger will adapt to voltage
> > variations though other users in the system are much less likely to do
> > so.

> Not having very much electrical engineering background, I cannot say for
> sure what will happen, but it seems likely that once the voltage drops
> much below 4.75V, the charger won't be operating at peak efficiency,
> which would be a waste.
> I can easily imagine that the hardware would switch off at some voltage
> level, rather than just making do with what is there.
> So I'm skeptical of this approach, but I'm open to being corrected by
> someone more knowledgeable than I.

Yes, the idea is that the charger will back off charging and stop
entirely if the rest of the system is consuming too much power to allow
it to continue effectively.  The same thing happens with wall power, if
a wall supply isn't able to power the charger (eg, because the rest of
the system is running flat out) it'll have to cope with that.

> Looking at it from a different perspective, according to the patch set,
> the limits that wm831x is able to impose are:

>  +0,
>  +2,
>  +100,
>  +500,
>  +900,
>  +1500,
>  +1800,
>  +550,

> These are, from the battery charger spec, minimums rather than maximums.
> e.g. a CDP provides at least 1500, and as much as 5000.  So it seems
> that the wm831x was designed to be told the minimum guaranteed available.
> But that is circumstantial evidence and might be misleading.

AIUI this is conservatisim in the system design - another way of reading
a spec with a range like this is that the consumer should aim for the
lower limit, the provider should aim for the upper limit and that way if
either of them has issues with tolerances things will still work out.
It predates wide availability of CDP so I wouldn't be surprised if that
bit were just an oversight.  Like I say this bit of hardware is totally
separate to the battery charger.


signature.asc
Description: PGP signature

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Mon, Sep 12, 2016 at 03:27:18PM +0200, NeilBrown wrote:
> On Mon, Sep 12 2016, Mark Brown wrote:

> > It's no worse than any other board file situation - if someone has that
> > problem they get to fix it.

> My point is that the present design does not appear to scale beyond a
> single USB power supply (as if there were two, they would be named in
> discovery order, which is not reliably stable).

For the practical purposes of people making systems (as opposed to
upstream where this is likely to get most use) it pretty much is.
Though quite how many systems have multiple chargers is itself also a
question.

> Your point is, I think, that when someone actually cares about that lack
> of scaling, they can fix it.

Yes.

> I am perfectly happy with that approach.  However if the code doesn't
> scale beyond one charger, it shouldn't pretend that it does.
> i.e.
>   Don't have "usb_charger_find_by_name()", just a global "usb_charger"
>   (or similar).
>   The first charger to register gets to be the "usb_charger".  The
>   second one gets an error.
> I could be quite happy with that sort of interface.

Well, a fairly standard way of extending would be to allow the explicit
assignment of names to chargers so this'd avoid such churn.

> > The whole point from the point of view of the wm831x driver is that it
> > just wants something to tell it how much current it's allowed to draw, I
> > appreciate that doesn't change your analysis of the bit in the middle
> > but the consumer driver bit seems fine here.

> Yes, the wm831x driver probably does the right thing.
> Other drivers might want to know not only the minimum they are allowed
> to draw, but also the maximum they should try even if they are carefully
> monitoring the voltage.
> So wm831x is doing the right thing with the wrong interface.  Maybe you
> can describe that as "fine".

That's not actually 100% clear to me - for what the wm831x is doing it
probably *does* want the higher limit.  This is a system inflow limit
(as it should be for this), at least the charger will adapt to voltage
variations though other users in the system are much less likely to do
so.


signature.asc
Description: PGP signature

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Sat, Sep 10, 2016 at 07:57:26AM +1000, NeilBrown wrote:
> On Fri, Sep 09 2016, Mark Brown wrote:

> > The wm831x driver in the patch series is an example of such hardware -
> > it is purely a power manager, it has no USB PHY hardware at all.  It's a

> The "probe" routine calls

>  +usb_charger_find_by_name(wm831x_pdata->usb_gadget);

> Presumably wm831x_pdata is initialised by a board file?

Yes.

> I strongly suspect it is initialized to  "usb-charger.0" because the
> names given to usb chargers are "usb-charger.%d" in discovery order.
> I don't see this being at all useful if there is ever more than one
> usb-charger.
> Do you?

It's no worse than any other board file situation - if someone has that
problem they get to fix it.

> So how can this wm831x driver actually find out what sort of charger is
> connected and so set the power limit?  I just don't see this working *at*
> *all*.

The whole point from the point of view of the wm831x driver is that it
just wants something to tell it how much current it's allowed to draw, I
appreciate that doesn't change your analysis of the bit in the middle
but the consumer driver bit seems fine here.


signature.asc
Description: PGP signature

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Fri, Sep 09, 2016 at 09:13:31AM +1000, NeilBrown wrote:

> Conceptually, the PHY is separate from the power manager and a solution
> which recognises that will be more universal.

The wm831x driver in the patch series is an example of such hardware -
it is purely a power manager, it has no USB PHY hardware at all.  It's a
current limiter intended to sit in line with the USB power lines to
ensure the system doesn't go over the maximum current draw (and also
integrates with the power source selection logic the chip has to pick
the best available power source for the system).


signature.asc
Description: PGP signature

Re: [PATCH] usb: phy: generic: request regulator optionally

[Mark Brown]

On Tue, Sep 06, 2016 at 11:01:15AM -0700, Stefan Agner wrote:
> On 2016-09-06 01:22, Mark Brown wrote:

> > This is nonsense unless the device can work without this supply.  Given
> > that the supply is called VCC that doesn't seem entirely likely.

> Afaik it is kind of a generic device tree binding, I guess the physical
> device can have various appearances and properties...

Is it really realistic that a meaningful proportion of them will work
without power?

> A quick survey showed several device trees which do not specify
> vcc-supply...

The regulator framework will attempt to be forgiving in what it accepts,
the absence of a mandatory supply is sadly not a good indication that
the supply does not physically exist...

> That said, I checked the device at hand, and it actually has a USB PHY
> power supply inputs, but the device tree does not model them.

...like here.

> > That's how to use _get_optional() but it's really unusual that you
> > should be using _get_optional().

> Despite the above findings, I still think it is the right thing to do as
> long as we specify vcc-supply to be optional.

I disagree, and bear in mind that it is more complex all round to handle
optional supples - the reason they exist is that on devices where
supplies may be omitted you usually have to do some kind of special case
handling (like enabling internal regulators or something).  If you don't
have any such special case handling but instead simply omit enables and
disables then that's a fairly clear abuse of the API.


signature.asc
Description: PGP signature

Re: [PATCH] usb: phy: generic: request regulator optionally

[Mark Brown]

On Tue, Sep 06, 2016 at 10:45:19AM +0300, Felipe Balbi wrote:
> Stefan Agner  writes:

> > According to the device tree bindings the vcc-supply is optional.

This is nonsense unless the device can work without this supply.  Given
that the supply is called VCC that doesn't seem entirely likely.

> > +   nop->vcc = devm_regulator_get_optional(dev, "vcc");
> > if (IS_ERR(nop->vcc)) {
> > dev_dbg(dev, "Error getting vcc regulator: %ld\n",
> > PTR_ERR(nop->vcc));
> > -   if (needs_vcc)
> > -   return -EPROBE_DEFER;
> > +   if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
> > +   return PTR_ERR(nop->vcc);

> does this look okay from a regulator API perspective?

That's how to use _get_optional() but it's really unusual that you
should be using _get_optional().


signature.asc
Description: PGP signature

Re: master build: 2 failures 3 warnings (v4.7-1605-ge658052)

[Mark Brown]

On Tue, Jul 26, 2016 at 02:38:00PM +0200, Arnd Bergmann wrote:

> The build report doesn't actually show the error unfortunately, but
> I'm pretty sure it's this one:

> drivers/staging/built-in.o: In function `nbu2ss_drv_probe':
> (.text+0x2428): undefined reference to `usb_ep_set_maxpacket_limit'

Yes, linker errors get eaten sadly.


signature.asc
Description: PGP signature

Re: next-20160705 build: 2 failures 6 warnings (next-20160705)

[Mark Brown]

On Tue, Jul 05, 2016 at 10:15:03AM +0100, Build bot for Mark Brown wrote:

For the past little while both arm and arm64 allmodconfig builds have
been failing with:

> drivers/built-in.o: In function `nbu2ss_drv_probe':
> binder.c:(.text+0x29438c): undefined reference to `usb_ep_set_maxpacket_limit'
> binder.c:(.text+0x294468): undefined reference to `usb_ep_set_maxpacket_limit'

That function is a static inline in linux/usb/gadget.h which does seem
to be included (the driver builds fine) so I'm not entirely sure why
this is failing - I've not had time to investigate properly, I don't
know if the compiler is misfiring here.


signature.asc
Description: PGP signature

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Tue, Jun 21, 2016 at 02:50:27PM +0300, Felipe Balbi wrote:
> Mark Brown <broo...@kernel.org> writes:

> > The wm831x has no DT support currently.

> okay, perhaps its time to add it.

The only platform using it would need the DT connector overlays
completing in order to be able to convert to DT.  I'm really not
convinced it's worth the effort either.


signature.asc
Description: PGP signature

Re: [PATCH v12 4/4] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Tue, Jun 21, 2016 at 01:30:49PM +0300, Felipe Balbi wrote:
> Baolin Wang  writes:
> > @@ -607,8 +647,31 @@ static int wm831x_power_probe(struct platform_device 
> > *pdev)
> > }
> > }

> > +   if (wm831x_pdata && wm831x_pdata->usb_gadget) {
> > +   power->usb_charger =
> > +   usb_charger_find_by_name(wm831x_pdata->usb_gadget);

> the fact that you rely on strings and pass them via pdata is an
> indication that you don't have enough description of the HW. Seems like
> we need to come up with a set of DT properties which tie a charger to a
> UDC.

> I'm thinking a phandle would be enough?

The wm831x has no DT support currently.


signature.asc
Description: PGP signature

Re: [PATCH 08/12] doc: binding: pwrseq-usb-generic: add binding doc for generic usb power sequence driver

[Mark Brown]

On Mon, Jun 20, 2016 at 11:16:07AM -0500, Rob Herring wrote:
> On Mon, Jun 20, 2016 at 07:26:51PM +0800, Peter Chen wrote:

> > If the node has property "power-sequence", the pwrseq core will create
> > related platform device, and the driver under pwrseq driver will handle
> > power sequence stuffs. 

> This I have issue with. If you are creating a platform device here, you 
> are trying to work-around limitations in the linux driver model. Either 
> we need some sort of pre-probe hook to the drivers to call or each 
> parent node driver is responsible for checking and calling pwr-seq 
> functions for child nodes. e.g. The host controller calls pwr-seq for 
> the hub, the hub driver calls the power seq for the asix chip. Soon as 
> we have a case too complex for the generic pwr-seq, we're going to need 
> the pre-probe hook as I don't want to see a continual expansion of 
> generic pwr-seq binding for ever more complex cases.

I think it's fairly clear that we need one or both of these mechanisms
for enumerable buses in embedded contexts - it's something that keeps
croping up.


signature.asc
Description: PGP signature

Re: [RFC v4 01/14] regulator: of: Add helper for getting all supplies

[Mark Brown]

On Thu, Jun 09, 2016 at 02:50:26PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 9, 2016 at 12:29 PM, Mark Brown <broo...@kernel.org> wrote:

> > The external interface shouldn't be DT specific, the Intel people are
> > busy importing all of DT into ACPI

> Well, not really.

> If you are referring to the pinctrl proposal discussed recently, that
> was a proposal from one group at Intel and AFAICS it has been
> abandoned.

Well, it was that, it was the device properties stuff in general and the
overlays stuff - plus just the fact that with the Minnowboard style use
case people have and the desire to use ACPI it seems like that's going
to be something you need.


signature.asc
Description: PGP signature

Re: [RFC v4 01/14] regulator: of: Add helper for getting all supplies

[Mark Brown]

On Thu, Jun 09, 2016 at 11:44:18AM +0200, Krzysztof Kozlowski wrote:
> Few drivers have a need of getting regulator supplies without knowing
> their names:
> 1. The Simple Framebuffer driver works on setup provided by bootloader
>(outside of scope of kernel);
> 2. Generic power sequence driver may be attached to any device node.
> 
> Add a Device Tree helper for parsing "-supply" properties and returning
> allocated bulk regulator consumers.

I'm still very concerned that this is just an invitation to people to
write half baked regulator consumers and half baked DTs to go along with
it, making it a standard API that doesn't have big red flags on it that
will flag up when "normal" drivers use it is not good.  Right now this
just looks like a standard API and people are going to just start using
it.  If we are going to do this perhaps we need a separate header or
something to help flag this up.

In the case of power sequences I'd expect the sequences to perform
operations on named supplies - the core shouldn't know what the supplies
are but the thing specifying the sequence should.

>  drivers/regulator/of_regulator.c   | 86 
> ++
>  include/linux/regulator/of_regulator.h | 13 +
>  2 files changed, 99 insertions(+)

The external interface shouldn't be DT specific, the Intel people are
busy importing all of DT into ACPI so they'll doubtless want an ACPI
version.


signature.asc
Description: PGP signature

Re: [PATCH v10 1/7] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Tue, Jun 07, 2016 at 09:42:48PM -0700, Greg Kroah-Hartman wrote:
> On Thu, Jun 02, 2016 at 09:37:23AM +0800, Lu Baolu wrote:

> > Add support to retrieve fixed voltage configure information through
> > ACPI interface. This is needed for Intel Bay Trail devices, where a
> > GPIO is used to control the USB vbus.

> Can't do anything with this until I get an ack from the "owners" of this
> file.

Please allow a reasonable time for review, it's been under a week since
this was posted.  I would *not* expect this to be applied to another
tree, please don't do that.


signature.asc
Description: PGP signature

Re: [RFC 4/8] usb: phy: move TCSR driver into new file

[Mark Brown]

On Fri, May 20, 2016 at 02:24:14PM +0200, Arnd Bergmann wrote:
> On Thursday 19 May 2016 14:08:43 Andy Gross wrote:

> > I'd rather do something like what we did for the GSBI.  It needed to
> > change some phy related bits in the TCSR as well.  We defined the TCSR
> > as a syscon, with binding documentation under mfd.  If we add a syscon
> > entry and use it if it is present, we can deal with that pretty
> > easily.  The offsets change for each soc, and this would also fix that
> > issue because we can change offset based on tcsr compat.

> Works for me, but be aware that this will break the server chips,
> as ACPI has no support for regmap devices.

Just to be clear there's nothing precluding the use of regmap on ACPI
devices, it's syscon it doesn't have anything for.


signature.asc
Description: PGP signature

Re: [PATCH v8 1/7] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Thu, May 05, 2016 at 01:32:57PM +0800, Lu Baolu wrote:

> + gpiod = gpiod_get(dev, "gpio", GPIOD_ASIS);
> + if (IS_ERR(gpiod))
> + return ERR_PTR(-ENODEV);

> + config->gpio = desc_to_gpio(gpiod);
> + config->enable_high = device_property_read_bool(dev,
> + "enable-active-high");

> + gpiod_put(gpiod);

This isn't going to work at all if the GPIO is shared between multiple
regulators but I can't immediately see a sensible way to fix that
without some surgery on the GPIO APIs so let's leave it for now.


signature.asc
Description: PGP signature

Re: [RFT PATCH 1/3] usb: misc: usb3503: Fix HUB mode after bootloader initialization

[Mark Brown]

On Wed, May 04, 2016 at 02:01:57PM +0200, Krzysztof Kozlowski wrote:

> This looks like pwrseq for MMC devices so the idea is to:
> 1. Move drivers/mmc/core/pwrseq* to separate directory
> (drivers/power/pwrseq ?)
> 2. Add support for pwrseq to USB core,
> 3. Add new pwrseq driver (or extend existing one): toggling the regulator.
> 4. Add pwrseq phandle to the DT node of USB hub (to the binding
> mentioned by Rob?).

> Does it look sensible?

It certainly seems like a similar problem space, USB seems very much
like MMC in both the spec and the way it tends to get used.


signature.asc
Description: PGP signature

Re: [RESEND PATCH v10 4/4] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Wed, May 04, 2016 at 08:59:23AM +0530, Manish Badarkhe wrote:

> > They're in the silicon, it's just a table of values that were put into
> > the silicon at design time.  The defines would just be TABLE_ENTRY_1 or
> > whatever.

> Thanks for the clarification, In that case, comments/documentation
> will work instead of making any defines.

This is a *really* common pattern in drivers.


signature.asc
Description: PGP signature

Re: [PATCH v7 1/7] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Tue, May 03, 2016 at 09:43:58AM +0800, Lu Baolu wrote:
> On 05/02/2016 07:00 PM, Mark Brown wrote:
> > On Fri, Apr 29, 2016 at 02:26:32PM +0800, Lu Baolu wrote:

> >> +  gpiod = gpiod_get(dev, "vbus_en", GPIOD_ASIS);
> >> +  if (IS_ERR(gpiod))
> >> +  return PTR_ERR(gpiod);

> > This is clearly an inappropriate name for the signal in generic code,
> > it's specific to your use case.

> I will change the gpio name to "gpio". Is that okay?

Yes, that looks good (and lines up with DT so hopefully the code can be
shared).


signature.asc
Description: PGP signature

Re: [RESEND PATCH v10 4/4] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Tue, May 03, 2016 at 09:30:48AM +0530, Manish Badarkhe wrote:
> On Tue, May 3, 2016 at 9:00 AM, Baolin Wang  wrote:

> > +static const unsigned int wm831x_usb_limits[] = {
> > +   0,
> > +   2,
> > +   100,
> > +   500,
> > +   900,
> > +   1500,
> > +   1800,
> > +   550,
> > +};

> Just for curiosity, How these current limits are getting decided?
> Can we have some proper defines over here so that it can be grasped easily?

They're in the silicon, it's just a table of values that were put into
the silicon at design time.  The defines would just be TABLE_ENTRY_1 or
whatever.


signature.asc
Description: PGP signature

Re: [PATCH v7 1/7] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Fri, Apr 29, 2016 at 02:26:32PM +0800, Lu Baolu wrote:

> + gpiod = gpiod_get(dev, "vbus_en", GPIOD_ASIS);
> + if (IS_ERR(gpiod))
> + return PTR_ERR(gpiod);

This is clearly an inappropriate name for the signal in generic code,
it's specific to your use case.


signature.asc
Description: PGP signature

Re: [RFT PATCH 1/3] usb: misc: usb3503: Fix HUB mode after bootloader initialization

[Mark Brown]

On Mon, May 02, 2016 at 11:49:12AM +0200, Krzysztof Kozlowski wrote:

> This VDD regulator supply actually is not a usb3503 USB HUB regulator
> supply... but a supply to the LAN attached to this HUB. Regulator off/on
> is needed for LAN to show up. The hub will show up with typical reset
> (which is also missing before my patchset btw).

> The LAN, as a USB device, is auto-probed so it cannot take the regulator
> and play with it. The simplest idea I have is to add it as "external
> supply"  to the parent: usb3503.

This is common enough that that just isn't going to scale well I fear
without some generic handling, either walking child devices at the bus
level or at the device level with a pre-probe() callback to get the
device to power on.  The latter is more appropriate to things like
Slimbus where the device is more likely to do active management at
runtime, it's not clear people are building USB devices like that.


signature.asc
Description: PGP signature

Re: [RFT PATCH 1/3] usb: misc: usb3503: Fix HUB mode after bootloader initialization

[Mark Brown]

On Fri, Apr 29, 2016 at 01:55:14PM +0200, Krzysztof Kozlowski wrote:
> On 04/29/2016 01:30 PM, Mark Brown wrote:

> > Supplies are only optional if they may be physically absent.  In this
> > case it's possible that on device regulators may be used instead, a
> > pattern more like that used for arizona-ldo1 where we represent those
> > regulators might be better as it's more clearly describing the
> > situation.  I'm just wondering if the supply lookup stuff there should
> > be factored out as this is not an uncommon pattern..

> > It should at least be clearly stated what's going on, ignoring failure
> > to get supplies is generally a bug and people will tend to blindly cut
> > and paste things (witness all the breakage in graphics drivers with
> > this).

> The VDD33 is really optional. The device can work in different
> configuration, e.g. only on VBAT. How the reset logic would work then? I
> don't know... I would suspect that it could be exactly the same (just
> replace VDD33 with VBAT) but I am not sure.

What the Arizona example I mentioned does is look for the property
specifying an external supply in DT and if there isn't one assumes that
it must be using the internal regulator.  That's a bit icky but it does
the right thing and is much simpler from a user point of view.


signature.asc
Description: PGP signature

Applied "regulator: max77686: Configure enable time to properly handle regulator enable" to the regulator tree

[Mark Brown]

The patch

   regulator: max77686: Configure enable time to properly handle regulator 
enable

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From a9597305d97f6cf7c9e89dc1461e834c446d91fd Mon Sep 17 00:00:00 2001
From: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Date: Fri, 29 Apr 2016 12:59:51 +0200
Subject: [PATCH] regulator: max77686: Configure enable time to properly handle
 regulator enable

The enable time for buck regulators was not configured but actually is
essential: consumers, like usb3503, doing hard reset (regulator off/on)
should wait for the regulator to settle.

Configure the enable time according to datasheet.

Signed-off-by: Krzysztof Kozlowski <k.kozlow...@samsung.com>
Signed-off-by: Mark Brown <broo...@kernel.org>
---
 drivers/regulator/max77686-regulator.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/drivers/regulator/max77686-regulator.c 
b/drivers/regulator/max77686-regulator.c
index d1ab6a4da88f..ac4fa581e0a5 100644
--- a/drivers/regulator/max77686-regulator.c
+++ b/drivers/regulator/max77686-regulator.c
@@ -41,6 +41,8 @@
 #define MAX77686_LDO_LOW_UVSTEP25000
 #define MAX77686_BUCK_MINUV75
 #define MAX77686_BUCK_UVSTEP   5
+#define MAX77686_BUCK_ENABLE_TIME  40  /* us */
+#define MAX77686_DVS_ENABLE_TIME   22  /* us */
 #define MAX77686_RAMP_DELAY10  /* uV/us */
 #define MAX77686_DVS_RAMP_DELAY27500   /* uV/us */
 #define MAX77686_DVS_MINUV 60
@@ -422,6 +424,7 @@ static struct regulator_ops max77686_buck_dvs_ops = {
.min_uV = MAX77686_BUCK_MINUV,  \
.uV_step= MAX77686_BUCK_UVSTEP, \
.ramp_delay = MAX77686_RAMP_DELAY,  \
+   .enable_time= MAX77686_BUCK_ENABLE_TIME,\
.n_voltages = MAX77686_VSEL_MASK + 1,   \
.vsel_reg   = MAX77686_REG_BUCK5OUT + (num - 5) * 2,\
.vsel_mask  = MAX77686_VSEL_MASK,   \
@@ -439,6 +442,7 @@ static struct regulator_ops max77686_buck_dvs_ops = {
.min_uV = MAX77686_BUCK_MINUV,  \
.uV_step= MAX77686_BUCK_UVSTEP, \
.ramp_delay = MAX77686_RAMP_DELAY,  \
+   .enable_time= MAX77686_BUCK_ENABLE_TIME,\
.n_voltages = MAX77686_VSEL_MASK + 1,   \
.vsel_reg   = MAX77686_REG_BUCK1OUT,\
.vsel_mask  = MAX77686_VSEL_MASK,   \
@@ -456,6 +460,7 @@ static struct regulator_ops max77686_buck_dvs_ops = {
.min_uV = MAX77686_DVS_MINUV,   \
.uV_step= MAX77686_DVS_UVSTEP,  \
.ramp_delay = MAX77686_DVS_RAMP_DELAY,  \
+   .enable_time= MAX77686_DVS_ENABLE_TIME, \
.n_voltages = MAX77686_DVS_VSEL_MASK + 1,   \
.vsel_reg   = MAX77686_REG_BUCK2DVS1 + (num - 2) * 10,  \
.vsel_mask  = MAX77686_DVS_VSEL_MASK,   \
-- 
2.8.0.rc3

--
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: [RFT PATCH 1/3] usb: misc: usb3503: Fix HUB mode after bootloader initialization

[Mark Brown]

On Fri, Apr 29, 2016 at 12:59:49PM +0200, Krzysztof Kozlowski wrote:

> +++ b/Documentation/devicetree/bindings/usb/usb3503.txt
> @@ -24,6 +24,7 @@ Optional properties:
>   pins (optional, if not provided, driver will not set rate of the
>   REFCLK signal and assume that a value from the primary reference
>   clock frequencies table is used)
> +- vdd33-supply: Optional supply for VDD 3.3 V power source.

Supplies are only optional if they may be physically absent.  In this
case it's possible that on device regulators may be used instead, a
pattern more like that used for arizona-ldo1 where we represent those
regulators might be better as it's more clearly describing the
situation.  I'm just wondering if the supply lookup stuff there should
be factored out as this is not an uncommon pattern..

It should at least be clearly stated what's going on, ignoring failure
to get supplies is generally a bug and people will tend to blindly cut
and paste things (witness all the breakage in graphics drivers with
this).

>  static int usb3503_reset(struct usb3503 *hub, int state)
>  {
> + int err;
> +
> + err = usb3503_regulator(hub, state);
> + if (err) {
> + dev_err(hub->dev, "unable to %s VDD33 regulator to (%d)\n",
> + (state ? "enable" : "disable"), err);
> + }

Are we sure that the callers all balance enables and disables and we
don't ever end up going through reset more than once on the way down?

> + hub->vdd_reg = devm_regulator_get_optional(dev, "vdd33");
> + if (IS_ERR(hub->vdd_reg)) {
> + if (PTR_ERR(hub->vdd_reg) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;

This should explicitly check for -ENODEV and return the error if it gets
anything else, that will mean that if the supply is needed but lookup
fails somehow due to a non-deferral error we'll handle it properly.

> + err = usb3503_regulator(hub, true);

The naming on this function is very obscure (and there's also a couple
of other supplies).  I'd suggest just folding this into the reset
function, or at least renaming so the reader can tell what these calls
do.


signature.asc
Description: PGP signature

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Thu, Apr 28, 2016 at 01:55:35PM +0800, Lu Baolu wrote:

> How about below code?

> +   gpiod = gpiod_get(dev, "vbus_en", GPIOD_ASIS);
> +   if (IS_ERR(gpiod))
> +   return PTR_ERR(gpiod);
> +
> +   config->gpio = desc_to_gpio(gpiod);
> +   config->enable_high = device_property_read_bool(dev,
> +   "enable-active-high");
> +   gpiod_put(gpiod);


That looks reasonable, though if you use "gpio" as the name like DT you
could keep the code the same which would be even better?  Not super
crticial though.


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 06:25:16PM +0800, Jisheng Zhang wrote:
> On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote:
> > On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:

> > > > +   vbus = devm_regulator_get(>dev, "vbus");  

> > > devm_regulator_get_optional() ??  

> > Does USB work without a VBUS?  Unless the answer is yes then I'd expect
> > this to be just a normal regulator_get().

> Per spec no. But the vbus may be transparent to SW on some platforms, so I
> think devm_regulator_get_optional() is better.

Really, no.  If it's physically there the software needs to be written
as such.  Please see the documentation and list archives for extensive
discussion on this topic.

> > This is all completely broken unless the supply is optional.

> The supply is optional.

To repeat a supply is only optional if it might be physically absent.


signature.asc
Description: PGP signature

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Wed, Apr 27, 2016 at 09:54:10AM +0800, Lu Baolu wrote:

> Please refer to Documentation/acpi/gpio-properties.txt.

That's not visibly what your driver is doing, that is also recommending
using a static name which is what I'm asking for.

> > Why does the device care?It's requesting the GPIO in
> > its own context and it's only requesting one GPIO, with DT we're just
> > always calling the GPIO "gpio" which works fine.

> This driver is not bound to an ACPI device node directly. It's a child
> of a mfd device, which is corresponding to a real ACPI device node.

If it's the child of a MFD it's got an ACPI device, the ACPI device is
the parent.  It should use the parent device or the parent should map
the GPIO through to the child as many other MFDs do, the whole concept
of a MFD is a Linux internal implementation detail.


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
> Mark Brown <broo...@kernel.org> writes:

> > this to be just a normal regulator_get().

> jokes aside, this regulator is optional because not all platforms
> require a SW controlled regulator, no ? Will normal regulator_get() give
> us a dummy regulator in case it's not listed in DT/ACPI ?

Yes we do that, but even regulators that are not software controlled
should really be described anyway since it's a much simpler rule for
people to understand, it ensures that we can just scale up on systems
where there does happen to be software control and it makes all the
resulting code much simpler and hence less error prone if we're not
randomly ignoring some errors.


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
> Jisheng Zhang  writes:

> > +   vbus = devm_regulator_get(>dev, "vbus");

> devm_regulator_get_optional() ??

Does USB work without a VBUS?  Unless the answer is yes then I'd expect
this to be just a normal regulator_get().

> 
> > +   if (PTR_ERR(vbus) == -ENODEV) {
> > +   vbus = NULL;
> > +   } else if (IS_ERR(vbus)) {
> > +   ret = PTR_ERR(vbus);
> > +   goto disable_clk;
> > +   } else if (vbus) {
> > +   ret = regulator_enable(vbus);
> > +   if (ret) {
> > +   dev_err(>dev,
> > +   "failed to enable usb vbus regulator: %d\n",
> > +   ret);
> > +   goto disable_clk;
> > +   }
> > +   }

This is all completely broken unless the supply is optional.


signature.asc
Description: PGP signature

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Tue, Apr 26, 2016 at 10:24:56AM +0800, Lu Baolu wrote:

> The GPIO name might be different in different use cases. For my case,
> it is "vbus_en", but other cases should use the different name.

> On ACPI compatible platforms, GPIO resources are reported via ACPI
> tables and (devm_)gpiod_get() hides the APCI complexity and returns
> the gpiod according to "gpio_name".

That's labelling that you might want to do on the supplier side or at
system level.  Why does the device care?  It's requesting the GPIO in
its own context and it's only requesting one GPIO, with DT we're just
always calling the GPIO "gpio" which works fine.


signature.asc
Description: PGP signature

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Mon, Apr 25, 2016 at 04:04:50PM +0800, Lu Baolu wrote:

> + ret = device_property_read_string(dev, "gpio-name", _name);
> + if (!ret) {
> + gpiod = gpiod_get(dev, gpio_name, GPIOD_ASIS);
> + if (!IS_ERR(gpiod)) {

This doesn't look like it's a standard ACPI binding for GPIOs, why are
we using a property to get the GPIO noame?


signature.asc
Description: PGP signature

Re: [PATCH v6 03/10] regulator: fixed: add device binding for platform device

[Mark Brown]

On Mon, Apr 25, 2016 at 04:04:49PM +0800, Lu Baolu wrote:

> This is needed to handle the GPIO connected USB vcc pin found on
> Intel Baytrail devices.

In what way is this needed?  The we defualt to using the driver name if
no platform ID table, AFAICT this is just restating the same string?


signature.asc
Description: PGP signature

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

[Mark Brown]

On Tue, Apr 05, 2016 at 05:43:20PM +0800, Peter Chen wrote:
> On Tue, Apr 05, 2016 at 05:34:02PM +0800, Baolin Wang wrote:

> > Mark, could you please address Peter's comments about if the the
> > wm831x can charge more than 1500mA for DCP? (I have no environment to
> > test wm831x) Thanks.

> I don't want you or Mark to test at hardware, I just would like to see
> some code that how PMIC, wm831x, and USB gadget driver work together.

If you want to see this running it's going to be easier for you to just
write an equivalent driver that just does a print instead of writing to
a register (which is all that the wm831x driver boils down to) and test
on hardware you have in front of you.


signature.asc
Description: PGP signature

Re: [PATCH v9 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Tue, Apr 05, 2016 at 04:12:22PM +0800, Peter Chen wrote:
> On Tue, Apr 05, 2016 at 03:54:31PM +0800, Baolin Wang wrote:
> > Hi Peter,

> > Yeah, this patchset did not give an example to read charger type from
> > PMIC registers. (Cause now the user 'wm831x_power' don't need this.)
> > But I think user can get it easily by implementing below callbacks.
> > (1) gadget->ops->get_charger_type();
> > (2) power_supply_get_property(uchger->psy, POWER_SUPPLY_PROP_CHARGE_TYPE, 
> > );
> > (3) uchger->get_charger_type();

> I just would like if you can have this, then, we (you) can test it, eg,
> you can test if the wm831x can charge more than 1500mA for DCP.

The hardware in the wm831x is extremely simple here - all it does is
take in the power rail from USB, apply a current limit to it and feed
it into the power source selection circuitry it has.  It doesn't see any
of the USB data signals, it relies on the rest of the system to identify
and configure the limit by writing a register.  The highest limit it
supports is 1.8A.

> > The framework does not want to focus on charger detection too much,
> > and just supplies one callback '->charger_detect()' for user to be
> > implemented if they ensure they need to do the SW charger detection
> > manually (Note: must at the right time to do the SW detection.). So
> > the usb charger just focus on dealing with the usb gadget power
> > negotiation, and it does not need to care much how to do charger
> > detection on your platform.

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


signature.asc
Description: PGP signature

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

[Mark Brown]

On Mon, Apr 04, 2016 at 01:47:50PM +0300, Felipe Balbi wrote:
> Mark Brown <broo...@kernel.org> writes:

> > It does in this new world order.  IIRC on an earlier round of review
> > there was some code that didn't use a bus but that got complaints that
> > it was trying to reimplement the bus functionality.

> fair enough, I'll wait for Greg to have some time to comment on
> this. Bottomline is that there is *no* real bus. Charger ICs will use

I've got a feeling Greg is zoning this out by now...

> SPI or I2C and that's a real bus, $subject is not.

Well, there's the physical connection between the system power supply
and the USB port if you're very keen on looking for hardware.


signature.asc
Description: PGP signature

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

[Mark Brown]

On Fri, Apr 01, 2016 at 08:43:10AM +0300, Felipe Balbi wrote:
> Mark Brown <broo...@kernel.org> writes:

> > IIRC Greg didn't want new classes?

> good point. Still, this doesn't seem to fit a but_type IMO.

It does in this new world order.  IIRC on an earlier round of review
there was some code that didn't use a bus but that got complaints that
it was trying to reimplement the bus functionality.


signature.asc
Description: PGP signature

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

[Mark Brown]

On Thu, Mar 31, 2016 at 09:42:58AM +0300, Felipe Balbi wrote:
> Baolin Wang  writes:

> > I want to use bus structure to manage the charger device. Maybe choose
> > class to manage them?

> I guess a class would fit better in this case.

IIRC Greg didn't want new classes?


signature.asc
Description: PGP signature

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

[Mark Brown]

On Wed, Mar 30, 2016 at 01:09:00PM +0300, Felipe Balbi wrote:
> Baolin Wang  writes:

> > +#include 
> > +#include 
> > +#include 
> > +#include 

> not very nice to depend on either of or platform_device here. What about
> PCI-based devices ?

The header inclusion shouldn't be conditional though.  But looking at
the patch I can't immediately see any use of these in the code anyway.

> > +static DEVICE_ATTR_RW(sdp_limit);

> why RW ? Who's going to use these ? Also, you're not documenting this
> new sysfs file.

If they end up not writeable should we just remove them entirely since
they should just be the spec values?


signature.asc
Description: PGP signature

Re: [PATCH v8 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Mon, Mar 28, 2016 at 03:13:51PM +0800, Peter Chen wrote:
> On Mon, Mar 28, 2016 at 02:51:40PM +0800, Baolin Wang wrote:

> > > I am afraid I still not find the user (udc driver) for this framework, I 
> > > would
> > > like to see how udc driver block the enumeration until the charger 
> > > detection
> > > has finished, or am I missing something?

> > It is not for udc driver but for power users who want to negotiate
> > with USB subsystem.

> Then, where is the code the test user to decide what kinds of USB charger
> (SDP, CDP, DCP) is connecting now?

Even without detection of CDP and DCP we have configurability within SDP
- there's the 2.5mA suspended limit, the 100mA default limit and the
higher 500mA limit which can be negotiated.


signature.asc
Description: PGP signature

Re: [PATCH v7 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Wed, Mar 16, 2016 at 01:05:27PM +0200, Felipe Balbi wrote:
> Mark Brown <broo...@kernel.org> writes:
> > On Mon, Feb 29, 2016 at 11:22:12PM +0900, Mark Brown wrote:
> >> On Mon, Jan 04, 2016 at 11:04:26AM +0800, Baolin Wang wrote:

> > I see Felipe is no longer at TI so his e-mail was bouncing - let's
> > resend this with his kernel.org address:

> I don't have the patches on my inbox. Neither on kernel.org nor on my

Right, they were last posted in January before you updated MAINTAINERS
so they'll have gone to your TI address.

> linux.intel.com account. Care to resend ?

Baolin, can you do that please?


signature.asc
Description: PGP signature

Re: [PATCH v7 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Mon, Feb 29, 2016 at 11:22:12PM +0900, Mark Brown wrote:
> On Mon, Jan 04, 2016 at 11:04:26AM +0800, Baolin Wang wrote:

I see Felipe is no longer at TI so his e-mail was bouncing - let's
resend this with his kernel.org address:

> > Currently the Linux kernel does not provide any standard integration of this
> > feature that integrates the USB subsystem with the system power regulation
> > provided by PMICs meaning that either vendors must add this in their kernels
> > or USB gadget devices based on Linux (such as mobile phones) may not behave
> > as they should. Thus provide a standard framework for doing this in kernel.

> So, the review of this seems to have ground to a bit of a halt - we're
> really not seeing any engagement or comments here, people aren't raising
> any problems or suggesting alternative approaches but this isn't moving
> forwards either.  This means that nothing running mainline that isn't
> totally offloaded to hardware can charge at even 500mA, let alone more,
> which seems like a failure to me.  What do we need to move this
> forwards?

> If there are concerns around ABI we could either make sure it's as basic
> as possible (so that it'll be easy to maintain compatibility if we think
> of something better) or just hide things from userspace so that we just
> have the in kernel implementation.




signature.asc
Description: PGP signature

Re: [PATCH v7 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Mark Brown]

On Mon, Jan 04, 2016 at 11:04:26AM +0800, Baolin Wang wrote:

> Currently the Linux kernel does not provide any standard integration of this
> feature that integrates the USB subsystem with the system power regulation
> provided by PMICs meaning that either vendors must add this in their kernels
> or USB gadget devices based on Linux (such as mobile phones) may not behave
> as they should. Thus provide a standard framework for doing this in kernel.

So, the review of this seems to have ground to a bit of a halt - we're
really not seeing any engagement or comments here, people aren't raising
any problems or suggesting alternative approaches but this isn't moving
forwards either.  This means that nothing running mainline that isn't
totally offloaded to hardware can charge at even 500mA, let alone more,
which seems like a failure to me.  What do we need to move this
forwards?

If there are concerns around ABI we could either make sure it's as basic
as possible (so that it'll be easy to maintain compatibility if we think
of something better) or just hide things from userspace so that we just
have the in kernel implementation.


signature.asc
Description: PGP signature

Re: [PATCH 0/3] USB: PHY: minor include cleanups

[Mark Brown]

On Tue, Feb 02, 2016 at 02:02:30PM -0600, Bjorn Helgaas wrote:
> Remove some gpio and regulator #includes when they can be replaced by
> trivial forward struct declarations.  Also move a linux/gpio/consumer.h
> #include from a header to the single .c files that uses it.

Please don't CC me on patches just for simple work on users of the
regulator API - if there's some regulator specific input needed then of
course that's fine but if it's just normal API user stuff there's no
need.


signature.asc
Description: PGP signature

Re: [PATCH v5 1/5] gadget: Introduce the notifier functions

[Mark Brown]

On Fri, Nov 06, 2015 at 08:56:44AM -0800, Greg KH wrote:
> On Fri, Nov 06, 2015 at 07:35:10PM +0800, Baolin Wang wrote:

> > Thus this patch adds a notifier mechanism for usb gadget to report a
> > event to usb charger when the usb gadget state is changed.

> I thought we said we did not want another notifier chain in the previous
> versions of this patch?

Did we come up with anything better?


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Sat, Oct 24, 2015 at 04:17:12PM +0200, Rafael J. Wysocki wrote:

> Well, I'm not quite sure why exactly everyone is so focused on probing here.

Probe deferral is really noisy even if it's working fine on a given
system so it's constantly being highlighted to people in a way that
other issues aren't if you're not directly having problems.

There's also the understanding people had that the order things get
bound changes the ordering for some of the other cases (perhaps it's a
good idea to do that, it seems likely to be sensible?).



signature.asc
Description: PGP signature

Re: Alternative approach to solve the deferred probe (was: [GIT PULL] On-demand device probing)

[Mark Brown]

On Tue, Oct 20, 2015 at 04:46:56PM +0100, Russell King - ARM Linux wrote:

> Something like this.  I haven't put a lot of effort into it to change all
> the places which return an -EPROBE_DEFER, and it also looks like we need
> some helpers to report when we have only an device_node (or should that
> be fwnode?)  See the commented out of_warn_deferred() in
> drivers/gpio/gpiolib-of.c.  Adding this stuff in the subsystems searching
> for resources should make debugging why things are getting deferred easier.

Yeah, plus I'd expect it to also result in better error reporting
overall if the subsystems are able to report when they fail to get
something rather than just returning an error to the driver.

> +/**
> + * dev_warn_deferred() - report why a probe has been deferred
> + */
> +void dev_warn_deferred(struct device *dev, const char *fmt, ...)
> +{
> + if (driver_deferred_probe_report) {
> + struct va_format vaf;
> + va_list ap;
> +
> + va_start(ap, fmt);
> + vaf.fmt = fmt;
> + vaf.va = 
> +
> + dev_warn(dev, "deferring probe: %pV", );
> + va_end(ap);
> + }
> +}
> +EXPORT_SYMBOL_GPL(dev_warn_deferred);

I'm not currently able to think of a nice way of writing this but I think
what I'd really like to see from a driver point of view is something
which decays into dev_err() if it's a non-deferral error.  That way
drivers can have minimal log and return error handling code and we will
still get the output sensibly.  The best I can think of is something
like

   void dev_warn_deferred(struct device *dev, int err, const char *fmt, ...)

which requires the caller to pass in err twice to get it logged.  That's
not a thing of beauty but it gets the job done...  but perhaps your
original interface is better, it's a bit cleaner.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Thu, Oct 22, 2015 at 04:02:13PM +0100, Russell King - ARM Linux wrote:

> If it was such a problem, then in the _eight_ days that this has been
> discussed so far, _someone_ would have sent some data showing the
> problem.  I think the fact is, there is no data.

> Someone prove me wrong.  Someone post the verifiable data showing that
> there is a problem to be solved here.

> Someone show what the specific failure cases are that are hampering
> vendors moving forwards.  Someone show the long boot times by way of
> kernel message log.  Someone show some evidence of the problems that
> have been alluded to.

> If no one can show some evidence, there isn't a problem here. :)

Yeah, I'm not convinced the timing is *such* a big deal either - I do
think that the log spam is a real problem but I think something much
less invasive like the interface you proposed is good for addressing
that.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Wed, Oct 21, 2015 at 08:59:51AM -0700, Frank Rowand wrote:
> On 10/19/2015 5:34 AM, Tomeu Vizoso wrote:

> > To be clear, I was saying that this series should NOT affect total
> > boot times much.

> I'm confused.  If I understood correctly, improving boot time was
> the key justification for accepting this patch set.  For example,
> from "[PATCH v7 0/20] On-demand device probing":
> 
>I have a problem with the panel on my Tegra Chromebook taking longer
>than expected to be ready during boot (Stéphane Marchesin reported what
>is basically the same issue in [0]), and have looked into ordered
>probing as a better way of solving this than moving nodes around in the
>DT or playing with initcall levels and linking order.
> 
>...
> 
>With this series I get the kernel to output to the panel in 0.5s,
>instead of 2.8s.

Overall boot time and time to get some individual built in component up
and running aren't the same thing - what this'll do is get things up
more in the link order of the leaf consumers rather than deferring those
leaf consumers when their dependencies aren't ready yet.

> While not as dramatic as your results, they are somewhat supportive.
> What has changed your assessment that the on-demand device probing
> patches will give a big boot performance increase?  Do you have
> new data or analysis?

See above, my understanding was that the performance improvements were
more around improved control/predictability/handwave of the boot
ordering rather than total time.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Wed, Oct 21, 2015 at 11:18:08AM -0700, Frank Rowand wrote:
> On 10/21/2015 9:27 AM, Mark Brown wrote:

> > Overall boot time and time to get some individual built in component up
> > and running aren't the same thing - what this'll do is get things up
> > more in the link order of the leaf consumers rather than deferring those
> > leaf consumers when their dependencies aren't ready yet.

> Thanks!  I read too much into what was being improved.

> So this patch series, which on other merits may be a good idea, is as
> a by product solving a specific ordering issue, moving successful panel
> initialization to an earlier point in the boot sequence, if I now
> understand more correctly.

Yeah, that's my understanding.

> In that context, this seems like yet another ad hoc way of causing the
> probe order to change in a way to solves one specific issue?  Could
> it just as likely move the boot order of some other driver on some
> other board later, to the detriment of somebody else?

Indeed.  My general feeling is that it does make the link order stuff
more predictable and easier to work with and it does have other merits
(in terms of the error reporting, though there's other ways to address
that like the one Russell is proposing).


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Tue, Oct 20, 2015 at 01:14:46PM -0400, Alan Stern wrote:
> On Tue, 20 Oct 2015, Tomeu Vizoso wrote:

> > This iteration of the series would make this quite easy, as
> > dependencies are calculated before probes are attempted:

> > https://lkml.org/lkml/2015/6/17/311

> But what Rafael is proposing is quite general; it would apply to _all_
> dependencies as opposed to just those present in DT drivers or those 
> affecting platform_devices.

We'll still need most of the DT bits that are there at the minute (the
ones strewn around the subsystems) AFAICT since it's at the point where
we parse the DT and work out what the dependencies are which we probably
want to do prior to getting the drivers up and will be different for
ACPI.  I think the level of DT dependency here looks a lot larger than
it actually is due to the fact that a lot of what's being modified is DT
parsing code.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Tue, Oct 20, 2015 at 10:40:03AM -0400, Alan Stern wrote:

> Furthermore, that applies only to devices that use synchronous suspend.  
> Async suspend is becoming common, and there the only restrictions are 
> parent-child relations plus whatever explicit requirements that drivers 
> impose by calling device_pm_wait_for_dev().

Hrm, this is the first I'd noticed that feature though I see the initial
commit dates from January.  It looks like most of the users are PCs at
the minute but we should be using it more widely for embedded things,
there's definitely some cases I'm aware of where it will allow us to
remove some open coding.

It does seem like we want to be feeding dependency information we
discover for probing way into the suspend dependencies...


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Mon, Oct 19, 2015 at 10:44:41AM +0100, David Woodhouse wrote:
> On Sun, 2015-10-18 at 20:53 +0100, Mark Brown wrote:

> > Do you mean firmware rather than bus here?  I think that's the confusion
> > I have...

> Certainly, if it literally is adding of_* calls then that would seem to
> be gratuitously firmware-specific. Nothing should be using those these
> days; any new code should be using the generic device property APIs
> (except in special cases).

It's not entirely clear to me that we should be moving to fwnode_
wholesale yet - the last advice was to hold off for a little while which
makes sense given that the ACPI community still doesn't seem to have
worked out what it wants to do here and how.  The x86 embedded people
are all gung ho but it's less clear that anyone else wants to use _DSD
in quite the same way (I know of some efforts to use _DSD separately to
the DT compatibility stuff) and there are some vendors who definitely do
have completely different binding schemes for ACPI and DT and therefore
specifically care which is in use.

It would really help if ACPI could get their binding review process in
place, and if we do want to actually start converting everything to
fwnode_ we need to start communicating that actively since otherwise
people can't really be expected to know.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Mon, Oct 19, 2015 at 01:47:50PM +0100, David Woodhouse wrote:
> On Mon, 2015-10-19 at 07:35 -0500, Rob Herring wrote:

> > See version 2 of the series[1] which did that. It became obvious that
> > was pointless because the call paths ended up looking like this:

> > Generic subsystem code -> DT look-up code -> fwnode_probe_device ->
> > of_probe_device

> You link to a thread which says that "AT LEAST CURRENTLY, the calling
> locations [the 'DT look-up code' you mention above] are DT specific
> functions anyway.

> But the point I'm making is that we are working towards *fixing* that,
> and *not* using DT-specific code in places where we should be using the
> generic APIs.

What is the plan for fixing things here?  It's not obvious (at least to
me) that we don't want to have the subsystems having knowledge of how
they are bound to a specific firmware which is what you seem to imply
here.  That seems like it's going to fall down since the different
firmware interfaces do have quite different ideas about how things fit
together at a system level and different compatibility needs which do
suggest that just trying to do a direct mapping from DT into ACPI may
well not make people happy but it sounds like that's the intention.

When it gets to drivers the situation is much more clear since it's
normally just simple properties, it's generally a bit more worrying if
drivers are needing to directly interact with cross-device linkage.
This is all subsystem level code though.

> None of that really negates that fact that we are *working* on cleaning
> these code paths up to be firmware-agnostic, and the fact that we
> haven't got to this one *yet* isn't necessarily a good reason to make
> it *worse* by adding new firmware-specificity to it.

It seems like we're going to have to refactor these bits of code when
they get generalised anyway so I'm not sure that the additional cost
here is that big.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Mon, Oct 19, 2015 at 04:29:40PM +0100, David Woodhouse wrote:
> On Mon, 2015-10-19 at 15:50 +0100, Mark Brown wrote:
> > > But the point I'm making is that we are working towards *fixing* that,
> > > and *not* using DT-specific code in places where we should be using the
> > > generic APIs.

> > What is the plan for fixing things here?  It's not obvious (at least to
> > me) that we don't want to have the subsystems having knowledge of how
> > they are bound to a specific firmware which is what you seem to imply
> > here. 

> I don't know that there *is* a coherent plan here to address it all.

> Certainly, we *will* need subsystems to have firmware-specific
> knowledge in some cases. Take GPIO as an example; ACPI *has* a way to
> describe GPIO, and properties which reference GPIO pins are intended to
> work through that — while in DT, properties which reference GPIO pins
> will have different contents. They'll be compatible at the driver
> level, in the sense that there's a call to get a given GPIO given the
> property name, but the subsystems *will* be doing different things
> behind the scenes.

I'd expect that to be the norm rather than the exception.

> > It seems like we're going to have to refactor these bits of code when
> > they get generalised anyway so I'm not sure that the additional cost
> > here is that big.

> That's an acceptable answer — "we're adding legacy code here but we
> know it's going to be refactored anyway". If that's true, all it takes
> is a note in the commit comment to that effect. That's different from
> having not thought about it :)

Given the above I'm not even sure it's legacy code, it's just as likely
we're going to get some parallel ACPI code added to the subsystems for
parsing their bindings.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Sat, Oct 17, 2015 at 08:47:09AM -0700, Greg Kroah-Hartman wrote:
> On Sat, Oct 17, 2015 at 10:04:55AM -0500, Rob Herring wrote:

> > I think Linus W, Mark B, and I all said a similar thing initially in
> > that dependencies should be handled in the driver core. We went down
> > the path of making this not firmware (aka bus) specific and an earlier
> > version had just that (with fwnode_* calls). That turned out to be
> > pointless as the calling locations were almost always in DT specific
> > code anyway. If you notice, the calls are next to other DT specific
> > calls generally (usually a "get"). So yes, I'd prefer not to have to
> > touch every subsystem, but we had to do that anyway to add DT support.

> If they are "next" to a call like that, why not put it in that call?  I
> really object to having to "sprinkle" this all over the kernel, for no
> obvious reason why that is happening at all (look at the USB patch for
> one such example.)

I did ask that question myself IIRC - we could probably get a long way
by trying to instantiate anything that looks probable when we do a
phandle lookup on it.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Sun, Oct 18, 2015 at 12:37:57PM -0700, Greg Kroah-Hartman wrote:
> On Sun, Oct 18, 2015 at 08:29:31PM +0100, Mark Brown wrote:
> > On Fri, Oct 16, 2015 at 11:57:50PM -0700, Greg Kroah-Hartman wrote:

> > > I can't see adding calls like this all over the tree just to solve a
> > > bus-specific problem, you are adding of_* calls where they aren't
> > > needed, or wanted, at all.

> > This isn't bus specific, I'm not sure what makes you say that?

> You are making it bus-specific by putting these calls all over the tree
> in different bus subsystems semi-randomly for all I can determine.

Do you mean firmware rather than bus here?  I think that's the confusion
I have...

> > One is that regardless of the actual performance of the system when
> > deferred probe goes off it splats errors all over the console which
> > makes it look like something is going wrong even if everything is fine
> > in the end.  If lots of deferred probing happens then the volume gets
> > big too.  People find this distracting, noisy and ugly - it obscures
> > actual issues and trains people to ignore errors.  I do think this is a
> > reasonable concern and that it's worth trying to mitigate against
> > deferral for this reason alone.  We don't want to just ignore the errors
> > and not print anything either since if the resource doesn't appear the
> > user needs to know what is preventing the driver from instantiating so
> > they can try to fix it.

> This has come up many times, I have no objection to just turning that
> message into a debug message that can be dynamically enabled for those
> people wanting to debug their systems for boot time issues.

It's not just the driver core logging, it's also all the individual
drivers logging that they failed to get whatever resource since silently
failing is not a great user experience.  Many, hopefully most, of the
drivers don't actually have special handling for probe deferral since
half the beauty of probe deferral is that the subsystem supplying the
resource can just return -EPROBE_DEFER when it notices something is
missing but might appear and then the drivers will do the right thing so
long as they have error handling code that they really should have
anyway.

We'd need to have a special dev_err() that handled probe deferral
errors for drivers to use during probe or some other smarts in the
logging infrastructure.  Which isn't a totally horrible idea.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Fri, Oct 16, 2015 at 11:57:50PM -0700, Greg Kroah-Hartman wrote:

> I can't see adding calls like this all over the tree just to solve a
> bus-specific problem, you are adding of_* calls where they aren't
> needed, or wanted, at all.

This isn't bus specific, I'm not sure what makes you say that?

> What is the root-problem of your delay in device probing?  I read your
> last patch series and I can't seem to figure out what the issue is that
> this is solving in any "better" way from the existing deferred probing.

So, I don't actually have any platforms that are especially bothered by
this (at least not for my use cases) so there's a bit of educated
guessing going on here but there's two broad things I'm aware of.  

One is that regardless of the actual performance of the system when
deferred probe goes off it splats errors all over the console which
makes it look like something is going wrong even if everything is fine
in the end.  If lots of deferred probing happens then the volume gets
big too.  People find this distracting, noisy and ugly - it obscures
actual issues and trains people to ignore errors.  I do think this is a
reasonable concern and that it's worth trying to mitigate against
deferral for this reason alone.  We don't want to just ignore the errors
and not print anything either since if the resource doesn't appear the
user needs to know what is preventing the driver from instantiating so
they can try to fix it.

The other is that if you're printing to a serial console then that's not
an especially fast operation so if you're getting lots of messages being
printed simply physically outputting them takes measurable time.  I'm
not aware of any performance concerns outside of that, but like I say
I'm not affected by this myself in any great way.  Obviously this can be
configured but not having actual errors on the console isn't super
awesome either for systems that make use of the logging there and we
don't have a good way of telling what's from deferral and what's not.


signature.asc
Description: PGP signature

Re: [GIT PULL] On-demand device probing

[Mark Brown]

On Wed, Oct 14, 2015 at 10:34:00AM +0200, Tomeu Vizoso wrote:

> git+ssh://git.collabora.co.uk/git/user/tomeu/linux.git
> on-demand-probes-for-next

In don't think that's the URL you intended to use (also everything looks
word wrapped here)?


signature.asc
Description: PGP signature

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

On Fri, Oct 09, 2015 at 04:17:27PM -0500, Felipe Balbi wrote:

> Mark, when can we try to have a discussion about how to get this
> upstream ? It seems like designing everything in the mailing list will
> just take forever. Any ideas ?

Can we take this off-list?  I'm in the UK, Baolin is in China and I
believe you're in the US so it might be a fun one to schedule...


signature.asc
Description: Digital signature

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

On Mon, Oct 05, 2015 at 10:15:11AM -0500, Felipe Balbi wrote:
> On Sun, Oct 04, 2015 at 11:55:06PM +0100, Mark Brown wrote:

> > The trouble is getting traction on adoption.  Vendors have a habit of doing
> > things like finding problems and rather than reporting them deciding that 
> > the
> > open source solution isn't great and going and writing their own thing
> > (especially when they're in stealth mode) rather than talking to anyone.
> > Sometimes we manage to avoid that but it can be challenging, and often we 
> > only
> > even hear about the new thing after it's shipping and there's a lot of 
> > inertia
> > behind changing it.  The kernel ABIs tend to be a lot sticker than userspace
> > things here.

> but this is not a solid enough argument to push anything into the kernel.

To my mind it is a concern when considering design - it's not the only
consideration certainly but it should influence if or how we split
things.

> > > the same thing will happen with Type-C and power delivery. There won't be 
> > > tuning
> > > to taste, of course :-) But there will be "very different ideas what what 
> > > you
> > > want to do with" your charging methodology.

> > Are those very different things about the use cases ("don't bother with
> > high speed data, get all the power you can" or whatever) or are they
> > about the details of the board?

> I'm pretty sure there will be interesting designs in the market. I'm pretty 
> sure
> there will be type-C systems which won't support USB 3.1 for example, even
> though they'll support USB Power Delivery in its entirety.

That's sounding like generic USB stuff to me.

> > Yes, definitely - my experience both in terms of watching how people
> > like handset vendors often work internally and in terms of getting
> > things adopted is that it's a lot easier if you can have one component
> > you need to update to handle new hardware rather than multiple ones that
> > need to be upgraded for things that are purely board differences.

> IMO, just because you have it in the kernel, it doesn't mean it'll be
> adopted. Look at pm_runtime, for example; it's a very nice API and, yet, not
> used by Android (or has that changed ?)

That's always been a bit of a myth - most Android systems use runtime PM
extensively when they're running, the discussion is really about the
edge case where you're idling the system.  The Android suspend behaviour
is about the system idle case and is as much about providing a simple
way to go into an idle state, especially bearing in mind that they have
apps installed from the app store there, as anything else.  It's the
making sure things are idle part of things that's different.

> > > okay, this is a fair point :-) I just don't see, as of now at least, how 
> > > we can
> > > get to that in a way that's somewhat future-proof. It seems like we will
> > > add more and more notifications until we can't maintain this anymore.

> > So we need to look at the new/upcoming USB specs and understand the

> not upcoming, it's already out there.

I assume there's ongoing work on further revisions to the spec though?

> > Yup, which is a really cool feature and keeps us all employed too! :)
> > This is one reason for attaching things to the USB stack, right now it
> > does a limited negotiation but in future like you say there's more and
> > more features coming.

> keep in mind that the same stack used to change a charging current, will also 
> be
> used to tell the other end to mux those lines differently.

Sure.

> > Does the problem not get easier if we break it down to just the charger
> > elements and realise that the USB C modes are going to have a lot more
> > policy in them?

> the thing with USB C is that it's not only for negotiating a charging power
> (with power delivery you're not necessarily tied to 5V, btw), that very stack
> will also be used to change to other alternate modes, and those can be 
> anything:
> Thunderbolt, PCIe, DisplayPort, etc.

Surely these features have sufficient orthogonality to allow us to split
things up and deal with some of the problems while providing spaces for
future work?


signature.asc
Description: Digital signature

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

On Fri, Oct 02, 2015 at 02:11:25PM -0500, Felipe Balbi wrote:
> On Fri, Oct 02, 2015 at 07:49:09PM +0100, Mark Brown wrote:
> > On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:

> > > > Things more difficult, if nothing else it means we need to get whatever
> > > > userspace component deployed in all relevant userspace stacks with
> > > > appropriate configuration in order to do things.

> > > but that will be a thing for the kernel too. We will have to deploy this 
> > > new
> > > kernel component in all relevant stacks with appropriate configuration in 
> > > order
> > > to do things :-) No changes there.

> > Sure, but it's one project which is developed entirely in the open -
> > that's a lot more manageable.

> We can have a fully open source userland daemon too. Much like systemd, bluez,
> neard, and many, many others. Why wouldn't that be a thing ?

The trouble is getting traction on adoption.  Vendors have a habit of
doing things like finding problems and rather than reporting them
deciding that the open source solution isn't great and going and writing
their own thing (especially when they're in stealth mode) rather than
talking to anyone.  Sometimes we manage to avoid that but it can be
challenging, and often we only even hear about the new thing after it's
shipping and there's a lot of inertia behind changing it.  The kernel
ABIs tend to be a lot sticker than userspace things here.

> Similar applies to power delivery charging profile themselves. Not all 
> profiles
> are mandatory. Some are optional and that will be completely device/board
> specific. How an OEM implements that in the PCB is also completely
> board-specific :-) Some might have a dumb IC hardcoding some messages, some
> might have something largely more flexible.

Right, and what I was trying to say was that it seems like the kernel
ought to be worrying about the board specific bits and userspace
worrying about what to do with those bits.

> > The things we've got with audio are a combination of tuning to taste and
> > (even more importantly) very different ideas about what you want to do
> > with an audio subsystem that influence how you set things up in a given
> > use case.

> the same thing will happen with Type-C and power delivery. There won't be 
> tuning
> to taste, of course :-) But there will be "very different ideas what what you
> want to do with" your charging methodology.

Are those very different things about the use cases ("don't bother with
high speed data, get all the power you can" or whatever) or are they
about the details of the board?

> > > Actually, I was thinking about it in a more granular fashion. Kernel 
> > > exposes a
> > > charger/power_supply, a few regulators, a UDC view and this single 
> > > userspace
> > > daemon figures out how to tie those together.

> > That sounds worrying - it means that new hardware support needs
> > supporting in every userspace (it seems inevitable that there will be at
> > least some reimplementations because that's fun) which gets old really
> > fast, and every userspace needs to understand every topology.

> very true, but it's also true for a kernel solution.

> It seems like you want it in the kernel because of it being convenient :-)

Yes, definitely - my experience both in terms of watching how people
like handset vendors often work internally and in terms of getting
things adopted is that it's a lot easier if you can have one component
you need to update to handle new hardware rather than multiple ones that
need to be upgraded for things that are purely board differences.

> > > The view here isn't really a USB port, it's just a source of power. But 
> > > how much
> > > power we can draw from it depends on, possibly, a ton of different chips 
> > > and
> > > different notifications.

> > Right, yes - it's the power input/output associated with the USB port.
> > The USB port is just the physical thing users can see so it's a good way
> > to label it.  That could mean that we ought to move the aggregation bit
> > into the power supply code of course, but then a lot of the decisions
> > are going to be specific to USB.

> in a sense, yes. But they can happen at a layer which knows nothing (or very
> little) about USB. Here's an example:

> Not everybody follows USB Battery Charging class. Some chargers don't short
> D+/D- to signify they are a wall charger. Some will use different resistance
> values on the ID pin to tell you how much current you can draw from them.

> From a PMIC (or something else) point of view, all it needs is a tiny current
> source and a an ADC, then it reports the AD

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

On Thu, Oct 01, 2015 at 12:58:49PM -0500, Felipe Balbi wrote:
> On Thu, Oct 01, 2015 at 06:43:08PM +0100, Mark Brown wrote:
> > On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:

> > > Frankly, I wanted all of this to be decided in userland with the
> > > kernel just providing notification and basic safety checks (we don't
> > > want to allow a bogus userspace daemon frying anybody's devices).

> > What's the advantage of pushing this to userspace?  By the time we
> > provide enough discoverability to enable userspace to configure itself
> > it seems like we'd have enough information to do the job anyway.


> you're going to be dealing with a setup where each vendor does one thing
> differently. Some will have it all in the SoC part of a single IP (dwc3 can be
> configured that way), some will push it out to companion IC, some might even 
> use
> some mostly discrete setup and so on...

Right, and that was exactly what this was supposed to be all about when
I was discussing this originally with Baolin (who's on holiday until
sometime next week BTW, hence me answering).

> We're gonna be dealing with a decision that involves information from multiple
> subsystems (USB, regulator, hwmon, power supply to name a few).

Sure, that was part of the idea here - provide a central point
representing the logical port where we can aggregate all the information
we get in and distribute it to consumers.  

> We tried doing it all in the kernel back in N800, N810 and N900/N9 days and 
> it's
> just plain difficult. To make matters worse, N900 had two USB PHYs, one for
> actual runtime use and another just for detecting the charger type on the 
> other
> end.

Can you elaborate on what the difficulties are and how punting to
userspace helps?  If anything I'd expect punting to userspace to make
things more difficult, if nothing else it means we need to get whatever
userspace component deployed in all relevant userspace stacks with
appropriate configuration in order to do things.  

We do punt a lot of configuration to userspace for audio because there
are substantial device specific policy elements in the configuration and
it's a far from painless experience getting the code and configuration
deployed where people need it, definitely a not great for users.

> On top of all that, we still need to figure out what to do when a particular
> configuration gets chosen, and what to do when the bus goes into the different
> suspend levels.

> It's going to be a lot of policy getting added to the kernel. On top of all
> that, when Type C and power delivery is finally a thing, there will an entire
> new stack for USB C commands (using the Type C CC pins or modulated on VBUS 
> for
> legacy connectors) which we will have to add to the kernel. And different
> devices will support different charging profiles (there are at least 6 of 
> those,
> IIRC).

Yes, USB C was part of the thinking with starting this work - it's going
to make ensuring that the system is paying attention to limits much more
important.  I think there's two things here.  One is working out how the
system is glued together and the other thing is working out what to do
with that system.  

I think that where we can do it the bit where work out how the various
components are connected and aggregate their functionality together is
definitely a kernel job.  It means that userspace doesn't need to worry
about implementation details of the particular system and instead gets a
consistent, standard view of the device (in this case a USB port and how
much power we can draw from it).

For the policy stuff we do want to have userspace be able to control
things but we need to think about how to do that - for example does the
entire flow need to be in userspace, or can we just expose settings
which userspace can manage?

> With all these different things going on, it's best to do what e.g. NFC folks
> did. Just a small set of hooks in the kernel, but actual implementation done 
> by
> a userspace daemon. This makes it even easier for middleware development since
> we can provide a higher level API for middleware to talk to the charging 
> daemon.

I'm not sure that the NFC model is working well for everyone - it's OK
if you happen to be running Android but if you aren't you're often left
sitting there working out what to do with an Android HAL.  We can do the
middleware thing without going quite that far, we do already have power
management daemons in userspace even on desktop (GNOME has upowerd for
example).

> Another benefit is that this charging daemon can also give hints to power
> management policy that e.g. we're charging at 10W or 100W and we can e.g. 
> switch
> to performance governor safely even though battery is rather low.

We definitely want userspace to be able to see the current state, even
if 

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

On Fri, Oct 02, 2015 at 12:23:11PM -0500, Felipe Balbi wrote:
> On Fri, Oct 02, 2015 at 05:47:43PM +0100, Mark Brown wrote:

> > Can you elaborate on what the difficulties are and how punting to
> > userspace helps?  If anything I'd expect punting to userspace to make

> the difficulty was mainly making sure all involved parties talk the same
> language. I mean, you plug cable and detect charger (this is usually done by 
> the
> PMIC or by a BQ-type device - probably done at drivers/power_supply).

> First difficulty comes right here. Power_supply notifies that we're attached 
> to
> a charger (sometimes it can't differentiate a host/hub charger from a wall
> charger), a few ms later you get a notification from the gadget that it got
> enumerated with a 500mA configuration. Depending on the order of things, your
> load will be configured either to 2A (maximum host/hub charger current) or
> 500mA. Yes, this can be mitigated :-)

> Focussing on this alone, you can have as much as 4 different subsystems
> involved, all throwing raw_notifiers at each other. Now each of these 
> subsystems
> need to maintain their own state machine about what notification we have
> received and what are the valid next states.

OK, so part of the goal here was to outsource all the state machines to
some generic code - what you're describing here is definitely a problem
and the idea was to provide a central place that has the logic and makes
the decisions about what we should be doing.  If we don't have that it's
going to get messy.

> I would rather have userspace be the single place getting notifications 
> because
> then we solve these details at a single location.

No argument on the place, making that place be userspace I'm less sold
on.

> > Things more difficult, if nothing else it means we need to get whatever
> > userspace component deployed in all relevant userspace stacks with
> > appropriate configuration in order to do things.

> but that will be a thing for the kernel too. We will have to deploy this new
> kernel component in all relevant stacks with appropriate configuration in 
> order
> to do things :-) No changes there.

Sure, but it's one project which is developed entirely in the open -
that's a lot more manageable.

> > We do punt a lot of configuration to userspace for audio because there
> > are substantial device specific policy elements in the configuration and
> > it's a far from painless experience getting the code and configuration
> > deployed where people need it, definitely a not great for users.

> it's the same for this situation. We will have a ton of device specific
> configuration, specially with power delivery class, billboard class, the
> alternate modes and so on. If we already do this part in userland, adding all
> those extras will be, IMO, far simpler.

Can you elaborate a bit?  As far as I can tell all those are still in
the generic USB space rather than the sort of device specifics I'm
thinking of.

The things we've got with audio are a combination of tuning to taste and
(even more importantly) very different ideas about what you want to do
with an audio subsystem that influence how you set things up in a given
use case.

> > I think that where we can do it the bit where work out how the various
> > components are connected and aggregate their functionality together is
> > definitely a kernel job.  It means that userspace doesn't need to worry
> > about implementation details of the particular system and instead gets a
> > consistent, standard view of the device (in this case a USB port and how
> > much power we can draw from it).

> Actually, I was thinking about it in a more granular fashion. Kernel exposes a
> charger/power_supply, a few regulators, a UDC view and this single userspace
> daemon figures out how to tie those together.

That sounds worrying - it means that new hardware support needs
supporting in every userspace (it seems inevitable that there will be at
least some reimplementations because that's fun) which gets old really
fast, and every userspace needs to understand every topology.

> The view here isn't really a USB port, it's just a source of power. But how 
> much
> power we can draw from it depends on, possibly, a ton of different chips and
> different notifications.

Right, yes - it's the power input/output associated with the USB port.
The USB port is just the physical thing users can see so it's a good way
to label it.  That could mean that we ought to move the aggregation bit
into the power supply code of course, but then a lot of the decisions
are going to be specific to USB.

> > For the policy stuff we do want to have userspace be able to control
> > things but we need to think about how to do that - for example does the
> > entire flow need to b

Re: [PATCH v4 1/5] gadget: Introduce the notifier functions

[Mark Brown]

On Thu, Oct 01, 2015 at 12:29:32PM -0500, Felipe Balbi wrote:

> Frankly, I wanted all of this to be decided in userland with the
> kernel just providing notification and basic safety checks (we don't
> want to allow a bogus userspace daemon frying anybody's devices).

What's the advantage of pushing this to userspace?  By the time we
provide enough discoverability to enable userspace to configure itself
it seems like we'd have enough information to do the job anyway.


signature.asc
Description: Digital signature

Re: [PATCH v4 5/5] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Wed, Aug 19, 2015 at 05:13:48PM +0800, Baolin Wang wrote:
 Integrate with the newly added USB charger interface to limit the current
 we draw from the USB input based on the input device configuration
 identified by the USB stack, allowing us to charge more quickly from high
 current inputs without drawing more current than specified from others.
 
 Signed-off-by: Mark Brown broo...@kernel.org
 Signed-off-by: Baolin Wang baolin.w...@linaro.org

When people (like Charles and Lee have) have reviewed a change you
should add any tags they gave when you resend the change so they don't
have to duplicate their work and other people know that the review has
happened.


signature.asc
Description: Digital signature

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Wed, Aug 19, 2015 at 08:02:37AM +0800, Peter Chen wrote:

 Below code may be correct for the goal you expressed.

for (i = 0; i  ARRAY_SIZE(wm831x_usb_limits); i++) {
if (limit = wm831x_usb_limits[i] 
wm831x_usb_limits[best]  wm831x_usb_limits[i])
best = i;
}

Yes, that's right.


signature.asc
Description: Digital signature

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Tue, Aug 18, 2015 at 01:20:12PM +0800, Peter Chen wrote:

 ok, I just had suspected below function's correctness, after looking
 it again, it always set 1800 as charging limit, does it be expected?

 +   /* Find the highest supported limit */
 +   best = 0;
 +   for (i = 0; i  ARRAY_SIZE(wm831x_usb_limits); i++) {
 +   if (limit  wm831x_usb_limits[i] 

The above check is intended to ensure that we don't go over the limit
that was passed in in the callback.  The goal is to select the highest
option that is less than the limit passed in.


signature.asc
Description: Digital signature

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Mon, Aug 17, 2015 at 06:58:16PM -0500, Felipe Balbi wrote:
 On Mon, Aug 17, 2015 at 10:26:23AM -0700, Mark Brown wrote:
  On Mon, Aug 17, 2015 at 09:07:08AM +0800, Peter Chen wrote:
   On Fri, Aug 14, 2015 at 05:47:46PM +0800, Baolin Wang wrote:

+   if (wm831x_pdata  wm831x_pdata-usb_gadget) {

   Where the wm831x_pdata-usb_gadget is initialized?

  It's platform data, it will be initialised by whatever registers the
  platform data.

 passing pointers through pdata ?

It's a char * to a name, not a pointer to struct.


signature.asc
Description: Digital signature

Re: [PATCH v2 3/3] power: wm831x_power: Support USB charger current limit management

[Mark Brown]

On Mon, Aug 17, 2015 at 09:07:08AM +0800, Peter Chen wrote:
 On Fri, Aug 14, 2015 at 05:47:46PM +0800, Baolin Wang wrote:

  +   1500,
  +   1800,
  +   550,
  +};

 Why 550 is the last, but not 1800?

You'd have to ask the hardware engineers who designed the chip.  I
suspect it's because 550 was added at a late stage in the design process
but I'm basically just guessing there.

*/
  @@ -606,8 +646,31 @@ static int wm831x_power_probe(struct platform_device 
  *pdev)
  }
  }

  +   if (wm831x_pdata  wm831x_pdata-usb_gadget) {

 Where the wm831x_pdata-usb_gadget is initialized?

It's platform data, it will be initialised by whatever registers the
platform data.


signature.asc
Description: Digital signature